Fixed #6707 -- Added RelatedManager.set() and made descriptors' __set__ use it.

Thanks Anssi Kääriäinen, Carl Meyer, Collin Anderson, and Tim Graham for the reviews.

django/contrib/contenttypes/fields.py

@@ -436,16 +436,8 @@ class ReverseGenericRelatedObjectsDescriptor(object):
         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)
+        manager.set(value)
-        with transaction.atomic(using=db, savepoint=False):
-            manager.clear()
-            for obj in value:
-                manager.add(obj)
 
 
 def create_generic_related_manager(superclass):
@@ -561,6 +553,31 @@ def create_generic_related_manager(superclass):
                         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

django/db/models/fields/related.py

@@ -796,6 +796,34 @@ def create_foreign_related_manager(superclass, rel_field, rel_model):
                             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 @@ class ForeignRelatedObjectsDescriptor(object):
         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)
+        manager.set(value)
-        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)
 
     @cached_property
     def related_manager_cls(self):
@@ -1002,6 +1020,43 @@ def create_many_related_manager(superclass, rel):
                     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 @@ class ManyRelatedObjectsDescriptor(object):
         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)
+        manager.set(value)
-        with transaction.atomic(using=db, savepoint=False):
-            manager.clear()
-            manager.add(*value)
 
 
 class ReverseManyRelatedObjectsDescriptor(object):
@@ -1251,15 +1292,8 @@ class ReverseManyRelatedObjectsDescriptor(object):
                 "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)
+        manager.set(value)
-        with transaction.atomic(using=db, savepoint=False):
-            manager.clear()
-            manager.add(*value)
 
 
 class ForeignObjectRel(object):

docs/ref/models/relations.txt

@@ -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
+       Note that ``add()``, ``create()``, ``remove()``, ``clear()``, and
-       apply database changes immediately for all types of related fields. In
+       ``set()`` all apply database changes immediately for all types of
-       other words, there is no need to call ``save()`` on either end of the
+       related fields. In other words, there is no need to call ``save()``
-       relationship.
+       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
+will first disassociate any existing objects in the related set before adding
-set before adding the contents of ``new_list``. Otherwise the objects in
+the contents of ``new_list``. Otherwise the objects in ``new_list`` will be
-``new_list`` will be added to the existing related object set.
+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``.

docs/releases/1.9.txt

@@ -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
 ~~~~~~~~~~~~~
 

docs/topics/db/queries.txt

@@ -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::

tests/generic_relations/tests.py

@@ -246,6 +246,58 @@ class GenericRelationsTests(TestCase):
             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

tests/m2m_signals/tests.py

@@ -252,6 +252,38 @@ class ManyToManySignalsTest(TestCase):
 
         # direct assignment clears the set first, then adds
         self.vw.default_parts = [self.wheelset, self.doors, self.engine]
+        expected_messages.append({
+            'instance': self.vw,
+            'action': 'pre_remove',
+            'reverse': False,
+            'model': Part,
+            'objects': [p6],
+        })
+        expected_messages.append({
+            'instance': self.vw,
+            'action': 'post_remove',
+            'reverse': False,
+            'model': Part,
+            'objects': [p6],
+        })
+        expected_messages.append({
+            'instance': self.vw,
+            'action': 'pre_add',
+            'reverse': False,
+            'model': Part,
+            'objects': [self.doors, self.engine, self.wheelset],
+        })
+        expected_messages.append({
+            'instance': self.vw,
+            'action': 'post_add',
+            'reverse': False,
+            'model': Part,
+            'objects': [self.doors, self.engine, self.wheelset],
+        })
+        self.assertEqual(self.m2m_changed_messages, expected_messages)
+
+        # set by clearing.
+        self.vw.default_parts.set([self.wheelset, self.doors, self.engine], clear=True)
         expected_messages.append({
             'instance': self.vw,
             'action': 'pre_clear',
@@ -280,22 +312,28 @@ class ManyToManySignalsTest(TestCase):
         })
         self.assertEqual(self.m2m_changed_messages, expected_messages)
 
+        # set by only removing what's necessary.
+        self.vw.default_parts.set([self.wheelset, self.doors], clear=False)
+        expected_messages.append({
+            'instance': self.vw,
+            'action': 'pre_remove',
+            'reverse': False,
+            'model': Part,
+            'objects': [self.engine],
+        })
+        expected_messages.append({
+            'instance': self.vw,
+            'action': 'post_remove',
+            'reverse': False,
+            'model': Part,
+            'objects': [self.engine],
+        })
+        self.assertEqual(self.m2m_changed_messages, expected_messages)
+
         # Check that signals still work when model inheritance is involved
         c4 = SportsCar.objects.create(name='Bugatti', price='1000000')
         c4b = Car.objects.get(name='Bugatti')
         c4.default_parts = [self.doors]
