[ENH] Add POST /setup/tag endpoint (#271)

Fixes: #64

- Implemented the POST /setup/tag FastAPI endpoint and underlying
database logic.
- Modified the response to return the full array of remaining tags to
match the old PHP API behavior.
- Added comprehensive unit tests and `py_api` vs `php_api migration`
parity tests.
- Updated `docs/migration.md` to document the new tag array response
behavior.

---------

Co-authored-by: igennova <luckynegi025@gmail.com>

Author: igennova

docs/migration.md

@@ -107,6 +107,11 @@ For example, after tagging dataset 21 with the tag `"foo"`:
 }
 ```
 
+## Setups
+
+### `POST /setup/tag` and `POST /setup/untag`
+When successful, the "tag" property in the returned response is now always a list, even if only one tag exists for the entity. When removing the last tag, the "tag" property will be an empty list `[]` instead of being omitted from the response.
+
 ## Studies
 
 ### `GET /{id_or_alias}`

src/database/setups.py

@@ -46,3 +46,16 @@ async def untag(setup_id: int, tag: str, connection: AsyncConnection) -> None:
         ),
         parameters={"setup_id": setup_id, "tag": tag},
     )
+
+
+async def tag(setup_id: int, tag: str, user_id: int, connection: AsyncConnection) -> None:
+    """Add tag `tag` to setup with id `setup_id`."""
+    await connection.execute(
+        text(
+            """
+            INSERT INTO setup_tag (id, tag, uploader)
+            VALUES (:setup_id, :tag, :user_id)
+            """,
+        ),
+        parameters={"setup_id": setup_id, "tag": tag, "user_id": user_id},
+    )

src/routers/openml/setups.py

@@ -6,14 +6,43 @@
 from sqlalchemy.ext.asyncio import AsyncConnection
 
 import database.setups
-from core.errors import SetupNotFoundError, TagNotFoundError, TagNotOwnedError
+from core.errors import (
+    SetupNotFoundError,
+    TagAlreadyExistsError,
+    TagNotFoundError,
+    TagNotOwnedError,
+)
 from database.users import User, UserGroup
 from routers.dependencies import expdb_connection, fetch_user_or_raise
 from routers.types import SystemString64
 
 router = APIRouter(prefix="/setup", tags=["setup"])
 
 
+@router.post(path="/tag")
+async def tag_setup(
+    setup_id: Annotated[int, Body()],
+    tag: Annotated[str, SystemString64],
+    user: Annotated[User, Depends(fetch_user_or_raise)],
+    expdb_db: Annotated[AsyncConnection, Depends(expdb_connection)],
+) -> dict[str, dict[str, str | list[str]]]:
+    """Add tag `tag` to setup with id `setup_id`."""
+    if not await database.setups.get(setup_id, expdb_db):
+        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 matched_tag_row:
+        msg = f"Setup {setup_id} already has tag {tag!r}."
+        raise TagAlreadyExistsError(msg)
+
+    await database.setups.tag(setup_id, tag, user.user_id, expdb_db)
+    all_tags = [t.tag for t in setup_tags] + [tag]
+    return {"setup_tag": {"id": str(setup_id), "tag": all_tags}}
+
+
 @router.post(path="/untag")
 async def untag_setup(
     setup_id: Annotated[int, Body()],
@@ -40,5 +69,7 @@ async def untag_setup(
         raise TagNotOwnedError(msg)
 
     await database.setups.untag(setup_id, matched_tag_row.tag, expdb_db)
-    remaining_tags = [t.tag.casefold() for t in setup_tags if t != matched_tag_row]
+    remaining_tags = [
+        t.tag for t in setup_tags if t.tag.casefold() != matched_tag_row.tag.casefold()
+    ]
     return {"setup_untag": {"id": str(setup_id), "tag": remaining_tags}}

tests/conftest.py

@@ -1,6 +1,6 @@
 import contextlib
 import json
-from collections.abc import AsyncIterator, Iterator
+from collections.abc import AsyncIterator, Iterable, Iterator
 from pathlib import Path
 from typing import Any, NamedTuple
 
@@ -28,6 +28,29 @@ async def automatic_rollback(engine: AsyncEngine) -> AsyncIterator[AsyncConnecti
             await transaction.rollback()
 
 
+@contextlib.asynccontextmanager
+async def temporary_records(
+    connection: AsyncConnection,
+    insert_queries: Iterable[tuple[str, dict[str, Any] | None]],
+    delete_queries: Iterable[tuple[str, dict[str, Any] | None]],
+    *,
+    persist: bool = False,
+) -> AsyncIterator[None]:
+    """Execute insert queries on enter and their corresponding delete queries on exit."""
+    for query, parameters in insert_queries:
+        await connection.execute(text(query), parameters=parameters)
+    if persist:
+        await connection.commit()
+
+    try:
+        yield
+    finally:
+        for query, parameters in delete_queries:
+            await connection.execute(text(query), parameters=parameters)
+        if persist:
+            await connection.commit()
+
+
 @pytest.fixture
 async def expdb_test() -> AsyncIterator[AsyncConnection]:
     async with automatic_rollback(expdb_database()) as connection:

tests/routers/openml/migration/setups_migration_test.py

@@ -1,16 +1,51 @@
 import contextlib
 import re
-from collections.abc import AsyncGenerator, Iterable
+from collections.abc import AsyncGenerator, Callable, Iterable
+from contextlib import AbstractAsyncContextManager
 from http import HTTPStatus
 
 import httpx
 import pytest
 from sqlalchemy import text
 from sqlalchemy.ext.asyncio import AsyncConnection
 
+from tests.conftest import temporary_records
 from tests.users import OWNER_USER, ApiKey
 
 
+@pytest.fixture
+def temporary_tags(
+    expdb_test: AsyncConnection,
+) -> Callable[..., AbstractAsyncContextManager[None]]:
+    @contextlib.asynccontextmanager
+    async def _temporary_tags(
+        tags: Iterable[str], setup_id: int, *, persist: bool = False
+    ) -> AsyncGenerator[None]:
+        insert_queries = [
+            (
+                "INSERT INTO setup_tag(`id`,`tag`,`uploader`) VALUES (:setup_id, :tag, :user_id);",
+                {"setup_id": setup_id, "tag": tag, "user_id": OWNER_USER.user_id},
+            )
+            for tag in tags
+        ]
+        delete_queries = [
+            (
+                "DELETE FROM setup_tag WHERE `id`=:setup_id AND `tag`=:tag",
+                {"setup_id": setup_id, "tag": tag},
+            )
+            for tag in tags
+        ]
+        async with temporary_records(
+            connection=expdb_test,
+            insert_queries=insert_queries,
+            delete_queries=delete_queries,
+            persist=persist,
+        ):
+            yield
+
+    return _temporary_tags
+
+
 @pytest.mark.parametrize(
     "api_key",
     [ApiKey.ADMIN, ApiKey.SOME_USER, ApiKey.OWNER_USER],
@@ -26,33 +61,11 @@ async def test_setup_untag_response_is_identical_when_tag_exists(
     other_tags: list[str],
     py_api: httpx.AsyncClient,
     php_api: httpx.AsyncClient,
-    expdb_test: AsyncConnection,
+    temporary_tags: Callable[..., AbstractAsyncContextManager[None]],
 ) -> None:
     setup_id = 1
     tag = "totally_new_tag_for_migration_testing"
 
-    @contextlib.asynccontextmanager
-    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
-        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(
@@ -147,3 +160,112 @@ async def test_setup_untag_response_is_identical_tag_doesnt_exist(
         r"Setup \d+ does not have tag '\S+'.",
         new.json()["detail"],
     )
+
+
+@pytest.mark.parametrize(
+    "api_key",
+    [ApiKey.ADMIN, ApiKey.SOME_USER],
+    ids=["Administrator", "non-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_tag_response_is_identical_when_tag_doesnt_exist(  # noqa: PLR0913
+    api_key: str,
+    other_tags: list[str],
+    py_api: httpx.AsyncClient,
+    php_api: httpx.AsyncClient,
+    expdb_test: AsyncConnection,
+    temporary_tags: Callable[..., AbstractAsyncContextManager[None]],
+) -> None:
+    setup_id = 1
+    tag = "totally_new_tag_for_migration_testing"
+
+    async with temporary_tags(tags=other_tags, setup_id=setup_id, persist=True):
+        original = await php_api.post(
+            "/setup/tag",
+            data={"api_key": api_key, "tag": tag, "setup_id": setup_id},
+        )
+
+        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_tags(tags=other_tags, setup_id=setup_id):
+        new = await py_api.post(
+            f"/setup/tag?api_key={api_key}",
+            json={"setup_id": setup_id, "tag": tag},
+        )
+
+    assert new.status_code == HTTPStatus.OK
+    assert original.status_code == new.status_code
+    original_tag = original.json()["setup_tag"]
+    new_tag = new.json()["setup_tag"]
+    assert original_tag["id"] == new_tag["id"]
+    if tags := original_tag.get("tag"):
+        if isinstance(tags, str):
+            assert tags == new_tag["tag"][0]
+        else:
+            assert set(tags) == set(new_tag["tag"])
+    else:
+        assert new_tag["tag"] == []
+
+
+async def test_setup_tag_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/tag",
+        data={"api_key": api_key, "tag": tag, "setup_id": setup_id},
+    )
+
+    new = await py_api.post(
+        f"/setup/tag?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"]["message"] == "Entity not found."
+    assert original.json()["error"]["code"] == new.json()["code"]
+    assert re.match(
+        r"Setup \d+ not found.",
+        new.json()["detail"],
+    )
+
+
+async def test_setup_tag_response_is_identical_tag_already_exists(
+    py_api: httpx.AsyncClient,
+    php_api: httpx.AsyncClient,
+    temporary_tags: Callable[..., AbstractAsyncContextManager[None]],
+) -> None:
+    setup_id = 1
+    tag = "totally_new_tag_for_migration_testing"
+    api_key = ApiKey.SOME_USER
+
+    async with temporary_tags(tags=[tag], setup_id=setup_id, persist=True):
+        original = await php_api.post(
+            "/setup/tag",
+            data={"api_key": api_key, "tag": tag, "setup_id": setup_id},
+        )
+
+        # In Python, since PHP committed it, it's also there for Python test context
+        new = await py_api.post(
+            f"/setup/tag?api_key={api_key}",
+            json={"setup_id": setup_id, "tag": tag},
+        )
+
+    assert original.status_code == HTTPStatus.INTERNAL_SERVER_ERROR
+    assert new.status_code == HTTPStatus.CONFLICT
+    assert original.json()["error"]["code"] == new.json()["code"]
+    assert original.json()["error"]["message"] == "Entity already tagged by this tag."
+    assert new.json()["detail"] == f"Setup {setup_id} already has tag {tag!r}."

tests/routers/openml/setups_test.py

@@ -83,3 +83,50 @@ async def test_setup_untag_success(
         text("SELECT * FROM setup_tag WHERE id = 1 AND tag = 'test_success_tag'")
     )
     assert len(rows.all()) == 0
+
+
+async def test_setup_tag_missing_auth(py_api: httpx.AsyncClient) -> None:
+    response = await py_api.post("/setup/tag", json={"setup_id": 1, "tag": "test_tag"})
+    assert response.status_code == HTTPStatus.UNAUTHORIZED
+    assert response.json()["code"] == "103"
+    assert response.json()["detail"] == "Authentication failed"
+
+
+async def test_setup_tag_unknown_setup(py_api: httpx.AsyncClient) -> None:
+    response = await py_api.post(
+        f"/setup/tag?api_key={ApiKey.SOME_USER}",
+        json={"setup_id": 999999, "tag": "test_tag"},
+    )
+    assert response.status_code == HTTPStatus.NOT_FOUND
+    assert re.match(r"Setup \d+ not found.", response.json()["detail"])
+
+
+@pytest.mark.mut
+async def test_setup_tag_already_exists(
+    py_api: httpx.AsyncClient, expdb_test: AsyncConnection
+) -> None:
+    await expdb_test.execute(
+        text("INSERT INTO setup_tag (id, tag, uploader) VALUES (1, 'existing_tag_123', 2);")
+    )
+    response = await py_api.post(
+        f"/setup/tag?api_key={ApiKey.SOME_USER}",
+        json={"setup_id": 1, "tag": "existing_tag_123"},
+    )
+    assert response.status_code == HTTPStatus.CONFLICT
+    assert response.json()["detail"] == "Setup 1 already has tag 'existing_tag_123'."
+
+
+@pytest.mark.mut
+async def test_setup_tag_success(py_api: httpx.AsyncClient, expdb_test: AsyncConnection) -> None:
+    response = await py_api.post(
+        f"/setup/tag?api_key={ApiKey.SOME_USER}",
+        json={"setup_id": 1, "tag": "my_new_success_tag"},
+    )
+
+    assert response.status_code == HTTPStatus.OK
+    assert "my_new_success_tag" in response.json()["setup_tag"]["tag"]
+
+    rows = await expdb_test.execute(
+        text("SELECT * FROM setup_tag WHERE id = 1 AND tag = 'my_new_success_tag'")
+    )
+    assert len(rows.all()) == 1