From 885399d89ff795f221150d12290e6faff2f16a43 Mon Sep 17 00:00:00 2001 From: igennova Date: Mon, 20 Apr 2026 03:21:05 +0530 Subject: [PATCH 1/8] Add DELETE /users/{id} with no-resources rule --- src/core/errors.py | 16 ++ src/database/users.py | 38 +++++ src/main.py | 2 + src/routers/openml/users.py | 54 ++++++ tests/routers/openml/users_delete_test.py | 191 ++++++++++++++++++++++ 5 files changed, 301 insertions(+) create mode 100644 src/routers/openml/users.py create mode 100644 tests/routers/openml/users_delete_test.py diff --git a/src/core/errors.py b/src/core/errors.py index 69d7e0c7..fe111c58 100644 --- a/src/core/errors.py +++ b/src/core/errors.py @@ -228,6 +228,22 @@ class ForbiddenError(ProblemDetailError): _default_status_code = HTTPStatus.FORBIDDEN +class UserNotFoundError(ProblemDetailError): + """Raised when a user id does not exist in the user database.""" + + uri = "https://openml.org/problems/user-not-found" + title = "User Not Found" + _default_status_code = HTTPStatus.NOT_FOUND + + +class AccountHasResourcesError(ProblemDetailError): + """Raised when account deletion is blocked because the user still owns resources.""" + + uri = "https://openml.org/problems/account-has-resources" + title = "Account Has Active Resources" + _default_status_code = HTTPStatus.CONFLICT + + # ============================================================================= # Tag Errors # ============================================================================= diff --git a/src/database/users.py b/src/database/users.py index 0b09fb0d..2507abfd 100644 --- a/src/database/users.py +++ b/src/database/users.py @@ -76,3 +76,41 @@ async def get_groups(self) -> list[UserGroup]: async def is_admin(self) -> bool: return UserGroup.ADMIN in await self.get_groups() + + +async def exists_by_id(*, user_id: int, connection: AsyncConnection) -> bool: + row = await connection.execute( + text("SELECT 1 FROM users WHERE id = :user_id LIMIT 1"), + parameters={"user_id": user_id}, + ) + return row.one_or_none() is not None + + +async def count_uploaded_resources(*, user_id: int, expdb: AsyncConnection) -> int: + """Count datasets, flows, runs, and studies uploaded or created by this user (expdb).""" + row = await expdb.execute( + text( + """ + SELECT ( + (SELECT COUNT(*) FROM dataset WHERE uploader = :uid) + + (SELECT COUNT(*) FROM implementation WHERE uploader = :uid) + + (SELECT COUNT(*) FROM `run` WHERE uploader = :uid) + + (SELECT COUNT(*) FROM study WHERE creator = :uid) + ) AS total + """, + ), + parameters={"uid": user_id}, + ) + return int(row.scalar_one()) + + +async def delete_user_rows(*, user_id: int, userdb: AsyncConnection) -> None: + """Remove group memberships then the user row (openml user database).""" + await userdb.execute( + text("DELETE FROM users_groups WHERE user_id = :user_id"), + parameters={"user_id": user_id}, + ) + await userdb.execute( + text("DELETE FROM users WHERE id = :user_id"), + parameters={"user_id": user_id}, + ) diff --git a/src/main.py b/src/main.py index 46cd79cf..0219db8b 100644 --- a/src/main.py +++ b/src/main.py @@ -34,6 +34,7 @@ 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 +from routers.openml.users import router as users_router @asynccontextmanager @@ -106,6 +107,7 @@ def create_api(configuration_file: Path | None = None) -> FastAPI: app.include_router(study_router) app.include_router(setup_router) app.include_router(run_router) + app.include_router(users_router) logger.info("App setup completed.") logger.remove(setup_sink) diff --git a/src/routers/openml/users.py b/src/routers/openml/users.py new file mode 100644 index 00000000..838f8703 --- /dev/null +++ b/src/routers/openml/users.py @@ -0,0 +1,54 @@ +"""User account HTTP endpoints.""" + +from typing import Annotated + +from fastapi import APIRouter, Depends, Path, Response +from sqlalchemy.ext.asyncio import AsyncConnection + +import database.users +from core.errors import AccountHasResourcesError, ForbiddenError, UserNotFoundError +from database.users import User +from routers.dependencies import expdb_connection, fetch_user_or_raise, userdb_connection + +router = APIRouter(prefix="/users", tags=["users"]) + + +@router.delete( + "/{user_id}", + responses={ + 204: {"description": "User account deleted."}, + 401: {"description": "Authentication failed or missing."}, + 403: {"description": "Not allowed to delete this account."}, + 404: {"description": "User id not found."}, + 409: {"description": "User still has datasets, flows, runs, or studies."}, + }, +) +async def delete_user_account( + user_id: Annotated[int, Path(description="Numeric user id to delete.", gt=0)], + current_user: Annotated[User, Depends(fetch_user_or_raise)], + expdb: Annotated[AsyncConnection, Depends(expdb_connection)], + userdb: Annotated[AsyncConnection, Depends(userdb_connection)], +) -> Response: + """Delete a user account when they have no uploaded resources (Phase 1). + + Callers may delete their own account. Administrators may delete any account + that satisfies the no-resources rule. + """ + if current_user.user_id != user_id and not await current_user.is_admin(): + msg = "You may only delete your own user account." + raise ForbiddenError(msg) + + if not await database.users.exists_by_id(user_id=user_id, connection=userdb): + msg = f"User {user_id} not found." + raise UserNotFoundError(msg) + + resource_count = await database.users.count_uploaded_resources(user_id=user_id, expdb=expdb) + if resource_count > 0: + msg = ( + "Cannot delete this account while datasets, flows, runs, or studies " + "are still associated with the user. Remove or transfer them first." + ) + raise AccountHasResourcesError(msg) + + await database.users.delete_user_rows(user_id=user_id, userdb=userdb) + return Response(status_code=204) diff --git a/tests/routers/openml/users_delete_test.py b/tests/routers/openml/users_delete_test.py new file mode 100644 index 00000000..7903c69b --- /dev/null +++ b/tests/routers/openml/users_delete_test.py @@ -0,0 +1,191 @@ +"""Tests for DELETE /users/{user_id} (Phase 1: no resources, self or admin).""" + +import uuid +from http import HTTPStatus + +import httpx +import pytest +from sqlalchemy import text +from sqlalchemy.ext.asyncio import AsyncConnection + +from core.errors import AccountHasResourcesError, ForbiddenError, UserNotFoundError +from routers.openml.users import delete_user_account +from tests.users import ADMIN_USER, SOME_USER, ApiKey + + +async def test_delete_user_missing_auth(py_api: httpx.AsyncClient) -> None: + response = await py_api.delete("/users/1") + assert response.status_code == HTTPStatus.UNAUTHORIZED + body = response.json() + assert body["code"] == "103" + assert body["detail"] == "No API key provided." + + +async def test_delete_user_not_found(py_api: httpx.AsyncClient) -> None: + response = await py_api.delete( + "/users/999999999", + params={"api_key": ApiKey.ADMIN}, + ) + assert response.status_code == HTTPStatus.NOT_FOUND + assert response.headers["content-type"] == "application/problem+json" + body = response.json() + assert body["type"] == UserNotFoundError.uri + assert body["detail"] == "User 999999999 not found." + + +async def test_delete_user_forbidden_non_admin_deletes_other( + py_api: httpx.AsyncClient, +) -> None: + response = await py_api.delete( + f"/users/{ADMIN_USER.user_id}", + params={"api_key": ApiKey.SOME_USER}, + ) + assert response.status_code == HTTPStatus.FORBIDDEN + body = response.json() + assert body["type"] == ForbiddenError.uri + assert body["detail"] == "You may only delete your own user account." + + +async def test_delete_user_conflict_when_user_has_resources( + py_api: httpx.AsyncClient, +) -> None: + """User 16 owns dataset 130 in the test database.""" + response = await py_api.delete( + "/users/16", + params={"api_key": ApiKey.ADMIN}, + ) + assert response.status_code == HTTPStatus.CONFLICT + assert response.headers["content-type"] == "application/problem+json" + body = response.json() + assert body["type"] == AccountHasResourcesError.uri + assert "datasets" in body["detail"] + + +@pytest.mark.mut +async def test_delete_user_api_success_self_delete( + py_api: httpx.AsyncClient, + user_test: AsyncConnection, +) -> None: + """Disposable user deletes their own account using their API key (session_hash).""" + api_key = "fedcba9876543210fedcba9876543210" + suffix = uuid.uuid4().hex[:10] + username = f"tmp_self_{suffix}" + email = f"{suffix}@openml-self-delete.test" + await user_test.execute( + text( + """ + INSERT INTO users ( + ip_address, username, password, email, created_on, + company, country, bio, session_hash + ) VALUES ( + '127.0.0.1', :username, 'x', :email, UNIX_TIMESTAMP(), + '', '', '', :api_key + ) + """, + ), + parameters={"username": username, "email": email, "api_key": api_key}, + ) + uid_row = await user_test.execute(text("SELECT LAST_INSERT_ID() AS id")) + (new_id,) = uid_row.one() + await user_test.execute( + text("INSERT INTO users_groups (user_id, group_id) VALUES (:uid, 2)"), + parameters={"uid": new_id}, + ) + + response = await py_api.delete( + f"/users/{new_id}", + params={"api_key": api_key}, + ) + assert response.status_code == HTTPStatus.NO_CONTENT + assert response.content == b"" + + exists = await user_test.execute( + text("SELECT 1 FROM users WHERE id = :id LIMIT 1"), + parameters={"id": new_id}, + ) + assert exists.one_or_none() is None + + +@pytest.mark.mut +async def test_delete_user_api_success_admin_deletes_disposable_user( + py_api: httpx.AsyncClient, + user_test: AsyncConnection, +) -> None: + suffix = uuid.uuid4().hex[:12] + username = f"tmp_del_{suffix}" + email = f"{suffix}@openml-delete.test" + await user_test.execute( + text( + """ + INSERT INTO users ( + ip_address, username, password, email, created_on, + company, country, bio + ) VALUES ( + '127.0.0.1', :username, 'x', :email, UNIX_TIMESTAMP(), + '', '', '' + ) + """, + ), + parameters={"username": username, "email": email}, + ) + uid_row = await user_test.execute(text("SELECT LAST_INSERT_ID() AS id")) + (new_id,) = uid_row.one() + await user_test.execute( + text("INSERT INTO users_groups (user_id, group_id) VALUES (:uid, 2)"), + parameters={"uid": new_id}, + ) + + response = await py_api.delete( + f"/users/{new_id}", + params={"api_key": ApiKey.ADMIN}, + ) + assert response.status_code == HTTPStatus.NO_CONTENT + assert response.content == b"" + + exists = await user_test.execute( + text("SELECT 1 FROM users WHERE id = :id LIMIT 1"), + parameters={"id": new_id}, + ) + assert exists.one_or_none() is None + + +# ── Direct handler tests ── + + +async def test_delete_user_direct_not_found( + user_test: AsyncConnection, + expdb_test: AsyncConnection, +) -> None: + with pytest.raises(UserNotFoundError, match=r"User 888888888 not found\."): + await delete_user_account( + user_id=888888888, + current_user=ADMIN_USER, + expdb=expdb_test, + userdb=user_test, + ) + + +async def test_delete_user_direct_forbidden( + user_test: AsyncConnection, + expdb_test: AsyncConnection, +) -> None: + with pytest.raises(ForbiddenError, match=r"You may only delete your own user account\."): + await delete_user_account( + user_id=ADMIN_USER.user_id, + current_user=SOME_USER, + expdb=expdb_test, + userdb=user_test, + ) + + +async def test_delete_user_direct_conflict_has_resources( + user_test: AsyncConnection, + expdb_test: AsyncConnection, +) -> None: + with pytest.raises(AccountHasResourcesError, match="Cannot delete this account"): + await delete_user_account( + user_id=16, + current_user=ADMIN_USER, + expdb=expdb_test, + userdb=user_test, + ) From e3b016dae79d918e8549aa6963229ded61905090 Mon Sep 17 00:00:00 2001 From: igennova Date: Mon, 20 Apr 2026 03:42:08 +0530 Subject: [PATCH 2/8] query update --- src/database/users.py | 29 ++++++++++++++++------- src/routers/openml/users.py | 23 +++++++++++------- tests/routers/openml/users_delete_test.py | 9 +++---- 3 files changed, 40 insertions(+), 21 deletions(-) diff --git a/src/database/users.py b/src/database/users.py index 2507abfd..4f539685 100644 --- a/src/database/users.py +++ b/src/database/users.py @@ -86,22 +86,33 @@ async def exists_by_id(*, user_id: int, connection: AsyncConnection) -> bool: return row.one_or_none() is not None -async def count_uploaded_resources(*, user_id: int, expdb: AsyncConnection) -> int: - """Count datasets, flows, runs, and studies uploaded or created by this user (expdb).""" +async def has_user_references(*, user_id: int, expdb: AsyncConnection) -> bool: + """Return ``True`` if any ``expdb`` row still references ``user_id``.""" row = await expdb.execute( text( """ - SELECT ( - (SELECT COUNT(*) FROM dataset WHERE uploader = :uid) - + (SELECT COUNT(*) FROM implementation WHERE uploader = :uid) - + (SELECT COUNT(*) FROM `run` WHERE uploader = :uid) - + (SELECT COUNT(*) FROM study WHERE creator = :uid) - ) AS total + SELECT EXISTS ( + SELECT 1 FROM dataset WHERE uploader = :uid + UNION ALL SELECT 1 FROM dataset_description WHERE uploader = :uid + UNION ALL SELECT 1 FROM dataset_status WHERE user_id = :uid + UNION ALL SELECT 1 FROM dataset_tag WHERE uploader = :uid + UNION ALL SELECT 1 FROM dataset_topic WHERE uploader = :uid + UNION ALL SELECT 1 FROM implementation WHERE uploader = :uid + UNION ALL SELECT 1 FROM implementation_tag WHERE uploader = :uid + UNION ALL SELECT 1 FROM `run` WHERE uploader = :uid + UNION ALL SELECT 1 FROM run_study WHERE uploader = :uid + UNION ALL SELECT 1 FROM run_tag WHERE uploader = :uid + UNION ALL SELECT 1 FROM setup_tag WHERE uploader = :uid + UNION ALL SELECT 1 FROM study WHERE creator = :uid + UNION ALL SELECT 1 FROM task WHERE creator = :uid + UNION ALL SELECT 1 FROM task_study WHERE uploader = :uid + UNION ALL SELECT 1 FROM task_tag WHERE uploader = :uid + ) AS has_refs """, ), parameters={"uid": user_id}, ) - return int(row.scalar_one()) + return bool(row.scalar_one()) async def delete_user_rows(*, user_id: int, userdb: AsyncConnection) -> None: diff --git a/src/routers/openml/users.py b/src/routers/openml/users.py index 838f8703..40a0baa3 100644 --- a/src/routers/openml/users.py +++ b/src/routers/openml/users.py @@ -3,6 +3,7 @@ from typing import Annotated from fastapi import APIRouter, Depends, Path, Response +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncConnection import database.users @@ -10,6 +11,11 @@ from database.users import User from routers.dependencies import expdb_connection, fetch_user_or_raise, userdb_connection +_ACCOUNT_HAS_RESOURCES_MSG = ( + "Cannot delete this account while records still reference the user " + "(datasets, flows, runs, studies, tags, etc.). Remove or transfer them first." +) + router = APIRouter(prefix="/users", tags=["users"]) @@ -42,13 +48,14 @@ async def delete_user_account( msg = f"User {user_id} not found." raise UserNotFoundError(msg) - resource_count = await database.users.count_uploaded_resources(user_id=user_id, expdb=expdb) - if resource_count > 0: - msg = ( - "Cannot delete this account while datasets, flows, runs, or studies " - "are still associated with the user. Remove or transfer them first." - ) - raise AccountHasResourcesError(msg) + if await database.users.has_user_references(user_id=user_id, expdb=expdb): + raise AccountHasResourcesError(_ACCOUNT_HAS_RESOURCES_MSG) + + try: + await database.users.delete_user_rows(user_id=user_id, userdb=userdb) + except IntegrityError as exc: + # Safety net: a user-reference table was added without updating + # ``has_user_references``. Surface a clean 409 instead of a 500. + raise AccountHasResourcesError(_ACCOUNT_HAS_RESOURCES_MSG) from exc - await database.users.delete_user_rows(user_id=user_id, userdb=userdb) return Response(status_code=204) diff --git a/tests/routers/openml/users_delete_test.py b/tests/routers/openml/users_delete_test.py index 7903c69b..407a427b 100644 --- a/tests/routers/openml/users_delete_test.py +++ b/tests/routers/openml/users_delete_test.py @@ -9,6 +9,7 @@ from sqlalchemy.ext.asyncio import AsyncConnection from core.errors import AccountHasResourcesError, ForbiddenError, UserNotFoundError +from database.users import UserGroup from routers.openml.users import delete_user_account from tests.users import ADMIN_USER, SOME_USER, ApiKey @@ -88,8 +89,8 @@ async def test_delete_user_api_success_self_delete( uid_row = await user_test.execute(text("SELECT LAST_INSERT_ID() AS id")) (new_id,) = uid_row.one() await user_test.execute( - text("INSERT INTO users_groups (user_id, group_id) VALUES (:uid, 2)"), - parameters={"uid": new_id}, + text("INSERT INTO users_groups (user_id, group_id) VALUES (:uid, :gid)"), + parameters={"uid": new_id, "gid": UserGroup.READ_WRITE.value}, ) response = await py_api.delete( @@ -131,8 +132,8 @@ async def test_delete_user_api_success_admin_deletes_disposable_user( uid_row = await user_test.execute(text("SELECT LAST_INSERT_ID() AS id")) (new_id,) = uid_row.one() await user_test.execute( - text("INSERT INTO users_groups (user_id, group_id) VALUES (:uid, 2)"), - parameters={"uid": new_id}, + text("INSERT INTO users_groups (user_id, group_id) VALUES (:uid, :gid)"), + parameters={"uid": new_id, "gid": UserGroup.READ_WRITE.value}, ) response = await py_api.delete( From 96f31223d7c0535fd3ffacd4d1074eb091784c61 Mon Sep 17 00:00:00 2001 From: igennova Date: Tue, 28 Apr 2026 22:53:51 +0530 Subject: [PATCH 3/8] suggested changes --- src/routers/openml/users.py | 15 +- tests/routers/openml/users_delete_test.py | 178 ++++++++++++---------- 2 files changed, 109 insertions(+), 84 deletions(-) diff --git a/src/routers/openml/users.py b/src/routers/openml/users.py index 40a0baa3..821ea5e8 100644 --- a/src/routers/openml/users.py +++ b/src/routers/openml/users.py @@ -3,6 +3,7 @@ from typing import Annotated from fastapi import APIRouter, Depends, Path, Response +from loguru import logger from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncConnection @@ -35,10 +36,11 @@ async def delete_user_account( expdb: Annotated[AsyncConnection, Depends(expdb_connection)], userdb: Annotated[AsyncConnection, Depends(userdb_connection)], ) -> Response: - """Delete a user account when they have no uploaded resources (Phase 1). + """Delete the user account if they have no associated resources. - Callers may delete their own account. Administrators may delete any account - that satisfies the no-resources rule. + The account to be deleted must not have associated resources (such as + datasets, tasks, or tags). Users may only delete their own account. + Administrators may delete any account that satisfies the no-resources rule. """ if current_user.user_id != user_id and not await current_user.is_admin(): msg = "You may only delete your own user account." @@ -54,8 +56,11 @@ async def delete_user_account( try: await database.users.delete_user_rows(user_id=user_id, userdb=userdb) except IntegrityError as exc: - # Safety net: a user-reference table was added without updating - # ``has_user_references``. Surface a clean 409 instead of a 500. + logger.error( + "Delete of user {user_id} failed with integrity error after pre-check.", + user_id=user_id, + ) raise AccountHasResourcesError(_ACCOUNT_HAS_RESOURCES_MSG) from exc + logger.info("User account {user_id} was removed.", user_id=user_id) return Response(status_code=204) diff --git a/tests/routers/openml/users_delete_test.py b/tests/routers/openml/users_delete_test.py index 407a427b..7780ca1e 100644 --- a/tests/routers/openml/users_delete_test.py +++ b/tests/routers/openml/users_delete_test.py @@ -1,11 +1,15 @@ """Tests for DELETE /users/{user_id} (Phase 1: no resources, self or admin).""" import uuid +from collections.abc import AsyncGenerator from http import HTTPStatus +from typing import NamedTuple import httpx import pytest +import pytest_mock from sqlalchemy import text +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncConnection from core.errors import AccountHasResourcesError, ForbiddenError, UserNotFoundError @@ -22,56 +26,18 @@ async def test_delete_user_missing_auth(py_api: httpx.AsyncClient) -> None: assert body["detail"] == "No API key provided." -async def test_delete_user_not_found(py_api: httpx.AsyncClient) -> None: - response = await py_api.delete( - "/users/999999999", - params={"api_key": ApiKey.ADMIN}, - ) - assert response.status_code == HTTPStatus.NOT_FOUND - assert response.headers["content-type"] == "application/problem+json" - body = response.json() - assert body["type"] == UserNotFoundError.uri - assert body["detail"] == "User 999999999 not found." - - -async def test_delete_user_forbidden_non_admin_deletes_other( - py_api: httpx.AsyncClient, -) -> None: - response = await py_api.delete( - f"/users/{ADMIN_USER.user_id}", - params={"api_key": ApiKey.SOME_USER}, - ) - assert response.status_code == HTTPStatus.FORBIDDEN - body = response.json() - assert body["type"] == ForbiddenError.uri - assert body["detail"] == "You may only delete your own user account." +class DisposableUser(NamedTuple): + user_id: int + api_key: str -async def test_delete_user_conflict_when_user_has_resources( - py_api: httpx.AsyncClient, -) -> None: - """User 16 owns dataset 130 in the test database.""" - response = await py_api.delete( - "/users/16", - params={"api_key": ApiKey.ADMIN}, - ) - assert response.status_code == HTTPStatus.CONFLICT - assert response.headers["content-type"] == "application/problem+json" - body = response.json() - assert body["type"] == AccountHasResourcesError.uri - assert "datasets" in body["detail"] - - -@pytest.mark.mut -async def test_delete_user_api_success_self_delete( - py_api: httpx.AsyncClient, - user_test: AsyncConnection, -) -> None: - """Disposable user deletes their own account using their API key (session_hash).""" - api_key = "fedcba9876543210fedcba9876543210" +@pytest.fixture +async def disposable_user(user_test: AsyncConnection) -> AsyncGenerator[DisposableUser]: + api_key = uuid.uuid4().hex suffix = uuid.uuid4().hex[:10] - username = f"tmp_self_{suffix}" - email = f"{suffix}@openml-self-delete.test" + username = f"tmp_user_{suffix}" + email = f"{suffix}@openml-delete.test" + await user_test.execute( text( """ @@ -92,17 +58,33 @@ async def test_delete_user_api_success_self_delete( text("INSERT INTO users_groups (user_id, group_id) VALUES (:uid, :gid)"), parameters={"uid": new_id, "gid": UserGroup.READ_WRITE.value}, ) + yield DisposableUser(user_id=new_id, api_key=api_key) + await user_test.execute( + text("DELETE FROM users_groups WHERE user_id = :uid"), + parameters={"uid": new_id}, + ) + await user_test.execute( + text("DELETE FROM users WHERE id = :uid"), + parameters={"uid": new_id}, + ) + +@pytest.mark.mut +async def test_delete_user_api_success_self_delete( + py_api: httpx.AsyncClient, + user_test: AsyncConnection, + disposable_user: DisposableUser, +) -> None: response = await py_api.delete( - f"/users/{new_id}", - params={"api_key": api_key}, + f"/users/{disposable_user.user_id}", + params={"api_key": disposable_user.api_key}, ) assert response.status_code == HTTPStatus.NO_CONTENT assert response.content == b"" exists = await user_test.execute( text("SELECT 1 FROM users WHERE id = :id LIMIT 1"), - parameters={"id": new_id}, + parameters={"id": disposable_user.user_id}, ) assert exists.one_or_none() is None @@ -111,33 +93,10 @@ async def test_delete_user_api_success_self_delete( async def test_delete_user_api_success_admin_deletes_disposable_user( py_api: httpx.AsyncClient, user_test: AsyncConnection, + disposable_user: DisposableUser, ) -> None: - suffix = uuid.uuid4().hex[:12] - username = f"tmp_del_{suffix}" - email = f"{suffix}@openml-delete.test" - await user_test.execute( - text( - """ - INSERT INTO users ( - ip_address, username, password, email, created_on, - company, country, bio - ) VALUES ( - '127.0.0.1', :username, 'x', :email, UNIX_TIMESTAMP(), - '', '', '' - ) - """, - ), - parameters={"username": username, "email": email}, - ) - uid_row = await user_test.execute(text("SELECT LAST_INSERT_ID() AS id")) - (new_id,) = uid_row.one() - await user_test.execute( - text("INSERT INTO users_groups (user_id, group_id) VALUES (:uid, :gid)"), - parameters={"uid": new_id, "gid": UserGroup.READ_WRITE.value}, - ) - response = await py_api.delete( - f"/users/{new_id}", + f"/users/{disposable_user.user_id}", params={"api_key": ApiKey.ADMIN}, ) assert response.status_code == HTTPStatus.NO_CONTENT @@ -145,7 +104,7 @@ async def test_delete_user_api_success_admin_deletes_disposable_user( exists = await user_test.execute( text("SELECT 1 FROM users WHERE id = :id LIMIT 1"), - parameters={"id": new_id}, + parameters={"id": disposable_user.user_id}, ) assert exists.one_or_none() is None @@ -157,36 +116,97 @@ async def test_delete_user_direct_not_found( user_test: AsyncConnection, expdb_test: AsyncConnection, ) -> None: - with pytest.raises(UserNotFoundError, match=r"User 888888888 not found\."): + with pytest.raises(UserNotFoundError, match=r"User 888888888 not found\.") as exc_info: await delete_user_account( user_id=888888888, current_user=ADMIN_USER, expdb=expdb_test, userdb=user_test, ) + assert exc_info.value.status_code == HTTPStatus.NOT_FOUND + assert exc_info.value.uri == UserNotFoundError.uri async def test_delete_user_direct_forbidden( user_test: AsyncConnection, expdb_test: AsyncConnection, ) -> None: - with pytest.raises(ForbiddenError, match=r"You may only delete your own user account\."): + with pytest.raises( + ForbiddenError, match=r"You may only delete your own user account\." + ) as exc_info: await delete_user_account( user_id=ADMIN_USER.user_id, current_user=SOME_USER, expdb=expdb_test, userdb=user_test, ) + assert exc_info.value.status_code == HTTPStatus.FORBIDDEN + assert exc_info.value.uri == ForbiddenError.uri async def test_delete_user_direct_conflict_has_resources( user_test: AsyncConnection, expdb_test: AsyncConnection, ) -> None: - with pytest.raises(AccountHasResourcesError, match="Cannot delete this account"): + with pytest.raises(AccountHasResourcesError, match="Cannot delete this account") as exc_info: await delete_user_account( user_id=16, current_user=ADMIN_USER, expdb=expdb_test, userdb=user_test, ) + assert exc_info.value.status_code == HTTPStatus.CONFLICT + assert exc_info.value.uri == AccountHasResourcesError.uri + + +@pytest.mark.mut +async def test_delete_user_direct_success_logs_info( + user_test: AsyncConnection, + expdb_test: AsyncConnection, + disposable_user: DisposableUser, + mocker: pytest_mock.MockerFixture, +) -> None: + log_info = mocker.patch("routers.openml.users.logger.info") + + response = await delete_user_account( + user_id=disposable_user.user_id, + current_user=ADMIN_USER, + expdb=expdb_test, + userdb=user_test, + ) + + assert response.status_code == HTTPStatus.NO_CONTENT + log_info.assert_called_once_with( + "User account {user_id} was removed.", + user_id=disposable_user.user_id, + ) + + +@pytest.mark.mut +async def test_delete_user_integrity_error_logs_and_raises_conflict( + user_test: AsyncConnection, + expdb_test: AsyncConnection, + disposable_user: DisposableUser, + mocker: pytest_mock.MockerFixture, +) -> None: + mocker.patch( + "database.users.delete_user_rows", + side_effect=IntegrityError( + "DELETE FROM users", {"user_id": disposable_user.user_id}, Exception("fk") + ), + ) + log_error = mocker.patch("routers.openml.users.logger.error") + + with pytest.raises(AccountHasResourcesError, match="Cannot delete this account") as exc_info: + await delete_user_account( + user_id=disposable_user.user_id, + current_user=ADMIN_USER, + expdb=expdb_test, + userdb=user_test, + ) + + assert exc_info.value.status_code == HTTPStatus.CONFLICT + log_error.assert_called_once_with( + "Delete of user {user_id} failed with integrity error after pre-check.", + user_id=disposable_user.user_id, + ) From 0b615b986e544548525b1bd862fda53a1edf5667 Mon Sep 17 00:00:00 2001 From: igennova Date: Thu, 7 May 2026 20:28:38 +0530 Subject: [PATCH 4/8] Refine user deletion tests and logging --- src/routers/openml/users.py | 17 ++++--- tests/routers/openml/users_delete_test.py | 60 ++++++++++------------- 2 files changed, 36 insertions(+), 41 deletions(-) diff --git a/src/routers/openml/users.py b/src/routers/openml/users.py index 821ea5e8..26c249b9 100644 --- a/src/routers/openml/users.py +++ b/src/routers/openml/users.py @@ -1,5 +1,6 @@ """User account HTTP endpoints.""" +from http import HTTPStatus from typing import Annotated from fastapi import APIRouter, Depends, Path, Response @@ -23,11 +24,13 @@ @router.delete( "/{user_id}", responses={ - 204: {"description": "User account deleted."}, - 401: {"description": "Authentication failed or missing."}, - 403: {"description": "Not allowed to delete this account."}, - 404: {"description": "User id not found."}, - 409: {"description": "User still has datasets, flows, runs, or studies."}, + HTTPStatus.NO_CONTENT: {"description": "User account deleted."}, + HTTPStatus.UNAUTHORIZED: {"description": "Authentication failed or missing."}, + HTTPStatus.FORBIDDEN: {"description": "Not allowed to delete this account."}, + HTTPStatus.NOT_FOUND: {"description": "User id not found."}, + HTTPStatus.CONFLICT: { + "description": "User still has datasets, flows, runs, or studies.", + }, }, ) async def delete_user_account( @@ -42,6 +45,8 @@ async def delete_user_account( datasets, tasks, or tags). Users may only delete their own account. Administrators may delete any account that satisfies the no-resources rule. """ + # How to handle users that do have associated resources is an ongoing discussion, + # see also: https://github.com/openml/server-api/issues/194 if current_user.user_id != user_id and not await current_user.is_admin(): msg = "You may only delete your own user account." raise ForbiddenError(msg) @@ -63,4 +68,4 @@ async def delete_user_account( raise AccountHasResourcesError(_ACCOUNT_HAS_RESOURCES_MSG) from exc logger.info("User account {user_id} was removed.", user_id=user_id) - return Response(status_code=204) + return Response(status_code=HTTPStatus.NO_CONTENT) diff --git a/tests/routers/openml/users_delete_test.py b/tests/routers/openml/users_delete_test.py index 7780ca1e..01b2c837 100644 --- a/tests/routers/openml/users_delete_test.py +++ b/tests/routers/openml/users_delete_test.py @@ -1,7 +1,6 @@ -"""Tests for DELETE /users/{user_id} (Phase 1: no resources, self or admin).""" +"""Tests for DELETE /users/{user_id}.""" import uuid -from collections.abc import AsyncGenerator from http import HTTPStatus from typing import NamedTuple @@ -15,7 +14,7 @@ from core.errors import AccountHasResourcesError, ForbiddenError, UserNotFoundError from database.users import UserGroup from routers.openml.users import delete_user_account -from tests.users import ADMIN_USER, SOME_USER, ApiKey +from tests.users import ADMIN_USER, OWNER_USER, SOME_USER, ApiKey async def test_delete_user_missing_auth(py_api: httpx.AsyncClient) -> None: @@ -32,7 +31,7 @@ class DisposableUser(NamedTuple): @pytest.fixture -async def disposable_user(user_test: AsyncConnection) -> AsyncGenerator[DisposableUser]: +async def disposable_user(user_test: AsyncConnection) -> DisposableUser: api_key = uuid.uuid4().hex suffix = uuid.uuid4().hex[:10] username = f"tmp_user_{suffix}" @@ -58,15 +57,9 @@ async def disposable_user(user_test: AsyncConnection) -> AsyncGenerator[Disposab text("INSERT INTO users_groups (user_id, group_id) VALUES (:uid, :gid)"), parameters={"uid": new_id, "gid": UserGroup.READ_WRITE.value}, ) - yield DisposableUser(user_id=new_id, api_key=api_key) - await user_test.execute( - text("DELETE FROM users_groups WHERE user_id = :uid"), - parameters={"uid": new_id}, - ) - await user_test.execute( - text("DELETE FROM users WHERE id = :uid"), - parameters={"uid": new_id}, - ) + return DisposableUser(user_id=new_id, api_key=api_key) + # No explicit teardown: the ``user_test`` fixture rolls back at the end + # of the test, which removes the rows inserted above. @pytest.mark.mut @@ -74,7 +67,10 @@ async def test_delete_user_api_success_self_delete( py_api: httpx.AsyncClient, user_test: AsyncConnection, disposable_user: DisposableUser, + mocker: pytest_mock.MockerFixture, ) -> None: + log_info = mocker.patch("routers.openml.users.logger.info") + response = await py_api.delete( f"/users/{disposable_user.user_id}", params={"api_key": disposable_user.api_key}, @@ -88,6 +84,11 @@ async def test_delete_user_api_success_self_delete( ) assert exists.one_or_none() is None + log_info.assert_any_call( + "User account {user_id} was removed.", + user_id=disposable_user.user_id, + ) + @pytest.mark.mut async def test_delete_user_api_success_admin_deletes_disposable_user( @@ -143,6 +144,12 @@ async def test_delete_user_direct_forbidden( assert exc_info.value.status_code == HTTPStatus.FORBIDDEN assert exc_info.value.uri == ForbiddenError.uri + admin_row = await user_test.execute( + text("SELECT 1 FROM users WHERE id = :id LIMIT 1"), + parameters={"id": ADMIN_USER.user_id}, + ) + assert admin_row.one_or_none() is not None + async def test_delete_user_direct_conflict_has_resources( user_test: AsyncConnection, @@ -150,7 +157,7 @@ async def test_delete_user_direct_conflict_has_resources( ) -> None: with pytest.raises(AccountHasResourcesError, match="Cannot delete this account") as exc_info: await delete_user_account( - user_id=16, + user_id=OWNER_USER.user_id, current_user=ADMIN_USER, expdb=expdb_test, userdb=user_test, @@ -158,28 +165,11 @@ async def test_delete_user_direct_conflict_has_resources( assert exc_info.value.status_code == HTTPStatus.CONFLICT assert exc_info.value.uri == AccountHasResourcesError.uri - -@pytest.mark.mut -async def test_delete_user_direct_success_logs_info( - user_test: AsyncConnection, - expdb_test: AsyncConnection, - disposable_user: DisposableUser, - mocker: pytest_mock.MockerFixture, -) -> None: - log_info = mocker.patch("routers.openml.users.logger.info") - - response = await delete_user_account( - user_id=disposable_user.user_id, - current_user=ADMIN_USER, - expdb=expdb_test, - userdb=user_test, - ) - - assert response.status_code == HTTPStatus.NO_CONTENT - log_info.assert_called_once_with( - "User account {user_id} was removed.", - user_id=disposable_user.user_id, + owner_row = await user_test.execute( + text("SELECT 1 FROM users WHERE id = :id LIMIT 1"), + parameters={"id": OWNER_USER.user_id}, ) + assert owner_row.one_or_none() is not None @pytest.mark.mut From 9d29e6bb7614abcd5e09dd45539592e63691b7ed Mon Sep 17 00:00:00 2001 From: igennova Date: Thu, 7 May 2026 21:23:13 +0530 Subject: [PATCH 5/8] Ignore TC002 (false positive on FastAPI Annotated[] / fixtures) --- pyproject.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/pyproject.toml b/pyproject.toml index d8078bf1..0054fd29 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -67,6 +67,7 @@ ignore = [ "D213", # Linter does not detect when types are used for Pydantic "TC001", + "TC002", "TC003", ] From a940c43f834a7ab3f9243dbdc7389b92ea96dfe5 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 7 May 2026 16:06:09 +0000 Subject: [PATCH 6/8] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/conftest.py | 4 ++-- tests/routers/openml/datasets_list_datasets_test.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 23a52955..3e4c5ff4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,8 +7,8 @@ import _pytest.mark import httpx import pytest -from _pytest.config import Config # noqa: TC002 used during collection by Pytest -from _pytest.nodes import Item # noqa: TC002 used during collection by Pytest +from _pytest.config import Config +from _pytest.nodes import Item from asgi_lifespan import LifespanManager from sqlalchemy import text diff --git a/tests/routers/openml/datasets_list_datasets_test.py b/tests/routers/openml/datasets_list_datasets_test.py index 868e9ee6..fc2fe6ee 100644 --- a/tests/routers/openml/datasets_list_datasets_test.py +++ b/tests/routers/openml/datasets_list_datasets_test.py @@ -2,7 +2,7 @@ from http import HTTPStatus from typing import TYPE_CHECKING, Any -import httpx # noqa: TC002 is used in a function signature inspected at runtime +import httpx import hypothesis import pytest from hypothesis import given From ee84cb5e288158b2ef60dece24a7978846e934fa Mon Sep 17 00:00:00 2001 From: igennova Date: Thu, 7 May 2026 21:41:00 +0530 Subject: [PATCH 7/8] Use per-line TC002 noqa instead of global ignore --- pyproject.toml | 1 - src/routers/openml/users.py | 2 +- tests/routers/openml/users_delete_test.py | 6 +++--- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index c5b6cd92..e1b7e609 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -67,7 +67,6 @@ ignore = [ "D213", # Linter does not detect when types are used for Pydantic "TC001", - "TC002", "TC003", ] diff --git a/src/routers/openml/users.py b/src/routers/openml/users.py index 26c249b9..43ea9bbd 100644 --- a/src/routers/openml/users.py +++ b/src/routers/openml/users.py @@ -6,7 +6,7 @@ from fastapi import APIRouter, Depends, Path, Response from loguru import logger from sqlalchemy.exc import IntegrityError -from sqlalchemy.ext.asyncio import AsyncConnection +from sqlalchemy.ext.asyncio import AsyncConnection # noqa: TC002 used at runtime by FastAPI Depends import database.users from core.errors import AccountHasResourcesError, ForbiddenError, UserNotFoundError diff --git a/tests/routers/openml/users_delete_test.py b/tests/routers/openml/users_delete_test.py index 01b2c837..127e89e7 100644 --- a/tests/routers/openml/users_delete_test.py +++ b/tests/routers/openml/users_delete_test.py @@ -4,12 +4,12 @@ from http import HTTPStatus from typing import NamedTuple -import httpx +import httpx # noqa: TC002 used at runtime by pytest fixtures import pytest -import pytest_mock +import pytest_mock # noqa: TC002 used at runtime by pytest fixtures from sqlalchemy import text from sqlalchemy.exc import IntegrityError -from sqlalchemy.ext.asyncio import AsyncConnection +from sqlalchemy.ext.asyncio import AsyncConnection # noqa: TC002 used at runtime by pytest fixtures from core.errors import AccountHasResourcesError, ForbiddenError, UserNotFoundError from database.users import UserGroup From 168f5506289e41b6aaa8905db192b64c9abff133 Mon Sep 17 00:00:00 2001 From: igennova Date: Thu, 7 May 2026 21:47:12 +0530 Subject: [PATCH 8/8] Restore TC002 noqa stripped by pre-commit.ci autofix --- tests/conftest.py | 4 ++-- tests/routers/openml/datasets_list_datasets_test.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 3e4c5ff4..23a52955 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -7,8 +7,8 @@ import _pytest.mark import httpx import pytest -from _pytest.config import Config -from _pytest.nodes import Item +from _pytest.config import Config # noqa: TC002 used during collection by Pytest +from _pytest.nodes import Item # noqa: TC002 used during collection by Pytest from asgi_lifespan import LifespanManager from sqlalchemy import text diff --git a/tests/routers/openml/datasets_list_datasets_test.py b/tests/routers/openml/datasets_list_datasets_test.py index fc2fe6ee..868e9ee6 100644 --- a/tests/routers/openml/datasets_list_datasets_test.py +++ b/tests/routers/openml/datasets_list_datasets_test.py @@ -2,7 +2,7 @@ from http import HTTPStatus from typing import TYPE_CHECKING, Any -import httpx +import httpx # noqa: TC002 is used in a function signature inspected at runtime import hypothesis import pytest from hypothesis import given