-
-
Notifications
You must be signed in to change notification settings - Fork 45
feat: Migrate POST /setup/tag endpoint to FastAPI #257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
f33fbd2
4451c7d
d20c523
af6b92a
8dfd48a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| from sqlalchemy import Connection, text | ||
| from sqlalchemy.engine import Row | ||
|
|
||
|
|
||
| def get(id_: int, connection: Connection) -> Row | None: | ||
| """Get the setup by its ID.""" | ||
| row = connection.execute( | ||
| text( | ||
| """ | ||
| SELECT * | ||
| FROM algorithm_setup | ||
| WHERE sid = :setup_id | ||
| """, | ||
| ), | ||
| parameters={"setup_id": id_}, | ||
| ) | ||
| return row.one_or_none() | ||
|
|
||
|
|
||
| def get_tags_for(id_: int, connection: Connection) -> list[str]: | ||
| """Get all tags for a specific setup.""" | ||
| rows = connection.execute( | ||
| text( | ||
| """ | ||
| SELECT tag | ||
| FROM setup_tag | ||
| WHERE id = :setup_id | ||
| """, | ||
| ), | ||
| parameters={"setup_id": id_}, | ||
| ) | ||
| return [row.tag for row in rows] | ||
|
|
||
|
|
||
| def tag(id_: int, tag_: str, *, user_id: int, connection: Connection) -> None: | ||
| """Insert a new tag for the setup.""" | ||
| connection.execute( | ||
| text( | ||
| """ | ||
| INSERT INTO setup_tag(`id`, `tag`, `uploader`) | ||
| VALUES (:setup_id, :tag, :user_id) | ||
| """, | ||
| ), | ||
| parameters={ | ||
| "setup_id": id_, | ||
| "user_id": user_id, | ||
| "tag": tag_, | ||
| }, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| from http import HTTPStatus | ||
| from typing import Annotated, Any | ||
|
|
||
| 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=["setups"]) | ||
|
|
||
|
|
||
| def create_authentication_failed_error() -> HTTPException: | ||
| return HTTPException( | ||
| status_code=HTTPStatus.PRECONDITION_FAILED, | ||
| detail={"code": "103", "message": "Authentication failed"}, | ||
| ) | ||
|
|
||
|
|
||
| def create_tag_exists_error(setup_id: int, tag: str) -> HTTPException: | ||
| return HTTPException( | ||
| # Changed from INTERNAL_SERVER_ERROR (500) to CONFLICT (409) | ||
| status_code=HTTPStatus.CONFLICT, | ||
| detail={ | ||
| "code": "473", | ||
| "message": "Entity already tagged by this tag.", | ||
| "additional_information": f"id={setup_id}; tag={tag}", | ||
| }, | ||
| ) | ||
|
|
||
|
|
||
| @router.post("/tag") | ||
| def tag_setup( | ||
| setup_id: Annotated[int, Body()], | ||
| tag: Annotated[str, Body(..., embed=False), SystemString64], | ||
| user: Annotated[User | None, Depends(fetch_user)] = None, | ||
| expdb_db: Annotated[Connection, Depends(expdb_connection)] = None, | ||
| ) -> dict[str, dict[str, Any]]: | ||
| # 1. AUTHENTICATE FIRST | ||
| if user is None: | ||
| raise create_authentication_failed_error() | ||
|
|
||
| # 2. VERIFY EXISTENCE | ||
| setup = database.setups.get(setup_id, expdb_db) | ||
| if not setup: | ||
| raise HTTPException(status_code=HTTPStatus.NOT_FOUND, detail="Setup not found") | ||
|
|
||
| # 3. VERIFY OWNERSHIP / PERMISSIONS | ||
| # (Fixes the crash by not looking for a Dataset 'visibility' column) | ||
| is_admin = UserGroup.ADMIN in user.groups | ||
| is_owner = getattr(setup, "uploader", None) == user.user_id | ||
|
|
||
| if not (is_admin or is_owner): | ||
| raise HTTPException(status_code=HTTPStatus.FORBIDDEN, detail="No access granted") | ||
|
|
||
| # 4. CHECK IF TAG EXISTS | ||
| tags = database.setups.get_tags_for(setup_id, expdb_db) | ||
| if tag.casefold() in [t.casefold() for t in tags]: | ||
| raise create_tag_exists_error(setup_id, tag) | ||
|
|
||
| # 5. APPLY THE TAG | ||
| database.setups.tag(setup_id, tag, user_id=user.user_id, connection=expdb_db) | ||
|
|
||
|
Comment on lines
+59
to
+65
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicate prevention is race-prone under concurrent requests. The current check-then-insert flow can still insert duplicates when two requests run at the same time. Enforce uniqueness in the DB (e.g., unique key on setup/tag) and map duplicate-write failure to the existing “already tagged” error. 🤖 Prompt for AI Agents |
||
| return { | ||
| "setup_tag": {"id": str(setup_id), "tag": [*tags, tag]}, | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Return tags in a deterministic order and avoid over-fetching.
SELECT *without ordering can yield unstable tag ordering in responses and tests. Fetch onlytagand add an explicitORDER BY.💡 Suggested query refinement
def get_tags_for(id_: int, connection: Connection) -> list[str]: """Get all tags for a specific setup.""" rows = connection.execute( text( """ - SELECT * + SELECT tag FROM setup_tag WHERE id = :setup_id + ORDER BY tag """, ), parameters={"setup_id": id_}, ) return [row.tag for row in rows]🤖 Prompt for AI Agents