From 71ada3a8e689a883b5ffdeb1744ea16f176ab730 Mon Sep 17 00:00:00 2001 From: Loic Bistuer Date: Fri, 30 Jan 2015 01:15:27 +0700 Subject: [PATCH] Fixed #6707 -- Added RelatedManager.set() and made descriptors' __set__ use it. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Thanks Anssi Kääriäinen, Carl Meyer, Collin Anderson, and Tim Graham for the reviews. --- django/contrib/contenttypes/fields.py | 35 ++++++--- django/db/models/fields/related.py | 102 +++++++++++++++++--------- docs/ref/models/relations.txt | 39 ++++++++-- docs/releases/1.9.txt | 24 +++++- docs/topics/db/queries.txt | 3 + tests/generic_relations/tests.py | 52 +++++++++++++ tests/m2m_signals/tests.py | 98 +++++++++++++------------ tests/many_to_many/tests.py | 39 ++++++++++ tests/many_to_one/tests.py | 39 ++++++++++ tests/many_to_one_null/tests.py | 19 ++++- 10 files changed, 350 insertions(+), 100 deletions(-) diff --git a/django/contrib/contenttypes/fields.py b/django/contrib/contenttypes/fields.py index 2bb8ce9e95..6a316e8d9a 100644 --- a/django/contrib/contenttypes/fields.py +++ b/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) - 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 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 diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 6cc26dcd57..0b0414bb0f 100644 --- a/django/db/models/fields/related.py +++ b/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) - 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 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) - with transaction.atomic(using=db, savepoint=False): - manager.clear() - manager.add(*value) + manager.set(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) - with transaction.atomic(using=db, savepoint=False): - manager.clear() - manager.add(*value) + manager.set(value) class ForeignObjectRel(object): diff --git a/docs/ref/models/relations.txt b/docs/ref/models/relations.txt index 7f5eb18ea6..8c3b5a2f33 100644 --- a/docs/ref/models/relations.txt +++ b/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 - 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 ` 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``. diff --git a/docs/releases/1.9.txt b/docs/releases/1.9.txt index eee63c6f6d..d00bfef096 100644 --- a/docs/releases/1.9.txt +++ b/docs/releases/1.9.txt @@ -127,7 +127,10 @@ Management Commands Models ^^^^^^ -* ... +* Added the :meth:`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 `) 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 ~~~~~~~~~~~~~ diff --git a/docs/topics/db/queries.txt b/docs/topics/db/queries.txt index c616d02b33..34342ab3eb 100644 --- a/docs/topics/db/queries.txt +++ b/docs/topics/db/queries.txt @@ -1190,6 +1190,9 @@ be found in the :doc:`related objects reference `. ``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:: diff --git a/tests/generic_relations/tests.py b/tests/generic_relations/tests.py index c6230b057e..bc2cfaa579 100644 --- a/tests/generic_relations/tests.py +++ b/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(), [ + "", + "", + ]) + + bacon.tags.set([fatty]) + self.assertQuerysetEqual(bacon.tags.all(), [ + "", + ]) + + bacon.tags.set([]) + self.assertQuerysetEqual(bacon.tags.all(), []) + + bacon.tags.set([fatty, salty], clear=True) + self.assertQuerysetEqual(bacon.tags.all(), [ + "", + "", + ]) + + bacon.tags.set([fatty], clear=True) + self.assertQuerysetEqual(bacon.tags.all(), [ + "", + ]) + + 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(), [ + "", + "", + ]) + + bacon.tags = [fatty] + self.assertQuerysetEqual(bacon.tags.all(), [ + "", + ]) + + 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 diff --git a/tests/m2m_signals/tests.py b/tests/m2m_signals/tests.py index ea4649551d..f41f6efd8e 100644 --- a/tests/m2m_signals/tests.py +++ b/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', diff --git a/tests/many_to_many/tests.py b/tests/many_to_many/tests.py index adb897840a..192bc5f0e8 100644 --- a/tests/many_to_many/tests.py +++ b/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(), + [ + '', + '', + ]) + self.assertQuerysetEqual(self.a4.publications.all(), + ['']) + self.a4.publications.set([self.p3.id]) + self.assertQuerysetEqual(self.p2.article_set.all(), + ['']) + self.assertQuerysetEqual(self.a4.publications.all(), + ['']) + + 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(), + [ + '', + '', + ]) + self.assertQuerysetEqual(self.a4.publications.all(), + ['']) + self.a4.publications.set([self.p3.id], clear=True) + self.assertQuerysetEqual(self.p2.article_set.all(), + ['']) + self.assertQuerysetEqual(self.a4.publications.all(), + ['']) + + 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] diff --git a/tests/many_to_one/tests.py b/tests/many_to_one/tests.py index e82d8eefe6..b66cfde9ed 100644 --- a/tests/many_to_one/tests.py +++ b/tests/many_to_one/tests.py @@ -80,11 +80,49 @@ class ManyToOneTests(TestCase): "", ]) + 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), "") + self.assertEqual(new_article2.reporter.id, self.r.id) + self.assertQuerysetEqual(self.r.article_set.all(), [ + "", + "", + "", + ]) + 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(), [""]) + self.assertQuerysetEqual(self.r2.article_set.all(), + [ + "", + "", + ]) + + # 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(), + [ + "", + "", + ]) + self.assertQuerysetEqual(self.r2.article_set.all(), [""]) + 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): "", ]) 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(), [""]) diff --git a/tests/many_to_one_null/tests.py b/tests/many_to_one_null/tests.py index a72b821f88..e39f527abe 100644 --- a/tests/many_to_one_null/tests.py +++ b/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(), ['']) + 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(), + ['', '']) + # Use manager.set(clear=True) + self.r2.article_set.set([self.a3, self.a4], clear=True) + self.assertQuerysetEqual(self.r2.article_set.all(), + ['', '']) + # 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), + ['', '', '']) + 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(), ['', ''])