From 4f8c7fd9d91b35e2c2922de4bb50c8c8066cbbc6 Mon Sep 17 00:00:00 2001 From: Keryn Knight Date: Sun, 1 Aug 2021 12:13:35 +0100 Subject: [PATCH] Fixed #32980 -- Made models cache related managers. --- django/contrib/contenttypes/fields.py | 8 ++ django/db/models/base.py | 27 ++++++- .../db/models/fields/related_descriptors.py | 26 ++++++- docs/releases/4.1.txt | 5 +- tests/custom_managers/models.py | 16 ++++ tests/custom_managers/tests.py | 76 ++++++++++++++++++- tests/m2m_regress/tests.py | 8 ++ tests/model_inheritance_regress/tests.py | 5 ++ tests/model_regress/test_state.py | 7 +- tests/prefetch_related/tests.py | 8 ++ 10 files changed, 173 insertions(+), 13 deletions(-) diff --git a/django/contrib/contenttypes/fields.py b/django/contrib/contenttypes/fields.py index b0b77b01d4..278b5bb585 100644 --- a/django/contrib/contenttypes/fields.py +++ b/django/contrib/contenttypes/fields.py @@ -505,6 +505,14 @@ 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 cc03ae1252..29bbde5e6b 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -382,11 +382,18 @@ class ModelBase(type): return cls._meta.default_manager -class ModelStateFieldsCacheDescriptor: +class ModelStateCacheDescriptor: + """ + Upon first access, replace itself with an empty dictionary on the instance. + """ + + def __set_name__(self, owner, name): + self.attribute_name = name + def __get__(self, instance, cls=None): if instance is None: return self - res = instance.fields_cache = {} + res = instance.__dict__[self.attribute_name] = {} return res @@ -398,7 +405,20 @@ class ModelState: # explicit (non-auto) PKs. This impacts validation only; it has no effect # on the actual save. adding = True - fields_cache = ModelStateFieldsCacheDescriptor() + 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 class Model(metaclass=ModelBase): @@ -552,7 +572,6 @@ 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() return state def __setstate__(self, state): diff --git a/django/db/models/fields/related_descriptors.py b/django/db/models/fields/related_descriptors.py index ef546e844a..d5aa968400 100644 --- a/django/db/models/fields/related_descriptors.py +++ b/django/db/models/fields/related_descriptors.py @@ -511,6 +511,14 @@ 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 @@ -532,8 +540,11 @@ class ReverseManyToOneDescriptor: """ if instance is None: return self - - return self.related_manager_cls(instance) + 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] def _get_set_deprecation_msg_params(self): return ( @@ -797,6 +808,17 @@ 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' % ('reverse' if self.reverse else 'forward'), diff --git a/docs/releases/4.1.txt b/docs/releases/4.1.txt index 2d1e0a93fe..ff1c59c852 100644 --- a/docs/releases/4.1.txt +++ b/docs/releases/4.1.txt @@ -255,7 +255,10 @@ Upstream support for MariaDB 10.2 ends in May 2022. Django 4.1 supports MariaDB 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. .. _deprecated-features-4.1: diff --git a/tests/custom_managers/models.py b/tests/custom_managers/models.py index 14222bfaf3..50d4bfb446 100644 --- a/tests/custom_managers/models.py +++ b/tests/custom_managers/models.py @@ -155,6 +155,22 @@ 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 8e5ab1418f..b397816668 100644 --- a/tests/custom_managers/tests.py +++ b/tests/custom_managers/tests.py @@ -2,10 +2,10 @@ from django.db import models from django.test import TestCase from .models import ( - Book, Car, CustomManager, CustomQuerySet, DeconstructibleCustomManager, - FastCarAsBase, FastCarAsDefault, FunPerson, OneToOneRestrictedModel, - Person, PersonFromAbstract, PersonManager, PublishedBookManager, - RelatedModel, RestrictedModel, + Book, Car, ConfusedBook, CustomManager, CustomQuerySet, + DeconstructibleCustomManager, FastCarAsBase, FastCarAsDefault, FunPerson, + OneToOneRestrictedModel, Person, PersonFromAbstract, PersonManager, + PublishedBookManager, RelatedModel, RestrictedModel, ) @@ -169,6 +169,10 @@ 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) Person.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_thing=self.b1) @@ -205,6 +209,60 @@ 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) @@ -245,6 +303,16 @@ 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) droopy = FunPerson.objects.create(first_name="Droopy", last_name="Dog", fun=False, favorite_book=self.b1) diff --git a/tests/m2m_regress/tests.py b/tests/m2m_regress/tests.py index a2c7fc99cc..0240893fdd 100644 --- a/tests/m2m_regress/tests.py +++ b/tests/m2m_regress/tests.py @@ -31,6 +31,14 @@ 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. diff --git a/tests/model_inheritance_regress/tests.py b/tests/model_inheritance_regress/tests.py index 2c15925da5..ff1531b3f0 100644 --- a/tests/model_inheritance_regress/tests.py +++ b/tests/model_inheritance_regress/tests.py @@ -354,6 +354,11 @@ 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 f41e4e52f4..66e519702a 100644 --- a/tests/model_regress/test_state.py +++ b/tests/model_regress/test_state.py @@ -1,8 +1,11 @@ -from django.db.models.base import ModelState, ModelStateFieldsCacheDescriptor +from django.db.models.base import ModelState, ModelStateCacheDescriptor from django.test import SimpleTestCase class ModelStateTests(SimpleTestCase): def test_fields_cache_descriptor(self): - self.assertIsInstance(ModelState.fields_cache, ModelStateFieldsCacheDescriptor) + self.assertIsInstance(ModelState.fields_cache, ModelStateCacheDescriptor) + + def test_related_managers_descriptor(self): + self.assertIsInstance(ModelState.related_managers_cache, ModelStateCacheDescriptor) diff --git a/tests/prefetch_related/tests.py b/tests/prefetch_related/tests.py index 242ff6c366..4ea7fccd5e 100644 --- a/tests/prefetch_related/tests.py +++ b/tests/prefetch_related/tests.py @@ -1147,6 +1147,14 @@ 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): """