Ignore join_transaction_mode when bind is an engine

Ignore :paramref:`_orm.Session.join_transaction_mode` in all cases when
the bind provided to the :class:`_orm.Session` is an
class:`_engine.Engine`.
Previously if an event that executed before the session logic,
like :meth:`_engine.ConnectionEvents.engine_connect`,
left the connection with an active transaction, the
paramref:`_orm.Session.join_transaction_mode` behavior took
place, leading to a surprising behavior.

Fixes: #11163
Change-Id: I10147876d07352f2dab898d615e98a9acd6eb91b

diff --git a/doc/build/changelog/unreleased_20/11163.rst b/doc/build/changelog/unreleased_20/11163.rst
new file mode 100644 (file)
index 0000000..da21b45
--- /dev/null
+++ b/doc/build/changelog/unreleased_20/11163.rst
@@ -0,0 +1,11 @@
+.. change::
+    :tags: orm
+    :tickets: 11163
+
+    Added a warning noting when an
+    :meth:`_engine.ConnectionEvents.engine_connect` event may be leaving
+    a transaction open, which can alter the behavior of a
+    :class:`_orm.Session` using such an engine as bind.
+    On SQLAlchemy 2.1 :paramref:`_orm.Session.join_transaction_mode` will
+    instead be ignored in all cases when the session bind is
+    an :class:`_engine.Engine`.

diff --git a/doc/build/changelog/unreleased_21/11163.rst b/doc/build/changelog/unreleased_21/11163.rst
new file mode 100644 (file)
index 0000000..c835571
--- /dev/null
+++ b/doc/build/changelog/unreleased_21/11163.rst
@@ -0,0 +1,12 @@
+.. change::
+    :tags: orm
+    :tickets: 11163
+
+    Ignore :paramref:`_orm.Session.join_transaction_mode` in all cases when
+    the bind provided to the :class:`_orm.Session` is an
+    :class:`_engine.Engine`.
+    Previously if an event that executed before the session logic,
+    like :meth:`_engine.ConnectionEvents.engine_connect`,
+    left the connection with an active transaction, the
+    :paramref:`_orm.Session.join_transaction_mode` behavior took
+    place, leading to a surprising behavior.

diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index c495a964f3e656adb007a8a19c9a0a5b0777b9e8..a23239e098ee55c35d3aeb6eb03e0556ef687015 100644 (file)
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -1208,30 +1208,38 @@ class SessionTransaction(_StateChange, TransactionalContext):
                 elif self.nested:
                     transaction = conn.begin_nested()
                 elif conn.in_transaction():
-                    join_transaction_mode = self.session.join_transaction_mode
 
-                    if join_transaction_mode == "conditional_savepoint":
-                        if conn.in_nested_transaction():
-                            join_transaction_mode = "create_savepoint"
-                        else:
-                            join_transaction_mode = "rollback_only"
-
-                    if join_transaction_mode in (
-                        "control_fully",
-                        "rollback_only",
-                    ):
-                        if conn.in_nested_transaction():
-                            transaction = (
-                                conn._get_required_nested_transaction()
-                            )
-                        else:
-                            transaction = conn._get_required_transaction()
-                        if join_transaction_mode == "rollback_only":
-                            should_commit = False
-                    elif join_transaction_mode == "create_savepoint":
-                        transaction = conn.begin_nested()
+                    if local_connect:
+                        _trans = conn.get_transaction()
+                        assert _trans is not None
+                        transaction = _trans
                     else:
-                        assert False, join_transaction_mode
+                        join_transaction_mode = (
+                            self.session.join_transaction_mode
+                        )
+
+                        if join_transaction_mode == "conditional_savepoint":
+                            if conn.in_nested_transaction():
+                                join_transaction_mode = "create_savepoint"
+                            else:
+                                join_transaction_mode = "rollback_only"
+
+                        if join_transaction_mode in (
+                            "control_fully",
+                            "rollback_only",
+                        ):
+                            if conn.in_nested_transaction():
+                                transaction = (
+                                    conn._get_required_nested_transaction()
+                                )
+                            else:
+                                transaction = conn._get_required_transaction()
+                            if join_transaction_mode == "rollback_only":
+                                should_commit = False
+                        elif join_transaction_mode == "create_savepoint":
+                            transaction = conn.begin_nested()
+                        else:
+                            assert False, join_transaction_mode
                 else:
                     transaction = conn.begin()
             except:

diff --git a/test/orm/test_transaction.py b/test/orm/test_transaction.py
index e502a88833086966a63daa591fc92c4de2bfea1a..eda7811846bd06ea48ab673baa1baf518b206163 100644 (file)
--- a/test/orm/test_transaction.py
+++ b/test/orm/test_transaction.py
@@ -108,7 +108,7 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest):
         trans.commit()
         assert len(sess.query(User).all()) == 1
 
-    @testing.variation(
+    join_transaction_mode = testing.variation(
         "join_transaction_mode",
         [
             "none",
@@ -118,6 +118,8 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest):
             "rollback_only",
         ],
     )
+
+    @join_transaction_mode
     @testing.variation("operation", ["commit", "close", "rollback", "nothing"])
     @testing.variation("external_state", ["none", "transaction", "savepoint"])
     def test_join_transaction_modes(
@@ -243,6 +245,57 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest):
         else:
             external_state.fail()
 
+    @join_transaction_mode
+    @testing.variation("operation", ["commit", "close", "rollback"])
+    def test_join_transaction_mode_with_event(
+        self, join_transaction_mode, operation
+    ):
+        eng = engines.testing_engine()
+        eng_conn = None
+        events = []
+
+        @event.listens_for(eng, "commit")
+        def on_commit(conn):
+            events.append("commit")
+
+        @event.listens_for(eng, "rollback")
+        def on_rollback(conn):
+            events.append("rollback")
+
+        @event.listens_for(eng.pool, "checkin")
+        def on_checkin(conn, record):
+            events.append("checkin")
+
+        @event.listens_for(eng, "engine_connect")
+        def make_stat(conn):
+            nonlocal eng_conn
+            eng_conn = conn
+            conn.begin()
+
+        if join_transaction_mode.none:
+            s = Session(eng)
+        else:
+            s = Session(eng, join_transaction_mode=join_transaction_mode.name)
+
+        s.connection()
+
+        expected = []
+        if operation.commit:
+            s.commit()
+            expected.append("commit")
+        elif operation.rollback:
+            s.rollback()
+            expected.append("rollback")
+        elif operation.close:
+            s.close()
+            expected.append("rollback")
+        else:
+            operation.fail()
+        is_(eng_conn.in_transaction(), False)
+
+        expected.append("checkin")
+        eq_(events, expected)
+
     def test_subtransaction_on_external_commit(self, connection_no_trans):
         users, User = self.tables.users, self.classes.User
 
@@ -839,7 +892,10 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest):
                 return_value=mock.Mock(
                     _is_future=False,
                     execution_options=mock.Mock(
-                        return_value=mock.Mock(_is_future=False)
+                        return_value=mock.Mock(
+                            _is_future=False,
+                            in_transaction=mock.Mock(return_value=False),
+                        )
                     ),
                 )
             )
@@ -857,7 +913,9 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest):
 
     def test_execution_options_ignored_mid_transaction(self):
         bind = mock.Mock()
-        conn = mock.Mock(engine=bind)
+        conn = mock.Mock(
+            engine=bind, in_transaction=mock.Mock(return_value=False)
+        )
         bind.connect = mock.Mock(return_value=conn)
         sess = Session(bind=bind)
         sess.execute(text("select 1"))