Skip to content
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

Revert field resolver logic to fix poor query performance #1401

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 all 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
153 changes: 150 additions & 3 deletions graphene_django/tests/test_fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
import datetime
from django.db.models import Count
import re
from django.db.models import Count, Prefetch

import pytest

from graphene import List, NonNull, ObjectType, Schema, String

from ..fields import DjangoListField
from ..types import DjangoObjectType
from .models import Article as ArticleModel
from .models import Reporter as ReporterModel
from .models import (
Article as ArticleModel,
Film as FilmModel,
FilmDetails as FilmDetailsModel,
Reporter as ReporterModel,
)


class TestDjangoListField:
Expand Down Expand Up @@ -500,3 +505,145 @@ class Query(ObjectType):

assert not result.errors
assert result.data == {"reporters": [{"firstName": "Tara"}]}

def test_select_related_and_prefetch_related_are_respected(
self, django_assert_num_queries
):
class Article(DjangoObjectType):
class Meta:
model = ArticleModel
fields = ("headline", "editor", "reporter")

class Film(DjangoObjectType):
class Meta:
model = FilmModel
fields = ("genre", "details")

class FilmDetail(DjangoObjectType):
class Meta:
model = FilmDetailsModel
fields = ("location",)

class Reporter(DjangoObjectType):
class Meta:
model = ReporterModel
fields = ("first_name", "articles", "films")

class Query(ObjectType):
articles = DjangoListField(Article)

@staticmethod
def resolve_articles(root, info):
# Optimize for querying associated editors and reporters, and the films and film
# details of those reporters. This is similar to what would happen using a library
# like https://github.com/tfoxy/graphene-django-optimizer for a query like the one
# below (albeit simplified and hardcoded here).
return ArticleModel.objects.select_related(
"editor", "reporter"
).prefetch_related(
Prefetch(
"reporter__films",
queryset=FilmModel.objects.select_related("details"),
),
)

schema = Schema(query=Query)

query = """
query {
articles {
headline

editor {
firstName
}

reporter {
firstName

films {
genre

details {
location
}
}
}
}
}
"""

r1 = ReporterModel.objects.create(first_name="Tara", last_name="West")
r2 = ReporterModel.objects.create(first_name="Debra", last_name="Payne")

ArticleModel.objects.create(
headline="Amazing news",
reporter=r1,
pub_date=datetime.date.today(),
pub_date_time=datetime.datetime.now(),
editor=r2,
)
ArticleModel.objects.create(
headline="Not so good news",
reporter=r2,
pub_date=datetime.date.today(),
pub_date_time=datetime.datetime.now(),
editor=r1,
)

film1 = FilmModel.objects.create(genre="ac")
film2 = FilmModel.objects.create(genre="ot")
film3 = FilmModel.objects.create(genre="do")
FilmDetailsModel.objects.create(location="Hollywood", film=film1)
FilmDetailsModel.objects.create(location="Antarctica", film=film3)
r1.films.add(film1, film2)
r2.films.add(film3)

# We expect 2 queries to be performed based on the above resolver definition: one for all
# articles joined with the reporters model (for associated editors and reporters), and one
# for the films prefetch (which includes its `select_related` JOIN logic in its queryset)
with django_assert_num_queries(2) as captured:
result = schema.execute(query)

assert not result.errors
assert result.data == {
"articles": [
{
"headline": "Amazing news",
"editor": {"firstName": "Debra"},
"reporter": {
"firstName": "Tara",
"films": [
{"genre": "AC", "details": {"location": "Hollywood"}},
{"genre": "OT", "details": None},
],
},
},
{
"headline": "Not so good news",
"editor": {"firstName": "Tara"},
"reporter": {
"firstName": "Debra",
"films": [
{"genre": "DO", "details": {"location": "Antarctica"}},
],
},
},
]
}

assert len(captured.captured_queries) == 2 # Sanity-check

# First we should have queried for all articles in a single query, joining on the reporters
# model (for the editors and reporters ForeignKeys)
assert re.match(
r'SELECT .* "tests_article" INNER JOIN "tests_reporter"',
captured.captured_queries[0]["sql"],
)

# Then we should have queried for all of the films of all reporters, joined with the film
# details for each film, using a single query
assert re.match(
r'SELECT .* FROM "tests_film" INNER JOIN "tests_film_reporters" .* LEFT OUTER JOIN "tests_filmdetails"',
captured.captured_queries[1]["sql"],
)
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"}}]},
}