From 91bea2649c933a05ff4735ae80a39d22928ce14c Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Thu, 14 May 2020 10:51:29 -0400 Subject: [PATCH] Actively unset reset agent in discard transaction The assumptions in _discard_transaction from 916e1fea25afcd07fa1d1d2f72043b372cd02223 were too narrow, assuming that if the given transaction were not our "current" one, that this would not be the reset agent. however as the legacy behvaior is that even a "nested" transaction gets set as "self._transaction", this did not accommodate for the nested transaction being thrown away. We will attempt to refine all of this logic in #5327 for 1.4 /master assuming this is feasible for the full suite of current use cases. Fixes: #5326 Change-Id: I6787e82c9e50c23317f87d0d094122c6a6f066da (cherry picked from commit 79de84b25e87bbb7fa94f0dd513b4abc76e05a7e) --- doc/build/changelog/unreleased_13/5326.rst | 9 +++++++++ lib/sqlalchemy/engine/base.py | 12 +++--------- test/engine/test_transaction.py | 16 +++++++++++++++- 3 files changed, 27 insertions(+), 10 deletions(-) create mode 100644 doc/build/changelog/unreleased_13/5326.rst diff --git a/doc/build/changelog/unreleased_13/5326.rst b/doc/build/changelog/unreleased_13/5326.rst new file mode 100644 index 0000000000..801ff4a423 --- /dev/null +++ b/doc/build/changelog/unreleased_13/5326.rst @@ -0,0 +1,9 @@ +.. change:: + :tags: bug, engine + :tickets: 5326 + + Further refinements to the fixes to the "reset" agent fixed in + :ticket:`5326`, which now emits a warning when it is not being correctly + invoked and corrects for the behavior. Additional scenarios have been + identified and fixed where this warning was being emitted. + diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py index 3b4b4c2459..4cf14b58d3 100644 --- a/lib/sqlalchemy/engine/base.py +++ b/lib/sqlalchemy/engine/base.py @@ -799,18 +799,12 @@ class Connection(Connectable): if trans is self.__transaction: if trans._parent is trans: self.__transaction = None - if self._still_open_and_connection_is_valid: - assert self.__connection._reset_agent is None else: self.__transaction = trans._parent - # not doing this assertion for now, however this is how - # it would look: - # if self._still_open_and_connection_is_valid: - # trans = self._transaction - # while not trans._is_root: - # trans = trans._parent - # assert self.__connection._reset_agent is trans + if self._still_open_and_connection_is_valid: + if self.__connection._reset_agent is trans: + self.__connection._reset_agent = None def _rollback_to_savepoint_impl(self, name, context): assert not self.__branch_from diff --git a/test/engine/test_transaction.py b/test/engine/test_transaction.py index 6bab13109b..970435e030 100644 --- a/test/engine/test_transaction.py +++ b/test/engine/test_transaction.py @@ -620,7 +620,7 @@ class ResetAgentTest(fixtures.TestBase): conn.close() @testing.requires.savepoints - def test_begin_nested_trans_close(self): + def test_begin_nested_trans_close_one(self): with testing.db.connect() as connection: t1 = connection.begin() assert connection.connection._reset_agent is t1 @@ -634,6 +634,20 @@ class ResetAgentTest(fixtures.TestBase): assert connection.connection._reset_agent is None assert not t1.is_active + @testing.requires.savepoints + def test_begin_nested_trans_close_two(self): + with testing.db.connect() as connection: + t1 = connection.begin() + assert connection.connection._reset_agent is t1 + t2 = connection.begin_nested() + assert connection.connection._reset_agent is t1 + assert connection._Connection__transaction is t2 + + assert connection.connection._reset_agent is t1 + t1.close() + assert connection.connection._reset_agent is None + assert not t1.is_active + @testing.requires.savepoints def test_begin_nested_trans_rollback(self): with testing.db.connect() as connection: -- 2.39.5