Fixed #20932, #25897 -- Streamlined manager inheritance.

django/db/migrations/state.py

@@ -500,8 +500,7 @@ class ModelState(object):
             else:
                 # Force this manager to be the first and thus default
                 managers_mapping[default_manager_name] = (0, models.Manager())
-            # Sort all managers by their creation counter
-            for _, manager, _ in sorted(model._meta.managers):
+            for manager in model._meta.managers:
                 if manager.name == "_base_manager" or not manager.use_in_migrations:
                     continue
                 reconstruct_manager(manager)

django/db/models/base.py

@@ -151,18 +151,6 @@ class ModelBase(type):
         if is_proxy and base_meta and base_meta.swapped:
             raise TypeError("%s cannot proxy the swapped model '%s'." % (name, base_meta.swapped))
 
-        if getattr(new_class, '_default_manager', None):
-            if not is_proxy:
-                # Multi-table inheritance doesn't inherit default manager from
-                # parents.
-                new_class._default_manager = None
-                new_class._base_manager = None
-            else:
-                # Proxy classes do inherit parent's default manager, if none is
-                # set explicitly.
-                new_class._default_manager = new_class._default_manager._copy_to_model(new_class)
-                new_class._base_manager = new_class._base_manager._copy_to_model(new_class)
-
         # Add all attributes to the class.
         for obj_name, obj in attrs.items():
             new_class.add_to_class(obj_name, obj)
@@ -217,7 +205,6 @@ class ModelBase(type):
         inherited_attributes = set()
         # Do the appropriate setup for any model parents.
         for base in new_class.mro():
-            original_base = base
             if base not in parents or not hasattr(base, '_meta'):
                 # Things without _meta aren't functional models, so they're
                 # uninteresting parents.
@@ -294,14 +281,6 @@ class ModelBase(type):
                 # Pass any non-abstract parent classes onto child.
                 new_class._meta.parents.update(base_parents)
 
-            # Inherit managers from the abstract base classes.
-            new_class.copy_managers(base._meta.abstract_managers)
-
-            # Proxy models inherit the non-abstract managers from their base,
-            # unless they have redefined any of them.
-            if is_proxy:
-                new_class.copy_managers(original_base._meta.concrete_managers)
-
             # Inherit private fields (like GenericForeignKey) from the parent
             # class
             for field in base._meta.private_fields:
@@ -330,15 +309,6 @@ class ModelBase(type):
         new_class._meta.apps.register_model(new_class._meta.app_label, new_class)
         return new_class
 
-    def copy_managers(cls, base_managers):
-        # This is in-place sorting of an Options attribute, but that's fine.
-        base_managers.sort()
-        for _, mgr_name, manager in base_managers:  # NOQA (redefinition of _)
-            val = getattr(cls, mgr_name, None)
-            if not val or val is manager:
-                new_manager = manager._copy_to_model(cls)
-                cls.add_to_class(mgr_name, new_manager)
-
     def add_to_class(cls, name, value):
         # We should call the contribute_to_class method only if it's bound
         if not inspect.isclass(value) and hasattr(value, 'contribute_to_class'):
@@ -376,6 +346,7 @@ class ModelBase(type):
             setattr(cls, 'get_absolute_url', get_absolute_url_override)
 
         ensure_default_manager(cls)
+
         signals.class_prepared.send(sender=cls)
 
 
@@ -1263,7 +1234,7 @@ class Model(six.with_metaclass(ModelBase)):
         """ Perform all manager checks. """
 
         errors = []
-        for __, manager, __ in cls._meta.managers:
+        for manager in cls._meta.managers:
             errors.extend(manager.check(**kwargs))
         return errors
 

django/db/models/manager.py

@@ -8,43 +8,40 @@ from django.utils import six
 from django.utils.encoding import python_2_unicode_compatible
 
 
-def ensure_default_manager(cls):
+def can_use_for_related_field(manager_class):
+    return manager_class is Manager or getattr(manager_class, 'use_for_related_fields', False)
+
+
+def ensure_default_manager(model):
     """
-    Ensures that a Model subclass contains a default manager  and sets the
-    _default_manager attribute on the class. Also sets up the _base_manager
-    points to a plain Manager instance (which could be the same as
-    _default_manager if it's not a subclass of Manager).
+    Ensures that a Model subclass contains a default manager and sets the
+    _default_manager and _base_manager attributes on the class.
     """
