Skip to content

Commit f67c5db

Browse files
sjdemartinifiraskafri
authored andcommitted
Revert field resolver logic to fix poor query performance
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.
1 parent 34cc860 commit f67c5db

File tree

2 files changed

+9
-20
lines changed

2 files changed

+9
-20
lines changed

graphene_django/converter.py

Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -315,26 +315,7 @@ def dynamic_type():
315315
if not _type:
316316
return
317317

318-
class CustomField(Field):
319-
def wrap_resolve(self, parent_resolver):
320-
"""
321-
Implements a custom resolver which go through the `get_node` method to ensure that
322-
it goes through the `get_queryset` method of the DjangoObjectType.
323-
"""
324-
resolver = super().wrap_resolve(parent_resolver)
325-
326-
def custom_resolver(root, info, **args):
327-
fk_obj = resolver(root, info, **args)
328-
if not isinstance(fk_obj, model):
329-
# In case the resolver is a custom one that overwrites
330-
# the default Django resolver
331-
# This happens, for example, when using custom awaitable resolvers.
332-
return fk_obj
333-
return _type.get_node(info, fk_obj.pk)
334-
335-
return custom_resolver
336-
337-
return CustomField(
318+
return Field(
338319
_type,
339320
description=get_django_field_description(field),
340321
required=not field.null,

graphene_django/tests/test_get_queryset.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,10 @@ def test_get_queryset_called_on_field(self):
121121
assert not result.errors
122122
assert result.data == {"reporter": {"firstName": "Jane"}}
123123

124+
# TODO: This test is currently expected to fail because the logic it depended on has been
125+
# removed, due to poor SQL performance and preventing query-optimization (see
126+
# https://github.com/graphql-python/graphene-django/pull/1315/files#r1015659857)
127+
@pytest.mark.xfail
124128
def test_get_queryset_called_on_foreignkey(self):
125129
# If a user tries to access a reporter through an article they should get our authorization error
126130
query = """
@@ -291,6 +295,10 @@ def test_get_queryset_called_on_node(self):
291295
assert not result.errors
292296
assert result.data == {"reporter": {"firstName": "Jane"}}
293297

298+
# TODO: This test is currently expected to fail because the logic it depended on has been
299+
# removed, due to poor SQL performance and preventing query-optimization (see
300+
# https://github.com/graphql-python/graphene-django/pull/1315/files#r1015659857)
301+
@pytest.mark.xfail
294302
def test_get_queryset_called_on_foreignkey(self):
295303
# If a user tries to access a reporter through an article they should get our authorization error
296304
query = """

0 commit comments

Comments
 (0)