feat(users): admin user CRUD — create, edit & delete from /users/admin - #217
Merged
Conversation
Design for letting admins create, edit, and delete users from the admin page, beyond the existing invite-only flow. Claude-Session: https://claude.ai/code/session_01HvC9Rhjs2MGaGC3KRaKAfg
Replace bare assert with RuntimeError guard in service.create_user; wrap creator.id with str() in admin_create_user for consistency with sibling flows and the UserCreated.created_by field type. Claude-Session: https://claude.ai/code/session_01HvC9Rhjs2MGaGC3KRaKAfg
delete_user eager-loaded the user's roles (selectinload) and oauth_accounts (lazy=selectin), then bulk-deleted the users_user_role rows the unit of work was still tracking. On flush, session.delete(user) tried to delete those association rows per-row → 0 matched → StaleDataError → 500. It only bit a real request session deleting a user that actually had a role; the unit tests missed it because in-session User.roles is lazy=noload and loads empty. Load the user with roles + oauth_accounts forced to noload (empty without a query: nothing tracked to race, and no async lazy-load of the delete-orphan oauth cascade mid-flush), then bulk-delete all four child tables explicitly. session.delete(user) remains so get_db flags the session written and commits. Add a regression test that deletes a role-bearing user through the request session (verified: fails on the old code with the exact StaleDataError, passes on the fix). Found via live browser QA. gitignore the .qa scratch dir. Claude-Session: https://claude.ai/code/session_01HvC9Rhjs2MGaGC3KRaKAfg
Deploying simple-module-python with |
| Latest commit: | 08970f4 |
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c219b15a.simple-module-python.pages.dev |
| Branch Preview URL: | https://worktree-admin-user-crud.simple-module-python.pages.dev |
Uh oh!
There was an error while loading. Please reload this page.
Merged
3 tasks
antosubash added a commit
that referenced
this pull request
Jun 21, 2026
…ndering & asset caching (#218) * perf(users): stop loading oauth_accounts on every authenticated request User.oauth_accounts was lazy="selectin", so every plain select(User) — the auth middleware/provider on *every* authenticated request, plus admin user lists — fired an extra selectin query for OAuth accounts it never reads. Profiling /api/users/me under load (py-spy) showed the SQLAlchemy selectin loader at ~25% of CPU; SQL echo confirmed 7 queries per request, 2 of them redundant users_oauth_account loads. Switch oauth_accounts to lazy="noload" (mirrors roles) and eager-load it explicitly only in UserDatabaseWithRoles.get_by_email — the entry point for fastapi-users' OAuth association flow that appends to the collection. The read-only auth path (get → current_user) no longer loads it; user deletion stays safe via the DB-level ondelete="CASCADE" on OAuthAccount.user_id. Result: /api/users/me 7→5 queries; throughput +10–25% across all authenticated endpoints (e.g. users/me 181→222 rps, settings/modules 331→414 rps), with the unauthenticated /health control unchanged. 79 users/oauth/auth/admin tests pass. Also make the Vite dev port configurable via SM_VITE_PORT so the dev stack can run without colliding with another local app on 5050. Claude-Session: https://claude.ai/code/session_016taVe3VLcoixu32CeE35RC * perf(users): drop EmailStr from response schemas (re-validation waste) UserRead and UserListItem are response models — their data comes straight from the DB (validated on write). Declaring email as EmailStr made FastAPI's response_model validation re-run email-validator for every serialized user. Under realistic load (k6, 10k seeded users, 20/page) py-spy showed validate_email at ~8% of total CPU on user-serializing endpoints; after the change it disappears from the profile entirely. EmailStr stays on the input schemas (UserCreate/UserUpdate/UserInvite) where format validation belongs. Claude-Session: https://claude.ai/code/session_016taVe3VLcoixu32CeE35RC * test(loadtest): add faker data seed + the missing locustfile `make loadtest` / `make loadtest-memray` referenced tests/loadtest/locustfile.py, which was never committed — both targets were broken. Add it: an AuthedUser locust scenario that uses the forged SM_LOADTEST_COOKIE (from scripts/loadtest_seed.py) and drives a weighted mix of the dashboard, paginated user/audit lists, search, and per-resource reads. /api/users/me is omitted (it needs the fastapi-users token, which the session-cookie path doesn't carry). Add tests/loadtest/seed.py: a faker bulk-data seed (default 10k users with role assignments + 100k audit entries) so list/search/pagination are exercised against realistic volumes instead of single-row tables. Idempotent; reuses existing data unless --force. Wired up as `make loadtest-seed`, with faker added to the dev dependency group and a tests/loadtest/README.md walking through the seed -> run -> profile flow. Claude-Session: https://claude.ai/code/session_016taVe3VLcoixu32CeE35RC * fix(users): restore OAuth delete-orphan cascade (regression from 43ac3ad) 43ac3ad set User.oauth_accounts to lazy="noload", which disabled the ORM delete-orphan cascade. On Postgres the FK ondelete=CASCADE still cleans up, but SQLite (the default DB, and the test DB) doesn't enforce FKs — so deleting a user orphaned their OAuth accounts. test_oauth.py's round-trip/cascade test caught it (it wasn't run in that commit). Restore lazy="selectin" + delete-orphan so the cascade works in the ORM on every backend, and keep it off the hot read path by adding noload(User.oauth_accounts) to the queries that never read it: the auth provider's _load_user/_resolve_bearer and the fastapi-users adapter's get() (which backs current_user on every request). get_by_email keeps the default selectin so the OAuth association flow still materialises the collection. Claude-Session: https://claude.ai/code/session_016taVe3VLcoixu32CeE35RC * perf(audit_log): list entries via column query, drop count subquery list_entries selected full AuditEntry ORM objects and counted via a subquery wrapper (select(count()).select_from(base.subquery())). Select only the columns AuditEntryRead needs (plain rows, no ORM hydration) and count the same WHERE conditions directly. Smaller win than the users list (AuditEntry has no relationships, so hydration was lighter): saturated locust /api/audit_log/ RPS 73->77. Direct DB timing shows the real cost is the exact count(*) over 100k rows (~3.3ms) vs <1ms for the page query — kept exact (no approximate-count complexity). tests/test_audit_log.py (15) pass; functional check confirms total, ordering, filters, and JSON changes. Claude-Session: https://claude.ai/code/session_016taVe3VLcoixu32CeE35RC * docs(deploy): size the DB pool to uvicorn --workers (avoid connection exhaustion) Load testing showed the single-worker process is the throughput ceiling (CPU/GIL): 4 uvicorn workers raised aggregate throughput ~2.2x and dropped list-endpoint p50 from ~550ms to ~40ms. But the default per-process pool (SM_DB_POOL_SIZE 10 + SM_DB_MAX_OVERFLOW 20 = 30) means 4 workers want up to 120 connections > Postgres default max_connections 100, so workers threw asyncpg.TooManyConnectionsError under load (~3% of requests). Document the math (total = workers × (pool_size + max_overflow) ≤ max_connections) and a worked example (4 workers → pool 5 + overflow 10 = 60, which load-tested at 762 req/s with zero failures) in the deployment guide, the env-vars reference, and .env.example. No code/behavior change — defaults stay tuned for single-process dev. Claude-Session: https://claude.ai/code/session_016taVe3VLcoixu32CeE35RC * fix(hosting): make production Inertia rendering work (asset manifest) In SM_ENVIRONMENT=production every Inertia page 500'd: setup_inertia never set InertiaConfig.manifest_json_path (defaults "") so fastapi-inertia did open("") → FileNotFoundError. Even with a path it would KeyError — fastapi-inertia looks the entry up by f"{root_directory}/{entrypoint}" = "./main.tsx", but Vite keys its manifest by the source path "main.tsx" — and assets_prefix="" produced /assets/... URLs instead of /static/dist/assets/.... Masked in dev (Vite dev server) and tests (testing env), so it was a latent prod-deploy blocker. Add _prod_manifest_path(): read the Vite manifest, re-key the isEntry chunk under "./main.tsx", write a normalized copy next to the build output (temp-file fallback if read-only), and set manifest_json_path + assets_prefix="static/dist" on the production branch only. Dev is untouched (still uses the Vite dev URL). Verified: prod pages 200 with /static/dist/assets/main-*.{js,css} that serve; dev still renders via the Vite dev server; 137 hosting tests pass + 3 new regression tests for the manifest re-keying (incl. scaffolded static/ layout). Found alongside (not in this commit): static assets are served uncompressed by the app (deployment relies on the reverse proxy for gzip/brotli). Claude-Session: https://claude.ai/code/session_016taVe3VLcoixu32CeE35RC * perf(hosting): cache Vite's content-hashed assets immutably The host /static mount served Vite's build assets with only ETag/Last-Modified, so every page visit re-validated each asset with a conditional GET (304s for the ~480 KB JS + ~140 KB CSS). Their filenames are content-hashed (main-<hash>.js), so the bytes for a URL never change. Add ImmutableStaticFiles (a StaticFiles subclass) that sets `Cache-Control: public, max-age=31536000, immutable` on responses under dist/assets/, and use it for the host static mount. Browsers now serve repeat visits straight from cache with no network round-trip; non-hashed paths (the manifest, etc.) keep the default. Dev is unaffected (assets come from the Vite dev server, not StaticFiles). Verified live in production mode; 139 hosting tests pass + 2 new regression tests. (Wire-size compression — gzip/brotli — is left to the reverse proxy per docs/reference/deployment.md.) Claude-Session: https://claude.ai/code/session_016taVe3VLcoixu32CeE35RC * fix: address code review findings (round 1) - _inertia_setup: when the Vite manifest has no isEntry chunk, return "" so production degrades gracefully instead of returning a path that KeyErrors at render time (matches the no-manifest path). - _inertia_setup: read-only-build-dir fallback now writes a temp file keyed by a hash of the source manifest path, so multiple apps on one host don't clobber each other's normalized manifest. - ImmutableStaticFiles: normalize OS path separators before the dist/assets/ prefix check so the immutable Cache-Control header also applies on Windows. - loadtest/seed.py: scope the --force UserRole delete to seeded users instead of truncating the whole table, so an accidental --force can't wipe real role links. - vite.config: note that SM_VITE_PORT must be kept in sync with the backend's SM_VITE_DEV_URL (dev <script src> + CSP) to avoid a broken dev session. - Add a regression test for the no-isEntry-chunk -> "" fallback. Claude-Session: https://claude.ai/code/session_016taVe3VLcoixu32CeE35RC * perf(users): list users via column query, not full ORM hydration Re-applies the list_users optimization onto #217's admin/queries.py split (the original commit targeted admin/service.py, before list_users moved). list_users loaded full User ORM objects + selectinload(roles) per page, then built UserListItem DTOs. Under load (locust, 10k seeded users, 20/page) ORM object hydration dominated — py-spy showed sqlalchemy instances/_load_for_path at ~57% of CPU on list endpoints. Select only the columns UserListItem needs (plain rows, no ORM entities) and fetch the page's role names in one batched query grouped in Python (DB-agnostic — no array_agg/group_concat, no Role ORM objects). Saturated locust earlier showed /api/users/admin p50 740ms -> 560ms (-24%), RPS +18%, aggregate +15%. Full users suite (296 tests) passes. Claude-Session: https://claude.ai/code/session_016taVe3VLcoixu32CeE35RC
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Admins can now create, edit, and delete users directly from
/users/admin(previously invite-only), all under the existingusers.managepermission.POST /api/users/admin) — active + verified user with an admin-set password; the user can log in immediately. NewCreate.tsxpage + a "Create user" button on the index.PATCH /api/users/admin/{id}) — change email + full name with a case-insensitive duplicate-email guard. NewDetailsCardon the Edit page.DELETE /api/users/admin/{id}) — hard delete behind anAlertDialogconfirm; self-delete is blocked (400). NewDangerZoneon the Edit page.admin/service.pysplit intoqueries.py(reads) +service.py(writes) to stay under the 300-line cap; admin CRUD tests consolidated intotest_*_admin_crud.py; negative-authz (403) coverage added for all three new endpoints. Additive only — no schema/migration changes.Built spec-first (
docs/superpowers/specs/…+docs/superpowers/plans/…), implemented task-by-task with per-task + whole-branch review.Verification
/qa) on the full stack (FastAPI :8000 + Vite :5050, seeded SQLite): create → login-as-new-user, inline validation, DetailsCard save, DangerZone confirm-delete, and the self-delete guard all verified. Found + fixed a P0 the unit tests and reviews all missed: deleting a role-bearing user 500'd via SQLAlchemyStaleDataError(bulk-deleting ORM-trackedusers_user_rolerows). Fixed by loading the user withroles/oauth_accountsforced tonoload+ deterministic bulk deletes; a regression test reproduces it (fails on the old code, passes on the fix).main):make lint✓ ·make test-py1391 passed ✓ ·make test-js22 passed ✓ · JS build ✓ ·make doctorno new users diagnostics.Test plan
/users/admin, create a user, log in as them, edit their email/name, then delete them.https://claude.ai/code/session_01HvC9Rhjs2MGaGC3KRaKAfg