Merge pull request carltongibson#550 from rpkilby/rework-filter-generation

Rework filter generation

Author: Carlton Gibson

django_filters/filterset.py

@@ -10,7 +10,6 @@
 from django.db.models.constants import LOOKUP_SEP
 from django.db.models.fields.related import ForeignObjectRel
 from django.utils import six
-from django.utils.translation import ugettext as _
 
 from .conf import settings
 from .compat import remote_field, remote_queryset
@@ -21,75 +20,20 @@
 from .utils import try_dbfield, get_all_model_fields, get_model_field, resolve_field
 
 
-def get_declared_filters(bases, attrs, with_base_filters=True):
-    filters = []
-    for filter_name, obj in list(attrs.items()):
-        if isinstance(obj, Filter):
-            obj = attrs.pop(filter_name)
-            if getattr(obj, 'name', None) is None:
-                obj.name = filter_name
-            filters.append((filter_name, obj))
-    filters.sort(key=lambda x: x[1].creation_counter)
-
-    if with_base_filters:
-        for base in bases[::-1]:
-            if hasattr(base, 'base_filters'):
-                filters = list(base.base_filters.items()) + filters
-    else:
-        for base in bases[::-1]:
-            if hasattr(base, 'declared_filters'):
-                filters = list(base.declared_filters.items()) + filters
+def get_filter_name(field_name, lookup_expr):
+    """
+    Combine a field name and lookup expression into a usable filter name.
+    Exact lookups are the implicit default, so "exact" is stripped from the
+    end of the filter name.
+    """
+    filter_name = LOOKUP_SEP.join([field_name, lookup_expr])
 
-    return OrderedDict(filters)
-
-
-def filters_for_model(model, fields=None, exclude=None, filter_for_field=None,
-                      filter_for_reverse_field=None):
-    field_dict = OrderedDict()
-
-    # Setting exclude with no fields implies all other fields.
-    if exclude is not None and fields is None:
-        fields = ALL_FIELDS
-
-    # All implies all db fields associated with a filter_class.
-    if fields == ALL_FIELDS:
-        fields = get_all_model_fields(model)
-
-    # Loop through the list of fields.
-    for f in fields:
-        # Skip the field if excluded.
-        if exclude is not None and f in exclude:
-            continue
-        field = get_model_field(model, f)
-        # Do nothing if the field doesn't exist.
-        if field is None:
-            field_dict[f] = None
-            continue
-        if isinstance(field, ForeignObjectRel):
-            filter_ = filter_for_reverse_field(field, f)
-            if filter_:
-                field_dict[f] = filter_
-        # If fields is a dictionary, it must contain lists.
-        elif isinstance(fields, dict):
-            # Create a filter for each lookup type.
-            for lookup_expr in fields[f]:
-                filter_ = filter_for_field(field, f, lookup_expr)
-
-                if filter_:
-                    filter_name = LOOKUP_SEP.join([f, lookup_expr])
-
-                    # Don't add "exact" to filter names
-                    _exact = LOOKUP_SEP + 'exact'
-                    if filter_name.endswith(_exact):
-                        filter_name = filter_name[:-len(_exact)]
-
-                    field_dict[filter_name] = filter_
-        # If fields is a list, it contains strings.
-        else:
-            filter_ = filter_for_field(field, f)
-            if filter_:
-                field_dict[f] = filter_
-    return field_dict
+    # This also works with transformed exact lookups, such as 'date__exact'
+    _exact = LOOKUP_SEP + 'exact'
+    if filter_name.endswith(_exact):
+        filter_name = filter_name[:-len(_exact)]
+
+    return filter_name
 
 
 def get_full_clean_override(together):
@@ -134,36 +78,36 @@ def __init__(self, options=None):
 
 class FilterSetMetaclass(type):
     def __new__(cls, name, bases, attrs):
-        try:
-            parents = [b for b in bases if issubclass(b, FilterSet)]
-        except NameError:
-            # We are defining FilterSet itself here
-            parents = None
-        declared_filters = get_declared_filters(bases, attrs, False)
-        new_class = super(
-            FilterSetMetaclass, cls).__new__(cls, name, bases, attrs)
-
-        if not parents:
-            return new_class
-
-        opts = new_class._meta = FilterSetOptions(
-            getattr(new_class, 'Meta', None))
-
-        if opts.model and (opts.fields is not None or opts.exclude is not None):
-            filters = new_class.filters_for_model(opts.model, opts)
-            filters.update(declared_filters)
-        else:
-            filters = declared_filters
+        attrs['declared_filters'] = cls.get_declared_filters(bases, attrs)
 
