Fix AutoFilter accidentally munging declared filter names (philipn#304)

Author: rpkilby

rest_framework_filters/filters.py

@@ -2,35 +2,62 @@
 
 from django.utils.module_loading import import_string
 from django_filters.rest_framework.filters import *  # noqa
-from django_filters.rest_framework.filters import Filter, ModelChoiceFilter
+from django_filters.rest_framework.filters import (
+    ModelChoiceFilter, ModelMultipleChoiceFilter,
+)
 
 ALL_LOOKUPS = '__all__'
 
 
-class AutoFilter(Filter):
-    """
-    Declarative alternative to using the `Meta.fields` dictionary syntax. These
-    fields are processed by the metaclass and resolved into per-lookup filters.
+class AutoFilter:
+    """A placeholder class that enables generating multiple per-lookup filters.
+
+    This is a declarative alternative to the ``Meta.fields`` dict syntax, and
+    the below are functionally equivalent:
+
+        class PersonFilter(filters.FilterSet):
+            name = AutoFilter(lookups=['exact', 'contains'])
+
+            class Meta:
+                model = Person
+                fields = []
+
+        class PersonFilter(filters.FilterSet):
+            class Meta:
+                model = Person
+                fields = {'name': ['exact', 'contains']}
+
+    Due to its declarative nature, an ``AutoFilter`` allows for paramater name
+    aliasing for its generated filters. e.g.,
+
+        class BlogFilter(filters.FilterSet):
+            title = AutoFilter(field_name='name', lookups=['contains'])
 
-    `AutoFilter`s benefit from their declarative nature in that it is possible
-    to change the parameter name of the generated filters. This is not possible
-    with the `Meta.fields` syntax.
+    The above generates a ``title__contains`` filter for the ``name`` field
+    intstead of ``name__contains``. This is not possible with ``Meta.fields``,
+    since the key must match the model field name.
+
+    Note that ``AutoFilter`` is a filter-like placeholder and is not present in
+    the ``FilterSet.filters``. In the above example, ``title`` would not be
+    filterable. However, an ``AutoFilter`` is typically replaced by a generated
+    ``exact`` filter of the same name, which enables filtering by that param.
     """
+    creation_counter = 0
 
-    def __init__(self, *args, lookups=None, **kwargs):
-        super().__init__(*args, **kwargs)
+    def __init__(self, field_name=None, *, lookups=None):
+        self.field_name = field_name
         self.lookups = lookups or []
 
+        self.creation_counter = AutoFilter.creation_counter
+        AutoFilter.creation_counter += 1
 
-class RelatedFilter(AutoFilter, ModelChoiceFilter):
-    """
-    A `ModelChoiceFilter` that defines a relationship to another `FilterSet`.
-    This related filterset is processed by the filter's `parent` instance, and
-    """
 
-    def __init__(self, filterset, *args, **kwargs):
+class BaseRelatedFilter:
+
+    def __init__(self, filterset, *args, lookups=None, **kwargs):
         super().__init__(*args, **kwargs)
         self.filterset = filterset
+        self.lookups = lookups or []
 
     def bind(self, bind_cls):
         """
@@ -63,13 +90,56 @@ def fset(self, value):
     filterset = property(**filterset())
 
     def get_queryset(self, request):
-        queryset = super(RelatedFilter, self).get_queryset(request)
+        queryset = super(BaseRelatedFilter, self).get_queryset(request)
         assert queryset is not None, \
-            "Expected `.get_queryset()` for related filter '%s.%s' to return a `QuerySet`, but got `None`." \
+            "Expected `.get_queryset()` for related filter '%s.%s' to " \
+            "return a `QuerySet`, but got `None`." \
             % (self.parent.__class__.__name__, self.field_name)
         return queryset
 
 
+class RelatedFilter(BaseRelatedFilter, ModelChoiceFilter):
+    """A ``ModelChoiceFilter`` that enables filtering across relationships.
+
+    Take the following example:
+
+        class ManagerFilter(filters.FilterSet):
+            class Meta:
+                model = Manager
+                fields = {'name': ['exact', 'in', 'startswith']}
+
+        class DepartmentFilter(filters.FilterSet):
+            manager = RelatedFilter(ManagerFilter, queryset=managers)
+
+            class Meta:
+                model = Department
+                fields = {'name': ['exact', 'in', 'startswith']}
+
+    In the above, the ``DepartmentFilter`` can traverse the ``manager``
+    relationship with the ``__`` lookup seperator, accessing the filters of the
+    ``ManagerFilter`` class. For example, the above would enable calls like:
+
+        /api/managers?name=john%20doe
+        /api/departments?manager__name=john%20doe
+
+    Related filters function similarly to auto filters in that they can generate
+    per-lookup filters. However, unlike auto filters, related filters are
+    functional and not just placeholders. They will not be replaced by a
+    generated ``exact`` filter.
+
+    Attributes:
+        filterset: The ``FilterSet`` that is traversed by this relationship.
+            May be a class, an absolute import path, or the name of a class
+            located in the same module as the origin filterset.
+        lookups: A list of lookups to generate per-lookup filters for. This
+            functions similarly to the ``AutoFilter.lookups`` argument.
+    """
+
+
+class RelatedMultipleFilter(BaseRelatedFilter, ModelMultipleChoiceFilter):
+    """A ``ModelMultipleChoiceFilter`` variant of ``RelatedFilter``."""
+
+
 class AllLookupsFilter(AutoFilter):
     def __init__(self, *args, **kwargs):
         super().__init__(*args, lookups=ALL_LOOKUPS, **kwargs)

rest_framework_filters/filterset.py

@@ -20,58 +20,99 @@ def related(filterset, filter_name):
 
 class FilterSetMetaclass(filterset.FilterSetMetaclass):
     def __new__(cls, name, bases, attrs):
-        new_class = super(FilterSetMetaclass, cls).__new__(cls, name, bases, attrs)
+        attrs['auto_filters'] = cls.get_auto_filters(bases, attrs)
 
-        new_class.auto_filters = [
-            name for name, f in new_class.declared_filters.items()
-            if isinstance(f, filters.AutoFilter)]
-        new_class.related_filters = [
-            name for name, f in new_class.declared_filters.items()
-            if isinstance(f, filters.RelatedFilter)]
+        new_class = super().__new__(cls, name, bases, attrs)
 
-        # see: :meth:`rest_framework_filters.filters.RelatedFilter.bind`
-        for name in new_class.related_filters:
-            new_class.declared_filters[name].bind(new_class)
+        new_class.related_filters = OrderedDict([
+            (name, f) for name, f in new_class.declared_filters.items()
+            if isinstance(f, filters.BaseRelatedFilter)])
 
-        # If model is defined, process auto filters
+        # See: :meth:`rest_framework_filters.filters.RelatedFilter.bind`
+        for name, f in new_class.related_filters.items():
+            f.bind(new_class)
+
+        # Only expand when model is defined. Model may be undefined for mixins.
         if new_class._meta.model is not None:
-            cls.expand_auto_filters(new_class)
+            for name, f in new_class.auto_filters.items():
+                expanded = cls.expand_auto_filter(new_class, name, f)
+                new_class.base_filters.update(expanded)
+
+            for name, f in new_class.related_filters.items():
+                expanded = cls.expand_auto_filter(new_class, name, f)
+                new_class.base_filters.update(expanded)
 
         return new_class
 
     @classmethod
-    def expand_auto_filters(cls, new_class):
-        """
-        Resolve `AutoFilter`s into their per-lookup filters. `AutoFilter`s are
-        a declarative alternative to the `Meta.fields` dictionary syntax, and
-        use the same machinery internally.
+    def get_auto_filters(cls, bases, attrs):
+        # Auto filters are specially handled since they aren't an actual filter
+        # subclass and aren't handled by the declared filter machinery. Note
+        # that this is a nearly identical copy of `get_declared_filters`.
+        auto_filters = [
+            (filter_name, attrs.pop(filter_name))
+            for filter_name, obj in list(attrs.items())
+            if isinstance(obj, filters.AutoFilter)
+        ]
+
+        # Default the `filter.field_name` to the attribute name on the filterset
+        for filter_name, f in auto_filters:
+            if getattr(f, 'field_name', None) is None:
+                f.field_name = filter_name
+
+        auto_filters.sort(key=lambda x: x[1].creation_counter)
+
+        # merge auto filters from base classes
+        for base in reversed(bases):
+            if hasattr(base, 'auto_filters'):
+                auto_filters = [
+                    (name, f) for name, f
+                    in base.auto_filters.items()
+                    if name not in attrs
+                ] + auto_filters
+
+        return OrderedDict(auto_filters)
+
+    @classmethod
+    def expand_auto_filter(cls, new_class, filter_name, f):
+        """Resolve an ``AutoFilter`` into its per-lookup filters.
+
+        This method name is slightly inaccurate since it handles both
+        :class:`rest_framework_filters.filters.AutoFilter` and
+        :class:`rest_framework_filters.filters.BaseRelatedFilter`, as well as
+        their subclasses, which all support per-lookup filter generation.
+
+        Args:
+            new_class: The ``FilterSet`` class to generate filters for.
+            filter_name: The attribute name of the filter on the ``FilterSet``.
+            f: The filter instance.
+
+        Returns:
+            A named map of generated filter objects.
         """
-        # get reference to opts/declared filters
-        orig_meta, orig_declared = new_class._meta, new_class.declared_filters
+        expanded = OrderedDict()
 
-        # override opts/declared filters w/ copies
+        # get reference to opts/declared filters so originals aren't modified
+        orig_meta, orig_declared = new_class._meta, new_class.declared_filters
         new_class._meta = copy.deepcopy(new_class._meta)
-        new_class.declared_filters = new_class.declared_filters.copy()
-
-        for name in new_class.auto_filters:
-            f = new_class.declared_filters[name]
+        new_class.declared_filters = {}
 
-            # Remove auto filters from declared_filters so that they *are* overwritten
-            # RelatedFilter is an exception, and should *not* be overwritten
-            if not isinstance(f, filters.RelatedFilter):
-                del new_class.declared_filters[name]
+        # Use meta.fields to generate auto filters
+        new_class._meta.fields = {f.field_name: f.lookups or []}
+        for gen_name, gen_f in new_class.get_filters().items():
+            # get_filters() generates param names from the model field name, so
+            # replace the field name with the param name from the filerset
+            gen_name = gen_name.replace(f.field_name, filter_name, 1)
 
-            # Use meta.fields to generate auto filters
-            new_class._meta.fields = {f.field_name: f.lookups or []}
-            for gen_name, gen_f in new_class.get_filters().items():
-                # get_filters() generates param names from the model field name
-                # Replace the field name with the parameter name from the filerset
-                gen_name = gen_name.replace(f.field_name, name, 1)
-                new_class.base_filters[gen_name] = gen_f
+            # do not overwrite declared filters
+            if gen_name not in orig_declared:
+                expanded[gen_name] = gen_f
 
         # restore reference to opts/declared filters
         new_class._meta, new_class.declared_filters = orig_meta, orig_declared
 
+        return expanded
+
 
 class SubsetDisabledMixin:
     """
@@ -95,6 +136,7 @@ def __init__(self, data=None, queryset=None, *, relationship=None, **kwargs):
 
     @classmethod
     def get_fields(cls):
+        # Extend the 'Meta.fields' dict syntax to allow '__all__' field lookups.
         fields = super(FilterSet, cls).get_fields()
 
         for name, lookups in fields.items():

tests/perf/README.md

@@ -13,6 +13,10 @@ relationships via `RelatedFilter`. The intent is to provide some assurance that:
 The performance tests have been isolated from the main test suite so that they can be
 ran independently. Simply run:
 
+    $ tox -e performance
+
+Or more directly:
+
     $ python manage.py test tests.perf
 
 

tests/test_filtering.py

@@ -63,10 +63,19 @@ class Meta:
                 model = Note
                 fields = []
 
+        class Subclass(Actual):
+            class Meta:
+                model = Note
+                fields = []
+
         GET = {'title__contains': 'Hello'}
         f = Actual(GET, queryset=Note.objects.all())
         self.assertEqual(len(list(f.qs)), 2)
 
+        GET = {'title__contains': 'Hello'}
+        f = Subclass(GET, queryset=Note.objects.all())
+        self.assertEqual(len(list(f.qs)), 2)
+
 
 class RelatedFilterTests(TestCase):
 
@@ -305,7 +314,7 @@ def test_nonexistent_related_field(self):
 
     def test_related_filters_inheritance(self):
         class ChildFilter(PostFilter):
-            foo = filters.RelatedFilter(PostFilter)
+            foo = filters.RelatedFilter(NoteFilter, field_name='note')
 
         self.assertEqual(['author', 'note', 'tags'], list(PostFilter.related_filters))
         self.assertEqual(['author', 'note', 'tags', 'foo'], list(ChildFilter.related_filters))