Skip to content

Commit

Permalink
Revert field resolver logic to fix poor query performance
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
DevStar1016 committed May 3, 2023
1 parent e9486e3 commit cf05d96
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 20 deletions.
21 changes: 1 addition & 20 deletions graphene_django/converter.py
Original file line number Diff line number Diff line change
Expand Up @@ -315,26 +315,7 @@ def dynamic_type():
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,
Expand Down
8 changes: 8 additions & 0 deletions graphene_django/tests/test_get_queryset.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ def test_get_queryset_called_on_field(self):
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 = """
Expand Down Expand Up @@ -291,6 +295,10 @@ def test_get_queryset_called_on_node(self):
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 = """
Expand Down

0 comments on commit cf05d96

Please sign in to comment.