Better transaction manager object design (#49103)

* better object design * Apply fixes from StyleCI * use method for testing * add or * fix condition --------- Co-authored-by: StyleCI Bot <bot@styleci.io>
laravel · Nov 23, 2023 · 0b475bb · 0b475bb
1 parent 99d21f3
commit 0b475bb
Show file tree

Hide file tree

Showing 6 changed files with 81 additions and 88 deletions.
diff --git a/src/Illuminate/Database/Concerns/ManagesTransactions.php b/src/Illuminate/Database/Concerns/ManagesTransactions.php
@@ -47,13 +47,16 @@ public function transaction(Closure $callback, $attempts = 1)
                     $this->getPdo()->commit();
                 }
 
-                $this->transactionsManager?->stageTransactions($this->getName(), $this->transactions);
-
-                $this->transactions = max(0, $this->transactions - 1);
-
-                if ($this->afterCommitCallbacksShouldBeExecuted()) {
-                    $this->transactionsManager?->commit($this->getName());
-                }
+                [$levelBeingCommitted, $this->transactions] = [
+                    $this->transactions,
+                    max(0, $this->transactions - 1),
+                ];
+
+                $this->transactionsManager?->commit(
+                    $this->getName(),
+                    $levelBeingCommitted,
+                    $this->transactions
+                );
             } catch (Throwable $e) {
                 $this->handleCommitTransactionException(
                     $e, $currentAttempt, $attempts
@@ -196,27 +199,18 @@ public function commit()
             $this->getPdo()->commit();
         }
 
-        $this->transactionsManager?->stageTransactions($this->getName(), $this->transactions);
+        [$levelBeingCommitted, $this->transactions] = [
+            $this->transactions,
+            max(0, $this->transactions - 1),
+        ];
 
-        $this->transactions = max(0, $this->transactions - 1);
-
-        if ($this->afterCommitCallbacksShouldBeExecuted()) {
-            $this->transactionsManager?->commit($this->getName());
-        }
+        $this->transactionsManager?->commit(
+            $this->getName(), $levelBeingCommitted, $this->transactions
+        );
 
         $this->fireConnectionEvent('committed');
     }
 
-    /**
-     * Determine if after commit callbacks should be executed.
-     *
-     * @return bool
-     */
-    protected function afterCommitCallbacksShouldBeExecuted()
-    {
-        return $this->transactionsManager?->afterCommitCallbacksShouldBeExecuted($this->transactions) || $this->transactions == 0;
-    }
-
     /**
      * Handle an exception encountered when committing a transaction.
      *

diff --git a/src/Illuminate/Database/DatabaseTransactionsManager.php b/src/Illuminate/Database/DatabaseTransactionsManager.php
@@ -59,39 +59,26 @@ public function begin($connection, $level)
     }
 
     /**
-     * Move relevant pending transactions to a committed state.
+     * Commit the root database transaction and execute callbacks.
      *
      * @param  string  $connection
      * @param  int  $levelBeingCommitted
-     * @return void
+     * @param  int  $newTransactionLevel
+     * @return array
      */
-    public function stageTransactions($connection, $levelBeingCommitted)
+    public function commit($connection, $levelBeingCommitted, $newTransactionLevel)
     {
-        $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 >= $levelBeingCommitted
-        );
+        $this->stageTransactions($connection, $levelBeingCommitted);
 
         if (isset($this->currentTransaction[$connection])) {
             $this->currentTransaction[$connection] = $this->currentTransaction[$connection]->parent;
         }
-    }
 
