Implement caching for GraphQL resolvers #2528
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a Strawberry Schema extension CacheExtension that caches GraphQL resolver results (keyed by field name + deterministic JSON args and SHA256 with a settings prefix), skips caching for introspection, non-Query parents, and protected/authenticated fields, registers the extension on the schema, introduces two cache settings, and adds unit tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ 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: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/common/extensions.py(1 hunks)backend/settings/base.py(1 hunks)backend/tests/apps/common/extensions_test.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1718
File: backend/apps/owasp/graphql/queries/project_health_metrics.py:0-0
Timestamp: 2025-07-09T08:41:58.129Z
Learning: In strawberry_django, when using strawberry_django.field decorator with filters, pagination, and ordering parameters, the framework automatically applies these to the QuerySet returned by the resolver method. Manual application using strawberry_django.filters.apply() is only needed for custom implementations, not when using the standard decorator approach.
🧬 Code graph analysis (1)
backend/tests/apps/common/extensions_test.py (1)
backend/apps/common/extensions.py (2)
CacheFieldExtension(10-48)resolve(39-48)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
rudransh-shrivastava
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/apps/common/extensions.py (1)
36-54: Consider cache key length limits for complex queries.Cache key generation looks solid—the use of
is not Nonecorrectly handles all valid source IDs, andDjangoJSONEncoderaddresses scalar serialization. However, complexkwargs(nested filters, large lists) may produce keys exceeding backend limits (e.g., Memcached's 250-character limit). Consider hashing theargs_strif you observe key-length errors in production.Optional approach if key length becomes an issue:
import hashlib # In generate_key method, replace line 54: args_hash = hashlib.md5(args_str.encode()).hexdigest() return f"{self.prefix}:{self._convert_path_to_str(info.path)}:{args_hash}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/common/extensions.py(1 hunks)backend/tests/apps/common/extensions_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/common/extensions_test.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
📚 Learning: 2025-06-29T00:41:32.198Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1676
File: backend/apps/owasp/graphql/filters/project_health_metrics.py:17-22
Timestamp: 2025-06-29T00:41:32.198Z
Learning: In the OWASP Nest codebase, when implementing GraphQL filters that convert string values to enums (like ProjectLevel), do not catch ValueError exceptions for invalid values. Let the errors propagate to provide proper error responses to GraphQL clients rather than silently ignoring invalid input.
Applied to files:
backend/apps/common/extensions.py
📚 Learning: 2025-07-31T07:05:25.056Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 0
File: :0-0
Timestamp: 2025-07-31T07:05:25.056Z
Learning: In the OWASP Nest project, Django views may not properly access authenticated users from sessions created by Strawberry GraphQL mutations. The issue occurs because Django's AuthenticationMiddleware doesn't always populate request.user from session data that GraphQL context successfully uses via info.context.request.user. The solution is to manually resolve users from session data using request.session.get('_auth_user_id') and User.objects.select_related('github_user').get(pk=user_id) to match the same authentication mechanism used by GraphQL context.
Applied to files:
backend/apps/common/extensions.py
📚 Learning: 2025-09-21T17:04:48.154Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Applied to files:
backend/apps/common/extensions.py
📚 Learning: 2025-07-03T03:47:05.436Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1675
File: backend/apps/owasp/graphql/queries/project_health_metrics.py:13-21
Timestamp: 2025-07-03T03:47:05.436Z
Learning: The OWASP Nest project does not use async operations anywhere in the codebase and doesn't have the required database driver for async operations. All database methods and GraphQL resolvers should remain synchronous to maintain consistency with the existing architecture.
Applied to files:
backend/apps/common/extensions.py
🔇 Additional comments (2)
backend/apps/common/extensions.py (2)
27-34: LGTM!The path-to-string conversion correctly traverses the Strawberry path linked list and handles the reversal to maintain root-to-leaf order.
56-64: LGTM!The use of
cache.get_or_set()is the idiomatic Django pattern and correctly handles cache misses, falsy values, and thread safety. This synchronous implementation aligns with your codebase's resolver architecture.
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 (7)
backend/apps/mentorship/api/internal/queries/module.py (1)
19-27: Critical: Caching unevaluated QuerySets defeats the purpose of caching.Both
get_program_modulesandget_project_modulesreturn Django QuerySets, which are lazy and only evaluated when accessed. TheCacheFieldExtension.resolve()method caches the return value of the resolver before Strawberry evaluates it. This means you're caching the unevaluated QuerySet object itself (essentially just the query definition), not the actual data.When the cached QuerySet is retrieved and Strawberry attempts to serialize it, the QuerySet must be re-evaluated against the database, defeating the purpose of caching. Additionally, QuerySets may not pickle correctly depending on your cache backend.
Solution: Force evaluation of the QuerySet before returning by wrapping it in
list():@strawberry.field(extensions=[CacheFieldExtension()]) def get_program_modules(self, program_key: str) -> list[ModuleNode]: """Get all modules by program Key. Returns an empty list if program is not found.""" - return ( + return list( Module.objects.filter(program__key=program_key) .select_related("program", "project") .prefetch_related("mentors__github_user") .order_by("started_at") )@strawberry.field(extensions=[CacheFieldExtension()]) def get_project_modules(self, project_key: str) -> list[ModuleNode]: """Get all modules by project Key. Returns an empty list if project is not found.""" - return ( + return list( Module.objects.filter(project__key=project_key) .select_related("program", "project") .prefetch_related("mentors__github_user") .order_by("started_at") )Note: This issue likely affects other GraphQL resolvers in this PR that return QuerySets. You may want to audit all files where
CacheFieldExtensionwas added.Also applies to: 29-37
backend/apps/github/api/internal/nodes/user.py (1)
36-51: Consider shorter cache timeout for user badges.User badges change when users earn new achievements. A 24-hour cache means newly earned badges won't appear until the cache expires, which could negatively impact user experience.
Consider one of the following:
- Use a shorter cache timeout (e.g., 5-15 minutes) for better freshness
- Implement cache invalidation when badges are awarded
- Override the default timeout for this field:
- @strawberry.field(extensions=[CacheFieldExtension()]) + @strawberry.field(extensions=[CacheFieldExtension(cache_timeout=900)]) # 15 minutes def badges(self) -> list[BadgeNode]:backend/apps/github/api/internal/queries/release.py (1)
15-68: 24-hour cache inappropriate for "recent_releases" query.The query name "recent_releases" implies time-sensitive data, but the 24-hour default cache timeout means newly published releases won't appear for up to a day. This creates a poor user experience where "recent" data is actually stale.
Use a shorter cache timeout appropriate for time-sensitive data:
- @strawberry.field(extensions=[CacheFieldExtension()]) + @strawberry.field(extensions=[CacheFieldExtension(cache_timeout=1800)]) # 30 minutes def recent_releases(Consider cache timeouts in the 15-60 minute range for "recent" queries to balance performance with data freshness.
backend/apps/github/api/internal/queries/issue.py (1)
15-66: 24-hour cache inappropriate for "recent_issues" query.Caching "recent issues" for 24 hours defeats the purpose of a "recent" query—newly created issues won't appear for up to a day. This is especially problematic for active projects where issues are created frequently.
Use a much shorter cache timeout for time-sensitive queries:
- @strawberry.field(extensions=[CacheFieldExtension()]) + @strawberry.field(extensions=[CacheFieldExtension(cache_timeout=900)]) # 15 minutes def recent_issues(Consider cache timeouts in the 5-30 minute range to ensure users see recently created issues while still benefiting from caching.
backend/apps/github/api/internal/queries/milestone.py (1)
16-81: 24-hour cache inappropriate for "recent_milestones" query.The query returns "recent milestones" ordered by creation date, but 24-hour caching means newly created milestones won't appear for up to a day. Additionally, milestone state changes (open → closed) won't be reflected until cache expiration.
Use a shorter cache timeout for time-sensitive milestone queries:
- @strawberry.field(extensions=[CacheFieldExtension()]) + @strawberry.field(extensions=[CacheFieldExtension(cache_timeout=1800)]) # 30 minutes def recent_milestones(Consider cache timeouts in the 15-60 minute range to balance performance with freshness for milestone data.
backend/apps/github/api/internal/queries/pull_request.py (1)
64-69: Fix potential AttributeError when project is not found.If no project matches the filter,
.first()returnsNone, and accessing.repositoriesonNonewill raise anAttributeError.Apply this diff to handle the missing project case:
if project: - queryset = queryset.filter( - repository_id__in=Project.objects.filter(key__iexact=f"www-project-{project}") - .first() - .repositories.values_list("id", flat=True) - ) + project_obj = Project.objects.filter(key__iexact=f"www-project-{project}").first() + if project_obj: + queryset = queryset.filter( + repository_id__in=project_obj.repositories.values_list("id", flat=True) + ) + else: + queryset = queryset.none()backend/apps/owasp/api/internal/queries/project.py (1)
56-67: Use shorter cache timeout for authorization checks.This field performs an authorization check that determines project leadership status. Caching authorization data with the default timeout risks serving stale permissions, which could allow unauthorized access or incorrectly deny access when leadership changes occur.
Consider one of these approaches:
- Use a much shorter cache timeout for this specific field:
- @strawberry.field(extensions=[CacheFieldExtension()]) + @strawberry.field(extensions=[CacheFieldExtension(cache_timeout=60)]) # 1 minute instead of default def is_project_leader(self, info: strawberry.Info, login: str) -> bool:
Implement cache invalidation when project leadership changes (e.g., via Django signals on Project model save/update).
Remove caching from this field entirely if leadership changes are frequent or if serving stale authorization data poses security risks.
🧹 Nitpick comments (5)
backend/apps/mentorship/api/internal/queries/module.py (1)
19-51: Consider operational aspects of your caching strategy.As you implement caching across GraphQL resolvers, consider these operational aspects:
Cache timeout tuning: The default
GRAPHQL_RESOLVER_CACHE_TIME_SECONDSapplies to all cached fields. Different queries may benefit from different timeouts based on their cost and data freshness requirements. The extension accepts an optionalcache_timeoutparameter if you need per-field customization.Cache monitoring: Implement monitoring for cache hit rates, cache size, and eviction patterns to ensure caching is effective and not causing memory issues.
Cache warming: For frequently accessed queries with expensive computations, consider implementing cache warming strategies on application startup or after data updates.
Documentation: Document which queries are cached and the expected staleness tolerance for each, as this affects the user experience.
Since this PR is in draft and includes a TODO about using the cache extension in high-cost queries, these considerations will help guide your decisions about which queries to cache and with what parameters.
backend/apps/github/api/internal/nodes/pull_request.py (1)
21-24: Reconsider caching simple attribute access.This field simply returns
self.author, which is typically pre-loaded viaselect_related("author")in queries. Caching this adds overhead (cache lookup, serialization) with minimal benefit, and creates a separate cache entry per PullRequestNode instance (due to source ID in the cache key), potentially leading to cache bloat.Consider removing the cache extension from this field unless profiling shows a specific performance benefit.
backend/apps/github/api/internal/nodes/milestone.py (1)
26-29: Reconsider caching simple attribute access.This field returns
self.author, likely pre-loaded viaselect_related. Caching adds overhead with minimal benefit and creates per-instance cache entries, potentially causing cache bloat.Remove the cache extension unless profiling demonstrates a specific performance benefit.
backend/apps/github/api/internal/nodes/release.py (1)
24-27: Reconsider caching simple attribute access.This field returns
self.author, typically pre-loaded viaselect_related. The cache extension adds overhead without meaningful benefit and creates per-instance cache entries.Remove the cache extension unless profiling shows a performance benefit.
backend/apps/github/api/internal/nodes/issue.py (1)
23-26: Reconsider caching simple attribute access.This field returns
self.author, typically pre-loaded viaselect_related. The cache extension adds overhead without meaningful benefit and creates per-instance cache entries.Remove the cache extension unless profiling shows a performance benefit.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
backend/apps/github/api/internal/nodes/issue.py(2 hunks)backend/apps/github/api/internal/nodes/milestone.py(2 hunks)backend/apps/github/api/internal/nodes/organization.py(2 hunks)backend/apps/github/api/internal/nodes/pull_request.py(2 hunks)backend/apps/github/api/internal/nodes/release.py(2 hunks)backend/apps/github/api/internal/nodes/repository.py(4 hunks)backend/apps/github/api/internal/nodes/user.py(2 hunks)backend/apps/github/api/internal/queries/issue.py(2 hunks)backend/apps/github/api/internal/queries/milestone.py(2 hunks)backend/apps/github/api/internal/queries/organization.py(2 hunks)backend/apps/github/api/internal/queries/pull_request.py(2 hunks)backend/apps/github/api/internal/queries/release.py(2 hunks)backend/apps/github/api/internal/queries/repository.py(3 hunks)backend/apps/github/api/internal/queries/repository_contributor.py(2 hunks)backend/apps/github/api/internal/queries/user.py(3 hunks)backend/apps/mentorship/api/internal/nodes/module.py(2 hunks)backend/apps/mentorship/api/internal/nodes/program.py(2 hunks)backend/apps/mentorship/api/internal/queries/mentorship.py(2 hunks)backend/apps/mentorship/api/internal/queries/module.py(4 hunks)backend/apps/mentorship/api/internal/queries/program.py(2 hunks)backend/apps/owasp/api/internal/nodes/board_of_directors.py(2 hunks)backend/apps/owasp/api/internal/nodes/chapter.py(2 hunks)backend/apps/owasp/api/internal/nodes/common.py(3 hunks)backend/apps/owasp/api/internal/nodes/member_snapshot.py(2 hunks)backend/apps/owasp/api/internal/nodes/project.py(4 hunks)backend/apps/owasp/api/internal/nodes/snapshot.py(2 hunks)backend/apps/owasp/api/internal/queries/board_of_directors.py(3 hunks)backend/apps/owasp/api/internal/queries/chapter.py(2 hunks)backend/apps/owasp/api/internal/queries/committee.py(2 hunks)backend/apps/owasp/api/internal/queries/event.py(2 hunks)backend/apps/owasp/api/internal/queries/member_snapshot.py(3 hunks)backend/apps/owasp/api/internal/queries/post.py(2 hunks)backend/apps/owasp/api/internal/queries/project.py(5 hunks)backend/apps/owasp/api/internal/queries/project_health_metrics.py(4 hunks)backend/apps/owasp/api/internal/queries/snapshot.py(3 hunks)backend/apps/owasp/api/internal/queries/sponsor.py(2 hunks)backend/apps/owasp/api/internal/queries/stats.py(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1718
File: backend/apps/owasp/graphql/queries/project_health_metrics.py:0-0
Timestamp: 2025-07-09T08:41:58.129Z
Learning: In strawberry_django, when using strawberry_django.field decorator with filters, pagination, and ordering parameters, the framework automatically applies these to the QuerySet returned by the resolver method. Manual application using strawberry_django.filters.apply() is only needed for custom implementations, not when using the standard decorator approach.
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.
Applied to files:
backend/apps/owasp/api/internal/nodes/member_snapshot.pybackend/apps/mentorship/api/internal/nodes/module.pybackend/apps/owasp/api/internal/queries/chapter.pybackend/apps/owasp/api/internal/nodes/snapshot.pybackend/apps/owasp/api/internal/nodes/chapter.pybackend/apps/mentorship/api/internal/queries/module.py
📚 Learning: 2025-08-31T13:47:15.861Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2155
File: frontend/src/server/queries/programsQueries.ts:81-81
Timestamp: 2025-08-31T13:47:15.861Z
Learning: In frontend/src/server/queries/programsQueries.ts, GET_PROGRAM_DETAILS is actively used in frontend/src/app/my/mentorship/programs/[programKey]/edit/page.tsx for program editing functionality and cannot be removed. It serves a different purpose than GET_PROGRAM_ADMIN_DETAILS, providing comprehensive program information needed for editing.
Applied to files:
backend/apps/mentorship/api/internal/queries/program.py
📚 Learning: 2025-07-13T05:55:46.436Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.
Applied to files:
backend/apps/mentorship/api/internal/queries/program.py
📚 Learning: 2025-07-09T08:41:58.129Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1718
File: backend/apps/owasp/graphql/queries/project_health_metrics.py:0-0
Timestamp: 2025-07-09T08:41:58.129Z
Learning: In strawberry_django, when using strawberry_django.field decorator with filters, pagination, and ordering parameters, the framework automatically applies these to the QuerySet returned by the resolver method. Manual application using strawberry_django.filters.apply() is only needed for custom implementations, not when using the standard decorator approach.
Applied to files:
backend/apps/owasp/api/internal/queries/project_health_metrics.py
🧬 Code graph analysis (37)
backend/apps/owasp/api/internal/nodes/common.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/github/api/internal/queries/release.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/mentorship/api/internal/queries/mentorship.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/owasp/api/internal/nodes/member_snapshot.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/github/api/internal/queries/pull_request.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/mentorship/api/internal/nodes/program.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/github/api/internal/nodes/release.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/owasp/api/internal/queries/post.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/github/api/internal/queries/issue.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/github/api/internal/nodes/organization.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/github/api/internal/nodes/pull_request.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/mentorship/api/internal/queries/program.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/owasp/api/internal/queries/sponsor.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/owasp/api/internal/queries/board_of_directors.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/owasp/api/internal/queries/committee.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/owasp/api/internal/queries/project_health_metrics.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/github/api/internal/queries/user.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/owasp/api/internal/nodes/board_of_directors.py (2)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)backend/apps/owasp/models/board_of_directors.py (1)
get_candidates(41-49)
backend/apps/github/api/internal/queries/repository.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/owasp/api/internal/queries/member_snapshot.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/github/api/internal/queries/repository_contributor.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/mentorship/api/internal/nodes/module.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/owasp/api/internal/queries/stats.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/owasp/api/internal/queries/chapter.py (2)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)backend/apps/owasp/api/internal/nodes/chapter.py (2)
key(50-52)ChapterNode(32-57)
backend/apps/github/api/internal/nodes/user.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/owasp/api/internal/nodes/snapshot.py (2)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)backend/apps/owasp/api/internal/nodes/project.py (1)
ProjectNode(40-113)
backend/apps/github/api/internal/nodes/issue.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/github/api/internal/nodes/repository.py (3)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)backend/apps/github/api/internal/nodes/milestone.py (1)
MilestoneNode(23-51)backend/apps/github/api/internal/nodes/release.py (1)
ReleaseNode(21-55)
backend/apps/owasp/api/internal/queries/event.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/github/api/internal/queries/milestone.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/owasp/api/internal/queries/project.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/owasp/api/internal/nodes/chapter.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/owasp/api/internal/nodes/project.py (6)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)backend/apps/github/api/internal/nodes/issue.py (1)
IssueNode(20-40)backend/apps/github/api/internal/nodes/milestone.py (1)
MilestoneNode(23-51)backend/apps/github/api/internal/nodes/pull_request.py (1)
PullRequestNode(18-43)backend/apps/owasp/models/project.py (1)
pull_requests(226-232)backend/apps/github/api/internal/nodes/release.py (1)
ReleaseNode(21-55)
backend/apps/github/api/internal/queries/organization.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/mentorship/api/internal/queries/module.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/owasp/api/internal/queries/snapshot.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
backend/apps/github/api/internal/nodes/milestone.py (1)
backend/apps/common/extensions.py (1)
CacheFieldExtension(13-64)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (39)
backend/apps/mentorship/api/internal/queries/module.py (2)
8-8: LGTM: Import added correctly.The import of
CacheFieldExtensionis necessary for the caching functionality applied to the query methods below.
39-51: Verify cache staleness implications for single object caching.Unlike the list methods,
get_modulereturns a singleModuleinstance (via.get()), which can be pickled and cached successfully. However, this cached instance can become stale when the underlying Module data is updated in the database.Ensure that:
- The cache timeout (
GRAPHQL_RESOLVER_CACHE_TIME_SECONDS) is appropriately tuned for your data freshness requirements.- You have a cache invalidation strategy in place for when Module objects are created, updated, or deleted.
- Related objects loaded via
select_relatedandprefetch_relatedare also considered in your staleness tolerance.Consider implementing cache invalidation signals or using a shorter timeout for frequently updated data:
# Example cache invalidation signal (to be added in models.py or signals.py) from django.db.models.signals import post_save, post_delete from django.core.cache import cache @receiver([post_save, post_delete], sender=Module) def invalidate_module_cache(sender, instance, **kwargs): # Invalidate relevant cache keys cache.delete_pattern(f"{settings.GRAPHQL_RESOLVER_CACHE_PREFIX}:*get_module*{instance.key}*")backend/apps/owasp/api/internal/queries/event.py (1)
5-5: LGTM! Caching addition is appropriate.The caching extension is correctly applied to the
upcoming_eventsquery field. The cache key will include thelimitparameter, ensuring proper cache segmentation.Also applies to: 14-17
backend/apps/owasp/api/internal/queries/chapter.py (1)
5-5: LGTM! Caching is well-suited for these query fields.Both
chapterandrecent_chaptersfields are appropriately cached with keys that include their respective parameters (keyandlimit).Also applies to: 14-25
backend/apps/owasp/api/internal/queries/stats.py (1)
5-5: LGTM! Excellent caching candidate for expensive aggregations.The
stats_overviewfield performs multiple database queries and aggregations, making it an ideal candidate for caching. Since it returns global statistics with no parameters, all users will share the same cached result.Also applies to: 18-44
backend/apps/mentorship/api/internal/queries/mentorship.py (1)
5-5: LGTM! Appropriate caching for lookup-intensive query.The
is_mentorfield performs database lookups and is parameterized bylogin, making it a good candidate for caching. The cache key will properly differentiate between different login values.Also applies to: 21-34
backend/apps/owasp/api/internal/queries/post.py (1)
5-5: LGTM! Standard caching pattern for recent items.The caching extension is correctly applied with the
limitparameter included in the cache key.Also applies to: 14-17
backend/apps/owasp/api/internal/queries/snapshot.py (1)
5-5: LGTM! Appropriate caching for snapshot queries.Both
snapshotandsnapshotsfields are suitable for caching, with their respective parameters (keyandlimit) properly included in cache key generation.Also applies to: 14-32
backend/apps/mentorship/api/internal/queries/program.py (1)
9-9: LGTM! Caching correctly applied to public query only.The
get_programfield is appropriately cached with theprogram_keyparameter. Note thatmy_programsis correctly NOT cached since it's user-specific withIsAuthenticatedpermission.Also applies to: 23-33
backend/apps/owasp/api/internal/nodes/board_of_directors.py (1)
6-6: Caching applied consistently to member resolution fields.The
CacheFieldExtensionis properly applied to bothcandidatesandmembersfields, which should reduce database load for board directory queries.However, consider that board member and candidate data cached for 24 hours may become stale if entity member status changes (e.g., a candidate is added/removed, review status changes). Verify whether this cache duration aligns with your data freshness requirements, or consider implementing cache invalidation signals on the
EntityMembermodel to clear related cache entries when changes occur.Also applies to: 22-30
backend/apps/owasp/api/internal/nodes/chapter.py (1)
40-47: LGTM! Appropriate caching for computed geographic data.Caching the
geo_locationfield makes sense since it's a computed value from latitude/longitude, which are relatively stable for chapters. The selective application (not caching simple indexed fields) demonstrates good judgment about what benefits from caching.backend/apps/mentorship/api/internal/nodes/program.py (1)
32-35: Caching applied to program admins field.The caching extension is properly applied to reduce database queries for program administrators.
Consider whether the 24-hour cache duration is appropriate if program admins change frequently. If admin roster changes need to be visible immediately, you may want to implement cache invalidation when program admin relationships are modified.
backend/apps/owasp/api/internal/nodes/common.py (1)
14-17: LGTM! Base class caching benefits all entity types.Applying caching to
entity_leadersandtop_contributorsin theGenericEntityNodebase class is an efficient approach that benefits all entity types (chapters, projects, committees, etc.) without code duplication.Also applies to: 29-32
backend/apps/owasp/api/internal/queries/board_of_directors.py (1)
14-28: LGTM! Query-level caching properly parameterized.Both query methods correctly apply caching with proper argument handling—the cache keys will differentiate between different years and limit values, preventing incorrect cache hits.
Also applies to: 30-41
backend/apps/owasp/api/internal/queries/sponsor.py (1)
14-27: LGTM! Caching benefits the in-memory sorting.Caching the
sponsorsquery is beneficial since it includes in-memory sorting logic that would otherwise execute on every request.Note that since this query has no parameters, all users/requests share the same cache entry. Changes to sponsor data (additions, type changes) won't be visible until the cache expires (24 hours by default). Verify this latency is acceptable, or consider implementing cache invalidation when sponsor records are modified.
backend/apps/owasp/api/internal/queries/committee.py (1)
14-29: LGTM! Caching applied with proper parameter handling.The
committeequery correctly applies caching, and the cache key will include the committee key parameter to prevent cache collisions between different committees.backend/apps/owasp/api/internal/queries/project_health_metrics.py (1)
19-44: LGTM! Caching appropriately applied to health metrics queries.The three health metrics fields are good candidates for caching since they likely involve aggregations and complex queries. The cache keys will properly differentiate between different filter/pagination/ordering combinations, preventing incorrect cache hits.
Note: The manual application of filters in
project_health_metrics_distinct_length(line 79) is appropriate for this use case where you need the count after filtering but outside the standard decorator flow. Based on learnings.Also applies to: 46-57, 59-81
backend/apps/owasp/api/internal/nodes/snapshot.py (1)
35-58: Caching appears appropriate for snapshot fields, but verify data immutability.The cache extension is applied to five relationship fields that return collections of related entities. Since snapshots typically represent immutable point-in-time data, 24-hour caching should be safe.
Please confirm that:
- Snapshot entities and their related data (new_chapters, new_issues, etc.) don't change after the snapshot is created
- The relationships themselves remain stable over the snapshot's lifetime
If snapshot data can be modified or relationships can change post-creation, consider a shorter cache timeout or implement cache invalidation on updates.
backend/apps/github/api/internal/queries/repository.py (2)
38-63: Consider shorter cache timeout for repository lists.The
repositories()query returns a list ordered by stars, which can change as repositories gain or lose stars. With 24-hour caching, the ordering and star counts will be stale.Verify whether 24-hour staleness is acceptable for repository listings, particularly since they're ordered by a frequently-changing metric (stars_count).
14-36: Consider implementing cache invalidation for repository metadata updates.Repository data (stars, forks, pushed_at, etc.) can change frequently. The current 24-hour cache timeout for the
repository()query means stale metadata will persist until expiration, potentially impacting active repositories.This is particularly important since repositories are synced via management commands (e.g.,
sync_repository()) that could trigger frequent updates without corresponding cache clears.Recommendations:
- Implement cache invalidation via Django signals when Repository objects are updated (e.g.,
post_save)- Consider shorter cache timeouts (1-6 hours) to balance freshness and performance
- Or configure webhook handlers to clear the GraphQL cache when repository data changes upstream
backend/apps/github/api/internal/queries/organization.py (1)
14-32: Caching appears reasonable for organization queries.Organization metadata is relatively stable (name, avatar, description), making 24-hour caching more appropriate than for "recent" queries. The cache key includes the login parameter, ensuring different organizations have separate cache entries.
Verify that 24-hour staleness is acceptable for organization metadata updates (e.g., avatar changes, description updates). If organizations frequently update their profiles, consider a shorter timeout (e.g., 6-12 hours).
backend/apps/owasp/api/internal/nodes/member_snapshot.py (1)
32-35: Caching appears appropriate for snapshot's github_user field.Since MemberSnapshot appears to represent point-in-time data, caching the related github_user field is reasonable. The cache key includes the snapshot ID, so each snapshot has its own cache entry.
Please confirm that MemberSnapshot entities are immutable and that the github_user relationship doesn't change after snapshot creation. If snapshots can be modified, consider a shorter cache timeout.
backend/apps/github/api/internal/queries/pull_request.py (1)
16-90: Confirm cache invalidation strategy for pull request data.Caching this query is appropriate given the complex filtering logic. However, ensure that cache entries are invalidated when pull requests are created, updated, or deleted to prevent serving stale data.
How will cache invalidation be handled when pull request data changes? Consider using Django signals or explicit cache invalidation in model save/delete methods.
backend/apps/github/api/internal/queries/user.py (1)
16-38: Confirm cache invalidation strategy for user-related queries.Caching these user-related queries is appropriate. However, ensure cache entries are invalidated when:
- User data changes (profile updates, name changes, etc.)
- Repository contributions are updated
How will you handle cache invalidation when user or contribution data changes?
Also applies to: 40-54
backend/apps/github/api/internal/nodes/organization.py (1)
43-72: Good use of caching for expensive aggregation.This field performs multiple database queries and aggregations, making it an excellent candidate for caching. Each organization will have its own cache entry, which is appropriate for this use case.
Ensure cache invalidation when repository stats change (stars, forks, issues, contributors). Consider invalidating on repository updates or using a periodic cache refresh.
backend/apps/github/api/internal/queries/repository_contributor.py (1)
14-65: Confirm cache invalidation for contributor rankings.Caching this complex query with multiple filter parameters is appropriate for performance. However, ensure the cache is invalidated when contribution data changes to keep rankings current.
How will cache invalidation be handled when repository contributions are updated?
backend/apps/owasp/api/internal/queries/project.py (4)
6-6: LGTM: Import added for caching extension.The import is correctly placed and necessary for the field-level caching functionality.
16-30: Verify cache invalidation strategy for project data.Caching the project lookup is reasonable for performance. However, ensure that cached project data is invalidated when the underlying Project model is updated (e.g., name changes, is_active status changes).
Consider implementing Django signals or cache invalidation in the Project model's save/delete methods to clear related cache entries when data changes.
32-43: Verify cache invalidation for dynamic project lists.Caching recent projects improves performance but may serve stale data when new projects are created or existing projects'
is_activestatus changes. Ensure the cache invalidation strategy handles these scenarios.
45-54: Verify acceptable staleness for search results.Caching search results is beneficial for frequently searched terms. However, newly created projects or status changes won't be reflected until the cache expires. Verify that the configured cache timeout provides an acceptable balance between performance and data freshness for search functionality.
backend/apps/owasp/api/internal/queries/member_snapshot.py (3)
5-5: LGTM: Import added for caching extension.
15-40: LGTM: Appropriate caching for snapshot queries.Member snapshots are historical data that change infrequently, making them excellent candidates for caching. The cache key will properly distinguish between different user_login and start_year parameter combinations.
42-65: LGTM: Snapshot list caching is appropriate.The historical nature of member snapshot data makes caching beneficial for performance without significant staleness concerns.
backend/apps/github/api/internal/nodes/repository.py (2)
8-8: LGTM: Import added for caching extension.
45-92: Verify cache invalidation when repository data is synchronized.The cached fields (issues, organization, project, recent_milestones, releases, top_contributors) will serve stale data after GitHub data synchronization updates the repository. Ensure your data sync process invalidates or refreshes these cached entries.
The cache key generation properly includes the source repository ID, ensuring separate cache entries for different repositories. However, consider implementing cache invalidation hooks in your GitHub data sync workflow to clear cached entries for updated repositories.
backend/apps/owasp/api/internal/nodes/project.py (4)
6-6: LGTM: Import added for caching extension.
43-61: Verify cache invalidation for health metrics updates.The health metrics fields query time-series data that grows when new metrics are calculated. Cached results, especially for
health_metrics_latest, must be invalidated when new ProjectHealthMetrics records are created to ensure users see the most recent data.Implement cache invalidation when new ProjectHealthMetrics records are created, either via Django signals or within your metrics calculation workflow.
78-98: Verify cache invalidation for recent GitHub activity.The recent activity fields (recent_issues, recent_milestones, recent_pull_requests, recent_releases) will serve stale data after GitHub synchronization. Ensure your sync process invalidates these cached entries for affected projects.
100-103: Verify cache invalidation when project repositories change.The repositories list should be invalidated when repositories are added, removed, or have their metadata updated (especially pushed_at and updated_at timestamps that affect ordering).
|
rudransh-shrivastava
force-pushed
the
feature/graphql-caching
branch
from
6e2d121 to
11b8c1f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/common/extensions.py(1 hunks)backend/settings/base.py(1 hunks)backend/settings/graphql.py(2 hunks)backend/tests/apps/common/extensions_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/settings/base.py
- backend/apps/common/extensions.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
🧬 Code graph analysis (2)
backend/settings/graphql.py (1)
backend/apps/common/extensions.py (1)
CacheExtension(13-61)
backend/tests/apps/common/extensions_test.py (1)
backend/apps/common/extensions.py (3)
generate_key(16-29)should_skip_cache(31-45)on_execute(47-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (7)
backend/settings/graphql.py (2)
7-7: LGTM!The import is correct and necessary for the schema extension.
44-44: LGTM!The schema extension integration is correct and follows Strawberry GraphQL conventions.
backend/tests/apps/common/extensions_test.py (5)
15-37: LGTM!The fixtures are well-structured and provide appropriate test setup.
39-76: LGTM!The key generation tests are comprehensive and correctly verify deterministic hashing, query differentiation, and variable differentiation.
79-116: LGTM!The cache-skipping logic tests are comprehensive and correctly cover all operation types and authentication states.
121-143: LGTM!The test fixtures are appropriately configured for testing the on_execute flow.
145-203: LGTM!These tests correctly verify the caching behavior for mutations, authenticated queries, cache hits, and cache misses.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
@rudransh-shrivastava did you run this code? I don't think the caching extension works at all.
|
@arkid15r yes I did, I had log statements everywhere and I've tested against unauthenticated, authenticated queries and mutations. I'll double check in a while. |
|
@arkid15r you're right, I added |
|
I found the issue. It's comparing enums incorrectly, and it's skipping the queries if the user is authenticated. I intended the authenticated queries not to be cached, but I think I need a better check for fields that require authentication instead. |
Yes, you've identified both issues preventing caching correctly. It looked to me like a cosplay code at some point 🤷♂️ |
|
@arkid15r Yes, I understand. I did write the extension entirely without AI, but I also wrote the bug. The tests are correct, but they test the wrong flow, which I thought was right. |
|
According to the docs (https://strawberry.rocks/docs/guides/custom-extensions), the two hooks that I can use here are For example, with the following query, I'll need to check each resolver separately. But the hooks don't provide information on query MyQuery {
chapter(key: "name") {
name
leaders
key
isActive
meetupGroup
region
relatedUrls
}
activeApiKeyCount
}@strawberry.field(permission_classes=[IsAuthenticated])
def active_api_key_count(self, info: Info) -> int:I think I can build a list of non-cacheable resolvers dynamically using the schema and use that to skip caching. I'll try this. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/apps/common/extensions.py (1)
49-52: Verify handling of non-JSON-serializable field arguments.A previous review flagged that
json.dumpswill raiseTypeErrorfor arguments likedatetime,UUID, orDecimal. The comment was marked as addressed, but the current code doesn't includeDjangoJSONEncoderor adefault=strfallback.Please verify whether:
- The GraphQL schema has no fields with such argument types, or
- The fix was intentionally reverted, or
- This should still be addressed
If needed, apply:
+from django.core.serializers.json import DjangoJSONEncoder ... - key = f"{field_name}:{json.dumps(field_args, sort_keys=True)}" + key = f"{field_name}:{json.dumps(field_args, sort_keys=True, cls=DjangoJSONEncoder)}"
🧹 Nitpick comments (1)
backend/tests/apps/common/extensions_test.py (1)
143-152: Consider verifying the resolver is not called on cache hit.The test verifies the cached result is returned but doesn't confirm that the underlying resolver (
mock_next) was not invoked. Adding this assertion would strengthen the test.assert result == cached_result mock_cache.get_or_set.assert_called_once() + mock_next.assert_not_called()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/common/extensions.py(1 hunks)backend/tests/apps/common/extensions_test.py(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
📚 Learning: 2025-09-21T17:04:48.154Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Applied to files:
backend/tests/apps/common/extensions_test.pybackend/apps/common/extensions.py
📚 Learning: 2025-09-21T11:34:33.377Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/about/page.tsx:47-53
Timestamp: 2025-09-21T11:34:33.377Z
Learning: User rudransh-shrivastava prefers to keep Apollo Client migration PRs focused solely on migration-related changes and not include additional bug fixes or improvements that aren't directly related to the migration itself.
Applied to files:
backend/tests/apps/common/extensions_test.pybackend/apps/common/extensions.py
📚 Learning: 2025-06-29T00:41:32.198Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1676
File: backend/apps/owasp/graphql/filters/project_health_metrics.py:17-22
Timestamp: 2025-06-29T00:41:32.198Z
Learning: In the OWASP Nest codebase, when implementing GraphQL filters that convert string values to enums (like ProjectLevel), do not catch ValueError exceptions for invalid values. Let the errors propagate to provide proper error responses to GraphQL clients rather than silently ignoring invalid input.
Applied to files:
backend/apps/common/extensions.py
📚 Learning: 2025-07-31T07:05:25.056Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 0
File: :0-0
Timestamp: 2025-07-31T07:05:25.056Z
Learning: In the OWASP Nest project, Django views may not properly access authenticated users from sessions created by Strawberry GraphQL mutations. The issue occurs because Django's AuthenticationMiddleware doesn't always populate request.user from session data that GraphQL context successfully uses via info.context.request.user. The solution is to manually resolve users from session data using request.session.get('_auth_user_id') and User.objects.select_related('github_user').get(pk=user_id) to match the same authentication mechanism used by GraphQL context.
Applied to files:
backend/apps/common/extensions.py
📚 Learning: 2025-07-03T03:47:05.436Z
Learnt from: ahmedxgouda
Repo: OWASP/Nest PR: 1675
File: backend/apps/owasp/graphql/queries/project_health_metrics.py:13-21
Timestamp: 2025-07-03T03:47:05.436Z
Learning: The OWASP Nest project does not use async operations anywhere in the codebase and doesn't have the required database driver for async operations. All database methods and GraphQL resolvers should remain synchronous to maintain consistency with the existing architecture.
Applied to files:
backend/apps/common/extensions.py
🧬 Code graph analysis (1)
backend/tests/apps/common/extensions_test.py (2)
backend/apps/common/extensions.py (3)
get_protected_fields(16-32)generate_key(38-52)resolve(54-67)backend/apps/mentorship/api/internal/nodes/mentor.py (1)
name(18-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (4)
backend/tests/apps/common/extensions_test.py (2)
11-47: LGTM!The key generation tests are comprehensive, covering determinism, field name variation, argument variation, and argument ordering consistency.
50-84: LGTM!Good use of
cache_clear()before each test to ensure isolation with thelru_cachedecorator. The mock schema correctly simulates protected vs public fields.backend/apps/common/extensions.py (2)
15-32: LGTM!The
lru_cache(maxsize=1)is appropriate since the schema is typically a singleton, and returning a tuple ensures immutability. The logic correctly identifies fields withPermissionExtension.
54-67: LGTM!The caching logic correctly:
- Bypasses introspection queries, non-Query parent types, and protected fields
- Uses
cache.get_or_setwhich is the Django-idiomatic approach and correctly handles all value types including falsy values
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/tests/apps/common/extensions_test.py (1)
144-163: Consider verifying cache.get_or_set call arguments.The cache hit and miss tests correctly validate the core behavior, but they don't verify the exact arguments passed to
cache.get_or_set. For more thorough coverage, consider asserting:
- The cache key matches the expected output from
generate_key(field_name, kwargs)- The timeout value matches
settings.GRAPHQL_RESOLVER_CACHE_TIME_SECONDSExample enhancement for the cache hit test:
@patch("apps.common.extensions.cache") def test_returns_cached_result_on_hit(self, mock_cache, extension, mock_info, mock_next): """Test that cached result is returned on cache hit.""" cached_result = {"name": "Cached OWASP"} mock_cache.get_or_set.return_value = cached_result result = extension.resolve(mock_next, None, mock_info, key="germany") assert result == cached_result - mock_cache.get_or_set.assert_called_once() + expected_key = extension.generate_key("chapter", {"key": "germany"}) + call_args = mock_cache.get_or_set.call_args + assert call_args[0][0] == expected_key # Verify cache key + assert "timeout" in call_args[1] or len(call_args[0]) > 2 # Verify timeout passed mock_next.assert_not_called()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/apps/common/extensions_test.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
📚 Learning: 2025-09-21T17:04:48.154Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/snapshots/[id]/page.tsx:0-0
Timestamp: 2025-09-21T17:04:48.154Z
Learning: User rudransh-shrivastava confirmed that suggested type safety improvements during Apollo Client migration were no longer relevant, reinforcing their preference to keep migration PRs focused on core migration changes rather than additional improvements.
Applied to files:
backend/tests/apps/common/extensions_test.py
📚 Learning: 2025-09-21T11:34:33.377Z
Learnt from: rudransh-shrivastava
Repo: OWASP/Nest PR: 2178
File: frontend/src/app/about/page.tsx:47-53
Timestamp: 2025-09-21T11:34:33.377Z
Learning: User rudransh-shrivastava prefers to keep Apollo Client migration PRs focused solely on migration-related changes and not include additional bug fixes or improvements that aren't directly related to the migration itself.
Applied to files:
backend/tests/apps/common/extensions_test.py
🧬 Code graph analysis (1)
backend/tests/apps/common/extensions_test.py (1)
backend/apps/common/extensions.py (3)
get_protected_fields(16-32)generate_key(38-52)resolve(54-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (4)
backend/tests/apps/common/extensions_test.py (4)
11-47: LGTM! Comprehensive key generation tests.The test cases properly verify deterministic hashing, uniqueness for different inputs, and argument order independence. The assertion on line 26 correctly validates the SHA256 digest length.
50-84: LGTM! Protected fields detection is well-tested.The cache clearing on lines 73 and 81 properly prevents test pollution. The mock schema setup correctly simulates the PermissionExtension behavior, and the tests verify both the camelCase conversion and return type.
116-141: LGTM! Cache skipping logic is thoroughly tested.The three skip scenarios (introspection, non-Query types, and protected fields) are properly validated. Each test correctly modifies the relevant attribute and verifies that the resolver is called directly without caching.
1-163: Excellent test coverage for CacheExtension.The test suite comprehensively covers key generation, protected field detection, and resolver caching behavior. The test structure is clean with well-organized fixtures, and the test names clearly describe what's being validated. The removal of the previously discussed falsy value parametrized tests (per the resolved comments) was the correct decision based on Strawberry GraphQL's actual behavior.
2 participants
Resolves #2417
Proposed change
Implement caching for GraphQL Endpoints:
Checklist
make check-testlocally; all checks and tests passed.