From fa89fdcd6b4a4ab4887740c0d0f4a73297a5b060 Mon Sep 17 00:00:00 2001 From: Malcolm Tredinnick Date: Fri, 20 Mar 2009 03:57:12 +0000 Subject: [PATCH] Fixed #2698 -- Fixed deleting in the presence of custom managers. A custom manager on a related object that filtered away objects would prevent those objects being deleted via the relation. This is now fixed. git-svn-id: http://code.djangoproject.com/svn/django/trunk@10104 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/db/models/base.py | 12 ++++- django/db/models/fields/related.py | 44 ++++++++++------ .../custom_managers_regress/models.py | 52 +++++++++++++++++-- 3 files changed, 87 insertions(+), 21 deletions(-) diff --git a/django/db/models/base.py b/django/db/models/base.py index cf5903d289..013dc96e92 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -519,7 +519,17 @@ class Model(object): else: sub_obj._collect_sub_objects(seen_objs, self.__class__, related.field.null) else: - for sub_obj in getattr(self, rel_opts_name).all(): + # To make sure we can access all elements, we can't use the + # normal manager on the related object. So we work directly + # with the descriptor object. + for cls in self.__class__.mro(): + if rel_opts_name in cls.__dict__: + rel_descriptor = cls.__dict__[rel_opts_name] + break + else: + raise AssertionError("Should never get here.") + delete_qs = rel_descriptor.delete_manager(self).all() + for sub_obj in delete_qs: sub_obj._collect_sub_objects(seen_objs, self.__class__, related.field.null) # Handle any ancestors (for the model-inheritance case). We do this by diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 9aa2421f03..1545c73f58 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -188,7 +188,7 @@ class SingleRelatedObjectDescriptor(object): return getattr(instance, self.cache_name) except AttributeError: params = {'%s__pk' % self.related.field.name: instance._get_pk_val()} - rel_obj = self.related.model._default_manager.get(**params) + rel_obj = self.related.model._base_manager.get(**params) setattr(instance, self.cache_name, rel_obj) return rel_obj @@ -296,13 +296,36 @@ class ForeignRelatedObjectsDescriptor(object): if instance is None: return self + return self.create_manager(instance, + self.related.model._default_manager.__class__) + + def __set__(self, instance, value): + if instance is None: + raise AttributeError, "Manager must be accessed via instance" + + manager = self.__get__(instance) + # If the foreign key can support nulls, then completely clear the related set. + # Otherwise, just move the named objects into the set. + if self.related.field.null: + manager.clear() + manager.add(*value) + + def delete_manager(self, instance): + """ + Returns a queryset based on the related model's base manager (rather + than the default manager, as returned by __get__). Used by + Model.delete(). + """ + return self.create_manager(instance, + self.related.model._base_manager.__class__) + + def create_manager(self, instance, superclass): + """ + Creates the managers used by other methods (__get__() and delete()). + """ rel_field = self.related.field rel_model = self.related.model - # Dynamically create a class that subclasses the related - # model's default manager. - superclass = self.related.model._default_manager.__class__ - class RelatedManager(superclass): def get_query_set(self): return superclass.get_query_set(self).filter(**(self.core_filters)) @@ -352,17 +375,6 @@ class ForeignRelatedObjectsDescriptor(object): return manager - def __set__(self, instance, value): - if instance is None: - raise AttributeError, "Manager must be accessed via instance" - - manager = self.__get__(instance) - # If the foreign key can support nulls, then completely clear the related set. - # Otherwise, just move the named objects into the set. - if self.related.field.null: - manager.clear() - manager.add(*value) - def create_many_related_manager(superclass, through=False): """Creates a manager that subclasses 'superclass' (which is a Manager) and adds behavior for many-to-many related objects.""" diff --git a/tests/regressiontests/custom_managers_regress/models.py b/tests/regressiontests/custom_managers_regress/models.py index 67fed84447..898730ea9a 100644 --- a/tests/regressiontests/custom_managers_regress/models.py +++ b/tests/regressiontests/custom_managers_regress/models.py @@ -11,9 +11,27 @@ class RestrictedManager(models.Manager): def get_query_set(self): return super(RestrictedManager, self).get_query_set().filter(is_public=True) -class RestrictedClass(models.Model): +class RelatedModel(models.Model): + name = models.CharField(max_length=50) + + def __unicode__(self): + return self.name + +class RestrictedModel(models.Model): name = models.CharField(max_length=50) is_public = models.BooleanField(default=False) + related = models.ForeignKey(RelatedModel) + + objects = RestrictedManager() + plain_manager = models.Manager() + + def __unicode__(self): + return self.name + +class OneToOneRestrictedModel(models.Model): + name = models.CharField(max_length=50) + is_public = models.BooleanField(default=False) + related = models.OneToOneField(RelatedModel) objects = RestrictedManager() plain_manager = models.Manager() @@ -24,15 +42,41 @@ class RestrictedClass(models.Model): __test__ = {"tests": """ Even though the default manager filters out some records, we must still be able to save (particularly, save by updating existing records) those filtered -instances. This is a regression test for #FIXME. ->>> obj = RestrictedClass.objects.create(name="hidden") +instances. This is a regression test for #8990, #9527 +>>> related = RelatedModel.objects.create(name="xyzzy") +>>> obj = RestrictedModel.objects.create(name="hidden", related=related) >>> obj.name = "still hidden" >>> obj.save() # If the hidden object wasn't seen during the save process, there would now be # two objects in the database. ->>> RestrictedClass.plain_manager.count() +>>> RestrictedModel.plain_manager.count() 1 +Deleting related objects should also not be distracted by a restricted manager +on the related object. This is a regression test for #2698. +>>> RestrictedModel.plain_manager.all().delete() +>>> for name, public in (('one', True), ('two', False), ('three', False)): +... _ = RestrictedModel.objects.create(name=name, is_public=public, related=related) + +# Reload the RelatedModel instance, just to avoid any instance artifacts. +>>> obj = RelatedModel.objects.get(name="xyzzy") +>>> obj.delete() + +# All of the RestrictedModel instances should have been deleted, since they +# *all* pointed to the RelatedModel. If the default manager is used, only the +# public one will be deleted. +>>> RestrictedModel.plain_manager.all() +[] + +# The same test case as the last one, but for one-to-one models, which are +# implemented slightly different internally, so it's a different code path. +>>> obj = RelatedModel.objects.create(name="xyzzy") +>>> _ = OneToOneRestrictedModel.objects.create(name="foo", is_public=False, related=obj) +>>> obj = RelatedModel.objects.get(name="xyzzy") +>>> obj.delete() +>>> OneToOneRestrictedModel.plain_manager.all() +[] + """ }