fix(Segments): Fix N+1 on get_segment retrieve#7563
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Docker builds report
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7563 +/- ##
=======================================
Coverage 98.48% 98.48%
=======================================
Files 1402 1402
Lines 53300 53315 +15
=======================================
+ Hits 52490 52505 +15
Misses 810 810 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Playwright Test Results (private-cloud - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-16)
Details
Playwright Test Results (oss - depot-ubuntu-latest-arm-16)
Details
Playwright Test Results (private-cloud - depot-ubuntu-latest-16)
Details
|
Visual Regression19 screenshots compared. See report for details. |
|
@gemini-code-assist review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the segment retrieve action by implementing prefetch_related for rules and conditions and select_related for project and organization relationships to prevent N+1 query issues. A new unit test has been added to verify the expected number of database queries for this endpoint. The reviewer suggests extending these optimizations to other actions like update, partial_update, destroy, and clone to ensure consistent performance and efficient permission checks across all detail-oriented endpoints.
| queryset = Segment.live_objects.filter(project=project, is_system_segment=False) | ||
|
|
||
| if self.action == "list": | ||
| if self.action in ("list", "retrieve"): |
There was a problem hiding this comment.
The N+1 optimization (prefetching rules and conditions) should also be applied to other actions that return the full segment representation, such as update, partial_update, and clone. This ensures that the response serialization and the cloning logic (which iterates over rules/conditions) do not trigger redundant database queries.
| if self.action in ("list", "retrieve"): | |
| if self.action in ("list", "retrieve", "update", "partial_update", "clone"): |
| if self.action == "retrieve": | ||
| queryset = queryset.select_related("project__organisation") |
There was a problem hiding this comment.
The select_related("project__organisation") optimization should be extended to all detail actions that trigger object permission checks via get_object(). This includes update, partial_update, destroy, associated_features, and clone. This prevents redundant lazy-loading of the project and organisation during the has_object_permission check in SegmentPermissions.
| if self.action == "retrieve": | |
| queryset = queryset.select_related("project__organisation") | |
| if self.action in ("retrieve", "update", "partial_update", "destroy", "associated_features", "clone"): | |
| queryset = queryset.select_related("project__organisation") |
2 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Closes #7557
The
retrieveaction onSegmentViewSetwas missing theprefetch_relatedapplied tolist, causing a separateSELECTquery for conditions for every rule on the segment (N+1). Additionally,has_object_permissionwas lazily loadingsegment.projectandproject.organisationon every retrieve request.prefetch_relatedto coverretrieveas well aslistselect_related("project__organisation")forretrieveto avoid the redundant lazy-loads triggered byhas_object_permissiontest_retrieve_segment__without_rbac__expected_num_queries)How did you test this code?
Added a
django_assert_num_queriestest that creates a segment with 3 nested rules and conditions and asserts the total query count is fixed regardless of the number of rules, covering bothadmin_clientandadmin_master_api_key_clientauth types.