-    if cls._meta.swapped:
-        setattr(cls, 'objects', SwappedManagerDescriptor(cls))
-        return
-    if not getattr(cls, '_default_manager', None):
-        if any(f.name == 'objects' for f in cls._meta.fields):
+
+    if not model._meta.managers:
+        if any(f.name == 'objects' for f in model._meta.fields):
             raise ValueError(
                 "Model %s must specify a custom Manager, because it has a "
-                "field named 'objects'" % cls.__name__
+                "field named 'objects'" % model.__name__
             )
-        # Create the default manager, if needed.
-        cls.add_to_class('objects', Manager())
-        cls._base_manager = cls.objects
-    elif not getattr(cls, '_base_manager', None):
-        default_mgr = cls._default_manager.__class__
-        if (default_mgr is Manager or
-                getattr(default_mgr, "use_for_related_fields", False)):
-            cls._base_manager = cls._default_manager
+        model.add_to_class('objects', Manager())
+
+    model._default_manager = model._meta.managers[0]
+
+    # Just alias _base_manager if default manager is suitable.
+    if can_use_for_related_field(model._default_manager.__class__):
+        model._base_manager = model._default_manager
+
+    # Otherwise search for a suitable manager type in the default manager MRO.
+    else:
+        for base_manager_class in model._default_manager.__class__.mro()[1:]:
+            if can_use_for_related_field(base_manager_class):
+                model._base_manager = base_manager_class()
+                model._base_manager.name = '_base_manager'
+                model._base_manager.model = model
+                break
         else:
-            # Default manager isn't a plain Manager class, or a suitable
-            # replacement, so we walk up the base class hierarchy until we hit
-            # something appropriate.
-            for base_class in default_mgr.mro()[1:]:
-                if (base_class is Manager or
-                        getattr(base_class, "use_for_related_fields", False)):
-                    cls.add_to_class('_base_manager', base_class())
-                    return
-            raise AssertionError(
-                "Should never get here. Please report a bug, including your "
-                "model and model manager setup."
-            )
+            raise ValueError("Could not find a suitable base manager.")
 
 
 @python_2_unicode_compatible
@@ -67,7 +64,6 @@ class BaseManager(object):
         self._set_creation_counter()
         self.model = None
         self.name = None
-        self._inherited = False
         self._db = None
         self._hints = {}
 
@@ -150,26 +146,13 @@ class BaseManager(object):
         return type(class_name, (cls,), class_dict)
 
     def contribute_to_class(self, model, name):
-        # TODO: Use weakref because of possible memory leak / circular reference.
-        self.model = model
         if not self.name:
             self.name = name
-        # Only contribute the manager if the model is concrete
-        if model._meta.abstract:
-            setattr(model, name, AbstractManagerDescriptor(model))
-        elif model._meta.swapped:
-            setattr(model, name, SwappedManagerDescriptor(model))
-        else:
-            # if not model._meta.abstract and not model._meta.swapped:
-            setattr(model, name, ManagerDescriptor(self))
-        if (not getattr(model, '_default_manager', None) or
-                self.creation_counter < model._default_manager.creation_counter):
-            model._default_manager = self
+        self.model = model
 
