From 21d8a82c7b0dc450a14957fa34b81250119ad6a7 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Sun, 25 Feb 2018 19:54:37 -0500 Subject: [PATCH] Merge existing query params in baked lazy load Corrected backport, was supposed to be in 1.2.5 however never got backported. 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. As part of this change, 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. In particular, this repairs one regression which occured when using the dogpile.cache "advanced" example, which was not returning cached results and instead emitting SQL due to an incompatibility with the baked query loader; with the change, the ``RelationshipCache`` option included for many releases in the dogpile example will disable the "baked" query altogether. Note that the dogpile example is also modernized to avoid both of these issues as part of issue :ticket:`4258`. This is a cherry-pick / squash from: 4a31c30fa5ebd6af0e72937b21b2e5ee79e12631 2e46f73f35b9036287f5f567c09a8cb786fe5fd3 b9f428a589a1718efa20e5555be45ae3f767e89e Change-Id: I899808734458e25a023142c2c5bb37cbed869479 Fixes: #4128 --- doc/build/changelog/unreleased_12/4128.rst | 26 ++++++++ .../post_criteria_subqueryload.rst | 8 +++ lib/sqlalchemy/ext/baked.py | 11 +++- lib/sqlalchemy/orm/interfaces.py | 62 ++++++++++++++----- lib/sqlalchemy/orm/strategies.py | 11 +++- test/ext/test_baked.py | 29 +++++++++ test/orm/test_lazy_relations.py | 56 ++++++++++++++++- 7 files changed, 181 insertions(+), 22 deletions(-) create mode 100644 doc/build/changelog/unreleased_12/4128.rst create mode 100644 doc/build/changelog/unreleased_12/post_criteria_subqueryload.rst diff --git a/doc/build/changelog/unreleased_12/4128.rst b/doc/build/changelog/unreleased_12/4128.rst new file mode 100644 index 0000000000..f9cfe1373e --- /dev/null +++ b/doc/build/changelog/unreleased_12/4128.rst @@ -0,0 +1,26 @@ +.. change:: + :tags: bug, orm + :tickets: 4128 + + 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. + + As part of this change, 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. + In particular, this repairs one regression which occured when + using the dogpile.cache "advanced" example, which was not + returning cached results and instead emitting SQL due to an + incompatibility with the baked query loader; with the change, + the ``RelationshipCache`` option included for many releases + in the dogpile example will disable the "baked" query altogether. + Note that the dogpile example is also modernized to avoid both + of these issues as part of issue :ticket:`4258`. diff --git a/doc/build/changelog/unreleased_12/post_criteria_subqueryload.rst b/doc/build/changelog/unreleased_12/post_criteria_subqueryload.rst new file mode 100644 index 0000000000..2b8942ffe6 --- /dev/null +++ b/doc/build/changelog/unreleased_12/post_criteria_subqueryload.rst @@ -0,0 +1,8 @@ +.. change:: + :tags: bug, orm + + 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. diff --git a/lib/sqlalchemy/ext/baked.py b/lib/sqlalchemy/ext/baked.py index 13ad4cf7c4..f4d71f4103 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 1a869b28ca..7e34cd0035 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.8 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 9bce5593d3..00c83cea49 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -708,6 +708,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 ) @@ -753,7 +754,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: diff --git a/test/ext/test_baked.py b/test/ext/test_baked.py index 47da6d0edf..6344819e16 100644 --- a/test/ext/test_baked.py +++ b/test/ext/test_baked.py @@ -806,6 +806,35 @@ class ResultTest(BakedTest): sess.close() + def test_subqueryload_post_context(self): + User = self.classes.User + Address = self.classes.Address + + assert_result = [ + User(id=7, + addresses=[Address(id=1, email_address='jack@bean.com')]) + ] + + self.bakery = baked.bakery(size=3) + + bq = self.bakery(lambda s: s.query(User)) + + bq += lambda q: q.options(subqueryload(User.addresses)) + bq += lambda q: q.order_by(User.id) + bq += lambda q: q.filter(User.name == bindparam('name')) + sess = Session() + + def set_params(q): + return q.params(name='jack') + + # test that the changes we make using with_post_criteria() + # are also applied to the subqueryload query. + def go(): + result = bq(sess).with_post_criteria(set_params).all() + eq_(assert_result, result) + + self.assert_sql_count(testing.db, go, 2) + class LazyLoaderTest(testing.AssertsCompiledSQL, BakedTest): run_setup_mappers = 'each' diff --git a/test/orm/test_lazy_relations.py b/test/orm/test_lazy_relations.py index 7bb3b4d02e..b79aeb14b2 100644 --- a/test/orm/test_lazy_relations.py +++ b/test/orm/test_lazy_relations.py @@ -4,6 +4,7 @@ from sqlalchemy.testing import assert_raises import datetime from sqlalchemy.orm import attributes, exc as orm_exc, configure_mappers import sqlalchemy as sa +from sqlalchemy.orm.interfaces import MapperOption from sqlalchemy import testing, and_, bindparam from sqlalchemy import Integer, String, ForeignKey, SmallInteger, Boolean from sqlalchemy import ForeignKeyConstraint @@ -16,7 +17,7 @@ from sqlalchemy.testing import eq_, is_true, is_false from sqlalchemy.testing import fixtures from test.orm import _fixtures from sqlalchemy.testing.assertsql import CompiledSQL - +from sqlalchemy.testing import mock class LazyTest(_fixtures.FixtureTest): run_inserts = 'once' @@ -289,6 +290,59 @@ class LazyTest(_fixtures.FixtureTest): fred = s.query(User).filter_by(name='fred').one() eq_(fred.addresses, []) # fred is missing + def test_custom_bind(self): + Address, addresses, users, User = ( + self.classes.Address, + self.tables.addresses, + self.tables.users, + self.classes.User) + + mapper(User, users, properties=dict( + addresses=relationship( + mapper(Address, addresses), + lazy='select', + primaryjoin=and_( + users.c.id == addresses.c.user_id, + users.c.name == bindparam("name") + ) + ) + )) + + canary = mock.Mock() + + class MyOption(MapperOption): + propagate_to_loaders = True + + def __init__(self, crit): + self.crit = crit + + def process_query_conditionally(self, query): + """process query during a lazyload""" + canary() + query._params = query._params.union(dict(name=self.crit)) + + s = Session() + ed = s.query(User).options(MyOption("ed")).filter_by(name='ed').one() + eq_(ed.addresses, [ + Address(id=2, user_id=8), + Address(id=3, user_id=8), + Address(id=4, user_id=8) + ]) + eq_(canary.mock_calls, [mock.call()]) + + fred = s.query(User).\ + options(MyOption("ed")).filter_by(name='fred').one() + eq_(fred.addresses, []) # fred is missing + eq_(canary.mock_calls, [mock.call(), mock.call()]) + + # the lazy query was not cached; the option is re-applied to the + # Fred object due to populate_existing() + fred = s.query(User).populate_existing().\ + options(MyOption("fred")).filter_by(name='fred').one() + eq_(fred.addresses, [Address(id=5, user_id=9)]) # fred is there + + eq_(canary.mock_calls, [mock.call(), mock.call(), mock.call()]) + def test_one_to_many_scalar(self): Address, addresses, users, User = ( self.classes.Address, -- 2.47.2