From a697424969f7f464bf6492b09a6cdac135499e02 Mon Sep 17 00:00:00 2001 From: Keryn Knight Date: Thu, 5 Aug 2021 19:11:14 +0100 Subject: [PATCH] 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. --- django/contrib/admin/options.py | 10 +- django/contrib/admin/utils.py | 8 +- django/contrib/contenttypes/fields.py | 2 +- django/db/models/base.py | 2 +- django/db/models/fields/related.py | 40 ++++++- .../db/models/fields/related_descriptors.py | 2 +- django/db/models/fields/related_lookups.py | 10 +- django/db/models/fields/reverse_related.py | 25 ++++- django/db/models/options.py | 2 +- django/db/models/sql/query.py | 7 +- tests/foreign_object/tests.py | 103 ++++++++++++++++++ 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_() 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__)