Fix null filtering for *Choice filters (#680)

* Add test cases from PR 639. Thanks @michael-mri!

* Fix tests for null choice iterator

* Add more null value tests

* Add null choice to various (Model)ChoiceFields

* Fix django < 1.11 field iterator handling

* Resolve #710 - add test for lazy callable choices

django_filters/fields.py

@@ -3,11 +3,13 @@
 from collections import namedtuple
 from datetime import datetime, time
 
+import django
 from django import forms
 from django.utils.dateparse import parse_datetime
 from django.utils.encoding import force_str
 from django.utils.translation import ugettext_lazy as _
 
+from .conf import settings
 from .utils import handle_timezone
 from .widgets import BaseCSVWidget, CSVWidget, LookupTypeWidget, RangeWidget
 
@@ -179,3 +181,114 @@ def clean(self, value):
                 code='invalid_values')
 
         return value
+
+
+class ChoiceIterator(object):
+    # Emulates the behavior of ModelChoiceIterator, but instead wraps
+    # the field's _choices iterable.
+
+    def __init__(self, field, choices):
+        self.field = field
+        self.choices = choices
+
+    def __iter__(self):
+        if self.field.empty_label is not None:
+            yield ("", self.field.empty_label)
+        if self.field.null_label is not None:
+            yield (self.field.null_value, self.field.null_label)
+
+        # Python 2 lacks 'yield from'
+        for choice in self.choices:
+            yield choice
+
+    def __len__(self):
+        add = 1 if self.field.empty_label is not None else 0
+        add += 1 if self.field.null_label is not None else 0
+        return len(self.choices) + add
+
+
+class ModelChoiceIterator(forms.models.ModelChoiceIterator):
+    # Extends the base ModelChoiceIterator to add in 'null' choice handling.
+    # This is a bit verbose since we have to insert the null choice after the
+    # empty choice, but before the remainder of the choices.
+
+    def __iter__(self):
+        iterable = super(ModelChoiceIterator, self).__iter__()
+
+        if self.field.empty_label is not None:
+            yield next(iterable)
+        if self.field.null_label is not None:
+            yield (self.field.null_value, self.field.null_label)
+
+        # Python 2 lacks 'yield from'
+        for value in iterable:
+            yield value
+
+    def __len__(self):
+        add = 1 if self.field.null_label is not None else 0
+        return super(ModelChoiceIterator, self).__len__() + add
+
+
+class ChoiceIteratorMixin(object):
+    def __init__(self, *args, **kwargs):
+        self.null_label = kwargs.pop('null_label', settings.NULL_CHOICE_LABEL)
+        self.null_value = kwargs.pop('null_value', settings.NULL_CHOICE_VALUE)
+
+        super(ChoiceIteratorMixin, self).__init__(*args, **kwargs)
+
+    def _get_choices(self):
+        if django.VERSION >= (1, 11):
+            return super(ChoiceIteratorMixin, self)._get_choices()
+
+        # HACK: Django < 1.11 does not allow a custom iterator to be provided.
+        # This code only executes for Model*ChoiceFields.
+        if hasattr(self, '_choices'):
+            return self._choices
+        return self.iterator(self)
+
+    def _set_choices(self, value):
+        super(ChoiceIteratorMixin, self)._set_choices(value)
+        value = self.iterator(self, self._choices)
+
+        self._choices = self.widget.choices = value
+    choices = property(_get_choices, _set_choices)
+
+
+# Unlike their Model* counterparts, forms.ChoiceField and forms.MultipleChoiceField do not set empty_label
+class ChoiceField(ChoiceIteratorMixin, forms.ChoiceField):
+    iterator = ChoiceIterator
+
+    def __init__(self, *args, **kwargs):
+        self.empty_label = kwargs.pop('empty_label', settings.EMPTY_CHOICE_LABEL)
+        super(ChoiceField, self).__init__(*args, **kwargs)
+
+
+class MultipleChoiceField(ChoiceIteratorMixin, forms.MultipleChoiceField):
+    iterator = ChoiceIterator
+
+    def __init__(self, *args, **kwargs):
+        self.empty_label = None
+        super(MultipleChoiceField, self).__init__(*args, **kwargs)
+
+
+class ModelChoiceField(ChoiceIteratorMixin, forms.ModelChoiceField):
+    iterator = ModelChoiceIterator
+
+    def to_python(self, value):
+        # bypass the queryset value check
+        if self.null_label is not None and value == self.null_value:
+            return value
+        return super(ModelChoiceField, self).to_python(value)
+
+
+class ModelMultipleChoiceField(ChoiceIteratorMixin, forms.ModelMultipleChoiceField):
+    iterator = ModelChoiceIterator
+
+    def _check_values(self, value):
+        null = self.null_label is not None and value and self.null_value in value
+        if null:  # remove the null value and any potential duplicates
+            value = [v for v in value if v != self.null_value]
+
+        result = list(super(ModelMultipleChoiceField, self)._check_values(value))
+        result += [self.null_value] if null else []
+        return result

