From 82a47fb7bbddedf179f110723003f3b28682d7fe Mon Sep 17 00:00:00 2001
From: Ryan P Kilby <rpkilby@ncsu.edu>
Date: Fri, 13 Jul 2018 05:31:22 -0400
Subject: [PATCH] Improve exception message for invalid filter result (#943)

* Improve exception message for invalid filter calls

The current exception is an AttributeError that lacks any context about
the filter call. e.g., "NoneType has no attribute filter". This message
is improved by including which filterset and which filter are incorrect.

* Fix tests broken by new exception
---
 django_filters/filterset.py |  3 +++
 tests/test_filtering.py     |  7 +++----
 tests/test_filterset.py     | 20 ++++++++++++++++----
 tests/utils.py              | 15 +++++++++++++++
 4 files changed, 37 insertions(+), 8 deletions(-)
 create mode 100644 tests/utils.py

diff --git a/django_filters/filterset.py b/django_filters/filterset.py
index 3fce096d1..a594a1229 100644
--- a/django_filters/filterset.py
+++ b/django_filters/filterset.py
@@ -222,6 +222,9 @@ def filter_queryset(self, queryset):
         """
         for name, value in self.form.cleaned_data.items():
             queryset = self.filters[name].filter(queryset, value)
+            assert isinstance(queryset, models.QuerySet), \
+                "Expected '%s.%s' to return a QuerySet, but got a %s instead." \
+                % (type(self).__name__, name, type(queryset).__name__)
         return queryset
 
     @property
diff --git a/tests/test_filtering.py b/tests/test_filtering.py
index 9e7521c42..538a682ed 100644
--- a/tests/test_filtering.py
+++ b/tests/test_filtering.py
@@ -45,6 +45,7 @@
     SpacewalkRecord,
     User
 )
+from .utils import MockQuerySet
 
 
 class CharFilterTests(TestCase):
@@ -1894,10 +1895,8 @@ class Meta:
                 model = User
                 fields = ['account']
 
-        qs = mock.NonCallableMagicMock()
-        f = F({'account': 'jdoe'}, queryset=qs)
-        result = f.qs
-        self.assertNotEqual(qs, result)
+        qs = MockQuerySet()
+        F({'account': 'jdoe'}, queryset=qs).qs
         qs.all.return_value.filter.assert_called_with(username__exact='jdoe')
 
     def test_filtering_without_meta(self):
diff --git a/tests/test_filterset.py b/tests/test_filterset.py
index d62b80199..9fd8aa4cb 100644
--- a/tests/test_filterset.py
+++ b/tests/test_filterset.py
@@ -40,6 +40,7 @@
     UUIDTestModel,
     Worker
 )
+from .utils import MockQuerySet
 
 
 def checkItemsEqual(L1, L2):
@@ -655,12 +656,14 @@ def test_creating_with_request(self):
 class FilterSetQuerysetTests(TestCase):
 
     class F(FilterSet):
+        invalid = CharFilter(method=lambda *args: None)
+
         class Meta:
             model = User
-            fields = ['username']
+            fields = ['username', 'invalid']
 
     def test_filter_queryset_called_once(self):
-        m = mock.Mock()
+        m = MockQuerySet()
         f = self.F({'username': 'bob'}, queryset=m)
 
         with mock.patch.object(f, 'filter_queryset',
@@ -693,7 +696,7 @@ def test_form_caching(self):
         self.assertIs(f.form, f.form)
 
     def test_qs_triggers_form_validation(self):
-        m = mock.Mock()
+        m = MockQuerySet()
         f = self.F({'username': 'bob'}, queryset=m)
 
         with mock.patch.object(f.form, 'full_clean',
@@ -702,6 +705,14 @@ def test_qs_triggers_form_validation(self):
             f.qs
             fn.assert_called()
 
+    def test_filters_must_return_queryset(self):
+        m = MockQuerySet()
+        f = self.F({'invalid': 'result'}, queryset=m)
+
+        msg = "Expected 'F.invalid' to return a QuerySet, but got a NoneType instead."
+        with self.assertRaisesMessage(AssertionError, msg):
+            f.qs
+
 
 # test filter.method here, as it depends on its parent FilterSet
 class FilterMethodTests(TestCase):
@@ -780,7 +791,7 @@ def test_parent_unresolvable(self):
     def test_method_self_is_parent(self):
         # Ensure the method isn't 're-parented' on the `FilterMethod` helper class.
         # Filter methods should have access to the filterset's properties.
-        request = mock.Mock()
+        request = MockQuerySet()
 
         class F(FilterSet):
             f = CharFilter(method='filter_f')
@@ -792,6 +803,7 @@ class Meta:
             def filter_f(inner_self, qs, name, value):
                 self.assertIsInstance(inner_self, F)
                 self.assertIs(inner_self.request, request)
+                return qs
 
         F({'f': 'foo'}, request=request, queryset=User.objects.all()).qs
 
diff --git a/tests/utils.py b/tests/utils.py
new file mode 100644
index 000000000..e11410eb3
--- /dev/null
+++ b/tests/utils.py
@@ -0,0 +1,15 @@
+from unittest import mock
+
+from django.db import models
+
+
+class MockQuerySet:
+    """
+    Generate a mock that is suitably similar to a QuerySet
+    """
+
+    def __new__(self):
+        m = mock.Mock(spec_set=models.QuerySet())
+        m.filter.return_value = m
+        m.all.return_value = m
+        return m