From 20399083f49faceef1e48530822137257bca9d6a Mon Sep 17 00:00:00 2001 From: Loic Bistuer Date: Thu, 27 Mar 2014 19:32:48 +0800 Subject: [PATCH] Fixed #19816 -- Pre-evaluate querysets used in direct relation assignments. Since assignments on M2M or reverse FK descriptors is composed of a `clear()`, followed by an `add()`, `clear()` could potentially affect the value of the assigned queryset before the `add()` step; pre-evaluating it solves the problem. This patch fixes the issue for ForeignRelatedObjectsDescriptor, ManyRelatedObjectsDescriptor, and ReverseGenericRelatedObjectsDescriptor. It completes 6cb6e1 which addressed ReverseManyRelatedObjectsDescriptor. --- django/contrib/contenttypes/fields.py | 5 ++++- django/db/models/fields/related.py | 17 +++++++++++------ tests/generic_relations/tests.py | 15 +++++++++++++++ tests/m2m_regress/tests.py | 12 ------------ tests/many_to_many/tests.py | 24 ++++++++++++++++++++++++ tests/many_to_one_null/tests.py | 12 ++++++++++++ 6 files changed, 66 insertions(+), 19 deletions(-) diff --git a/django/contrib/contenttypes/fields.py b/django/contrib/contenttypes/fields.py index 7e91957546..6ea225191a 100644 --- a/django/contrib/contenttypes/fields.py +++ b/django/contrib/contenttypes/fields.py @@ -393,8 +393,11 @@ class ReverseGenericRelatedObjectsDescriptor(object): return manager def __set__(self, instance, value): - manager = self.__get__(instance) + # 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() diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 523b64a7e2..a789772dce 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -763,8 +763,11 @@ class ForeignRelatedObjectsDescriptor(object): return self.related_manager_cls(instance) def __set__(self, instance, value): - manager = self.__get__(instance) + # 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. @@ -1113,8 +1116,11 @@ class ManyRelatedObjectsDescriptor(object): 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)) - manager = self.__get__(instance) + # 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() @@ -1170,12 +1176,11 @@ class ReverseManyRelatedObjectsDescriptor(object): opts = self.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)) - manager = self.__get__(instance) - - # clear() can change expected output of 'value' queryset, we force evaluation - # of queryset before clear; ticket #19816 + # 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() diff --git a/tests/generic_relations/tests.py b/tests/generic_relations/tests.py index ae90ace34a..b6f429cfa9 100644 --- a/tests/generic_relations/tests.py +++ b/tests/generic_relations/tests.py @@ -163,6 +163,21 @@ class GenericRelationsTests(TestCase): "" ]) + 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 + # ManyRelatedObjectsDescriptor.__set__. Refs #19816. + bacon = Vegetable.objects.create(name="Bacon", is_yucky=False) + bacon.tags.create(tag="fatty") + bacon.tags.create(tag="salty") + self.assertEqual(2, bacon.tags.count()) + + qs = bacon.tags.filter(tag="fatty") + bacon.tags = qs + + self.assertEqual(1, bacon.tags.count()) + self.assertEqual(1, qs.count()) + def test_generic_relation_related_name_default(self): # Test that GenericRelation by default isn't usable from # the reverse side. diff --git a/tests/m2m_regress/tests.py b/tests/m2m_regress/tests.py index d1f85ace3e..105e41b644 100644 --- a/tests/m2m_regress/tests.py +++ b/tests/m2m_regress/tests.py @@ -97,15 +97,3 @@ class M2MRegressionTests(TestCase): # causes a TypeError in add_lazy_relation m1 = RegressionModelSplit(name='1') m1.save() - - def test_m2m_filter(self): - worksheet = Worksheet.objects.create(id=1) - line_hi = Line.objects.create(name="hi") - line_bye = Line.objects.create(name="bye") - - worksheet.lines = [line_hi, line_bye] - hi = worksheet.lines.filter(name="hi") - - worksheet.lines = hi - self.assertEqual(1, worksheet.lines.count()) - self.assertEqual(1, hi.count()) diff --git a/tests/many_to_many/tests.py b/tests/many_to_many/tests.py index 545d50a90e..23c6cd543e 100644 --- a/tests/many_to_many/tests.py +++ b/tests/many_to_many/tests.py @@ -370,6 +370,30 @@ class ManyToManyTests(TestCase): self.assertQuerysetEqual(self.a4.publications.all(), ['']) + def test_forward_assign_with_queryset(self): + # Ensure that querysets used in m2m assignments are pre-evaluated + # so their value isn't affected by the clearing operation in + # ManyRelatedObjectsDescriptor.__set__. Refs #19816. + self.a1.publications = [self.p1, self.p2] + + qs = self.a1.publications.filter(id=self.a1.id) + self.a1.publications = qs + + self.assertEqual(1, self.a1.publications.count()) + self.assertEqual(1, qs.count()) + + def test_reverse_assign_with_queryset(self): + # Ensure that querysets used in M2M assignments are pre-evaluated + # so their value isn't affected by the clearing operation in + # ReverseManyRelatedObjectsDescriptor.__set__. Refs #19816. + self.p1.article_set = [self.a1, self.a2] + + qs = self.p1.article_set.filter(id=self.p1.id) + self.p1.article_set = qs + + self.assertEqual(1, self.p1.article_set.count()) + self.assertEqual(1, qs.count()) + def test_clear(self): # Relation sets can be cleared: self.p2.article_set.clear() diff --git a/tests/many_to_one_null/tests.py b/tests/many_to_one_null/tests.py index cd1800d77c..f3988cce7f 100644 --- a/tests/many_to_one_null/tests.py +++ b/tests/many_to_one_null/tests.py @@ -86,6 +86,18 @@ class ManyToOneNullTests(TestCase): self.assertQuerysetEqual(Article.objects.filter(reporter__isnull=True), ['', '']) + def test_assign_with_queryset(self): + # Ensure that querysets used in reverse FK assignments are pre-evaluated + # so their value isn't affected by the clearing operation in + # ForeignRelatedObjectsDescriptor.__set__. Refs #19816. + self.r2.article_set = [self.a2, self.a3] + + qs = self.r2.article_set.filter(id=self.a2.id) + self.r2.article_set = qs + + self.assertEqual(1, self.r2.article_set.count()) + self.assertEqual(1, qs.count()) + def test_clear_efficiency(self): r = Reporter.objects.create() for _ in range(3):