Fix lookups for reverse relationships (#915)

* Remove redundant rel lookup

* Make 'remote_queryset' compatible w/ foreign rels

* Use 'filter_for_field' for foreign rels

* Test generating lookups for foreign rel filters

* Deprecate 'filter_for_reverse_field'

django_filters/filterset.py

@@ -4,7 +4,11 @@
 from django import forms
 from django.db import models
 from django.db.models.constants import LOOKUP_SEP
-from django.db.models.fields.related import ForeignObjectRel
+from django.db.models.fields.related import (
+    ManyToManyRel,
+    ManyToOneRel,
+    OneToOneRel
+)
 
 from .conf import settings
 from .constants import ALL_FIELDS
@@ -33,9 +37,17 @@
 
 
 def remote_queryset(field):
-    model = field.remote_field.model
-    limit_choices_to = field.get_limit_choices_to()
+    """
+    Get the queryset for the other side of a relationship. This works
+    for both `RelatedField`s and `ForignObjectRel`s.
+    """
+    model = field.related_model
 
+    # Reverse relationships do not have choice limits
+    if not hasattr(field, 'get_limit_choices_to'):
+        return model._default_manager.all()
+
+    limit_choices_to = field.get_limit_choices_to()
     return model._default_manager.complex_filter(limit_choices_to)
 
 
@@ -58,6 +70,14 @@ def __new__(cls, name, bases, attrs):
         new_class._meta = FilterSetOptions(getattr(new_class, 'Meta', None))
         new_class.base_filters = new_class.get_filters()
 
+        # TODO: remove assertion in 2.1
+        assert not hasattr(new_class, 'filter_for_reverse_field'), (
+            "`%(cls)s.filter_for_reverse_field` has been removed. "
+            "`%(cls)s.filter_for_field` now generates filters for reverse fields. "
+            "See: https://django-filter.readthedocs.io/en/master/guide/migration.html"
+            % {'cls': new_class.__name__}
+        )
+
         return new_class
 
     @classmethod
@@ -110,6 +130,8 @@ def get_declared_filters(cls, bases, attrs):
     models.GenericIPAddressField:       {'filter_class': CharFilter},
     models.CommaSeparatedIntegerField:  {'filter_class': CharFilter},
     models.UUIDField:                   {'filter_class': UUIDFilter},
+
+    # Forward relationships
     models.OneToOneField: {
         'filter_class': ModelChoiceFilter,
         'extra': lambda f: {
@@ -132,6 +154,27 @@ def get_declared_filters(cls, bases, attrs):
             'queryset': remote_queryset(f),
         }
     },
+
+    # Reverse relationships
+    OneToOneRel: {
+        'filter_class': ModelChoiceFilter,
+        'extra': lambda f: {
+            'queryset': remote_queryset(f),
+            'null_label': settings.NULL_CHOICE_LABEL if f.null else None,
+        }
+    },
+    ManyToOneRel: {
+        'filter_class': ModelMultipleChoiceFilter,
+        'extra': lambda f: {
+            'queryset': remote_queryset(f),
+        }
+    },
+    ManyToManyRel: {
+        'filter_class': ModelMultipleChoiceFilter,
+        'extra': lambda f: {
+            'queryset': remote_queryset(f),
+        }
+    },
 }
 
 
@@ -287,11 +330,6 @@ def get_filters(cls):
             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 = cls.get_filter_name(field_name, lookup_expr)
 
@@ -337,19 +375,6 @@ def filter_for_field(cls, field, field_name, lookup_expr='exact'):
 
         return filter_class(**default)
 
-    @classmethod
-    def filter_for_reverse_field(cls, field, field_name):
-        rel = field.field.remote_field
-        queryset = field.field.model._default_manager.all()
-        default = {
-            'field_name': field_name,
-            'queryset': queryset,
-        }
-        if rel.multiple:
-            return ModelMultipleChoiceFilter(**default)
-        else:
-            return ModelChoiceFilter(**default)
-
     @classmethod
     def filter_for_lookup(cls, field, lookup_type):
         DEFAULTS = dict(cls.FILTER_DEFAULTS)
@@ -365,7 +390,7 @@ def filter_for_lookup(cls, field, lookup_type):
             return None, {}
 
         # perform lookup specific checks
-        if lookup_type == 'exact' and field.choices:
+        if lookup_type == 'exact' and getattr(field, 'choices', None):
             return ChoiceFilter, {'choices': field.choices}
 
         if lookup_type == 'isnull':

docs/guide/migration.txt

