perf(api): improve performance on states and modules endpoints#9275
Open
p11o wants to merge 5 commits into
Open
perf(api): improve performance on states and modules endpoints#9275p11o wants to merge 5 commits into
p11o wants to merge 5 commits into
Conversation
…yset Filtering via project__project_projectmember caused a JOIN that multiplied rows, requiring a DISTINCT to deduplicate. Replace with an Exists() correlated subquery on ProjectMember, which avoids the row inflation entirely and makes DISTINCT unnecessary.
…dule queryset
The five state-group counts used Count("issue_module__issue__state__group",
distinct=True) which traverses a 3-table join path per annotation and forces
the DB to deduplicate the inflated result set. Replace with Coalesce(Subquery)
per the pattern already established in sub_issue.py (PR makeplane#8889).
Also extract the duplicated get_queryset body from ModuleListCreateAPIEndpoint,
ModuleDetailAPIEndpoint, and ModuleArchiveUnarchiveAPIEndpoint into shared
helper functions to eliminate the copy-paste.
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR centralizes module queryset construction into shared helpers and changes state, issue activity, and issue view query filtering to use ChangesModule queryset centralization
Project membership Exists filters
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/plane/api/views/state.py (1)
172-186: 💤 Low valueConsider extracting a shared helper to reduce duplication.
The
get_querysetimplementations inStateListCreateAPIEndpoint(lines 49-61) andStateDetailAPIEndpoint(lines 173-185) are identical. Following the same pattern applied inmodule.pywhere_build_module_querysetwas extracted, a shared helper could consolidate this logic.♻️ Optional helper extraction
def _build_state_queryset(slug, project_id, user): """Build the base State queryset with membership check.""" member_check = ProjectMember.objects.filter( project_id=OuterRef("project_id"), member=user, is_active=True, deleted_at__isnull=True, ) return ( State.objects.filter(workspace__slug=slug) .filter(project_id=project_id) .filter(Exists(member_check)) .filter(is_triage=False) .filter(project__archived_at__isnull=True) .select_related("project", "workspace") )Then both endpoints can use:
def get_queryset(self): return _build_state_queryset( self.kwargs.get("slug"), self.kwargs.get("project_id"), self.request.user, )🤖 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 `@apps/api/plane/api/views/state.py` around lines 172 - 186, Extract the duplicated queryset building logic from both StateListCreateAPIEndpoint and StateDetailAPIEndpoint get_queryset methods into a shared helper function called _build_state_queryset. This helper should accept slug, project_id, and user as parameters and contain the ProjectMember membership check and all the filtering logic that builds the State queryset. Then update both get_queryset methods in StateListCreateAPIEndpoint and StateDetailAPIEndpoint to call this new helper function instead of duplicating the entire queryset construction logic.
🤖 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 `@apps/api/plane/api/views/state.py`:
- Around line 172-186: Extract the duplicated queryset building logic from both
StateListCreateAPIEndpoint and StateDetailAPIEndpoint get_queryset methods into
a shared helper function called _build_state_queryset. This helper should accept
slug, project_id, and user as parameters and contain the ProjectMember
membership check and all the filtering logic that builds the State queryset.
Then update both get_queryset methods in StateListCreateAPIEndpoint and
StateDetailAPIEndpoint to call this new helper function instead of duplicating
the entire queryset construction logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 191d05e1-ca58-45fa-a3f1-de10936682ea
📒 Files selected for processing (2)
apps/api/plane/api/views/module.pyapps/api/plane/api/views/state.py
CI requires 80% docstring coverage. Added one-line docstrings to all functions introduced or modified by the perf fixes: _build_module_queryset, and the get_queryset methods on ModuleListCreateAPIEndpoint, ModuleDetailAPIEndpoint, ModuleArchiveUnarchiveAPIEndpoint, and both State endpoint classes.
p11o
changed the title
…ctivity querysets Eliminates row-inflating JOINs through project_projectmember in IssueViewViewSet and IssueActivityEndpoint, replacing them with correlated Exists() subqueries. Removes the forced .distinct() from the views queryset.
Contributor
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 `@apps/api/plane/app/views/issue/activity.py`:
- Around line 35-59: The membership filter in the issue activity query is only
tied to the URL project_id, while IssueActivity and IssueComment rows are not
constrained to that same project, which can allow cross-project reads. Update
the queries in the activity view to bind returned rows to the same project as
member_check by adding a project-level constraint on IssueActivity and
IssueComment (using the existing issue_id/slug filters plus the project
identifier from the request), so the membership check and returned data both
refer to the same project. Use the existing symbols member_check,
issue_activities, issue_comments, and the surrounding filter/Exists chain to
apply the fix consistently.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5001af35-eb6a-45b5-8849-4688fac4724d
📒 Files selected for processing (2)
apps/api/plane/app/views/issue/activity.pyapps/api/plane/app/views/view/base.py
…tyEndpoint
Binds IssueActivity and IssueComment queries to project_id from the URL
and uses OuterRef("project_id") in the membership Exists() subquery, so
membership in the URL project no longer grants read access to rows from
a different project sharing the same issue_id.
Adds regression tests covering both the activity and comment branches.
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
These perf updates amount to a roughly 15s latency reduction as the api calls are sequential:
/api/projects/{id}/states/The
get_querysetfiltered membership via a JOIN throughproject__project_projectmember, which multiplied rows and required.distinct()to deduplicate. Replaced with anExists()correlated subquery onProjectMember— same semantics, no row inflation./api/projects/{id}/modules/Five of the six issue-count annotations used
Count("issue_module__issue__state__group", distinct=True), traversing a 3-table join path per annotation. Replaced withCoalesce(Subquery(...annotate(count=Count("id"))))— the same pattern used in #8889 (2.6s -> 0.07s improvement on sub-issues).Also extracted the three identical
get_querysetbodies fromModuleListCreateAPIEndpoint,ModuleDetailAPIEndpoint, andModuleArchiveUnarchiveAPIEndpointinto shared helpers.Type of Change
Test Scenarios
Ran the full Docker test suite (
docker compose -f docker-compose-test.yml up --build --abort-on-container-exit): 345 passed, 8 pre-existing failures inTestMagicSign*References
#8889 — prior art for the
Coalesce(Subquery)pattern in this codebaseSummary by CodeRabbit
Summary of changes