From 65d4f98b69328f734abb16aa62952ab6aae576f2 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Mon, 9 Aug 2021 14:43:53 -0400 Subject: [PATCH] create concise + deterministic cache key for unboundload.options Fixed issue in loader strategies where the use of the :meth:`_orm.Load.options` method, particularly when nesting multiple calls, would generate an overly long and more importantly non-deterministic cache key, leading to very large cache keys which were also not allowing efficient cache usage, both in terms of total memory used as well as number of entries used in the cache itself. Fixes: #6869 Change-Id: I42bd3564d55a5fb95a21d0e7eef30d50c1274da0 --- doc/build/changelog/unreleased_14/6869.rst | 10 ++ lib/sqlalchemy/orm/strategy_options.py | 109 ++++++++++++++++++--- test/orm/test_cache_key.py | 23 +++++ 3 files changed, 128 insertions(+), 14 deletions(-) create mode 100644 doc/build/changelog/unreleased_14/6869.rst diff --git a/doc/build/changelog/unreleased_14/6869.rst b/doc/build/changelog/unreleased_14/6869.rst new file mode 100644 index 0000000000..601da86ba4 --- /dev/null +++ b/doc/build/changelog/unreleased_14/6869.rst @@ -0,0 +1,10 @@ +.. change:: + :tags: bug, orm + :tickets: 6869 + + Fixed issue in loader strategies where the use of the + :meth:`_orm.Load.options` method, particularly when nesting multiple calls, + would generate an overly long and more importantly non-deterministic cache + key, leading to very large cache keys which were also not allowing + efficient cache usage, both in terms of total memory used as well as number + of entries used in the cache itself. diff --git a/lib/sqlalchemy/orm/strategy_options.py b/lib/sqlalchemy/orm/strategy_options.py index 399e33b896..ea1e5ea2a8 100644 --- a/lib/sqlalchemy/orm/strategy_options.py +++ b/lib/sqlalchemy/orm/strategy_options.py @@ -28,6 +28,7 @@ from .. import util from ..sql import and_ from ..sql import coercions from ..sql import roles +from ..sql import traversals from ..sql import visitors from ..sql.base import _generative from ..sql.base import Generative @@ -615,16 +616,88 @@ class _UnboundLoad(Load): self.local_opts = {} self._extra_criteria = () - _cache_key_traversal = [ - ("path", visitors.ExtendedInternalTraversal.dp_multi_list), - ("strategy", visitors.ExtendedInternalTraversal.dp_plain_obj), - ("_to_bind", visitors.ExtendedInternalTraversal.dp_has_cache_key_list), - ("_extra_criteria", visitors.InternalTraversal.dp_clauseelement_list), - ( - "local_opts", - visitors.ExtendedInternalTraversal.dp_string_multi_dict, - ), - ] + def _gen_cache_key(self, anon_map, bindparams, _unbound_option_seen=None): + """Inlined gen_cache_key + + Original traversal is:: + + + _cache_key_traversal = [ + ("path", visitors.ExtendedInternalTraversal.dp_multi_list), + ("strategy", visitors.ExtendedInternalTraversal.dp_plain_obj), + ( + "_to_bind", + visitors.ExtendedInternalTraversal.dp_has_cache_key_list, + ), + ( + "_extra_criteria", + visitors.InternalTraversal.dp_clauseelement_list), + ( + "local_opts", + visitors.ExtendedInternalTraversal.dp_string_multi_dict, + ), + ] + + The inlining is so that the "_to_bind" list can be flattened to not + repeat the same UnboundLoad options over and over again. + + See #6869 + + """ + + idself = id(self) + cls = self.__class__ + + if idself in anon_map: + return (anon_map[idself], cls) + else: + id_ = anon_map[idself] + + vis = traversals._cache_key_traversal_visitor + + seen = _unbound_option_seen + if seen is None: + seen = set() + + return ( + (id_, cls) + + vis.visit_multi_list( + "path", self.path, self, anon_map, bindparams + ) + + ("strategy", self.strategy) + + ( + ( + "_to_bind", + tuple( + elem._gen_cache_key( + anon_map, bindparams, _unbound_option_seen=seen + ) + for elem in self._to_bind + if elem not in seen and not seen.add(elem) + ), + ) + if self._to_bind + else () + ) + + ( + ( + "_extra_criteria", + tuple( + elem._gen_cache_key(anon_map, bindparams) + for elem in self._extra_criteria + ), + ) + if self._extra_criteria + else () + ) + + ( + vis.visit_string_multi_dict( + "local_opts", self.local_opts, self, anon_map, bindparams + ) + if self.local_opts + else () + ) + ) _is_chain_link = False @@ -663,12 +736,20 @@ class _UnboundLoad(Load): assert cloned.is_class_strategy == self.is_class_strategy assert cloned.is_opts_only == self.is_opts_only - new_to_bind = { + uniq = set() + + cloned._to_bind = parent._to_bind + + cloned._to_bind[:] = [ + elem + for elem in cloned._to_bind + if elem not in uniq and not uniq.add(elem) + ] + [ elem._apply_to_parent(parent, applied, bound, to_bind) for elem in to_bind - } - cloned._to_bind = parent._to_bind - cloned._to_bind.extend(new_to_bind) + if elem not in uniq and not uniq.add(elem) + ] + cloned.local_opts.update(self.local_opts) return cloned diff --git a/test/orm/test_cache_key.py b/test/orm/test_cache_key.py index 7b6feb96a2..e33c166efc 100644 --- a/test/orm/test_cache_key.py +++ b/test/orm/test_cache_key.py @@ -15,6 +15,7 @@ from sqlalchemy.orm import join as orm_join from sqlalchemy.orm import joinedload from sqlalchemy.orm import lazyload from sqlalchemy.orm import Load +from sqlalchemy.orm import load_only from sqlalchemy.orm import mapper from sqlalchemy.orm import Query from sqlalchemy.orm import relationship @@ -190,6 +191,28 @@ class CacheKeyTest(CacheKeyFixture, _fixtures.FixtureTest): compare_values=True, ) + def test_unbound_sub_options(self): + """test #6869""" + + User, Address, Keyword, Order, Item = self.classes( + "User", "Address", "Keyword", "Order", "Item" + ) + + self._run_cache_key_fixture( + lambda: ( + joinedload(User.addresses).options( + joinedload(Address.dingaling) + ), + joinedload(User.addresses).options( + joinedload(Address.dingaling).options(load_only("name")) + ), + joinedload(User.orders).options( + joinedload(Order.items).options(joinedload(Item.keywords)) + ), + ), + compare_values=True, + ) + def test_bound_options(self): User, Address, Keyword, Order, Item = self.classes( "User", "Address", "Keyword", "Order", "Item" -- 2.47.2