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

diff --git a/doc/build/changelog/unreleased_14/6869.rst b/doc/build/changelog/unreleased_14/6869.rst
new file mode 100644 (file)
index 0000000..601da86
--- /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 399e33b896247f04f3a11fc51065f85fbc1135ca..ea1e5ea2a8e5119e89bbabc35be08b1bf66d0e24 100644 (file)
--- 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 7b6feb96a2fd5f46c4892b5ed00879a2ef952022..e33c166efcb5cef7e0a22b41342637108d9d77d0 100644 (file)
--- 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"