From: Mike Bayer Date: Tue, 17 Aug 2021 19:03:57 +0000 (-0400) Subject: send user defined options from the current query X-Git-Tag: rel_1_4_23~3^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=29485026e9ea6500ea451b1d4a6f66795a02428d;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git send user defined options from the current query Revised the means by which the :attr:`_orm.ORMExecuteState.user_defined_options` accessor receives :class:`_orm.UserDefinedOption` and related option objects from the context, with particular emphasis on the "selectinload" on the loader strategy where this previously was not working; other strategies did not have this problem. The objects that are associated with the current query being executed, and not that of a query being cached, are now propagated unconditionally. This essentially separates them out from the "loader strategy" options which are explicitly associated with the compiled state of a query and need to be used in relation to the cached query. The effect of this fix is that a user-defined option, such as those used by the dogpile.caching example as well as for other recipes such as defining a "shard id" for the horizontal sharing extension, will be correctly propagated to eager and lazy loaders regardless of whether a cached query was ultimately invoked. Fixes: #6887 Change-Id: Ieaae5b01c85de26ea732ebd625e6e5823a470492 --- diff --git a/doc/build/changelog/unreleased_14/6887.rst b/doc/build/changelog/unreleased_14/6887.rst new file mode 100644 index 0000000000..8992dccba0 --- /dev/null +++ b/doc/build/changelog/unreleased_14/6887.rst @@ -0,0 +1,21 @@ +.. change:: + :tags: bug, orm + :tickets: 6887 + + Revised the means by which the + :attr:`_orm.ORMExecuteState.user_defined_options` accessor receives + :class:`_orm.UserDefinedOption` and related option objects from the + context, with particular emphasis on the "selectinload" on the loader + strategy where this previously was not working; other strategies did not + have this problem. The objects that are associated with the current query + being executed, and not that of a query being cached, are now propagated + unconditionally. This essentially separates them out from the "loader + strategy" options which are explicitly associated with the compiled state + of a query and need to be used in relation to the cached query. + + The effect of this fix is that a user-defined option, such as those used + by the dogpile.caching example as well as for other recipes such as + defining a "shard id" for the horizontal sharing extension, will be + correctly propagated to eager and lazy loaders regardless of whether + a cached query was ultimately invoked. + diff --git a/lib/sqlalchemy/orm/__init__.py b/lib/sqlalchemy/orm/__init__.py index a247b597b2..8e96478408 100644 --- a/lib/sqlalchemy/orm/__init__.py +++ b/lib/sqlalchemy/orm/__init__.py @@ -44,6 +44,7 @@ from .interfaces import MapperProperty from .interfaces import NOT_EXTENSION from .interfaces import ONETOMANY from .interfaces import PropComparator +from .interfaces import UserDefinedOption from .loading import merge_frozen_result from .loading import merge_result from .mapper import class_mapper diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index bf60c803d2..069e5e6674 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -1512,21 +1512,14 @@ class SubqueryLoader(PostLoader): loadopt, ): - if orig_query is context.query: - options = new_options = orig_query._with_options - else: - # There's currently no test that exercises the necessity of - # this step for subqueryload. Added in #6881, it is necessary for - # selectinload, but its necessity for subqueryload is still - # theoretical. - options = orig_query._with_options - - new_options = [ - orig_opt._adjust_for_extra_criteria(context) - if orig_opt._is_strategy_option - else orig_opt - for orig_opt in options - ] + # note that because the subqueryload object + # does not re-use the cached query, instead always making + # use of the current invoked query, while we have two queries + # here (orig and context.query), they are both non-cached + # queries and we can transfer the options as is without + # adjusting for new criteria. Some work on #6881 / #6889 + # brought this into question. + new_options = orig_query._with_options if loadopt and loadopt._extra_criteria: @@ -2933,9 +2926,12 @@ class SelectInLoader(PostLoader, util.MemoizedSlots): if orig_query is context.query: options = new_options = orig_query._with_options + user_defined_options = [] else: options = orig_query._with_options + # propagate compile state options from the original query, + # updating their "extra_criteria" as necessary. # note this will create a different cache key than # "orig" options if extra_criteria is present, because the copy # of extra_criteria will have different boundparam than that of @@ -2946,6 +2942,14 @@ class SelectInLoader(PostLoader, util.MemoizedSlots): 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 ] if loadopt and loadopt._extra_criteria: @@ -2959,6 +2963,8 @@ class SelectInLoader(PostLoader, util.MemoizedSlots): q = q.options(*new_options)._update_compile_options( {"_current_path": effective_path} ) + if user_defined_options: + q = q.options(*user_defined_options) if context.populate_existing: q = q.execution_options(populate_existing=True) diff --git a/lib/sqlalchemy/testing/fixtures.py b/lib/sqlalchemy/testing/fixtures.py index 30ada71cd3..e6af0c546c 100644 --- a/lib/sqlalchemy/testing/fixtures.py +++ b/lib/sqlalchemy/testing/fixtures.py @@ -548,7 +548,10 @@ _fixture_sessions = set() def fixture_session(**kw): kw.setdefault("autoflush", True) kw.setdefault("expire_on_commit", True) - sess = sa.orm.Session(config.db, **kw) + + bind = kw.pop("bind", config.db) + + sess = sa.orm.Session(bind, **kw) _fixture_sessions.add(sess) return sess diff --git a/test/orm/test_events.py b/test/orm/test_events.py index cd8adf8e35..6831c8b108 100644 --- a/test/orm/test_events.py +++ b/test/orm/test_events.py @@ -17,6 +17,8 @@ from sqlalchemy.orm import deferred from sqlalchemy.orm import events from sqlalchemy.orm import EXT_SKIP from sqlalchemy.orm import instrumentation +from sqlalchemy.orm import joinedload +from sqlalchemy.orm import lazyload from sqlalchemy.orm import Mapper from sqlalchemy.orm import mapper from sqlalchemy.orm import mapperlib @@ -26,6 +28,8 @@ from sqlalchemy.orm import selectinload from sqlalchemy.orm import Session from sqlalchemy.orm import sessionmaker from sqlalchemy.orm import subqueryload +from sqlalchemy.orm import UserDefinedOption +from sqlalchemy.sql.traversals import NO_CACHE from sqlalchemy.testing import assert_raises from sqlalchemy.testing import assert_raises_message from sqlalchemy.testing import AssertsCompiledSQL @@ -139,6 +143,97 @@ class ORMExecuteTest(_RemoveListeners, _fixtures.FixtureTest): ), ) + @testing.combinations( + ("fixed",), ("payload",), ("dont_cache"), argnames="key_type" + ) + @testing.combinations( + (lazyload, 10), + (selectinload, 3), + (joinedload, 1), + (subqueryload, 3), + argnames="loader_opt, num_opts", + ) + @testing.combinations((True,), (False,), argnames="use_query_cache") + def test_user_option_propagation( + self, key_type, loader_opt, num_opts, use_query_cache + ): + """test #6887. + + The one case here which would break before the bug was fixed is: + + use_query_cache=True, loader_opt=selectinload, key_type="fixed" + + Meaning with a fixed cache key and query caching in place, the user + defined loader option would be cached under the same name each time, + leading to use of the stale option when using selectinload, as this + strategy transfers query options from the cached ORM select into the + one that it generates. no other strategy currently does this. + + """ + User, Address, Dinagling = self.classes("User", "Address", "Dingaling") + + class SetShardOption(UserDefinedOption): + propagate_to_loaders = True + + def _gen_cache_key(self, anon_map, bindparams): + if key_type == "fixed": + return (None,) + elif key_type == "payload": + return (self.payload,) + elif key_type == "dont_cache": + anon_map[NO_CACHE] = True + return None + else: + assert False + + if use_query_cache: + s = fixture_session() + else: + s = fixture_session( + bind=testing.db.execution_options(compiled_cache=None) + ) + + m1 = Mock() + + @event.listens_for(s, "do_orm_execute") + def go(context): + for elem in context.user_defined_options: + if isinstance(elem, SetShardOption): + m1.update_execution_options(_sa_shard_id=elem.payload) + + stmt = select(User).options( + loader_opt(User.addresses).options(loader_opt(Address.dingaling)), + SetShardOption("some_shard"), + ) + for u in s.execute(stmt).unique().scalars(): + for a in u.addresses: + a.dingaling + + s.close() + + stmt = select(User).options( + loader_opt(User.addresses).options(loader_opt(Address.dingaling)), + SetShardOption("some_other_shard"), + ) + for u in s.execute(stmt).unique().scalars(): + for a in u.addresses: + a.dingaling + eq_( + m1.mock_calls, + ( + [call.update_execution_options(_sa_shard_id="some_shard")] + * num_opts + ) + + ( + [ + call.update_execution_options( + _sa_shard_id="some_other_shard" + ) + ] + * num_opts + ), + ) + def test_chained_events_one(self): sess = Session(testing.db, future=True)