From 39741dd0268c336c395551d345c8233f1e5825d1 Mon Sep 17 00:00:00 2001 From: igen nova Date: Sat, 21 Feb 2026 02:17:07 +0530 Subject: [PATCH 1/8] migration of /setup --- src/database/setups.py | 42 +++++++++++++ src/main.py | 2 + src/routers/openml/setups.py | 55 ++++++++++++++++ .../openml/migration/setups_migration_test.py | 62 +++++++++++++++++++ 4 files changed, 161 insertions(+) create mode 100644 src/database/setups.py create mode 100644 src/routers/openml/setups.py create mode 100644 tests/routers/openml/migration/setups_migration_test.py diff --git a/src/database/setups.py b/src/database/setups.py new file mode 100644 index 00000000..59601ae4 --- /dev/null +++ b/src/database/setups.py @@ -0,0 +1,42 @@ +from sqlalchemy import Connection, text +from sqlalchemy.engine import Row + + +def get(setup_id: int, connection: Connection) -> Row | None: + row = connection.execute( + text( + """ + SELECT * + FROM algorithm_setup + WHERE sid = :setup_id + """, + ), + parameters={"setup_id": setup_id}, + ) + return row.first() + + +def get_tags(setup_id: int, connection: Connection) -> list[Row]: + rows = connection.execute( + text( + """ + SELECT * + FROM setup_tag + WHERE id = :setup_id + """, + ), + parameters={"setup_id": setup_id}, + ) + return list(rows.all()) + + +def untag(setup_id: int, tag: str, connection: Connection) -> None: + connection.execute( + text( + """ + DELETE FROM setup_tag + WHERE id = :setup_id AND tag = :tag + """, + ), + parameters={"setup_id": setup_id, "tag": tag}, + ) diff --git a/src/main.py b/src/main.py index 560b4c50..07e14510 100644 --- a/src/main.py +++ b/src/main.py @@ -11,6 +11,7 @@ from routers.openml.evaluations import router as evaluationmeasures_router from routers.openml.flows import router as flows_router from routers.openml.qualities import router as qualities_router +from routers.openml.setups import router as setup_router from routers.openml.study import router as study_router from routers.openml.tasks import router as task_router from routers.openml.tasktype import router as ttype_router @@ -55,6 +56,7 @@ def create_api() -> FastAPI: app.include_router(task_router) app.include_router(flows_router) app.include_router(study_router) + app.include_router(setup_router) return app diff --git a/src/routers/openml/setups.py b/src/routers/openml/setups.py new file mode 100644 index 00000000..24ddc5e8 --- /dev/null +++ b/src/routers/openml/setups.py @@ -0,0 +1,55 @@ +from http import HTTPStatus +from typing import Annotated + +from fastapi import APIRouter, Body, Depends, HTTPException +from sqlalchemy import Connection + +import database.setups +from database.users import User, UserGroup +from routers.dependencies import expdb_connection, fetch_user +from routers.types import SystemString64 + +router = APIRouter(prefix="/setup", tags=["setup"]) + + +def create_authentication_failed_error() -> HTTPException: + return HTTPException( + status_code=HTTPStatus.PRECONDITION_FAILED, + detail={"code": "103", "message": "Authentication failed"}, + ) + + +@router.post(path="/untag") +def untag_setup( + setup_id: Annotated[int, Body()], + tag: Annotated[str, SystemString64], + user: Annotated[User | None, Depends(fetch_user)] = None, + expdb_db: Annotated[Connection, Depends(expdb_connection)] = None, +) -> dict[str, dict[str, str]]: + if user is None: + raise create_authentication_failed_error() + + if not database.setups.get(setup_id, expdb_db): + raise HTTPException( + status_code=HTTPStatus.PRECONDITION_FAILED, + detail={"code": "472", "message": "Entity not found."}, + ) + + setup_tags = database.setups.get_tags(setup_id, expdb_db) + matched_tag_row = next((t for t in setup_tags if t.tag.casefold() == tag.casefold()), None) + + if not matched_tag_row: + raise HTTPException( + status_code=HTTPStatus.PRECONDITION_FAILED, + detail={"code": "475", "message": "Tag not found."}, + ) + + if matched_tag_row.uploader != user.user_id and UserGroup.ADMIN not in user.groups: + raise HTTPException( + status_code=HTTPStatus.PRECONDITION_FAILED, + detail={"code": "476", "message": "Tag is not owned by you"}, + ) + + database.setups.untag(setup_id, matched_tag_row.tag, expdb_db) + + return {"setup_untag": {"id": str(setup_id)}} diff --git a/tests/routers/openml/migration/setups_migration_test.py b/tests/routers/openml/migration/setups_migration_test.py new file mode 100644 index 00000000..a744dc39 --- /dev/null +++ b/tests/routers/openml/migration/setups_migration_test.py @@ -0,0 +1,62 @@ +from http import HTTPStatus + +import httpx +import pytest +from starlette.testclient import TestClient + +from tests.users import ApiKey + + +@pytest.mark.parametrize( + "setup_id", + [1, 999999], + ids=["existing setup", "unknown setup"], +) +@pytest.mark.parametrize( + "api_key", + [ApiKey.ADMIN, ApiKey.SOME_USER, ApiKey.OWNER_USER], + ids=["Administrator", "regular user", "possible owner"], +) +@pytest.mark.parametrize( + "tag", + ["totally_new_tag_for_migration_testing"], +) +def test_setup_untag_response_is_identical( + setup_id: int, + tag: str, + api_key: str, + py_api: TestClient, + php_api: httpx.Client, +) -> None: + if setup_id == 1: + php_api.post( + "/setup/tag", + data={"api_key": ApiKey.SOME_USER, "tag": tag, "setup_id": setup_id}, + ) + + original = php_api.post( + "/setup/untag", + data={"api_key": api_key, "tag": tag, "setup_id": setup_id}, + ) + + if original.status_code == HTTPStatus.OK: + php_api.post( + "/setup/tag", + data={"api_key": ApiKey.SOME_USER, "tag": tag, "setup_id": setup_id}, + ) + + new = py_api.post( + f"/setup/untag?api_key={api_key}", + json={"setup_id": setup_id, "tag": tag}, + ) + + assert original.status_code == new.status_code + + if new.status_code != HTTPStatus.OK: + assert original.json()["error"] == new.json()["detail"] + return + + original_json = original.json() + new_json = new.json() + + assert original_json == new_json From d41b9ce0787aa538996a0ee85ac4601843edbfa5 Mon Sep 17 00:00:00 2001 From: igennova Date: Fri, 27 Feb 2026 18:12:55 +0530 Subject: [PATCH 2/8] Add setups_test.py and refactor fetch_user to use fetch_user_or_raise --- src/routers/dependencies.py | 14 ++++- src/routers/openml/datasets.py | 20 +++---- src/routers/openml/setups.py | 14 +---- tests/routers/openml/setups_test.py | 88 +++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 25 deletions(-) create mode 100644 tests/routers/openml/setups_test.py diff --git a/src/routers/dependencies.py b/src/routers/dependencies.py index 2ddccf83..e4de107f 100644 --- a/src/routers/dependencies.py +++ b/src/routers/dependencies.py @@ -1,6 +1,7 @@ +from http import HTTPStatus from typing import Annotated -from fastapi import Depends +from fastapi import Depends, HTTPException from pydantic import BaseModel from sqlalchemy import Connection @@ -29,6 +30,17 @@ def fetch_user( return User.fetch(api_key, user_data) if api_key else None +def fetch_user_or_raise( + user: Annotated[User | None, Depends(fetch_user)] = None, +) -> User: + if user is None: + raise HTTPException( + status_code=HTTPStatus.PRECONDITION_FAILED, + detail={"code": "103", "message": "Authentication failed"}, + ) + return user + + class Pagination(BaseModel): offset: int = 0 limit: int = 100 diff --git a/src/routers/openml/datasets.py b/src/routers/openml/datasets.py index dda25117..2bc1548d 100644 --- a/src/routers/openml/datasets.py +++ b/src/routers/openml/datasets.py @@ -19,7 +19,13 @@ _format_parquet_url, ) from database.users import User, UserGroup -from routers.dependencies import Pagination, expdb_connection, fetch_user, userdb_connection +from routers.dependencies import ( + Pagination, + expdb_connection, + fetch_user, + fetch_user_or_raise, + userdb_connection, +) from routers.types import CasualString128, IntegerRange, SystemString64, integer_range_regex from schemas.datasets.openml import DatasetMetadata, DatasetStatus, Feature, FeatureType @@ -32,29 +38,19 @@ def tag_dataset( data_id: Annotated[int, Body()], tag: Annotated[str, SystemString64], - user: Annotated[User | None, Depends(fetch_user)] = None, + user: Annotated[User, Depends(fetch_user_or_raise)], expdb_db: Annotated[Connection, Depends(expdb_connection)] = None, ) -> dict[str, dict[str, Any]]: tags = database.datasets.get_tags_for(data_id, expdb_db) if tag.casefold() in [t.casefold() for t in tags]: raise create_tag_exists_error(data_id, tag) - if user is None: - raise create_authentication_failed_error() - database.datasets.tag(data_id, tag, user_id=user.user_id, connection=expdb_db) return { "data_tag": {"id": str(data_id), "tag": [*tags, tag]}, } -def create_authentication_failed_error() -> HTTPException: - return HTTPException( - status_code=HTTPStatus.PRECONDITION_FAILED, - detail={"code": "103", "message": "Authentication failed"}, - ) - - def create_tag_exists_error(data_id: int, tag: str) -> HTTPException: return HTTPException( status_code=HTTPStatus.INTERNAL_SERVER_ERROR, diff --git a/src/routers/openml/setups.py b/src/routers/openml/setups.py index 24ddc5e8..71304e38 100644 --- a/src/routers/openml/setups.py +++ b/src/routers/openml/setups.py @@ -6,29 +6,19 @@ import database.setups from database.users import User, UserGroup -from routers.dependencies import expdb_connection, fetch_user +from routers.dependencies import expdb_connection, fetch_user_or_raise from routers.types import SystemString64 router = APIRouter(prefix="/setup", tags=["setup"]) -def create_authentication_failed_error() -> HTTPException: - return HTTPException( - status_code=HTTPStatus.PRECONDITION_FAILED, - detail={"code": "103", "message": "Authentication failed"}, - ) - - @router.post(path="/untag") def untag_setup( setup_id: Annotated[int, Body()], tag: Annotated[str, SystemString64], - user: Annotated[User | None, Depends(fetch_user)] = None, + user: Annotated[User, Depends(fetch_user_or_raise)], expdb_db: Annotated[Connection, Depends(expdb_connection)] = None, ) -> dict[str, dict[str, str]]: - if user is None: - raise create_authentication_failed_error() - if not database.setups.get(setup_id, expdb_db): raise HTTPException( status_code=HTTPStatus.PRECONDITION_FAILED, diff --git a/tests/routers/openml/setups_test.py b/tests/routers/openml/setups_test.py new file mode 100644 index 00000000..50b6b843 --- /dev/null +++ b/tests/routers/openml/setups_test.py @@ -0,0 +1,88 @@ +from collections.abc import Iterator +from http import HTTPStatus + +import pytest +from sqlalchemy import Connection, text +from starlette.testclient import TestClient + +from tests.users import ApiKey + + +@pytest.fixture +def mock_setup_tag(expdb_test: Connection) -> Iterator[None]: + expdb_test.execute( + text("DELETE FROM setup_tag WHERE id = 1 AND tag = 'test_unit_tag_123'"), + ) + expdb_test.execute( + text("INSERT INTO setup_tag (id, tag, uploader) VALUES (1, 'test_unit_tag_123', 2)") + ) + expdb_test.commit() + + yield + + expdb_test.execute( + text("DELETE FROM setup_tag WHERE id = 1 AND tag = 'test_unit_tag_123'"), + ) + expdb_test.commit() + + +def test_setup_untag_missing_auth(py_api: TestClient) -> None: + response = py_api.post("/setup/untag", json={"setup_id": 1, "tag": "test_tag"}) + assert response.status_code == HTTPStatus.PRECONDITION_FAILED + assert response.json()["detail"] == {"code": "103", "message": "Authentication failed"} + + +def test_setup_untag_unknown_setup(py_api: TestClient) -> None: + response = py_api.post( + f"/setup/untag?api_key={ApiKey.SOME_USER}", + json={"setup_id": 999999, "tag": "test_tag"}, + ) + assert response.status_code == HTTPStatus.PRECONDITION_FAILED + assert response.json()["detail"] == {"code": "472", "message": "Entity not found."} + + +def test_setup_untag_tag_not_found(py_api: TestClient) -> None: + response = py_api.post( + f"/setup/untag?api_key={ApiKey.SOME_USER}", + json={"setup_id": 1, "tag": "non_existent_tag_12345"}, + ) + assert response.status_code == HTTPStatus.PRECONDITION_FAILED + assert response.json()["detail"] == {"code": "475", "message": "Tag not found."} + + +@pytest.mark.mut +@pytest.mark.usefixtures("mock_setup_tag") +def test_setup_untag_not_owned_by_you(py_api: TestClient) -> None: + response = py_api.post( + f"/setup/untag?api_key={ApiKey.OWNER_USER}", + json={"setup_id": 1, "tag": "test_unit_tag_123"}, + ) + assert response.status_code == HTTPStatus.PRECONDITION_FAILED + assert response.json()["detail"] == {"code": "476", "message": "Tag is not owned by you"} + + +@pytest.mark.mut +@pytest.mark.parametrize( + "api_key", + [ApiKey.SOME_USER, ApiKey.ADMIN], + ids=["Owner", "Administrator"], +) +def test_setup_untag_success(api_key: str, py_api: TestClient, expdb_test: Connection) -> None: + expdb_test.execute(text("DELETE FROM setup_tag WHERE id = 1 AND tag = 'test_success_tag'")) + expdb_test.execute( + text("INSERT INTO setup_tag (id, tag, uploader) VALUES (1, 'test_success_tag', 2)") + ) + expdb_test.commit() + + response = py_api.post( + f"/setup/untag?api_key={api_key}", + json={"setup_id": 1, "tag": "test_success_tag"}, + ) + + assert response.status_code == HTTPStatus.OK + assert response.json() == {"setup_untag": {"id": "1"}} + + rows = expdb_test.execute( + text("SELECT * FROM setup_tag WHERE id = 1 AND tag = 'test_success_tag'") + ).all() + assert len(rows) == 0 From b63794dc680db78a3a07632ab267555837e2d483 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 11 Mar 2026 15:17:25 +0000 Subject: [PATCH 3/8] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/routers/dependencies.py | 2 +- src/routers/openml/datasets.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/routers/dependencies.py b/src/routers/dependencies.py index 0eb81f24..5d4021d8 100644 --- a/src/routers/dependencies.py +++ b/src/routers/dependencies.py @@ -1,5 +1,5 @@ -from http import HTTPStatus from collections.abc import AsyncGenerator +from http import HTTPStatus from typing import Annotated from fastapi import Depends, HTTPException diff --git a/src/routers/openml/datasets.py b/src/routers/openml/datasets.py index e829dc5e..d86ed848 100644 --- a/src/routers/openml/datasets.py +++ b/src/routers/openml/datasets.py @@ -12,7 +12,6 @@ import database.qualities from core.access import _user_has_access from core.errors import ( - AuthenticationFailedError, AuthenticationRequiredError, DatasetAdminOnlyError, DatasetNoAccessError, From 78fb0617d5869b60d6fc68e55ccc31a206dae412 Mon Sep 17 00:00:00 2001 From: PGijsbers Date: Wed, 11 Mar 2026 17:01:55 +0100 Subject: [PATCH 4/8] Update PR to use async, add docstrings, partial RFC updates --- src/database/setups.py | 20 ++++--- src/routers/dependencies.py | 10 ++-- src/routers/openml/setups.py | 17 +++--- tests/routers/openml/dataset_tag_test.py | 2 +- .../openml/migration/setups_migration_test.py | 15 +++-- tests/routers/openml/setups_test.py | 60 ++++++++++--------- 6 files changed, 68 insertions(+), 56 deletions(-) diff --git a/src/database/setups.py b/src/database/setups.py index 59601ae4..866a7de3 100644 --- a/src/database/setups.py +++ b/src/database/setups.py @@ -1,9 +1,13 @@ -from sqlalchemy import Connection, text +"""All database operations that directly operate on setups.""" + +from sqlalchemy import text from sqlalchemy.engine import Row +from sqlalchemy.ext.asyncio import AsyncConnection -def get(setup_id: int, connection: Connection) -> Row | None: - row = connection.execute( +async def get(setup_id: int, connection: AsyncConnection) -> Row | None: + """Get the setup with id `setup_id` from the database.""" + row = await connection.execute( text( """ SELECT * @@ -16,8 +20,9 @@ def get(setup_id: int, connection: Connection) -> Row | None: return row.first() -def get_tags(setup_id: int, connection: Connection) -> list[Row]: - rows = connection.execute( +async def get_tags(setup_id: int, connection: AsyncConnection) -> list[Row]: + """Get all tags for setup with `setup_id` from the database.""" + rows = await connection.execute( text( """ SELECT * @@ -30,8 +35,9 @@ def get_tags(setup_id: int, connection: Connection) -> list[Row]: return list(rows.all()) -def untag(setup_id: int, tag: str, connection: Connection) -> None: - connection.execute( +async def untag(setup_id: int, tag: str, connection: AsyncConnection) -> None: + """Remove tag `tag` from setup with id `setup_id`.""" + await connection.execute( text( """ DELETE FROM setup_tag diff --git a/src/routers/dependencies.py b/src/routers/dependencies.py index 5d4021d8..590ae36b 100644 --- a/src/routers/dependencies.py +++ b/src/routers/dependencies.py @@ -1,11 +1,11 @@ from collections.abc import AsyncGenerator -from http import HTTPStatus from typing import Annotated -from fastapi import Depends, HTTPException +from fastapi import Depends from pydantic import BaseModel from sqlalchemy.ext.asyncio import AsyncConnection +from core.errors import AuthenticationFailedError from database.setup import expdb_database, user_database from database.users import APIKey, User @@ -33,10 +33,8 @@ def fetch_user_or_raise( user: Annotated[User | None, Depends(fetch_user)] = None, ) -> User: if user is None: - raise HTTPException( - status_code=HTTPStatus.PRECONDITION_FAILED, - detail={"code": "103", "message": "Authentication failed"}, - ) + msg = "Authentication failed" + raise AuthenticationFailedError(msg) return user diff --git a/src/routers/openml/setups.py b/src/routers/openml/setups.py index 71304e38..eb7005c8 100644 --- a/src/routers/openml/setups.py +++ b/src/routers/openml/setups.py @@ -1,8 +1,10 @@ +"""All endpoints that relate to setups.""" + from http import HTTPStatus from typing import Annotated from fastapi import APIRouter, Body, Depends, HTTPException -from sqlalchemy import Connection +from sqlalchemy.ext.asyncio import AsyncConnection import database.setups from database.users import User, UserGroup @@ -13,19 +15,20 @@ @router.post(path="/untag") -def untag_setup( +async def untag_setup( setup_id: Annotated[int, Body()], tag: Annotated[str, SystemString64], user: Annotated[User, Depends(fetch_user_or_raise)], - expdb_db: Annotated[Connection, Depends(expdb_connection)] = None, + expdb_db: Annotated[AsyncConnection, Depends(expdb_connection)], ) -> dict[str, dict[str, str]]: - if not database.setups.get(setup_id, expdb_db): + """Remove tag `tag` from setup with id `setup_id`.""" + if not await database.setups.get(setup_id, expdb_db): raise HTTPException( status_code=HTTPStatus.PRECONDITION_FAILED, detail={"code": "472", "message": "Entity not found."}, ) - setup_tags = database.setups.get_tags(setup_id, expdb_db) + setup_tags = await database.setups.get_tags(setup_id, expdb_db) matched_tag_row = next((t for t in setup_tags if t.tag.casefold() == tag.casefold()), None) if not matched_tag_row: @@ -34,12 +37,12 @@ def untag_setup( detail={"code": "475", "message": "Tag not found."}, ) - if matched_tag_row.uploader != user.user_id and UserGroup.ADMIN not in user.groups: + if matched_tag_row.uploader != user.user_id and UserGroup.ADMIN not in await user.get_groups(): raise HTTPException( status_code=HTTPStatus.PRECONDITION_FAILED, detail={"code": "476", "message": "Tag is not owned by you"}, ) - database.setups.untag(setup_id, matched_tag_row.tag, expdb_db) + await database.setups.untag(setup_id, matched_tag_row.tag, expdb_db) return {"setup_untag": {"id": str(setup_id)}} diff --git a/tests/routers/openml/dataset_tag_test.py b/tests/routers/openml/dataset_tag_test.py index 646ac0c3..25042c89 100644 --- a/tests/routers/openml/dataset_tag_test.py +++ b/tests/routers/openml/dataset_tag_test.py @@ -83,7 +83,7 @@ async def test_dataset_tag_invalid_tag_is_rejected( py_api: httpx.AsyncClient, ) -> None: new = await py_api.post( - f"/datasets/tag?api_key{ApiKey.ADMIN}", + f"/datasets/tag?api_key={ApiKey.ADMIN}", json={"data_id": 1, "tag": tag}, ) diff --git a/tests/routers/openml/migration/setups_migration_test.py b/tests/routers/openml/migration/setups_migration_test.py index a744dc39..3351e69d 100644 --- a/tests/routers/openml/migration/setups_migration_test.py +++ b/tests/routers/openml/migration/setups_migration_test.py @@ -2,7 +2,6 @@ import httpx import pytest -from starlette.testclient import TestClient from tests.users import ApiKey @@ -21,31 +20,31 @@ "tag", ["totally_new_tag_for_migration_testing"], ) -def test_setup_untag_response_is_identical( +async def test_setup_untag_response_is_identical( setup_id: int, tag: str, api_key: str, - py_api: TestClient, - php_api: httpx.Client, + py_api: httpx.AsyncClient, + php_api: httpx.AsyncClient, ) -> None: if setup_id == 1: - php_api.post( + await php_api.post( "/setup/tag", data={"api_key": ApiKey.SOME_USER, "tag": tag, "setup_id": setup_id}, ) - original = php_api.post( + original = await php_api.post( "/setup/untag", data={"api_key": api_key, "tag": tag, "setup_id": setup_id}, ) if original.status_code == HTTPStatus.OK: - php_api.post( + await php_api.post( "/setup/tag", data={"api_key": ApiKey.SOME_USER, "tag": tag, "setup_id": setup_id}, ) - new = py_api.post( + new = await py_api.post( f"/setup/untag?api_key={api_key}", json={"setup_id": setup_id, "tag": tag}, ) diff --git a/tests/routers/openml/setups_test.py b/tests/routers/openml/setups_test.py index 50b6b843..fa246482 100644 --- a/tests/routers/openml/setups_test.py +++ b/tests/routers/openml/setups_test.py @@ -1,39 +1,41 @@ -from collections.abc import Iterator +from collections.abc import AsyncGenerator from http import HTTPStatus +import httpx import pytest -from sqlalchemy import Connection, text -from starlette.testclient import TestClient +from sqlalchemy import text +from sqlalchemy.ext.asyncio import AsyncConnection from tests.users import ApiKey @pytest.fixture -def mock_setup_tag(expdb_test: Connection) -> Iterator[None]: - expdb_test.execute( +async def mock_setup_tag(expdb_test: AsyncConnection) -> AsyncGenerator[None]: + await expdb_test.execute( text("DELETE FROM setup_tag WHERE id = 1 AND tag = 'test_unit_tag_123'"), ) - expdb_test.execute( + await expdb_test.execute( text("INSERT INTO setup_tag (id, tag, uploader) VALUES (1, 'test_unit_tag_123', 2)") ) - expdb_test.commit() + await expdb_test.commit() yield - expdb_test.execute( + await expdb_test.execute( text("DELETE FROM setup_tag WHERE id = 1 AND tag = 'test_unit_tag_123'"), ) - expdb_test.commit() + await expdb_test.commit() -def test_setup_untag_missing_auth(py_api: TestClient) -> None: - response = py_api.post("/setup/untag", json={"setup_id": 1, "tag": "test_tag"}) - assert response.status_code == HTTPStatus.PRECONDITION_FAILED - assert response.json()["detail"] == {"code": "103", "message": "Authentication failed"} +async def test_setup_untag_missing_auth(py_api: httpx.AsyncClient) -> None: + response = await py_api.post("/setup/untag", json={"setup_id": 1, "tag": "test_tag"}) + assert response.status_code == HTTPStatus.UNAUTHORIZED + assert response.json()["code"] == "103" + assert response.json()["detail"] == "Authentication failed" -def test_setup_untag_unknown_setup(py_api: TestClient) -> None: - response = py_api.post( +async def test_setup_untag_unknown_setup(py_api: httpx.AsyncClient) -> None: + response = await py_api.post( f"/setup/untag?api_key={ApiKey.SOME_USER}", json={"setup_id": 999999, "tag": "test_tag"}, ) @@ -41,8 +43,8 @@ def test_setup_untag_unknown_setup(py_api: TestClient) -> None: assert response.json()["detail"] == {"code": "472", "message": "Entity not found."} -def test_setup_untag_tag_not_found(py_api: TestClient) -> None: - response = py_api.post( +async def test_setup_untag_tag_not_found(py_api: httpx.AsyncClient) -> None: + response = await py_api.post( f"/setup/untag?api_key={ApiKey.SOME_USER}", json={"setup_id": 1, "tag": "non_existent_tag_12345"}, ) @@ -52,8 +54,8 @@ def test_setup_untag_tag_not_found(py_api: TestClient) -> None: @pytest.mark.mut @pytest.mark.usefixtures("mock_setup_tag") -def test_setup_untag_not_owned_by_you(py_api: TestClient) -> None: - response = py_api.post( +async def test_setup_untag_not_owned_by_you(py_api: httpx.AsyncClient) -> None: + response = await py_api.post( f"/setup/untag?api_key={ApiKey.OWNER_USER}", json={"setup_id": 1, "tag": "test_unit_tag_123"}, ) @@ -67,14 +69,18 @@ def test_setup_untag_not_owned_by_you(py_api: TestClient) -> None: [ApiKey.SOME_USER, ApiKey.ADMIN], ids=["Owner", "Administrator"], ) -def test_setup_untag_success(api_key: str, py_api: TestClient, expdb_test: Connection) -> None: - expdb_test.execute(text("DELETE FROM setup_tag WHERE id = 1 AND tag = 'test_success_tag'")) - expdb_test.execute( +async def test_setup_untag_success( + api_key: str, py_api: httpx.AsyncClient, expdb_test: AsyncConnection +) -> None: + await expdb_test.execute( + text("DELETE FROM setup_tag WHERE id = 1 AND tag = 'test_success_tag'") + ) + await expdb_test.execute( text("INSERT INTO setup_tag (id, tag, uploader) VALUES (1, 'test_success_tag', 2)") ) - expdb_test.commit() + await expdb_test.commit() - response = py_api.post( + response = await py_api.post( f"/setup/untag?api_key={api_key}", json={"setup_id": 1, "tag": "test_success_tag"}, ) @@ -82,7 +88,7 @@ def test_setup_untag_success(api_key: str, py_api: TestClient, expdb_test: Conne assert response.status_code == HTTPStatus.OK assert response.json() == {"setup_untag": {"id": "1"}} - rows = expdb_test.execute( + rows = await expdb_test.execute( text("SELECT * FROM setup_tag WHERE id = 1 AND tag = 'test_success_tag'") - ).all() - assert len(rows) == 0 + ) + assert len(rows.all()) == 0 From cea73333fcdb3471e5bcc0cf494aac656499d343 Mon Sep 17 00:00:00 2001 From: PGijsbers Date: Thu, 12 Mar 2026 14:10:03 +0100 Subject: [PATCH 5/8] Introduce RFC9457 responses, separate out test cases --- src/core/errors.py | 32 +++++ src/routers/openml/setups.py | 22 ++- .../openml/migration/setups_migration_test.py | 132 ++++++++++++++---- 3 files changed, 142 insertions(+), 44 deletions(-) diff --git a/src/core/errors.py b/src/core/errors.py index 5f874894..3f53364a 100644 --- a/src/core/errors.py +++ b/src/core/errors.py @@ -219,6 +219,24 @@ class TagAlreadyExistsError(ProblemDetailError): _default_code = 473 +class TagNotFoundError(ProblemDetailError): + """Raised when trying to remove or retrieve a tag that does not exist.""" + + uri = "https://openml.org/problems/tag-not-found" + title = "Tag Not Found" + _default_status_code = HTTPStatus.NOT_FOUND + _default_code = 475 + + +class TagNotOwnedError(ProblemDetailError): + """Raised when trying to remove a tag that was created by someone else.""" + + uri = "https://openml.org/problems/tag-not-owned" + title = "Tag Not Owned" + _default_status_code = HTTPStatus.FORBIDDEN + _default_code = 476 + + # ============================================================================= # Search/List Errors # ============================================================================= @@ -329,6 +347,20 @@ class FlowNotFoundError(ProblemDetailError): _default_status_code = HTTPStatus.NOT_FOUND +# ============================================================================= +# Setup Errors +# ============================================================================= + + +class SetupNotFoundError(ProblemDetailError): + """Raised when a setup cannot be found.""" + + uri = "https://openml.org/problems/setup-not-found" + title = "Setup Not Found" + _default_status_code = HTTPStatus.NOT_FOUND + _default_code = 472 + + # ============================================================================= # Service Errors # ============================================================================= diff --git a/src/routers/openml/setups.py b/src/routers/openml/setups.py index eb7005c8..52c8c96d 100644 --- a/src/routers/openml/setups.py +++ b/src/routers/openml/setups.py @@ -1,12 +1,12 @@ """All endpoints that relate to setups.""" -from http import HTTPStatus from typing import Annotated -from fastapi import APIRouter, Body, Depends, HTTPException +from fastapi import APIRouter, Body, Depends from sqlalchemy.ext.asyncio import AsyncConnection import database.setups +from core.errors import SetupNotFoundError, TagNotFoundError, TagNotOwnedError from database.users import User, UserGroup from routers.dependencies import expdb_connection, fetch_user_or_raise from routers.types import SystemString64 @@ -23,25 +23,21 @@ async def untag_setup( ) -> dict[str, dict[str, str]]: """Remove tag `tag` from setup with id `setup_id`.""" if not await database.setups.get(setup_id, expdb_db): - raise HTTPException( - status_code=HTTPStatus.PRECONDITION_FAILED, - detail={"code": "472", "message": "Entity not found."}, - ) + msg = f"Setup {setup_id} not found." + raise SetupNotFoundError(msg) setup_tags = await database.setups.get_tags(setup_id, expdb_db) matched_tag_row = next((t for t in setup_tags if t.tag.casefold() == tag.casefold()), None) if not matched_tag_row: - raise HTTPException( - status_code=HTTPStatus.PRECONDITION_FAILED, - detail={"code": "475", "message": "Tag not found."}, - ) + msg = f"Setup {setup_id} does not have tag {tag!r}." + raise TagNotFoundError(msg) if matched_tag_row.uploader != user.user_id and UserGroup.ADMIN not in await user.get_groups(): - raise HTTPException( - status_code=HTTPStatus.PRECONDITION_FAILED, - detail={"code": "476", "message": "Tag is not owned by you"}, + msg = ( + f"You may not remove tag {tag!r} of setup {setup_id} because it was not created by you." ) + raise TagNotOwnedError(msg) await database.setups.untag(setup_id, matched_tag_row.tag, expdb_db) diff --git a/tests/routers/openml/migration/setups_migration_test.py b/tests/routers/openml/migration/setups_migration_test.py index 3351e69d..492727c4 100644 --- a/tests/routers/openml/migration/setups_migration_test.py +++ b/tests/routers/openml/migration/setups_migration_test.py @@ -1,61 +1,131 @@ +import contextlib +import re +from collections.abc import AsyncGenerator from http import HTTPStatus import httpx import pytest +from sqlalchemy import text +from sqlalchemy.ext.asyncio import AsyncConnection -from tests.users import ApiKey +from tests.users import OWNER_USER, ApiKey -@pytest.mark.parametrize( - "setup_id", - [1, 999999], - ids=["existing setup", "unknown setup"], -) @pytest.mark.parametrize( "api_key", [ApiKey.ADMIN, ApiKey.SOME_USER, ApiKey.OWNER_USER], - ids=["Administrator", "regular user", "possible owner"], -) -@pytest.mark.parametrize( - "tag", - ["totally_new_tag_for_migration_testing"], + ids=["Administrator", "non-owner", "tag owner"], ) -async def test_setup_untag_response_is_identical( - setup_id: int, - tag: str, +async def test_setup_untag_response_is_identical_when_tag_exists( api_key: str, py_api: httpx.AsyncClient, php_api: httpx.AsyncClient, + expdb_test: AsyncConnection, ) -> None: - if setup_id == 1: - await php_api.post( - "/setup/tag", - data={"api_key": ApiKey.SOME_USER, "tag": tag, "setup_id": setup_id}, + setup_id = 1 + tag = "totally_new_tag_for_migration_testing" + + @contextlib.asynccontextmanager + async def temporary_tag() -> AsyncGenerator[None]: + await expdb_test.execute( + text( + "INSERT INTO setup_tag(`id`,`tag`,`uploader`) VALUES (:setup_id, :tag, :user_id);" + ), + parameters={"setup_id": setup_id, "tag": tag, "user_id": OWNER_USER.user_id}, + ) + await expdb_test.commit() + yield + await expdb_test.execute( + text("DELETE FROM setup_tag WHERE `id`=:setup_id AND `tag`=:tag"), + parameters={"setup_id": setup_id, "tag": tag}, ) + await expdb_test.commit() + + async with temporary_tag(): + original = await php_api.post( + "/setup/untag", + data={"api_key": api_key, "tag": tag, "setup_id": setup_id}, + ) + + # expdb_test transaction shared with Python API, + # no commit needed and rolled back at the end of the test + await expdb_test.execute( + text("INSERT INTO setup_tag(`id`,`tag`,`uploader`) VALUES (:setup_id, :tag, :user_id);"), + parameters={"setup_id": setup_id, "tag": tag, "user_id": OWNER_USER.user_id}, + ) + + new = await py_api.post( + f"/setup/untag?api_key={api_key}", + json={"setup_id": setup_id, "tag": tag}, + ) + + if new.status_code == HTTPStatus.OK: + assert original.status_code == new.status_code + assert original.json() == new.json() + return + + code, message = original.json()["error"].values() + assert original.status_code == HTTPStatus.PRECONDITION_FAILED + assert new.status_code == HTTPStatus.FORBIDDEN + assert code == new.json()["code"] + assert message == "Tag is not owned by you" + assert re.match( + r"You may not remove tag \S+ of setup \d+ because it was not created by you.", + new.json()["detail"], + ) + + +async def test_setup_untag_response_is_identical_setup_doesnt_exist( + py_api: httpx.AsyncClient, + php_api: httpx.AsyncClient, +) -> None: + setup_id = 999999 + tag = "totally_new_tag_for_migration_testing" + api_key = ApiKey.SOME_USER original = await php_api.post( "/setup/untag", data={"api_key": api_key, "tag": tag, "setup_id": setup_id}, ) - if original.status_code == HTTPStatus.OK: - await php_api.post( - "/setup/tag", - data={"api_key": ApiKey.SOME_USER, "tag": tag, "setup_id": setup_id}, - ) - new = await py_api.post( f"/setup/untag?api_key={api_key}", json={"setup_id": setup_id, "tag": tag}, ) - assert original.status_code == new.status_code + assert original.status_code == HTTPStatus.PRECONDITION_FAILED + assert new.status_code == HTTPStatus.NOT_FOUND + assert original.json()["error"]["message"] == "Entity not found." + assert original.json()["error"]["code"] == new.json()["code"] + assert re.match( + r"Setup \d+ not found.", + new.json()["detail"], + ) - if new.status_code != HTTPStatus.OK: - assert original.json()["error"] == new.json()["detail"] - return - original_json = original.json() - new_json = new.json() +async def test_setup_untag_response_is_identical_tag_doesnt_exist( + py_api: httpx.AsyncClient, + php_api: httpx.AsyncClient, +) -> None: + setup_id = 1 + tag = "totally_new_tag_for_migration_testing" + api_key = ApiKey.SOME_USER - assert original_json == new_json + original = await php_api.post( + "/setup/untag", + data={"api_key": api_key, "tag": tag, "setup_id": setup_id}, + ) + + new = await py_api.post( + f"/setup/untag?api_key={api_key}", + json={"setup_id": setup_id, "tag": tag}, + ) + + assert original.status_code == HTTPStatus.PRECONDITION_FAILED + assert new.status_code == HTTPStatus.NOT_FOUND + assert original.json()["error"]["code"] == new.json()["code"] + assert original.json()["error"]["message"] == "Tag not found." + assert re.match( + r"Setup \d+ does not have tag '\S+'.", + new.json()["detail"], + ) From c55f9e75c63876c1849a0e2da0c697cc253b886b Mon Sep 17 00:00:00 2001 From: PGijsbers Date: Thu, 12 Mar 2026 14:39:19 +0100 Subject: [PATCH 6/8] Preserve behavior of returning pre-existing tags --- src/routers/openml/setups.py | 6 +- .../openml/migration/setups_migration_test.py | 70 ++++++++++++------- 2 files changed, 47 insertions(+), 29 deletions(-) diff --git a/src/routers/openml/setups.py b/src/routers/openml/setups.py index 52c8c96d..c7823f8e 100644 --- a/src/routers/openml/setups.py +++ b/src/routers/openml/setups.py @@ -20,7 +20,7 @@ async def untag_setup( tag: Annotated[str, SystemString64], user: Annotated[User, Depends(fetch_user_or_raise)], expdb_db: Annotated[AsyncConnection, Depends(expdb_connection)], -) -> dict[str, dict[str, str]]: +) -> dict[str, dict[str, str | list[str]]]: """Remove tag `tag` from setup with id `setup_id`.""" if not await database.setups.get(setup_id, expdb_db): msg = f"Setup {setup_id} not found." @@ -40,5 +40,5 @@ async def untag_setup( raise TagNotOwnedError(msg) await database.setups.untag(setup_id, matched_tag_row.tag, expdb_db) - - return {"setup_untag": {"id": str(setup_id)}} + remaining_tags = [t.tag.casefold() for t in setup_tags if t != matched_tag_row] + return {"setup_untag": {"id": str(setup_id), "tag": remaining_tags}} diff --git a/tests/routers/openml/migration/setups_migration_test.py b/tests/routers/openml/migration/setups_migration_test.py index 492727c4..361dfd78 100644 --- a/tests/routers/openml/migration/setups_migration_test.py +++ b/tests/routers/openml/migration/setups_migration_test.py @@ -1,6 +1,6 @@ import contextlib import re -from collections.abc import AsyncGenerator +from collections.abc import AsyncGenerator, Iterable from http import HTTPStatus import httpx @@ -16,8 +16,14 @@ [ApiKey.ADMIN, ApiKey.SOME_USER, ApiKey.OWNER_USER], ids=["Administrator", "non-owner", "tag owner"], ) +@pytest.mark.parametrize( + "other_tags", + [[], ["some_other_tag"], ["foo_some_other_tag", "bar_some_other_tag"]], + ids=["none", "one tag", "two tags"], +) async def test_setup_untag_response_is_identical_when_tag_exists( api_key: str, + other_tags: list[str], py_api: httpx.AsyncClient, php_api: httpx.AsyncClient, expdb_test: AsyncConnection, @@ -26,22 +32,29 @@ async def test_setup_untag_response_is_identical_when_tag_exists( tag = "totally_new_tag_for_migration_testing" @contextlib.asynccontextmanager - async def temporary_tag() -> AsyncGenerator[None]: - await expdb_test.execute( - text( - "INSERT INTO setup_tag(`id`,`tag`,`uploader`) VALUES (:setup_id, :tag, :user_id);" - ), - parameters={"setup_id": setup_id, "tag": tag, "user_id": OWNER_USER.user_id}, - ) - await expdb_test.commit() + async def temporary_tags( + tags: Iterable[str], setup_id: int, *, persist: bool = False + ) -> AsyncGenerator[None]: + for tag in tags: + await expdb_test.execute( + text( + "INSERT INTO setup_tag(`id`,`tag`,`uploader`) VALUES (:setup_id, :tag, :user_id);" # noqa: E501 + ), + parameters={"setup_id": setup_id, "tag": tag, "user_id": OWNER_USER.user_id}, + ) + if persist: + await expdb_test.commit() yield - await expdb_test.execute( - text("DELETE FROM setup_tag WHERE `id`=:setup_id AND `tag`=:tag"), - parameters={"setup_id": setup_id, "tag": tag}, - ) - await expdb_test.commit() - - async with temporary_tag(): + for tag in tags: + await expdb_test.execute( + text("DELETE FROM setup_tag WHERE `id`=:setup_id AND `tag`=:tag"), + parameters={"setup_id": setup_id, "tag": tag}, + ) + if persist: + await expdb_test.commit() + + all_tags = [tag, *other_tags] + async with temporary_tags(tags=all_tags, setup_id=setup_id, persist=True): original = await php_api.post( "/setup/untag", data={"api_key": api_key, "tag": tag, "setup_id": setup_id}, @@ -49,19 +62,24 @@ async def temporary_tag() -> AsyncGenerator[None]: # expdb_test transaction shared with Python API, # no commit needed and rolled back at the end of the test - await expdb_test.execute( - text("INSERT INTO setup_tag(`id`,`tag`,`uploader`) VALUES (:setup_id, :tag, :user_id);"), - parameters={"setup_id": setup_id, "tag": tag, "user_id": OWNER_USER.user_id}, - ) - - new = await py_api.post( - f"/setup/untag?api_key={api_key}", - json={"setup_id": setup_id, "tag": tag}, - ) + async with temporary_tags(tags=all_tags, setup_id=setup_id): + new = await py_api.post( + f"/setup/untag?api_key={api_key}", + json={"setup_id": setup_id, "tag": tag}, + ) if new.status_code == HTTPStatus.OK: assert original.status_code == new.status_code - assert original.json() == new.json() + original_untag = original.json()["setup_untag"] + new_untag = new.json()["setup_untag"] + assert original_untag["id"] == new_untag["id"] + if tags := original_untag.get("tag"): + if isinstance(tags, str): + assert tags == new_untag["tag"][0] + else: + assert tags == new_untag["tag"] + else: + assert new_untag["tag"] == [] return code, message = original.json()["error"].values() From 3da8be32cba8f4d4c6ec1724e9d471c2df6b34aa Mon Sep 17 00:00:00 2001 From: PGijsbers Date: Thu, 12 Mar 2026 15:25:07 +0100 Subject: [PATCH 7/8] Assert RFC9457 responses --- tests/routers/openml/setups_test.py | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/tests/routers/openml/setups_test.py b/tests/routers/openml/setups_test.py index fa246482..ac631d45 100644 --- a/tests/routers/openml/setups_test.py +++ b/tests/routers/openml/setups_test.py @@ -1,3 +1,4 @@ +import re from collections.abc import AsyncGenerator from http import HTTPStatus @@ -39,8 +40,11 @@ async def test_setup_untag_unknown_setup(py_api: httpx.AsyncClient) -> None: f"/setup/untag?api_key={ApiKey.SOME_USER}", json={"setup_id": 999999, "tag": "test_tag"}, ) - assert response.status_code == HTTPStatus.PRECONDITION_FAILED - assert response.json()["detail"] == {"code": "472", "message": "Entity not found."} + assert response.status_code == HTTPStatus.NOT_FOUND + assert re.match( + r"Setup \d+ not found.", + response.json()["detail"], + ) async def test_setup_untag_tag_not_found(py_api: httpx.AsyncClient) -> None: @@ -48,8 +52,11 @@ async def test_setup_untag_tag_not_found(py_api: httpx.AsyncClient) -> None: f"/setup/untag?api_key={ApiKey.SOME_USER}", json={"setup_id": 1, "tag": "non_existent_tag_12345"}, ) - assert response.status_code == HTTPStatus.PRECONDITION_FAILED - assert response.json()["detail"] == {"code": "475", "message": "Tag not found."} + assert response.status_code == HTTPStatus.NOT_FOUND + assert re.match( + r"Setup \d+ does not have tag '\S+'.", + response.json()["detail"], + ) @pytest.mark.mut @@ -59,8 +66,11 @@ async def test_setup_untag_not_owned_by_you(py_api: httpx.AsyncClient) -> None: f"/setup/untag?api_key={ApiKey.OWNER_USER}", json={"setup_id": 1, "tag": "test_unit_tag_123"}, ) - assert response.status_code == HTTPStatus.PRECONDITION_FAILED - assert response.json()["detail"] == {"code": "476", "message": "Tag is not owned by you"} + assert response.status_code == HTTPStatus.FORBIDDEN + assert re.match( + r"You may not remove tag '\S+' of setup \d+ because it was not created by you.", + response.json()["detail"], + ) @pytest.mark.mut @@ -86,7 +96,7 @@ async def test_setup_untag_success( ) assert response.status_code == HTTPStatus.OK - assert response.json() == {"setup_untag": {"id": "1"}} + assert response.json() == {"setup_untag": {"id": "1", "tag": []}} rows = await expdb_test.execute( text("SELECT * FROM setup_tag WHERE id = 1 AND tag = 'test_success_tag'") From 6b4073b82237704c5e22513cac2077309154de9d Mon Sep 17 00:00:00 2001 From: PGijsbers Date: Thu, 12 Mar 2026 15:32:58 +0100 Subject: [PATCH 8/8] Avoid committing changes to the database --- tests/routers/openml/setups_test.py | 31 ++++++----------------------- 1 file changed, 6 insertions(+), 25 deletions(-) diff --git a/tests/routers/openml/setups_test.py b/tests/routers/openml/setups_test.py index ac631d45..5ea8b515 100644 --- a/tests/routers/openml/setups_test.py +++ b/tests/routers/openml/setups_test.py @@ -1,5 +1,4 @@ import re -from collections.abc import AsyncGenerator from http import HTTPStatus import httpx @@ -10,24 +9,6 @@ from tests.users import ApiKey -@pytest.fixture -async def mock_setup_tag(expdb_test: AsyncConnection) -> AsyncGenerator[None]: - await expdb_test.execute( - text("DELETE FROM setup_tag WHERE id = 1 AND tag = 'test_unit_tag_123'"), - ) - await expdb_test.execute( - text("INSERT INTO setup_tag (id, tag, uploader) VALUES (1, 'test_unit_tag_123', 2)") - ) - await expdb_test.commit() - - yield - - await expdb_test.execute( - text("DELETE FROM setup_tag WHERE id = 1 AND tag = 'test_unit_tag_123'"), - ) - await expdb_test.commit() - - async def test_setup_untag_missing_auth(py_api: httpx.AsyncClient) -> None: response = await py_api.post("/setup/untag", json={"setup_id": 1, "tag": "test_tag"}) assert response.status_code == HTTPStatus.UNAUTHORIZED @@ -60,8 +41,12 @@ async def test_setup_untag_tag_not_found(py_api: httpx.AsyncClient) -> None: @pytest.mark.mut -@pytest.mark.usefixtures("mock_setup_tag") -async def test_setup_untag_not_owned_by_you(py_api: httpx.AsyncClient) -> None: +async def test_setup_untag_not_owned_by_you( + py_api: httpx.AsyncClient, expdb_test: AsyncConnection +) -> None: + await expdb_test.execute( + text("INSERT INTO setup_tag (id, tag, uploader) VALUES (1, 'test_unit_tag_123', 2);") + ) response = await py_api.post( f"/setup/untag?api_key={ApiKey.OWNER_USER}", json={"setup_id": 1, "tag": "test_unit_tag_123"}, @@ -82,13 +67,9 @@ async def test_setup_untag_not_owned_by_you(py_api: httpx.AsyncClient) -> None: async def test_setup_untag_success( api_key: str, py_api: httpx.AsyncClient, expdb_test: AsyncConnection ) -> None: - await expdb_test.execute( - text("DELETE FROM setup_tag WHERE id = 1 AND tag = 'test_success_tag'") - ) await expdb_test.execute( text("INSERT INTO setup_tag (id, tag, uploader) VALUES (1, 'test_success_tag', 2)") ) - await expdb_test.commit() response = await py_api.post( f"/setup/untag?api_key={api_key}",