Fixed #24201 -- Added order_with_respect_to support to GenericForeignKey.

4 files changed, 99 insertions, 41 deletions

diff --git a/django/contrib/contenttypes/fields.py b/django/contrib/contenttypes/fields.py
index 837a9d5bca..1cd2fc0fa6 100644
--- a/django/contrib/contenttypes/fields.py
+++ b/django/contrib/contenttypes/fields.py
@@ -7,9 +7,10 @@ from django.core import checks
 from django.core.exceptions import FieldDoesNotExist, ObjectDoesNotExist
 from django.db import DEFAULT_DB_ALIAS, connection, models, router, transaction
 from django.db.models import DO_NOTHING, signals
-from django.db.models.base import ModelBase
+from django.db.models.base import ModelBase, make_foreign_order_accessors
 from django.db.models.fields.related import (
     ForeignObject, ForeignObjectRel, ForeignRelatedObjectsDescriptor,
+    lazy_related_operation,
 )
 from django.db.models.query_utils import PathInfo
 from django.utils.encoding import python_2_unicode_compatible, smart_text
@@ -61,6 +62,20 @@ class GenericForeignKey(object):
 
         setattr(cls, name, self)
 
+    def get_filter_kwargs_for_object(self, obj):
+        """See corresponding method on Field"""
+        return {
+            self.fk_field: getattr(obj, self.fk_field),
+            self.ct_field: getattr(obj, self.ct_field),
+        }
+
+    def get_forward_related_filter(self, obj):
+        """See corresponding method on RelatedField"""
+        return {
+            self.fk_field: obj.pk,
+            self.ct_field: ContentType.objects.get_for_model(obj).pk,
+        }
+
     def __str__(self):
         model = self.model
         app = model._meta.app_label
@@ -368,6 +383,21 @@ class GenericRelation(ForeignObject):
         self.model = cls
         setattr(cls, self.name, ReverseGenericRelatedObjectsDescriptor(self.remote_field))
 
+        # Add get_RELATED_order() and set_RELATED_order() methods if the model
+        # on the other end of this relation is ordered with respect to this.
+        def matching_gfk(field):
+            return (
+                isinstance(field, GenericForeignKey) and
+                self.content_type_field_name == field.ct_field and
+                self.object_id_field_name == field.fk_field
+            )
+
+        def make_generic_foreign_order_accessors(related_model, model):
+            if matching_gfk(model._meta.order_with_respect_to):
+                make_foreign_order_accessors(model, related_model)
+
+        lazy_related_operation(make_generic_foreign_order_accessors, self.model, self.remote_field.model)
+
     def set_attributes_from_rel(self):
         pass
 
diff --git a/django/db/models/base.py b/django/db/models/base.py
index 3cbabbc576..2c4a475cd4 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -311,21 +311,15 @@ class ModelBase(type):
             cls.get_next_in_order = curry(cls._get_next_or_previous_in_order, is_next=True)
             cls.get_previous_in_order = curry(cls._get_next_or_previous_in_order, is_next=False)
 
-            # defer creating accessors on the foreign class until we are
-            # certain it has been created
-            def make_foreign_order_accessors(cls, model, field):
-                setattr(
-                    field.remote_field.model,
-                    'get_%s_order' % cls.__name__.lower(),
-                    curry(method_get_order, cls)
-                )
-                setattr(
-                    field.remote_field.model,
-                    'set_%s_order' % cls.__name__.lower(),
-                    curry(method_set_order, cls)
-                )
-            wrt = opts.order_with_respect_to
-            lazy_related_operation(make_foreign_order_accessors, cls, wrt.remote_field.model, field=wrt)
+            # Defer creating accessors on the foreign class until it has been
+            # created and registered. If remote_field is None, we're ordering
+            # with respect to a GenericForeignKey and don't know what the
+            # foreign class is - we'll add those accessors later in
+            # contribute_to_class().
+            if opts.order_with_respect_to.remote_field:
+                wrt = opts.order_with_respect_to
+                remote = wrt.remote_field.model
+                lazy_related_operation(make_foreign_order_accessors, cls, remote)
 
         # Give the class a docstring -- its definition.
         if cls.__doc__ is None:
@@ -803,8 +797,8 @@ class Model(six.with_metaclass(ModelBase)):
                 # If this is a model with an order_with_respect_to
                 # autopopulate the _order field
                 field = meta.order_with_respect_to
-                order_value = cls._base_manager.using(using).filter(
-                    **{field.name: getattr(self, field.attname)}).count()
+                filter_args = field.get_filter_kwargs_for_object(self)
+                order_value = cls._base_manager.using(using).filter(**filter_args).count()
                 self._order = order_value
 
             fields = meta.local_concrete_fields
@@ -892,9 +886,8 @@ class Model(six.with_metaclass(ModelBase)):
             op = 'gt' if is_next else 'lt'
             order = '_order' if is_next else '-_order'
             order_field = self._meta.order_with_respect_to
