From 4a31c30fa5ebd6af0e72937b21b2e5ee79e12631 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 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 --- doc/build/changelog/unreleased_12/4128.rst | 16 +++++ .../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, 171 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..908bd115e1 --- /dev/null +++ b/doc/build/changelog/unreleased_12/4128.rst @@ -0,0 +1,16 @@ +.. 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. 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. 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 9d57044717..86eee831b0 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..915768b017 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 68b76e736e..4312747ac5 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: 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