From a3cb0c51a4875d22dbd37671a91d501144e50eba Mon Sep 17 00:00:00 2001 From: bog-walk <82039410+bog-walk@users.noreply.github.com> Date: Wed, 25 Sep 2024 05:53:36 -0400 Subject: [PATCH] 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 | 2 +- .../Writerside/topics/Breaking-Changes.md | 7 ++ .../ThreadLocalTransactionManager.kt | 13 ++- .../tests/shared/NestedTransactionsTest.kt | 86 ++++++++++++++++--- .../tests/shared/RollbackTransactionTest.kt | 67 +++++++++++++++ 5 files changed, 160 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5938d4ee3a..19243d1ff9 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/documentation-website/Writerside/topics/Breaking-Changes.md b/documentation-website/Writerside/topics/Breaking-Changes.md index c762298453..9b8e9def11 100644 --- a/documentation-website/Writerside/topics/Breaking-Changes.md +++ b/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 diff --git a/exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/transactions/ThreadLocalTransactionManager.kt b/exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/transactions/ThreadLocalTransactionManager.kt index 955bc70d05..360d2fb19a 100644 --- a/exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/transactions/ThreadLocalTransactionManager.kt +++ b/exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/transactions/ThreadLocalTransactionManager.kt @@ -260,7 +260,7 @@ fun transaction( transaction.commit() } } - } catch (cause: Throwable) { + } catch (cause: SQLException) { val currentStatement = transaction.currentStatement transaction.rollbackLoggingException { exposedLogger.warn( @@ -269,6 +269,17 @@ fun 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) } diff --git a/exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/NestedTransactionsTest.kt b/exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/NestedTransactionsTest.kt index 077a12c60c..b92c1fae2c 100644 --- a/exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/NestedTransactionsTest.kt +++ b/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() + } } diff --git a/exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/RollbackTransactionTest.kt b/exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/RollbackTransactionTest.kt index 883fb97131..4029d139ff 100644 --- a/exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/RollbackTransactionTest.kt +++ b/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) + } + } }