[1.8.x] Fixed #24328 -- cleaned up Options._get_fields() implementation

Backport of bad5f262bf4a17f157808ec1aa225ba9c94c1eee from master

5 files changed, 105 insertions, 104 deletions

diff --git a/django/db/models/base.py b/django/db/models/base.py
index 7ade69a5d0..f75d20325f 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -272,7 +272,8 @@ class ModelBase(type):
             else:
                 # .. and abstract ones.
                 for field in parent_fields:
-                    new_class.add_to_class(field.name, copy.deepcopy(field))
+                    new_field = copy.deepcopy(field)
+                    new_class.add_to_class(field.name, new_field)
 
                 # Pass any non-abstract parent classes onto child.
                 new_class._meta.parents.update(base._meta.parents)
diff --git a/django/db/models/options.py b/django/db/models/options.py
index 8920b5d86c..e029ffef72 100644
--- a/django/db/models/options.py
+++ b/django/db/models/options.py
@@ -22,6 +22,8 @@ from django.utils.lru_cache import lru_cache
 from django.utils.text import camel_case_to_spaces
 from django.utils.translation import override, string_concat
 
+PROXY_PARENTS = object()
+
 EMPTY_RELATION_TREE = tuple()
 
 IMMUTABLE_WARNING = (
@@ -473,8 +475,6 @@ class Options(object):
     @cached_property
     def _forward_fields_map(self):
         res = {}
-        # call get_fields() with export_ordered_set=True in order to have a
-        # field_instance -> names map
         fields = self._get_fields(reverse=False)
         for field in fields:
             res[field.name] = field
@@ -566,6 +566,10 @@ class Options(object):
             # included in the results.
             if field.is_relation and field.many_to_one and field.related_model is None:
                 continue
+            # Relations to child proxy models should not be included.
+            if (field.model != self.model and
+                    field.model._meta.concrete_model == self.concrete_model):
+                continue
 
             names.add(field.name)
             if hasattr(field, 'attname'):
@@ -576,14 +580,13 @@ class Options(object):
     def get_all_related_objects(self, local_only=False, include_hidden=False,
                                 include_proxy_eq=False):
 
-        include_parents = local_only is False
+        include_parents = True if local_only is False else PROXY_PARENTS
         fields = self._get_fields(
             forward=False, reverse=True,
             include_parents=include_parents,
             include_hidden=include_hidden,
         )
         fields = (obj for obj in fields if not isinstance(obj.field, ManyToManyField))
-
         if include_proxy_eq:
             children = chain.from_iterable(c._relation_tree
                                            for c in self.concrete_model._meta.proxied_children
@@ -606,9 +609,10 @@ class Options(object):
 
     @raise_deprecation(suggested_alternative="get_fields()")
     def get_all_related_many_to_many_objects(self, local_only=False):
+        include_parents = True if local_only is not True else PROXY_PARENTS
         fields = self._get_fields(
             forward=False, reverse=True,
-            include_parents=local_only is not True, include_hidden=True
+            include_parents=include_parents, include_hidden=True
         )
         return [obj for obj in fields if isinstance(obj.field, ManyToManyField)]
 
@@ -676,19 +680,16 @@ class Options(object):
 
         all_models = self.apps.get_models(include_auto_created=True)
         for model in all_models:
+            # Abstract model's fields are copied to child models, hence we will
+            # see the fields from the child models.
+            if model._meta.abstract:
+                continue
             fields_with_relations = (
-                f for f in model._meta._get_fields(reverse=False)
+                f for f in model._meta._get_fields(reverse=False, include_parents=False)
                 if f.is_relation and f.related_model is not None
             )
-            if model._meta.auto_created:
-                fields_with_relations = (
-                    f for f in fields_with_relations
-                    if not f.many_to_many
-                )
-
             for f in fields_with_relations:
                 if not isinstance(f.rel.to, six.string_types):
-                    # Set options_instance -> field
                     related_objects_graph[f.rel.to._meta].append(f)
 
         for model in all_models:
@@ -698,24 +699,14 @@ class Options(object):
             # @cached_property). This means that the _meta._relation_tree is
             # only called if related_objects is not in __dict__.
             related_objects = related_objects_graph[model._meta]
-
-            # If related_objects are empty, it makes sense to set
-            # EMPTY_RELATION_TREE. This will avoid allocating multiple empty
-            # relation trees.
-            relation_tree = EMPTY_RELATION_TREE
-            if related_objects:
-                relation_tree = related_objects
-            model._meta.__dict__['_relation_tree'] = relation_tree
+            model._meta.__dict__['_relation_tree'] = related_objects
+        # It seems it is possible that self is not in all_models, so guard
+        # against that with default for get().
+        return self.__dict__.get('_relation_tree', EMPTY_RELATION_TREE)
 
     @cached_property
     def _relation_tree(self):
-        # If cache is not present, populate the cache
-        self._populate_directed_relation_graph()
-        # It may happen, often when the registry is not ready, that a not yet
-        # registered model is queried. In this very rare case we simply return
-        # an EMPTY_RELATION_TREE. When the registry will be ready, cache will
-        # be flushed and this model will be computed properly.
-        return self.__dict__.get('_relation_tree', EMPTY_RELATION_TREE)
+        return self._populate_directed_relation_graph()
 
     def _expire_cache(self, forward=True, reverse=True):
         # This method is usually called by apps.cache_clear(), when the
@@ -744,16 +735,40 @@ class Options(object):
         - include_hidden:  include fields that have a related_name that
                            starts with a "+"
         """
+        if include_parents is False:
+            include_parents = PROXY_PARENTS
         return self._get_fields(include_parents=include_parents, include_hidden=include_hidden)
 
     def _get_fields(self, forward=True, reverse=True, include_parents=True, include_hidden=False,
-                    export_ordered_set=False):
+                    seen_models=None):
+        """
+        Internal helper function to return fields of the model.
+        * If forward=True, then fields defined on this model are returned.
+        * If reverse=True, then relations pointing to this model are returned.
+        * If include_hidden=True, then fields with is_hidden=True are returned.
+        * The include_parents argument toggles if fields from parent models
+          should be included. It has three values: True, False, and
+          PROXY_PARENTS. When set to PROXY_PARENTS, the call will return all
+          fields defined for the current model or any of its parents in the
+          parent chain to the model's concrete model.
+        """
+        if include_parents not in (True, False, PROXY_PARENTS):
+            raise TypeError("Invalid argument for include_parents: %s" % (include_parents,))
         # This helper function is used to allow recursion in ``get_fields()``
         # implementation and to provide a fast way for Django's internals to
         # access specific subsets of fields.
 
+        # We must keep track of which models we have already seen. Otherwise we
+        # could include the same field multiple times from different models.
+        topmost_call = False
+        if seen_models is None:
+            seen_models = set()
+            topmost_call = True
+        seen_models.add(self.model)
+
         # Creates a cache key composed of all arguments
-        cache_key = (forward, reverse, include_parents, include_hidden, export_ordered_set)
+        cache_key = (forward, reverse, include_parents, include_hidden, topmost_call)
+
         try:
             # In order to avoid list manipulation. Always return a shallow copy
             # of the results.
@@ -761,76 +776,54 @@ class Options(object):
         except KeyError:
             pass
 
-        # Using an OrderedDict preserves the order of insertion. This is
-        # important when displaying a ModelForm or the contrib.admin panel
-        # and no specific ordering is provided.
-        fields = OrderedDict()
-        options = {
-            'include_parents': include_parents,
-            'include_hidden': include_hidden,
-            'export_ordered_set': True,
-        }
-
-        # Abstract models cannot hold reverse fields.
-        if reverse and not self.abstract:
-            if include_parents:
-                parent_list = self.get_parent_list()
-                # Recursively call _get_fields() on each parent, with the same
-                # options provided in this call.
-                for parent in self.parents:
-                    for obj, _ in six.iteritems(parent._meta._get_fields(forward=False, **options)):
-                        if obj.many_to_many:
-                            # In order for a reverse ManyToManyRel object to be
-                            # valid, its creation counter must be > 0 and must
-                            # be in the parent list.
-                            if not (obj.field.creation_counter < 0 and obj.related_model not in parent_list):
-                                fields[obj] = True
-
-                        elif not ((obj.field.creation_counter < 0 or obj.field.rel.parent_link)
-                                  and obj.related_model not in parent_list):
-                            fields[obj] = True
-
+        fields = []
+        # Recursively call _get_fields() on each parent, with the same
+        # options provided in this call.
+        if include_parents is not False:
+            for parent in self.parents:
+                # In diamond inheritance it is possible that we see the same
+                # model from two different routes. In that case, avoid adding
+                # fields from the same parent again.
+                if parent in seen_models:
+                    continue
+                if (parent._meta.concrete_model != self.concrete_model and
+                        include_parents == PROXY_PARENTS):
+                    continue
+                for obj in parent._meta._get_fields(
+                        forward=forward, reverse=reverse, include_parents=include_parents,
+                        include_hidden=include_hidden, seen_models=seen_models):
+                    if hasattr(obj, 'parent_link') and obj.parent_link:
+                        continue
+                    fields.append(obj)
+        if reverse:
             # Tree is computed once and cached until the app cache is expired.
             # It is composed of a list of fields pointing to the current model
-            # from other models. If the model is a proxy model, then we also
-            # add the concrete model.
-            all_fields = (
-                self._relation_tree if not self.proxy else
-                chain(self._relation_tree, self.concrete_model._meta._relation_tree)
-            )
-
-            # Pull out all related objects from forward fields
-            for field in (f.rel for f in all_fields):
+            # from other models.
+            all_fields = self._relation_tree
+            for field in all_fields:
                 # If hidden fields should be included or the relation is not
                 # intentionally hidden, add to the fields dict.
-                if include_hidden or not field.hidden:
-                    fields[field] = True
+                if include_hidden or not field.rel.hidden:
+                    fields.append(field.rel)
+
         if forward:
-            if include_parents:
-                for parent in self.parents:
-                    # Add the forward fields of each parent.
-                    fields.update(parent._meta._get_fields(reverse=False, **options))
-            fields.update(
-                (field, True,)
-                for field in chain(self.local_fields, self.local_many_to_many)
+            fields.extend(
+                field for field in chain(self.local_fields, self.local_many_to_many)
             )
+            # Virtual fields are recopied to each child model, and they get a
+            # different model as field.model in each child. Hence we have to
+            # add the virtual fields separately from the topmost call. If we
+            # did this recursively similar to local_fields, we would get field
+            # instances with field.model != self.model.
+            if topmost_call:
+                fields.extend(
+                    f for f in self.virtual_fields
+                )
 
-        if not export_ordered_set:
-            # By default, fields contains field instances as keys and all
-            # possible names if the field instance as values. When
-            # _get_fields() is called, we only want to return field instances,
-            # so we just preserve the keys.
-            fields = list(fields.keys())
-
-            # Virtual fields are not inheritable, therefore they are inserted
-            # only when the recursive _get_fields() call comes to an end.
-            if forward:
-                fields.extend(self.virtual_fields)
-            fields = make_immutable_fields_list("get_fields()", fields)
+        # In order to avoid list manipulation. Always
+        # return a shallow copy of the results
+        fields = make_immutable_fields_list("get_fields()", fields)
 
         # Store result into cache for later access
         self._get_fields_cache[cache_key] = fields
-
-        # In order to avoid list manipulation. Always
-        # return a shallow copy of the results
         return fields
diff --git a/tests/model_meta/tests.py b/tests/model_meta/tests.py
index ef526b7bc4..c77a286308 100644
--- a/tests/model_meta/tests.py
+++ b/tests/model_meta/tests.py
@@ -228,11 +228,9 @@ class RelationTreeTests(TestCase):
             sorted([field.related_query_name() for field in Relation._meta._relation_tree
                    if not field.rel.field.rel.is_hidden()]),
             sorted([
-                'fk_abstract_rel', 'fk_abstract_rel', 'fk_abstract_rel', 'fk_base_rel', 'fk_base_rel',
-                'fk_base_rel', 'fk_concrete_rel', 'fk_concrete_rel', 'fo_abstract_rel', 'fo_abstract_rel',
-                'fo_abstract_rel', 'fo_base_rel', 'fo_base_rel', 'fo_base_rel', 'fo_concrete_rel',
-                'fo_concrete_rel', 'm2m_abstract_rel', 'm2m_abstract_rel', 'm2m_abstract_rel',
-                'm2m_base_rel', 'm2m_base_rel', 'm2m_base_rel', 'm2m_concrete_rel', 'm2m_concrete_rel',
+                'fk_abstract_rel', 'fk_base_rel', 'fk_concrete_rel', 'fo_abstract_rel',
+                'fo_base_rel', 'fo_concrete_rel', 'm2m_abstract_rel',
+                'm2m_base_rel', 'm2m_concrete_rel'
             ])
         )
         # Testing hidden related objects
@@ -243,9 +241,8 @@ class RelationTreeTests(TestCase):
                 'BasePerson_following_base+', 'BasePerson_following_base+', 'BasePerson_friends_abstract+',
                 'BasePerson_friends_abstract+', 'BasePerson_friends_base+', 'BasePerson_friends_base+',
                 'BasePerson_m2m_abstract+', 'BasePerson_m2m_base+', 'Relating_basepeople+',
-                'Relating_basepeople_hidden+', 'followers_abstract', 'followers_abstract', 'followers_abstract',
-                'followers_base', 'followers_base', 'followers_base', 'friends_abstract_rel_+', 'friends_abstract_rel_+',
-                'friends_abstract_rel_+', 'friends_base_rel_+', 'friends_base_rel_+', 'friends_base_rel_+', 'person',
+                'Relating_basepeople_hidden+', 'followers_abstract',
+                'followers_base', 'friends_abstract_rel_+', 'friends_base_rel_+',
                 'person', 'relating_basepeople', 'relating_baseperson',
             ])
         )
diff --git a/tests/proxy_models/models.py b/tests/proxy_models/models.py
index 90a542ffac..463fd2cd5b 100644
--- a/tests/proxy_models/models.py
+++ b/tests/proxy_models/models.py
@@ -158,7 +158,7 @@ class ProxyTrackerUser(TrackerUser):
 @python_2_unicode_compatible
 class Issue(models.Model):
     summary = models.CharField(max_length=255)
-    assignee = models.ForeignKey(TrackerUser)
+    assignee = models.ForeignKey(ProxyTrackerUser)
 
     def __str__(self):
         return ':'.join((self.__class__.__name__, self.summary,))
diff --git a/tests/proxy_models/tests.py b/tests/proxy_models/tests.py
index 807200924e..2f423a9c34 100644
--- a/tests/proxy_models/tests.py
+++ b/tests/proxy_models/tests.py
@@ -3,7 +3,7 @@ from __future__ import unicode_literals
 from django.apps import apps
 from django.contrib import admin
 from django.contrib.contenttypes.models import ContentType
-from django.core import checks, management
+from django.core import checks, exceptions, management
 from django.db import DEFAULT_DB_ALIAS, models
 from django.db.models import signals
 from django.test import TestCase, override_settings
@@ -327,8 +327,18 @@ class ProxyModelTests(TestCase):
         resp = StateProxy.objects.select_related().get(name='New South Wales')
         self.assertEqual(resp.name, 'New South Wales')
 
+    def test_filter_proxy_relation_reverse(self):
+        tu = TrackerUser.objects.create(
+            name='Contributor', status='contrib')
+        with self.assertRaises(exceptions.FieldError):
+            TrackerUser.objects.filter(issue=None),
+        self.assertQuerysetEqual(
+            ProxyTrackerUser.objects.filter(issue=None),
+            [tu], lambda x: x
+        )
+
     def test_proxy_bug(self):
-        contributor = TrackerUser.objects.create(name='Contributor',
+        contributor = ProxyTrackerUser.objects.create(name='Contributor',
             status='contrib')
         someone = BaseUser.objects.create(name='Someone')
         Bug.objects.create(summary='fix this', version='1.1beta',