Fixed #7154 -- Inherit all model managers from abstract base classes.

Also added documentation describing how manager inheritance works (and when
manager aren't inherited). Based on some patches from sebastian_noack and
emulbreh.


git-svn-id: http://code.djangoproject.com/svn/django/trunk@8851 bcc190cf-cafb-0310-a4f2-bffc1f526a37

django/db/models/base.py

@@ -67,11 +67,7 @@ class ModelBase(type):
                 if not hasattr(meta, 'get_latest_by'):
                     new_class._meta.get_latest_by = base_meta.get_latest_by
 
-        old_default_mgr = None
         if getattr(new_class, '_default_manager', None):
-            # We have a parent who set the default manager.
-            if new_class._default_manager.model._meta.abstract:
-                old_default_mgr = new_class._default_manager
             new_class._default_manager = None
 
         # Bail out early if we have already created this class.
@@ -111,6 +107,14 @@ class ModelBase(type):
                                 % (field.name, name, base.__name__))
                     new_class.add_to_class(field.name, copy.deepcopy(field))
 
+            # Inherit managers from the abstract base classes.
+            base_managers = base._meta.abstract_managers
+            base_managers.sort()
+            for _, mgr_name, manager in base_managers:
+                val = getattr(new_class, mgr_name, None)
+                if not val or val is manager:
+                    new_manager = manager._copy_to_model(new_class)
+                    new_class.add_to_class(mgr_name, new_manager)
         if abstract:
             # Abstract base models can't be instantiated and don't appear in
             # the list of models for an app. We do the final setup for them a
@@ -119,8 +123,6 @@ class ModelBase(type):
             new_class.Meta = attr_meta
             return new_class
 
-        if old_default_mgr and not new_class._default_manager:
-            new_class._default_manager = old_default_mgr._copy_to_model(new_class)
         new_class._prepare()
         register_models(new_class._meta.app_label, new_class)
 

django/db/models/manager.py

@@ -23,10 +23,9 @@ class Manager(object):
 
     def __init__(self):
         super(Manager, self).__init__()
-        # Increase the creation counter, and save our local copy.
-        self.creation_counter = Manager.creation_counter
-        Manager.creation_counter += 1
+        self._set_creation_counter()
         self.model = None
+        self._inherited = False
 
     def contribute_to_class(self, model, name):
         # TODO: Use weakref because of possible memory leak / circular reference.
@@ -34,6 +33,17 @@ class Manager(object):
         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
+        if model._meta.abstract or self._inherited:
+            model._meta.abstract_managers.append((self.creation_counter, name,
+                    self))
+
+    def _set_creation_counter(self):
+        """
+        Sets the creation counter value for this instance and increments the
+        class-level copy.
+        """
+        self.creation_counter = Manager.creation_counter
+        Manager.creation_counter += 1
 
     def _copy_to_model(self, model):
         """
@@ -43,7 +53,9 @@ class Manager(object):
         """
         assert issubclass(model, self.model)
         mgr = copy.copy(self)
+        mgr._set_creation_counter()
         mgr.model = model
+        mgr._inherited = True
         return mgr
 
     #######################

django/db/models/options.py

@@ -44,6 +44,9 @@ class Options(object):
         self.abstract = False
         self.parents = SortedDict()
         self.duplicate_targets = {}
+        # Managers that have been inherited from abstract base classes. These
+        # are passed onto any children.
+        self.abstract_managers = []
 
     def contribute_to_class(self, cls, name):
         from django.db import connection

docs/topics/db/managers.txt

@@ -189,3 +189,35 @@ attributes by giving it a ``use_for_related_fields`` property::
         
 
         ...
+
+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
+: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
+       class override all others; then come names on the first parent class,
+       and so on). Abstract base classes are designed to capture information
+       and behaviour 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.
+

tests/regressiontests/managers_regress/models.py

