Fix distinct bug #290

danyx23 · 2017-10-11T17:46:27Z

This adds a test case and fixes /issues/289. Please review and let me know if anythin should change, I'm not an expert in either graphql nor django :)

spockNinja · 2017-10-14T18:35:49Z

@danyx23

This problem should be resolved by #194 or #224, which handles more than just the distinct being lost.

If you can verify that those fixes solve your problem, we can close this PR and it's corresponding issue.

Thanks!

jussih · 2017-10-24T14:00:32Z

@spockNinja #194 or #224 do not fix this issue. Distinct and non-distinct querysets cannot be combined, assertion in Django prevents this.

Really needing distinct queries, I resorted to casting the qs to a list before returning it at the end of the resolver function. This prevents DjangoConnectionField from trying to merge it.

BTW what is the use case of enforced merge_querysets? It seems to basically do Model.objects.all() AND [resolved_iterable] which does nothing (except breaking distincts ;).

spockNinja

A few discussion points. Also, the new test is currently failing. See https://travis-ci.org/graphql-python/graphene-django/jobs/286635259#L744

spockNinja · 2017-11-24T20:32:51Z

graphene_django/fields.py

@@ -90,6 +90,10 @@ def connection_resolver(cls, resolver, connection, default_manager, max_limit,
        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:


How about moving this logic into merge_querysets? Then any of the subclasses that use that will get this fix for free.

Good idea, did that

spockNinja · 2017-11-24T20:33:22Z

graphene_django/fields.py

@@ -90,6 +90,10 @@ def connection_resolver(cls, resolver, connection, default_manager, max_limit,
        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()


How does this behave when the other distinct call had field args? (queryset.distinct('field_1', 'field_2'))

I honestly have no idea. From my superficial understanding it should be fine :). Do I understand it correctly that this is only possible when using postgres backends?

spockNinja · 2017-11-24T20:36:13Z

graphene_django/tests/test_query.py

+
+    result = schema.execute(query)
+    assert not result.errors
+    print(result.data)


Lets be sure to drop this print once the test is working.

the print is still there :)

Ah sorry, missed your comment - thanks, will fix now!

danyx23 · 2017-12-02T16:06:46Z

Updated the PR. The travis check seems to rebase this on top of master but my test was written against django-graphene 1.x (4 parameters to resolve functions). Is there a way to make travis test this against the 1.x branch and once everyone is fine with the PR we adapt it to 2.0 and merge an updated PR there? Or is the 1.x line no longer supported?

spockNinja · 2017-12-09T21:54:04Z

Since this PR is merging into master, it will need to use the latest resolver signature. If you also want a PR to add this for an older version, you can target the PR against the tagged version instead of the master branch.

There also appears to be a merge conflict in fields.py.

Thanks a bunch for your work. Let's get those last things fixed up and I'll pass this on to the owner for a final review. 😄

patrys · 2018-02-15T14:47:02Z

@danyx23 Are you still working on this? If not I'd be happy to pick it where you've left it and adapt it for the current master.

@jussih The default qs implementation returning model.objects.all() is an implementation detail of django-filters so unless you propose that we detect whether a queryset is filtered, I think there's no easy way to skip merging in the default case of unfiltered data.

danyx23 · 2018-02-20T15:38:28Z

Ah damn, I totally forgot about this. Yes, I'll rebase against master and send an updated PR!

…d involving a query with inner join and distinct that is then filtered and would be combined with a filtered queryset that is not distinct.

coveralls · 2018-02-28T16:49:55Z

Coverage decreased (-0.02%) to 94.562% when pulling c323406 on DouglasConnect:fix-distinct-bug into dbd3957 on graphql-python:master.

danyx23 · 2018-02-28T17:17:52Z

Rebased against master, tests are green. Please let me know if you are happy for this to merge. (It's the first time I rebased a github PR, I should probably have created a new one to preserve the history of the old commits, sorry abou that)

danyx23 · 2018-03-28T09:52:12Z

Do you mind merging this or do you consider it blocked because the coverage decreased by 0.08%?

patrick91 · 2018-03-28T10:32:36Z

graphene_django/tests/test_query.py

+    class Query(graphene.ObjectType):
+        films = DjangoConnectionField(FilmType)
+
+        # def resolve_all_reporters_with_berlin_films(self, args, context, info):


sorry last one :) Is this needed?

I guess after resolving this @spockNinja or @syrusakbary can merge :)

There might be a way to integrate it with the existing test suite but I didn't see a straightforward way to construct a query that would lead to one "half" of the resulting query being distinct and the other not. Doing it in this way keeps the addition localized to where it is needed. If you have a suggestion on how to integrate it with the existing model instead please let me know!

syrusakbary · 2018-06-05T19:57:08Z

The PR looks good 👍 . Merging

spockNinja suggested changes Nov 24, 2017

View reviewed changes

danyx23 added 3 commits February 28, 2018 14:41

Add failing test case

23eb8ee

Fix failing unit test by handling cases where a connection is resolve…

e05fbcc

…d involving a query with inner join and distinct that is then filtered and would be combined with a filtered queryset that is not distinct.

Update resolve method signature to 2.0 style in new test

4d41160

danyx23 force-pushed the fix-distinct-bug branch from 04f5770 to 4d41160 Compare February 28, 2018 16:46

Move distinct check code to merge_querysets again

34f5978

Remove print statement

c323406

patrick91 reviewed Mar 28, 2018

View reviewed changes

syrusakbary closed this Jun 5, 2018

syrusakbary reopened this Jun 5, 2018

syrusakbary merged commit 7563045 into graphql-python:master Jun 5, 2018

Fix distinct bug #290

Fix distinct bug #290

Uh oh!

Conversation

danyx23 commented Oct 11, 2017

Uh oh!

spockNinja commented Oct 14, 2017

Uh oh!

jussih commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spockNinja left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danyx23 commented Dec 2, 2017

Uh oh!

spockNinja commented Dec 9, 2017

Uh oh!

patrys commented Feb 15, 2018

Uh oh!

danyx23 commented Feb 20, 2018

Uh oh!

coveralls commented Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danyx23 commented Feb 28, 2018

Uh oh!

danyx23 commented Mar 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syrusakbary commented Jun 5, 2018

Uh oh!

Uh oh!

jussih commented Oct 24, 2017 •

edited

Loading

coveralls commented Feb 28, 2018 •

edited

Loading