[10.x] Fix how nested transaction callbacks are handled (#48859)

* failing test * naive solution * Add comment * Revert "naive solution" This reverts commit ef46049. * wip * Fix DatabaseTransactionsManager tests * fix null object * Fix Testing DatabaseTransactionsManager * make styleci happy * additional test * Pass connection name to stageTransactions * Add unit test * add tests for Testing DatabaseTransactionsManager * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * formatting --------- Signed-off-by: Mior Muhammad Zaki <[email protected]> Co-authored-by: Mior Muhammad Zaki <[email protected]> Co-authored-by: Taylor Otwell <[email protected]>
laravel · Nov 10, 2023 · 95e924d · 95e924d
1 parent 7faedf1
commit 95e924d
Show file tree

Hide file tree

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

diff --git a/src/Illuminate/Database/DatabaseTransactionsManager.php b/src/Illuminate/Database/DatabaseTransactionsManager.php
@@ -2,14 +2,23 @@
 
 namespace Illuminate\Database;
 
+use Illuminate\Support\Collection;
+
 class DatabaseTransactionsManager
 {
     /**
-     * All of the recorded transactions.
+     * All of the committed transactions.
+     *
+     * @var \Illuminate\Support\Collection<int, \Illuminate\Database\DatabaseTransactionRecord>
+     */
+    protected $committedTransactions;
+
+    /**
+     * All of the pending transactions.
      *
      * @var \Illuminate\Support\Collection<int, \Illuminate\Database\DatabaseTransactionRecord>
      */
-    protected $transactions;
+    protected $pendingTransactions;
 
     /**
      * Create a new database transactions manager instance.
@@ -18,7 +27,8 @@ class DatabaseTransactionsManager
      */
     public function __construct()
     {
-        $this->transactions = collect();
+        $this->committedTransactions = new Collection;
+        $this->pendingTransactions = new Collection;
     }
 
     /**
@@ -30,7 +40,7 @@ public function __construct()
      */
     public function begin($connection, $level)
     {
-        $this->transactions->push(
+        $this->pendingTransactions->push(
             new DatabaseTransactionRecord($connection, $level)
         );
     }
@@ -44,7 +54,7 @@ public function begin($connection, $level)
      */
     public function rollback($connection, $level)
     {
-        $this->transactions = $this->transactions->reject(
+        $this->pendingTransactions = $this->pendingTransactions->reject(
             fn ($transaction) => $transaction->connection == $connection && $transaction->level > $level
         )->values();
     }
@@ -57,11 +67,11 @@ public function rollback($connection, $level)
      */
     public function commit($connection)
     {
-        [$forThisConnection, $forOtherConnections] = $this->transactions->partition(
+        [$forThisConnection, $forOtherConnections] = $this->committedTransactions->partition(
             fn ($transaction) => $transaction->connection == $connection
         );
 
-        $this->transactions = $forOtherConnections->values();
+        $this->committedTransactions = $forOtherConnections->values();
 
         $forThisConnection->map->executeCallbacks();
     }
@@ -81,14 +91,31 @@ public function addCallback($callback)
         $callback();
     }
 
+    /**
+     * Move all the pending transactions to a committed state.
+     *
+     * @param  string  $connection
+     * @return void
+     */
+    public function stageTransactions($connection)
+    {
+        $this->committedTransactions = $this->committedTransactions->merge(
+            $this->pendingTransactions->filter(fn ($transaction) => $transaction->connection === $connection)
+        );
+
+        $this->pendingTransactions = $this->pendingTransactions->reject(
+            fn ($transaction) => $transaction->connection === $connection
+        );
+    }
+
     /**
      * Get the transactions that are applicable to callbacks.
      *
      * @return \Illuminate\Support\Collection<int, \Illuminate\Database\DatabaseTransactionRecord>
      */
     public function callbackApplicableTransactions()
     {
-        return $this->transactions;
+        return $this->pendingTransactions;
     }
 
     /**
@@ -103,12 +130,22 @@ public function afterCommitCallbacksShouldBeExecuted($level)
     }
 
     /**
-     * Get all the transactions.
+     * Get all of the pending transactions.
+     *
+     * @return \Illuminate\Support\Collection
+     */
+    public function getPendingTransactions()
+    {
+        return $this->pendingTransactions;
+    }
+
+    /**
+     * Get all of the committed transactions.
      *
      * @return \Illuminate\Support\Collection
      */
-    public function getTransactions()
+    public function getCommittedTransactions()
     {
-        return $this->transactions;
+        return $this->committedTransactions;
     }
 }
diff --git a/src/Illuminate/Foundation/Testing/DatabaseTransactionsManager.php b/src/Illuminate/Foundation/Testing/DatabaseTransactionsManager.php
@@ -21,7 +21,7 @@ public function addCallback($callback)
             return $callback();
         }
 
-        $this->transactions->last()->addCallback($callback);
+        $this->pendingTransactions->last()->addCallback($callback);
     }
 
     /**
@@ -31,7 +31,7 @@ public function addCallback($callback)
      */
     public function callbackApplicableTransactions()
     {
-        return $this->transactions->skip(1)->values();
+        return $this->pendingTransactions->skip(1)->values();
     }
 
     /**

diff --git a/tests/Database/DatabaseTransactionsManagerTest.php b/tests/Database/DatabaseTransactionsManagerTest.php
@@ -15,13 +15,13 @@ public function testBeginningTransactions()
         $manager->begin('default', 2);
         $manager->begin('admin', 1);
 
-        $this->assertCount(3, $manager->getTransactions());
-        $this->assertSame('default', $manager->getTransactions()[0]->connection);
-        $this->assertEquals(1, $manager->getTransactions()[0]->level);
-        $this->assertSame('default', $manager->getTransactions()[1]->connection);
-        $this->assertEquals(2, $manager->getTransactions()[1]->level);
-        $this->assertSame('admin', $manager->getTransactions()[2]->connection);
-        $this->assertEquals(1, $manager->getTransactions()[2]->level);
+        $this->assertCount(3, $manager->getPendingTransactions());
+        $this->assertSame('default', $manager->getPendingTransactions()[0]->connection);
+        $this->assertEquals(1, $manager->getPendingTransactions()[0]->level);
+        $this->assertSame('default', $manager->getPendingTransactions()[1]->connection);
+        $this->assertEquals(2, $manager->getPendingTransactions()[1]->level);
+        $this->assertSame('admin', $manager->getPendingTransactions()[2]->connection);
+        $this->assertEquals(1, $manager->getPendingTransactions()[2]->level);
     }
 
     public function testRollingBackTransactions()
@@ -34,13 +34,13 @@ public function testRollingBackTransactions()
 
         $manager->rollback('default', 1);
 
-        $this->assertCount(2, $manager->getTransactions());
+        $this->assertCount(2, $manager->getPendingTransactions());
 
-        $this->assertSame('default', $manager->getTransactions()[0]->connection);
-        $this->assertEquals(1, $manager->getTransactions()[0]->level);
+        $this->assertSame('default', $manager->getPendingTransactions()[0]->connection);
+        $this->assertEquals(1, $manager->getPendingTransactions()[0]->level);
 
-        $this->assertSame('admin', $manager->getTransactions()[1]->connection);
-        $this->assertEquals(1, $manager->getTransactions()[1]->level);
+        $this->assertSame('admin', $manager->getPendingTransactions()[1]->connection);
+        $this->assertEquals(1, $manager->getPendingTransactions()[1]->level);
     }
 
     public function testRollingBackTransactionsAllTheWay()
@@ -53,10 +53,10 @@ public function testRollingBackTransactionsAllTheWay()
 
         $manager->rollback('default', 0);
 
-        $this->assertCount(1, $manager->getTransactions());
+        $this->assertCount(1, $manager->getPendingTransactions());
 
-        $this->assertSame('admin', $manager->getTransactions()[0]->connection);
-        $this->assertEquals(1, $manager->getTransactions()[0]->level);
+        $this->assertSame('admin', $manager->getPendingTransactions()[0]->connection);
+        $this->assertEquals(1, $manager->getPendingTransactions()[0]->level);
     }
 
     public function testCommittingTransactions()
@@ -67,12 +67,15 @@ public function testCommittingTransactions()
         $manager->begin('default', 2);
         $manager->begin('admin', 1);
 
+        $manager->stageTransactions('default');
+        $manager->stageTransactions('admin');
         $manager->commit('default');
 
-        $this->assertCount(1, $manager->getTransactions());
+        $this->assertCount(0, $manager->getPendingTransactions());
+        $this->assertCount(1, $manager->getCommittedTransactions());
 
-        $this->assertSame('admin', $manager->getTransactions()[0]->connection);
-        $this->assertEquals(1, $manager->getTransactions()[0]->level);
+        $this->assertSame('admin', $manager->getCommittedTransactions()[0]->connection);
+        $this->assertEquals(1, $manager->getCommittedTransactions()[0]->level);
     }
 
     public function testCallbacksAreAddedToTheCurrentTransaction()
@@ -93,9 +96,9 @@ public function testCallbacksAreAddedToTheCurrentTransaction()
         $manager->addCallback(function () use (&$callbacks) {
         });
 
-        $this->assertCount(1, $manager->getTransactions()[0]->getCallbacks());
-        $this->assertCount(0, $manager->getTransactions()[1]->getCallbacks());
-        $this->assertCount(1, $manager->getTransactions()[2]->getCallbacks());
+        $this->assertCount(1, $manager->getPendingTransactions()[0]->getCallbacks());
+        $this->assertCount(0, $manager->getPendingTransactions()[1]->getCallbacks());
+        $this->assertCount(1, $manager->getPendingTransactions()[2]->getCallbacks());
     }
 
     public function testCommittingTransactionsExecutesCallbacks()
@@ -118,6 +121,7 @@ public function testCommittingTransactionsExecutesCallbacks()
 
         $manager->begin('admin', 1);
 
+        $manager->stageTransactions('default');
         $manager->commit('default');
 
         $this->assertCount(2, $callbacks);
@@ -144,6 +148,7 @@ public function testCommittingExecutesOnlyCallbacksOfTheConnection()
             $callbacks[] = ['admin', 1];
         });
 
+        $manager->stageTransactions('default');
         $manager->commit('default');
 
         $this->assertCount(1, $callbacks);
@@ -163,4 +168,33 @@ public function testCallbackIsExecutedIfNoTransactions()
         $this->assertCount(1, $callbacks);
         $this->assertEquals(['default', 1], $callbacks[0]);
     }
+
+    public function testStageTransactions()
+    {
+        $manager = (new DatabaseTransactionsManager);
+
+        $manager->begin('default', 1);
+        $manager->begin('admin', 1);
+
+        $this->assertCount(2, $manager->getPendingTransactions());
+
+        $pendingTransactions = $manager->getPendingTransactions();
+
+        $this->assertEquals(1, $pendingTransactions[0]->level);
+        $this->assertEquals('default', $pendingTransactions[0]->connection);
+        $this->assertEquals(1, $pendingTransactions[1]->level);
+        $this->assertEquals('admin', $pendingTransactions[1]->connection);
+
+        $manager->stageTransactions('default');
+
+        $this->assertCount(1, $manager->getPendingTransactions());
+        $this->assertCount(1, $manager->getCommittedTransactions());
+        $this->assertEquals('default', $manager->getCommittedTransactions()[0]->connection);
+
+        $manager->stageTransactions('admin');
+
+        $this->assertCount(0, $manager->getPendingTransactions());
+        $this->assertCount(2, $manager->getCommittedTransactions());
+        $this->assertEquals('admin', $manager->getCommittedTransactions()[1]->connection);
+    }
 }
diff --git a/tests/Database/DatabaseTransactionsTest.php b/tests/Database/DatabaseTransactionsTest.php
@@ -64,6 +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('commit')->once()->with('default');
 
         $this->connection()->setTransactionManager($transactionManager);
@@ -83,6 +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('commit')->once()->with('default');
 
         $this->connection()->setTransactionManager($transactionManager);
@@ -103,6 +105,7 @@ 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('commit')->once()->with('default');
 
         $this->connection()->setTransactionManager($transactionManager);
@@ -130,6 +133,8 @@ 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('commit')->once()->with('default');
         $transactionManager->shouldReceive('commit')->once()->with('second_connection');