Repository navigation
perf(groups): add reverse group membership indexes - #772
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughGroup summaries now populate ChangesGroup membership scaling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GroupListAPI
participant get_group
participant SQLDatabase
GroupListAPI->>get_group: Request group summaries
get_group->>SQLDatabase: Count user associations and fetch groups
SQLDatabase-->>get_group: Return groups with total_users
get_group-->>GroupListAPI: Return summaries without Group.users
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change preserves existing response behavior and includes regression coverage and migration validation; no actionable merge-blocking risk remains. Pre-merge checks |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
c9fdd96 to
2759487
Compare
…current head The migration branched off an older revision, and a merge migration joined it to dev's head (b4c7e8f1a2d3). That works alone, but once another migration PR lands the same way, dev has two Alembic heads and `alembic upgrade head` fails with "Multiple head revisions". Point the migration at dev's head and drop the merge migration. Also drop an extra blank line after the imports that ruff flags (I001).
The member count rewrite overlaps PasarGuard#967, which counts members in SQL in load_group_attrs without loading them and keeps a fallback when the count was not loaded. Leave Group.total_users and get_group as on dev so this PR adds only the reverse membership indexes, which those group-centric counts and bulk membership deletes use.
… node and core sort order The other migration PRs in the series (PasarGuard#774, PasarGuard#865, PasarGuard#875) have landed, so point this migration at the latest revision (d73f8a2c4e91) to keep a single Alembic head.
Summary
Group.total_usersin SQL for the group-list query instead of hydrating every relatedUserCloses #771
Type of change
Checklist
Testing
uv run ruff check .— passeduv run ruff format --check app/db/models.py app/db/crud/group.py tests/test_group_membership_indexes.py tests/api/test_group.py— passeduv run pytest -q tests/test_group_membership_indexes.py tests/api/test_group.py::test_groups_get_counts_users_without_loading_user_rows— 3 passeduv run alembic checkon SQLite — no new upgrade operationsCOMMIT,CREATE INDEX CONCURRENTLY, thenBEGIN; downgrade emitsDROP INDEX CONCURRENTLYSynthetic SQLite benchmark (500,000 memberships / 100 groups):
The benchmark is directional and is not presented as a production guarantee.
Screenshots
Not applicable (backend/database-only change).
Notes for reviewers
autocommit_block()for this path.Summary by CodeRabbit
Improvements
Bug Fixes
Tests