Issue #3384997 by mondrake, daffie: Rolling back to a savepoint should leave...

Issue #3384997 by mondrake, daffie: Rolling back to a savepoint should leave the savepoint in the transaction stack

(cherry picked from commit 666f9e98)

core/lib/Drupal/Core/Database/Transaction/TransactionManagerBase.php

@@ -8,7 +8,6 @@
 use Drupal\Core\Database\Transaction;
 use Drupal\Core\Database\TransactionCommitFailedException;
 use Drupal\Core\Database\TransactionNameNonUniqueException;
-use Drupal\Core\Database\TransactionNoActiveException;
 use Drupal\Core\Database\TransactionOutOfOrderException;
 
 /**
@@ -97,7 +96,7 @@ public function __construct(
    * When destructing, $stack must have been already emptied.
    */
   public function __destruct() {
-    assert($this->stack === [], "Transaction \$stack was not empty. " . var_export($this->stack, TRUE));
+    assert($this->stack === [], "Transaction \$stack was not empty. Active stack: " . $this->dumpStackItemsAsString());
   }
 
   /**
@@ -172,6 +171,29 @@ protected function voidStackItem(string $id): void {
     $this->removeStackItem($id);
   }
 
+  /**
+   * Produces a string representation of the stack items.
+   *
+   * A helper method for exception messages.
+   *
+   * Drivers should not override this method unless they also override the
+   * $stack property.
+   *
+   * @return string
+   *   The string representation of the stack items.
+   */
+  protected function dumpStackItemsAsString(): string {
+    if ($this->stack() === []) {
+      return '*** empty ***';
+    }
+
+    $temp = [];
+    foreach ($this->stack() as $id => $item) {
+      $temp[] = $id . '\\' . $item->name;
+    }
+    return implode(' > ', $temp);
+  }
+
   /**
    * {@inheritdoc}
    */
@@ -195,7 +217,7 @@ public function push(string $name = ''): Transaction {
     }
 
     if ($this->has($name)) {
-      throw new TransactionNameNonUniqueException($name . " is already in use.");
+      throw new TransactionNameNonUniqueException("A transaction named {$name} is already in use. Active stack: " . $this->dumpStackItemsAsString());
     }
 
     // Do the client-level processing.
