Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better transaction manager object design #49103

Merged
merged 5 commits into from
Nov 23, 2023
Merged

Better transaction manager object design #49103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
464c7fa
better object design
taylorotwell Nov 23, 2023
057c42c
Apply fixes from StyleCI
StyleCIBot Nov 23, 2023
7f728df
use method for testing
taylorotwell Nov 23, 2023
695e1d8
add or
taylorotwell Nov 23, 2023
6d47ef9
fix condition
taylorotwell Nov 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 17 additions & 23 deletions src/Illuminate/Database/Concerns/ManagesTransactions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
*
Expand Down
57 changes: 34 additions & 23 deletions src/Illuminate/Database/DatabaseTransactionsManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
);
}

/**
Expand Down
15 changes: 0 additions & 15 deletions tests/Database/DatabaseConnectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
32 changes: 20 additions & 12 deletions tests/Database/DatabaseTransactionsManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand All @@ -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]);
Expand Down
21 changes: 8 additions & 13 deletions tests/Database/DatabaseTransactionsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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);
Expand Down Expand Up @@ -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);

Expand Down
4 changes: 2 additions & 2 deletions tests/Foundation/Testing/DatabaseTransactionsManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down