django_filters/filters.py

@@ -18,11 +18,15 @@
 from .fields import (
     BaseCSVField,
     BaseRangeField,
+    ChoiceField,
     DateRangeField,
     DateTimeRangeField,
     IsoDateTimeField,
     Lookup,
     LookupTypeField,
+    ModelChoiceField,
+    ModelMultipleChoiceField,
+    MultipleChoiceField,
     RangeField,
     TimeRangeField
 )
@@ -187,32 +191,10 @@ class BooleanFilter(Filter):
 
 
 class ChoiceFilter(Filter):
-    field_class = forms.ChoiceField
+    field_class = ChoiceField
 
     def __init__(self, *args, **kwargs):
-        empty_label = kwargs.pop('empty_label', settings.EMPTY_CHOICE_LABEL)
-        null_label = kwargs.pop('null_label', settings.NULL_CHOICE_LABEL)
-        null_value = kwargs.pop('null_value', settings.NULL_CHOICE_VALUE)
-
-        self.null_value = null_value
-
-        if 'choices' in kwargs:
-            choices = kwargs.get('choices')
-
-            # coerce choices to list
-            if callable(choices):
-                choices = choices()
-            choices = list(choices)
-
-            # create the empty/null choices that prepend the original choices
-            prepend = []
-            if empty_label is not None:
-                prepend.append(('', empty_label))
-            if null_label is not None:
-                prepend.append((null_value, null_label))
-
-            kwargs['choices'] = prepend + choices
-
+        self.null_value = kwargs.get('null_value', settings.NULL_CHOICE_VALUE)
         super(ChoiceFilter, self).__init__(*args, **kwargs)
 
     def filter(self, qs, value):
