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.

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

DjangoObjectType.fields ignores select_related #1375

Closed
lufte opened this issue Nov 25, 2022 · 6 comments
Closed

DjangoObjectType.fields ignores select_related #1375

lufte opened this issue Nov 25, 2022 · 6 comments
Labels

Comments

@lufte
Copy link

lufte commented Nov 25, 2022

  • What is the current behavior?

The behavior for resolving a ForeignKey field of a DjangoObjectType is not the same when adding the field to fields versus declaring the field explicitly as an attribute of the class. In the second case, an extra query won't be made if the field has been cached by a previous select_related, but the first case always executes an additional query ignoring the select_related.

Create two Models with a foreign key between them, and a schema in which you can list all the parent models with their respective children, using a select_related call in the resolver. In the parent DjangoObjectType, try declaring the child model using both fields and an explicit attribute like child = graphene.NonNull('schema.ChildType'), and count the queries using both strategies.

  • What is the expected behavior?

The select_related field should be honored in both cases.

  • Please tell us about your environment:

    • Version: 3.0.0
    • Platform: Python 3.10.8, graphene==3.1.1
@lufte lufte added the 🐛bug label Nov 25, 2022
@grantmcconnaughey
Copy link

I can confirm this is also happening for me using graphene-django 3.0.0 and graphene 3.2.1.

Before the upgrade select_related was used, but now there are N+1 errors all throughout my project. I can also confirm that adding an explicit attribute does fix the issue.

@lufte Were you ever able to find a more global fix? I can certainly explicitly add attributes to all of my types but it would be simpler to fix this in one spot.

@lufte
Copy link
Author

lufte commented Mar 14, 2023

@grantmcconnaughey unfortunately no, I just added the fields explicitly and forgot all about it.

@firaskafri
Copy link
Collaborator

@lufte we have reverted a possible cause #1315, can you confirm if the issue still exists in 3.0.1?

@lufte
Copy link
Author

lufte commented May 5, 2023

Hi @firaskafri. I ran a quick test using my current project (I thought it would be faster than creating a minimal reproducible example, turned out that it isn't) but I still see the behavior. If you need a demo to properly reproduce the issue let me know and I can write one.

@sjdemartini
Copy link
Collaborator

@lufte looks like there was a typo in @firaskafri's comment above. Can you try again on 3.0.2, the latest release? (That's where the select_related/prefetch_related problems were fixed.) Judging by the original issue description, this scenario should be working fine now, and is even unit-tested:

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"],
)

@lufte
Copy link
Author

lufte commented May 5, 2023

@sjdemartini Yes, that was it! I've re-tested with 3.0.2 and now I see no difference between using fields = or declaring the field explicitly. Thanks guys, great work!

@lufte lufte closed this as completed May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants