Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Fixed #19816 -- Pre-evaluate querysets used in direct relation assign…

…ments.

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.
  • Loading branch information...
commit 20399083f49faceef1e48530822137257bca9d6a 1 parent 2f3e1fe
Loïc Bistuer loic authored
5 django/contrib/contenttypes/fields.py
View
@@ -393,8 +393,11 @@ def __get__(self, instance, instance_type=None):
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()
17 django/db/models/fields/related.py
View
@@ -763,8 +763,11 @@ def __get__(self, instance, instance_type=None):
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 @@ def __set__(self, instance, value):
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 @@ def __set__(self, instance, value):
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()
15 tests/generic_relations/tests.py
View
@@ -163,6 +163,21 @@ def test_generic_relations(self):
"<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.
12 tests/m2m_regress/tests.py
View
@@ -97,15 +97,3 @@ def test_m2m_abstract_split(self):
# 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())
24 tests/many_to_many/tests.py
View
@@ -370,6 +370,30 @@ def test_assign_ids(self):
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()
12 tests/many_to_one_null/tests.py
View
@@ -86,6 +86,18 @@ def test_assign_clear_related_set(self):
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):
Please sign in to comment.
Something went wrong with that request. Please try again.