-        abstract = False
-        if model._meta.abstract or (self._inherited and not self.model._meta.proxy):
-            abstract = True
-        model._meta.managers.append((self.creation_counter, self, abstract))
+        setattr(model, name, ManagerDescriptor(self))
+
+        model._meta.add_manager(self)
 
     def _set_creation_counter(self):
         """
@@ -179,19 +162,6 @@ class BaseManager(object):
         self.creation_counter = BaseManager.creation_counter
         BaseManager.creation_counter += 1
 
-    def _copy_to_model(self, model):
-        """
-        Makes a copy of the manager and assigns it to 'model', which should be
-        a child of the existing model (used when inheriting a manager from an
-        abstract base class).
-        """
-        assert issubclass(model, self.model)
-        mgr = copy.copy(self)
-        mgr._set_creation_counter()
-        mgr.model = model
-        mgr._inherited = True
-        return mgr
-
     def db_manager(self, using=None, hints=None):
         obj = copy.copy(self)
         obj._db = using or self._db
@@ -240,43 +210,29 @@ class Manager(BaseManager.from_queryset(QuerySet)):
 
 
 class ManagerDescriptor(object):
-    # This class ensures managers aren't accessible via model instances.
-    # For example, Poll.objects works, but poll_obj.objects raises AttributeError.
+
     def __init__(self, manager):
         self.manager = manager
 
     def __get__(self, instance, cls=None):
         if instance is not None:
             raise AttributeError("Manager isn't accessible via %s instances" % cls.__name__)
-        return self.manager
-
 
-class AbstractManagerDescriptor(object):
-    # This class provides a better error message when you try to access a
-    # manager on an abstract model.
-    def __init__(self, model):
-        self.model = model
-
-    def __get__(self, instance, cls=None):
-        raise AttributeError("Manager isn't available; %s is abstract" % (
-            self.model._meta.object_name,
-        ))
-
-
-class SwappedManagerDescriptor(object):
-    # This class provides a better error message when you try to access a
-    # manager on a swapped model.
-    def __init__(self, model):
-        self.model = model
-
-    def __get__(self, instance, cls=None):
-        raise AttributeError(
-            "Manager isn't available; '%s.%s' has been swapped for '%s'" % (
-                self.model._meta.app_label,
-                self.model._meta.object_name,
-                self.model._meta.swapped,
+        if cls._meta.abstract:
+            raise AttributeError("Manager isn't available; %s is abstract" % (
+                cls._meta.object_name,
+            ))
+
+        if cls._meta.swapped:
+            raise AttributeError(
+                "Manager isn't available; '%s.%s' has been swapped for '%s'" % (
+                    cls._meta.app_label,
+                    cls._meta.object_name,
+                    cls._meta.swapped,
+                )
             )
-        )
+
+        return cls._meta.managers_map[self.manager.name]
 
 
 class EmptyManager(Manager):

django/db/models/options.py

 from __future__ import unicode_literals
 
+import copy
 import warnings
 from bisect import bisect
 from collections import OrderedDict, defaultdict
@@ -73,7 +74,8 @@ def make_immutable_fields_list(name, data):
 @python_2_unicode_compatible
 class Options(object):
     FORWARD_PROPERTIES = {'fields', 'many_to_many', 'concrete_fields',
-                          'local_concrete_fields', '_forward_fields_map'}
+                          'local_concrete_fields', '_forward_fields_map',
+                          'managers', 'managers_map'}
     REVERSE_PROPERTIES = {'related_objects', 'fields_map', '_relation_tree'}
 
     default_apps = apps
@@ -83,6 +85,7 @@ class Options(object):
         self.local_fields = []
         self.local_many_to_many = []
         self.private_fields = []
+        self.local_managers = []
         self.model_name = None
         self.verbose_name = None
         self.verbose_name_plural = None
@@ -122,12 +125,6 @@ class Options(object):
         self.parents = OrderedDict()
         self.auto_created = False
 
-        # To handle various inheritance situations, we need to track where
-        # managers came from (concrete or abstract base classes). `managers`
-        # keeps a list of 3-tuples of the form:
-        # (creation_counter, instance, abstract(=True))
-        self.managers = []
-
         # List of all lookups defined in ForeignKey 'limit_choices_to' options
         # from *other* models. Needed for some admin checks. Internal use only.
         self.related_fkey_lookups = []
@@ -154,20 +151,6 @@ class Options(object):
     def installed(self):
         return self.app_config is not None
 
-    @property
-    def abstract_managers(self):
-        return [
-            (counter, instance.name, instance) for counter, instance, abstract
-            in self.managers if abstract
-        ]
-
-    @property
-    def concrete_managers(self):
-        return [
-            (counter, instance.name, instance) for counter, instance, abstract
-            in self.managers if not abstract
-        ]
-
     def contribute_to_class(self, cls, name):
         from django.db import connection
         from django.db.backends.utils import truncate_name
@@ -264,6 +247,10 @@ class Options(object):
                 auto = AutoField(verbose_name='ID', primary_key=True, auto_created=True)
                 model.add_to_class('id', auto)
 
+    def add_manager(self, manager):
+        self.local_managers.append(manager)
+        self._expire_cache()
+
     def add_field(self, field, private=False, virtual=NOT_PROVIDED):
         if virtual is not NOT_PROVIDED:
             warnings.warn(
@@ -371,6 +358,25 @@ class Options(object):
                     return swapped_for
         return None
 
+    @cached_property
+    def managers(self):
+        managers = []
+        bases = (b for b in self.model.mro() if hasattr(b, '_meta'))
+        for depth, base in enumerate(bases):
+            for manager in base._meta.local_managers:
+                manager = copy.copy(manager)
+                manager.model = self.model
+                managers.append((depth, manager.creation_counter, manager))
+
+        return make_immutable_fields_list(
+            "managers",
+            (m[2] for m in sorted(managers)),
+        )
+
+    @cached_property
+    def managers_map(self):
+        return {manager.name: manager for manager in reversed(self.managers)}
+
     @cached_property
     def fields(self):
         """

docs/topics/db/managers.txt

@@ -321,33 +321,26 @@ You may also store the generated class into a variable::
 Custom managers and model inheritance
 -------------------------------------
 
