[10.x] Only stage committed transactions (#49093)

* Only stage committed transactions Updates the TransactionManager's stageTransactions method to accept a $level parameter, allowing it to only stage the transactions that will actually be committed. The previous implementation staged all existing transactions, which caused the manager's pendingTransactions and committedTransactions properties to be incorrect, leading to unexpected behaviour after a child transaction was committed but before the parent transaction had been committed. * Fix styleci * push failing test * fix failing test * add another failing test * rename test * fix formatting * add another test * rename test * rename test * refactor to parents * small fix * check connection * key current transactions by connection * another fix * call values * formatting * add another test * comment --------- Co-authored-by: Taylor Otwell <[email protected]>
laravel · Nov 23, 2023 · 2d2d82b · 2d2d82b
1 parent b99d035
commit 2d2d82b
Show file tree

Hide file tree

Showing 7 changed files with 277 additions and 35 deletions.
diff --git a/src/Illuminate/Database/Concerns/ManagesTransactions.php b/src/Illuminate/Database/Concerns/ManagesTransactions.php
@@ -47,7 +47,7 @@ public function transaction(Closure $callback, $attempts = 1)
                     $this->getPdo()->commit();
                 }
 
-                $this->transactionsManager?->stageTransactions($this->getName());
+                $this->transactionsManager?->stageTransactions($this->getName(), $this->transactions);
 
                 $this->transactions = max(0, $this->transactions - 1);
 
@@ -196,7 +196,7 @@ public function commit()
             $this->getPdo()->commit();
         }
 
-        $this->transactionsManager?->stageTransactions($this->getName());
+        $this->transactionsManager?->stageTransactions($this->getName(), $this->transactions);
 
         $this->transactions = max(0, $this->transactions - 1);
 

diff --git a/src/Illuminate/Database/DatabaseTransactionRecord.php b/src/Illuminate/Database/DatabaseTransactionRecord.php
@@ -18,6 +18,13 @@ class DatabaseTransactionRecord
      */
     public $level;
 
+    /**
+     * The parent instance of this transaction.
+     *
+     * @var \Illuminate\Database\DatabaseTransactionRecord
+     */
+    public $parent;
+
     /**
      * The callbacks that should be executed after committing.
      *
@@ -30,12 +37,14 @@ class DatabaseTransactionRecord
      *
      * @param  string  $connection
      * @param  int  $level
+     * @param  \Illuminate\Database\DatabaseTransactionRecord|null  $parent
      * @return void
      */
-    public function __construct($connection, $level)
+    public function __construct($connection, $level, ?DatabaseTransactionRecord $parent = null)
     {
         $this->connection = $connection;
         $this->level = $level;
+        $this->parent = $parent;
     }
 
     /**

diff --git a/src/Illuminate/Database/DatabaseTransactionsManager.php b/src/Illuminate/Database/DatabaseTransactionsManager.php
@@ -20,6 +20,13 @@ class DatabaseTransactionsManager
      */
     protected $pendingTransactions;
 
+    /**
+     * The current transaction.
+     *
+     * @var array
+     */
+    protected $currentTransaction = [];
+
     /**
      * Create a new database transactions manager instance.
      *
@@ -41,32 +48,57 @@ public function __construct()
     public function begin($connection, $level)
     {
         $this->pendingTransactions->push(
-            new DatabaseTransactionRecord($connection, $level)
+            $newTransaction = new DatabaseTransactionRecord(
+                $connection,
+                $level,
+                $this->currentTransaction[$connection] ?? null
+            )
         );
+
+        $this->currentTransaction[$connection] = $newTransaction;
     }
 
     /**
-     * Rollback the active database transaction.
+     * Move relevant pending transactions to a committed state.
      *
      * @param  string  $connection
-     * @param  int  $level
+     * @param  int  $levelBeingCommitted
      * @return void
      */
-    public function rollback($connection, $level)
+    public function stageTransactions($connection, $levelBeingCommitted)
     {
+        $this->committedTransactions = $this->committedTransactions->merge(
+            $this->pendingTransactions->filter(
+                fn ($transaction) => $transaction->connection === $connection &&
+                                     $transaction->level >= $levelBeingCommitted
+            )
+        );
+
         $this->pendingTransactions = $this->pendingTransactions->reject(
-            fn ($transaction) => $transaction->connection == $connection && $transaction->level > $level
-        )->values();
+            fn ($transaction) => $transaction->connection === $connection &&
+                                 $transaction->level >= $levelBeingCommitted
+        );
+
+        if (isset($this->currentTransaction[$connection])) {
+            $this->currentTransaction[$connection] = $this->currentTransaction[$connection]->parent;
+        }
     }
 
     /**
-     * Commit the active database transaction.
+     * Commit the root database transaction and execute callbacks.
      *
      * @param  string  $connection
      * @return void
      */
     public function commit($connection)
     {
+        // This method is only called when the root database transaction is committed so there
+        // shouldn't be any pending transactions, but going to clear them here anyways just
+        // in case. This method could be refactored to receive a level in the future too.
+        $this->pendingTransactions = $this->pendingTransactions->reject(
+            fn ($transaction) => $transaction->connection === $connection
+        )->values();
+
         [$forThisConnection, $forOtherConnections] = $this->committedTransactions->partition(
             fn ($transaction) => $transaction->connection == $connection
         );
@@ -77,35 +109,88 @@ public function commit($connection)
     }
 
     /**
-     * Register a transaction callback.
+     * Rollback the active database transaction.
      *
-     * @param  callable  $callback
+     * @param  string  $connection
+     * @param  int  $newTransactionLevel
      * @return void
      */
