Skip to content

replace merge_queryset with resolve_queryset pattern #796

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

Merged
merged 8 commits into from
Nov 28, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions docs/queries.rst
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,13 @@ of Django's ``HTTPRequest`` in your resolve methods, such as checking for authen
return Question.objects.none()


DjangoObjectTypes
~~~~~~~~~~~~~~~~~

A Resolver that maps to a defined `DjangoObjectType` should only use methods that return a queryset.
Queryset methods like `values` will return dictionaries, use `defer` instead.


Plain ObjectTypes
-----------------

Expand Down
35 changes: 17 additions & 18 deletions graphene_django/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ def list_resolver(django_object_type, resolver, root, info, **args):
if queryset is None:
# Default to Django Model queryset
# N.B. This happens if DjangoListField is used in the top level Query object
model = django_object_type._meta.model
model_manager = django_object_type._meta.model.objects
queryset = maybe_queryset(
django_object_type.get_queryset(model.objects, info)
django_object_type.get_queryset(model_manager, info)
)
return queryset

Expand Down Expand Up @@ -108,25 +108,13 @@ def get_manager(self):

@classmethod
def resolve_queryset(cls, connection, queryset, info, args):
# queryset is the resolved iterable from ObjectType
return connection._meta.node.get_queryset(queryset, info)

@classmethod
def merge_querysets(cls, default_queryset, queryset):
if default_queryset.query.distinct and not queryset.query.distinct:
queryset = queryset.distinct()
elif queryset.query.distinct and not default_queryset.query.distinct:
default_queryset = default_queryset.distinct()
return queryset & default_queryset

@classmethod
def resolve_connection(cls, connection, default_manager, args, iterable):
if iterable is None:
iterable = default_manager
def resolve_connection(cls, connection, args, iterable):
iterable = maybe_queryset(iterable)
if isinstance(iterable, QuerySet):
if iterable.model.objects is not default_manager:
default_queryset = maybe_queryset(default_manager)
iterable = cls.merge_querysets(default_queryset, iterable)
_len = iterable.count()
else:
_len = len(iterable)
Expand All @@ -150,6 +138,7 @@ def connection_resolver(
resolver,
connection,
default_manager,
queryset_resolver,
max_limit,
enforce_first_or_last,
root,
Expand Down Expand Up @@ -177,9 +166,15 @@ def connection_resolver(
).format(last, info.field_name, max_limit)
args["last"] = min(last, max_limit)

# eventually leads to DjangoObjectType's get_queryset (accepts queryset)
# or a resolve_foo (does not accept queryset)
iterable = resolver(root, info, **args)
queryset = cls.resolve_queryset(connection, default_manager, info, args)
on_resolve = partial(cls.resolve_connection, connection, queryset, args)
if iterable is None:
iterable = default_manager
# thus the iterable gets refiltered by resolve_queryset
# but iterable might be promise
iterable = queryset_resolver(connection, iterable, info, args)
on_resolve = partial(cls.resolve_connection, connection, args)

if Promise.is_thenable(iterable):
return Promise.resolve(iterable).then(on_resolve)
Expand All @@ -192,6 +187,10 @@ def get_resolver(self, parent_resolver):
parent_resolver,
self.connection_type,
self.get_manager(),
self.get_queryset_resolver(),
self.max_limit,
self.enforce_first_or_last,
)

def get_queryset_resolver(self):
return self.resolve_queryset
68 changes: 8 additions & 60 deletions graphene_django/filter/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,69 +52,17 @@ def filtering_args(self):
return get_filtering_args_from_filterset(self.filterset_class, self.node_type)

@classmethod
def merge_querysets(cls, default_queryset, queryset):
# There could be the case where the default queryset (returned from the filterclass)
# and the resolver queryset have some limits on it.
# We only would be able to apply one of those, but not both
# at the same time.

# See related PR: https://github.com/graphql-python/graphene-django/pull/126

assert not (
default_queryset.query.low_mark and queryset.query.low_mark
), "Received two sliced querysets (low mark) in the connection, please slice only in one."
assert not (
default_queryset.query.high_mark and queryset.query.high_mark
), "Received two sliced querysets (high mark) in the connection, please slice only in one."
low = default_queryset.query.low_mark or queryset.query.low_mark
high = default_queryset.query.high_mark or queryset.query.high_mark
default_queryset.query.clear_limits()
queryset = super(DjangoFilterConnectionField, cls).merge_querysets(
default_queryset, queryset
)
queryset.query.set_limits(low, high)
return queryset

