Skip to content

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

Closed
ro70 opened this issue Oct 8, 2019 · 13 comments · Fixed by #782
Closed

Filtering array query params #718

ro70 opened this issue Oct 8, 2019 · 13 comments · Fixed by #782

Comments

@ro70
Copy link
Contributor

ro70 commented Oct 8, 2019

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

^filter(?P<ldelim>\[?)(?P<assoc>[\w\.\-]*)(?P<rdelim>\]?$)

to

^filter(?P<ldelim>\[?)(?P<assoc>[\w\.\-]*)(?P<rdelim>\]?)(\[\])?$

Everything seems to work fine. What do you think?

@sliverc
Copy link
Member

sliverc commented Oct 8, 2019

@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?

@n2ygk
Copy link
Contributor

n2ygk commented Oct 8, 2019

@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 filter[tags.in]=1,2 or, defining a FilterSet that does the "in", filter[tags]=1,2. Should be no problem for an individual to extended DjangoFilterBackend and override the filter_regex attribute. However, I'm not sure what the behavior is since I would expect filter[tags]=1&filter[tags]=2 to try to AND those.

@ro70 are you sure it's doing the right thing?

@hb9hnt
Copy link
Contributor

hb9hnt commented Oct 9, 2019

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:
https://api.emberjs.com/ember-data/3.13/classes/JSONAPIAdapter/methods?anchor=findMany

The IDs will be passed as a URL-encoded Array of IDs, in this form:
ids[]=1&ids[]=2&ids[]=3

The relevant code where the actual serialization takes place can be found here:
https://github.com/emberjs/data/blob/v3.13.1/packages/adapter/addon/-private/utils/serialize-query-params.ts#L20

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.

@sliverc
Copy link
Member

sliverc commented Oct 14, 2019

@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 findMany:

If you want to encode the IDs, differently, just override this (one-line) method.

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?

@n2ygk
Copy link
Contributor

n2ygk commented Oct 14, 2019

@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: GET /comments?filter[post]=1,2 HTTP/1.1

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.

@sliverc
Copy link
Member

sliverc commented Oct 15, 2019

At this point it seems according to the documentation Ember doesn't even use the filter query parameter but simply uses ids[]=1&ids[]=2&ids[]=3 as query parameters which is certainly not following the spec.

So it seems someone would need to extend the Ember JSONAPIAdapter anyway to use the query parameter filter. Also overwriting findMany in this adapter wouldn't be a big deal then.

So I think from my view for Ember users overwriting the JSONAPIAdapter would be the way to go If someone could provide a sample in this issue howto that would be great so in case other stumble upon it as well.

As a side note in the still working spec 1.1 I have found following referene which actually states that filter[name][] is a valid filter but still not defining what it does. But this is still WIP but just as a reference.

@n2ygk
Copy link
Contributor

n2ygk commented Oct 15, 2019

Use of ids[]=1... explains the MultipleIDMixin we deprecated a while ago.

@ro70
Copy link
Contributor Author

ro70 commented Mar 30, 2020

In the meanwhile I changed the query parameters to

?filter[tags]=1&filter[tags]=2

Now another problem occurs:

def get_filterset_kwargs(self, request, queryset, view):
    """
    Turns filter[<field>]=<value> into <field>=<value> which is what
    DjangoFilterBackend expects

    :raises ValidationError: for bad filter syntax
    """
    filter_keys = []
    # rewrite filter[field] query params to make DjangoFilterBackend work.
    data = request.query_params.copy()
    print(data)                                                       # data1
    for qp, val in request.query_params.lists():                      # <-- lists() instead of items()
        m = self.filter_regex.match(qp)
        if m and (not m.groupdict()['assoc'] or
                  m.groupdict()['ldelim'] != '[' or m.groupdict()['rdelim'] != ']'):
            raise ValidationError("invalid query parameter: {}".format(qp))
        if m and qp != self.search_param:
            if not val:
                raise ValidationError("missing {} test value".format(qp))
            # convert jsonapi relationship path to Django ORM's __ notation
            key = m.groupdict()['assoc'].replace('.', '__')
            # undo JSON_API_FORMAT_FIELD_NAMES conversion:
            key = format_value(key, 'underscore')
            data.setlist(key, val)                                    # <-- instead of data[key] = val
            filter_keys.append(key)
            del data[qp]
    print(data)                                                       # data2
    return {
        'data': data,
        'queryset': queryset,
        'request': request,
        'filter_keys': filter_keys,
    }