-    public function addCallback($callback)
+    public function rollback($connection, $newTransactionLevel)
     {
-        if ($current = $this->callbackApplicableTransactions()->last()) {
-            return $current->addCallback($callback);
+        if ($newTransactionLevel === 0) {
+            $this->removeAllTransactionsForConnection($connection);
+        } else {
+            $this->pendingTransactions = $this->pendingTransactions->reject(
+                fn ($transaction) => $transaction->connection == $connection &&
+                                     $transaction->level > $newTransactionLevel
+            )->values();
+
+            if ($this->currentTransaction) {
+                do {
+                    $this->removeCommittedTransactionsThatAreChildrenOf($this->currentTransaction[$connection]);
+
+                    $this->currentTransaction[$connection] = $this->currentTransaction[$connection]->parent;
+                } while (
+                    isset($this->currentTransaction[$connection]) &&
+                    $this->currentTransaction[$connection]->level > $newTransactionLevel
+                );
+            }
         }
-
-        $callback();
     }
 
     /**
-     * Move all the pending transactions to a committed state.
+     * Remove all pending, completed, and current transactions for the given connection name.
      *
      * @param  string  $connection
      * @return void
      */
-    public function stageTransactions($connection)
+    protected function removeAllTransactionsForConnection($connection)
     {
-        $this->committedTransactions = $this->committedTransactions->merge(
-            $this->pendingTransactions->filter(fn ($transaction) => $transaction->connection === $connection)
-        );
+        $this->currentTransaction[$connection] = null;
 
         $this->pendingTransactions = $this->pendingTransactions->reject(
-            fn ($transaction) => $transaction->connection === $connection
+            fn ($transaction) => $transaction->connection == $connection
+        )->values();
+
+        $this->committedTransactions = $this->committedTransactions->reject(
+            fn ($transaction) => $transaction->connection == $connection
+        )->values();
+    }
+
+    /**
+     * Remove all transactions that are children of the given transaction.
+     *
+     * @param  \Illuminate\Database\DatabaseTransactionRecord  $transaction
+     * @return void
+     */
+    protected function removeCommittedTransactionsThatAreChildrenOf(DatabaseTransactionRecord $transaction)
+    {
+        [$removedTransactions, $this->committedTransactions] = $this->committedTransactions->partition(
+            fn ($committed) => $committed->connection == $transaction->connection &&
+                               $committed->parent === $transaction
         );
+
+        // There may be multiple deeply nested transactions that have already committed that we
+        // also need to remove. We will recurse down the children of all removed transaction
+        // instances until there are no more deeply nested child transactions for removal.
+        $removedTransactions->each(
+            fn ($transaction) => $this->removeCommittedTransactionsThatAreChildrenOf($transaction)
+        );
+    }
+
+    /**
+     * Register a transaction callback.
+     *
+     * @param  callable  $callback
+     * @return void
+     */
+    public function addCallback($callback)
+    {
+        if ($current = $this->callbackApplicableTransactions()->last()) {
+            return $current->addCallback($callback);
+        }
+
+        $callback();
     }
 
     /**

diff --git a/tests/Database/DatabaseTransactionsManagerTest.php b/tests/Database/DatabaseTransactionsManagerTest.php
@@ -67,8 +67,8 @@ public function testCommittingTransactions()
         $manager->begin('default', 2);
         $manager->begin('admin', 1);
 
-        $manager->stageTransactions('default');
-        $manager->stageTransactions('admin');
+        $manager->stageTransactions('default', 1);
+        $manager->stageTransactions('admin', 1);
         $manager->commit('default');
 
         $this->assertCount(0, $manager->getPendingTransactions());
@@ -121,7 +121,7 @@ public function testCommittingTransactionsExecutesCallbacks()
 
         $manager->begin('admin', 1);
 
-        $manager->stageTransactions('default');
+        $manager->stageTransactions('default', 1);
         $manager->commit('default');
 
         $this->assertCount(2, $callbacks);
@@ -148,7 +148,7 @@ public function testCommittingExecutesOnlyCallbacksOfTheConnection()
             $callbacks[] = ['admin', 1];
         });
 
-        $manager->stageTransactions('default');
+        $manager->stageTransactions('default', 1);
         $manager->commit('default');
 
         $this->assertCount(1, $callbacks);
@@ -185,16 +185,29 @@ public function testStageTransactions()
         $this->assertEquals(1, $pendingTransactions[1]->level);
         $this->assertEquals('admin', $pendingTransactions[1]->connection);
 
-        $manager->stageTransactions('default');
+        $manager->stageTransactions('default', 1);
 
         $this->assertCount(1, $manager->getPendingTransactions());
         $this->assertCount(1, $manager->getCommittedTransactions());
         $this->assertEquals('default', $manager->getCommittedTransactions()[0]->connection);
 
-        $manager->stageTransactions('admin');
+        $manager->stageTransactions('admin', 1);
 
         $this->assertCount(0, $manager->getPendingTransactions());
         $this->assertCount(2, $manager->getCommittedTransactions());
         $this->assertEquals('admin', $manager->getCommittedTransactions()[1]->connection);
     }
+
+    public function testStageTransactionsOnlyStagesTheTransactionsAtOrAboveTheGivenLevel()
+    {
+        $manager = (new DatabaseTransactionsManager);
+
+        $manager->begin('default', 1);
+        $manager->begin('default', 2);
+        $manager->begin('default', 3);
+        $manager->stageTransactions('default', 2);
+
+        $this->assertCount(1, $manager->getPendingTransactions());
+        $this->assertCount(2, $manager->getCommittedTransactions());
+    }
 }
diff --git a/tests/Database/DatabaseTransactionsTest.php b/tests/Database/DatabaseTransactionsTest.php
@@ -64,7 +64,7 @@ public function testTransactionIsRecordedAndCommitted()
     {
         $transactionManager = m::mock(new DatabaseTransactionsManager);
         $transactionManager->shouldReceive('begin')->once()->with('default', 1);
-        $transactionManager->shouldReceive('stageTransactions')->once()->with('default');
+        $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 1);
         $transactionManager->shouldReceive('commit')->once()->with('default');
 
         $this->connection()->setTransactionManager($transactionManager);
@@ -84,7 +84,7 @@ public function testTransactionIsRecordedAndCommittedUsingTheSeparateMethods()
     {
         $transactionManager = m::mock(new DatabaseTransactionsManager);
         $transactionManager->shouldReceive('begin')->once()->with('default', 1);
-        $transactionManager->shouldReceive('stageTransactions')->once()->with('default');
+        $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 1);
         $transactionManager->shouldReceive('commit')->once()->with('default');
 
         $this->connection()->setTransactionManager($transactionManager);
@@ -105,7 +105,8 @@ public function testNestedTransactionIsRecordedAndCommitted()
         $transactionManager = m::mock(new DatabaseTransactionsManager);
         $transactionManager->shouldReceive('begin')->once()->with('default', 1);
         $transactionManager->shouldReceive('begin')->once()->with('default', 2);
-        $transactionManager->shouldReceive('stageTransactions')->twice()->with('default');
+        $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 1);
+        $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 2);
         $transactionManager->shouldReceive('commit')->once()->with('default');
 
         $this->connection()->setTransactionManager($transactionManager);
@@ -133,8 +134,9 @@ public function testNestedTransactionIsRecordeForDifferentConnectionsdAndCommitt
         $transactionManager->shouldReceive('begin')->once()->with('default', 1);
         $transactionManager->shouldReceive('begin')->once()->with('second_connection', 1);
         $transactionManager->shouldReceive('begin')->once()->with('second_connection', 2);
-        $transactionManager->shouldReceive('stageTransactions')->once()->with('default');
-        $transactionManager->shouldReceive('stageTransactions')->twice()->with('second_connection');
+        $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 1);
+        $transactionManager->shouldReceive('stageTransactions')->once()->with('second_connection', 1);
+        $transactionManager->shouldReceive('stageTransactions')->once()->with('second_connection', 2);
         $transactionManager->shouldReceive('commit')->once()->with('default');
         $transactionManager->shouldReceive('commit')->once()->with('second_connection');
 

diff --git a/tests/Foundation/Testing/DatabaseTransactionsManagerTest.php b/tests/Foundation/Testing/DatabaseTransactionsManagerTest.php
@@ -42,7 +42,7 @@ public function testItExecutesCallbacksForTheSecondTransaction()
 
         $this->assertFalse($testObject->ran);
 
-        $manager->stageTransactions('foo');
+        $manager->stageTransactions('foo', 1);
         $manager->commit('foo');
         $this->assertTrue($testObject->ran);
         $this->assertEquals(1, $testObject->runs);