-            obj = self._default_manager.filter(**{
-                order_field.name: getattr(self, order_field.attname)
-            }).filter(**{
+            filter_args = order_field.get_filter_kwargs_for_object(self)
+            obj = self._default_manager.filter(**filter_args).filter(**{
                 '_order__%s' % op: self._default_manager.values('_order').filter(**{
                     self._meta.pk.name: self.pk
                 })
@@ -1653,22 +1646,33 @@ class Model(six.with_metaclass(ModelBase)):
 def method_set_order(ordered_obj, self, id_list, using=None):
     if using is None:
         using = DEFAULT_DB_ALIAS
-    rel_val = getattr(self, ordered_obj._meta.order_with_respect_to.remote_field.field_name)
-    order_name = ordered_obj._meta.order_with_respect_to.name
+    order_wrt = ordered_obj._meta.order_with_respect_to
+    filter_args = order_wrt.get_forward_related_filter(self)
     # FIXME: It would be nice if there was an "update many" version of update
     # for situations like this.
     with transaction.atomic(using=using, savepoint=False):
         for i, j in enumerate(id_list):
-            ordered_obj.objects.filter(**{'pk': j, order_name: rel_val}).update(_order=i)
+            ordered_obj.objects.filter(pk=j, **filter_args).update(_order=i)
 
 
 def method_get_order(ordered_obj, self):
-    rel_val = getattr(self, ordered_obj._meta.order_with_respect_to.remote_field.field_name)
-    order_name = ordered_obj._meta.order_with_respect_to.name
+    order_wrt = ordered_obj._meta.order_with_respect_to
+    filter_args = order_wrt.get_forward_related_filter(self)
     pk_name = ordered_obj._meta.pk.name
-    return [r[pk_name] for r in
-            ordered_obj.objects.filter(**{order_name: rel_val}).values(pk_name)]
+    return ordered_obj.objects.filter(**filter_args).values_list(pk_name, flat=True)
+
 
+def make_foreign_order_accessors(model, related_model):
+    setattr(
+        related_model,
+        'get_%s_order' % model.__name__.lower(),
+        curry(method_get_order, model)
+    )
+    setattr(
+        related_model,
+        'set_%s_order' % model.__name__.lower(),
+        curry(method_set_order, model)
+    )
 
 ########
 # MISC #
diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py
index 5cd96d2902..91bf0285d9 100644
--- a/django/db/models/fields/__init__.py
+++ b/django/db/models/fields/__init__.py
@@ -678,6 +678,13 @@ class Field(RegisterLookupMixin):
             setattr(cls, 'get_%s_display' % self.name,
                     curry(cls._get_FIELD_display, field=self))
 
+    def get_filter_kwargs_for_object(self, obj):
+        """
+        Return a dict that when passed as kwargs to self.model.filter(), would
+        yield all instances having the same value for this field as obj has.
+        """
+        return {self.name: getattr(obj, self.attname)}
+
     def get_attname(self):
         return self.name
 
diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py
index c433ce239d..5e7bda5d5c 100644
--- a/django/db/models/fields/related.py
+++ b/django/db/models/fields/related.py
@@ -303,6 +303,33 @@ class RelatedField(Field):
                 field.do_related_class(related, model)
             lazy_related_operation(resolve_related_class, cls, self.remote_field.model, field=self)
 
+    def get_forward_related_filter(self, obj):
+        """
+        Return the keyword arguments that when supplied to
+        self.model.object.filter(), would select all instances related through
+        this field to the remote obj. This is used to build the querysets
+        returned by related descriptors. obj is an instance of
+        self.related_field.model.
+        """
+        return {
+            '%s__%s' % (self.name, rh_field.name): getattr(obj, rh_field.attname)
+            for _, rh_field in self.related_fields
+        }
+
+    def get_reverse_related_filter(self, obj):
+        """
+        Complement to get_forward_related_filter(). Return the keyword
+        arguments that when passed to self.related_field.model.object.filter()
+        select all instances of self.related_field.model related through
+        this field to obj. obj is an instance of self.model.
+        """
+        base_filter = {
+            rh_field.attname: getattr(obj, lh_field.attname)
+            for lh_field, rh_field in self.related_fields
+        }
+        base_filter.update(self.get_extra_descriptor_filter(obj) or {})
+        return base_filter
+
     @property
     def swappable_setting(self):
         """
@@ -453,11 +480,9 @@ class SingleRelatedObjectDescriptor(object):
             if related_pk is None:
                 rel_obj = None
             else:
-                params = {}
-                for lh_field, rh_field in self.related.field.related_fields:
-                    params['%s__%s' % (self.related.field.name, rh_field.name)] = getattr(instance, rh_field.attname)
+                filter_args = self.related.field.get_forward_related_filter(instance)
                 try:
-                    rel_obj = self.get_queryset(instance=instance).get(**params)
+                    rel_obj = self.get_queryset(instance=instance).get(**filter_args)
                 except self.related.related_model.DoesNotExist:
                     rel_obj = None
                 else:
@@ -603,16 +628,8 @@ class ReverseSingleRelatedObjectDescriptor(object):
             if None in val:
                 rel_obj = None
             else:
-                params = {
-                    rh_field.attname: getattr(instance, lh_field.attname)
-                    for lh_field, rh_field in self.field.related_fields}
                 qs = self.get_queryset(instance=instance)
-                extra_filter = self.field.get_extra_descriptor_filter(instance)
-                if isinstance(extra_filter, dict):
-                    params.update(extra_filter)
-                    qs = qs.filter(**params)
-                else:
-                    qs = qs.filter(extra_filter, **params)
+                qs = qs.filter(**self.field.get_reverse_related_filter(instance))
                 # Assuming the database enforces foreign keys, this won't fail.
                 rel_obj = qs.get()
                 if not self.field.remote_field.multiple: