From c29aa7c8c4b492a3b932d372119ba37a25353390 Mon Sep 17 00:00:00 2001 From: Jason Kraus Date: Fri, 11 Oct 2019 14:15:27 -0700 Subject: [PATCH 1/8] replace merge_queryset with resolve_queryset pattern --- graphene_django/fields.py | 37 ++++++++-------- graphene_django/filter/fields.py | 68 ++++------------------------- graphene_django/tests/test_query.py | 6 ++- 3 files changed, 30 insertions(+), 81 deletions(-) diff --git a/graphene_django/fields.py b/graphene_django/fields.py index e6daa889e..b8a96bf18 100644 --- a/graphene_django/fields.py +++ b/graphene_django/fields.py @@ -39,10 +39,8 @@ 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 - queryset = maybe_queryset( - django_object_type.get_queryset(model.objects, info) - ) + model = django_object_type._meta.model.objects + queryset = maybe_queryset(django_object_type.get_queryset(model, info)) return queryset def get_resolver(self, parent_resolver): @@ -108,25 +106,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) @@ -150,6 +136,7 @@ def connection_resolver( resolver, connection, default_manager, + queryset_resolver, max_limit, enforce_first_or_last, root, @@ -177,9 +164,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) @@ -192,6 +185,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 diff --git a/graphene_django/filter/fields.py b/graphene_django/filter/fields.py index 338becb72..994334694 100644 --- a/graphene_django/filter/fields.py +++ b/graphene_django/filter/fields.py @@ -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, ) diff --git a/graphene_django/tests/test_query.py b/graphene_django/tests/test_query.py index f24f84bca..e8e991155 100644 --- a/graphene_django/tests/test_query.py +++ b/graphene_django/tests/test_query.py @@ -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 ) @@ -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 ) @@ -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 From de4da4b15a562252d48546d2b1f2fa001149d648 Mon Sep 17 00:00:00 2001 From: Jason Kraus Date: Fri, 11 Oct 2019 14:26:02 -0700 Subject: [PATCH 2/8] skip double limit test --- graphene_django/filter/tests/test_fields.py | 1 + 1 file changed, 1 insertion(+) diff --git a/graphene_django/filter/tests/test_fields.py b/graphene_django/filter/tests/test_fields.py index 1ffa0f452..5a7e03b98 100644 --- a/graphene_django/filter/tests/test_fields.py +++ b/graphene_django/filter/tests/test_fields.py @@ -608,6 +608,7 @@ def resolve_all_reporters(self, info, **args): assert result.data == expected +@pytest.mark.skip(reason="no longer relevant?") def test_should_query_filter_node_double_limit_raises(): class ReporterFilter(FilterSet): limit = NumberFilter(method="filter_limit") From d176b6349761dd91f24699f5dd387885e642b6d4 Mon Sep 17 00:00:00 2001 From: Jason Kraus Date: Fri, 18 Oct 2019 13:43:33 -0700 Subject: [PATCH 3/8] Update graphene_django/fields.py Co-Authored-By: Jonathan Kim --- graphene_django/fields.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphene_django/fields.py b/graphene_django/fields.py index b8a96bf18..a8aef09f3 100644 --- a/graphene_django/fields.py +++ b/graphene_django/fields.py @@ -39,7 +39,7 @@ 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.objects + model_manager = django_object_type._meta.model.objects queryset = maybe_queryset(django_object_type.get_queryset(model, info)) return queryset From 0a39cb43d681d0e2277e74f7465c638a93b164f4 Mon Sep 17 00:00:00 2001 From: Jason Kraus Date: Fri, 18 Oct 2019 13:44:37 -0700 Subject: [PATCH 4/8] yank skipped test --- graphene_django/filter/tests/test_fields.py | 53 --------------------- 1 file changed, 53 deletions(-) diff --git a/graphene_django/filter/tests/test_fields.py b/graphene_django/filter/tests/test_fields.py index 5a7e03b98..8ed490b61 100644 --- a/graphene_django/filter/tests/test_fields.py +++ b/graphene_django/filter/tests/test_fields.py @@ -608,59 +608,6 @@ def resolve_all_reporters(self, info, **args): assert result.data == expected -@pytest.mark.skip(reason="no longer relevant?") -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: From 3247eeacff659930bb6d8b984df1b50610156130 Mon Sep 17 00:00:00 2001 From: Jason Kraus Date: Fri, 18 Oct 2019 13:50:50 -0700 Subject: [PATCH 5/8] fix bad variable ref --- graphene_django/fields.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/graphene_django/fields.py b/graphene_django/fields.py index a8aef09f3..47b44f64a 100644 --- a/graphene_django/fields.py +++ b/graphene_django/fields.py @@ -40,7 +40,9 @@ def list_resolver(django_object_type, resolver, root, info, **args): # Default to Django Model queryset # N.B. This happens if DjangoListField is used in the top level Query object model_manager = django_object_type._meta.model.objects - queryset = maybe_queryset(django_object_type.get_queryset(model, info)) + queryset = maybe_queryset( + django_object_type.get_queryset(model_manager, info) + ) return queryset def get_resolver(self, parent_resolver): From 077b5aa96fd855d577e63d8f3bb5f557a733732d Mon Sep 17 00:00:00 2001 From: Jason Kraus Date: Fri, 18 Oct 2019 14:03:58 -0700 Subject: [PATCH 6/8] add test for annotations --- graphene_django/tests/test_query.py | 52 +++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/graphene_django/tests/test_query.py b/graphene_django/tests/test_query.py index e8e991155..95db2d192 100644 --- a/graphene_django/tests/test_query.py +++ b/graphene_django/tests/test_query.py @@ -1120,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) From 38fc15c481f0c718cc891e258f60922413ba48ad Mon Sep 17 00:00:00 2001 From: Jason Kraus Date: Mon, 21 Oct 2019 11:21:08 -0700 Subject: [PATCH 7/8] add test for using queryset with django filters --- graphene_django/filter/tests/test_fields.py | 44 ++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/graphene_django/filter/tests/test_fields.py b/graphene_django/filter/tests/test_fields.py index 8ed490b61..1eba601e9 100644 --- a/graphene_django/filter/tests/test_fields.py +++ b/graphene_django/filter/tests/test_fields.py @@ -669,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() @@ -714,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: From 8074dfa7c05e169133ce2dd8e0a30d3e7c49c76f Mon Sep 17 00:00:00 2001 From: Jason Kraus Date: Mon, 21 Oct 2019 15:01:50 -0700 Subject: [PATCH 8/8] document ththat one should use defer instead of values with queysets and DjangoObjectTypes --- docs/queries.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/queries.rst b/docs/queries.rst index 67ebb0681..36cdab1fd 100644 --- a/docs/queries.rst +++ b/docs/queries.rst @@ -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 -----------------