refactor: migrate ModuleNode to use strawberry-django decorator for consistency#4256
Open
Jpeg-create wants to merge 10 commits intoOWASP:mainfrom
Open
refactor: migrate ModuleNode to use strawberry-django decorator for consistency#4256Jpeg-create wants to merge 10 commits intoOWASP:mainfrom
Jpeg-create wants to merge 10 commits intoOWASP:mainfrom
Conversation
Contributor
|
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:
Summary by CodeRabbit
WalkthroughModuleNode was migrated to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/apps/mentorship/api/internal/nodes/module.py (1)
59-62: Consider addingselect_relatedhint for project access.The
project_nameresolver accessesroot.project.name, which could cause an N+1 query if theprojectFK isn't prefetched upstream. If this field is frequently queried, consider adding optimization hints at the query layer or using@strawberry_django.field(select_related=["project"]).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/mentorship/api/internal/nodes/module.py` around lines 59 - 62, The project_name resolver on Module accesses root.project.name and can trigger N+1 queries; update the Strawberry-Django field decorator to include a select_related hint (e.g., use `@strawberry_django.field`(select_related=["project"])) so the Module.project FK is fetched with the parent query, or alternatively ensure the upstream query that resolves Module objects calls select_related("project"); modify the decorator on the project_name method (and any similar resolvers) accordingly.
🤖 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/mentorship/api/internal/nodes/module.py`:
- Line 230: The file ends without a trailing newline (see the declaration "tags:
list[str] = strawberry.field(default_factory=list)"); add a single newline
character at the end of the file so the last line is terminated, complying with
Ruff W292/PEP8.
---
Nitpick comments:
In `@backend/apps/mentorship/api/internal/nodes/module.py`:
- Around line 59-62: The project_name resolver on Module accesses
root.project.name and can trigger N+1 queries; update the Strawberry-Django
field decorator to include a select_related hint (e.g., use
`@strawberry_django.field`(select_related=["project"])) so the Module.project FK
is fetched with the parent query, or alternatively ensure the upstream query
that resolves Module objects calls select_related("project"); modify the
decorator on the project_name method (and any similar resolvers) accordingly.
🪄 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: CHILL
Plan: Pro
Run ID: 2caffa2b-8243-4255-990a-0501e0904e99
📒 Files selected for processing (2)
backend/apps/mentorship/api/internal/nodes/module.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
1 task
Jpeg-create
force-pushed
the
refactor/module-node-strawberry-django
branch
from
b1b8db3 to
fd9904c
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/apps/mentorship/api/internal/nodes/module.py (1)
59-62: Consider whether the null check is necessary.Based on the context snippet,
projectis defined as a non-nullable ForeignKey on the Module model (nonull=True). Theif root.project else Nonecheck may be defensive but unnecessary if the FK constraint is enforced.This is a minor observation—the current implementation is safe and handles potential data integrity edge cases gracefully.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/mentorship/api/internal/nodes/module.py` around lines 59 - 62, The null-check in the project_name resolver is unnecessary given Module.project is a non-nullable FK; simplify the project_name method in module.py (the strawberry_django.field resolver named project_name for Module) to directly return root.project.name (remove the "if root.project else None" defensive branch), or if you prefer to be explicit keep a single assert/validation that project is present before accessing .name to avoid silent None returns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/apps/mentorship/api/internal/nodes/module.py`:
- Around line 59-62: The null-check in the project_name resolver is unnecessary
given Module.project is a non-nullable FK; simplify the project_name method in
module.py (the strawberry_django.field resolver named project_name for Module)
to directly return root.project.name (remove the "if root.project else None"
defensive branch), or if you prefer to be explicit keep a single
assert/validation that project is present before accessing .name to avoid silent
None returns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 054dd12a-06c1-47e6-b7da-6bf3701c4321
📒 Files selected for processing (2)
backend/apps/mentorship/api/internal/nodes/module.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/mentorship/api/internal/nodes/module.py (1)
98-114:⚠️ Potential issue | 🟡 MinorAdd offset validation to prevent unexpected pagination results.
normalize_limitboundslimit, butoffsetis used directly inqueryset[offset : offset + normalized_limit]. A negative offset doesn't cause an error—it silently uses Python's negative indexing, returning unexpected results (potentially empty or partial data). This pattern is already validated inbackend/apps/owasp/api/internal/queries/project_health_metrics.py.💡 Suggested fix
if (normalized_limit := normalize_limit(limit, MAX_LIMIT)) is None: return [] + if offset < 0: + return [] queryset = root.issues.select_related("repository", "author").prefetch_related(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/mentorship/api/internal/nodes/module.py` around lines 98 - 114, The issues resolver uses normalize_limit but leaves offset unchecked, which allows negative offsets to trigger Python negative slicing; update the issues method in Module (function issues) to validate the offset is non-negative before slicing — e.g., compute a normalized_offset (or early-return []) if offset is < 0 (mirroring the pattern used in project_health_metrics.py), then use queryset.order_by("-updated_at")[normalized_offset : normalized_offset + normalized_limit] to avoid unexpected pagination results.
🤖 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/mentorship/api/internal/nodes/module.py`:
- Around line 155-160: The resolver is returning duplicate users when an
issue_number maps to multiple linked issues; update the IssueUserInterest
queryset (used in the interested_users resolver) to deduplicate by user before
returning — e.g., call .distinct("user") (or .distinct() if you prefer
DB-agnostic) on the queryset
(IssueUserInterest.objects.select_related("user").filter(...).order_by("user__login").distinct("user"))
and then return [i.user for i in interests] so each user is only returned once.
- Around line 86-87: Remove the redundant boolean guard that forces a DB
evaluation when assigning issue_ids; instead, assign issue_ids using
root.issues.filter(number=issue_number).values_list("id", flat=True) without
wrapping it in an if not check and let the downstream filters that use
__in=issue_ids naturally return empty results. Apply the same change in both
resolvers where issue_ids is computed so you avoid the extra queryset
evaluation.
---
Outside diff comments:
In `@backend/apps/mentorship/api/internal/nodes/module.py`:
- Around line 98-114: The issues resolver uses normalize_limit but leaves offset
unchecked, which allows negative offsets to trigger Python negative slicing;
update the issues method in Module (function issues) to validate the offset is
non-negative before slicing — e.g., compute a normalized_offset (or early-return
[]) if offset is < 0 (mirroring the pattern used in project_health_metrics.py),
then use queryset.order_by("-updated_at")[normalized_offset : normalized_offset
+ normalized_limit] to avoid unexpected pagination results.
🪄 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: CHILL
Plan: Pro
Run ID: 9ffc2e72-bd6b-4206-9d8d-b652c4c5fb86
📒 Files selected for processing (1)
backend/apps/mentorship/api/internal/nodes/module.py
Contributor
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/apps/mentorship/api/internal/nodes/module.py (1)
167-173:⚠️ Potential issue | 🟡 MinorAlign
task_deadlinefiltering with “latest assigned task” semantics.At Line 167–173, the query orders by
-assigned_atbut does not excludeassigned_at IS NULL. That can select an unassigned task on some DBs due to null ordering.Suggested fix
return ( Task.objects.filter( module=root, issue__number=issue_number, deadline_at__isnull=False, + assigned_at__isnull=False, ) .order_by("-assigned_at") .values_list("deadline_at", flat=True) .first() )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/mentorship/api/internal/nodes/module.py` around lines 167 - 173, The Task query that fetches task_deadline currently orders by "-assigned_at" but can return rows with assigned_at NULL; update the query that builds this values_list (the Task.objects.filter(...) block) to explicitly exclude unassigned tasks by adding a filter for assigned_at__isnull=False (or .exclude(assigned_at__isnull=True)) before the .order_by("-assigned_at") so the "latest assigned task" semantics only consider tasks with non-null assigned_at.
♻️ Duplicate comments (2)
backend/apps/mentorship/api/internal/nodes/module.py (2)
87-88:⚠️ Potential issue | 🟡 MinorAvoid QuerySet truthiness checks when building
issue_ids.At Line 87 and Line 153, boolean-evaluating the QuerySet triggers an extra DB hit. Assign
issue_idsdirectly and let__in=issue_idsnaturally yield empty results.Suggested fix
- if not (issue_ids := root.issues.filter(number=issue_number).values_list("id", flat=True)): - return [] + issue_ids = root.issues.filter(number=issue_number).values_list("id", flat=True)Apply the same change in both resolvers.
Based on learnings: In Django, boolean evaluation of a QuerySet performs a database query; prefer avoiding truthiness checks for existence-sensitive paths.
Also applies to: 153-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/mentorship/api/internal/nodes/module.py` around lines 87 - 88, The code currently boolean-evaluates a QuerySet when building issue_ids (e.g., using "if not (issue_ids := root.issues.filter(...).values_list(...))"), which causes an extra DB hit; instead assign issue_ids directly from root.issues.filter(...).values_list("id", flat=True) and remove the truthiness guard so subsequent queries use __in=issue_ids (letting an empty list naturally produce no results); apply this change in both resolver locations that set issue_ids to avoid the implicit QuerySet evaluation.
156-161:⚠️ Potential issue | 🟡 MinorDeduplicate
interested_usersto avoid repeated users.Line 156–161 can return the same user multiple times when
issue_numbermatches multiple linked issues. Return distinct users instead.Suggested fix
- interests = ( - IssueUserInterest.objects.select_related("user") - .filter(module=root, issue_id__in=issue_ids) - .order_by("user__login") - ) - return [i.user for i in interests] + return list( + User.objects.filter( + mentorship_interests__module=root, + mentorship_interests__issue_id__in=issue_ids, + ) + .order_by("login") + .distinct() + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/mentorship/api/internal/nodes/module.py` around lines 156 - 161, The current code in IssueUserInterest selection returns duplicate User objects when a single user has interests on multiple issues; update the logic in the function that builds `interests` (using IssueUserInterest, `root`, and `issue_ids`) to first select distinct user IDs (e.g. IssueUserInterest.objects.filter(module=root, issue_id__in=issue_ids).values_list("user_id", flat=True).distinct()), then fetch the User objects once (e.g. User.objects.filter(id__in=distinct_ids).order_by("login")) and return that list so users are deduplicated and still ordered by login.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/apps/mentorship/api/internal/nodes/module.py`:
- Around line 167-173: The Task query that fetches task_deadline currently
orders by "-assigned_at" but can return rows with assigned_at NULL; update the
query that builds this values_list (the Task.objects.filter(...) block) to
explicitly exclude unassigned tasks by adding a filter for
assigned_at__isnull=False (or .exclude(assigned_at__isnull=True)) before the
.order_by("-assigned_at") so the "latest assigned task" semantics only consider
tasks with non-null assigned_at.
---
Duplicate comments:
In `@backend/apps/mentorship/api/internal/nodes/module.py`:
- Around line 87-88: The code currently boolean-evaluates a QuerySet when
building issue_ids (e.g., using "if not (issue_ids :=
root.issues.filter(...).values_list(...))"), which causes an extra DB hit;
instead assign issue_ids directly from root.issues.filter(...).values_list("id",
flat=True) and remove the truthiness guard so subsequent queries use
__in=issue_ids (letting an empty list naturally produce no results); apply this
change in both resolver locations that set issue_ids to avoid the implicit
QuerySet evaluation.
- Around line 156-161: The current code in IssueUserInterest selection returns
duplicate User objects when a single user has interests on multiple issues;
update the logic in the function that builds `interests` (using
IssueUserInterest, `root`, and `issue_ids`) to first select distinct user IDs
(e.g. IssueUserInterest.objects.filter(module=root,
issue_id__in=issue_ids).values_list("user_id", flat=True).distinct()), then
fetch the User objects once (e.g.
User.objects.filter(id__in=distinct_ids).order_by("login")) and return that list
so users are deduplicated and still ordered by login.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b99c00f7-3c14-41fa-afdd-19c898b796b9
⛔ Files ignored due to path filters (4)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/moduleMutations.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/moduleQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/programsQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (1)
backend/apps/mentorship/api/internal/nodes/module.py
kasya
previously approved these changes
Mar 14, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4256 +/- ##
==========================================
- Coverage 99.41% 99.40% -0.02%
==========================================
Files 520 520
Lines 16441 16430 -11
Branches 2292 2238 -54
==========================================
- Hits 16345 16332 -13
- Misses 60 62 +2
Partials 36 36
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:
|
auto-merge was automatically disabled
March 15, 2026 10:24
Head branch was pushed to by a user without write access
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/apps/mentorship/api/internal/nodes/module.py (2)
179-191: Consider database-agnostic deduplication for portability.The
distinct("user")syntax on lines 189-190 is PostgreSQL-specific (DISTINCT ON), whileissue_menteesat line 93 uses the portable.distinct(). For consistency and database portability, consider querying User objects directly with standard.distinct().Database-agnostic alternative
`@strawberry_django.field` def interested_users(self, root: Module, issue_number: int) -> list[UserNode]: """Return users interested in this module's issue identified by its number.""" - return [ - issue_user_interest.user - for issue_user_interest in IssueUserInterest.objects.select_related("user") - .filter( - module=root, - issue_id__in=root.issues.filter(number=issue_number).values_list("id", flat=True), - ) - .order_by("user", "user__login") - .distinct("user") - ] + issue_ids = root.issues.filter(number=issue_number).values_list("id", flat=True) + return list( + User.objects.filter( + mentorship_interests__module=root, + mentorship_interests__issue_id__in=issue_ids, + ) + .order_by("login") + .distinct() + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/mentorship/api/internal/nodes/module.py` around lines 179 - 191, The current interested_users resolver uses IssueUserInterest.objects...distinct("user") which relies on PostgreSQL's DISTINCT ON; instead query the User model directly to make the query DB-agnostic: use User.objects.filter(issueuserinterest__module=root, issueuserinterest__issue_id__in=root.issues.filter(number=issue_number).values_list("id", flat=True)) with appropriate select_related/prefetch and .order_by("login").distinct() and return those users (referencing the interested_users resolver, IssueUserInterest, and Module/Issue relationships) so the deduplication works across databases.
54-67: Consider non-optional return types for required FKs.Both
programandprojectare required ForeignKey fields in the model (nonull=True), so their resolvers will always return a value. The current| Nonetypes are safe but make the GraphQL schema appear nullable when these fields are actually guaranteed.Optional type tightening
`@strawberry_django.field` - def program(self, root: Module) -> ProgramNode | None: + def program(self, root: Module) -> ProgramNode: """Get the program for this module.""" return root.program `@strawberry_django.field` - def project_id(self, root: Module) -> strawberry.ID | None: + def project_id(self, root: Module) -> strawberry.ID: """Get the project ID for this module.""" return root.project_id `@strawberry_django.field` - def project_name(self, root: Module) -> str | None: + def project_name(self, root: Module) -> str: """Get the project name for this module.""" - return root.project.name if root.project else None + return root.project.name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/apps/mentorship/api/internal/nodes/module.py` around lines 54 - 67, The GraphQL field resolvers currently mark program, project_id and project_name as nullable but the underlying Module model has non-nullable ForeignKeys, so update the return types: change program(self, root: Module) -> ProgramNode (remove | None), project_id(self, root: Module) -> strawberry.ID (remove | None), and project_name(self, root: Module) -> str (remove | None); keep resolver bodies the same but tighten the annotated return types on the methods program, project_id, and project_name to reflect the required FK guarantees.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/apps/mentorship/api/internal/nodes/module.py`:
- Around line 179-191: The current interested_users resolver uses
IssueUserInterest.objects...distinct("user") which relies on PostgreSQL's
DISTINCT ON; instead query the User model directly to make the query
DB-agnostic: use User.objects.filter(issueuserinterest__module=root,
issueuserinterest__issue_id__in=root.issues.filter(number=issue_number).values_list("id",
flat=True)) with appropriate select_related/prefetch and
.order_by("login").distinct() and return those users (referencing the
interested_users resolver, IssueUserInterest, and Module/Issue relationships) so
the deduplication works across databases.
- Around line 54-67: The GraphQL field resolvers currently mark program,
project_id and project_name as nullable but the underlying Module model has
non-nullable ForeignKeys, so update the return types: change program(self, root:
Module) -> ProgramNode (remove | None), project_id(self, root: Module) ->
strawberry.ID (remove | None), and project_name(self, root: Module) -> str
(remove | None); keep resolver bodies the same but tighten the annotated return
types on the methods program, project_id, and project_name to reflect the
required FK guarantees.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f797d150-6984-440c-9820-a0b95e4bdbd7
⛔ Files ignored due to path filters (4)
frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/moduleMutations.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/moduleQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/programsQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (3)
backend/apps/mentorship/api/internal/nodes/module.pybackend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.pyfrontend/src/types/mentorship.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/mentorship/api/internal/nodes/api_internal_module_test.py
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
3 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.
Resolves #4226
Migrates ModuleNode from @strawberry.type to @strawberry_django.type(Module, ...) for consistency with other nodes in the codebase.
Changes:
Checklist
make check-testlocally: all warnings addressed, tests passed