Add DELETE /users/{id} with no-resources rule#317
Add DELETE /users/{id} with no-resources rule#317igennova wants to merge 10 commits intoopenml:mainfrom
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds account-deletion functionality: two new RFC 9457 ProblemDetailError subclasses ( Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The delete flow performs three separate DB operations (existence check, resource count, and deletion) without an explicit transaction, so consider wrapping these in a single transaction or using DB constraints to avoid race conditions where the user or their resources change between checks.
- In
delete_user_rows, the two DELETE statements could leave the DB in an inconsistent state if the second fails after the first succeeds; consider enforcing this via foreign keys with cascading deletes or executing both deletes within an explicit transaction block. - The tests insert users with hard-coded
group_id = 2; consider using a named constant or fixture for this group ID so that tests are less brittle to changes in group configuration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The delete flow performs three separate DB operations (existence check, resource count, and deletion) without an explicit transaction, so consider wrapping these in a single transaction or using DB constraints to avoid race conditions where the user or their resources change between checks.
- In `delete_user_rows`, the two DELETE statements could leave the DB in an inconsistent state if the second fails after the first succeeds; consider enforcing this via foreign keys with cascading deletes or executing both deletes within an explicit transaction block.
- The tests insert users with hard-coded `group_id = 2`; consider using a named constant or fixture for this group ID so that tests are less brittle to changes in group configuration.
## Individual Comments
### Comment 1
<location path="src/routers/openml/users.py" line_range="26-35" />
<code_context>
+async def delete_user_account(
</code_context>
<issue_to_address>
**issue (bug_risk):** There is a race window between checking for resources and deleting the account.
Because `count_uploaded_resources` and `delete_user_rows` operate on different DBs/connections (`expdb` vs `userdb`), we can’t easily enforce a single transaction, so another process could create new datasets/flows/runs/studies in between. Consider either documenting this as a best-effort check or tightening invariants so new resources cannot be created once deletion starts (e.g., DB constraints or a soft-delete flag plus background cleanup).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #317 +/- ##
==========================================
+ Coverage 94.40% 94.61% +0.21%
==========================================
Files 67 69 +2
Lines 3112 3234 +122
Branches 229 232 +3
==========================================
+ Hits 2938 3060 +122
Misses 111 111
Partials 63 63 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/routers/openml/users.py (2)
54-54: Nit: useHTTPStatus.NO_CONTENTinstead of the literal.Matches the idiom used elsewhere in this repo (and in the
responses=dict keys above this is fine as int, but the return value tends to use the enum).♻️ Proposed tweak
-from typing import Annotated +from http import HTTPStatus +from typing import Annotated @@ - await database.users.delete_user_rows(user_id=user_id, userdb=userdb) - return Response(status_code=204) + await database.users.delete_user_rows(user_id=user_id, userdb=userdb) + return Response(status_code=HTTPStatus.NO_CONTENT)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routers/openml/users.py` at line 54, Replace the literal 204 return in the users route (the line returning Response(status_code=204) in src/routers/openml/users.py) with the enum HTTPStatus.NO_CONTENT; also add or update the import from http import HTTPStatus at the top of the module if it’s missing so the Response uses HTTPStatus.NO_CONTENT instead of the integer literal.
26-53: Consider audit logging for admin-initiated deletions.Admin deletions of another user's account are a high-impact, irreversible action but there's no
logger.info(...)/ structured audit event on the success path. Since the rest of the codebase already usesloguruwith contextual binding, a single log line (withactor_id,target_id, whether it was self vs admin) would make this much easier to triage later without adding behavior changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routers/openml/users.py` around lines 26 - 53, Add an audit log on the success path of delete_user_account: after the permission checks and before/after calling database.users.delete_user_rows, emit a loguru structured/info event that records actor_id (current_user.user_id), target_id (user_id), and whether the action was self_deletion or admin_initiated (use current_user.is_admin() to determine). Use the same contextual binding pattern as other modules (bind actor_id/target_id) and ensure the log is only emitted for successful deletions performed by admins or users deleting their own account so it appears in audit traces without changing behavior.src/database/users.py (1)
107-116: Optional: guard against an unexpected multi-row match.
DELETE FROM users WHERE id = :user_idrelies onidbeing the PK (safe). Consider also assertingrowcount == 1after the second execute (or wrapping in aSELECT ... FOR UPDATEearlier) to make unexpected races (user already deleted betweenexists_by_idand here) observable — otherwise the endpoint silently returns 204 even if nothing was deleted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/users.py` around lines 107 - 116, In delete_user_rows, guard against unexpected multi/no-row deletes by inspecting the result of the second DELETE (the one against "users WHERE id = :user_id")—capture the execute return (from AsyncConnection.execute), check result.rowcount == 1 and if not either raise an appropriate exception or return an observable error; alternatively document or implement an earlier SELECT ... FOR UPDATE to lock/verify the user before deletion. Ensure you reference the second execute in delete_user_rows so races where the user was already removed do not silently succeed.tests/routers/openml/users_delete_test.py (2)
49-61: Fragile reliance on seeded test data (user 16 / dataset 130).The test depends on the test fixture DB having exactly "user 16 owns dataset 130". This works today but will silently break if the seed data is regenerated or re-numbered. Consider either:
- Creating a disposable user + inserting a minimal
datasetrow withuploader=new_idinside the test (mirrors the pattern used in the success tests), OR- Pulling the expected user/dataset IDs from a shared constants module so the linkage is discoverable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/users_delete_test.py` around lines 49 - 61, The test test_delete_user_conflict_when_user_has_resources currently depends on hard-coded seeded IDs (user 16 / dataset 130); change it to create its own disposable user and a minimal dataset tied to that user's id (set dataset.uploader = new_id) inside the test before calling py_api.delete (mirroring the pattern used in the successful delete tests), then assert the response is HTTPStatus.CONFLICT, content-type problem+json, body["type"] == AccountHasResourcesError.uri and that "datasets" appears in body["detail"]; alternatively, if your test suite provides a shared constants module for seeded IDs, replace the hard-coded 16/130 with those constants so the ownership link is explicit.
16-21: Minor: assertapplication/problem+jsoncontent-type here too.The other error-response tests (
test_delete_user_not_found,test_delete_user_conflict_when_user_has_resources) verify the RFC 9457 media type but this one does not, even though the same handler produces it. Adding the assertion gives full coverage of the error envelope.🧪 Proposed addition
assert response.status_code == HTTPStatus.UNAUTHORIZED + assert response.headers["content-type"] == "application/problem+json" body = response.json()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/users_delete_test.py` around lines 16 - 21, The test test_delete_user_missing_auth is missing an assertion that the error response uses the RFC 9457 media type; add an assertion after calling py_api.delete("/users/1") to verify the response Content-Type is "application/problem+json" (or startswith that value to tolerate charset), so the test mirrors the other error-response tests (test_delete_user_not_found, test_delete_user_conflict_when_user_has_resources) and fully covers the error envelope.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/database/users.py`:
- Around line 107-116: In delete_user_rows, guard against unexpected
multi/no-row deletes by inspecting the result of the second DELETE (the one
against "users WHERE id = :user_id")—capture the execute return (from
AsyncConnection.execute), check result.rowcount == 1 and if not either raise an
appropriate exception or return an observable error; alternatively document or
implement an earlier SELECT ... FOR UPDATE to lock/verify the user before
deletion. Ensure you reference the second execute in delete_user_rows so races
where the user was already removed do not silently succeed.
In `@src/routers/openml/users.py`:
- Line 54: Replace the literal 204 return in the users route (the line returning
Response(status_code=204) in src/routers/openml/users.py) with the enum
HTTPStatus.NO_CONTENT; also add or update the import from http import HTTPStatus
at the top of the module if it’s missing so the Response uses
HTTPStatus.NO_CONTENT instead of the integer literal.
- Around line 26-53: Add an audit log on the success path of
delete_user_account: after the permission checks and before/after calling
database.users.delete_user_rows, emit a loguru structured/info event that
records actor_id (current_user.user_id), target_id (user_id), and whether the
action was self_deletion or admin_initiated (use current_user.is_admin() to
determine). Use the same contextual binding pattern as other modules (bind
actor_id/target_id) and ensure the log is only emitted for successful deletions
performed by admins or users deleting their own account so it appears in audit
traces without changing behavior.
In `@tests/routers/openml/users_delete_test.py`:
- Around line 49-61: The test test_delete_user_conflict_when_user_has_resources
currently depends on hard-coded seeded IDs (user 16 / dataset 130); change it to
create its own disposable user and a minimal dataset tied to that user's id (set
dataset.uploader = new_id) inside the test before calling py_api.delete
(mirroring the pattern used in the successful delete tests), then assert the
response is HTTPStatus.CONFLICT, content-type problem+json, body["type"] ==
AccountHasResourcesError.uri and that "datasets" appears in body["detail"];
alternatively, if your test suite provides a shared constants module for seeded
IDs, replace the hard-coded 16/130 with those constants so the ownership link is
explicit.
- Around line 16-21: The test test_delete_user_missing_auth is missing an
assertion that the error response uses the RFC 9457 media type; add an assertion
after calling py_api.delete("/users/1") to verify the response Content-Type is
"application/problem+json" (or startswith that value to tolerate charset), so
the test mirrors the other error-response tests (test_delete_user_not_found,
test_delete_user_conflict_when_user_has_resources) and fully covers the error
envelope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aadbfb3f-753a-450b-933c-0489450c1f46
📒 Files selected for processing (5)
src/core/errors.pysrc/database/users.pysrc/main.pysrc/routers/openml/users.pytests/routers/openml/users_delete_test.py
|
@PGijsbers |
PGijsbers
left a comment
PGijsbers
left a comment
There was a problem hiding this comment.
Thank you very much! Only some minor remarks.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/database/users.py (1)
23-26: Use plain integers for theIntEnummembers for clarity.While the tuple syntax
(1,)technically works—IntEnum extracts the single integer value—it's unconventional and obscures the intent. The code successfully callsUserGroup(group_id)and accesses.value, but writing tuple literals for single integers is not idiomatic and makes the code harder to read.Suggested fix
class UserGroup(IntEnum): - ADMIN = (1,) - READ_WRITE = (2,) - READ_ONLY = (3,) + ADMIN = 1 + READ_WRITE = 2 + READ_ONLY = 3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/users.py` around lines 23 - 26, The IntEnum members are defined with tuple literals which is non-idiomatic; update UserGroup to use plain integer literals (e.g., set ADMIN = 1, READ_WRITE = 2, READ_ONLY = 3) so UserGroup(group_id) and .value continue to work and the intent is clear; modify the UserGroup class declaration accordingly without changing any call sites that use UserGroup(group_id) or access .value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/database/users.py`:
- Around line 23-26: The IntEnum members are defined with tuple literals which
is non-idiomatic; update UserGroup to use plain integer literals (e.g., set
ADMIN = 1, READ_WRITE = 2, READ_ONLY = 3) so UserGroup(group_id) and .value
continue to work and the intent is clear; modify the UserGroup class declaration
accordingly without changing any call sites that use UserGroup(group_id) or
access .value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1ae166be-499d-47b2-b851-315a97a476d3
📒 Files selected for processing (3)
src/database/users.pysrc/routers/openml/users.pytests/routers/openml/users_delete_test.py
I’ve addressed the minor remarks. |
|
@PGijsbers gentle ping on this |
PGijsbers
left a comment
PGijsbers
left a comment
There was a problem hiding this comment.
Sorry, caught a few more minor things this time around. Should be good afterwards.
|
pre-commit.ci autofix |
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/database/users.py" line_range="118-120" />
<code_context>
+ return bool(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},
</code_context>
<issue_to_address>
**issue (bug_risk):** Deleting from `users_groups` and `users` in two separate statements without a transaction can leave the database in a partially-updated state.
If the `users_groups` delete succeeds but the `users` delete fails, you’ll leave a user with no group memberships. Please either wrap both deletes in a single transaction or rely on `ON DELETE CASCADE` from `users` to `users_groups` to keep them atomic. If this function assumes a surrounding transaction, document that explicitly.
</issue_to_address>
### Comment 2
<location path="src/routers/openml/users.py" line_range="61-68" />
<code_context>
+ 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:
+ 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)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The IntegrityError handling logs limited context and may make diagnosing unexpected integrity issues harder.
Currently the log omits the exception / constraint details and all integrity errors are mapped to `AccountHasResourcesError`, which can hide non–resource-related integrity issues. Please log the full exception (e.g. via `logger.exception` or by including `exc` in the structured context) to retain stack trace and DB error message, and consider distinguishing expected FK violations from other integrity errors.
Suggested implementation:
```python
try:
await database.users.delete_user_rows(user_id=user_id, userdb=userdb)
except IntegrityError as exc:
logger.exception(
"Delete of user {user_id} failed with integrity error after pre-check.",
user_id=user_id,
)
if _is_foreign_key_integrity_error(exc):
# Expected case: the user still has resources referencing their account
raise AccountHasResourcesError(_ACCOUNT_HAS_RESOURCES_MSG) from exc
# Unexpected integrity error; let it propagate so it can be surfaced/handled upstream
raise
logger.info("User account {user_id} was removed.", user_id=user_id)
```
To fully implement the FK vs. non-FK distinction you will also need to:
1. Define `_is_foreign_key_integrity_error` in `src/routers/openml/users.py` (or in a shared DB utilities module and import it here). For PostgreSQL with `psycopg2`/`asyncpg`, a typical implementation is:
- Check `getattr(exc.orig, "pgcode", None) == "23503"` (foreign_key_violation).
- Optionally, narrow further using `exc.orig.diag.constraint_name` if you want only specific constraints to map to `AccountHasResourcesError`.
Example skeleton (to be placed at module level, outside any function):
```python
from sqlalchemy.exc import IntegrityError
def _is_foreign_key_integrity_error(exc: IntegrityError) -> bool:
orig = getattr(exc, "orig", None)
pgcode = getattr(orig, "pgcode", None)
return pgcode == "23503"
```
Adjust this logic if you use a different backend/driver (e.g. MySQL error codes, SQLite message introspection).
2. If you add `_is_foreign_key_integrity_error` to a different module (e.g. `src/db/utils.py`), import it at the top of `src/routers/openml/users.py` and update the call site accordingly.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
for more information, see https://pre-commit.ci
2 participants
Fix : #194
Description
Adds a new
DELETE /users/{user_id}endpoint (Phase 1) so users can deletetheir own OpenML account, and administrators can delete any account, as long
as the account has no uploaded resources (datasets, flows, runs, studies).
Phase 1
204 No Content— account deleted successfully401 Unauthorized— no / invalid API key403 Forbidden— non-admin tries to delete another user's account404 Not Found—user_iddoes not exist409 Conflict— user still has uploaded datasets / flows / runs / studies(RFC 9457
AccountHasResourcesError)