From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Wed, 26 Jul 2023 16:10:48 +0000 (-0400)
Subject: dont rely on default_isolation_level to restore engine-level iso
X-Git-Tag: rel_2_0_20~29
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a23cdcc12c8302a1a2deebb64876aaada8c996f6;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

dont rely on default_isolation_level to restore engine-level iso

Fixed critical issue where setting
:paramref:`_sa.create_engine.isolation_level` to ``AUTOCOMMIT`` (as opposed
to using the :meth:`_engine.Engine.execution_options` method) would fail to
restore "autocommit" to a pooled connection if an alternate isolation level
were temporarily selected using
:paramref:`_engine.Connection.execution_options.isolation_level`.

Fixes: #10147
Change-Id: Ia22c15ab460cbbf8f6a731660874ace10df7c6a6
---

diff --git a/doc/build/changelog/unreleased_20/10147.rst b/doc/build/changelog/unreleased_20/10147.rst
new file mode 100644
index 0000000000..fb8b90d49b
--- /dev/null
+++ b/doc/build/changelog/unreleased_20/10147.rst
@@ -0,0 +1,10 @@
+.. change::
+    :tags: bug, engine
+    :tickets: 10147
+
+    Fixed critical issue where setting
+    :paramref:`_sa.create_engine.isolation_level` to ``AUTOCOMMIT`` (as opposed
+    to using the :meth:`_engine.Engine.execution_options` method) would fail to
+    restore "autocommit" to a pooled connection if an alternate isolation level
+    were temporarily selected using
+    :paramref:`_engine.Connection.execution_options.isolation_level`.
diff --git a/lib/sqlalchemy/engine/default.py b/lib/sqlalchemy/engine/default.py
index e0a5866c6e..4b8dd8797a 100644
--- a/lib/sqlalchemy/engine/default.py
+++ b/lib/sqlalchemy/engine/default.py
@@ -964,12 +964,21 @@ class DefaultDialect(Dialect):
         self.set_isolation_level(dbapi_conn, level)
 
     def reset_isolation_level(self, dbapi_conn):
-        # default_isolation_level is read from the first connection
-        # after the initial set of 'isolation_level', if any, so is
-        # the configured default of this dialect.
-        self._assert_and_set_isolation_level(
-            dbapi_conn, self.default_isolation_level
-        )
+        if self._on_connect_isolation_level is not None:
+            assert (
+                self._on_connect_isolation_level == "AUTOCOMMIT"
+                or self._on_connect_isolation_level
+                == self.default_isolation_level
+            )
+            self._assert_and_set_isolation_level(
+                dbapi_conn, self._on_connect_isolation_level
+            )
+        else:
+            assert self.default_isolation_level is not None
+            self._assert_and_set_isolation_level(
+                dbapi_conn,
+                self.default_isolation_level,
+            )
 
     def normalize_name(self, name):
         if name is None:
diff --git a/lib/sqlalchemy/testing/requirements.py b/lib/sqlalchemy/testing/requirements.py
index 8eef9fc7b4..479e1beb6f 100644
--- a/lib/sqlalchemy/testing/requirements.py
+++ b/lib/sqlalchemy/testing/requirements.py
@@ -1409,6 +1409,15 @@ class SuiteRequirements(Requirements):
         """
         return exclusions.open()
 
+    @property
+    def independent_readonly_connections(self):
+        """
+        Target must support simultaneous, independent database connections
+        that will be used in a readonly fashion.
+
+        """
+        return exclusions.open()
+
     @property
     def skip_mysql_on_windows(self):
         """Catchall for a large variety of MySQL on Windows failures"""
diff --git a/lib/sqlalchemy/testing/suite/test_dialect.py b/lib/sqlalchemy/testing/suite/test_dialect.py
index 7cbbfd8ead..6edf93ffdc 100644
--- a/lib/sqlalchemy/testing/suite/test_dialect.py
+++ b/lib/sqlalchemy/testing/suite/test_dialect.py
@@ -247,6 +247,28 @@ class IsolationLevelTest(fixtures.TestBase):
         ):
             eng.connect()
 
+    @testing.requires.independent_readonly_connections
+    def test_dialect_user_setting_is_restored(self, testing_engine):
+        levels = requirements.get_isolation_levels(config)
+        default = levels["default"]
+        supported = (
+            sorted(
+                set(levels["supported"]).difference([default, "AUTOCOMMIT"])
+            )
+        )[0]
+
+        e = testing_engine(options={"isolation_level": supported})
+
+        with e.connect() as conn:
+            eq_(conn.get_isolation_level(), supported)
+
+        with e.connect() as conn:
+            conn.execution_options(isolation_level=default)
+            eq_(conn.get_isolation_level(), default)
+
+        with e.connect() as conn:
+            eq_(conn.get_isolation_level(), supported)
+
 
 class AutocommitIsolationTest(fixtures.TablesTest):
     run_deletes = "each"
@@ -308,6 +330,34 @@ class AutocommitIsolationTest(fixtures.TablesTest):
         )
         self._test_conn_autocommits(conn, False)
 
+    @testing.requires.independent_readonly_connections
+    @testing.variation("use_dialect_setting", [True, False])
+    def test_dialect_autocommit_is_restored(
+        self, testing_engine, use_dialect_setting
+    ):
+        """test #10147"""
+
+        if use_dialect_setting:
+            e = testing_engine(options={"isolation_level": "AUTOCOMMIT"})
+        else:
+            e = testing_engine().execution_options(
+                isolation_level="AUTOCOMMIT"
+            )
+
+        levels = requirements.get_isolation_levels(config)
+
+        default = levels["default"]
+
+        with e.connect() as conn:
+            self._test_conn_autocommits(conn, True)
+
+        with e.connect() as conn:
+            conn.execution_options(isolation_level=default)
+            self._test_conn_autocommits(conn, False)
+
+        with e.connect() as conn:
+            self._test_conn_autocommits(conn, True)
+
 
 class EscapingTest(fixtures.TestBase):
     @provide_metadata