Repository navigation
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:
WalkthroughAdds a generic async dataloader utility, implements an interested-users DataLoader, wires per-request dataloaders into GraphQL context via a custom AsyncGraphQLView, converts several Strawberry resolvers to async Django ORM calls, and updates tests and pytest config for async execution. ChangesDataloader Infrastructure & Async Resolver Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
Summary by CodeRabbit
WalkthroughThis PR implements a DataLoader optimization for the ChangesDataLoader Implementation for Interested Users
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4609 +/- ##
==========================================
- Coverage 98.92% 98.87% -0.06%
==========================================
Files 527 528 +1
Lines 16956 16964 +8
Branches 2360 2360
==========================================
- Hits 16774 16773 -1
- Misses 97 106 +9
Partials 85 85
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:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/graphql-dataloaders #4609 +/- ##
============================================================
Coverage 98.74% 98.74%
============================================================
Files 538 540 +2
Lines 17068 17095 +27
Branches 2421 2422 +1
============================================================
+ Hits 16853 16880 +27
Misses 123 123
Partials 92 92
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/apps/github/api/internal/dataloaders/__init__.py`:
- Around line 8-10: Extract the literal loader key into a module-level constant
(e.g., INTERESTED_USERS_LOADER) in
backend/apps/github/api/internal/dataloaders/__init__.py and use it as the dict
key when returning {"INTERESTED_USERS_LOADER": make_interested_users_loader()}
(replace the quoted literal with the constant); then update any consumers
(notably the use in issue.py where the string "interested_users_loader" is
referenced) to import INTERESTED_USERS_LOADER from the dataloaders package and
use that constant instead of the bare string to avoid brittle duplicates/typos.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 821bc7cd-9a33-42f3-89e2-b7fc6e6722a9
📒 Files selected for processing (9)
backend/apps/common/api/internal/dataloaders/__init__.pybackend/apps/common/api/internal/dataloaders/utils.pybackend/apps/github/api/internal/dataloaders/__init__.pybackend/apps/github/api/internal/dataloaders/interested_users.pybackend/apps/github/api/internal/nodes/issue.pybackend/settings/graphql.pybackend/settings/graphql_context.pybackend/settings/urls.pybackend/tests/unit/apps/github/api/internal/nodes/issue_test.py
The base branch was changed.
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 (3)
backend/tests/unit/apps/owasp/api/internal/queries/stats_test.py (1)
17-37: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert the workspace lookup uses
OWASP_WORKSPACE_ID.Both tests stub the filtered queryset, but neither verifies the resolver actually calls
Workspace.objects.filter(slack_workspace_id=OWASP_WORKSPACE_ID). A regression to the wrong filter would still pass here.Suggested test hardening
import asyncio from unittest.mock import AsyncMock, MagicMock, patch +from apps.slack.constants import OWASP_WORKSPACE_ID from apps.owasp.api.internal.queries.stats import StatsQueryquery = StatsQuery() result = asyncio.run(query.stats_overview()) + mock_workspace_cls.objects.filter.assert_called_once_with( + slack_workspace_id=OWASP_WORKSPACE_ID + ) assert result.active_projects_stats == 270 assert result.active_chapters_stats == 340 assert result.contributors_stats == 15000 assert result.countries_stats == 90 assert result.slack_workspace_stats == 5000query = StatsQuery() result = asyncio.run(query.stats_overview()) + + mock_workspace_cls.objects.filter.assert_called_once_with( + slack_workspace_id=OWASP_WORKSPACE_ID + ) assert result.slack_workspace_stats == 0Also applies to: 47-65
🤖 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 `@backend/tests/unit/apps/owasp/api/internal/queries/stats_test.py` around lines 17 - 37, The workspace lookup in the StatsQuery tests must be asserted to ensure Workspace.objects.filter is called with the constant OWASP_WORKSPACE_ID; update the test(s) in stats_test.py (where StatsQuery().stats_overview() is invoked) to assert that mock_workspace_cls.objects.filter was called with slack_workspace_id=OWASP_WORKSPACE_ID (e.g. assert_called_once_with or assert_called_with), and add the same assertion in the second test block referenced (lines ~47-65) so the resolver’s filter argument is validated for regressions.backend/apps/mentorship/api/internal/queries/module.py (1)
28-31:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winWrap
program.user_has_accesswithsync_to_asyncin async context.
user_has_accessperforms synchronous ORM queries (self.admins.filter(nest_user=user).exists()andself.modules.filter(...).exists()), but is called synchronously from the asyncget_program_modulesresolver. This will raiseSynchronousOnlyOperation. Usesync_to_asyncto safely call it from async context:from asgiref.sync import sync_to_async if program.status != Program.ProgramStatus.PUBLISHED and not await sync_to_async( program.user_has_access )(info.context.request.user): return []🤖 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 `@backend/apps/mentorship/api/internal/queries/module.py` around lines 28 - 31, The async resolver get_program_modules calls the synchronous method program.user_has_access which performs ORM queries and will raise SynchronousOnlyOperation; wrap the call with asgiref.sync.sync_to_async and await it instead of calling directly (import sync_to_async and replace the direct program.user_has_access(...) call with await sync_to_async(program.user_has_access)(info.context.request.user)), keeping the existing conditional logic intact so the published-status check still short-circuits.backend/apps/mentorship/api/internal/queries/program.py (1)
36-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap
program.user_has_accesswithsync_to_asyncin this async resolver.This async resolver calls
program.user_has_access(...), which synchronously performs ORM operations (.filter().exists()). This will raiseSynchronousOnlyOperationat runtime. The same issue exists inmodule.py:get_program_modules(line 28).Fix
+from asgiref.sync import sync_to_async if program.status != Program.ProgramStatus.PUBLISHED and not await sync_to_async( program.user_has_access )(info.context.request.user): return None🤖 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 `@backend/apps/mentorship/api/internal/queries/program.py` around lines 36 - 39, The async resolver in program.py calls the synchronous method program.user_has_access(...) which performs ORM work and must be awaited via an async wrapper; import sync_to_async (from asgiref.sync), replace direct calls to program.user_has_access(...) with await sync_to_async(program.user_has_access)(info.context.request.user) (and do the same fix in module.py's get_program_modules around the call at line 28), ensuring you await the wrapper so no SynchronousOnlyOperation is raised.
🤖 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.
Outside diff comments:
In `@backend/apps/mentorship/api/internal/queries/module.py`:
- Around line 28-31: The async resolver get_program_modules calls the
synchronous method program.user_has_access which performs ORM queries and will
raise SynchronousOnlyOperation; wrap the call with asgiref.sync.sync_to_async
and await it instead of calling directly (import sync_to_async and replace the
direct program.user_has_access(...) call with await
sync_to_async(program.user_has_access)(info.context.request.user)), keeping the
existing conditional logic intact so the published-status check still
short-circuits.
In `@backend/apps/mentorship/api/internal/queries/program.py`:
- Around line 36-39: The async resolver in program.py calls the synchronous
method program.user_has_access(...) which performs ORM work and must be awaited
via an async wrapper; import sync_to_async (from asgiref.sync), replace direct
calls to program.user_has_access(...) with await
sync_to_async(program.user_has_access)(info.context.request.user) (and do the
same fix in module.py's get_program_modules around the call at line 28),
ensuring you await the wrapper so no SynchronousOnlyOperation is raised.
In `@backend/tests/unit/apps/owasp/api/internal/queries/stats_test.py`:
- Around line 17-37: The workspace lookup in the StatsQuery tests must be
asserted to ensure Workspace.objects.filter is called with the constant
OWASP_WORKSPACE_ID; update the test(s) in stats_test.py (where
StatsQuery().stats_overview() is invoked) to assert that
mock_workspace_cls.objects.filter was called with
slack_workspace_id=OWASP_WORKSPACE_ID (e.g. assert_called_once_with or
assert_called_with), and add the same assertion in the second test block
referenced (lines ~47-65) so the resolver’s filter argument is validated for
regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67c4fc5f-36be-4207-a7e3-d5c848bbb9e3
📒 Files selected for processing (11)
backend/apps/github/api/internal/dataloaders/__init__.pybackend/apps/github/api/internal/nodes/issue.pybackend/apps/github/api/internal/queries/organization.pybackend/apps/mentorship/api/internal/queries/module.pybackend/apps/mentorship/api/internal/queries/program.pybackend/apps/owasp/api/internal/queries/stats.pybackend/tests/unit/apps/github/api/internal/nodes/issue_test.pybackend/tests/unit/apps/github/api/internal/queries/organization_test.pybackend/tests/unit/apps/mentorship/api/internal/queries/api_queries_module_test.pybackend/tests/unit/apps/mentorship/api/internal/queries/api_queries_program_test.pybackend/tests/unit/apps/owasp/api/internal/queries/stats_test.py
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 11 files (changes from recent commits).
Prompt for AI agents (unresolved 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/mentorship/api/internal/queries/module.py">
<violation number="1" location="backend/apps/mentorship/api/internal/queries/module.py:19">
P1: This resolver is now async, but it still calls `program.user_has_access(...)`, which performs synchronous ORM `.exists()` queries and can raise `SynchronousOnlyOperation` in async execution.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
5af9f7f
| from apps.github.models.pull_request import PullRequest | ||
| from apps.mentorship.models.issue_user_interest import IssueUserInterest | ||
| from apps.mentorship.api.internal.dataloaders.interested_users import ( | ||
| INTERESTED_USERS_BY_ISSUE_ID_LOADER, |
There was a problem hiding this comment.
I think "INTERESTED_USERS_LOADER" should just be fine here; the class or function will clearly indicate it's been done by issue ID
There was a problem hiding this comment.
There will be a new loader soon for loading the interested users based on issue_number
| @@ -0,0 +1,30 @@ | |||
| """DataLoaders for interested users.""" | |||
There was a problem hiding this comment.
Here you can clearly indicate that it's using the issue ID. Then rename INTERESTED_USERS_BY_ISSUE_ID_LOADER = "interested_users_by_issue_id" (L9)
4936af0
7e5ae19
into
OWASP:feature/graphql-dataloaders
|
Proposed change
Resolves #4598
countto Asynchronous to avoidSynchronousOnlyOperationexceptionsync_to_asyncwith methods that may be used across other parts in the app rather than the resolverpytest-asynciofor asynchronous testsThe code to check the number of db commits:
GitHub.IssueNode.mp4
Checklist
make check-testlocally: all warnings addressed, tests passed