Merge "refine transfer of cached ORM options for selectin, lazy" into main

4 files changed, 103 insertions, 66 deletions

diff --git a/lib/sqlalchemy/orm/context.py b/lib/sqlalchemy/orm/context.py
index f8ea23139..b762908c5 100644
--- a/lib/sqlalchemy/orm/context.py
+++ b/lib/sqlalchemy/orm/context.py
@@ -163,28 +163,24 @@ class QueryContext:
         self.params = params
         self.top_level_context = load_options._sa_top_level_orm_context
 
+        cached_options = compile_state.select_statement._with_options
+        uncached_options = statement._with_options
+
+        # see issue #7447 , #8399 for some background
+        # propagated loader options will be present on loaded InstanceState
+        # objects under state.load_options and are typically used by
+        # LazyLoader to apply options to the SELECT statement it emits.
+        # For compile state options (i.e. loader strategy options), these
+        # need to line up with the ".load_path" attribute which in
+        # loader.py is pulled from context.compile_state.current_path.
+        # so, this means these options have to be the ones from the
+        # *cached* statement that's travelling with compile_state, not the
+        # *current* statement which won't match up for an ad-hoc
+        # AliasedClass
         self.propagated_loader_options = tuple(
-            # issue 7447.
-            # propagated loader options will be present on loaded InstanceState
-            # objects under state.load_options and are typically used by
-            # LazyLoader to apply options to the SELECT statement it emits.
-            # For compile state options (i.e. loader strategy options), these
-            # need to line up with the ".load_path" attribute which in
-            # loader.py is pulled from context.compile_state.current_path.
-            # so, this means these options have to be the ones from the
-            # *cached* statement that's travelling with compile_state, not the
-            # *current* statement which won't match up for an ad-hoc
-            # AliasedClass
-            cached_o
-            for cached_o in compile_state.select_statement._with_options
-            if cached_o.propagate_to_loaders and cached_o._is_compile_state
-        ) + tuple(
-            # for user defined loader options that are not "compile state",
-            # those just need to be present as they are
-            uncached_o
-            for uncached_o in statement._with_options
-            if uncached_o.propagate_to_loaders
-            and not uncached_o._is_compile_state
+            opt._adapt_cached_option_to_uncached_option(self, uncached_opt)
+            for opt, uncached_opt in zip(cached_options, uncached_options)
+            if opt.propagate_to_loaders
         )
 
         self.attributes = dict(compile_state.attributes)
diff --git a/lib/sqlalchemy/orm/interfaces.py b/lib/sqlalchemy/orm/interfaces.py
index 72f5c6a7b..f02d551d2 100644
--- a/lib/sqlalchemy/orm/interfaces.py
+++ b/lib/sqlalchemy/orm/interfaces.py
@@ -78,6 +78,7 @@ if typing.TYPE_CHECKING:
     from .attributes import InstrumentedAttribute
     from .context import _MapperEntity
     from .context import ORMCompileState
+    from .context import QueryContext
     from .decl_api import RegistryType
     from .loading import _PopulatorDict
     from .mapper import Mapper
@@ -1093,6 +1094,50 @@ class ORMOption(ExecutableOption):
 
     _is_strategy_option = False
 
