fix: EXPOSED-562 Any caught exception from inner transaction triggers…

… full rollback (#2251)

* fix: Any exception thrown by inner transaction triggers full rollback

A fix was included in the last release to ensure that failed operations in inner
transactions did not lead to saved commits (when useNestedTransactions = false)
if the exception was swallowed by some try-catch handler block. The fix made it
so that any exception triggers a rollback, which is problematic for exceptions
that are thrown by application/server/client Kotlin code. These do not necessarily
warrant a full database rollback.

The implemented fix has been loosened to only apply to database SQLExceptions.

Breaking changes doc has also been amended to reflect the change more clearly.

* fix: EXPOSED-562 Any exception thrown by inner transaction triggers full rollback

- Add extra retroactive details to breaking changes doc
- Add unit tests for thrown exceptions with nested transactions, to ensure no regression there
- Refactor transaction exception catch blocks

CHANGELOG.md

@@ -14,6 +14,7 @@ Breaking changes:
 * feat!: EXPOSED-476 Update Kotlin to 2.0.0 by @bog-walk in https://github.com/JetBrains/Exposed/pull/2188
 * refactor!: Move `statementsRequiredForDatabaseMigration` function from `SchemaUtils` to `MigrationUtils` by @joc-a in https://github.com/JetBrains/Exposed/pull/2195
 * feat!: EXPOSED-436 Allow using insert values on update with upsert() by @bog-walk in https://github.com/JetBrains/Exposed/pull/2172
+* fix!: EXPOSED-439 Outer transaction commits rows from failed inner transaction by @bog-walk in https://github.com/JetBrains/Exposed/pull/2186
 
 Deprecations:
 * deprecate: Raise deprecation levels of API elements by @bog-walk in https://github.com/JetBrains/Exposed/pull/2208
@@ -25,7 +26,6 @@ Features:
 * feat: EXPOSED-486 Support REPLACE INTO ... SELECT clause by @bog-walk in https://github.com/JetBrains/Exposed/pull/2199
 
 Bug fixes:
-* fix: EXPOSED-439 Outer transaction commits rows from failed inner transaction by @bog-walk in https://github.com/JetBrains/Exposed/pull/2186
 * fix: EXPOSED-464 `CurrentTimestampWithTimeZone` expression does not work as a default by @joc-a in https://github.com/JetBrains/Exposed/pull/2180
 * fix: EXPOSED-474 Unexpected value of type when using a ColumnTransfor… by @obabichevjb in https://github.com/JetBrains/Exposed/pull/2191
 * fix: EXPOSED-472 Alias IdTable fails with isNull and eq ops by @bog-walk in https://github.com/JetBrains/Exposed/pull/2189

documentation-website/Writerside/topics/Breaking-Changes.md

@@ -65,6 +65,13 @@ TestTable.upsert(
 }
 ```
 * The function `statementsRequiredForDatabaseMigration` has been moved from `SchemaUtils` to `MigrationUtils` in the `exposed-migration` module.
+* A nested transaction (with `useNestedTransactions = true`) that throws any exception will now rollback any commits since the last savepoint.
+  This ensures that the nested transaction is properly configured to act in the exact same way as a top-level transaction or `inTopLevelTransaction()`.
+
+  An inner transaction (with `useNestedTransactions = false`) that throws any exception will also rollback any commits since the last savepoint.
+  This ensures that any exception propagated from the inner transaction to the outer transaction will not be swallowed if caught by some
+  exception handler wrapping the inner transaction, and any inner commits will not be saved. In version 0.55.0, this change will be reduced
+  so that only inner transactions that throw an `SQLException` from the database will trigger such a rollback.
 
 ## 0.51.0
 

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/transactions/ThreadLocalTransactionManager.kt

@@ -260,7 +260,7 @@ fun <T> transaction(
                     transaction.commit()
                 }
             }
-        } catch (cause: Throwable) {
+        } catch (cause: SQLException) {
             val currentStatement = transaction.currentStatement
             transaction.rollbackLoggingException {
                 exposedLogger.warn(
@@ -269,6 +269,17 @@ fun <T> transaction(
                 )
             }
             throw cause
+        } catch (cause: Throwable) {
+            if (outer.db.useNestedTransactions) {
+                val currentStatement = transaction.currentStatement
+                transaction.rollbackLoggingException {
+                    exposedLogger.warn(
+                        "Transaction rollback failed: ${it.message}. Statement: $currentStatement",
+                        it
+                    )
+                }
+            }
+            throw cause
         } finally {
             TransactionManager.resetCurrent(outerManager)
         }

exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/NestedTransactionsTest.kt

@@ -22,6 +22,15 @@ import kotlin.test.assertNotNull
 import kotlin.test.fail
 
 class NestedTransactionsTest : DatabaseTestsBase() {
+    private val db by lazy {
+        Database.connect(
+            "jdbc:h2:mem:db1;DB_CLOSE_DELAY=-1;", "org.h2.Driver", "root", "",
+            databaseConfig = DatabaseConfig {
+                useNestedTransactions = true
+                defaultMaxAttempts = 1
+            }
+        )
+    }
 
     @Test
     fun testNestedTransactions() {
@@ -78,21 +87,9 @@ class NestedTransactionsTest : DatabaseTestsBase() {
     }
 
     @Test
-    fun testNestedTransactionNotCommittedAfterFailure() {
+    fun testNestedTransactionNotCommittedAfterDatabaseFailure() {
         Assume.assumeTrue(TestDB.H2_V2 in TestDB.enabledDialects())
-        val db = Database.connect(
-            "jdbc:h2:mem:db1;DB_CLOSE_DELAY=-1;", "org.h2.Driver", "root", "",
-            databaseConfig = DatabaseConfig {
-                useNestedTransactions = true
-                defaultMaxAttempts = 1
-            }
-        )
 
-        fun assertSingleRecordInNewTransactionAndReset() = transaction(db) {
-            val result = DMLTestsData.Cities.selectAll().single()[DMLTestsData.Cities.name]
-            assertEquals("City A", result)
-            DMLTestsData.Cities.deleteAll()
-        }
         val fakeSQLString = "BROKEN_SQL_THAT_CAUSES_EXCEPTION"
 
         transaction(db) {
@@ -147,4 +144,67 @@ class NestedTransactionsTest : DatabaseTestsBase() {
             SchemaUtils.drop(DMLTestsData.Cities)
         }
     }
+
+    @Test
+    fun testNestedTransactionNotCommittedAfterException() {
+        Assume.assumeTrue(TestDB.H2_V2 in TestDB.enabledDialects())
+
+        val exceptionMessage = "Failure!"
+
+        transaction(db) {
+            SchemaUtils.create(DMLTestsData.Cities)
+        }
+
+        transaction(db) {
+            val outerTxId = this.id
+
+            DMLTestsData.Cities.insert { it[name] = "City A" }
+            assertEquals(1, DMLTestsData.Cities.selectAll().count())
+
+            try {
+                inTopLevelTransaction(db.transactionManager.defaultIsolationLevel, db = db) {
+                    val innerTxId = this.id
+                    assertNotEquals(outerTxId, innerTxId)
+
+                    DMLTestsData.Cities.insert { it[name] = "City B" }
+                    error(exceptionMessage)
+                }
+            } catch (cause: IllegalStateException) {
+                assertContains(cause.toString(), exceptionMessage)
+            }
+        }
+
+        assertSingleRecordInNewTransactionAndReset()
+
+        transaction(db) {
+            val outerTxId = this.id
+
+            DMLTestsData.Cities.insert { it[name] = "City A" }
+            assertEquals(1, DMLTestsData.Cities.selectAll().count())
+
+            try {
+                transaction(db) {
+                    val innerTxId = this.id
+                    assertNotEquals(outerTxId, innerTxId)
+
+                    DMLTestsData.Cities.insert { it[name] = "City B" }
+                    error(exceptionMessage)
+                }
+            } catch (cause: IllegalStateException) {
+                assertContains(cause.toString(), exceptionMessage)
+            }
+        }
+
+        assertSingleRecordInNewTransactionAndReset()
+
+        transaction(db) {
+            SchemaUtils.drop(DMLTestsData.Cities)
+        }
+    }
+
+    private fun assertSingleRecordInNewTransactionAndReset() = transaction(db) {
+        val result = DMLTestsData.Cities.selectAll().single()[DMLTestsData.Cities.name]
+        assertEquals("City A", result)
+        DMLTestsData.Cities.deleteAll()
+    }
 }

exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/RollbackTransactionTest.kt

@@ -1,12 +1,18 @@
 package org.jetbrains.exposed.sql.tests.shared
 
+import org.jetbrains.exposed.sql.SchemaUtils
 import org.jetbrains.exposed.sql.insert
 import org.jetbrains.exposed.sql.selectAll
 import org.jetbrains.exposed.sql.tests.DatabaseTestsBase
+import org.jetbrains.exposed.sql.tests.TestDB
 import org.jetbrains.exposed.sql.transactions.inTopLevelTransaction
 import org.jetbrains.exposed.sql.transactions.transaction
 import org.jetbrains.exposed.sql.transactions.transactionManager
+import org.junit.Assume
 import org.junit.Test
+import java.sql.SQLException
+import kotlin.test.assertContains
+import kotlin.test.fail
 
 class RollbackTransactionTest : DatabaseTestsBase() {
 
@@ -54,4 +60,65 @@ class RollbackTransactionTest : DatabaseTestsBase() {
             assertEquals(0L, RollbackTable.selectAll().where { RollbackTable.value eq "after-dummy" }.count())
         }
     }
+
+    @Test
+    fun testRollbackWithoutSavepointsTriggeredByExceptions() {
+        Assume.assumeTrue(TestDB.H2_V2 in TestDB.enabledDialects())
+        TestDB.H2_V2.connect()
+
+        transaction {
+            SchemaUtils.create(RollbackTable)
+        }
+
+        // database exception triggers rollback from inner to outer tx
+        transaction {
+            val fakeSQLString = "BROKEN_SQL_THAT_CAUSES_EXCEPTION"
+            val outerTxId = this.id
+
+            RollbackTable.insert { it[value] = "City A" }
+            assertEquals(1, RollbackTable.selectAll().count())
+
+            try {
+                transaction {
+                    val innerTxId = this.id
+                    assertEquals(outerTxId, innerTxId)
+
+                    RollbackTable.insert { it[value] = "City B" }
+                    exec("$fakeSQLString()")
+                }
+                fail("Should have thrown an exception")
+            } catch (cause: SQLException) {
+                assertContains(cause.toString(), fakeSQLString)
+            }
+
+            assertEquals(0, RollbackTable.selectAll().count())
+        }
+
+        // non-db exception propagates from inner to outer without rollback and is handled, if caught.
+        // if not caught & exception propagates all the way to outer tx, full rollback occurs (as always).
+        transaction {
+            val outerTxId = this.id
+
+            RollbackTable.insert { it[value] = "City A" }
+            assertEquals(1, RollbackTable.selectAll().count())
+
+            try {
+                transaction(db) {
+                    val innerTxId = this.id
+                    assertEquals(outerTxId, innerTxId)
+
+                    RollbackTable.insert { it[value] = "City B" }
+                    error("Failure")
+                }
+            } catch (cause: IllegalStateException) {
+                assertContains(cause.toString(), "Failure")
+            }
+
+            assertEquals(2, RollbackTable.selectAll().count())
+        }
+
+        transaction {
+            SchemaUtils.drop(RollbackTable)
+        }
+    }
 }