-        not_defined = next((k for k, v in filters.items() if v is None), False)
-        if not_defined:
-            raise TypeError("Meta.fields contains a field that isn't defined "
-                            "on this FilterSet: {}".format(not_defined))
+        new_class = super(FilterSetMetaclass, cls).__new__(cls, name, bases, attrs)
+        new_class._meta = FilterSetOptions(getattr(new_class, 'Meta', None))
+        new_class.base_filters = new_class.get_filters()
 
-        new_class.declared_filters = declared_filters
-        new_class.base_filters = filters
         return new_class
 
+    @classmethod
+    def get_declared_filters(cls, bases, attrs):
+        filters = [
+            (filter_name, attrs.pop(filter_name))
+            for filter_name, obj in list(attrs.items())
+            if isinstance(obj, Filter)
+        ]
+
+        # Default the `filter.name` to the attribute name on the filterset
+        for filter_name, f in filters:
+            if getattr(f, 'name', None) is None:
+                f.name = filter_name
+
+        filters.sort(key=lambda x: x[1].creation_counter)
+
+        # merge declared filters from base classes
+        for base in reversed(bases):
+            if hasattr(base, 'declared_filters'):
+                filters = list(base.declared_filters.items()) + filters
+
+        return OrderedDict(filters)
+
 
 FILTER_FOR_DBFIELD_DEFAULTS = {
     models.AutoField:                   {'filter_class': NumberFilter},
@@ -235,12 +179,10 @@ def __init__(self, data=None, queryset=None, prefix=None, strict=None, request=N
         self.request = request
 
         self.filters = copy.deepcopy(self.base_filters)
-        # propagate the model being used through the filters
+
         for filter_ in self.filters.values():
+            # propagate the model and filterset to the filters
             filter_.model = self._meta.model
-
-        # Apply the parent to the filters, this will allow the filters to access the filterset
-        for filter_key, filter_ in six.iteritems(self.filters):
             filter_.parent = self
 
     @property
@@ -288,12 +230,88 @@ def form(self):
         return self._form
 
     @classmethod
-    def filters_for_model(cls, model, opts):
-        return filters_for_model(
-            model, opts.fields, opts.exclude,
-            cls.filter_for_field,
-            cls.filter_for_reverse_field
-        )
+    def get_fields(cls):
+        """
+        Resolve the 'fields' argument that should be used for generating filters on the
+        filterset. This is 'Meta.fields' sans the fields in 'Meta.exclude'.
+        """
+        model = cls._meta.model
+        fields = cls._meta.fields
+        exclude = cls._meta.exclude
+
+        assert not (fields is None and exclude is None), \
+            "Setting 'Meta.model' without either 'Meta.fields' or 'Meta.exclude' " \
+            "has been deprecated since 0.15.0 and is now disallowed. Add an explicit" \
+            "'Meta.fields' or 'Meta.exclude' to the %s class." % cls.__name__
+
+        # Setting exclude with no fields implies all other fields.
+        if exclude is not None and fields is None:
+            fields = ALL_FIELDS
+
+        # Resolve ALL_FIELDS into all fields for the filterset's model.
+        if fields == ALL_FIELDS:
+            fields = get_all_model_fields(model)
+
+        # Remove excluded fields
+        exclude = exclude or []
+        if not isinstance(fields, dict):
+            fields = [(f, ['exact']) for f in fields if f not in exclude]
+        else:
+            fields = [(f, lookups) for f, lookups in fields.items() if f not in exclude]
+
+        return OrderedDict(fields)
+
+    @classmethod
+    def get_filters(cls):
+        """
+        Get all filters for the filterset. This is the combination of declared and
+        generated filters.
+        """
+
+        # No model specified - skip filter generation
+        if not cls._meta.model:
+            return cls.declared_filters.copy()
+
+        # Determine the filters that should be included on the filterset.
+        filters = OrderedDict()
+        fields = cls.get_fields()
+        undefined = []
+
+        for field_name, lookups in fields.items():
+            field = get_model_field(cls._meta.model, field_name)
+
+            # warn if the field doesn't exist.
+            if field is None:
+                undefined.append(field_name)
+
+            # ForeignObjectRel does not support non-exact lookups
+            if isinstance(field, ForeignObjectRel):
+                filters[field_name] = cls.filter_for_reverse_field(field, field_name)
+                continue
+
+            for lookup_expr in lookups:
+                filter_name = get_filter_name(field_name, lookup_expr)
+
+                # If the filter is explicitly declared on the class, skip generation
+                if filter_name in cls.declared_filters:
+                    filters[filter_name] = cls.declared_filters[filter_name]
+                    continue
+
+                if field is not None:
+                    filters[filter_name] = cls.filter_for_field(field, field_name, lookup_expr)
+
+        # filter out declared filters
+        undefined = [f for f in undefined if f not in cls.declared_filters]
+        if undefined:
+            raise TypeError(
+                "'Meta.fields' contains fields that are not defined on this FilterSet: "
+                "%s" % ', '.join(undefined)
+            )
+
+        # Add in declared filters. This is necessary since we don't enforce adding
+        # declared filters to the 'Meta.fields' option
+        filters.update(cls.declared_filters)
+        return filters
 
     @classmethod
     def filter_for_field(cls, f, name, lookup_expr='exact'):

tests/test_conf.py

@@ -39,6 +39,7 @@ class StrictnessTests(TestCase):
     class F(FilterSet):
         class Meta:
             model = User
+            fields = []
 
     def test_settings_default(self):
         self.assertEqual(self.F().strict, STRICTNESS.RETURN_NO_RESULTS)

tests/test_filtering.py

@@ -209,6 +209,7 @@ class F(FilterSet):
 
             class Meta:
                 model = Article
+                fields = ['author']
 
         # sanity check to make sure the filter is setup correctly
         f = F({'author': '1'})

tests/test_filterset.py

@@ -322,20 +322,21 @@ class Meta:
                              ['title', 'price', 'average_rating'])
 
     def test_model_no_fields_or_exclude(self):
-        class F(FilterSet):
-            class Meta:
-                model = Book
+        with self.assertRaises(AssertionError) as excinfo:
+            class F(FilterSet):
+                class Meta:
+                    model = Book
 
-        self.assertEqual(len(F.declared_filters), 0)
-        self.assertEqual(len(F.base_filters), 0)
-        self.assertListEqual(list(F.base_filters), [])
+        self.assertIn(
+            "Setting 'Meta.model' without either 'Meta.fields' or 'Meta.exclude'",
+            str(excinfo.exception)
+        )
 
-    def test_model_exclude_is_none(self):
-        # equivalent to unset fields/exclude
+    def test_model_fields_empty(self):
         class F(FilterSet):
             class Meta:
                 model = Book
-                exclude = None
+                fields = []
 
         self.assertEqual(len(F.declared_filters), 0)
         self.assertEqual(len(F.base_filters), 0)
@@ -427,9 +428,12 @@ class F(FilterSet):
                 class Meta:
                     model = Book
                     fields = ('username', 'price', 'other', 'another')
-        self.assertEqual(excinfo.exception.args, (
-            "Meta.fields contains a field that isn't defined "
-            "on this FilterSet: other",))
+
+        self.assertEqual(
+            str(excinfo.exception),
+            "'Meta.fields' contains fields that are not defined on this FilterSet: "
+            "other, another"
+        )
 
     def test_meta_fields_dictionary_containing_unknown(self):
         with self.assertRaises(TypeError):
@@ -528,6 +532,16 @@ class Meta:
 
         self.assertEqual(list(F.base_filters.keys()), ['ip', 'mask'])
 
+    def test_custom_declared_field_no_warning(self):
+        class F(FilterSet):
+            mask = CharFilter()
+
+            class Meta:
+                model = NetworkSetting
+                fields = ['mask']
+
+        self.assertEqual(list(F.base_filters.keys()), ['mask'])
+
     def test_filterset_for_proxy_model(self):
         class F(FilterSet):
             class Meta:
@@ -612,6 +626,7 @@ def test_settings_default(self):
         class F(FilterSet):
             class Meta:
                 model = User
+                fields = []
 
         # Ensure default is not IGNORE
         self.assertEqual(F().strict, STRICTNESS.RETURN_NO_RESULTS)
@@ -624,6 +639,7 @@ def test_meta_value(self):
         class F(FilterSet):
             class Meta:
                 model = User
+                fields = []
                 strict = STRICTNESS.IGNORE
 
         self.assertEqual(F().strict, STRICTNESS.IGNORE)
@@ -632,6 +648,7 @@ def test_init_default(self):
         class F(FilterSet):
             class Meta:
                 model = User
+                fields = []
                 strict = STRICTNESS.IGNORE
 
         strict = STRICTNESS.RAISE_VALIDATION_ERROR
@@ -641,6 +658,7 @@ def test_legacy_value(self):
         class F(FilterSet):
             class Meta:
                 model = User
+                fields = []
 
         self.assertEqual(F(strict=False).strict, STRICTNESS.IGNORE)
 
@@ -770,6 +788,7 @@ class F(FilterSet):
 
             class Meta:
                 model = User
+                fields = []
 
             def filter_f(inner_self, qs, name, value):
                 self.assertIsInstance(inner_self, F)