fix: optimize interested users field in IssueNode#3788
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces simple joins with nested Django Prefetches to load interested users and their active badges for IssueNode; introduces a module-level USER_BADGES_PREFETCH and uses a prefetched attribute ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
…e_interestedUsers_field
Utkarsh-0304
dismissed stale reviews from coderabbitai[bot] and cubic-dev-ai[bot]
via
15ae30f
ahmedxgouda
left a comment
ahmedxgouda
left a comment
There was a problem hiding this comment.
Thank you for working on this. You need to optimize the nested relations that can be called within the query like badgeCount.
| prefetch_related=[ | ||
| Prefetch( | ||
| "participant_interests", | ||
| queryset=IssueUserInterest.objects.select_related("user").order_by("user__login"), |
There was a problem hiding this comment.
It is optimized now for this simple query:
But we still have N+1 queries for a query like this:
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3788 +/- ##
=======================================
Coverage 96.77% 96.77%
=======================================
Files 512 512
Lines 15823 15823
Branches 2168 2127 -41
=======================================
Hits 15312 15312
Misses 422 422
Partials 89 89
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
…e_interestedUsers_field
…e_interestedUsers_field
…om/Utkarsh-0304/Nest into fix/optimize_interestedUsers_field
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/apps/github/api/internal/nodes/issue.py`:
- Around line 66-85: The inner Prefetch for "user__user_badges" on the IssueNode
prefetch (to_attr="user_badges_list") doesn’t select_related the badge FK,
causing N+1 queries when UserNode.badges accesses user_badge.badge; update the
inner queryset for UserBadge (the Prefetch inside participant_interests) to
include .select_related("badge") so the badge relation is eagerly loaded (keep
the existing filter/order_by and to_attr).
In `@backend/apps/github/api/internal/nodes/user.py`:
- Around line 44-47: The fast path in the badges resolver uses getattr(root,
"user_badges_list") and then iterates user_badge.badge, which will fire N+1
queries unless the Prefetch that populates user_badges_list includes
select_related("badge"); update the Prefetch queryset where user_badges_list is
built (see the code in issue.py that creates the Prefetch) to add
.select_related("badge") so the badge relation is fetched eagerly and the
resolver (user_badges_list / badges) won’t trigger additional DB queries.
🧹 Nitpick comments (1)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)
104-104: Missing test coverage for the newuser_badges_listfast path.All modified tests set
user_badges_list = None, which only exercises the fallback path. There are no tests for whenuser_badges_listis present (the new optimization path added inuser.py). Consider adding tests that supply a populateduser_badges_listand verify that:
badge_countreturnslen(user_badges_list)badgesreturns the badge objects from the list without calling.filter()
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/apps/github/api/internal/nodes/user_test.py (1)
98-110:⚠️ Potential issue | 🟡 MinorMissing tests for the new
user_badges_listfast path.Setting
user_badges_list = Nonein every badge-related test ensures the fallback path is covered, but there are no tests exercising the new early-return optimization whenuser_badges_listis a pre-populated list. This is the main feature of the PR and should have dedicated test coverage for bothbadge_countandbadgesresolvers.Consider adding tests like:
def test_badge_count_field_with_prefetched_badges(self): mock_user = Mock() mock_user_badge = Mock(is_active=True) mock_user.user_badges_list = [mock_user_badge, mock_user_badge] field = self._get_field_by_name("badge_count", UserNode) result = field.base_resolver.wrapped_func(None, mock_user) assert result == 2 def test_badges_field_with_prefetched_badges(self): mock_badge = Mock(spec=BadgeNode) mock_user_badge = Mock() mock_user_badge.badge = mock_badge mock_user = Mock() mock_user.user_badges_list = [mock_user_badge] field = self._get_field_by_name("badges", UserNode) result = field.base_resolver.wrapped_func(None, mock_user) assert result == [mock_badge]Also applies to: 112-127, 129-147, 149-212
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 12 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/github/api/internal/nodes/issue.py">
<violation number="1" location="backend/apps/github/api/internal/nodes/issue.py:69">
P1: Removing the badge prefetch causes `UserNode.badges()` to silently return an empty list for interested users. The `badges` resolver reads from `getattr(root, "user_badges_list", [])`, which requires the `user__user_badges` Prefetch with `to_attr="user_badges_list"` to have been set up. Without it, any GraphQL query requesting `interestedUsers { badges { ... } }` will always get empty badges.
Consider either keeping the nested badge prefetch here, or adding `prefetch_related=[USER_BADGES_PREFETCH]` to the `badges` field decorator in `UserNode` so that strawberry-django handles it automatically.</violation>
</file>
<file name="backend/apps/github/api/internal/nodes/user.py">
<violation number="1" location="backend/apps/github/api/internal/nodes/user.py:44">
P2: Removing `prefetch_related=[USER_BADGES_PREFETCH]` from the `badges` field decorator means badges will silently return an empty list when `UserNode` is resolved from nested contexts (e.g., issue assignees/authors, mentorship mentees, snapshot users, etc.) that don't manually set up the prefetch. Previously, strawberry-django would automatically apply the prefetch when the field was resolved. Consider keeping `prefetch_related=[USER_BADGES_PREFETCH]` on this field to ensure correct behavior in all resolution contexts.</violation>
</file>
<file name="backend/tests/apps/github/api/internal/queries/user_test.py">
<violation number="1" location="backend/tests/apps/github/api/internal/queries/user_test.py:27">
P2: The assertion verifies `prefetch_related` was called but doesn't check it received `USER_BADGES_PREFETCH` as an argument. Since the whole point of this PR is to add the correct prefetch, use `assert_called_once_with(USER_BADGES_PREFETCH)` to verify the correct prefetch object is passed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
|
There was a problem hiding this comment.
Hi @Utkarsh-0304 !
I pushed changes to this PR to address some of my concerns and issues that I found.
I believe we'll need to look more into other ways to optimize backend calls without prefetches. My main concern is that some of these cases are only "theoretical" and are only possible when you go into GraphQL explorer. For example, there's no query where we'd need to have badges on interested_users.
But since this PR addresses the issue and also has some clean up that was needed - we'd push this as is 👌🏼
Thank you!
| return root.user_badges.filter(is_active=True).count() | ||
|
|
||
| @strawberry_django.field(prefetch_related=["user_badges__badge"]) | ||
| @strawberry_django.field(prefetch_related=[USER_BADGES_PREFETCH]) |
There was a problem hiding this comment.
I removed this resolver completely - the only place where we show badge count get's that from Algolia.
| return User.objects.filter(has_public_member_page=True, login=login).first() | ||
| return ( | ||
| User.objects.filter(has_public_member_page=True, login=login) | ||
| .prefetch_related(USER_BADGES_PREFETCH) |
There was a problem hiding this comment.
We only show badges on the single user page that is resolved here. With just adding prefetch to a @strawberry_django.field this was not working and not adding it here - this was always going into a fallback for [].
Prefetch on the field will be triggered when we fetch badges via another connection (example: Issue.author.badges if we'd ever need that).
Any optimization needs to be tested to make sure returned results are the same as before.
There was a problem hiding this comment.
Yes, I realised I forgot to handle the scenario involving only users.
I will ensure to test that the returned results remain consistent in future PRs.
Thanks for your review, @kasya
| @@ -1,24 +1,5 @@ | |||
| import { gql } from '@apollo/client' | |||
|
|
|||
| export const GET_LEADER_DATA = gql` | |||
4 participants
Proposed change
Resolves #3659
This PR optimises the interestedUsers field in RepositoryNode by eliminating the N + 1 query problem through the use of the Prefetch object.
Checklist
make check-testlocally: all warnings addressed, tests passed