@@ -231,7 +253,7 @@ public function unpile(string $name, string $id): void {
     // DDL statement breaking an active transaction). That should be listed in
     // $voidedItems, so we can remove it from there.
     if (!isset($this->stack()[$id]) || $this->stack()[$id]->name !== $name) {
-      assert(isset($this->voidedItems[$id]), "Transaction {$id}/{$name} was out of sequence.");
+      assert(isset($this->voidedItems[$id]), "Transaction {$id}/{$name} is out of sequence. Active stack: " . $this->dumpStackItemsAsString());
       unset($this->voidedItems[$id]);
       return;
     }
@@ -257,7 +279,7 @@ public function unpile(string $name, string $id): void {
       }
       else {
         // The stack got corrupted.
-        throw new TransactionOutOfOrderException();
+        throw new TransactionOutOfOrderException("Transaction {$id}/{$name} is out of order. Active stack: " . $this->dumpStackItemsAsString());
       }
 
       // Remove the transaction from the stack.
@@ -266,7 +288,7 @@ public function unpile(string $name, string $id): void {
     }
 
     // The stack got corrupted.
-    throw new TransactionOutOfOrderException();
+    throw new TransactionOutOfOrderException("Transaction {$id}/{$name} is out of order. Active stack: " . $this->dumpStackItemsAsString());
   }
 
   /**
@@ -288,38 +310,35 @@ public function rollback(string $name, string $id): void {
     }
     // End of BC layer.
 
-    // If the $id does not correspond to the one in the stack for that $name,
-    // we are facing an orphaned Transaction object (for example in case of a
-    // DDL statement breaking an active transaction), so there is nothing more
-    // to do.
-    if (!isset($this->stack()[$id]) || $this->stack()[$id]->name !== $name) {
-      return;
-    }
-
-    if (!$this->inTransaction()) {
-      throw new TransactionNoActiveException();
-    }
-
     // Rolled back item should match the last one in stack.
-    if ($id != array_key_last($this->stack())) {
-      throw new TransactionOutOfOrderException();
+    if ($id != array_key_last($this->stack()) || $name !== $this->stack()[$id]->name) {
+      throw new TransactionOutOfOrderException("Error attempting rollback of {$id}\\{$name}. Active stack: " . $this->dumpStackItemsAsString());
     }
 
-    // Do the client-level processing.
-    match ($this->stack()[$id]->type) {
-      StackItemType::Root => $this->processRootRollback(),
-      StackItemType::Savepoint => $this->rollbackClientSavepoint($name),
-    };
-
-    // Remove the transaction from the stack. The Transaction object is still
-    // active, so we need to void the stack item.
+    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.
+        $this->rollbackClientSavepoint($name);
+      }
+      elseif ($this->stackDepth() === 1 && $this->stack()[$id]->type === StackItemType::Root) {
+        // If this was the root Drupal transaction, we can rollback the client
+        // transaction. The transaction is closed.
+        $this->processRootRollback();
+        // The Transaction object remains open, and when it will get destructed
+        // no commit should happen. Void the stack item.
         $this->voidStackItem($id);
-
-    // If this was the last Drupal transaction open, we can commit the client
-    // transaction.
-    if ($this->stackDepth() === 0 && $this->getConnectionTransactionState() === ClientConnectionTransactionState::Active) {
-      $this->processRootCommit();
       }
+      else {
+        // The stack got corrupted.
+        throw new TransactionOutOfOrderException("Error attempting rollback of {$id}\\{$name}. Active stack: " . $this->dumpStackItemsAsString());
+      }
+      return;
+    }
+
+    // The stack got corrupted.
+    throw new TransactionOutOfOrderException("Error attempting rollback of {$id}\\{$name}. Active stack: " . $this->dumpStackItemsAsString());
   }
 
   /**

core/tests/Drupal/KernelTests/Core/Database/DriverSpecificTransactionTestBase.php

@@ -151,26 +151,151 @@ protected function transactionInnerLayer($suffix, $rollback = FALSE, $ddl_statem
   }
 
   /**
-   * Tests transaction rollback on a database that supports transactions.
-   *
-   * If the active connection does not support transactions, this test does
-   * nothing.
+   * Tests root transaction rollback.
    */
