From 4995aa2a0b9430bcc8885d67cadca95cb1be09cf Mon Sep 17 00:00:00 2001 From: Igor Scheller Date: Mon, 1 Jun 2020 22:00:33 +0200 Subject: [PATCH] migration: Prevent parallel migration runs --- bin/migrate | 17 ++-- src/Database/Migration/Migrate.php | 90 ++++++++++++++----- tests/Unit/Database/Migration/MigrateTest.php | 78 +++++++++++++--- 3 files changed, 150 insertions(+), 35 deletions(-) diff --git a/bin/migrate b/bin/migrate index 3aa021d4..7a70e527 100755 --- a/bin/migrate +++ b/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); diff --git a/src/Database/Migration/Migrate.php b/src/Database/Migration/Migrate.php index a0d5d4b0..572d560b 100644 --- a/src/Database/Migration/Migrate.php +++ b/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,29 +67,37 @@ class Migrate $migrations = $migrations->reverse(); } - foreach ($migrations as $migration) { - /** @var array $migration */ - $name = $migration['migration']; + try { + foreach ($migrations as $migration) { + /** @var array $migration */ + $name = $migration['migration']; - if ( - ($type == self::UP && isset($migration['id'])) - || ($type == self::DOWN && !isset($migration['id'])) - ) { - ($this->output)('Skipping ' . $name); - continue; + if ( + ($type == self::UP && isset($migration['id'])) + || ($type == self::DOWN && !isset($migration['id'])) + ) { + ($this->output)('Skipping ' . $name); + continue; + } + + ($this->output)('Migrating ' . $name . ' (' . $type . ')'); + + if (isset($migration['path'])) { + $this->migrate($migration['path'], $name, $type); + } + $this->setMigrated($name, $type); + + if ($oneStep) { + break; + } } + } catch (Throwable $e) { + $this->unlockTable(); - ($this->output)('Migrating ' . $name . ' (' . $type . ')'); - - if (isset($migration['path'])) { - $this->migrate($migration['path'], $name, $type); - } - $this->setMigrated($name, $type); - - if ($oneStep) { - return; - } + 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) diff --git a/tests/Unit/Database/Migration/MigrateTest.php b/tests/Unit/Database/Migration/MigrateTest.php index 345a4999..2ef8927d 100644 --- a/tests/Unit/Database/Migration/MigrateTest.php +++ b/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([ - ['id' => 1, 'migration' => '1234_01_23_123456_init_foo'], - ['id' => 2, 'migration' => '4567_11_01_000000_do_stuff'], - ])); + $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); } }