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.
This commit is contained in:
Keryn Knight 2021-08-05 19:11:14 +01:00 committed by Mariusz Felisiak
parent 3ff7b15bb7
commit a697424969
11 changed files with 182 additions and 29 deletions

View File

@ -400,14 +400,14 @@ class BaseModelAdmin(metaclass=forms.MediaDefiningClass):
# model anyways. For example, if you filter on employee__department__id, # model anyways. For example, if you filter on employee__department__id,
# then the id value would be found already from 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 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) 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 # This is not a relational field, so further parts
# must be transforms. # must be transforms.
break break
prev_field = field 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: if len(relation_parts) <= 1:
# Either a local field filter, or no fields at all. # Either a local field filter, or no fields at all.
@ -1020,9 +1020,9 @@ class ModelAdmin(BaseModelAdmin):
return field_name return field_name
else: else:
prev_field = field prev_field = field
if hasattr(field, 'get_path_info'): if hasattr(field, 'path_infos'):
# Update opts to follow the relation. # 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. # Otherwise, use the field with icontains.
return "%s__icontains" % field_name return "%s__icontains" % field_name

View File

@ -40,9 +40,9 @@ def lookup_spawns_duplicates(opts, lookup_path):
# Ignore query lookups. # Ignore query lookups.
continue continue
else: else:
if hasattr(field, 'get_path_info'): if hasattr(field, 'path_infos'):
# This field is a relation; update opts to follow the relation. # 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 opts = path_info[-1].to_opts
if any(path.m2m for path in path_info): if any(path.m2m for path in path_info):
# This field is a m2m relation so duplicates must be # This field is a m2m relation so duplicates must be
@ -435,8 +435,8 @@ class NotRelationField(Exception):
def get_model_from_relation(field): def get_model_from_relation(field):
if hasattr(field, 'get_path_info'): if hasattr(field, 'path_infos'):
return field.get_path_info()[-1].to_opts.model return field.path_infos[-1].to_opts.model
else: else:
raise NotRelationField raise NotRelationField

View File

@ -395,7 +395,7 @@ class GenericRelation(ForeignObject):
opts = field.remote_field.model._meta opts = field.remote_field.model._meta
parent_field_chain.reverse() parent_field_chain.reverse()
for field in parent_field_chain: for field in parent_field_chain:
path.extend(field.remote_field.get_path_info()) path.extend(field.remote_field.path_infos)
return path return path
def get_path_info(self, filtered_relation=None): def get_path_info(self, filtered_relation=None):

View File

@ -1828,7 +1828,7 @@ class Model(metaclass=ModelBase):
else: else:
fld = _cls._meta.get_field(part) fld = _cls._meta.get_field(part)
if fld.is_relation: if fld.is_relation:
_cls = fld.get_path_info()[-1].to_opts.model _cls = fld.path_infos[-1].to_opts.model
else: else:
_cls = None _cls = None
except (FieldDoesNotExist, AttributeError): except (FieldDoesNotExist, AttributeError):

View File

@ -447,7 +447,7 @@ class RelatedField(FieldCacheMixin, Field):
When filtering against this relation, return the field on the remote When filtering against this relation, return the field on the remote
model against which the filtering should happen. 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: if len(target_fields) > 1:
raise exceptions.FieldError( raise exceptions.FieldError(
"The relation has multiple target fields, but only single target field was asked for") "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.to_fields = to_fields
self.swappable = swappable 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): def check(self, **kwargs):
return [ return [
*super().check(**kwargs), *super().check(**kwargs),
@ -743,6 +750,10 @@ class ForeignObject(RelatedField):
filtered_relation=filtered_relation, filtered_relation=filtered_relation,
)] )]
@cached_property
def path_infos(self):
return self.get_path_info()
def get_reverse_path_info(self, filtered_relation=None): def get_reverse_path_info(self, filtered_relation=None):
"""Get path from the related model to this field's model.""" """Get path from the related model to this field's model."""
opts = self.model._meta opts = self.model._meta
@ -757,6 +768,10 @@ class ForeignObject(RelatedField):
filtered_relation=filtered_relation, filtered_relation=filtered_relation,
)] )]
@cached_property
def reverse_path_infos(self):
return self.get_reverse_path_info()
@classmethod @classmethod
@functools.lru_cache(maxsize=None) @functools.lru_cache(maxsize=None)
def get_lookups(cls): def get_lookups(cls):
@ -1541,12 +1556,17 @@ class ManyToManyField(RelatedField):
linkfield1 = int_model._meta.get_field(self.m2m_field_name()) linkfield1 = int_model._meta.get_field(self.m2m_field_name())
linkfield2 = int_model._meta.get_field(self.m2m_reverse_field_name()) linkfield2 = int_model._meta.get_field(self.m2m_reverse_field_name())
if direct: if direct:
join1infos = linkfield1.get_reverse_path_info() join1infos = linkfield1.reverse_path_infos
join2infos = linkfield2.get_path_info(filtered_relation) if filtered_relation:
join2infos = linkfield2.get_path_info(filtered_relation)
else:
join2infos = linkfield2.path_infos
else: else:
join1infos = linkfield2.get_reverse_path_info() join1infos = linkfield2.reverse_path_infos
join2infos = linkfield1.get_path_info(filtered_relation) 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 # 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 # of join 2. Assume the only reason these may differ is due to model
# inheritance. # inheritance.
@ -1564,9 +1584,17 @@ class ManyToManyField(RelatedField):
def get_path_info(self, filtered_relation=None): def get_path_info(self, filtered_relation=None):
return self._get_path_info(direct=True, filtered_relation=filtered_relation) 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): def get_reverse_path_info(self, filtered_relation=None):
return self._get_path_info(direct=False, filtered_relation=filtered_relation) 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): def _get_m2m_db_table(self, opts):
""" """
Function that can be curried to provide the m2m table name for this Function that can be curried to provide the m2m table name for this

