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

Conversation

zbyte64
Copy link
Collaborator

@zbyte64 zbyte64 commented Oct 11, 2019

There are a set of issues from our use of merge_queryset:

#429 : queryset cache gets nuked
#787 : Aggregate objects not supported
#758 : Annotations are lost

To resolve this we attempt to maintain a single queryset throughout the connection resolver instead of merging multiple querysets via the & operator. To accomplish this we replace the merge_queryset method with a get_queryset_resolver that returns a callable that modifies a queryset passed through.

As a result, the DjangoFilterConnectionField has a more terse interface for specifying how to modify filters.

Also I am not sure if graphene_django.filter.tests.test_fields.test_should_query_filter_node_double_limit_raises is still a valid test, but we should decide if we still wish to check limit changes even though it doesn't seem necessary anymore (this conflict doesn't raise a an error)

What is missing are tests for annotations and aggregations. If this purposed pattern is acceptable then I will write the unit tests for these.

Copy link
Member

@jkimbo jkimbo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change, it looks much cleaner. Minor comments only

@zbyte64
Copy link
Collaborator Author

zbyte64 commented Oct 18, 2019

Thanks for the feedback.

Added a test for annotations. Should I add a test specifically for interactions with DjangoFilterConnectionField?

@jkimbo
Copy link
Member

jkimbo commented Oct 19, 2019

Should I add a test specifically for interactions with DjangoFilterConnectionField?

@zbyte64 I think that would be really good actually because it feels like that part of the code is quite brittle.

@zbyte64
Copy link
Collaborator Author

zbyte64 commented Oct 21, 2019

There seems to already be an annotation test for django-filters: graphene_django.filter.tests.test_fields.test_annotation_is_perserved

I guess I need to go back and write a failing test because I don't know how that one passed.

@zbyte64
Copy link
Collaborator Author

zbyte64 commented Oct 21, 2019

Seems the key difference is using values. The code changes lets it get further along, but later graphene_django.type.DjangoObjectType.is_type_of raises an error. We can change this to return True for dictionaries, which is the result of a values query, but we would need to wrap it so that the key could be accessed as properties.

Edit: To clarify we can accept a dictionary and graphene is fine with that but if say ReporterType defines a resolve_foo method it may receive a dictionary instance or a model instance.

@zbyte64
Copy link
Collaborator Author

zbyte64 commented Oct 21, 2019

I think going forward we can support only and defer with querysets and but values is still problematic. Added a django-filters test: test_annotation_with_only to test just that.

@zbyte64 zbyte64 requested a review from jkimbo October 25, 2019 17:15
MicGong pushed a commit to drchrono/graphene-django that referenced this pull request Nov 6, 2019
MicGong pushed a commit to drchrono/graphene-django that referenced this pull request Nov 7, 2019
@jkimbo
Copy link
Member

jkimbo commented Nov 28, 2019

Again sorry for the very late review @zbyte64 . Merging

@jkimbo jkimbo merged commit a818ec9 into graphql-python:master Nov 28, 2019
@jkimbo
Copy link
Member

jkimbo commented Nov 28, 2019

Released as v2.7.0

bellini666 added a commit to bellini666/graphene-django that referenced this pull request Nov 28, 2019
The PR graphql-python#796 broke DjangoFilterConnectionField making it always get the
raw queryset from the model to apply the filters in it.

This makes sure that the DjangoObjectType's .get_queryset is called,
keeping any filtering it might have made.
bellini666 added a commit to bellini666/graphene-django that referenced this pull request Nov 28, 2019
The PR graphql-python#796 broke DjangoFilterConnectionField making it always get the
raw queryset from the model to apply the filters in it.

This makes sure that the DjangoObjectType's .get_queryset is called,
keeping any filtering it might have made.
jkimbo pushed a commit that referenced this pull request Nov 29, 2019
* Keep original queryset on DjangoFilterConnectionField

The PR #796 broke DjangoFilterConnectionField making it always get the
raw queryset from the model to apply the filters in it.

This makes sure that the DjangoObjectType's .get_queryset is called,
keeping any filtering it might have made.

* Add regression test
@zbyte64 zbyte64 deleted the merge_queryset branch December 1, 2019 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants