Fixed transaction handling for a number of operations on related objects.

Thanks Anssi and Aymeric for the reviews. Refs #21174.

django/contrib/contenttypes/fields.py

@@ -394,9 +394,12 @@ class ReverseGenericRelatedObjectsDescriptor(object):
 
     def __set__(self, instance, value):
         manager = self.__get__(instance)
-        manager.clear()
-        for obj in value:
-            manager.add(obj)
+
+        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)
 
 
 def create_generic_related_manager(superclass):
@@ -474,12 +477,14 @@ def create_generic_related_manager(superclass):
                     self.prefetch_cache_name)
 
         def add(self, *objs):
-            for obj in objs:
-                if not isinstance(obj, self.model):
-                    raise TypeError("'%s' instance expected" % self.model._meta.object_name)
-                setattr(obj, self.content_type_field_name, self.content_type)
-                setattr(obj, self.object_id_field_name, self.pk_val)
-                obj.save()
+            db = router.db_for_write(self.model, instance=self.instance)
+            with transaction.atomic(using=db, savepoint=False):
+                for obj in objs:
+                    if not isinstance(obj, self.model):
+                        raise TypeError("'%s' instance expected" % self.model._meta.object_name)
+                    setattr(obj, self.content_type_field_name, self.content_type)
+                    setattr(obj, self.object_id_field_name, self.pk_val)
+                    obj.save()
         add.alters_data = True
 
         def remove(self, *objs, **kwargs):
@@ -498,6 +503,8 @@ def create_generic_related_manager(superclass):
             db = router.db_for_write(self.model, instance=self.instance)
             queryset = queryset.using(db)
             if bulk:
+                # `QuerySet.delete()` creates its own atomic block which
+                # contains the `pre_delete` and `post_delete` signal handlers.
                 queryset.delete()
             else:
                 with transaction.atomic(using=db, savepoint=False):

docs/releases/1.8.txt

@@ -168,7 +168,19 @@ Backwards incompatible changes in 1.8
     deprecation timeline for a given feature, its removal may appear as a
     backwards incompatible change.
 
-...
+* Some operations on related objects such as
+  :meth:`~django.db.models.fields.related.RelatedManager.add()` or
+  :ref:`direct assignment<direct-assignment>` ran multiple data modifying
+  queries without wrapping them in transactions. To reduce the risk of data
+  corruption, all data modifying methods that affect multiple related objects
+  (i.e. ``add()``, ``remove()``, ``clear()``, and
+  :ref:`direct assignment<direct-assignment>`) now perform their data modifying
+  queries from within a transaction, provided your database supports
+  transactions.
+
+  This has one backwards incompatible side effect, signal handlers triggered
+  from these methods are now executed within the method's transaction and
+  any exception in a signal handler will prevent the whole operation.
 
 Miscellaneous
 ~~~~~~~~~~~~~

tests/many_to_many/tests.py

 from __future__ import unicode_literals
 
+from django.db import transaction
 from django.test import TestCase
 from django.utils import six
 
@@ -54,7 +55,9 @@ class ManyToManyTests(TestCase):
 
         # Adding an object of the wrong type raises TypeError
         with six.assertRaisesRegex(self, TypeError, "'Publication' instance expected, got <Article.*"):
-            a6.publications.add(a5)
+            with transaction.atomic():
+                a6.publications.add(a5)
+
         # Add a Publication directly via publications.add by using keyword arguments.
         a6.publications.create(title='Highlights for Adults')
         self.assertQuerysetEqual(a6.publications.all(),

tests/multiple_database/tests.py

@@ -295,19 +295,23 @@ class QueryTestCase(TestCase):
 
         # Add to an m2m with an object from a different database
         with self.assertRaises(ValueError):
-            marty.book_set.add(dive)
+            with transaction.atomic(using='default'):
+                marty.book_set.add(dive)
 
         # Set a m2m with an object from a different database
         with self.assertRaises(ValueError):
-            marty.book_set = [pro, dive]
+            with transaction.atomic(using='default'):
+                marty.book_set = [pro, dive]
 
         # Add to a reverse m2m with an object from a different database
         with self.assertRaises(ValueError):
-            dive.authors.add(marty)
+            with transaction.atomic(using='other'):
+                dive.authors.add(marty)
 
         # Set a reverse m2m with an object from a different database
         with self.assertRaises(ValueError):
-            dive.authors = [mark, marty]
+            with transaction.atomic(using='other'):
+                dive.authors = [mark, marty]
 
     def test_m2m_deletion(self):
         "Cascaded deletions of m2m relations issue queries on the right database"
@@ -762,7 +766,8 @@ class QueryTestCase(TestCase):
 
         # Add to a foreign key set with an object from a different database
         with self.assertRaises(ValueError):
-            dive.reviews.add(review1)
+            with transaction.atomic(using='other'):
+                dive.reviews.add(review1)
 
         # BUT! if you assign a FK object when the base object hasn't
         # been saved yet, you implicitly assign the database for the