Merge pull request #2487 from loic/ticket19816

Fixed #19816 -- Pre-evaluate querysets used in direct relation assignments.

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()

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()

tests/generic_relations/tests.py

@@ -163,6 +163,21 @@ class GenericRelationsTests(TestCase):
             "<Animal: Platypus>"
         ])
 
+    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.

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())

tests/many_to_many/tests.py

@@ -370,6 +370,30 @@ class ManyToManyTests(TestCase):
         self.assertQuerysetEqual(self.a4.publications.all(),
                                  ['<Publication: Science Weekly>'])
 
+    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()

tests/many_to_one_null/tests.py

@@ -86,6 +86,18 @@ class ManyToOneNullTests(TestCase):
         self.assertQuerysetEqual(Article.objects.filter(reporter__isnull=True),
                                  ['<Article: First>', '<Article: Fourth>'])
 
+    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):