-        expected_messages.append({
-            'instance': c4,
-            'action': 'pre_clear',
-            'reverse': False,
-            'model': Part,
-        })
-        expected_messages.append({
-            'instance': c4,
-            'action': 'post_clear',
-            'reverse': False,
-            'model': Part,
-        })
         expected_messages.append({
             'instance': c4,
             'action': 'pre_add',
@@ -340,18 +378,6 @@ class ManyToManySignalsTest(TestCase):
         )
 
         self.alice.friends = [self.bob, self.chuck]
-        expected_messages.append({
-            'instance': self.alice,
-            'action': 'pre_clear',
-            'reverse': False,
-            'model': Person,
-        })
-        expected_messages.append({
-            'instance': self.alice,
-            'action': 'post_clear',
-            'reverse': False,
-            'model': Person,
-        })
         expected_messages.append({
             'instance': self.alice,
             'action': 'pre_add',
@@ -369,18 +395,6 @@ class ManyToManySignalsTest(TestCase):
         self.assertEqual(self.m2m_changed_messages, expected_messages)
 
         self.alice.fans = [self.daisy]
-        expected_messages.append({
-            'instance': self.alice,
-            'action': 'pre_clear',
-            'reverse': False,
-            'model': Person,
-        })
-        expected_messages.append({
-            'instance': self.alice,
-            'action': 'post_clear',
-            'reverse': False,
-            'model': Person,
-        })
         expected_messages.append({
             'instance': self.alice,
             'action': 'pre_add',
@@ -398,18 +412,6 @@ class ManyToManySignalsTest(TestCase):
         self.assertEqual(self.m2m_changed_messages, expected_messages)
 
         self.chuck.idols = [self.alice, self.bob]
-        expected_messages.append({
-            'instance': self.chuck,
-            'action': 'pre_clear',
-            'reverse': True,
-            'model': Person,
-        })
-        expected_messages.append({
-            'instance': self.chuck,
-            'action': 'post_clear',
-            'reverse': True,
-            'model': Person,
-        })
         expected_messages.append({
             'instance': self.chuck,
             'action': 'pre_add',

tests/many_to_many/tests.py

@@ -332,6 +332,45 @@ class ManyToManyTests(TestCase):
             ])
         self.assertQuerysetEqual(self.a3.publications.all(), [])
 
+    def test_set(self):
+        self.p2.article_set.set([self.a4, self.a3])
+        self.assertQuerysetEqual(self.p2.article_set.all(),
+            [
+                '<Article: NASA finds intelligent life on Earth>',
+                '<Article: Oxygen-free diet works wonders>',
+            ])
+        self.assertQuerysetEqual(self.a4.publications.all(),
+                                 ['<Publication: Science News>'])
+        self.a4.publications.set([self.p3.id])
+        self.assertQuerysetEqual(self.p2.article_set.all(),
+                                 ['<Article: NASA finds intelligent life on Earth>'])
+        self.assertQuerysetEqual(self.a4.publications.all(),
+                                 ['<Publication: Science Weekly>'])
+
+        self.p2.article_set.set([])
+        self.assertQuerysetEqual(self.p2.article_set.all(), [])
+        self.a4.publications.set([])
+        self.assertQuerysetEqual(self.a4.publications.all(), [])
+
+        self.p2.article_set.set([self.a4, self.a3], clear=True)
+        self.assertQuerysetEqual(self.p2.article_set.all(),
+            [
+                '<Article: NASA finds intelligent life on Earth>',
+                '<Article: Oxygen-free diet works wonders>',
+            ])
+        self.assertQuerysetEqual(self.a4.publications.all(),
+                                 ['<Publication: Science News>'])
+        self.a4.publications.set([self.p3.id], clear=True)
+        self.assertQuerysetEqual(self.p2.article_set.all(),
+                                 ['<Article: NASA finds intelligent life on Earth>'])
+        self.assertQuerysetEqual(self.a4.publications.all(),
+                                 ['<Publication: Science Weekly>'])
+
+        self.p2.article_set.set([], clear=True)
+        self.assertQuerysetEqual(self.p2.article_set.all(), [])
+        self.a4.publications.set([], clear=True)
+        self.assertQuerysetEqual(self.a4.publications.all(), [])
+
     def test_assign(self):
         # Relation sets can be assigned. Assignment clears any existing set members
         self.p2.article_set = [self.a4, self.a3]

tests/many_to_one/tests.py

@@ -80,11 +80,49 @@ class ManyToOneTests(TestCase):
                 "<Article: This is a test>",
             ])
 
+    def test_set(self):
+        new_article = self.r.article_set.create(headline="John's second story",
+                                                pub_date=datetime.date(2005, 7, 29))
+        new_article2 = self.r2.article_set.create(headline="Paul's story",
+                                                  pub_date=datetime.date(2006, 1, 17))
+
+        # Assign the article to the reporter.
+        new_article2.reporter = self.r
+        new_article2.save()
+        self.assertEqual(repr(new_article2.reporter), "<Reporter: John Smith>")
+        self.assertEqual(new_article2.reporter.id, self.r.id)
+        self.assertQuerysetEqual(self.r.article_set.all(), [
+            "<Article: John's second story>",
+            "<Article: Paul's story>",
+            "<Article: This is a test>",
+        ])
+        self.assertQuerysetEqual(self.r2.article_set.all(), [])
+
+        # Set the article back again.
+        self.r2.article_set.set([new_article, new_article2])
+        self.assertQuerysetEqual(self.r.article_set.all(), ["<Article: This is a test>"])
+        self.assertQuerysetEqual(self.r2.article_set.all(),
+            [
+                "<Article: John's second story>",
+                "<Article: Paul's story>",
+            ])
+
+        # Funny case - because the ForeignKey cannot be null,
+        # existing members of the set must remain.
+        self.r.article_set.set([new_article])
+        self.assertQuerysetEqual(self.r.article_set.all(),
+            [
+                "<Article: John's second story>",
+                "<Article: This is a test>",
+            ])
+        self.assertQuerysetEqual(self.r2.article_set.all(), ["<Article: Paul's story>"])
+
     def test_assign(self):
         new_article = self.r.article_set.create(headline="John's second story",
                                                 pub_date=datetime.date(2005, 7, 29))
         new_article2 = self.r2.article_set.create(headline="Paul's story",
                                                   pub_date=datetime.date(2006, 1, 17))
+
         # Assign the article to the reporter directly using the descriptor.
         new_article2.reporter = self.r
         new_article2.save()
@@ -96,6 +134,7 @@ class ManyToOneTests(TestCase):
             "<Article: This is a test>",
         ])
         self.assertQuerysetEqual(self.r2.article_set.all(), [])
+
         # Set the article back again using set descriptor.
         self.r2.article_set = [new_article, new_article2]
         self.assertQuerysetEqual(self.r.article_set.all(), ["<Article: This is a test>"])

tests/many_to_one_null/tests.py

@@ -74,9 +74,26 @@ class ManyToOneNullTests(TestCase):
         self.assertRaises(Reporter.DoesNotExist, self.r.article_set.remove, self.a4)
         self.assertQuerysetEqual(self.r2.article_set.all(), ['<Article: Fourth>'])
 
+    def test_set(self):
+        # Use manager.set() to allocate ForeignKey. Null is legal, so existing
+        # members of the set that are not in the assignment set are set to null.
+        self.r2.article_set.set([self.a2, self.a3])
+        self.assertQuerysetEqual(self.r2.article_set.all(),
+                                 ['<Article: Second>', '<Article: Third>'])
+        # Use manager.set(clear=True)
+        self.r2.article_set.set([self.a3, self.a4], clear=True)
+        self.assertQuerysetEqual(self.r2.article_set.all(),
+                                 ['<Article: Fourth>', '<Article: Third>'])
+        # Clear the rest of the set
+        self.r2.article_set.set([])
+        self.assertQuerysetEqual(self.r2.article_set.all(), [])
+        self.assertQuerysetEqual(Article.objects.filter(reporter__isnull=True),
+                                 ['<Article: Fourth>', '<Article: Second>', '<Article: Third>'])
+
     def test_assign_clear_related_set(self):
         # Use descriptor assignment to allocate ForeignKey. Null is legal, so
-        # existing members of set that are not in the assignment set are set null
+        # existing members of the set that are not in the assignment set are
+        # set to null.
         self.r2.article_set = [self.a2, self.a3]
         self.assertQuerysetEqual(self.r2.article_set.all(),
                                  ['<Article: Second>', '<Article: Third>'])