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 (file)
index 0000000..fb8b90d
--- /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 e0a5866c6ecdc73c8cf742cdf13368d2fc18249a..4b8dd8797a860be5861d3cb7dcca8102d026a8aa 100644 (file)
--- 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 8eef9fc7b4e148bd046b8ab694fd1468f3035b61..479e1beb6faa8bf59925f3516468223564088bd0 100644 (file)
--- 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 7cbbfd8eadbb37d425cc3fa9b71dc121fc264aac..6edf93ffdc3cc3f502bc01876bc807e6f1d52696 100644 (file)
--- 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