+"""
+Various edge-cases for model managers.
+"""
+
+from django.db import models
+
+class OnlyFred(models.Manager):
+    def get_query_set(self):
+        return super(OnlyFred, self).get_query_set().filter(name='fred')
+
+class OnlyBarney(models.Manager):
+    def get_query_set(self):
+        return super(OnlyBarney, self).get_query_set().filter(name='barney')
+
+class Value42(models.Manager):
+    def get_query_set(self):
+        return super(Value42, self).get_query_set().filter(value=42)
+
+class AbstractBase1(models.Model):
+    name = models.CharField(max_length=50)
+
+    class Meta:
+        abstract = True
+
+    # Custom managers
+    manager1 = OnlyFred()
+    manager2 = OnlyBarney()
+    objects = models.Manager()
+
+class AbstractBase2(models.Model):
+    value = models.IntegerField()
+
+    class Meta:
+        abstract = True
+
+    # Custom manager
+    restricted = Value42()
+
+# No custom manager on this class to make sure the default case doesn't break.
+class AbstractBase3(models.Model):
+    comment = models.CharField(max_length=50)
+
+    class Meta:
+        abstract = True
+
+class Parent(models.Model):
+    name = models.CharField(max_length=50)
+
+    manager = OnlyFred()
+
+    def __unicode__(self):
+        return self.name
+
+# Managers from base classes are inherited and, if no manager is specified
+# *and* the parent has a manager specified, the first one (in the MRO) will
+# become the default.
+class Child1(AbstractBase1):
+    data = models.CharField(max_length=25)
+
+    def __unicode__(self):
+        return self.data
+
+class Child2(AbstractBase1, AbstractBase2):
+    data = models.CharField(max_length=25)
+
+    def __unicode__(self):
+        return self.data
+
+class Child3(AbstractBase1, AbstractBase3):
+    data = models.CharField(max_length=25)
+
+    def __unicode__(self):
+        return self.data
+
+class Child4(AbstractBase1):
+    data = models.CharField(max_length=25)
+
+    # Should be the default manager, although the parent managers are
+    # inherited.
+    default = models.Manager()
+
+    def __unicode__(self):
+        return self.data
+
+class Child5(AbstractBase3):
+    name = models.CharField(max_length=25)
+
+    default = OnlyFred()
+    objects = models.Manager()
+
+    def __unicode__(self):
+        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
+
+__test__ = {"API_TESTS": """
+>>> a1 = Child1.objects.create(name='fred', data='a1')
+>>> a2 = Child1.objects.create(name='barney', data='a2')
+>>> b1 = Child2.objects.create(name='fred', data='b1', value=1)
+>>> b2 = Child2.objects.create(name='barney', data='b2', value=42)
+>>> c1 = Child3.objects.create(name='fred', data='c1', comment='yes')
+>>> c2 = Child3.objects.create(name='barney', data='c2', comment='no')
+>>> d1 = Child4.objects.create(name='fred', data='d1')
+>>> d2 = Child4.objects.create(name='barney', data='d2')
+>>> e1 = Child5.objects.create(name='fred', comment='yes')
+>>> e2 = Child5.objects.create(name='barney', comment='no')
+>>> f1 = Child6.objects.create(name='fred', data='f1', value=42)
+>>> f2 = Child6.objects.create(name='barney', data='f2', value=42)
+>>> g1 = Child7.objects.create(name='fred')
+>>> g2 = Child7.objects.create(name='barney')
+
+>>> Child1.manager1.all()
+[<Child1: a1>]
+>>> Child1.manager2.all()
+[<Child1: a2>]
+>>> Child1._default_manager.all()
+[<Child1: a1>]
+
+>>> Child2._default_manager.all()
+[<Child2: b1>]
+>>> Child2.restricted.all()
+[<Child2: b2>]
+
+>>> Child3._default_manager.all()
+[<Child3: c1>]
+>>> Child3.manager1.all()
+[<Child3: c1>]
+>>> Child3.manager2.all()
+[<Child3: c2>]
+
+# Since Child6 inherits from Child4, the corresponding rows from f1 and f2 also
+# appear here. This is the expected result.
+>>> Child4._default_manager.order_by('data')
+[<Child4: d1>, <Child4: d2>, <Child4: f1>, <Child4: f2>]
+>>> Child4.manager1.all()
+[<Child4: d1>, <Child4: f1>]
+
+>>> Child5._default_manager.all()
+[<Child5: fred>]
+
+>>> Child6._default_manager.all()
+[<Child6: f1>]
+
+>>> Child7._default_manager.order_by('name')
+[<Child7: barney>, <Child7: fred>]
+
+"""}