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

@@ -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):
     """

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 = []

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"

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`).

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`.

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)

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

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

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"]:

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

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)

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):
     """