fix(api): annotate WorkspaceModulesEndpoint to avoid per-row lookups (N+1)#9378
fix(api): annotate WorkspaceModulesEndpoint to avoid per-row lookups (N+1)#9378duanearnett wants to merge 1 commit into
Conversation
The workspace-scoped modules list feeds ModuleSerializer a queryset that omits several computed fields the serializer exposes (member_ids, is_favorite, total/completed_estimate_points). The project-scoped ModuleViewSet.get_queryset already annotates these via ArrayAgg / Exists / Subquery; the workspace endpoint did not, so those fields are resolved per row during serialization -> one members lookup per module (N+1) on the workspace modules list. Add the same annotations here so the list is served in a constant number of queries regardless of module count. Issue-count annotations are left unchanged to preserve existing behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
duane seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📝 WalkthroughWalkthroughThe WorkspaceModulesEndpoint's get method is updated to precompute additional fields at the queryset level via ORM subqueries: is_favorite existence check, member_ids array aggregation, and total/completed estimate point sums, reducing per-row serialization work. ChangesWorkspace Modules Queryset Precomputation
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/plane/app/views/workspace/module.py (1)
85-108: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffCartesian-product cost from mixing
ArrayAgg(member_ids) with theCountissue annotations.
member_idsadds amembersM2M join to the same queryset that already fans out overissue_modulefor the six issue-count annotations, producing members × issue_modules rows per module. Thedistinct=Trueguards keep the counts and array correct, but the intermediate result set (and grouping cost) grows multiplicatively. This mirrorsModuleViewSet, so it's likely acceptable, but worth confirming it doesn't regress list latency on large workspaces; isolatingmember_idsinto its ownSubquery(ArrayAgg(...))avoids the extra join entirely.🤖 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/app/views/workspace/module.py` around lines 85 - 108, The queryset in module aggregation is introducing a multiplicative join by combining member_ids ArrayAgg with the existing issue count annotations, which can inflate the intermediate row set. Update the annotation logic around member_ids in the workspace module queryset to avoid joining members on the same query as the issue_module-based counts, preferably by moving member_ids into its own Subquery(ArrayAgg(...)) or equivalent isolated annotation. Keep the existing count annotations in place, and use the Module queryset structure as the reference point when adjusting the annotate chain.
🤖 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/workspace/module.py`:
- Around line 86-98: The member_ids aggregation is currently joining through
members__id and modulemember__deleted_at separately, which can allow
soft-deleted memberships to leak into the result. Update the annotate/ArrayAgg
in the module query to aggregate directly from the ModuleMember path, keeping
the deleted_at__isnull=True filter on that same join and preserving the distinct
UUID list behavior.
---
Nitpick comments:
In `@apps/api/plane/app/views/workspace/module.py`:
- Around line 85-108: The queryset in module aggregation is introducing a
multiplicative join by combining member_ids ArrayAgg with the existing issue
count annotations, which can inflate the intermediate row set. Update the
annotation logic around member_ids in the workspace module queryset to avoid
joining members on the same query as the issue_module-based counts, preferably
by moving member_ids into its own Subquery(ArrayAgg(...)) or equivalent isolated
annotation. Keep the existing count annotations in place, and use the Module
queryset structure as the reference point when adjusting the annotate chain.
🪄 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: 04bf223d-3b1b-4f4e-b7e0-347f5f429a90
📒 Files selected for processing (1)
apps/api/plane/app/views/workspace/module.py
| .annotate( | ||
| member_ids=Coalesce( | ||
| ArrayAgg( | ||
| "members__id", | ||
| distinct=True, | ||
| filter=Q( | ||
| members__id__isnull=False, | ||
| modulemember__deleted_at__isnull=True, | ||
| ), | ||
| ), | ||
| Value([], output_field=ArrayField(UUIDField())), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect Module.members M2M + ModuleMember through model, and compare member_ids annotation with ModuleViewSet.
fd -t f 'module.py' apps/api/plane/db/models | while read -r f; do
echo "=== $f ==="
rg -nP -C3 '(class Module\b|class ModuleMember\b|members\s*=\s*models\.ManyToManyField|through=|related_name=)' "$f"
done
echo "===== ModuleViewSet member_ids / through-name usage ====="
rg -nP -C4 'member_ids\s*=' apps/api/plane/app/views
rg -nP -C1 'module_members|modulemember' apps/api/plane/app/views/module apps/api/plane/db/modelsRepository: makeplane/plane
Length of output: 6397
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== ModuleMember model and inheritance ==="
sed -n '1,220p' apps/api/plane/db/models/module.py | cat -n
echo "=== Workspace module view ==="
sed -n '1,180p' apps/api/plane/app/views/workspace/module.py | cat -n
echo "=== Base module view ==="
sed -n '250,310p' apps/api/plane/app/views/module/base.py | cat -n
echo "=== Archive module view ==="
sed -n '235,265p' apps/api/plane/app/views/module/archive.py | cat -n
echo "=== Search for ModuleMember reverse-name usage ==="
rg -n "modulemember__|module_members__|modulemember_set|ModuleMember" apps/api/plane -g '!**/__pycache__/**'Repository: makeplane/plane
Length of output: 22962
Aggregate from the through model on one join. members__id plus modulemember__deleted_at can split across two ModuleMember joins, so deleted memberships may still appear in member_ids. Switch the aggregate to the ModuleMember path and keep the soft-delete filter there.
🤖 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/app/views/workspace/module.py` around lines 86 - 98, The
member_ids aggregation is currently joining through members__id and
modulemember__deleted_at separately, which can allow soft-deleted memberships to
leak into the result. Update the annotate/ArrayAgg in the module query to
aggregate directly from the ModuleMember path, keeping the
deleted_at__isnull=True filter on that same join and preserving the distinct
UUID list behavior.
2 participants
Summary
WorkspaceModulesEndpoint(GET /api/workspaces/<slug>/modules/) serializes withModuleSerializer, but builds a queryset that omits several computed fields the serializer exposes:member_ids,is_favorite,total_estimate_points, andcompleted_estimate_points.The project-scoped
ModuleViewSet.get_queryset()already annotates all of these (ArrayAggformember_ids,Existsforis_favorite,Subqueryfor estimate points). Because the workspace endpoint does not, those fields end up resolved per row during serialization — most notably a separatememberslookup for every module in the workspace. On workspaces with many modules this makes the endpoint scale as O(number of modules) database queries instead of a constant number, and it's a common source of slow initial page loads.Fix
Annotate the same computed fields on the workspace queryset, mirroring the existing project-scoped
ModuleViewSetimplementation:is_favorite→Exists(...)member_ids→Coalesce(ArrayAgg("members__id", distinct=True, filter=...), [])total_estimate_points/completed_estimate_points→Subquery(...)The existing issue-count annotations are left unchanged to preserve current behavior; this PR only adds the missing annotations so no serializer field falls back to a per-row lookup. Net effect: the module list is served in a constant number of queries regardless of module count.
Notes
plane/app/views/module/base.py::ModuleViewSet.get_queryset.🤖 Generated with Claude Code
Summary by CodeRabbit