From: Federico Caselli <cfederico87@gmail.com>
Date: Fri, 15 Mar 2024 18:15:27 +0000 (+0100)
Subject: Warn if an engine bind left an open transaction
X-Git-Tag: rel_2_0_32~30^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=673152560e53d9f60ab97b1cdb15fa52e01a8831;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

Warn if an engine bind left an open transaction

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`.

Fixes: #11163
Change-Id: I10147876d07352f2dab898d615e98a9acd6eb91b
(cherry picked from commit 69a2d0903e427e99acceedcd4e29a17d0b012bbe)
---

diff --git a/doc/build/changelog/unreleased_20/11163.rst b/doc/build/changelog/unreleased_20/11163.rst
new file mode 100644
index 0000000000..da21b45378
--- /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/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index c455ffac93..eb81f16e01 100644
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -1211,6 +1211,17 @@ class SessionTransaction(_StateChange, TransactionalContext):
                         else:
                             join_transaction_mode = "rollback_only"
 
+                        if local_connect:
+                            util.warn(
+                                "The engine provided as bind produced a "
+                                "connection that is already in a transaction. "
+                                "This is usually caused by a core event, "
+                                "such as 'engine_connect', that has left a "
+                                "transaction open. The effective join "
+                                "transaction mode used by this session is "
+                                f"{join_transaction_mode!r}. To silence this "
+                                "warning, do not leave transactions open"
+                            )
                     if join_transaction_mode in (
                         "control_fully",
                         "rollback_only",
diff --git a/test/orm/test_transaction.py b/test/orm/test_transaction.py
index e502a88833..47bcf69b57 100644
--- 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,36 @@ class SessionTransactionTest(fixtures.RemovesEvents, FixtureTest):
         else:
             external_state.fail()
 
+    @join_transaction_mode
+    def test_join_transaction_mode_with_event(self, join_transaction_mode):
+        eng = engines.testing_engine()
+
+        @event.listens_for(eng, "engine_connect")
+        def make_transaction(conn):
+            conn.begin()
+
+        if join_transaction_mode.none:
+            s = Session(eng)
+        else:
+            s = Session(eng, join_transaction_mode=join_transaction_mode.name)
+        if (
+            join_transaction_mode.none
+            or join_transaction_mode.conditional_savepoint
+        ):
+            with expect_warnings(
+                "The engine provided as bind produced a "
+                "connection that is already in a transaction. "
+                "This is usually caused by a core event, "
+                "such as 'engine_connect', that has left a "
+                "transaction open. The effective join "
+                "transaction mode used by this session is "
+                "'rollback_only'. To silence this "
+                "warning, do not leave transactions open"
+            ):
+                s.connection()
+        else:
+            s.connection()
+
     def test_subtransaction_on_external_commit(self, connection_no_trans):
         users, User = self.tables.users, self.classes.User
 
@@ -839,7 +871,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 +892,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"))