Without the marked modifications in the get_filterset_kwargs method of the rest_framework_json_api.django_filters.DjangoFilterBackend
data1 and data2 will be

data1: <QueryDict: {'filter[tags]': ['1', '2']}>
data2: <QueryDict: {'tags': ['2']}>

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

data1: <QueryDict: {'filter[tags]': ['1', '2']}>
data2: <QueryDict: {'tags': ['1', '2']}>

djangorestframework-jsonapi==3.0.0

@sliverc
Copy link
Member

sliverc commented Apr 6, 2020

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 AND operator and comma separated values as OR.

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.

@ro70
Copy link
Contributor Author

ro70 commented Apr 11, 2020

@sliverc The specification mentions an example like

GET /x?filter[tags]=1,2

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

GET /x?filter[tags]=1&filter[tags]=2  (*1)

or

GET /x?filter[tags][]=1&filter[tags][]=2  (*2)

Maybe in the (*1) query the first parameter will be overwritten, so that only the filter[tags]=2 will be considered.
The (*2) query tells that we have a multivalue parameter here.

@sliverc
Copy link
Member

sliverc commented Apr 11, 2020

@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:

  1. at this point the JSON API spec doesn't state that [][] is allowed
  2. simply changing regex to implement [][] is not a complete implementation as it would mean someone could actually add an additional [] to non-multivalues - so validation would need to be adjusted too. Because of my first point I think we should leave this for now till it is clearer what JSON API spec 1.1 will do with filters.

It would be great if you could open a PR.

@ro70
Copy link
Contributor Author

ro70 commented Apr 15, 2020

@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 rest_framework_json_api.django_filters.DjangoFilterBackend (see above) make the following work (I didn't care about the filter validation up to now):

class XFilterSet(FilterSet):
    tags__id = ModelMultipleChoiceFilter(                 # (1)
        field_name='tags',
        to_field_name = 'id',
        lookup_expr = 'in',
        conjoined = True,  # to "and" the values
        queryset=Tag.objects.all(),
    )
    tags__id__in = ModelMultipleChoiceFilter(            # (2)
        field_name='tags',
        to_field_name = 'id',
        lookup_expr = 'in',
        conjoined = False,  # to "or" the values (default)
        queryset=Tag.objects.all(),
    )
    class Meta:
       model = Tag
       fields = ()

class XViewSet(ModelViewSet):
   filterset_class = XFilterSet

tags__id filter

The tags__id filter (1) allows "anded" requests like

GET /x?filter[tags.id]=1&filter[tags.id]=2

or single filtering

GET /x?filter[tags.id]=1

tags__id__in filter

The tags__id__in filter (2) takes "ored" requests like

GET /x?filter[tags.id.in]=1,2

This filter declaration can be replaced by a meta field (the standard "in" filter):

fields = {
    'tags__id': ['in', ],
}

This approach easily allows to change the filters to (without id):

GET /x?filter[tags]=1&filter[tags]=2
GET /x?filter[tags.in]=1,2

if you like.


Lookup approach

I thought about a custom "allin" lookup (based on the "in" lookup) to make requests like

GET /x?filter[tags.id.allin]=1,2

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 .

@sliverc
Copy link
Member

sliverc commented Apr 19, 2020

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).
So a allin lookup is not DJA specific in this regard. And how DJA users use DjangoFilterSet is also up to them as JSON API spec doesn't define how filter arguments are used.
If you are referring to changes suggested in #718 (comment) I think that it is a good way forward to make the backend more powerful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants