Multi-param validation & filtering

[rpkilby]

@carltongibson - just jotting down some preliminary thoughts. Do you have any feedback?

---

Context:

Users periodically request how to validate and filter against multiple params. The goto advice has been to implement something based on MultiWidget, however there are three main deficiencies in my mind:

• It's verbose, users will typically need to subclass MultiWidget, MultiValueField, and Filter.
• It's difficult to implement. For new Django users, the MultiWidget/MultiValueField API is somewhat confusing (maybe I just didn't grok it for some reason, but I remember struggling with it in the past)
• As mentioned elsewhere, validation errors don't account for sub-widgets and are raised under the unsuffixed parameter name. This is acceptable for forms usage, where all sub widgets and errors are rendered together, but this mismatch doesn't work in an API context.

Proposal:

Add new API surface around multi-param filtering. This consists of two things:

• A Meta.multi or Meta.multiple option that maps group labels (?) to a list of filter names.
• Handling of validate_multi_* and filter_multi_*, which correspond to the multi labels.

In practice this would look like:

class MyFilter(filters.FilterSet):
    field_a = filters.CharFilter(...)
    field_b = filters.CharFilter(...)

    class Meta:
        model = MyModel
        multi = {
            'group_a': ['field_a', 'field_b'],
        }

    def validate_multi_group_a(self, form, *, field_a, field_b):
        if not some_condition(field_a, field_b):
            form.add_error('field_a', '...')
            form.add_error('field_b', '...')

            # Note: not the following, since it halts validation
            raise ValidationError({'field_a': ['...'], 'field_b': ['...']})

    def filter_multi_group_a(self, qs, *, field_a, field_b):
        return qs.do_a_thing(field_a=field_a, field_b=field_b)

The above hooks are purely opt-in.

• If a validate_multi_<group name> is defined, it will be called during the validation phase. Note that individual filter validation is still performed, and the validated values are passed to the method.
• If a filter_multi_<group name> is defined, it will be called during the filter phase in lieu of the individual .filter() methods.
• Empty values are passed to these methods, since it's necessary for validation/condition checking.

Notes:

• The above should be a relatively simple internal implementation, since the FilterSet is just calling hook methods at the appropriate time. One possible complication is validation, which will require adding a .clean() method to the form class creation.. it should be fine...
• The above should be simple to implement by users, since they only have to touch the FilterSet API, instead of mucking around with forms.

• The above is flexible - users can assert any arbitrary validation rules (e.g., fields must be all present together, they must be exclusive, some fields are required in some cases, etc...).

  • Example for the old Meta.together option (require all values are valid/present).

    def validate_multi_group_a(self, form, *, field_a, field_b):
        if field_a and not field_b:
            form.add_error('field_b', 'Field A and Field B are required together.')
        elif not field_a and field_b:
            form.add_error('field_a', 'Field A and Field B are required together.')

  • Example for the rejected Meta.exclusive option (see #784).

    def validate_multi_group_a(self, form, *, field_a, field_b):
        if field_a and field_b:
            form.add_error('field_a', 'Field A and Field B are mutually exclusive.')
            form.add_error('field_b', 'Field A and Field B are mutually exclusive.')

  • Example for more complex filtering (the MuliWidget use case).

    def validate_multi_group_a(self, form, *, field_a, field_b):
        """same as above for Meta.together"""
    
    def filter_multi_group_a(self, qs, *, field_a, field_b):
        return qs.filter(field_a__isnull=field_a, field_b__gte=field_b).

[carltongibson]

OK. Nice.

Can I ask, How would it look if we had a GroupFilter class that took Filters and allowed implementing filter() plus validate()?

I.e. just pulling what you have here into a composed class, rather than having the API directly on FilterSet.

[rpkilby]

That was actually my first thought - leverage the declarative filters. And I've kind of gone back and forth on both implementations after writing several lengthy paragraphs on the matter. Changed my opinion mid-response.. 😅

At this point, I see two proposals:

1. The one above.

2. The below:

   # Note that FilterGroup is not a Filter subclass
   class MyFilterGroup(FilterGroup):
       def filter(self, qs, **params):
           ...
       def validate(self, form, **params):
           ...
   
   class MyFilter(FilterSet):
       field_a = CharFilter(...)
       field_b = CharFilter(...)
       group = MyFilterGroup(
           filters=['field_a', 'field_b'],
       )

At this point, I'm leaning towards option 2. Option 1 is still attractive in that the code is simple - it's not necessary to write custom classes. That said, there isn't any abstraction for reusability. We could write helper functions, but that's about it. Option 2 would allow us/users to write reusable FilterGroups. Right now, the only thing I don't like about the declarative approach is that they don't make sense as a declared attribute on the FilterSet. With Filters, the attribute name is the exposed query param. With FilterGroups, the attribute name is functionless. e.g., a user wouldn't ever include ?group=foo in their query string.

Maybe a third option is to mix the styles? Use FilterGroups in the meta options? e.g.,

# Note that FilterGroup is not a Filter subclass
class MyFilter(FilterSet):
    field_a = CharFilter(...)
    field_b = CharFilter(...)
    field_c = CharFilter(...)
    field_d = CharFilter(...)

    class Meta:
        model = MyModel
        fields = ['field_a', 'field_b', 'field_c', 'field_d']
        groups = [
            ExclusiveGroup(filters=['field_a', 'field_b']),
            RequiredGroup(filters=['field_c', 'field_d']),
        ]

How does that last option sound?

[rpkilby]

@carltongibson - I'm going to go ahead and WIP up that last example.

[carltongibson]

OK. I haven't come to a conclusion. (🎯: "Nice API") 👍

[rpkilby]

btw, I'm still working on this. I've just caught up on some unrelated Circle CI work, which is now finished. I hope to have this ready either this weekend or the next.

[rpkilby]

ach, I'm eventually going to finish this up. It's just taken a back seat to some work on DRF and djangorestframework-filters (still trying to get 1.0 out the door... it's only been 3+ years in the making!). Also, the PR should maybe address iss 1020.

[rpkilby]

Took me a year, but it's just about ready lol.