-
-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(optimizer): Prevent issuing duplicated queries for certain uses of first() and get() #675
fix(optimizer): Prevent issuing duplicated queries for certain uses of first() and get() #675
Conversation
…f first() and get()
Reviewer's Guide by SourceryThis PR optimizes the handling of prefetched QuerySets in StrawberryDjangoField by preventing duplicate queries when using first() and get() methods. The implementation checks if a QuerySet is prefetched and, if so, mimics the behavior of these methods using the prefetched data instead of executing additional database queries. Sequence diagram for optimized QuerySet handlingsequenceDiagram
participant User
participant StrawberryDjangoField
participant QuerySet
participant Database
User->>StrawberryDjangoField: Request data
StrawberryDjangoField->>QuerySet: Check if prefetched
alt Prefetched
QuerySet->>StrawberryDjangoField: Return prefetched data
StrawberryDjangoField->>User: Return data
else Not prefetched
QuerySet->>Database: Execute query
Database->>QuerySet: Return data
QuerySet->>StrawberryDjangoField: Return data
StrawberryDjangoField->>User: Return data
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @diesieben07 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…f first() and get()
…fetching-for-single-fields' into strawberry-graphqlGH-662/fix-prefetching-for-single-fields # Conflicts: # strawberry_django/fields/field.py # tests/test_optimizer.py
for more information, see https://pre-commit.ci
@bellini666 I'm not sure why the type check is failing. What is "Error: Fields without default values cannot appear after fields with default values (reportGeneralTypeIssues)" referring to? Any guidance would be appreciated if you can. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #675 +/- ##
==========================================
+ Coverage 88.99% 89.08% +0.09%
==========================================
Files 41 41
Lines 3752 3767 +15
==========================================
+ Hits 3339 3356 +17
+ Misses 413 411 -2 ☔ View full report in Codecov by Sentry. |
2dd6fa6
to
3f81116
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ty! :)
I've made a small adjustment to use qs._result_cache
instead of len(qs)
. Although it should be ok, I think it's better to be safe than sorry 😅
a98263b
to
913c5f6
Compare
Thank you! :) |
For "many-type" fields (i.e. ManyToManyField or a reverse FK) when the corresponding GraphQL field has a singular result, StrawberryDjangoField will use
QuerySet#get
andQuerySet#first
to produce the result. This prevents the QuerySet from using any prefetched data, because those methods do an implicit slice.This PR adds a check to not call get/first in case the QuerySet is prefetched and instead mimic their behavior to prevent duplicated queries.
Types of Changes
Issues Fixed or Closed by This PR
Fixes #662
Checklist
Summary by Sourcery
Fix duplicated queries in StrawberryDjangoField by avoiding get() and first() on prefetched QuerySets and add tests to verify the fix.
Bug Fixes:
Tests: