Extract User CRUD out of the crud/__init__.py monolith - #2458
Conversation
Move the seven user functions out of crud/__init__.py into crud/user.py, following the split already done for source, task, tag and others. Consumers import the module directly; nothing is re-exported. All seven have callers, so nothing was dropped. get_user and get_user_by_id do overlap, but both are live: get_user is the tenant-filtered lookup used by the user router and Celery tasks, while get_user_by_id is the unfiltered auth lookup that runs before a tenant context exists. The function-local imports in get_user_by_email and delete_user (func, flag_modified, ProjectMembership, bypass_tenant_filter) move to module level -- none of those modules import crud back. create_user's import of on_user_org_assigned is lifted too; auth/org_membership_hook is deliberately dependency-free so it can be imported from core. Patch targets in the tests follow the functions to their new module, and test_explore.py / test_progress.py now patch explore.user_crud instead of explore.crud, which would otherwise have failed loudly on a missing attribute.
6e2ff1b to
4d92a09
Compare
![peqy[bot]](https://avatars.githubusercontent.com/in/1930020?s=60&v=4){kind=small}
There was a problem hiding this comment.
Extract looks solid and consistent with the ongoing crud split.
[Improvement] refresh_tokens() passes str(token_row.user_id) into user_crud.get_user(), which doesn’t coerce IDs before doing User.id == .... Safer to pass the UUID through (or use get_user_by_id).
[Improvement] crud/user.py::delete_user() commits internally while other CRUD helpers rely on the session context manager for commit/rollback. If this MR is meant to be a pure move, consider keeping commit ownership with callers (or making the pattern consistent).
Found 2 issues (0 critical, 2 improvements).
| raise | ||
|
|
||
| user = crud.get_user(db, str(token_row.user_id)) | ||
| user = user_crud.get_user(db, str(token_row.user_id)) |
There was a problem hiding this comment.
[Improvement] Avoid passing a string UUID into get_user here
user_crud.get_user ultimately does User.id == <passed value> without coercing the ID, so str(token_row.user_id) relies on driver/DB implicit casting. Since token_row.user_id is already a UUID, it’d be safer to pass it through directly (or just call user_crud.get_user_by_id(db, token_row.user_id)).
| ValueError: If user tries to delete themselves | ||
| """ | ||
| # Security check: Prevent users from deleting themselves | ||
| if str(target_user_id) == str(user_id): |
There was a problem hiding this comment.
[Improvement] Transaction management: avoid committing inside CRUD
delete_user() calls db.commit()/db.refresh(), while the other CRUD functions rely on the session context manager to commit/rollback. Since this file is described as a “pure move”, consider keeping the original contract and let callers own the commit (or at least be consistent across CRUD modules). Committing inside a CRUD helper can be surprising in larger transactions (e.g., when caller wants atomic multi-step changes).
get_user moved to crud/user.py in #2458, and reaching it through the crud package raises AttributeError unless something else imported the submodule first. Starting a tuning run for a metric with no model of its own hit that every time.
get_user moved to crud/user.py in #2458, and reaching it through the crud package raises AttributeError unless something else imported the submodule first. Starting a tuning run for a metric with no model of its own hit that every time.
1 participant
Purpose
crud/__init__.pyis a 1946-line monolith that the codebase is splitting one entity at a time, following #2411, #2412, #2438, #2439, #2441, #2442, #2449, #2450 and #2451. This takes the User block out. Perapps/backend/AGENTS.mdthe package only shrinks from here — nothing new goes back into__init__.py.What Changed
get_user,get_users,create_user,update_user,delete_user,get_user_by_emailandget_user_by_idinto a newcrud/user.py(198 lines). The block sat exactly between the Topic and Organization banners; Organization is untouched.crud/__init__.pydrops 154 lines.from rhesis.backend.app.crud import user as user_crud. The alias matters here —useris an extremely common local variable and parameter name in this codebase, so the call sites were read individually rather than find-and-replaced.get_userandget_user_by_idoverlap but are not interchangeable:get_useris the tenant-filtered lookup used by the user router, Celery tasks and task notifications, whileget_user_by_idis the unfiltered auth lookup that runs before a tenant context exists (session/JWT/token resolution, polyphemus).func,flag_modified,ProjectMembership,bypass_tenant_filter— pluson_user_org_assignedout ofcreate_user's body.auth/org_membership_hook.pyis deliberately dependency-free (from __future__ import annotationsand aTYPE_CHECKINGguard, no runtime imports beyond stdlib), so there is no cycle; verified by importing the module.from rhesis.backend.app import crudin 9 files. The rest still usecrudfor other entities.crud.create_user(inorg_membership_hook.py,local_init.py,ee/rbac/default_role.pyand two test files) tocrud.user.create_user, since they would otherwise name a function that no longer lives there.Additional Context
crud/__init__.py, andgit merge-treeconfirms__init__.pymerges clean across all six pairings.apps/polyphemus/andee/backend/as well as the backend app. Flagging rather than hiding it.services/explorer/embeddings.pyandutils/user_model_utils.py. Both are the same trivial shape — each branch inserts its owncrudsubmodule import at the same spot, so the resolution is to keep both lines. Once both land, the last barecrud.use disappears from each file and thefrom rhesis.backend.app import crudimport must be dropped (ruffF401).get_test_setsandget_tests, which Add metric tuning for custom metrics [feature branch] #2446 modifies.Testing
Pure move — behaviour is unchanged, so the existing suite is the check. User CRUD sits under the auth path, so this was run broadly rather than on the user tests alone.
6677 passed, 47 skipped, 1 xfailed, 0 failures — effectively the whole backend suite. Skips are pre-existing (deferred security tests, missing
PERSPECTIVE_API_KEY).Ruff was measured against a HEAD baseline of the same file set: no new check errors and no new format drift. The 12 remaining findings and 3 format-check failures are identical at HEAD and were left alone.
crud/user.pyitself is clean.A repo-wide grep for the seven old
crud.*names acrossapps/,tests/,sdk/,ee/,packages/,penelope/anddocs/returns nothing, over all file types rather than just.py. Confirmed at runtime too:hasattr(crud, 'get_user')andhasattr(crud, 'get_user_by_id')are bothFalse, and every touched module plusrhesis.backend.tasks,rhesis.backend.ee.sso.user_utilsand both polyphemus services import cleanly.Two things worth a reviewer's eye:
tests/backend/tasks/test_explore.pyandtests/backend/tasks/architect/test_progress.pypatchrhesis.backend.tasks.endpoint.explore.crudas a whole-module mock. Sinceexplore.pyno longer has acrudattribute, those 8 patches would have raisedAttributeError; they now targetexplore.user_crud.The
on_user_org_assignedimport lift is the one change that is not purely mechanical — if that local import was defensive on purpose rather than incidental, say so and it goes back.