-
Notifications
You must be signed in to change notification settings - Fork 301
Filtering array query params #718
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
Comments
@ro70 the json api specification is a bit vague on this but the example in https://jsonapi.org/recommendations/#filtering doesn't cover this use case. However do you have an Ember reference which explains this use of query params? @n2ygk What is your take on this? |
@sliverc I've only ever seen the example you cited. I don't quite understand the purpose of the extra brackets. It seems the way I would do this is @ro70 are you sure it's doing the right thing? |
The fact that Ember Data (specifically the JsonAPIAdapter) serializes arrays in the query parameters by adding empty brackets isn't very well documented. A hint that this is the case can be found here:
The relevant code where the actual serialization takes place can be found here: Even though this is not detailed out in the JSON API Specification I'd argue to consider the way Ember does this more or less the de-facto standard as there isn't any other reference client side implementation of the JSON API specification. If not DJA should at least offer the option to handle query parameter lists the Ember way. |
@PUREMATH Thanks for the reference. For DJA the json api specification is the reference and Ember actually admits that it is currently not fully JSON API compliant see emberjs/data#2905 There it is also mentioned that filtering is not supported yet. From the Ember reference
I guess changing this within Ember is simple and would be the preferable way. Maybe such a Ember snippet could be added to the DJA documentation. @n2ygk What is your take on it? |
@sliverc I suppose @PUREMATH could ask the question on the jsonapi forum and see if there's any kind of broad usage or consensus around supporting the bracket-style of filtering lists. Any implementation would not be "standard-compliant" since the standard doesn't define the use of filter at all beyond using the parameter name filter. The only thing we have to work from is the recommendation which shows a list membership example that does not use extra brackets: Perhaps it would be a good idea to add documentation of how to get the above example to work. Something like: from django_filters import rest_framework as filters
...
class CommentSerializer(HyperlinkedModelSerializer):
class Meta:
model = Comment
fields = "__all__"
posts = ResourceRelatedField(model=Post,...)
class CommentFilterSet(filters.FilterSet):
"""
Extend :py:class:`django_filters.rest_framework.FilterSet` for the Comment model
"""
#: make `filter[post]` be a list membership filter:
post = filters.CharFilter(field_name="posts", lookup_expr="in")
class Meta:
model = Comment
class CommentViewSet(ModelViewSet):
queryset = Comment.objects.all()
serializer_class = CommentSerializer
filterset_class = CommentFilterSet I don't see a need to extend core DJA to support the extra bracket notation but certainly would like to see documentation of how one might extend it. |
At this point it seems according to the documentation Ember doesn't even use the filter query parameter but simply uses So it seems someone would need to extend the Ember So I think from my view for Ember users overwriting the As a side note in the still working spec 1.1 I have found following referene which actually states that |
Use of |
In the meanwhile I changed the query parameters to
Now another problem occurs:
Without the marked modifications in the
data2 is not as expected. The assignment follows the "last-value logic" (see https://kite.com/python/docs/django.http.QueryDict) With the modifications it looks like
djangorestframework-jsonapi==3.0.0 |
According to the specification recommendations multiple filter values should be separated by a comma and as far as I know depending on how the filter class is configured this should already work today. The idea is that query parameters are combined with the Just swallowing query parameters is not nice though but would need to think about the best way of warning the user maybe in the query validator class. |
@sliverc The specification mentions an example like
where I get all x having tags with id 1 or 2. But I'd like to have the possibility to "and" those filters too. I'd like to get all x having tags with id 1 and 2. Therefore I need something like
or
Maybe in the (*1) query the first parameter will be overwritten, so that only the |
@ro70 This makes total sense especially with your tags example and I have overlooked this use case before. I prefer to implement *1 though over *2 because:
It would be great if you could open a PR. |
@sliverc Below a few thoughts about "anded/ored" filters. I'd like to know what you think about it. Should I go further with one of these approaches or do you have other ideas/conceptions in mind? FilterSet approach A few changes in the
tags__id filter The
or single filtering
tags__id__in filter The
This filter declaration can be replaced by a meta field (the standard "in" filter):
This approach easily allows to change the filters to (without
if you like. Lookup approach I thought about a custom "allin" lookup (based on the "in" lookup) to make requests like
But I don't think this is an easy task (or even possible at all). See https://github.com/django/django/blob/master/django/db/models/lookups.py#L357 . |
We only want to implement in DJA what is actually JSON API spec specific; otherwise such implementation need to happen in the respective upstream project (Django Filter, DRF or even Django itself). |
Some clients (e.g. Ember) send requests like
?filter[tags][]=1&filter[tags][]=2
for the membership in a list of values (JSON API).To process this kind of filter I only changed (https://github.com/django-json-api/django-rest-framework-json-api/blob/master/rest_framework_json_api/django_filters/backends.py#L66) from
to
Everything seems to work fine. What do you think?
The text was updated successfully, but these errors were encountered: