From 3e4f429eb1e7809b57929a03342c566d693c71ef Mon Sep 17 00:00:00 2001 From: Louis Charette Date: Sun, 19 Nov 2023 22:01:38 -0500 Subject: [PATCH] Fix race condition with Connection update --- app/src/Bakery/BakeCommand.php | 7 +- app/src/Bakery/Helper/DatabaseTest.php | 13 ++- app/src/Bakery/SetupDbCommand.php | 2 - app/src/Database/Migrator/Migrator.php | 21 +++-- .../Integration/Bakery/DebugCommandTest.php | 8 +- .../DatabaseMigrationRepositoryTest.php | 2 +- .../Migrator/MigratorDependencyTest.php | 19 ++++- .../Migrator/MigratorRollbackTest.php | 49 ++++++++++-- .../Unit/Database/Migrator/MigratorTest.php | 80 ++++++++++++++++--- 9 files changed, 164 insertions(+), 37 deletions(-) diff --git a/app/src/Bakery/BakeCommand.php b/app/src/Bakery/BakeCommand.php index a692159f..0541a501 100644 --- a/app/src/Bakery/BakeCommand.php +++ b/app/src/Bakery/BakeCommand.php @@ -84,7 +84,12 @@ protected function execute(InputInterface $input, OutputInterface $output): int $application = $this->getApplication(); foreach ($this->aggregateCommands() as $commandName) { $command = $application->find($commandName); - $command->run($input, $output); + $result = $command->run($input, $output); + + // If the previous command fails, stop the process + if ($result === self::FAILURE) { + return $result; + } } return self::SUCCESS; diff --git a/app/src/Bakery/Helper/DatabaseTest.php b/app/src/Bakery/Helper/DatabaseTest.php index bf24e55a..8aee9bc8 100644 --- a/app/src/Bakery/Helper/DatabaseTest.php +++ b/app/src/Bakery/Helper/DatabaseTest.php @@ -13,7 +13,7 @@ namespace UserFrosting\Sprinkle\Core\Bakery\Helper; use DI\Attribute\Inject; -use Illuminate\Database\Connection; +use Illuminate\Database\Capsule\Manager as Capsule; use PDOException; /** @@ -21,12 +21,12 @@ * * N.B.: Make use of the Database Capsule Manager and not the Connection alias * service, as connection might not be up to date if another command (setup:db) - * has changed the db config + * has changed the db config, even if the service is set on the container. */ trait DatabaseTest { #[Inject] - protected Connection $connection; + protected Capsule $capsule; /** * Test database connection directly using PDO. @@ -37,7 +37,12 @@ trait DatabaseTest protected function testDB(): bool { try { - $this->connection->getPdo(); + $connectionName = $this->capsule->getDatabaseManager()->getDefaultConnection(); + $connection = $this->capsule->getConnection($connectionName); + $pdo = $connection->getPdo(); + if ($pdo === null) { + throw new PDOException('PDO not found'); + } } catch (PDOException $e) { $message = 'Could not connect to the database connection' . PHP_EOL; $message .= 'Exception: ' . $e->getMessage() . PHP_EOL . PHP_EOL; diff --git a/app/src/Bakery/SetupDbCommand.php b/app/src/Bakery/SetupDbCommand.php index 52d7d4dd..29fb5f95 100644 --- a/app/src/Bakery/SetupDbCommand.php +++ b/app/src/Bakery/SetupDbCommand.php @@ -43,8 +43,6 @@ class SetupDbCommand extends Command /** * Inject services. - * - * @param \DI\Container $ci */ public function __construct( protected ResourceLocatorInterface $locator, diff --git a/app/src/Database/Migrator/Migrator.php b/app/src/Database/Migrator/Migrator.php index 8188f2b9..e5f457e1 100644 --- a/app/src/Database/Migrator/Migrator.php +++ b/app/src/Database/Migrator/Migrator.php @@ -12,6 +12,7 @@ namespace UserFrosting\Sprinkle\Core\Database\Migrator; +use Illuminate\Database\Capsule\Manager as Capsule; use Illuminate\Database\Connection; use UserFrosting\Sprinkle\Core\Exceptions\MigrationDependencyNotMetException; use UserFrosting\Sprinkle\Core\Exceptions\MigrationRollbackException; @@ -22,14 +23,14 @@ class Migrator { /** - * @param MigrationRepositoryInterface $repository The migration repository - * @param MigrationLocatorInterface $locator The Migration locator - * @param Connection $dbConnection The database connection + * @param MigrationRepositoryInterface $repository The migration repository + * @param MigrationLocatorInterface $locator The Migration locator + * @param Capsule $db The database */ public function __construct( protected MigrationRepositoryInterface $repository, protected MigrationLocatorInterface $locator, - protected Connection $dbConnection, + protected Capsule $db, ) { } @@ -630,6 +631,16 @@ public function setLocator(MigrationLocatorInterface $locator): static return $this; } + /** + * Return the database connection name. + * + * @return string|null The connection name (default: null, aka the default connection) + */ + public function getConnectionName(): ?string + { + return $this->db->getDatabaseManager()->getDefaultConnection(); + } + /** * Return the database connection instance. * @@ -637,7 +648,7 @@ public function setLocator(MigrationLocatorInterface $locator): static */ protected function getConnection(): Connection { - return $this->dbConnection; + return $this->db->getConnection($this->getConnectionName()); } /** diff --git a/app/tests/Integration/Bakery/DebugCommandTest.php b/app/tests/Integration/Bakery/DebugCommandTest.php index 3f0cd8ba..eca8ac0c 100644 --- a/app/tests/Integration/Bakery/DebugCommandTest.php +++ b/app/tests/Integration/Bakery/DebugCommandTest.php @@ -12,7 +12,7 @@ namespace UserFrosting\Sprinkle\Core\Tests\Integration\Bakery; -use Illuminate\Database\Connection; +use Illuminate\Database\Capsule\Manager as Capsule; use Mockery; use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; use PDOException; @@ -43,10 +43,10 @@ public function testCommand(): void public function testCommandForFailDatabase(): void { // Setup mocks - $class = Mockery::mock(Connection::class) - ->shouldReceive('getPdo')->andThrow(PDOException::class) + $class = Mockery::mock(Capsule::class) + ->shouldReceive('getDatabaseManager')->andThrow(PDOException::class) ->getMock(); - $this->ci->set(Connection::class, $class); + $this->ci->set(Capsule::class, $class); $result = $this->getCommandTester(); diff --git a/app/tests/Integration/Database/Migrator/DatabaseMigrationRepositoryTest.php b/app/tests/Integration/Database/Migrator/DatabaseMigrationRepositoryTest.php index 4c3df5e5..6be622a1 100644 --- a/app/tests/Integration/Database/Migrator/DatabaseMigrationRepositoryTest.php +++ b/app/tests/Integration/Database/Migrator/DatabaseMigrationRepositoryTest.php @@ -43,7 +43,7 @@ public function testRepositoryCreation(): void // Create table $repository->create(); - + // Table should exist $this->assertTrue($builder->hasTable('migrationTest')); $this->assertTrue($repository->exists()); diff --git a/app/tests/Unit/Database/Migrator/MigratorDependencyTest.php b/app/tests/Unit/Database/Migrator/MigratorDependencyTest.php index 3d6c25c2..aaf1840c 100644 --- a/app/tests/Unit/Database/Migrator/MigratorDependencyTest.php +++ b/app/tests/Unit/Database/Migrator/MigratorDependencyTest.php @@ -12,6 +12,7 @@ namespace UserFrosting\Sprinkle\Core\Tests\Unit\Database\Migrator; +use Illuminate\Database\Capsule\Manager as Capsule; use Illuminate\Database\Connection; use Mockery; use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; @@ -58,7 +59,11 @@ public function testConstruct(): Migrator ->getMock(); $connection = Mockery::mock(Connection::class); - $analyser = new Migrator($installed, $available, $connection); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->getMock(); + + $analyser = new Migrator($installed, $available, $database); $this->assertInstanceOf(Migrator::class, $analyser); @@ -156,7 +161,11 @@ public function testGetPendingThirdStageDependency(): void ->getMock(); $connection = Mockery::mock(Connection::class); - $analyser = new Migrator($installed, $available, $connection); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->getMock(); + + $analyser = new Migrator($installed, $available, $database); $this->assertSame([ StubAnalyserMigrationC::class, // C is before B because B depend on C @@ -184,7 +193,11 @@ public function testGetPendingWithNonAvailable(): void ->getMock(); $connection = Mockery::mock(Connection::class); - $analyser = new Migrator($installed, $available, $connection); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->getMock(); + + $analyser = new Migrator($installed, $available, $database); $this->expectException(MigrationDependencyNotMetException::class); $this->expectExceptionMessage(StubAnalyserMigrationG::class . ' depends on ' . StubAnalyserMigrationF::class . ", but it's not available."); diff --git a/app/tests/Unit/Database/Migrator/MigratorRollbackTest.php b/app/tests/Unit/Database/Migrator/MigratorRollbackTest.php index e1f08f82..577166af 100644 --- a/app/tests/Unit/Database/Migrator/MigratorRollbackTest.php +++ b/app/tests/Unit/Database/Migrator/MigratorRollbackTest.php @@ -12,6 +12,7 @@ namespace UserFrosting\Sprinkle\Core\Tests\Unit\Database\Migrator; +use Illuminate\Database\Capsule\Manager as Capsule; use Illuminate\Database\Connection; use Mockery; use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; @@ -49,7 +50,11 @@ public function testGetMigrationsForRollback(): void ->getMock(); $connection = Mockery::mock(Connection::class); - $analyser = new Migrator($installed, $available, $connection); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->getMock(); + + $analyser = new Migrator($installed, $available, $database); $result = $analyser->getMigrationsForRollback(1); $this->assertSame([StubAnalyserRollbackMigrationD::class], $result); @@ -74,7 +79,11 @@ public function testGetMigrationsForReset(): void ->getMock(); $connection = Mockery::mock(Connection::class); - $analyser = new Migrator($installed, $available, $connection); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->getMock(); + + $analyser = new Migrator($installed, $available, $database); $result = $analyser->getMigrationsForReset(); $this->assertSame([StubAnalyserRollbackMigrationD::class], $result); @@ -108,7 +117,11 @@ public function testValidateRollbackMigration(): void ->getMock(); $connection = Mockery::mock(Connection::class); - $analyser = new Migrator($installed, $available, $connection); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->getMock(); + + $analyser = new Migrator($installed, $available, $database); // Run command $this->assertNull($analyser->validateRollbackMigration(StubAnalyserRollbackMigrationD::class)); @@ -127,7 +140,11 @@ public function testValidateRollbackMigrationForNotInstalledException(): void ->getMock(); $available = Mockery::mock(MigrationLocatorInterface::class); $connection = Mockery::mock(Connection::class); - $analyser = new Migrator($installed, $available, $connection); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->getMock(); + + $analyser = new Migrator($installed, $available, $database); // Start by testing false $this->assertFalse($analyser->canRollbackMigration(StubAnalyserRollbackMigrationD::class)); @@ -163,7 +180,11 @@ public function testValidateRollbackMigrationForStaleException(): void ]) ->getMock(); $connection = Mockery::mock(Connection::class); - $analyser = new Migrator($installed, $available, $connection); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->getMock(); + + $analyser = new Migrator($installed, $available, $database); // Start by testing false $this->assertFalse($analyser->canRollbackMigration(StubAnalyserRollbackMigrationD::class)); @@ -201,7 +222,11 @@ public function testValidateRollbackMigrationForDependenciesNotMet(): void ->shouldReceive('has')->with(StubAnalyserRollbackMigrationC::class)->twice()->andReturn(true) ->getMock(); $connection = Mockery::mock(Connection::class); - $analyser = new Migrator($installed, $available, $connection); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->getMock(); + + $analyser = new Migrator($installed, $available, $database); // Run command $this->assertFalse($analyser->canRollbackMigration(StubAnalyserRollbackMigrationC::class)); @@ -239,7 +264,11 @@ public function testValidateRollbackMigrationForDependenciesDoesntExist(): void ->shouldReceive('has')->with(StubAnalyserRollbackMigrationC::class)->twice()->andReturn(false) ->getMock(); $connection = Mockery::mock(Connection::class); - $analyser = new Migrator($installed, $available, $connection); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->getMock(); + + $analyser = new Migrator($installed, $available, $database); // Run command $this->assertFalse($analyser->canRollbackMigration(StubAnalyserRollbackMigrationA::class)); @@ -274,7 +303,11 @@ public function testValidateRollbackMigrationForDependenciesDoesntExistWithDirec ->shouldReceive('has')->with(StubAnalyserRollbackMigrationC::class)->twice()->andReturn(false) ->getMock(); $connection = Mockery::mock(Connection::class); - $analyser = new Migrator($installed, $available, $connection); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->getMock(); + + $analyser = new Migrator($installed, $available, $database); // Run command $this->assertFalse($analyser->canRollbackMigration(StubAnalyserRollbackMigrationB::class)); diff --git a/app/tests/Unit/Database/Migrator/MigratorTest.php b/app/tests/Unit/Database/Migrator/MigratorTest.php index 0b7f76ba..9d40d9b7 100644 --- a/app/tests/Unit/Database/Migrator/MigratorTest.php +++ b/app/tests/Unit/Database/Migrator/MigratorTest.php @@ -12,7 +12,9 @@ namespace UserFrosting\Sprinkle\Core\Tests\Unit\Database\Migrator; +use Illuminate\Database\Capsule\Manager as Capsule; use Illuminate\Database\Connection; +use Illuminate\Database\DatabaseManager; use Illuminate\Database\Schema\Grammars\Grammar; use Mockery; use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration; @@ -37,6 +39,7 @@ class MigratorTest extends TestCase protected MigrationRepositoryInterface $repository; protected MigrationLocatorInterface $locator; protected Connection $connection; + protected Capsule $database; /** * Setup base mock and migrator instance. @@ -48,13 +51,16 @@ public function setUp(): void // Create mock objects $this->connection = Mockery::mock(Connection::class); + $this->database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($this->connection) + ->getMock(); $this->repository = Mockery::mock(MigrationRepositoryInterface::class); $this->locator = Mockery::mock(MigrationLocatorInterface::class); } protected function getMigrator(): Migrator { - return new Migrator($this->repository, $this->locator, $this->connection); + return new Migrator($this->repository, $this->locator, $this->database); } public function testConstructor(): Migrator @@ -149,9 +155,16 @@ public function testMigrate(Migrator $migrator): void $connection = Mockery::mock(Connection::class) ->shouldReceive('getSchemaGrammar')->times(2)->andReturn($grammar) ->getMock(); + $manager = Mockery::mock(DatabaseManager::class) + ->shouldReceive('getDefaultConnection')->andReturn(null) + ->getMock(); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->shouldReceive('getDatabaseManager')->andReturn($manager) + ->getMock(); // Create partial mock of migrator, so we can spoof "getPending" - $migrator = Mockery::mock(Migrator::class, [$repository, $locator, $connection])->makePartial(); + $migrator = Mockery::mock(Migrator::class, [$repository, $locator, $database])->makePartial(); $migrator->shouldReceive('getPending')->once()->andReturn([$migration1::class, $migration2::class]); // Migrate (Step = false) @@ -196,9 +209,16 @@ public function testMigrateWithStepsAndTransaction(Migrator $migrator): void ->shouldReceive('getSchemaGrammar')->times(2)->andReturn($grammar) ->shouldReceive('transaction')->times(2) ->getMock(); + $manager = Mockery::mock(DatabaseManager::class) + ->shouldReceive('getDefaultConnection')->andReturn(null) + ->getMock(); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->shouldReceive('getDatabaseManager')->andReturn($manager) + ->getMock(); // Create partial mock of migrator, so we can spoof "getPending" - $migrator = Mockery::mock(Migrator::class, [$repository, $locator, $connection])->makePartial(); + $migrator = Mockery::mock(Migrator::class, [$repository, $locator, $database])->makePartial(); $migrator->shouldReceive('getPending')->once()->andReturn([$migration1::class, $migration2::class]); // Migrate (Step = true) @@ -245,9 +265,16 @@ public function testPretendToMigrate(): void $connection = Mockery::mock(Connection::class) ->shouldReceive('pretend')->once()->andReturn($queries) ->getMock(); + $manager = Mockery::mock(DatabaseManager::class) + ->shouldReceive('getDefaultConnection')->andReturn(null) + ->getMock(); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->shouldReceive('getDatabaseManager')->andReturn($manager) + ->getMock(); // Create partial mock of migrator, so we can spoof "getPending" - $migrator = Mockery::mock(Migrator::class, [$this->repository, $locator, $connection])->makePartial(); + $migrator = Mockery::mock(Migrator::class, [$this->repository, $locator, $database])->makePartial(); $migrator->shouldReceive('getPending')->once()->andReturn([$migration::class]); // Pretend to migrate @@ -311,9 +338,16 @@ public function testRollback(Migrator $migrator): void $connection = Mockery::mock(Connection::class) ->shouldReceive('getSchemaGrammar')->times(2)->andReturn($grammar) ->getMock(); + $manager = Mockery::mock(DatabaseManager::class) + ->shouldReceive('getDefaultConnection')->andReturn(null) + ->getMock(); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->shouldReceive('getDatabaseManager')->andReturn($manager) + ->getMock(); // Create partial mock of migrator, so we can spoof "getPending" - $migrator = Mockery::mock(Migrator::class, [$repository, $locator, $connection])->makePartial(); + $migrator = Mockery::mock(Migrator::class, [$repository, $locator, $database])->makePartial(); $migrator->shouldReceive('getMigrationsForRollback')->with(1)->once()->andReturn([$migration1::class, $migration2::class]); // Rollback (steps = 1; default) @@ -357,9 +391,16 @@ public function testRollbackWithStepsAndTransaction(Migrator $migrator): void ->shouldReceive('getSchemaGrammar')->times(2)->andReturn($grammar) ->shouldReceive('transaction')->times(2) ->getMock(); + $manager = Mockery::mock(DatabaseManager::class) + ->shouldReceive('getDefaultConnection')->andReturn(null) + ->getMock(); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->shouldReceive('getDatabaseManager')->andReturn($manager) + ->getMock(); // Create partial mock of migrator, so we can spoof "getPending" - $migrator = Mockery::mock(Migrator::class, [$repository, $locator, $connection])->makePartial(); + $migrator = Mockery::mock(Migrator::class, [$repository, $locator, $database])->makePartial(); $migrator->shouldReceive('getMigrationsForRollback')->with(2)->once()->andReturn([$migration1::class, $migration2::class]); // Migrate (Step = true) @@ -406,9 +447,16 @@ public function testPretendToRollback(): void $connection = Mockery::mock(Connection::class) ->shouldReceive('pretend')->once()->andReturn($queries) ->getMock(); + $manager = Mockery::mock(DatabaseManager::class) + ->shouldReceive('getDefaultConnection')->andReturn(null) + ->getMock(); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->shouldReceive('getDatabaseManager')->andReturn($manager) + ->getMock(); // Create partial mock of migrator, so we can spoof "getPending" - $migrator = Mockery::mock(Migrator::class, [$this->repository, $locator, $connection])->makePartial(); + $migrator = Mockery::mock(Migrator::class, [$this->repository, $locator, $database])->makePartial(); $migrator->shouldReceive('getMigrationsForRollback')->once()->andReturn([$migration::class]); // Pretend to migrate @@ -472,9 +520,16 @@ public function testReset(Migrator $migrator): void $connection = Mockery::mock(Connection::class) ->shouldReceive('getSchemaGrammar')->times(2)->andReturn($grammar) ->getMock(); + $manager = Mockery::mock(DatabaseManager::class) + ->shouldReceive('getDefaultConnection')->andReturn(null) + ->getMock(); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->shouldReceive('getDatabaseManager')->andReturn($manager) + ->getMock(); // Create partial mock of migrator, so we can spoof "getPending" - $migrator = Mockery::mock(Migrator::class, [$repository, $locator, $connection])->makePartial(); + $migrator = Mockery::mock(Migrator::class, [$repository, $locator, $database])->makePartial(); $migrator->shouldReceive('getMigrationsForReset')->once()->andReturn([$migration1::class, $migration2::class]); // Reset @@ -521,9 +576,16 @@ public function testPretendToReset(): void $connection = Mockery::mock(Connection::class) ->shouldReceive('pretend')->once()->andReturn($queries) ->getMock(); + $manager = Mockery::mock(DatabaseManager::class) + ->shouldReceive('getDefaultConnection')->andReturn(null) + ->getMock(); + $database = Mockery::mock(Capsule::class) + ->shouldReceive('getConnection')->with(null)->andReturn($connection) + ->shouldReceive('getDatabaseManager')->andReturn($manager) + ->getMock(); // Create partial mock of migrator, so we can spoof "getPending" - $migrator = Mockery::mock(Migrator::class, [$this->repository, $locator, $connection])->makePartial(); + $migrator = Mockery::mock(Migrator::class, [$this->repository, $locator, $database])->makePartial(); $migrator->shouldReceive('getMigrationsForReset')->once()->andReturn([$migration::class]); // Pretend to migrate