summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorKeryn Knight <keryn@kerynknight.com>2021-08-05 19:11:14 +0100
committerMariusz Felisiak <felisiak.mariusz@gmail.com>2021-11-03 11:27:04 +0100
commita697424969f7f464bf6492b09a6cdac135499e02 (patch)
tree179e1165bc361e3fe8b822135790ea971449a4d4
parent3ff7b15bb79f2ee5b7af245c55ae14546243bb77 (diff)
Fixed #32996 -- Cached PathInfos on relations.
PathInfo values are ostensibly static over the lifetime of the object for which they're requested, so the data can be memoized, quickly amortising the cost over the process' duration.
-rw-r--r--django/contrib/admin/options.py10
-rw-r--r--django/contrib/admin/utils.py8
-rw-r--r--django/contrib/contenttypes/fields.py2
-rw-r--r--django/db/models/base.py2
-rw-r--r--django/db/models/fields/related.py40
-rw-r--r--django/db/models/fields/related_descriptors.py2
-rw-r--r--django/db/models/fields/related_lookups.py10
-rw-r--r--django/db/models/fields/reverse_related.py25
-rw-r--r--django/db/models/options.py2
-rw-r--r--django/db/models/sql/query.py7
-rw-r--r--tests/foreign_object/tests.py103
11 files changed, 182 insertions, 29 deletions
diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py
index 7b2b893c10..66f3396a6b 100644
--- a/django/contrib/admin/options.py
+++ b/django/contrib/admin/options.py
@@ -400,14 +400,14 @@ class BaseModelAdmin(metaclass=forms.MediaDefiningClass):
# model anyways. For example, if you filter on employee__department__id,
# then the id value would be found already from employee__department_id.
if not prev_field or (prev_field.is_relation and
- field not in prev_field.get_path_info()[-1].target_fields):
+ field not in prev_field.path_infos[-1].target_fields):
relation_parts.append(part)
- if not getattr(field, 'get_path_info', None):
+ if not getattr(field, 'path_infos', None):
# This is not a relational field, so further parts
# must be transforms.
break
prev_field = field
- model = field.get_path_info()[-1].to_opts.model
+ model = field.path_infos[-1].to_opts.model
if len(relation_parts) <= 1:
# Either a local field filter, or no fields at all.
@@ -1020,9 +1020,9 @@ class ModelAdmin(BaseModelAdmin):
return field_name
else:
prev_field = field
- if hasattr(field, 'get_path_info'):
+ if hasattr(field, 'path_infos'):
# Update opts to follow the relation.
- opts = field.get_path_info()[-1].to_opts
+ opts = field.path_infos[-1].to_opts
# Otherwise, use the field with icontains.
return "%s__icontains" % field_name
diff --git a/django/contrib/admin/utils.py b/django/contrib/admin/utils.py
index 5821db5ffe..baaaa9e43f 100644
--- a/django/contrib/admin/utils.py
+++ b/django/contrib/admin/utils.py
@@ -40,9 +40,9 @@ def lookup_spawns_duplicates(opts, lookup_path):
# Ignore query lookups.
continue
else:
- if hasattr(field, 'get_path_info'):
+ if hasattr(field, 'path_infos'):
# This field is a relation; update opts to follow the relation.
- path_info = field.get_path_info()
+ path_info = field.path_infos
opts = path_info[-1].to_opts
if any(path.m2m for path in path_info):
# This field is a m2m relation so duplicates must be
@@ -435,8 +435,8 @@ class NotRelationField(Exception):
def get_model_from_relation(field):
- if hasattr(field, 'get_path_info'):
- return field.get_path_info()[-1].to_opts.model
+ if hasattr(field, 'path_infos'):
+ return field.path_infos[-1].to_opts.model
else:
raise NotRelationField
diff --git a/django/contrib/contenttypes/fields.py b/django/contrib/contenttypes/fields.py
index 63ee408bf5..b0b77b01d4 100644
--- a/django/contrib/contenttypes/fields.py
+++ b/django/contrib/contenttypes/fields.py
@@ -395,7 +395,7 @@ class GenericRelation(ForeignObject):
opts = field.remote_field.model._meta
parent_field_chain.reverse()
for field in parent_field_chain:
- path.extend(field.remote_field.get_path_info())
+ path.extend(field.remote_field.path_infos)
return path
def get_path_info(self, filtered_relation=None):
diff --git a/django/db/models/base.py b/django/db/models/base.py
index 3c46afcf9f..cc03ae1252 100644
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -1828,7 +1828,7 @@ class Model(metaclass=ModelBase):
else:
fld = _cls._meta.get_field(part)
if fld.is_relation:
- _cls = fld.get_path_info()[-1].to_opts.model
+ _cls = fld.path_infos[-1].to_opts.model
else:
_cls = None
except (FieldDoesNotExist, AttributeError):
diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py
index 2e8c87c1b1..8533803ba6 100644
--- a/django/db/models/fields/related.py
+++ b/django/db/models/fields/related.py
@@ -447,7 +447,7 @@ class RelatedField(FieldCacheMixin, Field):
When filtering against this relation, return the field on the remote
model against which the filtering should happen.
"""
- target_fields = self.get_path_info()[-1].target_fields
+ target_fields = self.path_infos[-1].target_fields
if len(target_fields) > 1:
raise exceptions.FieldError(
"The relation has multiple target fields, but only single target field was asked for")
@@ -499,6 +499,13 @@ class ForeignObject(RelatedField):
self.to_fields = to_fields
self.swappable = swappable
+ def __copy__(self):
+ obj = super().__copy__()
+ # Remove any cached PathInfo values.
+ obj.__dict__.pop('path_infos', None)
+ obj.__dict__.pop('reverse_path_infos', None)
+ return obj
+
def check(self, **kwargs):
return [
*super().check(**kwargs),
@@ -743,6 +750,10 @@ class ForeignObject(RelatedField):
filtered_relation=filtered_relation,
)]
+ @cached_property
+ def path_infos(self):
+ return self.get_path_info()
+
def get_reverse_path_info(self, filtered_relation=None):
"""Get path from the related model to this field's model."""
opts = self.model._meta
@@ -757,6 +768,10 @@ class ForeignObject(RelatedField):
filtered_relation=filtered_relation,
)]
+ @cached_property
+ def reverse_path_infos(self):
+ return self.get_reverse_path_info()
+
@classmethod
@functools.lru_cache(maxsize=None)
def get_lookups(cls):
@@ -1541,12 +1556,17 @@ class ManyToManyField(RelatedField):
linkfield1 = int_model._meta.get_field(self.m2m_field_name())
linkfield2 = int_model._meta.get_field(self.m2m_reverse_field_name())
if direct:
- join1infos = linkfield1.get_reverse_path_info()
- join2infos = linkfield2.get_path_info(filtered_relation)
+ join1infos = linkfield1.reverse_path_infos
+ if filtered_relation:
+ join2infos = linkfield2.get_path_info(filtered_relation)
+ else:
+ join2infos = linkfield2.path_infos
else:
- join1infos = linkfield2.get_reverse_path_info()
- join2infos = linkfield1.get_path_info(filtered_relation)
-
+ join1infos = linkfield2.reverse_path_infos
+ if filtered_relation:
+ join2infos = linkfield1.get_path_info(filtered_relation)
+ else:
+ join2infos = linkfield1.path_infos
# Get join infos between the last model of join 1 and the first model
# of join 2. Assume the only reason these may differ is due to model
# inheritance.
@@ -1564,9 +1584,17 @@ class ManyToManyField(RelatedField):
def get_path_info(self, filtered_relation=None):
return self._get_path_info(direct=True, filtered_relation=filtered_relation)
+ @cached_property
+ def path_infos(self):
+ return self.get_path_info()
+
def get_reverse_path_info(self, filtered_relation=None):
return self._get_path_info(direct=False, filtered_relation=filtered_relation)
+ @cached_property
+ def reverse_path_infos(self):
+ return self.get_reverse_path_info()
+
def _get_m2m_db_table(self, opts):
"""
Function that can be curried to provide the m2m table name for this
diff --git a/django/db/models/fields/related_descriptors.py b/django/db/models/fields/related_descriptors.py
index cb77a0c476..ef546e844a 100644
--- a/django/db/models/fields/related_descriptors.py
+++ b/django/db/models/fields/related_descriptors.py
@@ -599,7 +599,7 @@ def create_reverse_many_to_one_manager(superclass, rel):
# for related object id.
rel_obj_id = tuple([
getattr(self.instance, target_field.attname)
- for target_field in self.field.get_path_info()[-1].target_fields
+ for target_field in self.field.path_infos[-1].target_fields
])
else:
rel_obj_id = getattr(self.instance, target_field.attname)
diff --git a/django/db/models/fields/related_lookups.py b/django/db/models/fields/related_lookups.py
index 34cca8ba5e..50f8b44158 100644
--- a/django/db/models/fields/related_lookups.py
+++ b/django/db/models/fields/related_lookups.py
@@ -30,7 +30,7 @@ def get_normalized_value(value, lhs):
from django.db.models import Model
if isinstance(value, Model):
value_list = []
- sources = lhs.output_field.get_path_info()[-1].target_fields
+ sources = lhs.output_field.path_infos[-1].target_fields
for source in sources:
while not isinstance(value, source.model) and source.remote_field:
source = source.remote_field.model._meta.get_field(source.remote_field.field_name)
@@ -55,10 +55,10 @@ class RelatedIn(In):
# ForeignKey to IntegerField given value 'abc'. The ForeignKey itself
# doesn't have validation for non-integers, so we must run validation
# using the target field.
- if hasattr(self.lhs.output_field, 'get_path_info'):
+ if hasattr(self.lhs.output_field, 'path_infos'):
# Run the target field's get_prep_value. We can safely assume there is
# only one as we don't get to the direct value branch otherwise.
- target_field = self.lhs.output_field.get_path_info()[-1].target_fields[-1]
+ target_field = self.lhs.output_field.path_infos[-1].target_fields[-1]
self.rhs = [target_field.get_prep_value(v) for v in self.rhs]
return super().get_prep_lookup()
@@ -113,10 +113,10 @@ class RelatedLookupMixin:
# ForeignKey to IntegerField given value 'abc'. The ForeignKey itself
# doesn't have validation for non-integers, so we must run validation
# using the target field.
- if self.prepare_rhs and hasattr(self.lhs.output_field, 'get_path_info'):
+ if self.prepare_rhs and hasattr(self.lhs.output_field, 'path_infos'):
# Get the target field. We can safely assume there is only one
# as we don't get to the direct value branch otherwise.
- target_field = self.lhs.output_field.get_path_info()[-1].target_fields[-1]
+ target_field = self.lhs.output_field.path_infos[-1].target_fields[-1]
self.rhs = target_field.get_prep_value(self.rhs)
return super().get_prep_lookup()
diff --git a/django/db/models/fields/reverse_related.py b/django/db/models/fields/reverse_related.py
index 65950590e2..6f0c788bbd 100644
--- a/django/db/models/fields/reverse_related.py
+++ b/django/db/models/fields/reverse_related.py
@@ -71,7 +71,7 @@ class ForeignObjectRel(FieldCacheMixin):
When filtering against this relation, return the field on the remote
model against which the filtering should happen.
"""
- target_fields = self.get_path_info()[-1].target_fields
+ target_fields = self.path_infos[-1].target_fields
if len(target_fields) > 1:
raise exceptions.FieldError("Can't use target_field for multicolumn relations.")
return target_fields[0]
@@ -138,6 +138,18 @@ class ForeignObjectRel(FieldCacheMixin):
def __hash__(self):
return hash(self.identity)
+ def __getstate__(self):
+ state = self.__dict__.copy()
+ # Delete the path_infos cached property because it can be recalculated
+ # at first invocation after deserialization. The attribute must be
+ # removed because subclasses like ManyToOneRel may have a PathInfo
+ # which contains an intermediate M2M table that's been dynamically
+ # created and doesn't exist in the .models module.
+ # This is a reverse relation, so there is no reverse_path_infos to
+ # delete.
+ state.pop('path_infos', None)
+ return state
+
def get_choices(
self, include_blank=True, blank_choice=BLANK_CHOICE_DASH,
limit_choices_to=None, ordering=(),
@@ -195,7 +207,14 @@ class ForeignObjectRel(FieldCacheMixin):
return opts.model_name + ('_set' if self.multiple else '')
def get_path_info(self, filtered_relation=None):
- return self.field.get_reverse_path_info(filtered_relation)
+ if filtered_relation:
+ return self.field.get_reverse_path_info(filtered_relation)
+ else:
+ return self.field.reverse_path_infos
+
+ @cached_property
+ def path_infos(self):
+ return self.get_path_info()
def get_cache_name(self):
"""
@@ -234,7 +253,7 @@ class ManyToOneRel(ForeignObjectRel):
self.field_name = field_name
def __getstate__(self):
- state = self.__dict__.copy()
+ state = super().__getstate__()
state.pop('related_model', None)
return state
diff --git a/django/db/models/options.py b/django/db/models/options.py
index 0788ba3298..5f0f8f0e5f 100644
--- a/django/db/models/options.py
+++ b/django/db/models/options.py
@@ -702,7 +702,7 @@ class Options:
for i, ancestor in enumerate(chain[:-1]):
child = chain[i + 1]
link = child._meta.get_ancestor_link(ancestor)
- path.extend(link.get_reverse_path_info())
+ path.extend(link.reverse_path_infos)
return path
def _populate_directed_relation_graph(self):
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
index 171e72cfb8..2c5f11cbbf 100644
--- a/django/db/models/sql/query.py
+++ b/django/db/models/sql/query.py
@@ -1528,8 +1528,11 @@ class Query(BaseExpression):
path.extend(path_to_parent)
cur_names_with_path[1].extend(path_to_parent)
opts = path_to_parent[-1].to_opts
- if hasattr(field, 'get_path_info'):
- pathinfos = field.get_path_info(filtered_relation)
+ if hasattr(field, 'path_infos'):
+ if filtered_relation:
+ pathinfos = field.get_path_info(filtered_relation)
+ else:
+ pathinfos = field.path_infos
if not allow_many:
for inner_pos, p in enumerate(pathinfos):
if p.m2m:
diff --git a/tests/foreign_object/tests.py b/tests/foreign_object/tests.py
index 72d50cad6b..8b61c87e5d 100644
--- a/tests/foreign_object/tests.py
+++ b/tests/foreign_object/tests.py
@@ -1,4 +1,6 @@
+import copy
import datetime
+import pickle
from operator import attrgetter
from django.core.exceptions import FieldError
@@ -482,3 +484,104 @@ class TestExtraJoinFilterQ(TestCase):
qs = qs.select_related('active_translation_q')
with self.assertNumQueries(1):
self.assertEqual(qs[0].active_translation_q.title, 'title')
+
+
+class TestCachedPathInfo(TestCase):
+ def test_equality(self):
+ """
+ The path_infos and reverse_path_infos attributes are equivalent to
+ calling the get_<method>() with no arguments.
+ """
+ foreign_object = Membership._meta.get_field('person')
+ self.assertEqual(
+ foreign_object.path_infos,
+ foreign_object.get_path_info(),
+ )
+ self.assertEqual(
+ foreign_object.reverse_path_infos,
+ foreign_object.get_reverse_path_info(),
+ )
+
+ def test_copy_removes_direct_cached_values(self):
+ """
+ Shallow copying a ForeignObject (or a ForeignObjectRel) removes the
+ object's direct cached PathInfo values.
+ """
+ foreign_object = Membership._meta.get_field('person')
+ # Trigger storage of cached_property into ForeignObject's __dict__.
+ foreign_object.path_infos
+ foreign_object.reverse_path_infos
+ # The ForeignObjectRel doesn't have reverse_path_infos.
+ foreign_object.remote_field.path_infos
+ self.assertIn('path_infos', foreign_object.__dict__)
+ self.assertIn('reverse_path_infos', foreign_object.__dict__)
+ self.assertIn('path_infos', foreign_object.remote_field.__dict__)
+ # Cached value is removed via __getstate__() on ForeignObjectRel
+ # because no __copy__() method exists, so __reduce_ex__() is used.
+ remote_field_copy = copy.copy(foreign_object.remote_field)
+ self.assertNotIn('path_infos', remote_field_copy.__dict__)
+ # Cached values are removed via __copy__() on ForeignObject for
+ # consistency of behavior.
+ foreign_object_copy = copy.copy(foreign_object)
+ self.assertNotIn('path_infos', foreign_object_copy.__dict__)
+ self.assertNotIn('reverse_path_infos', foreign_object_copy.__dict__)
+ # ForeignObjectRel's remains because it's part of a shallow copy.
+ self.assertIn('path_infos', foreign_object_copy.remote_field.__dict__)
+
+ def test_deepcopy_removes_cached_values(self):
+ """
+ Deep copying a ForeignObject removes the object's cached PathInfo
+ values, including those of the related ForeignObjectRel.
+ """
+ foreign_object = Membership._meta.get_field('person')
+ # Trigger storage of cached_property into ForeignObject's __dict__.
+ foreign_object.path_infos
+ foreign_object.reverse_path_infos
+ # The ForeignObjectRel doesn't have reverse_path_infos.
+ foreign_object.remote_field.path_infos
+ self.assertIn('path_infos', foreign_object.__dict__)
+ self.assertIn('reverse_path_infos', foreign_object.__dict__)
+ self.assertIn('path_infos', foreign_object.remote_field.__dict__)
+ # Cached value is removed via __getstate__() on ForeignObjectRel
+ # because no __deepcopy__() method exists, so __reduce_ex__() is used.
+ remote_field_copy = copy.deepcopy(foreign_object.remote_field)
+ self.assertNotIn('path_infos', remote_field_copy.__dict__)
+ # Field.__deepcopy__() internally uses __copy__() on both the
+ # ForeignObject and ForeignObjectRel, so all cached values are removed.
+ foreign_object_copy = copy.deepcopy(foreign_object)
+ self.assertNotIn('path_infos', foreign_object_copy.__dict__)
+ self.assertNotIn('reverse_path_infos', foreign_object_copy.__dict__)
+ self.assertNotIn('path_infos', foreign_object_copy.remote_field.__dict__)
+
+ def test_pickling_foreignobjectrel(self):
+ """
+ Pickling a ForeignObjectRel removes the path_infos attribute.
+
+ ForeignObjectRel implements __getstate__(), so copy and pickle modules
+ both use that, but ForeignObject implements __reduce__() and __copy__()
+ separately, so doesn't share the same behaviour.
+ """
+ foreign_object_rel = Membership._meta.get_field('person').remote_field
+ # Trigger storage of cached_property into ForeignObjectRel's __dict__.
+ foreign_object_rel.path_infos
+ self.assertIn('path_infos', foreign_object_rel.__dict__)
+ foreign_object_rel_restored = pickle.loads(pickle.dumps(foreign_object_rel))
+ self.assertNotIn('path_infos', foreign_object_rel_restored.__dict__)
+
+ def test_pickling_foreignobject(self):
+ """
+ Pickling a ForeignObject does not remove the cached PathInfo values.
+
+ ForeignObject will always keep the path_infos and reverse_path_infos
+ attributes within the same process, because of the way
+ Field.__reduce__() is used for restoring values.
+ """
+ foreign_object = Membership._meta.get_field('person')
+ # Trigger storage of cached_property into ForeignObjectRel's __dict__
+ foreign_object.path_infos
+ foreign_object.reverse_path_infos
+ self.assertIn('path_infos', foreign_object.__dict__)
+ self.assertIn('reverse_path_infos', foreign_object.__dict__)
+ foreign_object_restored = pickle.loads(pickle.dumps(foreign_object))
+ self.assertIn('path_infos', foreign_object_restored.__dict__)
+ self.assertIn('reverse_path_infos', foreign_object_restored.__dict__)