@@ -45,6 +45,15 @@ The ``Filter.lookup_expr`` argument no longer accepts ``None`` or a list of
 expressions. Use the :ref:`LookupChoiceFilter <lookup-choice-filter>` instead.
 
 
+FilterSet ``filter_for_reverse_field`` removed (`#915`__)
+---------------------------------------------------------
+__ https://github.com/carltongibson/django-filter/pull/915
+
+The ``filter_for_field`` method now generates filters for reverse relationships,
+removing  the need for ``filter_for_reverse_field``. As a result, reverse
+relationships now also obey ``Meta.filter_overrides``.
+
+
 View attributes renamed (`#867`__)
 ----------------------------------
 __ https://github.com/carltongibson/django-filter/pull/867

tests/test_filterset.py

@@ -242,11 +242,12 @@ class Meta:
         self.assertEqual(params['widget'], BooleanWidget)
 
 
-class FilterSetFilterForReverseFieldTests(TestCase):
+class ReverseFilterSetFilterForFieldTests(TestCase):
+    # Test reverse relationships for `filter_for_field`
 
     def test_reverse_o2o_relationship(self):
         f = Account._meta.get_field('profile')
-        result = FilterSet.filter_for_reverse_field(f, 'profile')
+        result = FilterSet.filter_for_field(f, 'profile')
         self.assertIsInstance(result, ModelChoiceFilter)
         self.assertEqual(result.field_name, 'profile')
         self.assertTrue('queryset' in result.extra)
@@ -255,7 +256,7 @@ def test_reverse_o2o_relationship(self):
 
     def test_reverse_fk_relationship(self):
         f = User._meta.get_field('comments')
-        result = FilterSet.filter_for_reverse_field(f, 'comments')
+        result = FilterSet.filter_for_field(f, 'comments')
         self.assertIsInstance(result, ModelMultipleChoiceFilter)
         self.assertEqual(result.field_name, 'comments')
         self.assertTrue('queryset' in result.extra)
@@ -264,7 +265,7 @@ def test_reverse_fk_relationship(self):
 
     def test_reverse_m2m_relationship(self):
         f = Book._meta.get_field('lovers')
-        result = FilterSet.filter_for_reverse_field(f, 'lovers')
+        result = FilterSet.filter_for_field(f, 'lovers')
         self.assertIsInstance(result, ModelMultipleChoiceFilter)
         self.assertEqual(result.field_name, 'lovers')
         self.assertTrue('queryset' in result.extra)
@@ -273,7 +274,7 @@ def test_reverse_m2m_relationship(self):
 
     def test_reverse_non_symmetrical_selfref_m2m_field(self):
         f = DirectedNode._meta.get_field('inbound_nodes')
-        result = FilterSet.filter_for_reverse_field(f, 'inbound_nodes')
+        result = FilterSet.filter_for_field(f, 'inbound_nodes')
         self.assertIsInstance(result, ModelMultipleChoiceFilter)
         self.assertEqual(result.field_name, 'inbound_nodes')
         self.assertTrue('queryset' in result.extra)
@@ -282,13 +283,33 @@ def test_reverse_non_symmetrical_selfref_m2m_field(self):
 
     def test_reverse_m2m_field_with_through_model(self):
         f = Worker._meta.get_field('employers')
-        result = FilterSet.filter_for_reverse_field(f, 'employers')
+        result = FilterSet.filter_for_field(f, 'employers')
         self.assertIsInstance(result, ModelMultipleChoiceFilter)
         self.assertEqual(result.field_name, 'employers')
         self.assertTrue('queryset' in result.extra)
         self.assertIsNotNone(result.extra['queryset'])
         self.assertEqual(result.extra['queryset'].model, Business)
 
+    def test_reverse_relationship_lookup_expr(self):
+        f = Book._meta.get_field('lovers')
+        result = FilterSet.filter_for_field(f, 'lovers', 'isnull')
+        self.assertIsInstance(result, BooleanFilter)
+        self.assertEqual(result.field_name, 'lovers')
+        self.assertEqual(result.lookup_expr, 'isnull')
+
+
+class FilterSetFilterForReverseFieldTests(TestCase):
+
+    def test_method_raises_assertion(self):
+        msg = ("`F.filter_for_reverse_field` has been removed. "
+               "`F.filter_for_field` now generates filters for reverse fields.")
+
+        with self.assertRaisesMessage(AssertionError, msg):
+            class F(FilterSet):
+                @classmethod
+                def filter_for_reverse_field(cls, field, field_name):
+                    pass
+
 
 class FilterSetClassCreationTests(TestCase):