Rework validation, add queryset filter method

[rpkilby]

This is an evolution of #783. In short, I think the validation/strictness behavior can be improved by moving it out of the FilterSet and into the view code. The current .qs implementation is somewhat difficult to extend, and is a little confusing given that it performs both form validation and queryset filtering.

Overall changes:

• Expands the FilterSet API to include the following:
  • .is_valid() - proxies form.is_valid()
  • .errors - proxies form.errors
  • .filter_queryset(qs) - allows users to easily override queryset handling. Cached by .qs
  • .get_form_class() - easier overriding of the form class on a per-instance basis, cached by .form
• FilterSet-level strictness handling is removed in favor of shifting it to the view layer
  • Generic View has a strict/non-strict attribute (previously, RETURN_NO_RESULTS/IGNORE)
  • DjangoFilterBackend has a raise/non-raise attribute (previously, RAISE_VALIDATION_ERROR/IGNORE)
• Reworked raw_validation. Error details now include codes. However, these codes are only reachable via exc.get_full_details(), and do not automatically show up in the rendered error response.
• Other small testing/docs changes

Additional thoughts:

• Adds a third option for strict to raise a Validation Error #136 is definitely targeting API use. Raising a forms.ValidationError in the context of a Django view does not make any sense, especially given that none of the provided view code actually handles this exception. Raising an exception has been moved to the backend, where it uses the same utility code to raise the filterset.errors.
• The 'no results'/'ignore' handling is capable of being handled at the view layer, dependent on a FilterMixin.strict option.
• Given that views are easily customizable, the FILTERS_STRICTNESS setting is no longer necessary.
• This slightly changes the Meta.together handling by
  • Allowing multiple 'together' set errors to be displayed (instead of just one).
  • Removes the fields from the form.cleaned_data

TODO:

• Determine how much deprecation warning needs to occur (if at all) no deprecation for strictness
• Add tests for FilterBackend/FilterMixin validation behavior
• Deprecate strictness in docs (remove relevant docs, add migration)
• Document new public FilterSet API postponing to docs overhaul

---

Additionally, this resolves #740 by extending #738. Inlining the original PR comment:

I was thinking about #740 a while back, and realized that overriding the FilterSet.qs property is slightly awkward, due to the _qs caching. In the case of #740, Prefetch calls can only occur once, as duplicate calls can raise an exception. If you want to cache the prefetch, a correct solution looks like the following:

@property
def qs(self):
    # ensure that we haven't already cached, as this also implies prefetching
    already_prefetched = hasattr(self, '_qs')
    super().qs
    if self.is_bound and not already_prefetched:
        self._qs = self._qs.prefetch_related(...)
    return self._qs

Overriding filter_queyset() is a little more intuitive, as it would only be executed once.

def filter_queryset(self, queryset):
    qs = super().filter_queryset(queryset)
    return qs.prefetch_related(...)

[codecov-io]

Codecov Report

Merging #788 into develop will increase coverage by 0.04%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop     #788      +/-   ##
===========================================
+ Coverage    97.81%   97.86%   +0.04%     
===========================================
  Files           15       15              
  Lines         1145     1123      -22     
===========================================
- Hits          1120     1099      -21     
+ Misses          25       24       -1

Impacted Files	Coverage Δ
django_filters/__init__.py	94.11% <ø> (-0.33%)	⬇️
django_filters/constants.py	100% <ø> (ø)	⬆️
django_filters/conf.py	97.05% <ø> (-0.09%)	⬇️
django_filters/filterset.py	100% <100%> (ø)	⬆️
django_filters/rest_framework/filterset.py	100% <100%> (ø)	⬆️
django_filters/rest_framework/backends.py	94.82% <100%> (+0.38%)	⬆️
django_filters/utils.py	99.02% <100%> (+0.83%)	⬆️
django_filters/views.py	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02a128f...b9f9d27. Read the comment docs.

[rpkilby]

@carltongibson, could you give this a brief look before 1.1? I'd like to add a deprecation note if the overall gist seems reasonable

[rpkilby]

In short, the .qs property is broken into FilterSet.is_valid()/FilterSet.errors and FilterSet.filter_queryset(queryset).

The only API-level change is that the STRICTNESS option is deprecated.

• The 'raise' behavior is moved to the DRF DjangoFilterBackend.
• The Ignore/Return No Results is moved to the view class.

[carltongibson]

Yes. In general I like this.

• Making filter_queryset the go-to place to override is a win I think.
• Happy with the thinking on STRICTNESS.

[rpkilby]

Sweet. the only change necessary for 1.1 would be a deprecation note for the FILTERS_STRICTNESS setting, which would be moved to the view and backend classes.

[carltongibson]

@rpkilby OK If you add that, we'll call 1.1 a wrap.

Thanks!

[carltongibson]

OK. This is great.

(I'll pick up the docs updates later on. It's not pressing.)

[carltongibson]

These super calls don't look right.

[rpkilby]

What doesn't look right here? The example is intended to show how you would accidentally generate a NameError.

[carltongibson]

This one does though.

[sshishov]

Hello guys, as documentation is not ready yet, I would like to clarify how properly to override strictness for filterset_fields if I use default filterset_class. Currently I got 400 response but I want it to be empty list and 200. Best regards, Sergey

[sshishov]

Also I noticed that even if I do not want to raise exception here, then it is not raised, but queryset returned without filtering. I guess in case if invalid filterset and do not raise exception it should return qs.none()

[blueyed]