@classmethod
def connection_resolver(
cls,
resolver,
connection,
default_manager,
max_limit,
enforce_first_or_last,
filterset_class,
filtering_args,
root,
info,
**args
def resolve_queryset(
cls, connection, iterable, info, args, filtering_args, filterset_class
):
filter_kwargs = {k: v for k, v in args.items() if k in filtering_args}
qs = filterset_class(
data=filter_kwargs,
queryset=default_manager.get_queryset(),
request=info.context,
return filterset_class(
data=filter_kwargs, queryset=iterable, request=info.context
).qs

return super(DjangoFilterConnectionField, cls).connection_resolver(
resolver,
connection,
qs,
max_limit,
enforce_first_or_last,
root,
info,
**args
)

def get_resolver(self, parent_resolver):
def get_queryset_resolver(self):
return partial(
self.connection_resolver,
parent_resolver,
self.connection_type,
self.get_manager(),
self.max_limit,
self.enforce_first_or_last,
self.filterset_class,
self.filtering_args,
self.resolve_queryset,
filterset_class=self.filterset_class,
filtering_args=self.filtering_args,
)
96 changes: 43 additions & 53 deletions graphene_django/filter/tests/test_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -608,58 +608,6 @@ def resolve_all_reporters(self, info, **args):
assert result.data == expected


def test_should_query_filter_node_double_limit_raises():
class ReporterFilter(FilterSet):
limit = NumberFilter(method="filter_limit")

def filter_limit(self, queryset, name, value):
return queryset[:value]

class Meta:
model = Reporter
fields = ["first_name"]

class ReporterType(DjangoObjectType):
class Meta:
model = Reporter
interfaces = (Node,)

class Query(ObjectType):
all_reporters = DjangoFilterConnectionField(
ReporterType, filterset_class=ReporterFilter
)

def resolve_all_reporters(self, info, **args):
return Reporter.objects.order_by("a_choice")[:2]

Reporter.objects.create(
first_name="Bob", last_name="Doe", email="bobdoe@example.com", a_choice=2
)
Reporter.objects.create(
first_name="John", last_name="Doe", email="johndoe@example.com", a_choice=1
)

schema = Schema(query=Query)
query = """
query NodeFilteringQuery {
allReporters(limit: 1) {
edges {
node {
id
firstName
}
}
}
}
"""

result = schema.execute(query)
assert len(result.errors) == 1
assert str(result.errors[0]) == (
"Received two sliced querysets (high mark) in the connection, please slice only in one."
)


def test_order_by_is_perserved():
class ReporterType(DjangoObjectType):
class Meta:
Expand Down Expand Up @@ -721,7 +669,7 @@ def resolve_all_reporters(self, info, reverse_order=False, **args):
assert reverse_result.data == reverse_expected


def test_annotation_is_perserved():
def test_annotation_is_preserved():
class ReporterType(DjangoObjectType):
full_name = String()

Expand Down Expand Up @@ -766,6 +714,48 @@ def resolve_all_reporters(self, info, **args):
assert result.data == expected


def test_annotation_with_only():
class ReporterType(DjangoObjectType):
full_name = String()

class Meta:
model = Reporter
interfaces = (Node,)
filter_fields = ()

class Query(ObjectType):
all_reporters = DjangoFilterConnectionField(ReporterType)

def resolve_all_reporters(self, info, **args):
return Reporter.objects.only("first_name", "last_name").annotate(
full_name=Concat(
"first_name", Value(" "), "last_name", output_field=TextField()
)
)

Reporter.objects.create(first_name="John", last_name="Doe")

schema = Schema(query=Query)

query = """
query NodeFilteringQuery {
allReporters(first: 1) {
edges {
node {
fullName
}
}
}
}
"""
expected = {"allReporters": {"edges": [{"node": {"fullName": "John Doe"}}]}}

result = schema.execute(query)

assert not result.errors
assert result.data == expected


def test_integer_field_filter_type():
class PetType(DjangoObjectType):
class Meta:
Expand Down
58 changes: 57 additions & 1 deletion graphene_django/tests/test_query.py
Original file line number Diff line number Diff line change
Expand Up @@ -638,6 +638,8 @@ class Meta:
class Query(graphene.ObjectType):
all_reporters = DjangoConnectionField(ReporterType)

assert Query.all_reporters.max_limit == 100

r = Reporter.objects.create(
first_name="John", last_name="Doe", email="johndoe@example.com", a_choice=1
)
Expand Down Expand Up @@ -679,6 +681,8 @@ class Meta:
class Query(graphene.ObjectType):
all_reporters = DjangoConnectionField(ReporterType)

assert Query.all_reporters.max_limit == 100

r = Reporter.objects.create(
first_name="John", last_name="Doe", email="johndoe@example.com", a_choice=1
)
Expand Down Expand Up @@ -804,7 +808,7 @@ def resolve_all_reporters(self, info, **args):
schema = graphene.Schema(query=Query)
query = """
query ReporterLastQuery {
allReporters(first: 2) {
allReporters(first: 1) {
edges {
node {
id
Expand Down Expand Up @@ -1116,3 +1120,55 @@ def resolve_films(root, info):
with django_assert_num_queries(3) as captured:
result = schema.execute(query)
assert not result.errors


def test_should_preserve_annotations():
class ReporterType(DjangoObjectType):
class Meta:
model = Reporter
interfaces = (graphene.relay.Node,)

class FilmType(DjangoObjectType):
reporters = DjangoConnectionField(ReporterType)
reporters_count = graphene.Int()

class Meta:
model = Film
interfaces = (graphene.relay.Node,)

class Query(graphene.ObjectType):
films = DjangoConnectionField(FilmType)

def resolve_films(root, info):
qs = Film.objects.prefetch_related("reporters")
return qs.annotate(reporters_count=models.Count("reporters"))

r1 = Reporter.objects.create(first_name="Dave", last_name="Smith")
r2 = Reporter.objects.create(first_name="Jane", last_name="Doe")

f1 = Film.objects.create()
f1.reporters.set([r1, r2])
f2 = Film.objects.create()
f2.reporters.set([r2])

query = """
query {
films {
edges {
node {
reportersCount
}
}
}
}
"""
schema = graphene.Schema(query=Query)
result = schema.execute(query)
assert not result.errors, str(result)

expected = {
"films": {
"edges": [{"node": {"reportersCount": 2}}, {"node": {"reportersCount": 1}}]
}
}
assert result.data == expected, str(result.data)