-    /**
-     * Commit the root database transaction and execute callbacks.
-     *
-     * @param  string  $connection
-     * @return void
-     */
-    public function commit($connection)
-    {
+        if (! $this->afterCommitCallbacksShouldBeExecuted($newTransactionLevel) &&
+            $newTransactionLevel !== 0) {
+            return [];
+        }
+
         // 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.
@@ -106,6 +93,30 @@ public function commit($connection)
         $this->committedTransactions = $forOtherConnections->values();
 
         $forThisConnection->map->executeCallbacks();
+
+        return $forThisConnection;
+    }
+
+    /**
+     * Move relevant pending transactions to a committed state.
+     *
+     * @param  string  $connection
+     * @param  int  $levelBeingCommitted
+     * @return void
+     */
+    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 >= $levelBeingCommitted
+        );
     }
 
     /**

diff --git a/tests/Database/DatabaseConnectionTest.php b/tests/Database/DatabaseConnectionTest.php
@@ -7,7 +7,6 @@
 use Exception;
 use Illuminate\Contracts\Events\Dispatcher;
 use Illuminate\Database\Connection;
-use Illuminate\Database\DatabaseTransactionsManager;
 use Illuminate\Database\Events\QueryExecuted;
 use Illuminate\Database\Events\TransactionBeginning;
 use Illuminate\Database\Events\TransactionCommitted;
@@ -290,20 +289,6 @@ public function testCommittingFiresEventsIfSet()
         $connection->commit();
     }
 
-    public function testAfterCommitIsExecutedOnFinalCommit()
-    {
-        $pdo = $this->getMockBuilder(DatabaseConnectionTestMockPDO::class)->onlyMethods(['beginTransaction', 'commit'])->getMock();
-        $transactionsManager = $this->getMockBuilder(DatabaseTransactionsManager::class)->onlyMethods(['afterCommitCallbacksShouldBeExecuted'])->getMock();
-        $transactionsManager->expects($this->once())->method('afterCommitCallbacksShouldBeExecuted')->with(0)->willReturn(true);
-
-        $connection = $this->getMockConnection([], $pdo);
-        $connection->setTransactionManager($transactionsManager);
-
-        $connection->transaction(function () {
-            // do nothing
-        });
-    }
-
     public function testRollBackedFiresEventsIfSet()
     {
         $pdo = $this->createMock(DatabaseConnectionTestMockPDO::class);

diff --git a/tests/Database/DatabaseTransactionsManagerTest.php b/tests/Database/DatabaseTransactionsManagerTest.php
@@ -66,16 +66,24 @@ public function testCommittingTransactions()
         $manager->begin('default', 1);
         $manager->begin('default', 2);
         $manager->begin('admin', 1);
+        $manager->begin('admin', 2);
 
-        $manager->stageTransactions('default', 1);
-        $manager->stageTransactions('admin', 1);
-        $manager->commit('default');
+        $manager->commit('default', 2, 1);
+        $executedTransactions = $manager->commit('default', 1, 0);
 
-        $this->assertCount(0, $manager->getPendingTransactions());
-        $this->assertCount(1, $manager->getCommittedTransactions());
+        $executedAdminTransactions = $manager->commit('admin', 2, 1);
+
+        $this->assertCount(1, $manager->getPendingTransactions()); // One pending "admin" transaction left...
+        $this->assertCount(2, $executedTransactions); // Two committed tranasctions on "default"
+        $this->assertCount(0, $executedAdminTransactions); // Zero executed committed tranasctions on "default"
 
+        // Level 2 "admin" callback has been staged...
         $this->assertSame('admin', $manager->getCommittedTransactions()[0]->connection);
-        $this->assertEquals(1, $manager->getCommittedTransactions()[0]->level);
+        $this->assertEquals(2, $manager->getCommittedTransactions()[0]->level);
+
+        // Level 1 "admin" callback still pending...
+        $this->assertSame('admin', $manager->getPendingTransactions()[0]->connection);
+        $this->assertEquals(1, $manager->getPendingTransactions()[0]->level);
     }
 
     public function testCallbacksAreAddedToTheCurrentTransaction()
@@ -121,12 +129,12 @@ public function testCommittingTransactionsExecutesCallbacks()
 
         $manager->begin('admin', 1);
 
-        $manager->stageTransactions('default', 1);
-        $manager->commit('default');
+        $manager->commit('default', 2, 1);
+        $manager->commit('default', 1, 0);
 
         $this->assertCount(2, $callbacks);
-        $this->assertEquals(['default', 1], $callbacks[0]);
-        $this->assertEquals(['default', 2], $callbacks[1]);
+        $this->assertEquals(['default', 2], $callbacks[0]);
+        $this->assertEquals(['default', 1], $callbacks[1]);
     }
 
     public function testCommittingExecutesOnlyCallbacksOfTheConnection()
@@ -148,8 +156,8 @@ public function testCommittingExecutesOnlyCallbacksOfTheConnection()
             $callbacks[] = ['admin', 1];
         });
 
-        $manager->stageTransactions('default', 1);
-        $manager->commit('default');
+        $manager->commit('default', 2, 1);
+        $manager->commit('default', 1, 0);
 
         $this->assertCount(1, $callbacks);
         $this->assertEquals(['default', 1], $callbacks[0]);

diff --git a/tests/Database/DatabaseTransactionsTest.php b/tests/Database/DatabaseTransactionsTest.php
@@ -64,8 +64,7 @@ public function testTransactionIsRecordedAndCommitted()
     {
         $transactionManager = m::mock(new DatabaseTransactionsManager);
         $transactionManager->shouldReceive('begin')->once()->with('default', 1);
-        $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 1);
-        $transactionManager->shouldReceive('commit')->once()->with('default');
+        $transactionManager->shouldReceive('commit')->once()->with('default', 1, 0);
 
         $this->connection()->setTransactionManager($transactionManager);
 
@@ -84,8 +83,7 @@ public function testTransactionIsRecordedAndCommittedUsingTheSeparateMethods()
     {
         $transactionManager = m::mock(new DatabaseTransactionsManager);
         $transactionManager->shouldReceive('begin')->once()->with('default', 1);
-        $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 1);
-        $transactionManager->shouldReceive('commit')->once()->with('default');
+        $transactionManager->shouldReceive('commit')->once()->with('default', 1, 0);
 
         $this->connection()->setTransactionManager($transactionManager);
 
@@ -105,9 +103,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')->once()->with('default', 1);
-        $transactionManager->shouldReceive('stageTransactions')->once()->with('default', 2);
-        $transactionManager->shouldReceive('commit')->once()->with('default');
+        $transactionManager->shouldReceive('commit')->once()->with('default', 2, 1);
+        $transactionManager->shouldReceive('commit')->once()->with('default', 1, 0);
 
         $this->connection()->setTransactionManager($transactionManager);
 
@@ -134,11 +131,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', 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');
+        $transactionManager->shouldReceive('commit')->once()->with('default', 1, 0);
+        $transactionManager->shouldReceive('commit')->once()->with('second_connection', 2, 1);
+        $transactionManager->shouldReceive('commit')->once()->with('second_connection', 1, 0);
 
         $this->connection()->setTransactionManager($transactionManager);
         $this->connection('second_connection')->setTransactionManager($transactionManager);
@@ -196,7 +191,7 @@ public function testTransactionIsRolledBackUsingSeparateMethods()
         $transactionManager = m::mock(new DatabaseTransactionsManager);
         $transactionManager->shouldReceive('begin')->once()->with('default', 1);
         $transactionManager->shouldReceive('rollback')->once()->with('default', 0);
-        $transactionManager->shouldNotReceive('commit');
+        $transactionManager->shouldNotReceive('commit', 1, 0);
 
         $this->connection()->setTransactionManager($transactionManager);
 

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