Skip to content
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

Correct filtering of __in with array #935

Closed
gabn88 opened this issue Jul 2, 2018 · 11 comments
Closed

Correct filtering of __in with array #935

gabn88 opened this issue Jul 2, 2018 · 11 comments

Comments

@gabn88
Copy link

gabn88 commented Jul 2, 2018

Let's say I want to filter myModel that has a foreignkey to MySubModel, to all instances that have as MySubModel with the following ids: [1,5,9], so I send an API call to:

MyAPI/mymodel?mysubmodel__in=1&mysubmodel__in=5&mysubmodel__in=9
I would expect the correct result, namely:

MyModel.filter(mysubmodel__in=[1,5,9])
but instead it does:

MyModel.filter(mysubmodel__in=1).filter(mysubmodel__in=5).filter(mysubmodel__in=9)
(at least that is what I expect from the result).

Is it possible to fix this in django-filter? Or do I need a custom filter?

@rpkilby
Copy link
Collaborator

rpkilby commented Jul 2, 2018

What type of filter are you using? The CSV-based filter should translate this:

MyAPI/mymodel?mysubmodel__in=1,5,9

into this:

MyModel.objects.filter(mysubmodel__in=[1,5,9])

@gabn88
Copy link
Author

gabn88 commented Jul 2, 2018

I'm using ngResources (angularJS) on the client, it translates the javascript code:

API.myModel.query({mysubmodel__in: [1,5,9]})

into:

MyAPI/mymodel?mysubmodel__in=1&mysubmodel__in=5&mysubmodel__in=9

@rpkilby
Copy link
Collaborator

rpkilby commented Jul 2, 2018

Two thoughts:

  • Is there no API... adapter(?)... that allows you to override how the query is constructed?
  • What is the underlying filterset definition for the mysubmodel__in filter?

@gabn88
Copy link
Author

gabn88 commented Jul 2, 2018

There is no adapter. There is an easy way to fix, namely:

API.myModel.query({mysubmodel__in: ''+[1,5,9]})

but I thought it would be good if django-filter supported the AngularJS syntax of the box? Or is there is reason that

MyAPI/mymodel?mysubmodel__in=1&mysubmodel__in=5&mysubmodel__in=9
and
MyAPI/mymodel?mysubmodel__in=1,5,9

are treated differently?

@rpkilby
Copy link
Collaborator

rpkilby commented Jul 2, 2018

Depending on the filter, this:

MyAPI/mymodel?mysubmodel__in=1&mysubmodel__in=5&mysubmodel__in=9

should be treated like this:

MyAPI/mymodel?mysubmodel__in=1

How are you creating the in filter? Are you autogenerating is with Meta.fields, or are you declaring it? Are you using the ModelMultipleChoiceFilter? The kind of filter will define the behavior you see.

@carltongibson
Copy link
Owner

It depends on the widget. Some widgets expect multiple values and get a list back from the QueryDict. Others expect just a single value.

Something like a multiple values filter should do more or less the right thing already.

@gabn88
Copy link
Author

gabn88 commented Jul 3, 2018

Ok, I think I got this.

First, a workaround for this bug is to do: MyAPI/mymodel?mysubmodel__in=1,5,9. This is what I used and it works fine. However, this requires our client dev to be very aware of the API filters, which often isn't the case.

So there seems to be a bug/unexpected behaviour on the FilterSet.

My FilterSet is defined as:

class MyModelFilter(django_filters.rest_framework.FilterSet):
    class Meta:
        fields = {'mysubmodel': ['in']}

However,

MyAPI/mymodel?mysubmodel__in=1&mysubmodel__in=5&mysubmodel__in=9
does not give the expected result. One does expect it to filter like this: filter(mysubmodel__in=[1,5,9]), but it seems to do: MyModel.filter(mysubmodel__in=1).filter(mysubmodel__in=5).filter(mysubmodel__in=9), which NEVER makes sense to do, because it always gives back None.

I have no idea how to fix this, and it has no hurry as there is an easy work-around, but it is definitely unexpected behaviour.

I gues 'somewhere' there must be an if call, something like this:

if (filter == expression with __in): 
    for filter in all_filters:
         if checkOtherFilterIsExpressionWith__inOnSameSubModel():
               combine__inExpressionsIntoOneArrayAndFilterOnThat()

@carltongibson
Copy link
Owner

carltongibson commented Jul 3, 2018

How about this:

class MyModelFilter(django_filters.rest_framework.FilterSet):
    mysubmodel = django_filters.MultipleChoiceFilter()    

Does it not already do exactly what you want?

The issue is that the fields automagic filter generation is creating a CSV compatible filter for you. If you just declare the filter by hand you can get the (old-school) ?mysubmodel=1&mysubmodel=5&mysubmodel=9 behaviour you're looking for.

@gabn88
Copy link
Author

gabn88 commented Jul 3, 2018

Oh, wow. That sounds great!

Does the new MyAPI/mymodel?mysubmodel__in=1,5,9 then also still work?

@carltongibson
Copy link
Owner

No — because that needs a CSV friendly filter... It's one way or the other (unless you declare two filters).

The CSV stuff was introduced because API crafters tend to prefer it.

@gabn88
Copy link
Author

gabn88 commented Jul 3, 2018

Ok, thanks for the information. It seems to be a matter of taste then. I will go with the taste of the API crafters 👍 although it still feels a bit strange that it does not fallback in case of the __in expression, like explained before.

I will close this issue, as you have answered all my questions. 🥇

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

No branches or pull requests

3 participants