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

Logical OR on multiple values for a CharField #1076

Closed
jeremystretch opened this issue May 3, 2019 · 11 comments
Closed

Logical OR on multiple values for a CharField #1076

jeremystretch opened this issue May 3, 2019 · 11 comments

Comments

@jeremystretch
Copy link

Use Case

Suppose I have a Device model defined like this:

class Device(models.Model):
    name = models.CharField(max_length=100)

And I have a Django REST API endpoint at /devices/ which lists all Devices. What I'd like to do is turn a query such as

GET /devices/?name=Foo&name=Bar&name=Baz

into the Queryset

Device.objects.filter(Q(name='Foo') | Q(name='Bar') | Q(name='Baz'))

so that any Device named Foo OR Bar OR Baz is returned. The response should not trigger a validation error if any of these names are not found.

When using a stock CharFilter on the name field, only the last parameter is passed to the filter, resulting in the Queryset

Device.objects.filter(name='Baz')

What I've Tried

MultipleChoiceFilter doesn't work because it requires a predefined set of choices to be provided.

AllValuesMultipleFilter sort of works, but it populates a list of all available values from the database to check against. This is undesirable both because of the additional overhead introduced and because it raises a ValidationError if any of the provided values do not exist. I don't want to validate the provided values; I only want to match on them.

Providing a custom method on CharFilter doesn't seem to be an option because we need to provide a Django form field capable of accepting multiple values.

Apologies if I've missed something obvious. I've looked around for other issues relating to this but didn't find anything quite the same (#137 was very similar).

@rpkilby
Copy link
Collaborator

rpkilby commented May 3, 2019

If an in lookup is an acceptable substitution, then you could use the CSV-based BaseInFilter. e.g.,

# declarative
class CharInFilter(BaseInFilter, CharFilter):
    pass

class F(FilterSet):
    name = CharInFilter(field_name='name', lookup_expr='in')

    class Meta:
        model = Device
        fields = ['name']

# generative
class F(FilterSet):
    class Meta:
        model = Device
        fields = {'name': ['in']}

The call would look like:

GET /devices/?name=Foo,Bar,Baz

@jeremystretch
Copy link
Author

Thanks for the suggestion! It's something I considered early on, but I'd prefer to stay away from CSV-formatted parameters. Partly because it complicates the logic needed to form a request, but also because it deviates from the way ChoiceFields are handled natively. For example, suppose the Device model was:

class Device(models.Model):
    name = models.CharField(max_length=100)
    site = models.ForeignKey('Site')

And we want to find devices named Foo, Bar, or Baz in SiteA or SiteB. If we used the BaseInFilter approach, the request string would look like

GET /devices/?name=Foo,Bar,Baz&site=SiteA&site=SiteB

We end up mixing two different methods conveying the same logic. I could definitely see using the approach to distinguish between OR and AND logic though.

@rpkilby
Copy link
Collaborator

rpkilby commented May 6, 2019

So, it sounds like you want a MultipleChoiceField, but you don't want it to actually validate?

In that case, you might be able to just define your own form field class that inherits MultipleChoiceField, but the validate method is just a noop. e.g.,

class NonValidatingMultipleChoiceField(forms.MultipleChoiceField):
    def validate(self, value):
        pass

# Or inherit `filters.AllValuesMultipleFilter`
class NonValidatingMultipleChoiceFilter(filters.MultipleChoiceFilter):
    field_class = NonValidatingMultipleChoiceField

@jeremystretch
Copy link
Author

Aha, I think that works! I was focused on finding a way to override choices specifically without considering that we could just bypass validation altogether. Thank you!

Is there any interest in extending one of the built-in filters to support this behavior? If so, I'd be happy to submit a PR. And if not I think the approach you've pointed out is perfectly fine.

@rpkilby
Copy link
Collaborator

rpkilby commented May 6, 2019

cc @carltongibson, but probably not. Part of the point of this package is to validate inputs. Removing validation is removing some of this value. Maybe it's something we could add as a docs snippet?

@carltongibson
Copy link
Owner

Need ☕️ but, can't we use an In filter with the &site=...&site=... query string syntax?

@rpkilby
Copy link
Collaborator

rpkilby commented May 7, 2019

Out of the box, the only filters that support &site=...&site=... are the multiple choice filters. However, the issue is that he wants to avoid the validation that comes with choice-based filters.

@carltongibson
Copy link
Owner

carltongibson commented May 8, 2019

OK, good. ☕️ but 24hrs later. 🙂

But, what I'm thinking is that there's no reason why BaseInFilter couldn't use a widget (via it's field) that wasn't actually CSV based. (e.g. QueryArrayWidget?) As long as value_from_datadict() returns a list, the filter shouldn't care.

@rpkilby
Copy link
Collaborator

rpkilby commented May 8, 2019

I mean, all BaseInFilter does is provide the CSV field/widget mixing. All the real work is performed by the mixed in BaseCSVField and BaseCSVWidget. It could definitely work with a different widget that returns the full value list (like the builtin SelectMultiple widget). I guess it just depends on what features you want (e.g., render the multiple choice list, but don't validate against it).

re: QueryArrayWidget, not sure if that would work for his case, since it allows for three different array syntaxes. In this case I think he just wants the traditional param=value&param=value.

Anyway, is there anything to do here?

@carltongibson
Copy link
Owner

Anyway, is there anything to do here?

No. 😀

But thanks for the thoughtful input, as always!

@rpkilby
Copy link
Collaborator

rpkilby commented May 8, 2019

yeah. Also thanks @jeremystretch. Interesting issue.

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