From 64f1c8543f9efea1985122131ab3080471b3f414 Mon Sep 17 00:00:00 2001
From: catch <catch@35733.no-reply.drupal.org>
Date: Mon, 11 Dec 2023 09:59:38 +0000
Subject: [PATCH] Issue #3407080 by mondrake, alexpott: Leaving the savepoint
in the transaction stack upon rollback is incorrect
---
.../Transaction/TransactionManagerBase.php | 8 +-
.../DriverSpecificTransactionTestBase.php | 105 +++++++++++++++++-
2 files changed, 108 insertions(+), 5 deletions(-)
diff --git a/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php b/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php
index 0205f7dd65be..34bb51689dc4 100644
--- a/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php
+++ b/core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php
@@ -318,9 +318,13 @@ public function rollback(string $name, string $id): void {
if ($this->getConnectionTransactionState() === ClientConnectionTransactionState::Active) {
if ($this->stackDepth() > 1 && $this->stack()[$id]->type === StackItemType::Savepoint) {
// Rollback the client transaction to the savepoint when the Drupal
- // transaction is not a root one. The savepoint and therefore the
- // client connection remain active.
+ // transaction is not a root one. Then, release the savepoint too. The
+ // client connection remains active.
$this->rollbackClientSavepoint($name);
+ $this->releaseClientSavepoint($name);
+ // The Transaction object remains open, and when it will get destructed
+ // no commit should happen. Void the stack item.
+ $this->voidStackItem($id);
}
elseif ($this->stackDepth() === 1 && $this->stack()[$id]->type === StackItemType::Root) {
// If this was the root Drupal transaction, we can rollback the client
diff --git a/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificTransactionTestBase.php b/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificTransactionTestBase.php
index b8a71abba1e2..5d91cbd8ee74 100644
--- a/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificTransactionTestBase.php
+++ b/core/tests/Drupal/KernelTests/Core/Database/DriverSpecificTransactionTestBase.php
@@ -175,6 +175,50 @@ public function testRollbackRoot() {
$this->assertSame(0, $this->connection->transactionManager()->stackDepth());
}
+ /**
+ * Tests root transaction rollback after savepoint rollback.
+ */
+ public function testRollbackRootAfterSavepointRollback() {
+ $this->assertFalse($this->connection->inTransaction());
+ $this->assertSame(0, $this->connection->transactionManager()->stackDepth());
+
+ // Start root transaction. Corresponds to 'BEGIN TRANSACTION' on the
+ // database.
+ $transaction = $this->connection->startTransaction();
+ $this->assertTrue($this->connection->inTransaction());
+ $this->assertSame(1, $this->connection->transactionManager()->stackDepth());
+
+ // Insert a single row into the testing table.
+ $this->insertRow('David');
+ $this->assertRowPresent('David');
+
+ // Starts a savepoint transaction. Corresponds to 'SAVEPOINT savepoint_1'
+ // on the database.
+ $savepoint = $this->connection->startTransaction();
+ $this->assertTrue($this->connection->inTransaction());
+ $this->assertSame(2, $this->connection->transactionManager()->stackDepth());
+
+ // Insert a single row into the testing table.
+ $this->insertRow('Roger');
+ $this->assertRowPresent('David');
+ $this->assertRowPresent('Roger');
+
+ // Rollback savepoint. It should get released too. Corresponds to 'ROLLBACK
+ // TO savepoint_1' plus 'RELEASE savepoint_1' on the database.
+ $savepoint->rollBack();
+ $this->assertRowPresent('David');
+ $this->assertRowAbsent('Roger');
+ $this->assertTrue($this->connection->inTransaction());
+ $this->assertSame(1, $this->connection->transactionManager()->stackDepth());
+
+ // Try to rollback root. No savepoint is active, this should succeed.
+ $transaction->rollBack();
+ $this->assertRowAbsent('David');
+ $this->assertRowAbsent('Roger');
+ $this->assertFalse($this->connection->inTransaction());
+ $this->assertSame(0, $this->connection->transactionManager()->stackDepth());
+ }
+
/**
* Tests root transaction rollback failure when savepoint is open.
*/
@@ -232,13 +276,13 @@ public function testRollbackSavepoint() {
$this->assertRowPresent('David');
$this->assertRowPresent('Roger');
- // Rollback to savepoint. It should remain open. Corresponds to 'ROLLBACK
- // TO savepoint_1' on the database.
+ // Rollback savepoint. It should get released too. Corresponds to 'ROLLBACK
+ // TO savepoint_1' plus 'RELEASE savepoint_1' on the database.
$savepoint->rollBack();
$this->assertRowPresent('David');
$this->assertRowAbsent('Roger');
$this->assertTrue($this->connection->inTransaction());
- $this->assertSame(2, $this->connection->transactionManager()->stackDepth());
+ $this->assertSame(1, $this->connection->transactionManager()->stackDepth());
// Insert a row.
$this->insertRow('Syd');
@@ -252,6 +296,61 @@ public function testRollbackSavepoint() {
$this->assertSame(0, $this->connection->transactionManager()->stackDepth());
}
+ /**
+ * Tests savepoint transaction duplicated rollback.
+ */
+ public function testRollbackTwiceSameSavepoint() {
+ $this->assertFalse($this->connection->inTransaction());
+ $this->assertSame(0, $this->connection->transactionManager()->stackDepth());
+
+ // Start root transaction. Corresponds to 'BEGIN TRANSACTION' on the
+ // database.
+ $transaction = $this->connection->startTransaction();
+ $this->assertTrue($this->connection->inTransaction());
+ $this->assertSame(1, $this->connection->transactionManager()->stackDepth());
+
+ // Insert a row.
+ $this->insertRow('David');
+ $this->assertRowPresent('David');
+
+ // Starts a savepoint transaction. Corresponds to 'SAVEPOINT savepoint_1'
+ // on the database.
+ $savepoint = $this->connection->startTransaction();
+ $this->assertTrue($this->connection->inTransaction());
+ $this->assertSame(2, $this->connection->transactionManager()->stackDepth());
+
+ // Insert a row.
+ $this->insertRow('Roger');
+ $this->assertRowPresent('David');
+ $this->assertRowPresent('Roger');
+
+ // Rollback savepoint. It should get released too. Corresponds to 'ROLLBACK
+ // TO savepoint_1' plus 'RELEASE savepoint_1' on the database.
+ $savepoint->rollBack();
+ $this->assertRowPresent('David');
+ $this->assertRowAbsent('Roger');
+ $this->assertTrue($this->connection->inTransaction());
+ $this->assertSame(1, $this->connection->transactionManager()->stackDepth());
+
+ // Insert a row.
+ $this->insertRow('Syd');
+
+ // Rollback savepoint again. Should fail since it was released already.
+ try {
+ $savepoint->rollBack();
+ $this->fail('Expected TransactionOutOfOrderException was not thrown');
+ }
+ catch (\Exception $e) {
+ $this->assertInstanceOf(TransactionOutOfOrderException::class, $e);
+ $this->assertMatchesRegularExpression("/^Error attempting rollback of .*\\\\savepoint_1\\. Active stack: .*\\\\drupal_transaction/", $e->getMessage());
+ }
+ $this->assertRowPresent('David');
+ $this->assertRowAbsent('Roger');
+ $this->assertRowPresent('Syd');
+ $this->assertTrue($this->connection->inTransaction());
+ $this->assertSame(1, $this->connection->transactionManager()->stackDepth());
+ }
+
/**
* Tests savepoint transaction rollback failure when later savepoints exist.
*/
--
GitLab