diff options
| -rw-r--r-- | doc/build/changelog/migration_14.rst | 29 | ||||
| -rw-r--r-- | doc/build/changelog/unreleased_14/4712.rst | 20 | ||||
| -rw-r--r-- | doc/build/errors.rst | 42 | ||||
| -rw-r--r-- | lib/sqlalchemy/engine/base.py | 67 | ||||
| -rw-r--r-- | test/dialect/test_sqlite.py | 4 | ||||
| -rw-r--r-- | test/engine/test_transaction.py | 30 |
6 files changed, 174 insertions, 18 deletions
diff --git a/doc/build/changelog/migration_14.rst b/doc/build/changelog/migration_14.rst index 66fe1c094..3127ca977 100644 --- a/doc/build/changelog/migration_14.rst +++ b/doc/build/changelog/migration_14.rst @@ -212,3 +212,32 @@ configured to raise an exception using the Python warnings filter. :ticket:`4662` + + +Behavior Changes - Core +======================== + +.. _change_4712: + +Connection-level transactions can now be inactive based on subtransaction +------------------------------------------------------------------------- + +A :class:`.Connection` now includes the behavior where a :class:`.Transaction` +can be made inactive due to a rollback on an inner transaction, however the +:class:`.Transaction` will not clear until it is itself rolled back. + +This is essentially a new error condition which will disallow statement +executions to proceed on a :class:`.Connection` if an inner "sub" transaction +has been rolled back. The behavior works very similarly to that of the +ORM :class:`.Session`, where if an outer transaction has been begun, it needs +to be rolled back to clear the invalid transaction; this behavior is described +in :ref:`faq_session_rollback` + +While the :class:`.Connection` has had a less strict behavioral pattern than +the :class:`.Session`, this change was made as it helps to identify when +a subtransaction has rolled back the DBAPI transaction, however the external +code isn't aware of this and attempts to continue proceeding, which in fact +runs operations on a new transaction. The "test harness" pattern described +at :ref:`session_external_transaction` is the common place for this to occur. + +The new behavior is described in the errors page at :ref:`error_8s2a`. diff --git a/doc/build/changelog/unreleased_14/4712.rst b/doc/build/changelog/unreleased_14/4712.rst new file mode 100644 index 000000000..cc8764c72 --- /dev/null +++ b/doc/build/changelog/unreleased_14/4712.rst @@ -0,0 +1,20 @@ +.. change:: + :tags: bug, engine + :tickets: 4712 + + The :class:`.Connection` object will now not clear a rolled-back + transaction until the outermost transaction is explicitly rolled back. + This is essentially the same behavior that the ORM :class:`.Session` has + had for a long time, where an explicit call to ``.rollback()`` on all + enclosing transactions is required for the transaction to logically clear, + even though the DBAPI-level transaction has already been rolled back. + The new behavior helps with situations such as the "ORM rollback test suite" + pattern where the test suite rolls the transaction back within the ORM + scope, but the test harness which seeks to control the scope of the + transaction externally does not expect a new transaction to start + implicitly. + + .. seealso:: + + :ref:`change_4712` + diff --git a/doc/build/errors.rst b/doc/build/errors.rst index e7f95fa24..fa2da99d0 100644 --- a/doc/build/errors.rst +++ b/doc/build/errors.rst @@ -188,6 +188,48 @@ sooner. :ref:`connections_toplevel` +.. _error_8s2a: + +This connection is on an inactive transaction. Please rollback() fully before proceeding +------------------------------------------------------------------------------------------ + +This error condition was added to SQLAlchemy as of version 1.4. The error +refers to the state where a :class:`.Connection` is placed into a transaction +using a method like :meth:`.Connection.begin`, and then a further "sub" transaction +is created within that scope; the "sub" transaction is then rolled back using +:meth:`.Transaction.rollback`, however the outer transaction is not rolled back. + +The pattern looks like:: + + engine = create_engine(...) + + connection = engine.connect() + transaction = connection.begin() + + transaction2 = connection.begin() + transaction2.rollback() + + connection.execute("select 1") # we are rolled back; will now raise + + transaction.rollback() + + +Above, ``transaction2`` is a "sub" transaction, which indicates a logical +nesting of transactions within an outer one. SQLAlchemy makes great use of +this pattern more commonly in the ORM :class:`.Session`, where the FAQ entry +:ref:`faq_session_rollback` describes the rationale within the ORM. + +The "subtransaction" pattern in Core comes into play often when using the ORM +pattern described at :ref:`session_external_transaction`. As this pattern +involves a behavior called "connection branching", where a :class:`.Connection` +serves a "branched" :class:`.Connection` object to the :class:`.Session` via +its :meth:`.Connection.connect` method, the same transaction behavior comes +into play; if the :class:`.Session` rolls back the transaction, and savepoints +have not been used to prevent a rollback of the entire transaction, the +outermost transaction started on the :class:`.Connection` is now in an inactive +state. + + .. _error_dbapi: DBAPI Errors diff --git a/lib/sqlalchemy/engine/base.py b/lib/sqlalchemy/engine/base.py index 6467e91b9..7aee1a73b 100644 --- a/lib/sqlalchemy/engine/base.py +++ b/lib/sqlalchemy/engine/base.py @@ -708,7 +708,10 @@ class Connection(Connectable): def in_transaction(self): """Return True if a transaction is in progress.""" - return self._root.__transaction is not None + return ( + self._root.__transaction is not None + and self._root.__transaction.is_active + ) def _begin_impl(self, transaction): assert not self.__branch_from @@ -726,7 +729,7 @@ class Connection(Connectable): except BaseException as e: self._handle_dbapi_exception(e, None, None, None, None) - def _rollback_impl(self): + def _rollback_impl(self, deactivate_only=False): assert not self.__branch_from if self._has_events or self.engine._has_events: @@ -745,9 +748,6 @@ class Connection(Connectable): and self.connection._reset_agent is self.__transaction ): self.connection._reset_agent = None - self.__transaction = None - else: - self.__transaction = None def _commit_impl(self, autocommit=False): assert not self.__branch_from @@ -782,7 +782,18 @@ class Connection(Connectable): self.engine.dialect.do_savepoint(self, name) return name - def _rollback_to_savepoint_impl(self, name, context): + def _discard_transaction(self, trans): + if trans is self.__transaction: + if trans._is_root: + assert trans._parent is trans + self.__transaction = None + else: + assert trans._parent is not trans + self.__transaction = trans._parent + + def _rollback_to_savepoint_impl( + self, name, context, deactivate_only=False + ): assert not self.__branch_from if self._has_events or self.engine._has_events: @@ -790,7 +801,6 @@ class Connection(Connectable): if self._still_open_and_connection_is_valid: self.engine.dialect.do_rollback_to_savepoint(self, name) - self.__transaction = context def _release_savepoint_impl(self, name, context): assert not self.__branch_from @@ -1182,6 +1192,17 @@ class Connection(Connectable): e, util.text_type(statement), parameters, None, None ) + if self._root.__transaction and not self._root.__transaction.is_active: + raise exc.InvalidRequestError( + "This connection is on an inactive %stransaction. " + "Please rollback() fully before proceeding." + % ( + "savepoint " + if isinstance(self.__transaction, NestedTransaction) + else "" + ), + code="8s2a", + ) if context.compiled: context.pre_exec() @@ -1671,11 +1692,16 @@ class Transaction(object): single: thread safety; Transaction """ + _is_root = False + def __init__(self, connection, parent): self.connection = connection self._actual_parent = parent self.is_active = True + def _deactivate(self): + self.is_active = False + @property def _parent(self): return self._actual_parent or self @@ -1700,13 +1726,14 @@ class Transaction(object): """Roll back this :class:`.Transaction`. """ - if not self._parent.is_active: - return - self._do_rollback() - self.is_active = False + + if self._parent.is_active: + self._do_rollback() + self.is_active = False + self.connection._discard_transaction(self) def _do_rollback(self): - self._parent.rollback() + self._parent._deactivate() def commit(self): """Commit this :class:`.Transaction`.""" @@ -1734,13 +1761,19 @@ class Transaction(object): class RootTransaction(Transaction): + _is_root = True + def __init__(self, connection): super(RootTransaction, self).__init__(connection, None) self.connection._begin_impl(self) - def _do_rollback(self): + def _deactivate(self): + self._do_rollback(deactivate_only=True) + self.is_active = False + + def _do_rollback(self, deactivate_only=False): if self.is_active: - self.connection._rollback_impl() + self.connection._rollback_impl(deactivate_only=deactivate_only) def _do_commit(self): if self.is_active: @@ -1761,7 +1794,11 @@ class NestedTransaction(Transaction): super(NestedTransaction, self).__init__(connection, parent) self._savepoint = self.connection._savepoint_impl() - def _do_rollback(self): + def _deactivate(self): + self._do_rollback(deactivate_only=True) + self.is_active = False + + def _do_rollback(self, deactivate_only=False): if self.is_active: self.connection._rollback_to_savepoint_impl( self._savepoint, self._parent diff --git a/test/dialect/test_sqlite.py b/test/dialect/test_sqlite.py index b9a5c28c1..1ec1d5d6f 100644 --- a/test/dialect/test_sqlite.py +++ b/test/dialect/test_sqlite.py @@ -2132,11 +2132,13 @@ class SavepointTest(fixtures.TablesTest): connection = self.bind.connect() transaction = connection.begin() connection.execute(users.insert(), user_id=1, user_name="user1") - connection.begin_nested() + trans2 = connection.begin_nested() connection.execute(users.insert(), user_id=2, user_name="user2") trans3 = connection.begin() connection.execute(users.insert(), user_id=3, user_name="user3") trans3.rollback() + + trans2.rollback() connection.execute(users.insert(), user_id=4, user_name="user4") transaction.commit() eq_( diff --git a/test/engine/test_transaction.py b/test/engine/test_transaction.py index 81f86089b..f6f5cd9fc 100644 --- a/test/engine/test_transaction.py +++ b/test/engine/test_transaction.py @@ -75,7 +75,6 @@ class TransactionTest(fixtures.TestBase): connection.execute(users.insert(), user_id=2, user_name="user2") connection.execute(users.insert(), user_id=3, user_name="user3") transaction.rollback() - result = connection.execute("select * from query_users") assert len(result.fetchall()) == 0 connection.close() @@ -167,11 +166,28 @@ class TransactionTest(fixtures.TestBase): branched.execute(users.insert(), user_id=2, user_name="user2") nested.rollback() assert not connection.in_transaction() - eq_(connection.scalar("select count(*) from query_users"), 0) + + assert_raises_message( + exc.InvalidRequestError, + "This connection is on an inactive transaction. Please", + connection.execute, + "select 1", + ) finally: connection.close() + def test_inactive_due_to_subtransaction_no_commit(self): + connection = testing.db.connect() + trans = connection.begin() + trans2 = connection.begin() + trans2.rollback() + assert_raises_message( + exc.InvalidRequestError, + "This transaction is inactive", + trans.commit, + ) + def test_branch_autorollback(self): connection = testing.db.connect() try: @@ -406,9 +422,19 @@ class TransactionTest(fixtures.TestBase): connection.execute(users.insert(), user_id=1, user_name="user1") trans2 = connection.begin_nested() connection.execute(users.insert(), user_id=2, user_name="user2") + trans3 = connection.begin() connection.execute(users.insert(), user_id=3, user_name="user3") trans3.rollback() + + assert_raises_message( + exc.InvalidRequestError, + "This connection is on an inactive savepoint transaction.", + connection.execute, + "select 1", + ) + trans2.rollback() + connection.execute(users.insert(), user_id=4, user_name="user4") transaction.commit() eq_( |