-
Notifications
You must be signed in to change notification settings - Fork 767
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
Conversation
There was a problem hiding this 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
Co-Authored-By: Jonathan Kim <jkimbo@gmail.com>
Thanks for the feedback. Added a test for annotations. Should I add a test specifically for interactions with |
@zbyte64 I think that would be really good actually because it feels like that part of the code is quite brittle. |
There seems to already be an annotation test for django-filters: I guess I need to go back and write a failing test because I don't know how that one passed. |
Seems the key difference is using Edit: To clarify we can accept a dictionary and graphene is fine with that but if say |
I think going forward we can support |
…and DjangoObjectTypes
Again sorry for the very late review @zbyte64 . Merging |
Released as v2.7.0 |
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.
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.
* 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
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 themerge_queryset
method with aget_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.