diff --git a/django/db/models/base.py b/django/db/models/base.py index 1ea0f619b7..121626ca93 100644 --- a/django/db/models/base.py +++ b/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) diff --git a/django/db/models/manager.py b/django/db/models/manager.py index 96bd53f240..683a8f8d10 100644 --- a/django/db/models/manager.py +++ b/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 ####################### diff --git a/django/db/models/options.py b/django/db/models/options.py index cee45d2abd..6e14db2f94 100644 --- a/django/db/models/options.py +++ b/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 diff --git a/docs/topics/db/managers.txt b/docs/topics/db/managers.txt index dabe0d6a0b..f7f0f25a32 100644 --- a/docs/topics/db/managers.txt +++ b/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 `: + + 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. + diff --git a/tests/regressiontests/managers_regress/__init__.py b/tests/regressiontests/managers_regress/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/regressiontests/managers_regress/models.py b/tests/regressiontests/managers_regress/models.py new file mode 100644 index 0000000000..41f6ec39a1 --- /dev/null +++ b/tests/regressiontests/managers_regress/models.py @@ -0,0 +1,153 @@ +""" +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.manager2.all() +[] +>>> Child1._default_manager.all() +[] + +>>> Child2._default_manager.all() +[] +>>> Child2.restricted.all() +[] + +>>> Child3._default_manager.all() +[] +>>> Child3.manager1.all() +[] +>>> Child3.manager2.all() +[] + +# 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.manager1.all() +[, ] + +>>> Child5._default_manager.all() +[] + +>>> Child6._default_manager.all() +[] + +>>> Child7._default_manager.order_by('name') +[, ] + +"""}