-
-
Notifications
You must be signed in to change notification settings - Fork 118
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
feat: Nested optimization for lists and connections #540
Conversation
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 @bellini666 - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 7 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #540 +/- ##
==========================================
+ Coverage 88.60% 88.67% +0.07%
==========================================
Files 41 41
Lines 3439 3524 +85
==========================================
+ Hits 3047 3125 +78
- Misses 392 399 +7 ☔ View full report in Codecov by Sentry. |
74e9f84
to
3faaa30
Compare
if limit >= 0: | ||
queryset = queryset.filter(_strawberry_row_number__lte=offset + limit) |
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.
should we enforce a limit?
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.
This is just adding support to do the pagination from the optimizer.
I agree that we maybe should enforce a limit, but I'd do that in another PR focused on that
|
||
def get_queryset( | ||
self, | ||
queryset: _QS, | ||
info: Info, | ||
*, | ||
pagination: Optional[object] = None, | ||
_strawberry_related_field_id: Optional[str] = None, |
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.
why is this private? 😊
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.
This is passed in **kwargs
. Wanted to make sure this would not conflict at all with some user-given argument in the resolver.
But maybe the leading strawberry_
is enough? What do you think?
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.
probably, but it is fine to keep the underscore
3faaa30
to
5e18fb5
Compare
|
||
def get_queryset( | ||
self, | ||
queryset: _QS, | ||
info: Info, | ||
*, | ||
pagination: Optional[object] = None, | ||
_strawberry_related_field_id: Optional[str] = None, |
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.
probably, but it is fine to keep the underscore
Does this also apply to connections that inherit from |
@bellini666 As you suggested, I tried using this PR. |
Yes, because it is not safe if you are doing customizations, as the optimizer will actually change what But if you are doing But if you still need to optimize a connection subclass, we can think of a way of doing that... maybe by defining a |
5e18fb5
to
6747fa3
Compare
70d1032
to
26ac62f
Compare
26ac62f
to
305cacb
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.
🔥
This is going to enable optimization of nested relations even when using filters/ordering/pagination, which previously would mean throwing the results away and fetching new ones, causing more than n+1 issues.
Note: Although nested connections are being optimized as well, we are only doing that for those annotated with
ListConnection
andListConnectionWithTotalCount
, as custom connections may contain more complex code which are not safe to be optimized the way we are doing here.We are also dropping support for Django versions older than 4.2 as we cannot filter over window functions in older versions. That is also a recommendation from Django itself when 5.0 was released: https://docs.djangoproject.com/en/5.0/releases/5.0
Fix #337
Fix #340
Fix #345
Fix #389
Fix #521