Skip to content
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

Nested queries are not properly optimized in graphene-django v3.0.0 (but are in v3.0.0b7) #86

Closed
sjdemartini opened this issue Oct 19, 2022 · 4 comments

Comments

@sjdemartini
Copy link
Collaborator

graphene-django just released a non-beta version of v3 last month (https://github.com/graphql-python/graphene-django/releases/tag/v3.0.0), and graphene-django-optimizer does not appear to optimize queries properly with the new graphene-django release unfortunately.

The latest version of graphene-django-optimizer (0.9.1) appears to work properly with graphene-django v3.0.0b7 (a beta version released a couple years ago, which is listed in the dev-env-requirements.txt for this repo). But when bumping that, SQL-optimization no longer works in many cases, such as with nested fields requiring prefetch_related.

See branch here with version bumps and failing tests: #85

@sjdemartini sjdemartini changed the title Nested queries are not properly optimized in graphene-django v3 (but are in v3.0.0b7) Nested queries are not properly optimized in graphene-django v3.0.0 (but are in v3.0.0b7) Oct 20, 2022
sjdemartini added a commit to sjdemartini/graphene-django that referenced this issue Apr 29, 2023
This reverts the change to `convert_field_to_djangomodel` introduced in
graphql-python#1315 for the
reasons discussed here
https://github.com/graphql-python/graphene-django/pull/1315/files#r1015659857.
As mentioned there, without reverting this code, "queries are forced
every time an object is resolved, making an exponential number of
queries when nesting without any possibility of optimizing".

That regression prevented `graphene-django-optimizer` from working with
`graphene-django` v3.0.0b9+ (where this change first was published), as
discussed in
graphql-python#1356 (comment),
tfoxy/graphene-django-optimizer#86, and
tfoxy/graphene-django-optimizer#83 (comment).

For now, this marks the two tests that depended on this problematic code
as "expected to fail", and perhaps they can be reintroduced if there's a
way to support this logic in a way that does not prevent
`select_related` and `prefetch_related` query-optimization and introduce
nested N+1s.

As mentioned here
graphql-python#1315 (comment),
this is blocking upgrade to graphene-django v3 for many users, and
fixing this would allow many to begin upgrading and contributing to keep
graphene-django going.
@sjdemartini
Copy link
Collaborator Author

I've tested the graphene-django PR here graphql-python/graphene-django#1401 and confirmed that it fixes the failing query-optimization tests mentioned above and should resolve this issue.

firaskafri pushed a commit to graphql-python/graphene-django that referenced this issue May 3, 2023
This reverts the change to `convert_field_to_djangomodel` introduced in
#1315 for the
reasons discussed here
https://github.com/graphql-python/graphene-django/pull/1315/files#r1015659857.
As mentioned there, without reverting this code, "queries are forced
every time an object is resolved, making an exponential number of
queries when nesting without any possibility of optimizing".

That regression prevented `graphene-django-optimizer` from working with
`graphene-django` v3.0.0b9+ (where this change first was published), as
discussed in
#1356 (comment),
tfoxy/graphene-django-optimizer#86, and
tfoxy/graphene-django-optimizer#83 (comment).

For now, this marks the two tests that depended on this problematic code
as "expected to fail", and perhaps they can be reintroduced if there's a
way to support this logic in a way that does not prevent
`select_related` and `prefetch_related` query-optimization and introduce
nested N+1s.

As mentioned here
#1315 (comment),
this is blocking upgrade to graphene-django v3 for many users, and
fixing this would allow many to begin upgrading and contributing to keep
graphene-django going.
@firaskafri
Copy link

@sjdemartini I believe you could close this issue now as your fix was released in graphene-django v3.0.2?

@sjdemartini
Copy link
Collaborator Author

Yep, confirmed this is working with 3.0.2! Thanks for merging and releasing @firaskafri! 🙌

One quick minor side note: I saw in the release notes for https://github.com/graphql-python/graphene-django/releases/tag/v3.0.2 that it says "Just a heads up for folks who use get_queryset as an auth layer. If you need that, you can stick with versions between >=3.0.0b9 and <3.1.0 for now" but I think it should probably say "between >=3.0.0b9 and <3.0.2".

@firaskafri
Copy link

@sjdemartini yes you are right!

igodev0001 pushed a commit to igodev0001/graphene-django that referenced this issue Jul 4, 2023
This reverts the change to `convert_field_to_djangomodel` introduced in
graphql-python/graphene-django#1315 for the
reasons discussed here
https://github.com/graphql-python/graphene-django/pull/1315/files#r1015659857.
As mentioned there, without reverting this code, "queries are forced
every time an object is resolved, making an exponential number of
queries when nesting without any possibility of optimizing".

That regression prevented `graphene-django-optimizer` from working with
`graphene-django` v3.0.0b9+ (where this change first was published), as
discussed in
graphql-python/graphene-django#1356 (comment),
tfoxy/graphene-django-optimizer#86, and
tfoxy/graphene-django-optimizer#83 (comment).

For now, this marks the two tests that depended on this problematic code
as "expected to fail", and perhaps they can be reintroduced if there's a
way to support this logic in a way that does not prevent
`select_related` and `prefetch_related` query-optimization and introduce
nested N+1s.

As mentioned here
graphql-python/graphene-django#1315 (comment),
this is blocking upgrade to graphene-django v3 for many users, and
fixing this would allow many to begin upgrading and contributing to keep
graphene-django going.
superlevure pushed a commit to loft-orbital/graphene-django that referenced this issue Jul 19, 2023
This reverts the change to `convert_field_to_djangomodel` introduced in
graphql-python#1315 for the
reasons discussed here
https://github.com/graphql-python/graphene-django/pull/1315/files#r1015659857.
As mentioned there, without reverting this code, "queries are forced
every time an object is resolved, making an exponential number of
queries when nesting without any possibility of optimizing".

That regression prevented `graphene-django-optimizer` from working with
`graphene-django` v3.0.0b9+ (where this change first was published), as
discussed in
graphql-python#1356 (comment),
tfoxy/graphene-django-optimizer#86, and
tfoxy/graphene-django-optimizer#83 (comment).

For now, this marks the two tests that depended on this problematic code
as "expected to fail", and perhaps they can be reintroduced if there's a
way to support this logic in a way that does not prevent
`select_related` and `prefetch_related` query-optimization and introduce
nested N+1s.

As mentioned here
graphql-python#1315 (comment),
this is blocking upgrade to graphene-django v3 for many users, and
fixing this would allow many to begin upgrading and contributing to keep
graphene-django going.
DevStar1016 added a commit to DevStar1016/graphine-django that referenced this issue Sep 11, 2023
This reverts the change to `convert_field_to_djangomodel` introduced in
graphql-python/graphene-django#1315 for the
reasons discussed here
https://github.com/graphql-python/graphene-django/pull/1315/files#r1015659857.
As mentioned there, without reverting this code, "queries are forced
every time an object is resolved, making an exponential number of
queries when nesting without any possibility of optimizing".

That regression prevented `graphene-django-optimizer` from working with
`graphene-django` v3.0.0b9+ (where this change first was published), as
discussed in
graphql-python/graphene-django#1356 (comment),
tfoxy/graphene-django-optimizer#86, and
tfoxy/graphene-django-optimizer#83 (comment).

For now, this marks the two tests that depended on this problematic code
as "expected to fail", and perhaps they can be reintroduced if there's a
way to support this logic in a way that does not prevent
`select_related` and `prefetch_related` query-optimization and introduce
nested N+1s.

As mentioned here
graphql-python/graphene-django#1315 (comment),
this is blocking upgrade to graphene-django v3 for many users, and
fixing this would allow many to begin upgrading and contributing to keep
graphene-django going.
Yaroslav-Dev007 added a commit to Yaroslav-Dev007/graphene-django that referenced this issue Feb 9, 2025
This reverts the change to `convert_field_to_djangomodel` introduced in
graphql-python/graphene-django#1315 for the
reasons discussed here
https://github.com/graphql-python/graphene-django/pull/1315/files#r1015659857.
As mentioned there, without reverting this code, "queries are forced
every time an object is resolved, making an exponential number of
queries when nesting without any possibility of optimizing".

That regression prevented `graphene-django-optimizer` from working with
`graphene-django` v3.0.0b9+ (where this change first was published), as
discussed in
graphql-python/graphene-django#1356 (comment),
tfoxy/graphene-django-optimizer#86, and
tfoxy/graphene-django-optimizer#83 (comment).

For now, this marks the two tests that depended on this problematic code
as "expected to fail", and perhaps they can be reintroduced if there's a
way to support this logic in a way that does not prevent
`select_related` and `prefetch_related` query-optimization and introduce
nested N+1s.

As mentioned here
graphql-python/graphene-django#1315 (comment),
this is blocking upgrade to graphene-django v3 for many users, and
fixing this would allow many to begin upgrading and contributing to keep
graphene-django going.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants