fix: unit test for graphene pr#1412 by tcleonard · Pull Request #1315 · graphql-python/graphene-django (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation8 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Thomas Leonard and others added 2 commits
…fix_foreign_key_field
Issue graphql-python#1111: foreign key should also call get_queryset method
firaskafri added a commit that referenced this pull request
| resolver = super().wrap_resolve(parent_resolver) |
|---|
| def custom_resolver(root, info, **args): |
| fk_obj = resolver(root, info, **args) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You run the resolver, get a result (django model) back and then run _type.get_node to fetch it again making a 2nd unnecessary query?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an issue I've also been getting, queries are forced every time an object is resolved, making an exponential number of queries when nesting without any possibility of optimizing
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fairly new to this code base and I am trying to resolve the issue raised by @ekampf
In my attempt to fix this:
- I started by getting the id of the field to be resolved (which is the foreign-key object) from the root in an attempt to stop calling one of the functions that resulting in a hit to the database.
- Then I check the resolver
- No Custom resolver
* if theresolverequals to the default resolver (meaning that there is no custom resolver for the field in the root node), callget_nodeand return.
* At this point and this scenario, the issue is solved, because there is one resolver that was called resulting in one hit to the database and thats fine! - Custom Resolver Exists
* At this point, there is a custom resolver, which means that the node should go through multiple resolvers: theresolvermethod +get_node(I say should because the tests and the code written to make sure that the processes of resolving the node always goes through theget_node)
* ** In this case, I did not include when the resolver is awaitable
- No Custom resolver
- But the wall I faced after that is the fact that if is required to call
get_nodeeven though the node is resolved as a result of calling theresolverfunction. Which stopped me for a second, why there are two functions responsible for resolving the
@tcleonard any thoughts how this can be fixed?
I think this is not the right way to assure that get_node is always called. The solution should be more concrete.
I was thinking of a way to pass the instance returned from get_node to the resolver function if node exists. Because there is no reason to get the instance 2 times. But I am not sure how this is can be done.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing out the issues, and for attempting to fix! In the meantime until a solution can be worked out that doesn't cause N+1 queries and doesn't prevent query-optimization (assuming the original PR author or someone else do care to support the original intention of this PR), I've opened a PR to revert the problematic code here #1401, since this caused a significant regression relative to graphene-django v3.0.0b7 is a blocker for many people looking to upgrade to graphene-django v3.
@firaskafri Would it be possible to revert this PR? It seems to have caused unfixable N+1 issues, even when using select_related, for foreign key fields. It'll be a blocker for using graphene-django 3 for a lot of people, and it's not obvious this is happening unless you use something like django-silk to uncover the huge increase in queries.
sjdemartini added a commit to sjdemartini/graphene-django that referenced this pull request
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.
This was referenced
Apr 29, 2023
sjdemartini added a commit to sjdemartini/graphene-django that referenced this pull request
…related
This test passes after reverting the CustomField resolver change
introduced in
graphql-python#1315, but fails
with that resolver code present. For instance, adding back the resolver
code gives a test failure showing:
Failed: Expected to perform 2 queries but 11 were doneThis should ensure there aren't regressions that prevent query-optimization in the future.
sjdemartini added a commit to sjdemartini/graphene-django that referenced this pull request
…related
This test passes after reverting the CustomField resolver change
introduced in
graphql-python#1315, but fails
with that resolver code present. For instance, adding back the resolver
code gives a test failure showing:
Failed: Expected to perform 2 queries but 11 were doneThis should ensure there aren't regressions that prevent query-optimization in the future.
sjdemartini added a commit to sjdemartini/graphene-django that referenced this pull request
…related
This test passes after reverting the CustomField resolver change
introduced in
graphql-python#1315, but fails
with that resolver code present. For instance, adding back the resolver
code gives a test failure showing:
Failed: Expected to perform 2 queries but 11 were doneThis should ensure there aren't regressions that prevent query-optimization in the future.
firaskafri pushed a commit that referenced this pull request
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 pushed a commit that referenced this pull request
…related
This test passes after reverting the CustomField resolver change
introduced in
#1315, but fails
with that resolver code present. For instance, adding back the resolver
code gives a test failure showing:
Failed: Expected to perform 2 queries but 11 were doneThis should ensure there aren't regressions that prevent query-optimization in the future.
@grantmcconnaughey this was reverted and released in 3.0.2.
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.0.2 for now. Feel free to suggest a different way that won't lead to those pesky N+1 SQL query issues.
igodev0001 pushed a commit to igodev0001/graphene-django that referenced this pull request
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.
igodev0001 pushed a commit to igodev0001/graphene-django that referenced this pull request
…related
This test passes after reverting the CustomField resolver change
introduced in
graphql-python/graphene-django#1315, but fails
with that resolver code present. For instance, adding back the resolver
code gives a test failure showing:
Failed: Expected to perform 2 queries but 11 were doneThis should ensure there aren't regressions that prevent query-optimization in the future.
superlevure pushed a commit to loft-orbital/graphene-django that referenced this pull request
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.
superlevure pushed a commit to loft-orbital/graphene-django that referenced this pull request
…related
This test passes after reverting the CustomField resolver change
introduced in
graphql-python#1315, but fails
with that resolver code present. For instance, adding back the resolver
code gives a test failure showing:
Failed: Expected to perform 2 queries but 11 were doneThis should ensure there aren't regressions that prevent query-optimization in the future.
DevStar1016 added a commit to DevStar1016/graphine-django that referenced this pull request
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.
DevStar1016 added a commit to DevStar1016/graphine-django that referenced this pull request
…related
This test passes after reverting the CustomField resolver change
introduced in
graphql-python/graphene-django#1315, but fails
with that resolver code present. For instance, adding back the resolver
code gives a test failure showing:
Failed: Expected to perform 2 queries but 11 were doneThis should ensure there aren't regressions that prevent query-optimization in the future.
Yaroslav-Dev007 added a commit to Yaroslav-Dev007/graphene-django that referenced this pull request
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 pull request
…related
This test passes after reverting the CustomField resolver change
introduced in
graphql-python/graphene-django#1315, but fails
with that resolver code present. For instance, adding back the resolver
code gives a test failure showing:
Failed: Expected to perform 2 queries but 11 were doneThis should ensure there aren't regressions that prevent query-optimization in the future.
BohdanS000 added a commit to BohdanS000/python-django-graphql that referenced this pull request
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.
BohdanS000 added a commit to BohdanS000/python-django-graphql that referenced this pull request
…related
This test passes after reverting the CustomField resolver change
introduced in
graphql-python/graphene-django#1315, but fails
with that resolver code present. For instance, adding back the resolver
code gives a test failure showing:
Failed: Expected to perform 2 queries but 11 were doneThis should ensure there aren't regressions that prevent query-optimization in the future.
light0725 added a commit to light0725/graphene-django that referenced this pull request
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.
light0725 added a commit to light0725/graphene-django that referenced this pull request
…related
This test passes after reverting the CustomField resolver change
introduced in
graphql-python/graphene-django#1315, but fails
with that resolver code present. For instance, adding back the resolver
code gives a test failure showing:
Failed: Expected to perform 2 queries but 11 were doneThis should ensure there aren't regressions that prevent query-optimization in the future.