From 23eb8eea80aa8921c4b73006a73d53e657a8207d Mon Sep 17 00:00:00 2001 From: Daniel Bachler Date: Wed, 11 Oct 2017 15:43:50 +0200 Subject: [PATCH 1/5] Add failing test case --- graphene_django/tests/models.py | 4 ++ graphene_django/tests/test_query.py | 59 +++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/graphene_django/tests/models.py b/graphene_django/tests/models.py index 6ae705d..029b318 100644 --- a/graphene_django/tests/models.py +++ b/graphene_django/tests/models.py @@ -19,6 +19,10 @@ class FilmDetails(models.Model): class Film(models.Model): + genre = models.CharField(max_length=2, help_text='Genre', choices=[ + ('do', 'Documentary'), + ('ot', 'Other') + ], default='ot') reporters = models.ManyToManyField('Reporter', related_name='films') diff --git a/graphene_django/tests/test_query.py b/graphene_django/tests/test_query.py index cb2bde4..70fae46 100644 --- a/graphene_django/tests/test_query.py +++ b/graphene_django/tests/test_query.py @@ -5,6 +5,8 @@ from django.db import models from django.utils.functional import SimpleLazyObject from py.test import raises +from django.db.models import Q + import graphene from graphene.relay import Node @@ -17,6 +19,8 @@ from .models import ( Article, CNNReporter, Reporter, + Film, + FilmDetails, ) pytestmark = pytest.mark.django_db @@ -431,6 +435,61 @@ def test_should_query_node_filtering(): assert result.data == expected +@pytest.mark.skipif(not DJANGO_FILTER_INSTALLED, + reason="django-filter should be installed") +def test_should_query_node_filtering_with_distinct_queryset(): + class FilmType(DjangoObjectType): + + class Meta: + model = Film + interfaces = (Node, ) + filter_fields = ('genre',) + + class Query(graphene.ObjectType): + films = DjangoConnectionField(FilmType) + + # def resolve_all_reporters_with_berlin_films(self, args, context, info): + # return Reporter.objects.filter(Q(films__film__location__contains="Berlin") | Q(a_choice=1)) + + def resolve_films(self, args, context, info): + return Film.objects.filter(Q(details__location__contains="Berlin") | Q(genre__in=['ot'])).distinct() + + f = Film.objects.create( + ) + fd = FilmDetails.objects.create( + location="Berlin", + film=f + ) + + schema = graphene.Schema(query=Query) + query = ''' + query NodeFilteringQuery { + films { + edges { + node { + genre + } + } + } + } + ''' + + expected = { + 'films': { + 'edges': [{ + 'node': { + 'genre': 'ot' + } + }] + } + } + + result = schema.execute(query) + assert not result.errors + print(result.data) + assert result.data == expected + + @pytest.mark.skipif(not DJANGO_FILTER_INSTALLED, reason="django-filter should be installed") def test_should_query_node_multiple_filtering(): From e05fbcc1b34d31f9569feeacd5d430e08f421f9a Mon Sep 17 00:00:00 2001 From: Daniel Bachler Date: Wed, 11 Oct 2017 15:54:00 +0200 Subject: [PATCH 2/5] Fix failing unit test by handling cases where a connection is resolved involving a query with inner join and distinct that is then filtered and would be combined with a filtered queryset that is not distinct. --- graphene_django/fields.py | 4 ++++ graphene_django/tests/test_query.py | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/graphene_django/fields.py b/graphene_django/fields.py index e755b93..854c691 100644 --- a/graphene_django/fields.py +++ b/graphene_django/fields.py @@ -77,6 +77,10 @@ class DjangoConnectionField(ConnectionField): if isinstance(iterable, QuerySet): if iterable is not default_manager: default_queryset = maybe_queryset(default_manager) + if default_queryset.query.distinct and not iterable.query.distinct: + iterable = iterable.distinct() + elif iterable.query.distinct and not default_queryset.query.distinct: + default_queryset = default_queryset.distinct() iterable = cls.merge_querysets(default_queryset, iterable) _len = iterable.count() else: diff --git a/graphene_django/tests/test_query.py b/graphene_django/tests/test_query.py index 70fae46..913a93f 100644 --- a/graphene_django/tests/test_query.py +++ b/graphene_django/tests/test_query.py @@ -478,7 +478,7 @@ def test_should_query_node_filtering_with_distinct_queryset(): 'films': { 'edges': [{ 'node': { - 'genre': 'ot' + 'genre': 'OT' } }] } From 4d41160379cf5757764f8ca5690d616a6de3bbf8 Mon Sep 17 00:00:00 2001 From: Daniel Bachler Date: Wed, 28 Feb 2018 17:45:25 +0100 Subject: [PATCH 3/5] Update resolve method signature to 2.0 style in new test --- graphene_django/tests/test_query.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graphene_django/tests/test_query.py b/graphene_django/tests/test_query.py index 913a93f..62524d4 100644 --- a/graphene_django/tests/test_query.py +++ b/graphene_django/tests/test_query.py @@ -451,7 +451,7 @@ def test_should_query_node_filtering_with_distinct_queryset(): # def resolve_all_reporters_with_berlin_films(self, args, context, info): # return Reporter.objects.filter(Q(films__film__location__contains="Berlin") | Q(a_choice=1)) - def resolve_films(self, args, context, info): + def resolve_films(self, info, **args): return Film.objects.filter(Q(details__location__contains="Berlin") | Q(genre__in=['ot'])).distinct() f = Film.objects.create( From 34f59784469eb18a4b6a9e6938e8e62517ff110a Mon Sep 17 00:00:00 2001 From: Daniel Bachler Date: Wed, 28 Feb 2018 17:52:27 +0100 Subject: [PATCH 4/5] Move distinct check code to merge_querysets again --- graphene_django/fields.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/graphene_django/fields.py b/graphene_django/fields.py index 854c691..5576454 100644 --- a/graphene_django/fields.py +++ b/graphene_django/fields.py @@ -67,6 +67,10 @@ class DjangoConnectionField(ConnectionField): @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 @@ -77,10 +81,6 @@ class DjangoConnectionField(ConnectionField): if isinstance(iterable, QuerySet): if iterable is not default_manager: default_queryset = maybe_queryset(default_manager) - if default_queryset.query.distinct and not iterable.query.distinct: - iterable = iterable.distinct() - elif iterable.query.distinct and not default_queryset.query.distinct: - default_queryset = default_queryset.distinct() iterable = cls.merge_querysets(default_queryset, iterable) _len = iterable.count() else: From c32340662cd36b083100614e4dbb399f5b45a972 Mon Sep 17 00:00:00 2001 From: Daniel Bachler Date: Wed, 28 Mar 2018 12:28:58 +0200 Subject: [PATCH 5/5] Remove print statement --- graphene_django/tests/test_query.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/graphene_django/tests/test_query.py b/graphene_django/tests/test_query.py index 62524d4..09ca702 100644 --- a/graphene_django/tests/test_query.py +++ b/graphene_django/tests/test_query.py @@ -486,7 +486,6 @@ def test_should_query_node_filtering_with_distinct_queryset(): result = schema.execute(query) assert not result.errors - print(result.data) assert result.data == expected @@ -735,7 +734,7 @@ def test_should_query_promise_connectionfields(): def resolve_all_reporters(self, info, **args): return Promise.resolve([Reporter(id=1)]) - + schema = graphene.Schema(query=Query) query = ''' query ReporterPromiseConnectionQuery { @@ -783,7 +782,7 @@ def test_should_query_connectionfields_with_last(): def resolve_all_reporters(self, info, **args): return Reporter.objects.all() - + schema = graphene.Schema(query=Query) query = ''' query ReporterLastQuery { @@ -838,7 +837,7 @@ def test_should_query_connectionfields_with_manager(): def resolve_all_reporters(self, info, **args): return Reporter.objects.all() - + schema = graphene.Schema(query=Query) query = ''' query ReporterLastQuery { @@ -1071,7 +1070,7 @@ def test_proxy_model_fails(): """ This test asserts that if you try to query for a proxy model, that query will fail with: - GraphQLError('Expected value of type "CNNReporterType" but got: + GraphQLError('Expected value of type "CNNReporterType" but got: CNNReporter.',) This is because a proxy model has the identical model definition