Skip to content

Commit

Permalink
Fix AttributeError when using optimizer and prefetch_related (#533)
Browse files Browse the repository at this point in the history
* Add a test capturing the failure

* Short-circuit when a queryset has been sliced

There are several places in the codebase where qs._result_cache is evaluated.

If a QuerySet has been sliced (e.g., prefetched with pagination in DjangoOptimizerExtension), it has been evaluated as a list and will not have the `_result_cache` attrib.

* Update test to verify short-circuiting works as expected
  • Loading branch information
jacobwegner committed May 26, 2024
1 parent 971231b commit bebf21a
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 0 deletions.
4 changes: 4 additions & 0 deletions strawberry_django/optimizer.py
Original file line number Diff line number Diff line change
Expand Up @@ -694,6 +694,10 @@ def optimize(
if isinstance(qs, BaseManager):
qs = cast(QuerySet[_M], qs.all())

if isinstance(qs, list):
# return sliced queryset as-is
return qs

# Avoid optimizing twice and also modify an already resolved queryset
if (
get_queryset_config(qs).optimized or qs._result_cache is not None # type: ignore
Expand Down
4 changes: 4 additions & 0 deletions strawberry_django/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,10 @@ def filter_with_perms(qs: QuerySet[_M], info: Info) -> QuerySet[_M]:
if not context.checkers or context.is_safe:
return qs

if isinstance(qs, list):
# return sliced queryset as-is
return qs

# Do not do anything is results are cached
if qs._result_cache is not None: # type: ignore
set_perm_safe(False)
Expand Down
4 changes: 4 additions & 0 deletions strawberry_django/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@


def default_qs_hook(qs: models.QuerySet[_M]) -> models.QuerySet[_M]:
if isinstance(qs, list):
# return sliced queryset as-is
return qs

# FIXME: We probably won't need this anymore when we can use graphql-core 3.3.0+
# as its `complete_list_value` gives a preference to async iteration it if is
# provided by the object.
Expand Down
79 changes: 79 additions & 0 deletions tests/test_optimizer.py
Original file line number Diff line number Diff line change
Expand Up @@ -1025,3 +1025,82 @@ def test_query_nested_connection_with_filter(db, gql_client: GraphQLTestClient):
assert {
edge["node"]["id"] for edge in result["issuesWithFilters"]["edges"]
} == expected


@pytest.mark.django_db(transaction=True)
def test_query_with_optimizer_paginated_prefetch():
@strawberry_django.type(Milestone, pagination=True)
class MilestoneTypeWithNestedPrefetch:
@strawberry_django.field()
def name(self, info) -> str:
return self.name

@strawberry_django.type(
Project,
)
class ProjectTypeWithPrefetch:
@strawberry_django.field()
def name(self, info) -> str:
return self.name

milestones: List[MilestoneTypeWithNestedPrefetch]

milestone1 = MilestoneFactory.create()
project = milestone1.project
MilestoneFactory.create(project=project)

@strawberry.type
class Query:
projects: List[ProjectTypeWithPrefetch] = strawberry_django.field()

query1 = utils.generate_query(Query, enable_optimizer=False)
query_str = """
query TestQuery {
projects {
name
milestones (pagination: {limit: 1}) {
name
}
}
}
"""

# NOTE: The following assertion doesn't work because the
# DjangoOptimizerExtension instance is not the one within the
# generate_query wrapper
"""
assert DjangoOptimizerExtension.enabled.get()
"""
result1 = query1(query_str)

assert isinstance(result1, ExecutionResult)
assert not result1.errors
assert result1.data == {
"projects": [
{
"name": project.name,
"milestones": [
{
"name": milestone1.name,
},
],
},
],
}

query2 = utils.generate_query(Query, enable_optimizer=True)
result2 = query2(query_str)

assert isinstance(result2, ExecutionResult)
assert result2.data == {
"projects": [
{
"name": project.name,
"milestones": [
{
"name": milestone1.name,
},
],
},
],
}

0 comments on commit bebf21a

Please sign in to comment.