From b9838c65ec2fee36c1fd0d46494ba44da27a9b34 Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Fri, 3 May 2024 17:18:06 +0100 Subject: [PATCH] Fixed #35405 -- Converted get_cache_name into a cached property in FieldCacheMixin. FieldCacheMixin is used by related fields to track their cached values. This work migrates get_cache_name() to be a cached property to optimize performance by reducing unnecessary function calls when working with related fields, given that its value remains constant. Co-authored-by: Simon Charette Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com> Co-authored-by: Natalia <124304+nessita@users.noreply.github.com> --- django/contrib/contenttypes/fields.py | 3 +- django/db/models/fields/mixins.py | 33 ++++++-- django/db/models/fields/related.py | 3 +- .../db/models/fields/related_descriptors.py | 10 +-- django/db/models/fields/reverse_related.py | 3 +- docs/internals/deprecation.txt | 2 + docs/releases/5.1.txt | 2 + tests/model_fields/test_mixins.py | 75 +++++++++++++++++++ 8 files changed, 117 insertions(+), 14 deletions(-) create mode 100644 tests/model_fields/test_mixins.py diff --git a/django/contrib/contenttypes/fields.py b/django/contrib/contenttypes/fields.py index 770f88265c6..a3e87f6ed45 100644 --- a/django/contrib/contenttypes/fields.py +++ b/django/contrib/contenttypes/fields.py @@ -140,7 +140,8 @@ class GenericForeignKey(FieldCacheMixin, Field): else: return [] - def get_cache_name(self): + @cached_property + def cache_name(self): return self.name def get_content_type(self, obj=None, id=None, using=None, model=None): diff --git a/django/db/models/fields/mixins.py b/django/db/models/fields/mixins.py index e7f282210e9..9f2809dfc8a 100644 --- a/django/db/models/fields/mixins.py +++ b/django/db/models/fields/mixins.py @@ -1,31 +1,52 @@ +import warnings + from django.core import checks +from django.utils.deprecation import RemovedInDjango60Warning +from django.utils.functional import cached_property NOT_PROVIDED = object() class FieldCacheMixin: - """Provide an API for working with the model's fields value cache.""" + """ + An API for working with the model's fields value cache. + Subclasses must set self.cache_name to a unique entry for the cache - + typically the field’s name. + """ + + # RemovedInDjango60Warning. def get_cache_name(self): raise NotImplementedError - def get_cached_value(self, instance, default=NOT_PROVIDED): + @cached_property + def cache_name(self): + # RemovedInDjango60Warning: when the deprecation ends, replace with: + # raise NotImplementedError cache_name = self.get_cache_name() + warnings.warn( + f"Override {self.__class__.__qualname__}.cache_name instead of " + "get_cache_name().", + RemovedInDjango60Warning, + ) + return cache_name + + def get_cached_value(self, instance, default=NOT_PROVIDED): try: - return instance._state.fields_cache[cache_name] + return instance._state.fields_cache[self.cache_name] except KeyError: if default is NOT_PROVIDED: raise return default def is_cached(self, instance): - return self.get_cache_name() in instance._state.fields_cache + return self.cache_name in instance._state.fields_cache def set_cached_value(self, instance, value): - instance._state.fields_cache[self.get_cache_name()] = value + instance._state.fields_cache[self.cache_name] = value def delete_cached_value(self, instance): - del instance._state.fields_cache[self.get_cache_name()] + del instance._state.fields_cache[self.cache_name] class CheckFieldDefaultMixin: diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 3e4bfe34c1b..7d42d1ea38a 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -509,7 +509,8 @@ class RelatedField(FieldCacheMixin, Field): ) return target_fields[0] - def get_cache_name(self): + @cached_property + def cache_name(self): return self.name diff --git a/django/db/models/fields/related_descriptors.py b/django/db/models/fields/related_descriptors.py index c7848ee63a3..bc288c47ece 100644 --- a/django/db/models/fields/related_descriptors.py +++ b/django/db/models/fields/related_descriptors.py @@ -210,7 +210,7 @@ class ForwardManyToOneDescriptor: rel_obj_attr, instance_attr, True, - self.field.get_cache_name(), + self.field.cache_name, False, ) @@ -486,7 +486,7 @@ class ReverseOneToOneDescriptor: rel_obj_attr, instance_attr, True, - self.related.get_cache_name(), + self.related.cache_name, False, ) @@ -744,7 +744,7 @@ def create_reverse_many_to_one_manager(superclass, rel): def _remove_prefetched_objects(self): try: self.instance._prefetched_objects_cache.pop( - self.field.remote_field.get_cache_name() + self.field.remote_field.cache_name ) except (AttributeError, KeyError): pass # nothing to clear from cache @@ -760,7 +760,7 @@ def create_reverse_many_to_one_manager(superclass, rel): ) try: return self.instance._prefetched_objects_cache[ - self.field.remote_field.get_cache_name() + self.field.remote_field.cache_name ] except (AttributeError, KeyError): queryset = super().get_queryset() @@ -798,7 +798,7 @@ def create_reverse_many_to_one_manager(superclass, rel): if not self.field.is_cached(rel_obj): instance = instances_dict[rel_obj_attr(rel_obj)] setattr(rel_obj, self.field.name, instance) - cache_name = self.field.remote_field.get_cache_name() + cache_name = self.field.remote_field.cache_name return queryset, rel_obj_attr, instance_attr, False, cache_name, False def add(self, *objs, bulk=True): diff --git a/django/db/models/fields/reverse_related.py b/django/db/models/fields/reverse_related.py index c9f9232ff73..a39bfd07daa 100644 --- a/django/db/models/fields/reverse_related.py +++ b/django/db/models/fields/reverse_related.py @@ -248,7 +248,8 @@ class ForeignObjectRel(FieldCacheMixin): def path_infos(self): return self.get_path_info() - def get_cache_name(self): + @cached_property + def cache_name(self): """ Return the name of the cache key to use for storing an instance of the forward model on the reverse model. diff --git a/docs/internals/deprecation.txt b/docs/internals/deprecation.txt index 4f89481ac7a..1a74a2a46bb 100644 --- a/docs/internals/deprecation.txt +++ b/docs/internals/deprecation.txt @@ -82,6 +82,8 @@ details on these changes. * The ``OS_OPEN_FLAGS`` attribute of :class:`~django.core.files.storage.FileSystemStorage` will be removed. +* The ``get_cache_name()`` method of ``FieldCacheMixin`` will be removed. + .. _deprecation-removed-in-5.1: 5.1 diff --git a/docs/releases/5.1.txt b/docs/releases/5.1.txt index 388487b3224..bb5e4f3fe41 100644 --- a/docs/releases/5.1.txt +++ b/docs/releases/5.1.txt @@ -474,6 +474,8 @@ Miscellaneous overwriting files in storage, set the new :attr:`~django.core.files.storage.FileSystemStorage.allow_overwrite` option to ``True`` instead. +* The ``get_cache_name()`` method of ``FieldCacheMixin`` is deprecated in favor + of the ``cache_name`` cached property. Features removed in 5.1 ======================= diff --git a/tests/model_fields/test_mixins.py b/tests/model_fields/test_mixins.py new file mode 100644 index 00000000000..5ccfac4d789 --- /dev/null +++ b/tests/model_fields/test_mixins.py @@ -0,0 +1,75 @@ +from django.db.models.fields.mixins import FieldCacheMixin +from django.test import SimpleTestCase +from django.utils.deprecation import RemovedInDjango60Warning +from django.utils.functional import cached_property + +from .models import Foo + + +# RemovedInDjango60Warning. +class ExampleOld(FieldCacheMixin): + def get_cache_name(self): + return "example" + + +class Example(FieldCacheMixin): + @cached_property + def cache_name(self): + return "example" + + +class FieldCacheMixinTests(SimpleTestCase): + def setUp(self): + self.instance = Foo() + self.field = Example() + + # RemovedInDjango60Warning: when the deprecation ends, replace with: + # def test_cache_name_not_implemented(self): + # with self.assertRaises(NotImplementedError): + # FieldCacheMixin().cache_name + def test_get_cache_name_not_implemented(self): + with self.assertRaises(NotImplementedError): + FieldCacheMixin().get_cache_name() + + # RemovedInDjango60Warning. + def test_get_cache_name_deprecated(self): + msg = "Override ExampleOld.cache_name instead of get_cache_name()." + with self.assertWarnsMessage(RemovedInDjango60Warning, msg): + result = ExampleOld().cache_name + self.assertEqual(result, "example") + + def test_cache_name(self): + result = Example().cache_name + self.assertEqual(result, "example") + + def test_get_cached_value_missing(self): + with self.assertRaises(KeyError): + self.field.get_cached_value(self.instance) + + def test_get_cached_value_default(self): + default = object() + result = self.field.get_cached_value(self.instance, default=default) + self.assertIs(result, default) + + def test_get_cached_value_after_set(self): + value = object() + + self.field.set_cached_value(self.instance, value) + result = self.field.get_cached_value(self.instance) + + self.assertIs(result, value) + + def test_is_cached_false(self): + result = self.field.is_cached(self.instance) + self.assertFalse(result) + + def test_is_cached_true(self): + self.field.set_cached_value(self.instance, 1) + result = self.field.is_cached(self.instance) + self.assertTrue(result) + + def test_delete_cached_value(self): + self.field.set_cached_value(self.instance, 1) + self.field.delete_cached_value(self.instance) + result = self.field.is_cached(self.instance) + self.assertFalse(result)