It keeps the partial queryset, yet - I think this is tested for in https://github.com/carltongibson/django-filter/pull/788/files#diff-b18add7202661e9eacece0aded8e619bR301.

[carltongibson]

Override filter_queryset and do something like this:

if not filterset.is_valid():
    return filterset.qs.none()
return super().filter_queryset(request, queryset, view)

[sshishov]

Thanks @carltongibson , I just thought that it would be nice to have some kinda setting or one line setup. As I have to create and override backend and setting. It would be nice to have one line configuration and use standard classes

[rpkilby]

The only thing you'd need to override is the backend's filter_queryset method. e.g.,

class StrictDjangoFilterBackend(django_filters.rest_framework.DjangoFilterBackend):
    """Return no results if the query doesn't validate."""

    def filter_queryset(self, request, queryset, view):
        try:
            return super().filter_queryset(request, queryset, view):
        except serializers.ValidationError:
            return queryset.none()

[anveshagarwal]

hey. I just want to know where can i find docs to properly comply strictness in view level now?
i have a test in which if the parameter is invalid i.e filterset is not valid, it always throws error which is very much clear from this https://github.com/carltongibson/django-filter/blob/master/django_filters/rest_framework/backends.py#L94 as raise_exception is always true. How can i simply disable it. And also when the filterset is not valid, it returns the whole queryset, but like previous, i want to return an empty queryset in that case.
And i am using it with drf

[rpkilby]

Unfortunately, it's not really documented.

How can i simply disable it.

You can subclass the backend and set raise_exception = False.

And also when the filterset is not valid, it returns the whole queryset, but like previous, i want to return an empty queryset in that case.

In that case, you'd also need to override the filter_queryset method to return qs.none().

[anveshagarwal]

@rpkilby This helped a lot!

[anveshagarwal]

You can subclass the backend and set raise_exception = False.

Can i set raise_exception = False from a viewset in anyway?

[rpkilby]

Not currently. Although, you could create a backend that does pull raise_exception from the view.

[MRigal]

Hi @carltongibson
Although a bit late to the party, I would like to question one point:

Previously we had a test on the filters if value is not None https://github.com/carltongibson/django-filter/pull/788/files#diff-c82ea95d2a317d98860bf154f27d3e17L191
Now we have (always) set_filters https://github.com/carltongibson/django-filter/pull/788/files#diff-c82ea95d2a317d98860bf154f27d3e17R181

In particular, this is in contradiction with the docs, which states that

Filtering by an empty string

It’s not currently possible to filter by an empty string, since empty values are interpreted as a skipped filter.

GET http://localhost/api/my-model?myfield=

Is it really intended behaviour? We developed our API knowing empty filters would be handled this way. For sure, we can Subclass the class, define our own filter_queryset and adapt the imports, but this is quite dirty. We also don't want to adjust our all our clean Methods or touch deeper in the DRF handling of views.

I think our use-case is a pretty common one and is not handled properly by this change. Also I think it is in contradiction with the docs.

Thanks for your work, for the hopefully upcoming answer and I'll be glad to help if you wish so

[rpkilby]

Hi @MRigal.

Previously we had a test on the filters if value is not None

Yes - this did not allow None values to pass through, but ?myfield= would be parsed as "", not None. So, removing this check should have had no effect. It's actually the Filter.filter() method that filters out empty strings (and other "empty" values).

django-filter/django_filters/filters.py

Lines 139 to 141 in 9305753

	def filter(self, qs, value):
	if value in EMPTY_VALUES:
	return qs

Worth noting that the value is not None check was partially redundant, as None is included in the EMPTY_VALUES and would have been filtered out anyway.

django-filter/django_filters/constants.py

Line 5 in 9305753

EMPTY_VALUES = ([], (), {}, '', None)

That said, a filter subclass that overwrites filter, or a given method, would need to know to filter empty values. I don't think we've documented this.

In particular, this is in contradiction with the docs, which states that

The docs are a little wonky here. It's not that it's not possible, it's that you wouldn't want to, since empty strings cannot be represented as bare values in a query string, as they are identical to empty form inputs.

We also don't want to adjust our all our clean Methods or touch deeper in the DRF handling of views.

I'm not completely following what you're doing here, but in general:

• builtin filters should ignore empty strings
• custom filters that overwrite filter() would need to check against EMPTY_VALUES.
• filter methods would also need to check against EMPTY_VALUES. This was incorrect. FilterMethod.__call__ does check against EMPTY_VALUES, so the actual method does not also need to check this.

[MRigal]

Hi @rpkilby

Thanks for this quick and very well written answer. Our problem was actually smaller than I thought. We were actually calling the filter in super() which would have indeed make the check against EMPTY_VALUES but the problem happened before where we were handling the value.

It was working before due to the double-check on the set, but the problem was on our Filter classes, as the work done there should have been paying more attention on which data comes in.

Django-filters is actually working as expected, also not in contradiction with the docs and it is much better with this rewrite. Sorry for the mess and keep up with the good job!

[rpkilby]

No worries, and thanks! And the docs may not be entirely incorrect, but they could be more clear as to what's going on. Also, we should probably mention checking against EMPTY_VALUES when providing a filter method.

[rpkilby]

Brief update - I was actually wrong about methods. The FilterMethod class actually does check against EMPTY_VALUES, as seen here:

django-filter/django_filters/filters.py

Lines 757 to 761 in 1880430

	def __call__(self, qs, value):
	if value in EMPTY_VALUES:
	return qs
	return self.method(qs, self.f.field_name, value)

So, the only issue would be custom Filters implementing/overriding the filter() method.