From cc337a74f1808b216fff96f1695d8b066d2636f6 Mon Sep 17 00:00:00 2001 From: Russell Keith-Magee Date: Sat, 6 Oct 2012 14:21:57 +0800 Subject: [PATCH] Fixed #19069 -- Improved the error message when trying to query a swapped model. Thanks to Preston Holmes for the suggestion. --- django/db/models/manager.py | 37 +++- .../managers_regress/models.py | 13 ++ .../regressiontests/managers_regress/tests.py | 168 ++++++++++++++++-- 3 files changed, 201 insertions(+), 17 deletions(-) diff --git a/django/db/models/manager.py b/django/db/models/manager.py index 522a8a2306..8da8af487c 100644 --- a/django/db/models/manager.py +++ b/django/db/models/manager.py @@ -13,7 +13,11 @@ def ensure_default_manager(sender, **kwargs): _default_manager if it's not a subclass of Manager). """ cls = sender - if cls._meta.abstract or cls._meta.swapped: + if cls._meta.abstract: + setattr(cls, 'objects', AbstractManagerDescriptor(cls)) + return + elif cls._meta.swapped: + setattr(cls, 'objects', SwappedManagerDescriptor(cls)) return if not getattr(cls, '_default_manager', None): # Create the default manager, if needed. @@ -58,7 +62,12 @@ class Manager(object): # TODO: Use weakref because of possible memory leak / circular reference. self.model = model # Only contribute the manager if the model is concrete - if not model._meta.abstract and not model._meta.swapped: + 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 @@ -224,6 +233,30 @@ class ManagerDescriptor(object): 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, type=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, type=None): + raise AttributeError("Manager isn't available; %s has been swapped for '%s'" % ( + self.model._meta.object_name, self.model._meta.swapped + )) + + class EmptyManager(Manager): def get_query_set(self): return self.get_empty_query_set() diff --git a/tests/regressiontests/managers_regress/models.py b/tests/regressiontests/managers_regress/models.py index 892505f24a..d72970d86e 100644 --- a/tests/regressiontests/managers_regress/models.py +++ b/tests/regressiontests/managers_regress/models.py @@ -10,14 +10,17 @@ 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) @@ -29,6 +32,7 @@ class AbstractBase1(models.Model): manager2 = OnlyBarney() objects = models.Manager() + class AbstractBase2(models.Model): value = models.IntegerField() @@ -38,6 +42,7 @@ class AbstractBase2(models.Model): # 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) @@ -45,6 +50,7 @@ class AbstractBase3(models.Model): class Meta: abstract = True + @python_2_unicode_compatible class Parent(models.Model): name = models.CharField(max_length=50) @@ -54,6 +60,7 @@ class Parent(models.Model): def __str__(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. @@ -64,6 +71,7 @@ class Child1(AbstractBase1): def __str__(self): return self.data + @python_2_unicode_compatible class Child2(AbstractBase1, AbstractBase2): data = models.CharField(max_length=25) @@ -71,6 +79,7 @@ class Child2(AbstractBase1, AbstractBase2): def __str__(self): return self.data + @python_2_unicode_compatible class Child3(AbstractBase1, AbstractBase3): data = models.CharField(max_length=25) @@ -78,6 +87,7 @@ class Child3(AbstractBase1, AbstractBase3): def __str__(self): return self.data + @python_2_unicode_compatible class Child4(AbstractBase1): data = models.CharField(max_length=25) @@ -89,6 +99,7 @@ class Child4(AbstractBase1): def __str__(self): return self.data + @python_2_unicode_compatible class Child5(AbstractBase3): name = models.CharField(max_length=25) @@ -99,10 +110,12 @@ class Child5(AbstractBase3): def __str__(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 diff --git a/tests/regressiontests/managers_regress/tests.py b/tests/regressiontests/managers_regress/tests.py index dd6cb66857..f3721a4c01 100644 --- a/tests/regressiontests/managers_regress/tests.py +++ b/tests/regressiontests/managers_regress/tests.py @@ -1,26 +1,42 @@ from __future__ import absolute_import +import copy +from django.conf import settings +from django.db import models +from django.db.models.loading import cache from django.test import TestCase +from django.test.utils import override_settings -from .models import Child1, Child2, Child3, Child4, Child5, Child6, Child7 +from .models import ( + Child1, + Child2, + Child3, + Child4, + Child5, + Child6, + Child7, + AbstractBase1, + AbstractBase2, + AbstractBase3, +) class ManagersRegressionTests(TestCase): def test_managers(self): - 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.objects.create(name='fred', data='a1') + Child1.objects.create(name='barney', data='a2') + Child2.objects.create(name='fred', data='b1', value=1) + Child2.objects.create(name='barney', data='b2', value=42) + Child3.objects.create(name='fred', data='c1', comment='yes') + Child3.objects.create(name='barney', data='c2', comment='no') + Child4.objects.create(name='fred', data='d1') + Child4.objects.create(name='barney', data='d2') + Child5.objects.create(name='fred', comment='yes') + Child5.objects.create(name='barney', comment='no') + Child6.objects.create(name='fred', data='f1', value=42) + Child6.objects.create(name='barney', data='f2', value=42) + Child7.objects.create(name='fred') + Child7.objects.create(name='barney') self.assertQuerysetEqual(Child1.manager1.all(), [""]) self.assertQuerysetEqual(Child1.manager2.all(), [""]) @@ -54,3 +70,125 @@ class ManagersRegressionTests(TestCase): "" ] ) + + def test_abstract_manager(self): + # Accessing the manager on an abstract model should + # raise an attribute error with an appropriate message. + try: + AbstractBase3.objects.all() + self.fail('Should raise an AttributeError') + except AttributeError as e: + # This error message isn't ideal, but if the model is abstract and + # a lot of the class instantiation logic isn't invoked; if the + # manager is implied, then we don't get a hook to install the + # error-raising manager. + self.assertEqual(str(e), "type object 'AbstractBase3' has no attribute 'objects'") + + def test_custom_abstract_manager(self): + # Accessing the manager on an abstract model with an custom + # manager should raise an attribute error with an appropriate + # message. + try: + AbstractBase2.restricted.all() + self.fail('Should raise an AttributeError') + except AttributeError as e: + self.assertEqual(str(e), "Manager isn't available; AbstractBase2 is abstract") + + def test_explicit_abstract_manager(self): + # Accessing the manager on an abstract model with an explicit + # manager should raise an attribute error with an appropriate + # message. + try: + AbstractBase1.objects.all() + self.fail('Should raise an AttributeError') + except AttributeError as e: + self.assertEqual(str(e), "Manager isn't available; AbstractBase1 is abstract") + + def test_swappable_manager(self): + try: + # This test adds dummy models to the app cache. These + # need to be removed in order to prevent bad interactions + # with the flush operation in other tests. + old_app_models = copy.deepcopy(cache.app_models) + old_app_store = copy.deepcopy(cache.app_store) + + settings.TEST_SWAPPABLE_MODEL = 'managers_regress.Parent' + + class SwappableModel(models.Model): + class Meta: + swappable = 'TEST_SWAPPABLE_MODEL' + + # Accessing the manager on a swappable model should + # raise an attribute error with a helpful message + try: + SwappableModel.objects.all() + self.fail('Should raise an AttributeError') + except AttributeError as e: + self.assertEqual(str(e), "Manager isn't available; SwappableModel has been swapped for 'managers_regress.Parent'") + + finally: + del settings.TEST_SWAPPABLE_MODEL + cache.app_models = old_app_models + cache.app_store = old_app_store + + def test_custom_swappable_manager(self): + try: + # This test adds dummy models to the app cache. These + # need to be removed in order to prevent bad interactions + # with the flush operation in other tests. + old_app_models = copy.deepcopy(cache.app_models) + old_app_store = copy.deepcopy(cache.app_store) + + settings.TEST_SWAPPABLE_MODEL = 'managers_regress.Parent' + + class SwappableModel(models.Model): + + stuff = models.Manager() + + class Meta: + swappable = 'TEST_SWAPPABLE_MODEL' + + # Accessing the manager on a swappable model with an + # explicit manager should raise an attribute error with a + # helpful message + try: + SwappableModel.stuff.all() + self.fail('Should raise an AttributeError') + except AttributeError as e: + self.assertEqual(str(e), "Manager isn't available; SwappableModel has been swapped for 'managers_regress.Parent'") + + finally: + del settings.TEST_SWAPPABLE_MODEL + cache.app_models = old_app_models + cache.app_store = old_app_store + + def test_explicit_swappable_manager(self): + try: + # This test adds dummy models to the app cache. These + # need to be removed in order to prevent bad interactions + # with the flush operation in other tests. + old_app_models = copy.deepcopy(cache.app_models) + old_app_store = copy.deepcopy(cache.app_store) + + settings.TEST_SWAPPABLE_MODEL = 'managers_regress.Parent' + + class SwappableModel(models.Model): + + objects = models.Manager() + + class Meta: + swappable = 'TEST_SWAPPABLE_MODEL' + + # Accessing the manager on a swappable model with an + # explicit manager should raise an attribute error with a + # helpful message + try: + SwappableModel.objects.all() + self.fail('Should raise an AttributeError') + except AttributeError as e: + self.assertEqual(str(e), "Manager isn't available; SwappableModel has been swapped for 'managers_regress.Parent'") + + finally: + del settings.TEST_SWAPPABLE_MODEL + cache.app_models = old_app_models + cache.app_store = old_app_store