From 5e0aa362d91d000984995ce374c2d7547d8d107f Mon Sep 17 00:00:00 2001 From: Mariusz Felisiak Date: Fri, 30 Sep 2022 18:18:33 +0200 Subject: [PATCH] Fixed #33984 -- Reverted "Fixed #32980 -- Made models cache related managers." This reverts 4f8c7fd9d91b35e2c2922de4bb50c8c8066cbbc6 and adds two regression tests: - test_related_manager_refresh(), and - test_create_copy_with_m2m(). Thanks joeli for the report. --- django/contrib/contenttypes/fields.py | 8 -- django/db/models/base.py | 27 +------ .../db/models/fields/related_descriptors.py | 26 +------ docs/releases/4.1.2.txt | 4 + docs/releases/4.1.txt | 3 +- tests/custom_managers/models.py | 16 ---- tests/custom_managers/tests.py | 76 ------------------- tests/m2m_regress/tests.py | 22 ++++-- tests/many_to_many/tests.py | 19 +++++ tests/model_inheritance_regress/tests.py | 5 -- tests/model_regress/test_state.py | 9 +-- tests/prefetch_related/tests.py | 8 -- 12 files changed, 47 insertions(+), 176 deletions(-) diff --git a/django/contrib/contenttypes/fields.py b/django/contrib/contenttypes/fields.py index 5b327b6ae4..bb93aa5d1e 100644 --- a/django/contrib/contenttypes/fields.py +++ b/django/contrib/contenttypes/fields.py @@ -552,14 +552,6 @@ class ReverseGenericManyToOneDescriptor(ReverseManyToOneDescriptor): self.rel, ) - @cached_property - def related_manager_cache_key(self): - # By default, GenericRel instances will be marked as hidden unless - # related_query_name is given (their accessor name being "+" when - # hidden), which would cause multiple GenericRelations declared on a - # single model to collide, so always use the remote field's name. - return self.field.get_cache_name() - def create_generic_related_manager(superclass, rel): """ diff --git a/django/db/models/base.py b/django/db/models/base.py index cb51ed42c4..584db319da 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -434,18 +434,11 @@ class ModelBase(type): return cls._meta.default_manager -class ModelStateCacheDescriptor: - """ - Upon first access, replace itself with an empty dictionary on the instance. - """ - - def __set_name__(self, owner, name): - self.attribute_name = name - +class ModelStateFieldsCacheDescriptor: def __get__(self, instance, cls=None): if instance is None: return self - res = instance.__dict__[self.attribute_name] = {} + res = instance.fields_cache = {} return res @@ -458,20 +451,7 @@ class ModelState: # explicit (non-auto) PKs. This impacts validation only; it has no effect # on the actual save. adding = True - fields_cache = ModelStateCacheDescriptor() - related_managers_cache = ModelStateCacheDescriptor() - - def __getstate__(self): - state = self.__dict__.copy() - if "fields_cache" in state: - state["fields_cache"] = self.fields_cache.copy() - # Manager instances stored in related_managers_cache won't necessarily - # be deserializable if they were dynamically created via an inner - # scope, e.g. create_forward_many_to_many_manager() and - # create_generic_related_manager(). - if "related_managers_cache" in state: - state["related_managers_cache"] = {} - return state + fields_cache = ModelStateFieldsCacheDescriptor() class Model(metaclass=ModelBase): @@ -633,6 +613,7 @@ class Model(metaclass=ModelBase): """Hook to allow choosing the attributes to pickle.""" state = self.__dict__.copy() state["_state"] = copy.copy(state["_state"]) + state["_state"].fields_cache = state["_state"].fields_cache.copy() # memoryview cannot be pickled, so cast it to bytes and store # separately. _memoryview_attrs = [] diff --git a/django/db/models/fields/related_descriptors.py b/django/db/models/fields/related_descriptors.py index beb15f9cad..04c956bd1e 100644 --- a/django/db/models/fields/related_descriptors.py +++ b/django/db/models/fields/related_descriptors.py @@ -590,14 +590,6 @@ class ReverseManyToOneDescriptor: self.rel = rel self.field = rel.field - @cached_property - def related_manager_cache_key(self): - # Being able to access the manager instance precludes it from being - # hidden. The rel's accessor name is used to allow multiple managers - # to the same model to coexist. e.g. post.attached_comment_set and - # post.attached_link_set are separately cached. - return self.rel.get_cache_name() - @cached_property def related_manager_cls(self): related_model = self.rel.related_model @@ -619,11 +611,8 @@ class ReverseManyToOneDescriptor: """ if instance is None: return self - key = self.related_manager_cache_key - instance_cache = instance._state.related_managers_cache - if key not in instance_cache: - instance_cache[key] = self.related_manager_cls(instance) - return instance_cache[key] + + return self.related_manager_cls(instance) def _get_set_deprecation_msg_params(self): return ( @@ -941,17 +930,6 @@ class ManyToManyDescriptor(ReverseManyToOneDescriptor): reverse=self.reverse, ) - @cached_property - def related_manager_cache_key(self): - if self.reverse: - # Symmetrical M2Ms won't have an accessor name, but should never - # end up in the reverse branch anyway, as the related_name ends up - # being hidden, and no public manager is created. - return self.rel.get_cache_name() - else: - # For forward managers, defer to the field name. - return self.field.get_cache_name() - def _get_set_deprecation_msg_params(self): return ( "%s side of a many-to-many set" diff --git a/docs/releases/4.1.2.txt b/docs/releases/4.1.2.txt index d607c34c92..498c645920 100644 --- a/docs/releases/4.1.2.txt +++ b/docs/releases/4.1.2.txt @@ -43,3 +43,7 @@ Bugfixes * Fixed a regression in Django 4.1 that caused a crash for :class:`View` subclasses with asynchronous handlers when handling non-allowed HTTP methods (:ticket:`34062`). + +* Reverted caching related managers for ``ForeignKey``, ``ManyToManyField``, + and ``GenericRelation`` that caused the incorrect refreshing of related + objects (:ticket:`33984`). diff --git a/docs/releases/4.1.txt b/docs/releases/4.1.txt index 11268df298..08fb7c2abc 100644 --- a/docs/releases/4.1.txt +++ b/docs/releases/4.1.txt @@ -530,7 +530,8 @@ Miscellaneous * Related managers for :class:`~django.db.models.ForeignKey`, :class:`~django.db.models.ManyToManyField`, and :class:`~django.contrib.contenttypes.fields.GenericRelation` are now cached - on the :class:`~django.db.models.Model` instance to which they belong. + on the :class:`~django.db.models.Model` instance to which they belong. *This + change was reverted in Django 4.1.2.* * The Django test runner now returns a non-zero error code for unexpected successes from tests marked with :py:func:`unittest.expectedFailure`. diff --git a/tests/custom_managers/models.py b/tests/custom_managers/models.py index 56448b45e9..53a07c462d 100644 --- a/tests/custom_managers/models.py +++ b/tests/custom_managers/models.py @@ -164,22 +164,6 @@ class Book(models.Model): base_manager_name = "annotated_objects" -class ConfusedBook(models.Model): - title = models.CharField(max_length=50) - author = models.CharField(max_length=30) - favorite_things = GenericRelation( - Person, - content_type_field="favorite_thing_type", - object_id_field="favorite_thing_id", - ) - less_favorite_things = GenericRelation( - FunPerson, - content_type_field="favorite_thing_type", - object_id_field="favorite_thing_id", - related_query_name="favorite_things", - ) - - class FastCarManager(models.Manager): def get_queryset(self): return super().get_queryset().filter(top_speed__gt=150) diff --git a/tests/custom_managers/tests.py b/tests/custom_managers/tests.py index 6b90a6ce37..5ae5c719d8 100644 --- a/tests/custom_managers/tests.py +++ b/tests/custom_managers/tests.py @@ -4,7 +4,6 @@ from django.test import TestCase from .models import ( Book, Car, - ConfusedBook, CustomManager, CustomQuerySet, DeconstructibleCustomManager, @@ -196,10 +195,6 @@ class CustomManagerTests(TestCase): ordered=False, ) - def test_fk_related_manager_reused(self): - self.assertIs(self.b1.favorite_books, self.b1.favorite_books) - self.assertIn("favorite_books", self.b1._state.related_managers_cache) - def test_gfk_related_manager(self): Person.objects.create( first_name="Bugs", last_name="Bunny", fun=True, favorite_thing=self.b1 @@ -248,67 +243,6 @@ class CustomManagerTests(TestCase): ordered=False, ) - def test_gfk_related_manager_reused(self): - self.assertIs( - self.b1.fun_people_favorite_things, - self.b1.fun_people_favorite_things, - ) - self.assertIn( - "fun_people_favorite_things", - self.b1._state.related_managers_cache, - ) - - def test_gfk_related_manager_not_reused_when_alternate(self): - self.assertIsNot( - self.b1.favorite_things(manager="fun_people"), - self.b1.favorite_things(manager="fun_people"), - ) - - def test_gfk_related_manager_no_overlap_when_not_hidden(self): - """ - If a GenericRelation defines a related_query_name (and thus the - related_name) which shadows another GenericRelation, it should not - cause those separate managers to clash. - """ - book = ConfusedBook.objects.create( - title="How to program", - author="Rodney Dangerfield", - ) - person = Person.objects.create( - first_name="Bugs", - last_name="Bunny", - fun=True, - favorite_thing=book, - ) - fun_person = FunPerson.objects.create( - first_name="Droopy", - last_name="Dog", - fun=False, - favorite_thing=book, - ) - # The managers don't collide in the internal cache. - self.assertIsNot(book.favorite_things, book.less_favorite_things) - self.assertIs(book.favorite_things, book.favorite_things) - self.assertIs(book.less_favorite_things, book.less_favorite_things) - # Both managers are cached separately despite the collision in names. - self.assertIn("favorite_things", book._state.related_managers_cache) - self.assertIn("less_favorite_things", book._state.related_managers_cache) - # "less_favorite_things" isn't available as a reverse related manager, - # so never ends up in the cache. - self.assertQuerysetEqual(fun_person.favorite_things.all(), [book]) - with self.assertRaises(AttributeError): - fun_person.less_favorite_things - self.assertIn("favorite_things", fun_person._state.related_managers_cache) - self.assertNotIn( - "less_favorite_things", - fun_person._state.related_managers_cache, - ) - # The GenericRelation doesn't exist for Person, only FunPerson, so the - # exception prevents the cache from being polluted. - with self.assertRaises(AttributeError): - person.favorite_things - self.assertNotIn("favorite_things", person._state.related_managers_cache) - def test_m2m_related_manager(self): bugs = Person.objects.create(first_name="Bugs", last_name="Bunny", fun=True) self.b1.authors.add(bugs) @@ -355,16 +289,6 @@ class CustomManagerTests(TestCase): ordered=False, ) - def test_m2m_related_forward_manager_reused(self): - self.assertIs(self.b1.authors, self.b1.authors) - self.assertIn("authors", self.b1._state.related_managers_cache) - - def test_m2m_related_revers_manager_reused(self): - bugs = Person.objects.create(first_name="Bugs", last_name="Bunny") - self.b1.authors.add(bugs) - self.assertIs(bugs.books, bugs.books) - self.assertIn("books", bugs._state.related_managers_cache) - def test_removal_through_default_fk_related_manager(self, bulk=True): bugs = FunPerson.objects.create( first_name="Bugs", last_name="Bunny", fun=True, favorite_book=self.b1 diff --git a/tests/m2m_regress/tests.py b/tests/m2m_regress/tests.py index d5ad559e85..d3f352ffd9 100644 --- a/tests/m2m_regress/tests.py +++ b/tests/m2m_regress/tests.py @@ -39,14 +39,6 @@ class M2MRegressionTests(TestCase): self.assertSequenceEqual(e1.topics.all(), [t1]) self.assertSequenceEqual(e1.related.all(), [t2]) - def test_m2m_managers_reused(self): - s1 = SelfRefer.objects.create(name="s1") - e1 = Entry.objects.create(name="e1") - self.assertIs(s1.references, s1.references) - self.assertIs(s1.related, s1.related) - self.assertIs(e1.topics, e1.topics) - self.assertIs(e1.related, e1.related) - def test_internal_related_name_not_in_error_msg(self): # The secret internal related names for self-referential many-to-many # fields shouldn't appear in the list when an error is made. @@ -79,6 +71,20 @@ class M2MRegressionTests(TestCase): w.save() w.delete() + def test_create_copy_with_m2m(self): + t1 = Tag.objects.create(name="t1") + Entry.objects.create(name="e1") + entry = Entry.objects.first() + entry.topics.set([t1]) + old_topics = entry.topics.all() + entry.pk = None + entry._state.adding = True + entry.save() + entry.topics.set(old_topics) + entry = Entry.objects.get(pk=entry.pk) + self.assertCountEqual(entry.topics.all(), old_topics) + self.assertSequenceEqual(entry.topics.all(), [t1]) + def test_add_m2m_with_base_class(self): # Regression for #11956 -- You can add an object to a m2m with the # base class without causing integrity errors diff --git a/tests/many_to_many/tests.py b/tests/many_to_many/tests.py index 53e870ddad..729f011446 100644 --- a/tests/many_to_many/tests.py +++ b/tests/many_to_many/tests.py @@ -92,6 +92,25 @@ class ManyToManyTests(TestCase): a5.authors.remove(user_2.username) self.assertSequenceEqual(a5.authors.all(), []) + def test_related_manager_refresh(self): + user_1 = User.objects.create(username="Jean") + user_2 = User.objects.create(username="Joe") + self.a3.authors.add(user_1.username) + self.assertSequenceEqual(user_1.article_set.all(), [self.a3]) + # Change the username on a different instance of the same user. + user_1_from_db = User.objects.get(pk=user_1.pk) + self.assertSequenceEqual(user_1_from_db.article_set.all(), [self.a3]) + user_1_from_db.username = "Paul" + self.a3.authors.set([user_2.username]) + user_1_from_db.save() + # Assign a different article. + self.a4.authors.add(user_1_from_db.username) + self.assertSequenceEqual(user_1_from_db.article_set.all(), [self.a4]) + # Refresh the instance with an evaluated related manager. + user_1.refresh_from_db() + self.assertEqual(user_1.username, "Paul") + self.assertSequenceEqual(user_1.article_set.all(), [self.a4]) + def test_add_remove_invalid_type(self): msg = "Field 'id' expected a number but got 'invalid'." for method in ["add", "remove"]: diff --git a/tests/model_inheritance_regress/tests.py b/tests/model_inheritance_regress/tests.py index 936a555684..4fd6b918bd 100644 --- a/tests/model_inheritance_regress/tests.py +++ b/tests/model_inheritance_regress/tests.py @@ -416,11 +416,6 @@ class ModelInheritanceTest(TestCase): parties = list(p4.bachelorparty_set.all()) self.assertEqual(parties, [bachelor, messy_parent]) - def test_abstract_base_class_m2m_relation_inheritance_manager_reused(self): - p1 = Person.objects.create(name="Alice") - self.assertIs(p1.birthdayparty_set, p1.birthdayparty_set) - self.assertIs(p1.bachelorparty_set, p1.bachelorparty_set) - def test_abstract_verbose_name_plural_inheritance(self): """ verbose_name_plural correctly inherited from ABC if inheritance chain diff --git a/tests/model_regress/test_state.py b/tests/model_regress/test_state.py index 1814458ba7..8b0f087829 100644 --- a/tests/model_regress/test_state.py +++ b/tests/model_regress/test_state.py @@ -1,12 +1,7 @@ -from django.db.models.base import ModelState, ModelStateCacheDescriptor +from django.db.models.base import ModelState, ModelStateFieldsCacheDescriptor from django.test import SimpleTestCase class ModelStateTests(SimpleTestCase): def test_fields_cache_descriptor(self): - self.assertIsInstance(ModelState.fields_cache, ModelStateCacheDescriptor) - - def test_related_managers_descriptor(self): - self.assertIsInstance( - ModelState.related_managers_cache, ModelStateCacheDescriptor - ) + self.assertIsInstance(ModelState.fields_cache, ModelStateFieldsCacheDescriptor) diff --git a/tests/prefetch_related/tests.py b/tests/prefetch_related/tests.py index 4c92ccc9ca..4faf13bcaf 100644 --- a/tests/prefetch_related/tests.py +++ b/tests/prefetch_related/tests.py @@ -1358,14 +1358,6 @@ class ForeignKeyToFieldTest(TestCase): ], ) - def test_m2m_manager_reused(self): - author = Author.objects.prefetch_related( - "favorite_authors", - "favors_me", - ).first() - self.assertIs(author.favorite_authors, author.favorite_authors) - self.assertIs(author.favors_me, author.favors_me) - class LookupOrderingTest(TestCase): """