Skip to content

Revert field resolver logic to fix poor query performance #1401

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 3, 2023
Merged

Revert field resolver logic to fix poor query performance #1401

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae189e7
Revert field resolver logic to fix poor query performance
sjdemartini Apr 29, 2023
aebc4a3
Remove obsolete tests and add note about rationale
sjdemartini May 1, 2023
5e8952d
Add test validating query performance with select_related + prefetch_…
sjdemartini May 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 1 addition & 20 deletions graphene_django/converter.py
View file
Open in desktop
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
138 changes: 6 additions & 132 deletions graphene_django/tests/test_get_queryset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ class TestShouldCallGetQuerySetOnForeignKey:
Check that the get_queryset method is called in both forward and reversed direction
of a foreignkey on types.
(see issue #1111)

NOTE: For now, we do not expect this get_queryset method to be called for nested
objects, as the original attempt to do so prevented SQL query-optimization with
`select_related`/`prefetch_related` and caused N+1 queries. See discussions here
https://github.com/graphql-python/graphene-django/pull/1315/files#r1015659857
and here https://github.com/graphql-python/graphene-django/pull/1401.
Comment on lines +20 to +24
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

"""

@pytest.fixture(autouse=True)
Expand Down Expand Up @@ -121,69 +127,6 @@ def test_get_queryset_called_on_field(self):
assert not result.errors
assert result.data == {"reporter": {"firstName": "Jane"}}

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 = """
query getArticle($id: ID!) {
article(id: $id) {
headline
reporter {
firstName
}
}
}
"""

result = self.schema.execute(query, variables={"id": self.articles[0].id})
assert len(result.errors) == 1
assert result.errors[0].message == "Not authorized to access reporters."

# An admin user should be able to get reporters through an article
query = """
query getArticle($id: ID!) {
article(id: $id) {
headline
reporter {
firstName
}
}
}
"""

result = self.schema.execute(
query,
variables={"id": self.articles[0].id},
context_value={"admin": True},
)
assert not result.errors
assert result.data["article"] == {
"headline": "A fantastic article",
"reporter": {"firstName": "Jane"},
}

# An admin user should not be able to access draft article through a reporter
query = """
query getReporter($id: ID!) {
reporter(id: $id) {
firstName
articles {
headline
}
}
}
"""

result = self.schema.execute(
query,
variables={"id": self.reporter.id},
context_value={"admin": True},
)
assert not result.errors
assert result.data["reporter"] == {
"firstName": "Jane",
"articles": [{"headline": "A fantastic article"}],
}


class TestShouldCallGetQuerySetOnForeignKeyNode:
"""
Expand Down Expand Up @@ -290,72 +233,3 @@ def test_get_queryset_called_on_node(self):
)
assert not result.errors
assert result.data == {"reporter": {"firstName": "Jane"}}

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 = """
query getArticle($id: ID!) {
article(id: $id) {
headline
reporter {
firstName
}
}
}
"""

result = self.schema.execute(
query, variables={"id": to_global_id("ArticleType", self.articles[0].id)}
)
assert len(result.errors) == 1
assert result.errors[0].message == "Not authorized to access reporters."

# An admin user should be able to get reporters through an article
query = """
query getArticle($id: ID!) {
article(id: $id) {
headline
reporter {
firstName
}
}
}
"""

result = self.schema.execute(
query,
variables={"id": to_global_id("ArticleType", self.articles[0].id)},
context_value={"admin": True},
)
assert not result.errors
assert result.data["article"] == {
"headline": "A fantastic article",
"reporter": {"firstName": "Jane"},
}

# An admin user should not be able to access draft article through a reporter
query = """
query getReporter($id: ID!) {
reporter(id: $id) {
firstName
articles {
edges {
node {
headline
}
}
}
}
}
"""

result = self.schema.execute(
query,
variables={"id": to_global_id("ReporterType", self.reporter.id)},
context_value={"admin": True},
)
assert not result.errors
assert result.data["reporter"] == {
"firstName": "Jane",
"articles": {"edges": [{"node": {"headline": "A fantastic article"}}]},
}