+    def _adapt_cached_option_to_uncached_option(
+        self, context: QueryContext, uncached_opt: ORMOption
+    ) -> ORMOption:
+        """adapt this option to the "uncached" version of itself in a
+        loader strategy context.
+
+        given "self" which is an option from a cached query, as well as the
+        corresponding option from the uncached version of the same query,
+        return the option we should use in a new query, in the context of a
+        loader strategy being asked to load related rows on behalf of that
+        cached query, which is assumed to be building a new query based on
+        entities passed to us from the cached query.
+
+        Currently this routine chooses between "self" and "uncached" without
+        manufacturing anything new. If the option is itself a loader strategy
+        option which has a path, that path needs to match to the entities being
+        passed to us by the cached query, so the :class:`_orm.Load` subclass
+        overrides this to return "self". For all other options, we return the
+        uncached form which may have changing state, such as a
+        with_loader_criteria() option which will very often have new state.
+
+        This routine could in the future involve
+        generating a new option based on both inputs if use cases arise,
+        such as if with_loader_criteria() needed to match up to
+        ``AliasedClass`` instances given in the parent query.
+
+        However, longer term it might be better to restructure things such that
+        ``AliasedClass`` entities are always matched up on their cache key,
+        instead of identity, in things like paths and such, so that this whole
+        issue of "the uncached option does not match the entities" goes away.
+        However this would make ``PathRegistry`` more complicated and difficult
+        to debug as well as potentially less performant in that it would be
+        hashing enormous cache keys rather than a simple AliasedInsp. UNLESS,
+        we could get cache keys overall to be reliably hashed into something
+        like an md5 key.
+
+        .. versionadded:: 1.4.41
+
+        """
+        if uncached_opt is not None:
+            return uncached_opt
+        else:
+            return self
+
 
 class CompileStateOption(HasCacheKey, ORMOption):
     """base for :class:`.ORMOption` classes that affect the compilation of
diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index 2c707439a..599a12761 100644
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -1026,7 +1026,6 @@ class LazyLoader(
 
         if extra_options:
             stmt._with_options += extra_options
-
         stmt._compile_options += {"_current_path": effective_path}
 
         if use_get:
@@ -3092,29 +3091,25 @@ class SelectInLoader(PostLoader, util.MemoizedSlots):
         # cached query, meaning it won't match on paths and loader lookups
         # and loaders like this one will be skipped if it is used in options.
         #
-        # Now we want to transfer loader options from the parent query to the
-        # "selectinload" query we're about to run.   Which query do we transfer
-        # the options from?  We use the cached query, because the options in
-        # that query will be in terms of the effective entity we were just
-        # handed.
+        # as it turns out, standard loader options like selectinload(),
+        # lazyload() that have a path need
+        # to come from the cached query so that the AliasedInsp etc. objects
+        # that are in the query line up with the object that's in the path
+        # of the strategy object. however other options like
+        # with_loader_criteria() that doesn't have a path (has a fixed entity)
+        # and needs to have access to the latest closure state in order to
+        # be correct, we need to use the uncached one.
         #
-        # But now the selectinload query we are running is *also*
-        # cached.  What if it's cached and running from some previous iteration
-        # of that AliasedInsp?  Well in that case it will also use the previous
-        # iteration of the loader options.   If the query expires and
-        # gets generated again, it will be handed the current effective_entity
-        # and the current _with_options, again in terms of whatever
-        # compile_state.select_statement happens to be right now, so the
-        # query will still be internally consistent and loader callables
-        # will be correctly invoked.
+        # as of #8399 we let the loader option itself figure out what it
+        # wants to do given cached and uncached version of itself.
 
         effective_path = path[self.parent_property]
 
         if orig_query is context.query:
-            options = new_options = orig_query._with_options
-            user_defined_options = []
+            new_options = orig_query._with_options
         else:
-            options = orig_query._with_options
+            cached_options = orig_query._with_options
+            uncached_options = context.query._with_options
 
             # propagate compile state options from the original query,
             # updating their "extra_criteria" as necessary.
@@ -3122,20 +3117,13 @@ class SelectInLoader(PostLoader, util.MemoizedSlots):
             # "orig" options if extra_criteria is present, because the copy
             # of extra_criteria will have different boundparam than that of
             # the QueryableAttribute in the path
-
             new_options = [
-                orig_opt._adjust_for_extra_criteria(context)
-                if orig_opt._is_strategy_option
-                else orig_opt
-                for orig_opt in options
-                if orig_opt._is_compile_state or orig_opt._is_legacy_option
-            ]
-
-            # propagate user defined options from the current query
-            user_defined_options = [
-                opt
-                for opt in context.query._with_options
-                if not opt._is_compile_state and not opt._is_legacy_option
+                orig_opt._adapt_cached_option_to_uncached_option(
+                    context, uncached_opt
+                )
+                for orig_opt, uncached_opt in zip(
+                    cached_options, uncached_options
+                )
             ]
 
         if loadopt and loadopt._extra_criteria:
@@ -3149,13 +3137,9 @@ class SelectInLoader(PostLoader, util.MemoizedSlots):
         if recursion_depth is not None:
             effective_path = effective_path._truncate_recursive()
 
-        q = q.options(*new_options)._update_compile_options(
-            {"_current_path": effective_path}
-        )
-
-        if user_defined_options:
-            q = q.options(*user_defined_options)
+        q = q.options(*new_options)
 
+        q = q._update_compile_options({"_current_path": effective_path})
         if context.populate_existing:
             q = q.execution_options(populate_existing=True)
 
diff --git a/lib/sqlalchemy/orm/strategy_options.py b/lib/sqlalchemy/orm/strategy_options.py
index aa51eca16..2aed60b61 100644
--- a/lib/sqlalchemy/orm/strategy_options.py
+++ b/lib/sqlalchemy/orm/strategy_options.py
@@ -67,6 +67,7 @@ if typing.TYPE_CHECKING:
     from .context import QueryContext
     from .interfaces import _StrategyKey
     from .interfaces import MapperProperty
+    from .interfaces import ORMOption
     from .mapper import Mapper
     from .path_registry import _PathRepresentation
     from ..sql._typing import _ColumnExpressionArgument
@@ -1072,6 +1073,11 @@ class Load(_AbstractLoad):
         load.propagate_to_loaders = False
         return load
 
+    def _adapt_cached_option_to_uncached_option(
+        self, context: QueryContext, uncached_opt: ORMOption
+    ) -> ORMOption:
+        return self._adjust_for_extra_criteria(context)
+
     def _adjust_for_extra_criteria(self, context: QueryContext) -> Load:
         """Apply the current bound parameters in a QueryContext to all
         occurrences "extra_criteria" stored within this ``Load`` object,
@@ -1082,12 +1088,15 @@ class Load(_AbstractLoad):
 
         orig_cache_key: Optional[CacheKey] = None
         replacement_cache_key: Optional[CacheKey] = None
+        found_crit = False
 
         def process(opt: _LoadElement) -> _LoadElement:
             if not opt._extra_criteria:
                 return opt
 
-            nonlocal orig_cache_key, replacement_cache_key
+            nonlocal orig_cache_key, replacement_cache_key, found_crit
+
+            found_crit = True
 
             # avoid generating cache keys for the queries if we don't
             # actually have any extra_criteria options, which is the
@@ -1107,14 +1116,17 @@ class Load(_AbstractLoad):
             )
             return opt
 
-        cloned = self._generate()
-
-        if self.context:
-            cloned.context = tuple(
-                process(value._clone()) for value in self.context
-            )
+        new_context = tuple(
+            process(value._clone()) if value._extra_criteria else value
+            for value in self.context
+        )
 
-        return cloned
+        if found_crit:
+            cloned = self._clone()
+            cloned.context = new_context
+            return cloned
+        else:
+            return self
 
     def _reconcile_query_entities_with_us(self, mapper_entities, raiseerr):
         """called at process time to allow adjustment of the root