Fix race condition with Connection update

userfrosting · Nov 20, 2023 · 3e4f429 · 3e4f429
1 parent 24ea4e9
commit 3e4f429
Show file tree

Hide file tree

Showing 9 changed files with 164 additions and 37 deletions.
diff --git 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
@@ -13,20 +13,20 @@
 namespace UserFrosting\Sprinkle\Core\Bakery\Helper;
 
 use DI\Attribute\Inject;
-use Illuminate\Database\Connection;
+use Illuminate\Database\Capsule\Manager as Capsule;
 use PDOException;
 
 /**
  * Database Test Trait. Include method to test the db connection.
  *
  * 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
@@ -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
@@ -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,14 +631,24 @@ 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.
      *
      * @return Connection
      */
     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
@@ -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
@@ -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
@@ -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
@@ -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));