strawberry-graphql / strawberry-django

Strawberry GraphQL Django extension
MIT License
391 stars 115 forks source link

feat: Nested optimization for lists and connections #540

Closed bellini666 closed 2 weeks ago

bellini666 commented 3 weeks ago

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 and ListConnectionWithTotalCount, 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

codecov-commenter commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 88.98305% with 13 lines in your changes missing coverage. Please review.

Project coverage is 88.67%. Comparing base (eec919c) to head (305cacb).

Files Patch % Lines
strawberry_django/optimizer.py 83.67% 8 Missing :warning:
strawberry_django/relay.py 89.65% 3 Missing :warning:
strawberry_django/fields/field.py 80.00% 1 Missing :warning:
strawberry_django/pagination.py 95.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Eraldo commented 3 weeks ago

Note: Although nested connections are being optimized as well, we are only doing that for those annotated with ListConnection and ListConnectionWithTotalCount, as custom connections may contain more complex code which are not safe to be optimized the way we are doing here.

Does this also apply to connections that inherit from ListConnection or ListConnectionWithTotalCount? 🤔

Eraldo commented 3 weeks ago

@bellini666 As you suggested, I tried using this PR. I upgraded my minimal reproduction and it does not solve issue: https://github.com/strawberry-graphql/strawberry-django/issues/535

bellini666 commented 3 weeks ago

Note: Although nested connections are being optimized as well, we are only doing that for those annotated with ListConnection and ListConnectionWithTotalCount, as custom connections may contain more complex code which are not safe to be optimized the way we are doing here.

Does this also apply to connections that inherit from ListConnection or ListConnectionWithTotalCount? 🤔

Yes, because it is not safe if you are doing customizations, as the optimizer will actually change what nodes is

But if you are doing FruitConnection = ListConnectionWithTotalCount[Fruit] then you are safe.

But if you still need to optimize a connection subclass, we can think of a way of doing that... maybe by defining a ALLOW_UNSAFE_OPTIMIZATIONS = True in the class?