From fa8addc8013a07ea4397332f1b23eb6d482d93fc Mon Sep 17 00:00:00 2001
From: Ryan P Kilby <rpkilby@ncsu.edu>
Date: Thu, 12 Jul 2018 21:59:55 -0400
Subject: [PATCH 1/2] 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.
---
 django_filters/filterset.py |  3 +++
 tests/test_filterset.py     | 12 +++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/django_filters/filterset.py b/django_filters/filterset.py
index b8cc06e41..9ea1f8c1b 100644
--- a/django_filters/filterset.py
+++ b/django_filters/filterset.py
@@ -179,6 +179,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_filterset.py b/tests/test_filterset.py
index 3b431b025..dcf7690ce 100644
--- a/tests/test_filterset.py
+++ b/tests/test_filterset.py
@@ -634,9 +634,11 @@ 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()
@@ -681,6 +683,14 @@ def test_qs_triggers_form_validation(self):
             f.qs
             fn.assert_called()
 
+    def test_filters_must_return_queryset(self):
+        m = mock.Mock()
+        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):

From cd81823c059a7fb8eb82da32d1ff07239d6f364b Mon Sep 17 00:00:00 2001
From: Ryan P Kilby <rpkilby@ncsu.edu>
Date: Thu, 12 Jul 2018 22:04:23 -0400
Subject: [PATCH 2/2] Fix tests broken by new exception

---
 tests/test_filtering.py |  7 +++----
 tests/test_filterset.py | 10 ++++++----
 tests/utils.py          | 15 +++++++++++++++
 3 files changed, 24 insertions(+), 8 deletions(-)
 create mode 100644 tests/utils.py

diff --git a/tests/test_filtering.py b/tests/test_filtering.py
index 889e62d61..a98a6f99d 100644
--- a/tests/test_filtering.py
+++ b/tests/test_filtering.py
@@ -42,6 +42,7 @@
     SpacewalkRecord,
     User
 )
+from .utils import MockQuerySet
 
 
 class CharFilterTests(TestCase):
@@ -1869,10 +1870,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 dcf7690ce..4c5ce0400 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):
@@ -641,7 +642,7 @@ class Meta:
             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',
@@ -674,7 +675,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',
@@ -684,7 +685,7 @@ def test_qs_triggers_form_validation(self):
             fn.assert_called()
 
     def test_filters_must_return_queryset(self):
-        m = mock.Mock()
+        m = MockQuerySet()
         f = self.F({'invalid': 'result'}, queryset=m)
 
         msg = "Expected 'F.invalid' to return a QuerySet, but got a NoneType instead."
@@ -769,7 +770,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')
@@ -781,6 +782,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