-  public function testTransactionRollBackSupported() {
-    try {
-      // Create two nested transactions. Roll back from the inner one.
-      $this->transactionOuterLayer('B', TRUE);
+  public function testRollbackRoot() {
+    $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');
 
-      // Neither of the rows we inserted in the two transaction layers
-      // should be present in the tables post-rollback.
-      $saved_age = $this->connection->query('SELECT [age] FROM {test} WHERE [name] = :name', [':name' => 'DavidB'])->fetchField();
-      $this->assertNotSame('24', $saved_age, 'Cannot retrieve DavidB row after commit.');
-      $saved_age = $this->connection->query('SELECT [age] FROM {test} WHERE [name] = :name', [':name' => 'DanielB'])->fetchField();
-      $this->assertNotSame('19', $saved_age, 'Cannot retrieve DanielB row after commit.');
+    // Rollback. Since we are at the root, the transaction is closed.
+    // Corresponds to 'ROLLBACK' on the database.
+    $transaction->rollBack();
+    $this->assertRowAbsent('David');
+    $this->assertFalse($this->connection->inTransaction());
+    $this->assertSame(0, $this->connection->transactionManager()->stackDepth());
   }
-    catch (\Exception $e) {
-      $this->fail($e->getMessage());
+
+  /**
+   * Tests root transaction rollback failure when savepoint is open.
+   */
+  public function testRollbackRootWithActiveSavepoint() {
+    $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());
+
+    // Try to rollback root. Since we a savepoint is active, this should fail.
+    $this->expectException(TransactionOutOfOrderException::class);
+    $this->expectExceptionMessageMatches("/^Error attempting rollback of .*\\\\drupal_transaction\\. Active stack: .*\\\\drupal_transaction > .*\\\\savepoint_1/");
+    $transaction->rollBack();
   }
+
+  /**
+   * Tests savepoint transaction rollback.
+   */
+  public function testRollbackSavepoint() {
+    $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 to savepoint. It should remain open. Corresponds to 'ROLLBACK
+    // TO 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());
+
+    // Insert a row.
+    $this->insertRow('Syd');
+
+    // Commit root. Corresponds to 'COMMIT' on the database.
+    unset($transaction);
+    $this->assertRowPresent('David');
+    $this->assertRowAbsent('Roger');
+    $this->assertRowPresent('Syd');
+    $this->assertFalse($this->connection->inTransaction());
+    $this->assertSame(0, $this->connection->transactionManager()->stackDepth());
+  }
+
+  /**
+   * Tests savepoint transaction rollback failure when later savepoints exist.
+   */
+  public function testRollbackSavepointWithLaterSavepoint() {
+    $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.
+    $savepoint1 = $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');
+
+    // Starts a savepoint transaction. Corresponds to 'SAVEPOINT savepoint_2'
+    // on the database.
+    $savepoint2 = $this->connection->startTransaction();
+    $this->assertTrue($this->connection->inTransaction());
+    $this->assertSame(3, $this->connection->transactionManager()->stackDepth());
+
+    // Insert a row.
+    $this->insertRow('Syd');
+    $this->assertRowPresent('David');
+    $this->assertRowPresent('Roger');
+    $this->assertRowPresent('Syd');
+
+    // Try to rollback to savepoint 1. Out of order.
+    $this->expectException(TransactionOutOfOrderException::class);
+    $this->expectExceptionMessageMatches("/^Error attempting rollback of .*\\\\savepoint_1\\. Active stack: .*\\\\drupal_transaction > .*\\\\savepoint_1 > .*\\\\savepoint_2/");
+    $savepoint1->rollBack();
   }
 
   /**
@@ -389,55 +514,6 @@ public function testTransactionStacking() {
     $this->assertRowPresent('outer');
     $this->assertRowAbsent('inner');
     $this->assertRowPresent('outer-after-inner-rollback');
-
-    // Rollback the inner transaction after committing the outer one.
-    $this->cleanUp();
-    $transaction = $this->connection->startTransaction();
-    $this->insertRow('outer');
-    $transaction2 = $this->connection->startTransaction();
-    $this->insertRow('inner');
-    // Unset the outer (root) transaction, should commit.
-    unset($transaction);
-    $this->assertFalse($this->connection->inTransaction());
-    // Unpile the inner (savepoint) Transaction object, it should be a no-op
-    // anyway given it was dropped by the database already, and removed from
-    // our transaction stack.
-    $transaction2->rollBack();
-    unset($transaction2);
-    $this->assertFalse($this->connection->inTransaction(), 'Transaction closed after popping the inner transaction');
-    $this->assertRowPresent('outer');
-    $this->assertRowPresent('inner');
-
-    // Rollback the outer transaction while the inner transaction is active.
-    // In that case, an exception will be triggered because we cannot
-    // ensure that the final result will have any meaning.
-    $this->cleanUp();
-    $transaction = $this->connection->startTransaction();
-    $this->insertRow('outer');
-    $transaction2 = $this->connection->startTransaction();
-    $this->insertRow('inner');
-    $transaction3 = $this->connection->startTransaction();
-    $this->insertRow('inner2');
-    // Rollback the outer transaction.
-    try {
-      $transaction->rollBack();
-      unset($transaction);
-      $this->fail('Rolling back the outer transaction while the inner transaction is active resulted in an exception.');
-    }
-    catch (TransactionOutOfOrderException $e) {
-      // Expected exception; just continue testing.
-    }
-    // Rollback of the root Transaction failed, we are still in an active
-    // client transaction.
-    $this->assertTrue($this->connection->inTransaction());
-    // Release latest savepoint (=inner2) transaction.
-    unset($transaction3);
-    // Rollback remaining transactions in backwards order.
-    $transaction2->rollBack();
-    $transaction->rollBack();
-    $this->assertRowAbsent('outer');
-    $this->assertRowAbsent('inner');
-    $this->assertRowAbsent('inner2');
   }
 
   /**
@@ -714,7 +790,7 @@ public function testTransactionManagerFailureOnPendingStackItems(): void {
     $reflectionMethod->invoke($testConnection->transactionManager(), 'bar', new StackItem('qux', StackItemType::Savepoint));
 
     $this->expectException(\AssertionError::class);
-    $this->expectExceptionMessage('Transaction $stack was not empty');
+    $this->expectExceptionMessageMatches("/^Transaction .stack was not empty\\. Active stack: bar\\\\qux/");
     unset($testConnection);
     Database::closeConnection('test_fail');
   }