Browse Source

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 <charette.s@gmail.com>
Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>
Co-authored-by: Natalia <124304+nessita@users.noreply.github.com>
Adam Johnson 10 months ago
parent
commit
b9838c65ec

+ 2 - 1
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):

+ 27 - 6
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:

+ 2 - 1
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
 
 

+ 5 - 5
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):

+ 2 - 1
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.

+ 2 - 0
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

+ 2 - 0
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
 =======================

+ 75 - 0
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)