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) + } + } }