From f450bc9f44bc1270eae911daa157c155c29d1d9d Mon Sep 17 00:00:00 2001 From: Loic Bistuer Date: Fri, 22 Nov 2013 00:14:16 +0700 Subject: [PATCH] Added a bulk option to RelatedManager remove() and clear() methods Refs #21169 --- django/contrib/contenttypes/generic.py | 27 ++++++++++++---- django/db/models/fields/related.py | 29 +++++++++++------ django/db/models/query.py | 2 +- docs/ref/models/relations.txt | 14 +++++++- docs/releases/1.7.txt | 11 +++++-- tests/custom_managers/tests.py | 44 ++++++++++++++++---------- 6 files changed, 90 insertions(+), 37 deletions(-) diff --git a/django/contrib/contenttypes/generic.py b/django/contrib/contenttypes/generic.py index 469e64db33..9ef499bb16 100644 --- a/django/contrib/contenttypes/generic.py +++ b/django/contrib/contenttypes/generic.py @@ -8,7 +8,7 @@ from functools import partial from django.core.exceptions import ObjectDoesNotExist from django.db import connection -from django.db import models, router, DEFAULT_DB_ALIAS +from django.db import models, router, transaction, DEFAULT_DB_ALIAS from django.db.models import signals from django.db.models.fields.related import ForeignObject, ForeignObjectRel from django.db.models.related import PathInfo @@ -382,16 +382,29 @@ def create_generic_related_manager(superclass): obj.save() add.alters_data = True - def remove(self, *objs): - db = router.db_for_write(self.model, instance=self.instance) - self.using(db).filter(pk__in=[o.pk for o in objs]).delete() + def remove(self, *objs, **kwargs): + if not objs: + return + bulk = kwargs.pop('bulk', True) + self._clear(self.filter(pk__in=[o.pk for o in objs]), bulk) remove.alters_data = True - def clear(self): - db = router.db_for_write(self.model, instance=self.instance) - self.using(db).delete() + def clear(self, **kwargs): + bulk = kwargs.pop('bulk', True) + self._clear(self, bulk) clear.alters_data = True + def _clear(self, queryset, bulk): + db = router.db_for_write(self.model, instance=self.instance) + queryset = queryset.using(db) + if bulk: + queryset.delete() + else: + with transaction.commit_on_success_unless_managed(using=db, savepoint=False): + for obj in queryset: + obj.delete() + _clear.alters_data = True + def create(self, **kwargs): kwargs[self.content_type_field_name] = self.content_type kwargs[self.object_id_field_name] = self.pk_val diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 62c7248de2..4a945e0f45 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -463,13 +463,11 @@ def create_foreign_related_manager(superclass, rel_field, rel_model): # remove() and clear() are only provided if the ForeignKey can have a value of null. if rel_field.null: - def remove(self, *objs): - # If there aren't any objects, there is nothing to do. + def remove(self, *objs, **kwargs): if not objs: return - + bulk = kwargs.pop('bulk', True) val = rel_field.get_foreign_related_value(self.instance) - old_ids = set() for obj in objs: # Is obj actually part of this descriptor set? @@ -477,14 +475,26 @@ def create_foreign_related_manager(superclass, rel_field, rel_model): old_ids.add(obj.pk) else: raise rel_field.rel.to.DoesNotExist("%r is not related to %r." % (obj, self.instance)) - - self.filter(pk__in=old_ids).update(**{rel_field.name: None}) + self._clear(self.filter(pk__in=old_ids), bulk) remove.alters_data = True - def clear(self): - self.update(**{rel_field.name: None}) + def clear(self, **kwargs): + bulk = kwargs.pop('bulk', True) + self._clear(self, bulk) clear.alters_data = True + def _clear(self, queryset, bulk): + db = router.db_for_write(self.model, instance=self.instance) + queryset = queryset.using(db) + if bulk: + queryset.update(**{rel_field.name: None}) + else: + with transaction.commit_on_success_unless_managed(using=db, savepoint=False): + for obj in queryset: + setattr(obj, rel_field.name, None) + obj.save() + _clear.alters_data = True + return RelatedManager @@ -657,6 +667,7 @@ def create_many_related_manager(superclass, rel): signals.m2m_changed.send(sender=self.through, action="pre_clear", instance=self.instance, reverse=self.reverse, model=self.model, pk_set=None, using=db) + filters = self._build_remove_filters(super(ManyRelatedManager, self).get_queryset().using(db)) self.through._default_manager.using(db).filter(filters).delete() @@ -746,8 +757,6 @@ def create_many_related_manager(superclass, rel): # source_field_name: the PK colname in join table for the source object # target_field_name: the PK colname in join table for the target object # *objs - objects to remove - - # If there aren't any objects, there is nothing to do. if not objs: return diff --git a/django/db/models/query.py b/django/db/models/query.py index 59a2b45d02..1737626d16 100644 --- a/django/db/models/query.py +++ b/django/db/models/query.py @@ -1030,7 +1030,7 @@ class QuerySet(object): """ Checks if this QuerySet has any filtering going on. Note that this isn't equivalent for checking if all objects are present in results, - for example qs[1:]._has_filters() -> True. + for example qs[1:]._has_filters() -> False. """ return self.query.has_filters() diff --git a/docs/ref/models/relations.txt b/docs/ref/models/relations.txt index 2fca954676..0c47bc4047 100644 --- a/docs/ref/models/relations.txt +++ b/docs/ref/models/relations.txt @@ -110,6 +110,17 @@ Related objects reference the ``blog`` :class:`~django.db.models.ForeignKey` doesn't have ``null=True``, this is invalid. + .. versionchanged 1.7:: + + For :class:`~django.db.models.ForeignKey` objects, this method accepts + a ``bulk`` argument to control how to perform the operation. + If ``True`` (the default), ``QuerySet.update()`` is used. + If ``bulk=False``, the ``save()`` method of each individual model + instance is called instead. This triggers the + :data:`~django.db.models.signals.pre_save` and + :data:`~django.db.models.signals.post_save` signals and comes at the + expense of performance. + .. method:: clear() Removes all objects from the related object set:: @@ -121,7 +132,8 @@ Related objects reference them. Just like ``remove()``, ``clear()`` is only available on - :class:`~django.db.models.ForeignKey`\s where ``null=True``. + :class:`~django.db.models.ForeignKey`\s where ``null=True`` and it also + accepts the ``bulk`` keyword argument. .. note:: diff --git a/docs/releases/1.7.txt b/docs/releases/1.7.txt index a453301999..f8cdcf1f3b 100644 --- a/docs/releases/1.7.txt +++ b/docs/releases/1.7.txt @@ -430,6 +430,11 @@ Models * :class:`F expressions ` support the power operator (``**``). +* The ``remove()`` and ``clear()`` methods of the related managers created by + ``ForeignKey`` and ``GenericForeignKey`` now accept the ``bulk`` keyword + argument to control whether or not to perform operations in bulk + (i.e. using ``QuerySet.update()``). Defaults to ``True``. + Signals ^^^^^^^ @@ -589,11 +594,13 @@ Fixing the issues introduced some backward incompatible changes: - The default implementation of ``remove()`` for ``ForeignKey`` related managers changed from a series of ``Model.save()`` calls to a single ``QuerySet.update()`` call. The change means that ``pre_save`` and - ``post_save`` signals aren't sent anymore. + ``post_save`` signals aren't sent anymore. You can use the ``bulk=False`` + keyword argument to revert to the previous behavior. - The ``remove()`` and ``clear()`` methods for ``GenericForeignKey`` related managers now perform bulk delete. The ``Model.delete()`` method isn't called - on each instance anymore. + on each instance anymore. You can use the ``bulk=False`` keyword argument to + revert to the previous behavior. - The ``remove()`` and ``clear()`` methods for ``ManyToManyField`` related managers perform nested queries when filtering is involved, which may or diff --git a/tests/custom_managers/tests.py b/tests/custom_managers/tests.py index af36110eca..440274e0e0 100644 --- a/tests/custom_managers/tests.py +++ b/tests/custom_managers/tests.py @@ -226,11 +226,11 @@ class CustomManagerTests(TestCase): ordered=False, ) - def test_removal_through_default_fk_related_manager(self): + def test_removal_through_default_fk_related_manager(self, bulk=True): bugs = FunPerson.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_book=self.b1) droopy = FunPerson.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_book=self.b1) - self.b1.fun_people_favorite_books.remove(droopy) + self.b1.fun_people_favorite_books.remove(droopy, bulk=bulk) self.assertQuerysetEqual( FunPerson._base_manager.filter(favorite_book=self.b1), [ "Bugs", @@ -240,7 +240,7 @@ class CustomManagerTests(TestCase): ordered=False, ) - self.b1.fun_people_favorite_books.remove(bugs) + self.b1.fun_people_favorite_books.remove(bugs, bulk=bulk) self.assertQuerysetEqual( FunPerson._base_manager.filter(favorite_book=self.b1), [ "Droopy", @@ -251,7 +251,7 @@ class CustomManagerTests(TestCase): bugs.favorite_book = self.b1 bugs.save() - self.b1.fun_people_favorite_books.clear() + self.b1.fun_people_favorite_books.clear(bulk=bulk) self.assertQuerysetEqual( FunPerson._base_manager.filter(favorite_book=self.b1), [ "Droopy", @@ -260,12 +260,15 @@ class CustomManagerTests(TestCase): ordered=False, ) - def test_removal_through_specified_fk_related_manager(self): + def test_slow_removal_through_default_fk_related_manager(self): + self.test_removal_through_default_fk_related_manager(bulk=False) + + def test_removal_through_specified_fk_related_manager(self, bulk=True): Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_book=self.b1) droopy = Person.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_book=self.b1) # Check that the fun manager DOESN'T remove boring people. - self.b1.favorite_books(manager='fun_people').remove(droopy) + self.b1.favorite_books(manager='fun_people').remove(droopy, bulk=bulk) self.assertQuerysetEqual( self.b1.favorite_books(manager='boring_people').all(), [ "Droopy", @@ -274,7 +277,7 @@ class CustomManagerTests(TestCase): ordered=False, ) # Check that the boring manager DOES remove boring people. - self.b1.favorite_books(manager='boring_people').remove(droopy) + self.b1.favorite_books(manager='boring_people').remove(droopy, bulk=bulk) self.assertQuerysetEqual( self.b1.favorite_books(manager='boring_people').all(), [ ], @@ -285,7 +288,7 @@ class CustomManagerTests(TestCase): droopy.save() # Check that the fun manager ONLY clears fun people. - self.b1.favorite_books(manager='fun_people').clear() + self.b1.favorite_books(manager='fun_people').clear(bulk=bulk) self.assertQuerysetEqual( self.b1.favorite_books(manager='boring_people').all(), [ "Droopy", @@ -300,11 +303,14 @@ class CustomManagerTests(TestCase): ordered=False, ) - def test_removal_through_default_gfk_related_manager(self): + def test_slow_removal_through_specified_fk_related_manager(self): + self.test_removal_through_specified_fk_related_manager(bulk=False) + + def test_removal_through_default_gfk_related_manager(self, bulk=True): bugs = FunPerson.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_thing=self.b1) droopy = FunPerson.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_thing=self.b1) - self.b1.fun_people_favorite_things.remove(droopy) + self.b1.fun_people_favorite_things.remove(droopy, bulk=bulk) self.assertQuerysetEqual( FunPerson._base_manager.order_by('first_name').filter(favorite_thing_id=self.b1.pk), [ "Bugs", @@ -314,7 +320,7 @@ class CustomManagerTests(TestCase): ordered=False, ) - self.b1.fun_people_favorite_things.remove(bugs) + self.b1.fun_people_favorite_things.remove(bugs, bulk=bulk) self.assertQuerysetEqual( FunPerson._base_manager.order_by('first_name').filter(favorite_thing_id=self.b1.pk), [ "Droopy", @@ -325,7 +331,7 @@ class CustomManagerTests(TestCase): bugs.favorite_book = self.b1 bugs.save() - self.b1.fun_people_favorite_things.clear() + self.b1.fun_people_favorite_things.clear(bulk=bulk) self.assertQuerysetEqual( FunPerson._base_manager.order_by('first_name').filter(favorite_thing_id=self.b1.pk), [ "Droopy", @@ -334,12 +340,15 @@ class CustomManagerTests(TestCase): ordered=False, ) - def test_removal_through_specified_gfk_related_manager(self): + def test_slow_removal_through_default_gfk_related_manager(self): + self.test_removal_through_default_gfk_related_manager(bulk=False) + + def test_removal_through_specified_gfk_related_manager(self, bulk=True): Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True, favorite_thing=self.b1) droopy = Person.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_thing=self.b1) # Check that the fun manager DOESN'T remove boring people. - self.b1.favorite_things(manager='fun_people').remove(droopy) + self.b1.favorite_things(manager='fun_people').remove(droopy, bulk=bulk) self.assertQuerysetEqual( self.b1.favorite_things(manager='boring_people').all(), [ "Droopy", @@ -349,7 +358,7 @@ class CustomManagerTests(TestCase): ) # Check that the boring manager DOES remove boring people. - self.b1.favorite_things(manager='boring_people').remove(droopy) + self.b1.favorite_things(manager='boring_people').remove(droopy, bulk=bulk) self.assertQuerysetEqual( self.b1.favorite_things(manager='boring_people').all(), [ ], @@ -360,7 +369,7 @@ class CustomManagerTests(TestCase): droopy.save() # Check that the fun manager ONLY clears fun people. - self.b1.favorite_things(manager='fun_people').clear() + self.b1.favorite_things(manager='fun_people').clear(bulk=bulk) self.assertQuerysetEqual( self.b1.favorite_things(manager='boring_people').all(), [ "Droopy", @@ -375,6 +384,9 @@ class CustomManagerTests(TestCase): ordered=False, ) + def test_slow_removal_through_specified_gfk_related_manager(self): + self.test_removal_through_specified_gfk_related_manager(bulk=False) + def test_removal_through_default_m2m_related_manager(self): bugs = FunPerson.objects.create(first_name="Bugs", last_name="Bunny", fun=True) self.b1.fun_authors.add(bugs)