diff options
| -rw-r--r-- | doc/build/changelog/unreleased_14/5845.rst | 19 | ||||
| -rw-r--r-- | lib/sqlalchemy/engine/base.py | 3 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/session.py | 12 | ||||
| -rw-r--r-- | lib/sqlalchemy/testing/fixtures.py | 2 | ||||
| -rw-r--r-- | test/dialect/test_sqlite.py | 4 | ||||
| -rw-r--r-- | test/engine/test_execute.py | 50 | ||||
| -rw-r--r-- | test/orm/test_events.py | 16 |
7 files changed, 100 insertions, 6 deletions
diff --git a/doc/build/changelog/unreleased_14/5845.rst b/doc/build/changelog/unreleased_14/5845.rst new file mode 100644 index 000000000..4d9ad0ca3 --- /dev/null +++ b/doc/build/changelog/unreleased_14/5845.rst @@ -0,0 +1,19 @@ +.. change:: + :tags: bug, engine, sqlite + :tickets: 5845 + + Fixed bug in the 2.0 "future" version of :class:`.Engine` where emitting + SQL during the :meth:`.EngineEvents.begin` event hook would cause a + re-entrant (recursive) condition due to autobegin, affecting among other + things the recipe documented for SQLite to allow for savepoints and + serializable isolation support. + + +.. change:: + :tags: bug, orm, regression + :tickets: 5845 + + Fixed issue in new :class:`_orm.Session` similar to that of the + :class:`_engine.Connection` where the new "autobegin" logic could be + tripped into a re-entrant (recursive) state if SQL were executed within the + :meth:`.SessionEvents.after_transaction_create` event hook.
\ No newline at end of file diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py index 72d66b7c8..31bf885db 100644 --- a/lib/sqlalchemy/engine/base.py +++ b/lib/sqlalchemy/engine/base.py @@ -813,10 +813,11 @@ class Connection(Connectable): if self._echo: self.engine.logger.info("BEGIN (implicit)") + self.__in_begin = True + if self._has_events or self.engine._has_events: self.dispatch.begin(self) - self.__in_begin = True try: self.engine.dialect.do_begin(self.connection) except BaseException as e: diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index e8312f393..8a7a64f78 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -531,6 +531,10 @@ class SessionTransaction(object): self._take_snapshot(autobegin=autobegin) + # make sure transaction is assigned before we call the + # dispatch + self.session._transaction = self + self.session.dispatch.after_transaction_create(self.session, self) @property @@ -1242,7 +1246,8 @@ class Session(_SessionClassMethods): def _autobegin(self): if not self.autocommit and self._transaction is None: - self._transaction = SessionTransaction(self, autobegin=True) + trans = SessionTransaction(self, autobegin=True) + assert self._transaction is trans return True return False @@ -1299,7 +1304,7 @@ class Session(_SessionClassMethods): if self._transaction is not None: if subtransactions or _subtrans or nested: trans = self._transaction._begin(nested=nested) - self._transaction = trans + assert self._transaction is trans if nested: self._nested_transaction = trans else: @@ -1307,7 +1312,8 @@ class Session(_SessionClassMethods): "A transaction is already begun on this Session." ) else: - self._transaction = SessionTransaction(self, nested=nested) + trans = SessionTransaction(self, nested=nested) + assert self._transaction is trans return self._transaction # needed for __enter__/__exit__ hook def begin_nested(self): diff --git a/lib/sqlalchemy/testing/fixtures.py b/lib/sqlalchemy/testing/fixtures.py index dcdeee5c9..9141d9b11 100644 --- a/lib/sqlalchemy/testing/fixtures.py +++ b/lib/sqlalchemy/testing/fixtures.py @@ -93,7 +93,7 @@ class TestBase(object): from . import engines def gen_testing_engine( - url=None, options=None, future=False, asyncio=False + url=None, options=None, future=None, asyncio=False ): if options is None: options = {} diff --git a/test/dialect/test_sqlite.py b/test/dialect/test_sqlite.py index 587148525..0500a20bd 100644 --- a/test/dialect/test_sqlite.py +++ b/test/dialect/test_sqlite.py @@ -2430,6 +2430,10 @@ class SavepointTest(fixtures.TablesTest): connection.close() +class FutureSavepointTest(fixtures.FutureEngineMixin, SavepointTest): + pass + + class TypeReflectionTest(fixtures.TestBase): __only_on__ = "sqlite" diff --git a/test/engine/test_execute.py b/test/engine/test_execute.py index b9eb4f815..7b997a017 100644 --- a/test/engine/test_execute.py +++ b/test/engine/test_execute.py @@ -1417,6 +1417,27 @@ class EngineEventsTest(fixtures.TestBase): eq_(canary.be2.call_count, 1) eq_(canary.be3.call_count, 2) + def test_emit_sql_in_autobegin(self, testing_engine): + e1 = testing_engine(config.db_url) + + canary = Mock() + + @event.listens_for(e1, "begin") + def begin(connection): + result = connection.execute(select(1)).scalar() + canary.got_result(result) + + with e1.connect() as conn: + assert not conn._is_future + + with conn.begin(): + conn.execute(select(1)).scalar() + assert conn.in_transaction() + + assert not conn.in_transaction() + + eq_(canary.mock_calls, [call.got_result(1)]) + def test_per_connection_plus_engine(self, testing_engine): canary = Mock() e1 = testing_engine(config.db_url) @@ -2309,7 +2330,34 @@ class EngineEventsTest(fixtures.TestBase): class FutureEngineEventsTest(fixtures.FutureEngineMixin, EngineEventsTest): - pass + def test_future_fixture(self, testing_engine): + e1 = testing_engine() + + assert e1._is_future + with e1.connect() as conn: + assert conn._is_future + + def test_emit_sql_in_autobegin(self, testing_engine): + e1 = testing_engine(config.db_url) + + canary = Mock() + + @event.listens_for(e1, "begin") + def begin(connection): + result = connection.execute(select(1)).scalar() + canary.got_result(result) + + with e1.connect() as conn: + assert conn._is_future + conn.execute(select(1)).scalar() + + assert conn.in_transaction() + + conn.commit() + + assert not conn.in_transaction() + + eq_(canary.mock_calls, [call.got_result(1)]) class HandleErrorTest(fixtures.TestBase): diff --git a/test/orm/test_events.py b/test/orm/test_events.py index e85c23d6f..d9caa221d 100644 --- a/test/orm/test_events.py +++ b/test/orm/test_events.py @@ -2173,6 +2173,22 @@ class SessionEventsTest(_RemoveListeners, _fixtures.FixtureTest): sess.rollback() eq_(assertions, [True, True]) + @testing.combinations((True,), (False,)) + def test_autobegin_no_reentrant(self, future): + s1 = fixture_session(future=future) + + canary = Mock() + + @event.listens_for(s1, "after_transaction_create") + def after_transaction_create(session, transaction): + result = session.execute(select(1)).scalar() + canary.got_result(result) + + with s1.begin(): + pass + + eq_(canary.mock_calls, [call.got_result(1)]) + def test_flush_noautocommit_hook(self): User, users = self.classes.User, self.tables.users |