-Class inheritance and model managers aren't quite a perfect match for each
-other. Managers are often specific to the classes they are defined on and
-inheriting them in subclasses isn't necessarily a good idea. Also, because the
-first manager declared is the *default manager*, it is important to allow that
-to be controlled. So here's how Django handles custom managers and
+Here's how Django handles custom managers and
 :ref:`model inheritance <model-inheritance>`:
 
-1. Managers defined on non-abstract base classes are *not* inherited by
-   child classes. If you want to reuse a manager from a non-abstract base,
-   redeclare it explicitly on the child class. These sorts of managers are
-   likely to be fairly specific to the class they are defined on, so
-   inheriting them can often lead to unexpected results (particularly as
-   far as the default manager goes). Therefore, they aren't passed onto
-   child classes.
-
-2. Managers from abstract base classes are always inherited by the child
-   class, using Python's normal name resolution order (names on the child
+1. Managers from base classes are always inherited by the child class,
+   using Python's normal name resolution order (names on the child
    class override all others; then come names on the first parent class,
-   and so on). Abstract base classes are designed to capture information
-   and behavior that is common to their child classes. Defining common
-   managers is an appropriate part of this common information.
-
-3. The default manager on a class is either the first manager declared on
-   the class, if that exists, or the default manager of the first abstract
-   base class in the parent hierarchy, if that exists. If no default
-   manager is explicitly declared, Django's normal default manager is
-   used.
+   and so on).
+
+2. The default manager on a class is either the first manager declared on the
+   class, if that exists, or the default manager of the first parent class in
+   the parent hierarchy, if that exists. If no manager is explicitly declared,
+   Django automatically creates the `objects` manager and it becomes the default
+   manager.
+
+.. versionchanged:: 1.10
+
+    In older versions, manager inheritance varied depending on the type of
+    model inheritance (i.e. :ref:`abstract-base-classes`,
+    :ref:`multi-table-inheritance`, or :ref:`proxy-models`), especially
+    with regards to electing the default manager.
 
 These rules provide the necessary flexibility if you want to install a
 collection of custom managers on a group of models, via an abstract base

docs/topics/db/models.txt

@@ -1287,33 +1287,19 @@ Differences between proxy inheritance and unmanaged models
 
 Proxy model inheritance might look fairly similar to creating an unmanaged
 model, using the :attr:`~django.db.models.Options.managed` attribute on a
-model's ``Meta`` class. The two alternatives are not quite the same and it's
-worth considering which one you should use.
-
-One difference is that you can (and, in fact, must unless you want an empty
-model) specify model fields on models with ``Meta.managed=False``. You could,
-with careful setting of :attr:`Meta.db_table
-<django.db.models.Options.db_table>` create an unmanaged model that shadowed
-an existing model and add Python methods to it. However, that would be very
-repetitive and fragile as you need to keep both copies synchronized if you
+model's ``Meta`` class.
+
+With careful setting of :attr:`Meta.db_table
+<django.db.models.Options.db_table>` you could create an unmanaged model that
+shadows an existing model and adds Python methods to it. However, that would be
+very repetitive and fragile as you need to keep both copies synchronized if you
 make any changes.
 
-The other difference that is more important for proxy models, is how model
-managers are handled. Proxy models are intended to behave exactly like the
-model they are proxying for. So they inherit the parent model's managers,
-including the default manager. In the normal multi-table model inheritance
-case, children do not inherit managers from their parents as the custom
-managers aren't always appropriate when extra fields are involved. The
-:ref:`manager documentation <custom-managers-and-inheritance>` has more
-details about this latter case.
-
-When these two features were implemented, attempts were made to squash them
-into a single option. It turned out that interactions with inheritance, in
-general, and managers, in particular, made the API very complicated and
-potentially difficult to understand and use. It turned out that two options
-were needed in any case, so the current separation arose.
-
-So, the general rules are:
+On the other hand, proxy models are intended to behave exactly like the model
+they are proxying for. They are always in sync with the parent model since they
+directly inherit its fields and managers.
+
+The general rules are:
 
 1. If you are mirroring an existing model or database table and don't want
    all the original database table columns, use ``Meta.managed=False``.

tests/auth_tests/test_basic.py

@@ -3,29 +3,16 @@ from __future__ import unicode_literals
 
 import warnings
 
-from django.apps import apps
 from django.contrib.auth import get_user_model
 from django.contrib.auth.models import AnonymousUser, User
 from django.core.exceptions import ImproperlyConfigured
 from django.db import IntegrityError
-from django.dispatch import receiver
 from django.test import TestCase, override_settings
-from django.test.signals import setting_changed
 from django.utils import translation
 
 from .models import CustomUser
 
 
