perf(crud): eliminate N+1 queries by eagerly loading relations - #724
Conversation
- Add selectinload for Admin.users, Admin.usage_logs, and Admin.role in get_admins to prevent sequential queries when not in compact mode - Add selectinload for Group.users and Group.inbounds in get_group to eagerly load relations - Add selectinload for Node.usage_logs in get_nodes, get_limited_nodes, and get_nodes_to_reset_usage to avoid loading attributes sequentially - Replace individual db.refresh() calls with single batch re-fetch queries in bulk_reset_node_usage and update_users_proxy_settings - Use .unique() on query results when eager loading relations to handle duplicates from joins - Remove sequential load_admin_attrs, load_group_attrs, and load_node_attrs calls where relations are now eagerly loaded - Use selectinload in _build_user_select_stmt to eagerly load all user relations (admin, next_plan, usage_logs, groups) - Improves query performance by reducing database round trips and eliminating N+1 query patterns across admin, group, node, and user CRUD operations
WalkthroughCRUD queries now use eager loading and batched re-fetches. The usage-recording job caches the database dialect. Admin warning notifications now use locked, deduplicated bulk reminder creation with uniqueness enforcement, SQLite-compatible migration support, and idempotency/failure tests. ChangesORM and job efficiency
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReviewAdmins
participant AdminReminderStore
participant NotificationService
ReviewAdmins->>AdminReminderStore: Lock admins and query reminder records
AdminReminderStore-->>ReviewAdmins: Return eligible admins
ReviewAdmins->>NotificationService: Send warning notifications
NotificationService-->>ReviewAdmins: Report success or failure
ReviewAdmins->>AdminReminderStore: Insert successful reminders in bulk
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/db/crud/bulk.py`:
- Around line 448-453: Preserve input order in the refresh-and-return flow by
mapping the queried users by ID, then rebuilding the result according to
updated_user_ids before returning. Update the logic after load_user_attrs in the
relevant bulk update function, while retaining attribute loading and the
existing count return value.
- Around line 448-451: Update the refreshed-user query in the bulk proxy-setting
path to include the required relationship eager-load options, matching the
dependencies currently loaded by load_user_attrs with load_admin_role=True.
Remove the per-user load_user_attrs loop and return/use the batched, fully
populated User results instead.
In `@app/jobs/review_admins.py`:
- Around line 86-90: Move reminder persistence out of the pre-notification path
in the review-admin flow: send each notification first, then persist only the
corresponding successfully sent reminders through
bulk_create_admin_notification_reminders. Ensure failures or interruptions do
not commit reminders for unsent notifications, following the helper’s documented
post-send contract.
- Around line 73-84: The reminder creation flow around the threshold-admin loop
and its bulk helper must remain idempotent: atomically insert only missing
data-usage reminders for each admin and threshold, even with a non-unique index.
Return or track which reminders were newly inserted, and populate
notifications_to_send only for those new records so recurring job runs do not
resend notifications.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b257731f-5609-4b2d-89c6-8aa7494edbb9
📒 Files selected for processing (7)
app/db/crud/admin.pyapp/db/crud/bulk.pyapp/db/crud/group.pyapp/db/crud/node.pyapp/db/crud/user.pyapp/jobs/record_usages.pyapp/jobs/review_admins.py
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/db/crud/admin.py (1)
533-579: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider a DB-level unique constraint in addition to the application lock.
The new
with_for_update()+ check-then-insert pattern correctly serializes concurrent calls to this function for the same admins, closing the race a prior review flagged. However, idempotency here still rests entirely on every future reminder-creation path acquiring the sameAdminrow lock first — theadmin_notification_reminderstable itself still only has a non-unique index on(admin_id, type)(not even includingthreshold). A realUNIQUEconstraint on(admin_id, type, threshold)would give a durable, schema-level guarantee independent of call-site discipline.♻️ Suggested migration addition
__table_args__ = (Index("ix_admin_notification_reminders_admin_id_type", "admin_id", "type"),) + __table_args__ = ( + UniqueConstraint("admin_id", "type", "threshold", name="uq_admin_notification_reminders"), + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/db/crud/admin.py` around lines 533 - 579, Add a database-level unique constraint on admin_notification_reminders covering admin_id, type, and threshold, and include the corresponding schema migration. Keep bulk_create_admin_notification_reminders’ application locking and duplicate-check logic intact, while ensuring the migration handles any existing duplicate rows before creating the constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/db/crud/admin.py`:
- Around line 533-579: Add a database-level unique constraint on
admin_notification_reminders covering admin_id, type, and threshold, and include
the corresponding schema migration. Keep
bulk_create_admin_notification_reminders’ application locking and
duplicate-check logic intact, while ensuring the migration handles any existing
duplicate rows before creating the constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b66fe4d7-81cc-4e3d-a9f2-4c521d7a6b0f
📒 Files selected for processing (5)
app/db/crud/admin.pyapp/db/crud/bulk.pyapp/db/migrations/versions/bb4a32b7f5ce_add_admin_notification_reminders.pyapp/jobs/review_admins.pytests/test_review_admins_unit.py
🚧 Files skipped from review as they are similar to previous changes (1)
- app/db/crud/bulk.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/db/models.py`:
- Around line 752-755: The nullable AdminNotificationReminder.threshold does not
match the uniqueness expected by create_admin_notification_reminder_if_absent().
Update app/db/models.py lines 752-755 and
app/db/migrations/versions/fb32155473c1_add_unique_constraint_to_admin_.py lines
35-38 consistently: either backfill/remove NULL reminders and make threshold
non-null before applying the unique constraint, or implement backend-specific
nullable-unique behavior so repeated NULL thresholds are handled correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e165dca-a1c2-4b9c-89cc-d70bed2316b0
📒 Files selected for processing (3)
app/db/migrations/versions/fb32155473c1_add_unique_constraint_to_admin_.pyapp/db/models.pytests/test_review_admins_unit.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_review_admins_unit.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Summary
Type of change
Summary by CodeRabbit
thresholdvalues (including non-null defaults with SQLite compatibility).