View File

@ -599,7 +599,7 @@ def create_reverse_many_to_one_manager(superclass, rel):
# for related object id. # for related object id.
rel_obj_id = tuple([ rel_obj_id = tuple([
getattr(self.instance, target_field.attname) 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: else:
rel_obj_id = getattr(self.instance, target_field.attname) rel_obj_id = getattr(self.instance, target_field.attname)

View File

@ -30,7 +30,7 @@ def get_normalized_value(value, lhs):
from django.db.models import Model from django.db.models import Model
if isinstance(value, Model): if isinstance(value, Model):
value_list = [] 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: for source in sources:
while not isinstance(value, source.model) and source.remote_field: while not isinstance(value, source.model) and source.remote_field:
source = source.remote_field.model._meta.get_field(source.remote_field.field_name) 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 # ForeignKey to IntegerField given value 'abc'. The ForeignKey itself
# doesn't have validation for non-integers, so we must run validation # doesn't have validation for non-integers, so we must run validation
# using the target field. # 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 # 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. # 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] self.rhs = [target_field.get_prep_value(v) for v in self.rhs]
return super().get_prep_lookup() return super().get_prep_lookup()
@ -113,10 +113,10 @@ class RelatedLookupMixin:
# ForeignKey to IntegerField given value 'abc'. The ForeignKey itself # ForeignKey to IntegerField given value 'abc'. The ForeignKey itself
# doesn't have validation for non-integers, so we must run validation # doesn't have validation for non-integers, so we must run validation
# using the target field. # 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 # Get the target field. We can safely assume there is only one
# as we don't get to the direct value branch otherwise. # 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) self.rhs = target_field.get_prep_value(self.rhs)
return super().get_prep_lookup() return super().get_prep_lookup()

View File

@ -71,7 +71,7 @@ class ForeignObjectRel(FieldCacheMixin):
When filtering against this relation, return the field on the remote When filtering against this relation, return the field on the remote
model against which the filtering should happen. 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: if len(target_fields) > 1:
raise exceptions.FieldError("Can't use target_field for multicolumn relations.") raise exceptions.FieldError("Can't use target_field for multicolumn relations.")
return target_fields[0] return target_fields[0]
@ -138,6 +138,18 @@ class ForeignObjectRel(FieldCacheMixin):
def __hash__(self): def __hash__(self):
return hash(self.identity) 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( def get_choices(
self, include_blank=True, blank_choice=BLANK_CHOICE_DASH, self, include_blank=True, blank_choice=BLANK_CHOICE_DASH,
limit_choices_to=None, ordering=(), limit_choices_to=None, ordering=(),
@ -195,7 +207,14 @@ class ForeignObjectRel(FieldCacheMixin):
return opts.model_name + ('_set' if self.multiple else '') return opts.model_name + ('_set' if self.multiple else '')
def get_path_info(self, filtered_relation=None): 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): def get_cache_name(self):
""" """
@ -234,7 +253,7 @@ class ManyToOneRel(ForeignObjectRel):
self.field_name = field_name self.field_name = field_name
def __getstate__(self): def __getstate__(self):
state = self.__dict__.copy() state = super().__getstate__()
state.pop('related_model', None) state.pop('related_model', None)
return state return state

View File

@ -702,7 +702,7 @@ class Options:
for i, ancestor in enumerate(chain[:-1]): for i, ancestor in enumerate(chain[:-1]):
child = chain[i + 1] child = chain[i + 1]
link = child._meta.get_ancestor_link(ancestor) link = child._meta.get_ancestor_link(ancestor)
path.extend(link.get_reverse_path_info()) path.extend(link.reverse_path_infos)
return path return path
def _populate_directed_relation_graph(self): def _populate_directed_relation_graph(self):

View File

@ -1528,8 +1528,11 @@ class Query(BaseExpression):
path.extend(path_to_parent) path.extend(path_to_parent)
cur_names_with_path[1].extend(path_to_parent) cur_names_with_path[1].extend(path_to_parent)
opts = path_to_parent[-1].to_opts opts = path_to_parent[-1].to_opts
if hasattr(field, 'get_path_info'): if hasattr(field, 'path_infos'):
pathinfos = field.get_path_info(filtered_relation) if filtered_relation:
pathinfos = field.get_path_info(filtered_relation)
else:
pathinfos = field.path_infos
if not allow_many: if not allow_many:
for inner_pos, p in enumerate(pathinfos): for inner_pos, p in enumerate(pathinfos):
if p.m2m: if p.m2m:

View File

@ -1,4 +1,6 @@
import copy
import datetime import datetime
import pickle
from operator import attrgetter from operator import attrgetter
from django.core.exceptions import FieldError from django.core.exceptions import FieldError
@ -482,3 +484,104 @@ class TestExtraJoinFilterQ(TestCase):
qs = qs.select_related('active_translation_q') qs = qs.select_related('active_translation_q')
with self.assertNumQueries(1): with self.assertNumQueries(1):
self.assertEqual(qs[0].active_translation_q.title, 'title') 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__)