Permalink
Browse files

Fixed #6707 -- Added RelatedManager.set() and made descriptors' __se…

…t__ use it.

Thanks Anssi Kääriäinen, Carl Meyer, Collin Anderson, and Tim Graham for the reviews.
  • Loading branch information...
loic committed Jan 29, 2015
1 parent 49516f7 commit 71ada3a8e689a883b5ffdeb1744ea16f176ab730
@@ -436,16 +436,8 @@ def __get__(self, instance, instance_type=None):
return manager
def __set__(self, instance, value):
- # Force evaluation of `value` in case it's a queryset whose
- # value could be affected by `manager.clear()`. Refs #19816.
- value = tuple(value)
-
manager = self.__get__(instance)
- db = router.db_for_write(manager.model, instance=manager.instance)
- with transaction.atomic(using=db, savepoint=False):
- manager.clear()
- for obj in value:
- manager.add(obj)
+ manager.set(value)
def create_generic_related_manager(superclass):
@@ -561,6 +553,31 @@ def _clear(self, queryset, bulk):
obj.delete()
_clear.alters_data = True
+ def set(self, objs, **kwargs):
+ clear = kwargs.pop('clear', False)
+
+ db = router.db_for_write(self.model, instance=self.instance)
+ with transaction.atomic(using=db, savepoint=False):
+ if clear:
+ # Force evaluation of `objs` in case it's a queryset whose value
+ # could be affected by `manager.clear()`. Refs #19816.
+ objs = tuple(objs)
+
+ self.clear()
+ self.add(*objs)
+ else:
+ old_objs = set(self.using(db).all())
+ new_objs = []
+ for obj in objs:
+ if obj in old_objs:
+ old_objs.remove(obj)
+ else:
+ new_objs.append(obj)
+
+ self.remove(*old_objs)
+ self.add(*new_objs)
+ set.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
@@ -796,6 +796,34 @@ def _clear(self, queryset, bulk):
obj.save(update_fields=[rel_field.name])
_clear.alters_data = True
+ def set(self, objs, **kwargs):
+ clear = kwargs.pop('clear', False)
+
+ if rel_field.null:
+ db = router.db_for_write(self.model, instance=self.instance)
+ with transaction.atomic(using=db, savepoint=False):
+ if clear:
+ # Force evaluation of `objs` in case it's a queryset whose value
+ # could be affected by `manager.clear()`. Refs #19816.
+ objs = tuple(objs)
+
+ self.clear()
+ self.add(*objs)
+ else:
+ old_objs = set(self.using(db).all())
+ new_objs = []
+ for obj in objs:
+ if obj in old_objs:
+ old_objs.remove(obj)
+ else:
+ new_objs.append(obj)
+
+ self.remove(*old_objs)
+ self.add(*new_objs)
+ else:
+ self.add(*objs)
+ set.alters_data = True
+
return RelatedManager
@@ -815,18 +843,8 @@ def __get__(self, instance, instance_type=None):
return self.related_manager_cls(instance)
def __set__(self, instance, value):
- # Force evaluation of `value` in case it's a queryset whose
- # value could be affected by `manager.clear()`. Refs #19816.
- value = tuple(value)
-
manager = self.__get__(instance)
- db = router.db_for_write(manager.model, instance=manager.instance)
- with transaction.atomic(using=db, savepoint=False):
- # 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)
+ manager.set(value)
@cached_property
def related_manager_cls(self):
@@ -1002,6 +1020,43 @@ def clear(self):
model=self.model, pk_set=None, using=db)
clear.alters_data = True
+ def set(self, objs, **kwargs):
+ if not rel.through._meta.auto_created:
+ opts = self.through._meta
+ raise AttributeError(
+ "Cannot set values on a ManyToManyField which specifies an "
+ "intermediary model. Use %s.%s's Manager instead." %
+ (opts.app_label, opts.object_name)
+ )
+
+ clear = kwargs.pop('clear', False)
+
+ db = router.db_for_write(self.through, instance=self.instance)
+ with transaction.atomic(using=db, savepoint=False):
+ if clear:
+ # Force evaluation of `objs` in case it's a queryset whose value
+ # could be affected by `manager.clear()`. Refs #19816.
+ objs = tuple(objs)
+
+ self.clear()
+ self.add(*objs)
+ else:
+ old_ids = set(self.using(db).values_list(self.target_field.related_field.attname, flat=True))
+
+ new_objs = []
+ for obj in objs:
+ fk_val = (self.target_field.get_foreign_related_value(obj)[0]
+ if isinstance(obj, self.model) else obj)
+
+ if fk_val in old_ids:
+ old_ids.remove(fk_val)
+ else:
+ new_objs.append(obj)
+
+ self.remove(*old_ids)
+ self.add(*new_objs)
+ set.alters_data = True
+
def create(self, **kwargs):
# This check needs to be done here, since we can't later remove this
# from the method lookup table, as we do with add and remove.
@@ -1181,22 +1236,8 @@ def __get__(self, instance, instance_type=None):
return manager
def __set__(self, instance, value):
- if not self.related.field.rel.through._meta.auto_created:
- opts = self.related.field.rel.through._meta
- raise AttributeError(
- "Cannot set values on a ManyToManyField which specifies an "
- "intermediary model. Use %s.%s's Manager instead." % (opts.app_label, opts.object_name)
- )
-
- # Force evaluation of `value` in case it's a queryset whose
- # value could be affected by `manager.clear()`. Refs #19816.
- value = tuple(value)
-
manager = self.__get__(instance)
- db = router.db_for_write(manager.through, instance=manager.instance)
- with transaction.atomic(using=db, savepoint=False):
- manager.clear()
- manager.add(*value)
+ manager.set(value)
class ReverseManyRelatedObjectsDescriptor(object):
@@ -1251,15 +1292,8 @@ def __set__(self, instance, value):
"intermediary model. Use %s.%s's Manager instead." % (opts.app_label, opts.object_name)
)
- # Force evaluation of `value` in case it's a queryset whose
- # value could be affected by `manager.clear()`. Refs #19816.
- value = tuple(value)
-
manager = self.__get__(instance)
- db = router.db_for_write(manager.through, instance=manager.instance)
- with transaction.atomic(using=db, savepoint=False):
- manager.clear()
- manager.add(*value)
+ manager.set(value)
class ForeignObjectRel(object):
@@ -135,12 +135,31 @@ Related objects reference
:class:`~django.db.models.ForeignKey`\s where ``null=True`` and it also
accepts the ``bulk`` keyword argument.
+ .. method:: set(objs, clear=False)
+
+ .. versionadded:: 1.9
+
+ Replace the set of related objects::
+
+ >>> new_list = [obj1, obj2, obj3]
+ >>> e.related_set.set(new_list)
+
+ This method accepts a ``clear`` argument to control how to perform the
+ operation. If ``False`` (the default), the elements missing from the
+ new set are removed using ``remove()`` and only the new ones are added.
+ If ``clear=True``, the ``clear()`` method is called instead and the
+ whole set is added at once.
+
+ Note that since ``set()`` is a compound operation, it is subject to
+ race conditions. For instance, new objects may be added to the database
+ in between the call to ``clear()`` and the call to ``add()``.
+
.. note::
- Note that ``add()``, ``create()``, ``remove()``, and ``clear()`` all
- apply database changes immediately for all types of related fields. In
- other words, there is no need to call ``save()`` on either end of the
- relationship.
+ Note that ``add()``, ``create()``, ``remove()``, ``clear()``, and
+ ``set()`` all apply database changes immediately for all types of
+ related fields. In other words, there is no need to call ``save()``
+ on either end of the relationship.
Also, if you are using :ref:`an intermediate model
<intermediary-manytomany>` for a many-to-many relationship, some of the
@@ -158,6 +177,12 @@ new iterable of objects to it::
>>> e.related_set = new_list
If the foreign key relationship has ``null=True``, then the related manager
-will first call ``clear()`` to disassociate any existing objects in the related
-set before adding the contents of ``new_list``. Otherwise the objects in
-``new_list`` will be added to the existing related object set.
+will first disassociate any existing objects in the related set before adding
+the contents of ``new_list``. Otherwise the objects in ``new_list`` will be
+added to the existing related object set.
+
+.. versionchanged:1.9
+
+ In earlier versions, direct assignment used to perform ``clear()`` followed
+ by ``add()``. It now performs a ``set()`` with the keyword argument
+ ``clear=False``.
View
@@ -127,7 +127,10 @@ Management Commands
Models
^^^^^^
-* ...
+* Added the :meth:`RelatedManager.set()
+ <django.db.models.fields.related.RelatedManager.set()>` method to the related
+ managers created by ``ForeignKey``, ``GenericForeignKey``, and
+ ``ManyToManyField``.
Signals
^^^^^^^
@@ -192,6 +195,25 @@ used by the egg loader to detect if setuptools was installed. The ``is_usable``
attribute is now removed and the egg loader instead fails at runtime if
setuptools is not installed.
+Related set direct assignment
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+:ref:`Direct assignment <direct-assignment>`) used to perform a ``clear()``
+followed by a call to ``add()``. This caused needlessly large data changes
+and prevented using the :data:`~django.db.models.signals.m2m_changed` signal
+to track individual changes in many-to-many relations.
+
+Direct assignment now relies on the the new
+:meth:`django.db.models.fields.related.RelatedManager.set()` method on
+related managers which by default only processes changes between the
+existing related set and the one that's newly assigned. The previous behavior
+can be restored by replacing direct assignment by a call to ``set()`` with
+the keyword argument ``clear=True``.
+
+``ModelForm``, and therefore ``ModelAdmin``, internally rely on direct
+assignment for many-to-many relations and as a consequence now use the new
+behavior.
+
Miscellaneous
~~~~~~~~~~~~~
@@ -1190,6 +1190,9 @@ be found in the :doc:`related objects reference </ref/models/relations>`.
``clear()``
Removes all objects from the related object set.
+``set(objs)``
+ Replace the set of related objects.
+
To assign the members of a related set in one fell swoop, just assign to it
from any iterable object. The iterable can contain object instances, or just
a list of primary key values. For example::
@@ -246,6 +246,58 @@ def test_tag_deletion_related_objects_unaffected(self):
self.comp_func
)
+ def test_set(self):
+ bacon = Vegetable.objects.create(name="Bacon", is_yucky=False)
+ fatty = bacon.tags.create(tag="fatty")
+ salty = bacon.tags.create(tag="salty")
+
+ bacon.tags.set([fatty, salty])
+ self.assertQuerysetEqual(bacon.tags.all(), [
+ "<TaggedItem: fatty>",
+ "<TaggedItem: salty>",
+ ])
+
+ bacon.tags.set([fatty])
+ self.assertQuerysetEqual(bacon.tags.all(), [
+ "<TaggedItem: fatty>",
+ ])
+
+ bacon.tags.set([])
+ self.assertQuerysetEqual(bacon.tags.all(), [])
+
+ bacon.tags.set([fatty, salty], clear=True)
+ self.assertQuerysetEqual(bacon.tags.all(), [
+ "<TaggedItem: fatty>",
+ "<TaggedItem: salty>",
+ ])
+
+ bacon.tags.set([fatty], clear=True)
+ self.assertQuerysetEqual(bacon.tags.all(), [
+ "<TaggedItem: fatty>",
+ ])
+
+ bacon.tags.set([], clear=True)
+ self.assertQuerysetEqual(bacon.tags.all(), [])
+
+ def test_assign(self):
+ bacon = Vegetable.objects.create(name="Bacon", is_yucky=False)
+ fatty = bacon.tags.create(tag="fatty")
+ salty = bacon.tags.create(tag="salty")
+
+ bacon.tags = [fatty, salty]
+ self.assertQuerysetEqual(bacon.tags.all(), [
+ "<TaggedItem: fatty>",
+ "<TaggedItem: salty>",
+ ])
+
+ bacon.tags = [fatty]
+ self.assertQuerysetEqual(bacon.tags.all(), [
+ "<TaggedItem: fatty>",
+ ])
+
+ bacon.tags = []
+ self.assertQuerysetEqual(bacon.tags.all(), [])
+
def test_assign_with_queryset(self):
# Ensure that querysets used in reverse GFK assignments are pre-evaluated
# so their value isn't affected by the clearing operation in
Oops, something went wrong.

1 comment on commit 71ada3a

@jsenecal

This comment has been minimized.

Show comment
Hide comment
@jsenecal

jsenecal Jun 1, 2015

Would it be possible to have the above fix applied to the 1.8 branch ?

Would it be possible to have the above fix applied to the 1.8 branch ?

Please sign in to comment.