@@ -254,13 +236,14 @@ class MultipleChoiceFilter(Filter):
     `distinct` defaults to `True` as to-many relationships will generally
     require this.
     """
-    field_class = forms.MultipleChoiceField
+    field_class = MultipleChoiceField
 
     always_filter = True
 
     def __init__(self, *args, **kwargs):
         kwargs.setdefault('distinct', True)
         self.conjoined = kwargs.pop('conjoined', False)
+        self.null_value = kwargs.get('null_value', settings.NULL_CHOICE_VALUE)
         super(MultipleChoiceFilter, self).__init__(*args, **kwargs)
 
     def is_noop(self, qs, value):
@@ -288,6 +271,8 @@ def filter(self, qs, value):
         if not self.conjoined:
             q = Q()
         for v in set(value):
+            if v == self.null_value:
+                v = None
             predicate = self.get_filter_predicate(v)
             if self.conjoined:
                 qs = self.get_method(qs)(**predicate)
@@ -390,12 +375,16 @@ def field(self):
         return super(QuerySetRequestMixin, self).field
 
 
-class ModelChoiceFilter(QuerySetRequestMixin, Filter):
-    field_class = forms.ModelChoiceField
+class ModelChoiceFilter(QuerySetRequestMixin, ChoiceFilter):
+    field_class = ModelChoiceField
+
+    def __init__(self, *args, **kwargs):
+        kwargs.setdefault('empty_label', settings.EMPTY_CHOICE_LABEL)
+        super(ModelChoiceFilter, self).__init__(*args, **kwargs)
 
 
 class ModelMultipleChoiceFilter(QuerySetRequestMixin, MultipleChoiceFilter):
-    field_class = forms.ModelMultipleChoiceField
+    field_class = ModelMultipleChoiceField
 
 
 class NumberFilter(Filter):

django_filters/filterset.py

@@ -159,13 +159,15 @@ def get_declared_filters(cls, bases, attrs):
         'extra': lambda f: {
             'queryset': remote_queryset(f),
             'to_field_name': remote_field(f).field_name,
+            'null_label': settings.NULL_CHOICE_LABEL if f.null else None,
         }
     },
     models.ForeignKey: {
         'filter_class': ModelChoiceFilter,
         'extra': lambda f: {
             'queryset': remote_queryset(f),
             'to_field_name': remote_field(f).field_name,
+            'null_label': settings.NULL_CHOICE_LABEL if f.null else None,
         }
     },
     models.ManyToManyField: {

tests/test_filtering.py

@@ -254,6 +254,44 @@ class Meta:
         self.assertQuerysetEqual(
             f.qs, ['aaron', 'alex', 'carl', 'jacob'], lambda o: o.username)
 
+    def test_filtering_on_null_choice(self):
+        User.objects.create(username='alex', status=1)
+        User.objects.create(username='jacob', status=2)
+        User.objects.create(username='aaron', status=2)
+        User.objects.create(username='carl', status=0)
+
+        Article.objects.create(author_id=1, published=now())
+        Article.objects.create(author_id=2, published=now())
+        Article.objects.create(author_id=3, published=now())
+        Article.objects.create(author_id=4, published=now())
+        Article.objects.create(author_id=None, published=now())
+
+        choices = [(u.pk, str(u)) for u in User.objects.order_by('id')]
+
+        class F(FilterSet):
+            author = MultipleChoiceFilter(
+                choices=choices,
+                null_value='null',
+                null_label='NULL',
+            )
+
+            class Meta:
+                model = Article
+                fields = ['author']
+
+        # sanity check to make sure the filter is setup correctly
+        f = F({'author': ['1']})
+        self.assertQuerysetEqual(f.qs, ['alex'], lambda o: str(o.author), False)
+
+        f = F({'author': ['null']})
+        self.assertQuerysetEqual(f.qs, [None], lambda o: o.author, False)
+
+        f = F({'author': ['1', 'null']})
+        self.assertQuerysetEqual(
+            f.qs, ['alex', None],
+            lambda o: o.author and str(o.author),
+            False)
+
 
 class TypedMultipleChoiceFilterTests(TestCase):
 
@@ -487,6 +525,21 @@ class Meta:
         f = F({'author': jacob.pk}, queryset=qs)
         self.assertQuerysetEqual(f.qs, [1, 3], lambda o: o.pk, False)
 
+    @override_settings(FILTERS_NULL_CHOICE_LABEL='No Author')
+    def test_filtering_null(self):
+        Article.objects.create(published=now())
+        alex = User.objects.create(username='alex')
+        Article.objects.create(author=alex, published=now())
+
+        class F(FilterSet):
+            class Meta:
+                model = Article
+                fields = ['author', 'name']
+
+        qs = Article.objects.all()
+        f = F({'author': 'null'}, queryset=qs)
+        self.assertQuerysetEqual(f.qs, [None], lambda o: o.author, False)
+
     def test_callable_queryset(self):
         # Sanity check for callable queryset arguments.
         # Ensure that nothing is improperly cached
@@ -554,6 +607,18 @@ class Meta:
         f = F({'favorite_books': ['4']}, queryset=qs)
         self.assertQuerysetEqual(f.qs, [], lambda o: o.username)
 
+    @override_settings(FILTERS_NULL_CHOICE_LABEL='No Favorites')
+    def test_filtering_null(self):
+        class F(FilterSet):
+            class Meta:
+                model = User
+                fields = ['favorite_books']
+
+        qs = User.objects.all()
+        f = F({'favorite_books': ['null']}, queryset=qs)
+
+        self.assertQuerysetEqual(f.qs, ['jacob'], lambda o: o.username)
+
     def test_filtering_dictionary(self):
         class F(FilterSet):
             class Meta: