From 3a47d42fa33012b2156bf04058d933df6b3082d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Bistuer?= Date: Fri, 19 Feb 2016 02:27:55 +0700 Subject: [PATCH] Fixed #20932, #25897 -- Streamlined manager inheritance. --- django/db/migrations/state.py | 3 +- django/db/models/base.py | 33 +------- django/db/models/manager.py | 136 +++++++++++-------------------- django/db/models/options.py | 48 ++++++----- docs/topics/db/managers.txt | 39 ++++----- docs/topics/db/models.txt | 32 ++------ tests/auth_tests/test_basic.py | 13 --- tests/managers_regress/models.py | 4 +- tests/managers_regress/tests.py | 2 +- tests/many_to_one/tests.py | 4 +- tests/migrations/test_state.py | 8 +- tests/one_to_one/tests.py | 8 +- 12 files changed, 112 insertions(+), 218 deletions(-) diff --git a/django/db/migrations/state.py b/django/db/migrations/state.py index 0134acde725..ca44a39ca36 100644 --- a/django/db/migrations/state.py +++ b/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) diff --git a/django/db/models/base.py b/django/db/models/base.py index 19dca3926d1..f7735985dd3 100644 --- a/django/db/models/base.py +++ b/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 diff --git a/django/db/models/manager.py b/django/db/models/manager.py index 4636869a12c..b8aa295ba38 100644 --- a/django/db/models/manager.py +++ b/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 + if cls._meta.abstract: + raise AttributeError("Manager isn't available; %s is abstract" % ( + cls._meta.object_name, + )) -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.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): diff --git a/django/db/models/options.py b/django/db/models/options.py index 74a362e99cf..389ba7c93e1 100644 --- a/django/db/models/options.py +++ b/django/db/models/options.py @@ -1,5 +1,6 @@ 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): """ diff --git a/docs/topics/db/managers.txt b/docs/topics/db/managers.txt index a82b78e1a0b..80598f0c982 100644 --- a/docs/topics/db/managers.txt +++ b/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 `: -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. + and so on). -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. +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 diff --git a/docs/topics/db/models.txt b/docs/topics/db/models.txt index 1dc1b17202c..69562f6544d 100644 --- a/docs/topics/db/models.txt +++ b/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. +model's ``Meta`` class. -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 -` 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 +With careful setting of :attr:`Meta.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 ` has more -details about this latter case. +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. -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: +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``. diff --git a/tests/auth_tests/test_basic.py b/tests/auth_tests/test_basic.py index 87122d92297..caefa9c7a06 100644 --- a/tests/auth_tests/test_basic.py +++ b/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" diff --git a/tests/managers_regress/models.py b/tests/managers_regress/models.py index bc2fa91c6ae..b39e69b9add 100644 --- a/tests/managers_regress/models.py +++ b/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 diff --git a/tests/managers_regress/tests.py b/tests/managers_regress/tests.py index 746a59a9bbe..3a51ff75428 100644 --- a/tests/managers_regress/tests.py +++ b/tests/managers_regress/tests.py @@ -50,7 +50,7 @@ class ManagersRegressionTests(TestCase): ]) self.assertQuerysetEqual(Child4.manager1.all(), ["", ""], ordered=False) self.assertQuerysetEqual(Child5._default_manager.all(), [""]) - self.assertQuerysetEqual(Child6._default_manager.all(), [""]) + self.assertQuerysetEqual(Child6._default_manager.all(), ["", ""], ordered=False) self.assertQuerysetEqual( Child7._default_manager.order_by('name'), ["", ""] diff --git a/tests/many_to_one/tests.py b/tests/many_to_one/tests.py index c5a0e3f8f3e..eaa01bbfcbd 100644 --- a/tests/many_to_one/tests.py +++ b/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 diff --git a/tests/migrations/test_state.py b/tests/migrations/test_state.py index aae2b71aac2..f2e91267e90 100644 --- a/tests/migrations/test_state.py +++ b/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): diff --git a/tests/one_to_one/tests.py b/tests/one_to_one/tests.py index 074a95f1b5f..ecb009f9612 100644 --- a/tests/one_to_one/tests.py +++ b/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