migration: Prevent parallel migration runs

bin/migrate

@@ -25,20 +25,27 @@ $errorHandler->setHandler(Handler::ENV_PRODUCTION, new NullHandler());
 $migration = $app->get('db.migration');
 $migration->setOutput(function ($text) { echo $text . PHP_EOL; });
 
-if (isset($argv[1]) && in_array(strtolower($argv[1]), ['help', '--help', '-h'])) {
-    echo PHP_EOL . 'Usage: ' . $argv[0] . ' [up|down] [one-step]' . PHP_EOL . PHP_EOL;
+$script = array_shift($argv);
+$argv = array_map('strtolower', $argv);
+if (in_array('help', $argv) || in_array('--help', $argv) || in_array('-h', $argv)) {
+    echo PHP_EOL . 'Usage: ' . $script . ' [up|down] [one-step] [force|-f]' . PHP_EOL . PHP_EOL;
     exit;
 }
 
 $method = Migrate::UP;
-if (isset($argv[1]) && strtolower($argv[1]) == 'down') {
+if (in_array('down', $argv)) {
     $argv = array_values($argv);
     $method = Migrate::DOWN;
 }
 
 $oneStep = false;
-if (isset($argv[2]) && strtolower($argv[2]) == 'one-step') {
+if (in_array('one-step', $argv)) {
     $oneStep = true;
 }
 
-$migration->run($baseDir, $method, $oneStep);
+$force = false;
+if (in_array('force', $argv) || in_array('--force', $argv) || in_array('-f', $argv)) {
+    $force = true;
+}
+
+$migration->run($baseDir, $method, $oneStep, $force);

src/Database/Migration/Migrate.php

@@ -3,11 +3,13 @@
 namespace Engelsystem\Database\Migration;
 
 use Engelsystem\Application;
+use Exception;
 use Illuminate\Database\Query\Builder;
 use Illuminate\Database\Schema\Blueprint;
 use Illuminate\Database\Schema\Builder as SchemaBuilder;
 use Illuminate\Support\Collection;
 use Illuminate\Support\Str;
+use Throwable;
 
 class Migrate
 {
@@ -49,10 +51,13 @@ class Migrate
      * @param string $path
      * @param string $type (up|down)
      * @param bool   $oneStep
+     * @param bool   $forceMigration
      */
-    public function run($path, $type = self::UP, $oneStep = false)
+    public function run($path, $type = self::UP, $oneStep = false, $forceMigration = false)
     {
         $this->initMigration();
+
+        $this->lockTable($forceMigration);
         $migrations = $this->mergeMigrations(
             $this->getMigrations($path),
             $this->getMigrated()
@@ -62,6 +67,7 @@ class Migrate
             $migrations = $migrations->reverse();
         }
 
+        try {
             foreach ($migrations as $migration) {
                 /** @var array $migration */
                 $name = $migration['migration'];
@@ -82,9 +88,16 @@ class Migrate
                 $this->setMigrated($name, $type);
 
                 if ($oneStep) {
-                return;
+                    break;
+                }
             }
+        } catch (Throwable $e) {
+            $this->unlockTable();
+
+            throw $e;
         }
+
+        $this->unlockTable();
     }
 
     /**
@@ -143,6 +156,7 @@ class Migrate
     {
         return $this->getTableQuery()
             ->orderBy('id')
+            ->where('migration', '!=', 'lock')
             ->get();
     }
 
@@ -184,10 +198,45 @@ class Migrate
         $table->insert(['migration' => $migration]);
     }
 
+    /**
+     * Lock the migrations table
+     *
+     * @param bool $forceMigration
+     *
+     * @throws Throwable
+     */
+    protected function lockTable($forceMigration = false)
+    {
+        $this->schema->getConnection()->transaction(function () use ($forceMigration) {
+            $lock = $this->getTableQuery()
+                ->where('migration', 'lock')
+                ->lockForUpdate()
+                ->first();
+
+            if ($lock && !$forceMigration) {
+                throw new Exception('Unable to acquire migration table lock');
+            }
+
+            $this->getTableQuery()
+                ->insert(['migration' => 'lock']);
+        });
+    }
+
+    /**
+     * Unlock a previously locked table
+     */
+    protected function unlockTable()
+    {
+        $this->getTableQuery()
+            ->where('migration', 'lock')
+            ->delete();
+    }
+
     /**
      * Get a list of migration files
      *
      * @param string $dir
+     *
      * @return Collection
      */
     protected function getMigrations($dir)
@@ -212,6 +261,7 @@ class Migrate
      * List all migration files from the given directory
      *
      * @param string $dir
+     *
      * @return array
      */
     protected function getMigrationFiles($dir)

tests/Unit/Database/Migration/MigrateTest.php

@@ -4,12 +4,13 @@ namespace Engelsystem\Test\Unit\Database\Migration;
 
 use Engelsystem\Application;
 use Engelsystem\Database\Migration\Migrate;
+use Engelsystem\Test\Unit\TestCase;
+use Exception;
 use Illuminate\Database\Capsule\Manager as CapsuleManager;
 use Illuminate\Database\Schema\Builder as SchemaBuilder;
 use Illuminate\Support\Collection;
 use Illuminate\Support\Str;
 use PHPUnit\Framework\MockObject\MockObject;
-use PHPUnit\Framework\TestCase;
 
 class MigrateTest extends TestCase
 {
@@ -33,11 +34,18 @@ class MigrateTest extends TestCase
         /** @var Migrate|MockObject $migration */
         $migration = $this->getMockBuilder(Migrate::class)
             ->setConstructorArgs([$builder, $app])
-            ->onlyMethods(['initMigration', 'getMigrationFiles', 'getMigrated', 'migrate', 'setMigrated'])
+            ->onlyMethods([
+                'initMigration',
+                'getMigrationFiles',
+                'getMigrated',
+                'migrate',
+                'setMigrated',
+                'lockTable',
+                'unlockTable',
+            ])
             ->getMock();
 
-        $migration->expects($this->atLeastOnce())
-            ->method('initMigration');
+        $this->setExpects($migration, 'initMigration', null, null, $this->atLeastOnce());
         $migration->expects($this->atLeastOnce())
             ->method('getMigrationFiles')
             ->willReturn([
@@ -46,12 +54,10 @@ class MigrateTest extends TestCase
                 'foo/4567_11_01_000000_do_stuff.php',
                 'foo/9999_99_99_999999_another_foo.php',
             ]);
-        $migration->expects($this->atLeastOnce())
-            ->method('getMigrated')
-            ->willReturn(new Collection([
+        $this->setExpects($migration, 'getMigrated', null, new Collection([
             ['id' => 1, 'migration' => '1234_01_23_123456_init_foo'],
             ['id' => 2, 'migration' => '4567_11_01_000000_do_stuff'],
-            ]));
+        ]), $this->atLeastOnce());
         $migration->expects($this->atLeastOnce())
             ->method('migrate')
             ->withConsecutive(
@@ -72,6 +78,8 @@ class MigrateTest extends TestCase
                 ['9876_03_22_210000_random_hack', Migrate::UP],
                 ['4567_11_01_000000_do_stuff', Migrate::DOWN]
             );
+        $this->setExpects($migration, 'lockTable', null, null, $this->atLeastOnce());
+        $this->setExpects($migration, 'unlockTable', null, null, $this->atLeastOnce());
 
         $migration->run('foo', Migrate::UP);
 
@@ -111,6 +119,49 @@ class MigrateTest extends TestCase
         $migration->run('foo', Migrate::DOWN, true);
     }
 
+    /**
+     * @covers \Engelsystem\Database\Migration\Migrate::run
+     */
+    public function testRunExceptionUnlockTable()
+    {
+        /** @var Application|MockObject $app */
+        $app = $this->getMockBuilder(Application::class)
+            ->onlyMethods(['instance'])
+            ->getMock();
+        /** @var SchemaBuilder|MockObject $builder */
+        $builder = $this->getMockBuilder(SchemaBuilder::class)
+            ->disableOriginalConstructor()
+            ->getMock();
+        /** @var Migrate|MockObject $migration */
+        $migration = $this->getMockBuilder(Migrate::class)
+            ->setConstructorArgs([$builder, $app])
+            ->onlyMethods([
+                'initMigration',
+                'lockTable',
+                'getMigrations',
+                'getMigrated',
+                'migrate',
+                'unlockTable',
+            ])
+            ->getMock();
+
+        $this->setExpects($migration, 'initMigration');
+        $this->setExpects($migration, 'lockTable');
+        $this->setExpects($migration, 'unlockTable');
+        $this->setExpects($migration, 'getMigrations', null, collect([
+            ['migration' => '1234_01_23_123456_init_foo', 'path' => '/foo']
+        ]));
+        $this->setExpects($migration, 'getMigrated', null, collect([]));
+        $migration->expects($this->once())
+            ->method('migrate')
+            ->willReturnCallback(function () {
+                throw new Exception();
+            });
+
+        $this->expectException(Exception::class);
+        $migration->run('');
+    }
+
     /**
      * @covers \Engelsystem\Database\Migration\Migrate::getMigrated
      * @covers \Engelsystem\Database\Migration\Migrate::getMigrationFiles
@@ -118,6 +169,8 @@ class MigrateTest extends TestCase
      * @covers \Engelsystem\Database\Migration\Migrate::initMigration
      * @covers \Engelsystem\Database\Migration\Migrate::migrate
      * @covers \Engelsystem\Database\Migration\Migrate::setMigrated
+     * @covers \Engelsystem\Database\Migration\Migrate::lockTable
+     * @covers \Engelsystem\Database\Migration\Migrate::unlockTable
      */
     public function testRunIntegration()
     {
@@ -145,6 +198,7 @@ class MigrateTest extends TestCase
 
         $migrations = $db->table('migrations')->get();
         $this->assertCount(3, $migrations);
+        $this->assertFalse($migrations->contains('migration', 'lock'));
 
         $this->assertTrue($migrations->contains('migration', '2001_04_11_123456_create_lorem_ipsum_table'));
         $this->assertTrue($migrations->contains('migration', '2017_12_24_053300_another_stuff'));
@@ -163,5 +217,9 @@ class MigrateTest extends TestCase
         $this->assertCount(0, $migrations);
 
         $this->assertFalse($schema->hasTable('lorem_ipsum'));
+
+        $db->table('migrations')->insert(['migration' => 'lock']);
+        $this->expectException(Exception::class);
+        $migration->run(__DIR__ . '/Stub', Migrate::UP);
     }
 }