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

Rework validation, add queryset filter method #788

Merged
merged 11 commits into from
Oct 24, 2017
1 change: 0 additions & 1 deletion django_filters/__init__.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
# flake8: noqa
import pkgutil

from .constants import STRICTNESS
from .filterset import FilterSet
from .filters import *

Expand Down
3 changes: 0 additions & 3 deletions django_filters/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
from django.core.signals import setting_changed
from django.utils.translation import ugettext_lazy as _

from .constants import STRICTNESS
from .utils import deprecate

DEFAULTS = {
Expand All @@ -13,8 +12,6 @@
'NULL_CHOICE_LABEL': None,
'NULL_CHOICE_VALUE': 'null',

'STRICTNESS': STRICTNESS.RETURN_NO_RESULTS,

'VERBOSE_LOOKUPS': {
# transforms don't need to be verbose, since their expressions are chained
'date': _('date'),
Expand Down
19 changes: 0 additions & 19 deletions django_filters/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,3 @@


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


class STRICTNESS(object):
class IGNORE(object):
pass

class RETURN_NO_RESULTS(object):
pass

class RAISE_VALIDATION_ERROR(object):
pass

# Values of False & True chosen for backward compatability reasons.
# Originally, these were the only options.
_LEGACY = {
False: IGNORE,
True: RETURN_NO_RESULTS,
"RAISE": RAISE_VALIDATION_ERROR,
}
84 changes: 45 additions & 39 deletions django_filters/filterset.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from django.db.models.fields.related import ForeignObjectRel

from .conf import settings
from .constants import ALL_FIELDS, STRICTNESS
from .constants import ALL_FIELDS
from .filters import (
BaseInFilter,
BaseRangeFilter,
Expand Down Expand Up @@ -47,8 +47,6 @@ def __init__(self, options=None):

self.filter_overrides = getattr(options, 'filter_overrides', {})

self.strict = getattr(options, 'strict', None)

self.form = getattr(options, 'form', forms.Form)


Expand Down Expand Up @@ -140,7 +138,7 @@ def get_declared_filters(cls, bases, attrs):
class BaseFilterSet(object):
FILTER_DEFAULTS = FILTER_FOR_DBFIELD_DEFAULTS

def __init__(self, data=None, queryset=None, *, request=None, prefix=None, strict=None):
def __init__(self, data=None, queryset=None, *, request=None, prefix=None):
if queryset is None:
queryset = self._meta.model._default_manager.all()
model = queryset.model
Expand All @@ -151,59 +149,67 @@ def __init__(self, data=None, queryset=None, *, request=None, prefix=None, stric
self.request = request
self.form_prefix = prefix

# What to do on on validation errors
# Fallback to meta, then settings strictness
if strict is None:
strict = self._meta.strict
if strict is None:
strict = settings.STRICTNESS

# transform legacy values
self.strict = STRICTNESS._LEGACY.get(strict, strict)

self.filters = copy.deepcopy(self.base_filters)

# propagate the model and filterset to the filters
for filter_ in self.filters.values():
filter_.model = model
filter_.parent = self

def is_valid(self):
"""
Return True if the underlying form has no errors, or False otherwise.
"""
return self.is_bound and self.form.is_valid()

@property
def errors(self):
"""
Return an ErrorDict for the data provided for the underlying form.
"""
return self.form.errors

def filter_queryset(self, queryset):
"""
Filter the queryset with the underlying form's `cleaned_data`. You must
call `is_valid()` or `errors` before calling this method.

This method should be overridden if additional filtering needs to be
applied to the queryset before it is cached.
"""
for name, value in self.form.cleaned_data.items():
queryset = self.filters[name].filter(queryset, value)
return queryset

@property
def qs(self):
if not hasattr(self, '_qs'):
if not self.is_bound:
self._qs = self.queryset.all()
return self._qs

if not self.form.is_valid():
if self.strict == STRICTNESS.RAISE_VALIDATION_ERROR:
raise forms.ValidationError(self.form.errors)
elif self.strict == STRICTNESS.RETURN_NO_RESULTS:
self._qs = self.queryset.none()
return self._qs
# else STRICTNESS.IGNORE... ignoring

# start with all the results and filter from there
qs = self.queryset.all()
for name, filter_ in self.filters.items():
value = self.form.cleaned_data.get(name)
if self.is_bound:
# ensure form validation before filtering
self.errors
qs = self.filter_queryset(qs)
self._qs = qs
return self._qs

if value is not None: # valid & clean data
qs = filter_.filter(qs, value)
def get_form_class(self):
"""
Returns a django Form suitable of validating the filterset data.

self._qs = qs
This method should be overridden if the form class needs to be
customized relative to the filterset instance.
"""
fields = OrderedDict([
(name, filter_.field)
for name, filter_ in self.filters.items()])

return self._qs
return type(str('%sForm' % self.__class__.__name__),
(self._meta.form,), fields)

@property
def form(self):
if not hasattr(self, '_form'):
fields = OrderedDict([
(name, filter_.field)
for name, filter_ in self.filters.items()])

Form = type(str('%sForm' % self.__class__.__name__),
(self._meta.form,), fields)
Form = self.get_form_class()
if self.is_bound:
self._form = Form(self.data, prefix=self.form_prefix)
else:
Expand Down
9 changes: 7 additions & 2 deletions django_filters/rest_framework/backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@
from django.template import loader

from . import filters, filterset
from .. import compat
from .. import compat, utils


class DjangoFilterBackend(object):
default_filter_set = filterset.FilterSet
raise_exception = True

@property
def template(self):
Expand Down Expand Up @@ -49,8 +50,12 @@ def filter_queryset(self, request, queryset, view):
filter_class = self.get_filter_class(view, queryset)

if filter_class:
return filter_class(request.query_params, queryset=queryset, request=request).qs
filterset = filter_class(request.query_params, queryset=queryset, request=request)
if not filterset.is_valid() and self.raise_exception:
from rest_framework.exceptions import ValidationError

raise ValidationError(utils.raw_validation(filterset.errors))
return filterset.qs
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return queryset

def to_html(self, request, queryset, view):
Expand Down
12 changes: 1 addition & 11 deletions django_filters/rest_framework/filterset.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
from copy import deepcopy

from django import forms
from django.db import models
from django.utils.translation import ugettext_lazy as _

from django_filters import filterset

from .. import compat, utils
from .. import compat
from .filters import BooleanFilter, IsoDateTimeFilter

FILTER_FOR_DBFIELD_DEFAULTS = deepcopy(filterset.FILTER_FOR_DBFIELD_DEFAULTS)
Expand Down Expand Up @@ -38,12 +37,3 @@ def form(self):
form.helper = helper

return form

@property
def qs(self):
from rest_framework.exceptions import ValidationError

try:
return super(FilterSet, self).qs
except forms.ValidationError as e:
raise ValidationError(utils.raw_validation(e))
11 changes: 10 additions & 1 deletion django_filters/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class FilterMixin(object):
"""
filterset_class = None
filter_fields = ALL_FIELDS
strict = True

def get_filterset_class(self):
"""
Expand Down Expand Up @@ -58,13 +59,21 @@ def get_filterset_kwargs(self, filterset_class):
raise ImproperlyConfigured(msg % args)
return kwargs

def get_strict(self):
return self.strict


class BaseFilterView(FilterMixin, MultipleObjectMixin, View):

def get(self, request, *args, **kwargs):
filterset_class = self.get_filterset_class()
self.filterset = self.get_filterset(filterset_class)
self.object_list = self.filterset.qs

if self.filterset.is_valid() or not self.get_strict():
self.object_list = self.filterset.qs
else:
self.object_list = self.filterset.queryset.none()

context = self.get_context_data(filter=self.filterset,
object_list=self.object_list)
return self.render_to_response(context)
Expand Down
65 changes: 16 additions & 49 deletions docs/ref/filterset.txt
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,7 @@ Meta options
- :ref:`fields <fields>`
- :ref:`exclude <exclude>`
- :ref:`form <form>`
- :ref:`together <together>`
- :ref:`filter_overrides <filter_overrides>`
- :ref:`strict <strict>`


.. _model:
Expand Down Expand Up @@ -101,26 +99,6 @@ form class from which ``FilterSet.form`` will subclass. This works similar to
the ``form`` option on a ``ModelAdmin.``


.. _together:

Group fields with ``together``
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The inner ``Meta`` class also takes an optional ``together`` argument. This
is a list of lists, each containing field names. For convenience can be a
single list/tuple when dealing with a single set of fields. Fields within a
field set must either be all or none present in the request for
``FilterSet.form`` to be valid::

import django_filters

class ProductFilter(django_filters.FilterSet):
class Meta:
model = Product
fields = ['price', 'release_date', 'rating']
together = ['rating', 'price']


.. _filter_overrides:

Customise filter generation with ``filter_overrides``
Expand Down Expand Up @@ -149,39 +127,28 @@ This is a map of model fields to filter classes with options::
},
}

Overriding ``FilterSet`` methods
--------------------------------

.. _strict:
When overriding classmethods, calling ``super(MyFilterSet, cls)`` may result
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These super calls don't look right.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

in a ``NameError`` exception. This is due to the ``FilterSetMetaclass`` calling
these classmethods before the ``FilterSet`` class has been fully created.
There are two recommmended workarounds:

Handling validation errors with ``strict``
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1. If using python 3.6 or newer, use the argumentless ``super()`` syntax.
2. For older versions of python, use an intermediate class. Ex::

The ``strict`` option determines the filterset's behavior when filters fail to validate. Example use:
class Intermediate(django_filters.FilterSet):

.. code-block:: python

from django_filters import FilterSet, STRICTNESS
@classmethod
def method(cls, arg):
super(Intermediate, cls).method(arg)
...

class ProductFilter(FilterSet):
class ProductFilter(Intermediate):
class Meta:
model = Product
fields = ['name', 'release_date']
strict = STRICTNESS.RETURN_NO_RESULTS

Currently, there are three different behaviors:

- ``STRICTNESS.RETURN_NO_RESULTS`` (default) This returns an empty queryset. The
filterset form can then be rendered to display the input errors.
- ``STRICTNESS.IGNORE`` Instead of returning an empty queryset, invalid filters
effectively become a noop. Valid filters are applied to the queryset however.
- ``STRICTNESS.RAISE_VALIDATION_ERROR`` This raises a ``ValidationError`` for
all invalid filters. This behavior is generally useful with APIs.

If the ``strict`` option is not provided, then the filterset will default to the
value of the ``FILTERS_STRICTNESS`` setting.


Overriding ``FilterSet`` methods
--------------------------------
fields = ['...']

``filter_for_lookup()``
~~~~~~~~~~~~~~~~~~~~~~~
Expand Down Expand Up @@ -212,4 +179,4 @@ filters for a model field, you can override ``filter_for_lookup()``. Ex::
return django_filters.DateRangeFilter, {}

# use default behavior otherwise
return super(ProductFilter, cls).filter_for_lookup(f, lookup_type)
return super().filter_for_lookup(f, lookup_type)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one does though.

3 changes: 1 addition & 2 deletions tests/rest_framework/test_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from rest_framework import generics, serializers, status
from rest_framework.test import APIRequestFactory

from django_filters import STRICTNESS, filters
from django_filters import filters
from django_filters.rest_framework import DjangoFilterBackend, FilterSet

from .models import (
Expand Down Expand Up @@ -287,7 +287,6 @@ def test_html_rendering(self):
response = view(request).render()
self.assertEqual(response.status_code, status.HTTP_200_OK)

@override_settings(FILTERS_STRICTNESS=STRICTNESS.RAISE_VALIDATION_ERROR)
def test_strictness_validation_error(self):
"""
Ensure validation errors return a proper error response instead of
Expand Down
4 changes: 0 additions & 4 deletions tests/settings.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@

# ensure package/conf is importable
from django_filters import STRICTNESS
from django_filters.conf import DEFAULTS

DATABASES = {
Expand Down Expand Up @@ -40,6 +39,3 @@
# help verify that DEFAULTS is importable from conf.
def FILTERS_VERBOSE_LOOKUPS():
return DEFAULTS['VERBOSE_LOOKUPS']


FILTERS_STRICTNESS = STRICTNESS.RETURN_NO_RESULTS