From ae189e760e76c815bbe1ab1cd3e9d6c92bf03db4 Mon Sep 17 00:00:00 2001
From: Steven DeMartini <sjdemartini@users.noreply.github.com>
Date: Sat, 29 Apr 2023 12:01:55 -0700
Subject: [PATCH] Revert field resolver logic to fix poor query performance

This reverts the change to `convert_field_to_djangomodel` introduced in
https://github.com/graphql-python/graphene-django/pull/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
https://github.com/graphql-python/graphene-django/issues/1356#issuecomment-1284718187,
https://github.com/tfoxy/graphene-django-optimizer/issues/86, and
https://github.com/tfoxy/graphene-django-optimizer/pull/83#issuecomment-1451987397.

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
https://github.com/graphql-python/graphene-django/pull/1315#issuecomment-1468594361,
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.
---
 graphene_django/converter.py               | 21 +--------------------
 graphene_django/tests/test_get_queryset.py |  8 ++++++++
 2 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/graphene_django/converter.py b/graphene_django/converter.py
index 386103a..9ad6c9d 100644
--- a/graphene_django/converter.py
+++ b/graphene_django/converter.py
@@ -315,26 +315,7 @@ def convert_field_to_djangomodel(field, registry=None):
         if not _type:
             return
 
-        class CustomField(Field):
-            def wrap_resolve(self, parent_resolver):
-                """
-                Implements a custom resolver which go through the `get_node` method to ensure that
-                it goes through the `get_queryset` method of the DjangoObjectType.
-                """
-                resolver = super().wrap_resolve(parent_resolver)
-
-                def custom_resolver(root, info, **args):
-                    fk_obj = resolver(root, info, **args)
-                    if not isinstance(fk_obj, model):
-                        # In case the resolver is a custom one that overwrites
-                        # the default Django resolver
-                        # This happens, for example, when using custom awaitable resolvers.
-                        return fk_obj
-                    return _type.get_node(info, fk_obj.pk)
-
-                return custom_resolver
-
-        return CustomField(
+        return Field(
             _type,
             description=get_django_field_description(field),
             required=not field.null,
diff --git a/graphene_django/tests/test_get_queryset.py b/graphene_django/tests/test_get_queryset.py
index 91bdc70..63027b9 100644
--- a/graphene_django/tests/test_get_queryset.py
+++ b/graphene_django/tests/test_get_queryset.py
@@ -121,6 +121,10 @@ class TestShouldCallGetQuerySetOnForeignKey:
         assert not result.errors
         assert result.data == {"reporter": {"firstName": "Jane"}}
 
+    # TODO: This test is currently expected to fail because the logic it depended on has been
+    # removed, due to poor SQL performance and preventing query-optimization (see
+    # https://github.com/graphql-python/graphene-django/pull/1315/files#r1015659857)
+    @pytest.mark.xfail
     def test_get_queryset_called_on_foreignkey(self):
         # If a user tries to access a reporter through an article they should get our authorization error
         query = """
@@ -291,6 +295,10 @@ class TestShouldCallGetQuerySetOnForeignKeyNode:
         assert not result.errors
         assert result.data == {"reporter": {"firstName": "Jane"}}
 
+    # TODO: This test is currently expected to fail because the logic it depended on has been
+    # removed, due to poor SQL performance and preventing query-optimization (see
+    # https://github.com/graphql-python/graphene-django/pull/1315/files#r1015659857)
+    @pytest.mark.xfail
     def test_get_queryset_called_on_foreignkey(self):
         # If a user tries to access a reporter through an article they should get our authorization error
         query = """