-@receiver(setting_changed)
-def user_model_swapped(**kwargs):
-    if kwargs['setting'] == 'AUTH_USER_MODEL':
-        from django.db.models.manager import ensure_default_manager
-        # Reset User manager
-        setattr(User, 'objects', User._default_manager)
-        ensure_default_manager(User)
-        apps.clear_cache()
-
-
 class BasicTestCase(TestCase):
     def test_user(self):
         "Check that users can be created and can set their password"

tests/managers_regress/models.py

@@ -115,14 +115,12 @@ class Child5(AbstractBase3):
         return self.name
 
 
-# Will inherit managers from AbstractBase1, but not Child4.
 class Child6(Child4):
     value = models.IntegerField()
 
 
-# Will not inherit default manager from parent.
 class Child7(Parent):
-    pass
+    objects = models.Manager()
 
 
 # RelatedManagers

tests/managers_regress/tests.py

@@ -50,7 +50,7 @@ class ManagersRegressionTests(TestCase):
         ])
         self.assertQuerysetEqual(Child4.manager1.all(), ["<Child4: d1>", "<Child4: f1>"], ordered=False)
         self.assertQuerysetEqual(Child5._default_manager.all(), ["<Child5: fred>"])
-        self.assertQuerysetEqual(Child6._default_manager.all(), ["<Child6: f1>"])
+        self.assertQuerysetEqual(Child6._default_manager.all(), ["<Child6: f1>", "<Child6: f2>"], ordered=False)
         self.assertQuerysetEqual(
             Child7._default_manager.order_by('name'),
             ["<Child7: barney>", "<Child7: fred>"]

tests/many_to_one/tests.py

@@ -600,13 +600,13 @@ class ManyToOneTests(TestCase):
         # If the manager is marked "use_for_related_fields", it'll get used instead
         # of the "bare" queryset. Usually you'd define this as a property on the class,
         # but this approximates that in a way that's easier in tests.
-        School.objects.use_for_related_fields = True
+        School._default_manager.use_for_related_fields = True
         try:
             private_student = Student.objects.get(pk=private_student.pk)
             with self.assertRaises(School.DoesNotExist):
                 private_student.school
         finally:
-            School.objects.use_for_related_fields = False
+            School._default_manager.use_for_related_fields = False
 
     def test_hasattr_related_object(self):
         # The exception raised on attribute access when a related object

tests/migrations/test_state.py

@@ -262,13 +262,11 @@ class StateTests(SimpleTestCase):
         self.assertEqual(len(new_apps.get_model("migrations", "SubTag")._meta.local_fields), 2)
 
         Food = new_apps.get_model("migrations", "Food")
-        managers = sorted(Food._meta.managers)
-        self.assertEqual([mgr.name for _, mgr, _ in managers],
+        self.assertEqual([mgr.name for mgr in Food._meta.managers],
                          ['default', 'food_mgr1', 'food_mgr2'])
-        self.assertTrue(all(isinstance(mgr.name, six.text_type) for _, mgr, _ in managers))
-        self.assertEqual([mgr.__class__ for _, mgr, _ in managers],
+        self.assertTrue(all(isinstance(mgr.name, six.text_type) for mgr in Food._meta.managers))
+        self.assertEqual([mgr.__class__ for mgr in Food._meta.managers],
                          [models.Manager, FoodManager, FoodManager])
-        self.assertIs(managers[0][1], Food._default_manager)
 
     def test_render_model_inheritance(self):
         class Book(models.Model):

tests/one_to_one/tests.py

@@ -457,21 +457,21 @@ class OneToOneTests(TestCase):
         # If the manager is marked "use_for_related_fields", it'll get used instead
         # of the "bare" queryset. Usually you'd define this as a property on the class,
         # but this approximates that in a way that's easier in tests.
-        School.objects.use_for_related_fields = True
+        School._default_manager.use_for_related_fields = True
         try:
             private_director = Director._base_manager.get(pk=private_director.pk)
             with self.assertRaises(School.DoesNotExist):
                 private_director.school
         finally:
-            School.objects.use_for_related_fields = False
+            School._default_manager.use_for_related_fields = False
 
-        Director.objects.use_for_related_fields = True
+        Director._default_manager.use_for_related_fields = True
         try:
             private_school = School._base_manager.get(pk=private_school.pk)
             with self.assertRaises(Director.DoesNotExist):
                 private_school.director
         finally:
-            Director.objects.use_for_related_fields = False
+            Director._default_manager.use_for_related_fields = False
 
     def test_hasattr_related_object(self):
         # The exception raised on attribute access when a related object