Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Added a bulk option to RelatedManager remove() and clear() methods

Refs #21169
  • Loading branch information...
commit f450bc9f44bc1270eae911daa157c155c29d1d9d 1 parent 52015b9
@loic loic authored akaariai committed
View
27 django/contrib/contenttypes/generic.py
@@ -8,7 +8,7 @@
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 add(self, *objs):
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
View
29 django/db/models/fields/related.py
@@ -463,13 +463,11 @@ def get_or_create(self, **kwargs):
# 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 remove(self, *objs):
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()
@charettes Collaborator

Could we use only and update_fields here?

rel_field_name =  rel_field.name
update_fields = [rel_field_name]
for obj in queryset.only('pk', rel_field_name):
    setattr(obj, rel_field_name, None)
    obj.save(update_fields=update_fields)

It would make the slow path faster :)

@akaariai Collaborator

Technically you don't need update_fields at all - deferred models automatically update just the loaded fields. Still, coding with update_fields=[rel_field_name] might be cleaner.

@loic Collaborator
loic added a note

We discussed this on IRC, I don't think deferring is a good idea here.

That's premature optimization IMO, if you go down the slow path (bulk=False) it's likely because you want the object itself, only getting the PK would be better served by the post_update signal.

update_fields alone would be useful though.

@loic Collaborator
loic added a note

Done in 91fce67.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ _clear.alters_data = True
+
return RelatedManager
@@ -657,6 +667,7 @@ def clear(self):
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 _remove_items(self, source_field_name, target_field_name, *objs):
# 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
View
2  django/db/models/query.py
@@ -1030,7 +1030,7 @@ def _has_filters(self):
"""
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()
View
14 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::
View
11 docs/releases/1.7.txt
@@ -430,6 +430,11 @@ Models
* :class:`F expressions <django.db.models.F>` 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
View
44 tests/custom_managers/tests.py
@@ -226,11 +226,11 @@ def test_m2m_related_manager(self):
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 @@ def test_removal_through_default_fk_related_manager(self):
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 @@ def test_removal_through_default_fk_related_manager(self):
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 @@ def test_removal_through_default_fk_related_manager(self):
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 @@ def test_removal_through_specified_fk_related_manager(self):
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 @@ def test_removal_through_specified_fk_related_manager(self):
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 @@ def test_removal_through_specified_fk_related_manager(self):
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 @@ def test_removal_through_default_gfk_related_manager(self):
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 @@ def test_removal_through_default_gfk_related_manager(self):
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 @@ def test_removal_through_default_gfk_related_manager(self):
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 @@ def test_removal_through_specified_gfk_related_manager(self):
)
# 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 @@ def test_removal_through_specified_gfk_related_manager(self):
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 @@ def test_removal_through_specified_gfk_related_manager(self):
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)
Please sign in to comment.
Something went wrong with that request. Please try again.