From: Mike Bayer Date: Sun, 24 May 2026 14:05:29 +0000 (-0400) Subject: dont produce side effects for do_orm_execute X-Git-Tag: rel_2_0_50~1 X-Git-Url: http://git.ipfire.org/gitweb/index.cgi?a=commitdiff_plain;h=34f92ab90c5af9dd08727883df89632a083c76a0;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git dont produce side effects for do_orm_execute Fixed issue where the presence of a do_orm_execute event hook would cause internal execution options such as yield_per and loader-specific state from the first orm_pre_session_exec pass to leak into the second pass, leading to errors when using relationship loaders such as selectinload and immediateload. The execution options passed to the second compilation pass are now based on the original options plus only the explicit updates made via ORMExecuteState.update_execution_options() within the event hook. Fixes: #13301 Change-Id: Ide64d7202102930b68a2ab903054d538cd2f99dd --- diff --git a/doc/build/changelog/unreleased_20/13301.rst b/doc/build/changelog/unreleased_20/13301.rst new file mode 100644 index 0000000000..d4fd2d4d04 --- /dev/null +++ b/doc/build/changelog/unreleased_20/13301.rst @@ -0,0 +1,13 @@ +.. change:: + :tags: bug, orm + :tickets: 13301 + + Fixed issue where the presence of a :meth:`.SessionEvents.do_orm_execute` + event hook would cause internal execution options such as ``yield_per`` and + loader-specific state from the first ``orm_pre_session_exec`` pass to leak + into the second pass, leading to errors when using relationship loaders + such as :func:`.selectinload` and :func:`.immediateload`. The execution + options passed to the second compilation pass are now based on the original + options plus only the explicit updates made via + :meth:`.ORMExecuteState.update_execution_options` within the event hook. + diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index 623ab00a02..a10d943723 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -345,7 +345,7 @@ class ORMExecuteState(util.MemoizedSlots): _compile_state_cls: Optional[Type[ORMCompileState]] _starting_event_idx: int _events_todo: List[Any] - _update_execution_options: Optional[_ExecuteOptions] + _update_execution_options: _ExecuteOptions def __init__( self, @@ -372,6 +372,7 @@ class ORMExecuteState(util.MemoizedSlots): self.bind_arguments = bind_arguments self._compile_state_cls = compile_state_cls self._events_todo = list(events_todo) + self._update_execution_options = util.EMPTY_DICT def _remaining_events(self) -> List[_InstanceLevelDispatch[Session]]: return self._events_todo[self._starting_event_idx + 1 :] @@ -643,6 +644,9 @@ class ORMExecuteState(util.MemoizedSlots): def update_execution_options(self, **opts: Any) -> None: """Update the local execution options with new values.""" self.local_execution_options = self.local_execution_options.union(opts) + self._update_execution_options = self._update_execution_options.union( + opts + ) def _orm_compile_options( self, @@ -2176,6 +2180,17 @@ class Session(_SessionClassMethods, EventTarget): events_todo = list(events_todo) + [_add_event] if events_todo: + # save the original execution options before + # orm_pre_session_exec processes them, so that we can pass + # the unprocessed options (plus any explicit updates from event + # hooks) to the second orm_pre_session_exec call. This + # prevents internal state like _sa_orm_load_options and + # yield_per from the first call leaking into the second call, + # which would otherwise cause issues like yield_per incorrectly + # propagating into post-load (selectinload etc.) queries. + # part of #13301. + original_execution_options = execution_options + if compile_state_cls is not None: # for event handlers, do the orm_pre_session_exec # pass ahead of the event handlers, so that things like @@ -2214,7 +2229,13 @@ class Session(_SessionClassMethods, EventTarget): return fn_result statement = orm_exec_state.statement - execution_options = orm_exec_state.local_execution_options + + # use the original execution options plus only the explicit + # updates from event hooks, not the processed options from + # the first orm_pre_session_exec call + execution_options = original_execution_options.union( + orm_exec_state._update_execution_options + ) if compile_state_cls is not None: # now run orm_pre_session_exec() "for real". if there were diff --git a/test/orm/test_events.py b/test/orm/test_events.py index 8f71e4e92d..e673bed443 100644 --- a/test/orm/test_events.py +++ b/test/orm/test_events.py @@ -24,6 +24,7 @@ from sqlalchemy.orm import configure_mappers from sqlalchemy.orm import declarative_base from sqlalchemy.orm import deferred from sqlalchemy.orm import EXT_SKIP +from sqlalchemy.orm import immediateload from sqlalchemy.orm import instrumentation from sqlalchemy.orm import joinedload from sqlalchemy.orm import lazyload @@ -36,6 +37,7 @@ from sqlalchemy.orm import Session from sqlalchemy.orm import sessionmaker from sqlalchemy.orm import subqueryload from sqlalchemy.orm import UserDefinedOption +from sqlalchemy.orm.context import QueryContext from sqlalchemy.sql.cache_key import NO_CACHE from sqlalchemy.testing import assert_raises from sqlalchemy.testing import assert_raises_message @@ -50,13 +52,16 @@ from sqlalchemy.testing.assertions import expect_raises_message from sqlalchemy.testing.assertsql import CompiledSQL from sqlalchemy.testing.fixtures import fixture_session from sqlalchemy.testing.fixtures import RemoveORMEventsGlobally +from sqlalchemy.testing.fixtures import RemovesEvents from sqlalchemy.testing.schema import Column from sqlalchemy.testing.schema import Table from sqlalchemy.testing.util import gc_collect from test.orm import _fixtures -class ORMExecuteTest(RemoveORMEventsGlobally, _fixtures.FixtureTest): +class ORMExecuteTest( + RemovesEvents, RemoveORMEventsGlobally, _fixtures.FixtureTest +): run_setup_mappers = "once" run_inserts = "once" run_deletes = None @@ -182,6 +187,7 @@ class ORMExecuteTest(RemoveORMEventsGlobally, _fixtures.FixtureTest): assert False if use_query_cache: + testing.db._compiled_cache.clear() s = fixture_session() else: s = fixture_session( @@ -229,6 +235,342 @@ class ORMExecuteTest(RemoveORMEventsGlobally, _fixtures.FixtureTest): ), ) + @testing.combinations( + (lazyload,), + (selectinload,), + (immediateload,), + (joinedload,), + (subqueryload,), + argnames="loader_opt", + ) + @testing.variation("use_event", [True, False]) + def test_execution_option_visibility_and_mutation( + self, loader_opt, use_event + ): + """test the full contract of how options are passed to + do_orm_execute and then onto a relationship loader. + + part of #13301 + + the non-event part of this test has overlap with + test_session.py -> test_eagerloader_exec_option, which is also + looking at how top level options find their way into eager loader + queries. in this test though we try to establish every possible + path for options with all loaders. + + """ + User, Address, Dinagling = self.classes("User", "Address", "Dingaling") + + s = fixture_session() + + m1 = Mock() + + def _before_cursor_execute( + conn, cursor, statement, parameters, context, executemany + ): + opts = { + k: v + for k, v in context.execution_options.items() + if isinstance(k, str) and k.startswith("user_") + } + m1.before_cursor_execute(opts) + + self.event_listen( + testing.db, "before_cursor_execute", _before_cursor_execute + ) + + if use_event: + + def go(context): + opts = { + k: v + for k, v in context.execution_options.items() + if isinstance(k, str) and k.startswith("user_") + } + m1.do_orm_execute(context.is_relationship_load, opts) + + context.update_execution_options( + user_defined_three="z", + user_defined_two_override="z2", + ) + + # local_execution_options receive the updates + eq_( + context.local_execution_options["user_defined_three"], + "z", + ) + eq_( + context.local_execution_options[ + "user_defined_two_override" + ], + "z2", + ) + + # the .execution_options does not show the updates + # (not sure if i like this but this is how it was + # done) + if not context.is_relationship_load: + # only test for the top-level load. the options + # may be present in a relationship load. + assert ( + "user_defined_three" not in context.execution_options + ) + eq_( + context.execution_options["user_defined_two_override"], + "y2", + ) + + self.event_listen(s, "do_orm_execute", go) + + stmt = ( + select(User) + .options( + loader_opt(User.addresses).options( + loader_opt(Address.dingaling) + ), + ) + .execution_options( + user_defined_one="x", + user_defined_one_override="x2", + ) + ) + + for u in ( + s.execute( + stmt, + execution_options={ + "user_defined_two": "y", + "user_defined_two_override": "y2", + "user_defined_one_override": "y2", + }, + ) + .unique() + .scalars() + ): + for a in u.addresses: + a.dingaling + + if use_event: + upfront_expected_calls = [ + call.do_orm_execute( + False, + { + "user_defined_one": "x", + "user_defined_two": "y", + "user_defined_two_override": "y2", + "user_defined_one_override": "y2", + }, + ), + call.before_cursor_execute( + { + "user_defined_one": "x", + "user_defined_two_override": "z2", + "user_defined_one_override": "y2", + "user_defined_two": "y", + "user_defined_three": "z", + }, + ), + ] + relationship_level_calls = [ + call.do_orm_execute( + True, + { + "user_defined_one": "x", + "user_defined_two_override": "z2", + "user_defined_one_override": "y2", + "user_defined_two": "y", + "user_defined_three": "z", + }, + ), + call.before_cursor_execute( + { + "user_defined_one": "x", + "user_defined_two_override": "z2", + "user_defined_one_override": "y2", + "user_defined_two": "y", + "user_defined_three": "z", + }, + ), + ] + joined_and_lazy_calls = [ + call.do_orm_execute( + True, + {}, + ), + call.before_cursor_execute( + { + "user_defined_two_override": "z2", + "user_defined_three": "z", + }, + ), + ] + else: + upfront_expected_calls = [ + call.before_cursor_execute( + { + "user_defined_one": "x", + "user_defined_two_override": "y2", + "user_defined_one_override": "y2", + "user_defined_two": "y", + }, + ), + ] + relationship_level_calls = [ + call.before_cursor_execute( + { + "user_defined_one": "x", + "user_defined_two_override": "y2", + "user_defined_one_override": "y2", + "user_defined_two": "y", + }, + ), + ] + joined_and_lazy_calls = [ + call.before_cursor_execute({}), + ] + + if loader_opt in (selectinload, subqueryload, immediateload): + eq_( + m1.mock_calls, + upfront_expected_calls + + ( + relationship_level_calls + * (2 if loader_opt in (selectinload, subqueryload) else 9) + ), + ) + elif loader_opt in (lazyload, joinedload): + eq_( + m1.mock_calls, + upfront_expected_calls + + ( + joined_and_lazy_calls + * (9 if loader_opt is lazyload else 0) + ), + ) + + @testing.combinations( + (selectinload,), + (immediateload,), + argnames="loader_opt", + ) + @testing.variation("option_type", ["yield_per", "autoflush"]) + @testing.variation( + "event_style", + [ + "no_event", + "noop_event", + "set_in_event_for_relationship", + ], + ) + def test_load_option_propagation_to_post_load( + self, + loader_opt, + option_type, + event_style, + ): + """test that yield_per and autoflush options set on the + top-level query don't leak into load_options for post-load + queries (selectinload, immediateload), and that options + explicitly set via update_execution_options in a + do_orm_execute event hook for relationship loads do propagate + correctly. + + part of #13301 + + """ + + User = self.classes.User + + s = fixture_session() + + expected_lead_options = { + "yield_per": None, + "autoflush": True, + "is_post_load": False, + } + expected_post_load_options = { + "yield_per": None, + "autoflush": True, + "is_post_load": True, + } + operation_should_fail = ( + event_style.set_in_event_for_relationship and option_type.yield_per + ) + + if event_style.noop_event: + + def go(context): + pass + + self.event_listen(s, "do_orm_execute", go) + + elif event_style.set_in_event_for_relationship: + + def go(context): + if context.is_relationship_load: + if option_type.yield_per: + context.update_execution_options(yield_per=7) + expected_post_load_options["yield_per"] = 7 + elif option_type.autoflush: + context.update_execution_options(autoflush=False) + expected_post_load_options["autoflush"] = False + + self.event_listen(s, "do_orm_execute", go) + elif event_style.no_event: + pass + + captured_load_options = [] + + orig_init = QueryContext.__init__ + + def tracking_init(self_qc, *args, **kwargs): + orig_init(self_qc, *args, **kwargs) + captured_load_options.append( + { + "yield_per": self_qc.load_options._yield_per, + "autoflush": (self_qc.load_options._autoflush), + "is_post_load": ( + self_qc.load_options._sa_top_level_orm_context + is not None + ), + } + ) + + with patch.object(QueryContext, "__init__", tracking_init): + + stmt = select(User).options(loader_opt(User.addresses)) + + if not event_style.set_in_event_for_relationship: + if option_type.yield_per: + stmt = stmt.execution_options(yield_per=5) + expected_lead_options["yield_per"] = 5 + elif option_type.autoflush: + stmt = stmt.execution_options(autoflush=False) + expected_lead_options["autoflush"] = False + + if operation_should_fail: + with expect_raises_message( + sa_exc.InvalidRequestError, + "Can't use the ORM yield_per feature in " + "conjunction with unique", + ): + s.scalars(stmt).all() + else: + s.scalars(stmt).all() + + eq_( + captured_load_options, + [expected_lead_options] + + ( + [expected_post_load_options] + * ( + 1 + if loader_opt is selectinload or operation_should_fail + else 4 + ) + ), + ) + def test_override_parameters_scalar(self): """test that session.scalar() maintains the 'scalar-ness' of the result when using re-execute events.