fix: unit test for graphene pr#1412 #1315

tcleonard · 2022-03-20T21:49:43Z

once graphql-python/graphene#1412 is merged

…method

…fix_foreign_key_field Issue graphql-python#1111: foreign key should also call get_queryset method

firaskafri · 2022-09-22T17:20:39Z

graphql-python/graphene#1412 is merged

This reverts commit 5d81ba0.

ekampf · 2022-11-07T16:50:34Z

graphene_django/converter.py

+                resolver = super().wrap_resolve(parent_resolver)
+
+                def custom_resolver(root, info, **args):
+                    fk_obj = resolver(root, info, **args)


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?

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

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 the resolver equals to the default resolver (meaning that there is no custom resolver for the field in the root node), call get_node and 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: the resolver method + 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 the get_node)

** In this case, I did not include when the resolver is awaitable

But the wall I faced after that is the fact that if is required to call get_node even though the node is resolved as a result of calling the resolver function. 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.

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.

grantmcconnaughey · 2023-03-14T18:10:41Z

@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.

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.

…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 done ``` This should ensure there aren't regressions that prevent query-optimization in the future.

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.

…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 done ``` This should ensure there aren't regressions that prevent query-optimization in the future.

firaskafri · 2023-05-03T09:05:22Z

@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.

grantmcconnaughey · 2023-05-03T16:49:09Z

@firaskafri Thank you! 🎉

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.

…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 done ``` This should ensure there aren't regressions that prevent query-optimization in the future.

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.

…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 done ``` This should ensure there aren't regressions that prevent query-optimization in the future.

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.

…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 done ``` This should ensure there aren't regressions that prevent query-optimization in the future.

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.

…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 done ``` This should ensure there aren't regressions that prevent query-optimization in the future.

Thomas Leonard and others added 2 commits March 20, 2022 21:44

Issue graphql-python#1111: foreign key should also call get_queryset …

9d564a0

…method

Merge pull request #12 from loft-orbital/users/tcleonard/issue-#1111-…

c4aa139

…fix_foreign_key_field Issue graphql-python#1111: foreign key should also call get_queryset method

tcleonard force-pushed the users/tcleonard/fix-test-for-graphene-PR#1412 branch 3 times, most recently from 9b3a117 to 9b0af18 Compare March 20, 2022 22:06

fix: test for graphene PR graphql-python/graphene#1412

ca55529

tcleonard force-pushed the users/tcleonard/fix-test-for-graphene-PR#1412 branch from 9b0af18 to ca55529 Compare March 20, 2022 22:15

firaskafri self-requested a review September 22, 2022 17:20

firaskafri approved these changes Sep 22, 2022

View reviewed changes

firaskafri merged commit 5d81ba0 into graphql-python:main Sep 23, 2022

firaskafri added a commit that referenced this pull request Sep 23, 2022

Revert "fix: unit test for graphene pr#1412 (#1315)"

a898df5

This reverts commit 5d81ba0.

firaskafri mentioned this pull request Sep 23, 2022

Revert "fix: unit test for graphene pr#1412" #1350

Closed

syrusakbary mentioned this pull request Oct 12, 2022

Fix custom foreignkey resolvers #1361

Merged

ekampf reviewed Nov 7, 2022

View reviewed changes

mahdyhamad mentioned this pull request Mar 7, 2023

fix(converter.py): performance fix in `convert_field_to_djangomodel.dynamic_type.CustomField #1391

Closed

This was referenced Apr 29, 2023

Revert field resolver logic to fix poor query performance #1401

Merged

Check dependent libraries: https://github.com/tfoxy/graphene-django-optimizer and https://github.com/eamigo86/graphene-django-extras #1356

Closed

qeternity mentioned this pull request Apr 30, 2023

Revert field resolver logic to fix poor query performance zumalabs/graphene-django#1

Merged

firaskafri mentioned this pull request May 3, 2023

Bump version #1405

Merged

firaskafri mentioned this pull request May 5, 2023

DjangoObjectType.fields ignores select_related #1375

Closed

superlevure mentioned this pull request May 22, 2023

fix: fk resolver permissions leak #1411

Merged

1 task

superlevure deleted the users/tcleonard/fix-test-for-graphene-PR#1412 branch August 10, 2023 21:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: unit test for graphene pr#1412 #1315

fix: unit test for graphene pr#1412 #1315

tcleonard commented Mar 20, 2022

firaskafri commented Sep 22, 2022

ekampf Nov 7, 2022

Arno500 Feb 14, 2023

mahdyhamad Mar 5, 2023 •

edited

sjdemartini Apr 29, 2023 •

edited

grantmcconnaughey commented Mar 14, 2023

firaskafri commented May 3, 2023 •

edited

grantmcconnaughey commented May 3, 2023

fix: unit test for graphene pr#1412 #1315

fix: unit test for graphene pr#1412 #1315

Conversation

tcleonard commented Mar 20, 2022

firaskafri commented Sep 22, 2022

ekampf Nov 7, 2022

Choose a reason for hiding this comment

Arno500 Feb 14, 2023

Choose a reason for hiding this comment

mahdyhamad Mar 5, 2023 • edited

Choose a reason for hiding this comment

sjdemartini Apr 29, 2023 • edited

Choose a reason for hiding this comment

grantmcconnaughey commented Mar 14, 2023

firaskafri commented May 3, 2023 • edited

grantmcconnaughey commented May 3, 2023

mahdyhamad Mar 5, 2023 •

edited

sjdemartini Apr 29, 2023 •

edited

firaskafri commented May 3, 2023 •

edited