graphql-python/graphene-django
1
1
Fork 0
mirror of https://github.com/graphql-python/graphene-django.git synced 2024-11-22 09:37:07 +03:00
Code Issues Packages Projects Releases Wiki Activity

Revert field resolver logic to fix poor query performance

Browse Source 
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.
This commit is contained in:
Steven DeMartini 2023-04-29 12:01:55 -07:00 committed by Firas K
parent 34cc86063b
commit f67c5dbc8c
2 changed files with 9 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
graphene_django/converter.py
View File

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

8
graphene_django/tests/test_get_queryset.py
View File

@ -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 = """