Merge existing query params in baked lazy load

Fixed a long-standing regression that occurred in version
1.0, which prevented the use of a custom :class:`.MapperOption`
that alters the _params of a :class:`.Query` object for a
lazy load, since the lazy loader itself would overwrite those
parameters.   This applies to the "temporal range" example
on the wiki.  Note however that the
:meth:`.Query.populate_existing` method is now required in
order to rewrite the mapper options associated with an object
already loaded in the identity map.  Also, a custom defined
:class:`.MapperOption` will now cause lazy loaders related to
the target object to use a non-baked query by default unless
the :meth:`.MapperOption._generate_cache_key` method is implemented.

Fixed bug where the new :meth:`.baked.Result.with_post_criteria`
method would not interact with a subquery-eager loader correctly,
in that the "post criteria" would not be applied to embedded
subquery eager loaders.   This is related to :ticket:`4128` in that
the post criteria feature is now used by the lazy loader.

Change-Id: I899808734458e25a023142c2c5bb37cbed869479
Fixes: #4128

3 files changed, 63 insertions, 21 deletions

diff --git a/lib/sqlalchemy/ext/baked.py b/lib/sqlalchemy/ext/baked.py
index 9d5704471..86eee831b 100644
--- a/lib/sqlalchemy/ext/baked.py
+++ b/lib/sqlalchemy/ext/baked.py
@@ -240,7 +240,8 @@ class BakedQuery(object):
                     baked_queries.append((k, bk._cache_key, v))
                 del context.attributes[k]
 
-    def _unbake_subquery_loaders(self, session, context, params):
+    def _unbake_subquery_loaders(
+            self, session, context, params, post_criteria):
         """Retrieve subquery eager loaders stored by _bake_subquery_loaders
         and turn them back into Result objects that will iterate just
         like a Query object.
@@ -250,7 +251,10 @@ class BakedQuery(object):
             bk = BakedQuery(self._bakery,
                             lambda sess, q=query: q.with_session(sess))
             bk._cache_key = cache_key
-            context.attributes[k] = bk.for_session(session).params(**params)
+            q = bk.for_session(session)
+            for fn in post_criteria:
+                q = fn(q)
+            context.attributes[k] = q.params(**params)
 
 
 class Result(object):
@@ -329,7 +333,8 @@ class Result(object):
         context.session = self.session
         context.attributes = context.attributes.copy()
 
-        bq._unbake_subquery_loaders(self.session, context, self._params)
+        bq._unbake_subquery_loaders(
+            self.session, context, self._params, self._post_criteria)
 
         context.statement.use_labels = True
         if context.autoflush and not context.populate_existing:
diff --git a/lib/sqlalchemy/orm/interfaces.py b/lib/sqlalchemy/orm/interfaces.py
index 1a869b28c..915768b01 100644
--- a/lib/sqlalchemy/orm/interfaces.py
+++ b/lib/sqlalchemy/orm/interfaces.py
@@ -597,26 +597,54 @@ class MapperOption(object):
         self.process_query(query)
 
     def _generate_cache_key(self, path):
-        """Used by the baked loader to see if this option can be cached.
-
-        A given MapperOption that returns a cache key must return a key
-        that uniquely identifies the complete state of this option, which
-        will match any other MapperOption that itself retains the identical
-        state.  This includes path options, flags, etc.
-
-        If the MapperOption does not apply to the given path and would
-        not affect query results on such a path, it should return None.
-
-        if the MapperOption **does** apply to the give path, however cannot
-        produce a safe cache key, it should return False; this will cancel
-        caching of the result.   An unsafe cache key is one that includes
-        an ad-hoc user object, typically an AliasedClass object.  As these
-        are usually created per-query, they don't work as cache keys.
+        """Used by the "baked lazy loader" to see if this option can be cached.
+
+        The "baked lazy loader" refers to the :class:`.Query` that is
+        produced during a lazy load operation for a mapped relationship.
+        It does not yet apply to the "lazy" load operation for deferred
+        or expired column attributes, however this may change in the future.
+
+        This loader generates SQL for a query only once and attempts to  cache
+        it; from that point on, if the SQL has been cached it will no longer
+        run the :meth:`.Query.options` method of the :class:`.Query`.   The
+        :class:`.MapperOption` object that wishes to participate within a lazy
+        load operation therefore needs to tell the baked loader that it either
+        needs to forego this caching, or that it needs to include the state of
+        the :class:`.MapperOption` itself as part of its cache key, otherwise
+        SQL or other query state that has been affected by the
+        :class:`.MapperOption` may be cached in place of a query that does not
+        include these modifications, or the option may not be invoked at all.
+
+        By default, this method returns the value ``False``, which means
+        the :class:`.BakedQuery` generated by the lazy loader will
+        not cache the SQL when this :class:`.MapperOption` is present.
+        This is the safest option and ensures both that the option is
+        invoked every time, and also that the cache isn't filled up with
+        an unlimited number of :class:`.Query` objects for an unlimited
+        number of :class:`.MapperOption` objects.
+
+        .. versionchanged:: 1.2.5 the default return value of
+           :meth:`.MapperOption._generate_cache_key` is False; previously it
+           was ``None`` indicating "safe to cache, don't include as part of
+           the cache key"
+
+        To enable caching of :class:`.Query` objects within lazy loaders, a
+        given :class:`.MapperOption` that returns a cache key must return a key
+        that uniquely identifies the complete state of this option, which will
+        match any other :class:`.MapperOption` that itself retains the
+        identical state.  This includes path options, flags, etc.    It should
+        be a state that is repeatable and part of a limited set of possible
+        options.
+
+        If the :class:`.MapperOption` does not apply to the given path and
+        would not affect query results on such a path, it should return None,
+        indicating the :class:`.Query` is safe to cache for this given
+        loader path and that this :class:`.MapperOption` need not be
+        part of the cache key.
 
 
         """
-
-        return None
+        return False
 
 
 class LoaderStrategy(object):
diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index 68b76e736..4312747ac 100644
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -699,6 +699,7 @@ class LazyLoader(AbstractRelationshipLoader, util.MemoizedSlots):
             # caching, for example, since "some_alias" is user-defined and
             # is usually a throwaway object.
             effective_path = state.load_path[self.parent_property]
+
             q._add_lazyload_options(
                 state.load_options, effective_path
             )
@@ -744,7 +745,15 @@ class LazyLoader(AbstractRelationshipLoader, util.MemoizedSlots):
             self._invoke_raise_load(state, passive, "raise_on_sql")
 
         q.add_criteria(lambda q: q.filter(lazy_clause))
-        result = q(session).params(**params).all()
+
+        # set parameters in the query such that we don't overwrite
+        # parameters that are already set within it
+        def set_default_params(q):
+            params.update(q._params)
+            q._params = params
+            return q
+
+        result = q(session).with_post_criteria(set_default_params).all()
         if self.uselist:
             return result
         else: