From 2b75aaf8e5e8cb86e772e87b247d8f9a92e28a46 Mon Sep 17 00:00:00 2001 From: Joao Gilberto Magalhaes Date: Sat, 15 Jul 2023 15:06:26 -0500 Subject: [PATCH] Address issue #49 --- README.md | 38 +++++----------- src/Migration.php | 31 ++++++++++--- tests/MigrationTest.php | 44 ++++++++++++++++--- tests/dirstructure/migrations/up/00001.sql | 2 - tests/dirstructure/migrations/up/00002.sql | 2 - tests/dirstructure/migrations/up/00003.sql | 1 - tests/dirstructure2/base.sql | 0 tests/dirstructure2/migrations/down/0000.sql | 0 tests/dirstructure2/migrations/down/00001.sql | 0 tests/dirstructure2/migrations/down/00002.sql | 0 tests/dirstructure2/migrations/up/00001.sql | 0 tests/dirstructure2/migrations/up/00002.sql | 1 + tests/dirstructure2/migrations/up/00003.sql | 0 13 files changed, 75 insertions(+), 44 deletions(-) create mode 100644 tests/dirstructure2/base.sql create mode 100644 tests/dirstructure2/migrations/down/0000.sql create mode 100644 tests/dirstructure2/migrations/down/00001.sql create mode 100644 tests/dirstructure2/migrations/down/00002.sql create mode 100644 tests/dirstructure2/migrations/up/00001.sql create mode 100644 tests/dirstructure2/migrations/up/00002.sql create mode 100644 tests/dirstructure2/migrations/up/00003.sql diff --git a/README.md b/README.md index d79f7e4..74c986b 100644 --- a/README.md +++ b/README.md @@ -205,39 +205,23 @@ $migration->getDbDriver(); To use it, please visit: -## Tips on writing SQL migrations for Postgres +### Avoiding Partial Migration -### Rely on explicit transactions +A partial migration is when the migration script is interrupted in the middle of the process due to an error or a manual interruption. -```sql --- DO -BEGIN; +The migration table will be with the status `partial up` or `partial down` and it needs to be fixed manually before be able to migrate again. -ALTER TABLE 1; -UPDATE 1; -UPDATE 2; -UPDATE 3; -ALTER TABLE 2; +To avoid this situation you can specify the migration will be run in a transactional context. +If the migration script fails, the transaction will be rolled back and the migration table will be marked as `complete` and +the version will be the immediately previous version before the script that causes the error. -COMMIT; +To enable this feature you need to call the method `withTransactionEnabled` passing `true` as parameter: - --- DON'T -ALTER TABLE 1; -UPDATE 1; -UPDATE 2; -UPDATE 3; -ALTER TABLE 2; +```php +withTransactionEnabled(true); ``` - -It is generally desirable to wrap migration scripts inside a `BEGIN; ... COMMIT;` block. -This way, if _any_ of the inner statements fail, _none_ of them are committed and the -database does not end up in an inconsistent state. - -Mind that in case of a failure `byjg/migration` will always mark the migration as `partial` -and warn you when you attempt to run it again. The difference is that with explicit -transactions you know that the database cannot be in an inconsistent state after an -unexpected failure. +## Tips on writing SQL migrations for Postgres ### On creating triggers and SQL functions diff --git a/src/Migration.php b/src/Migration.php index 8c87af1..b19a242 100644 --- a/src/Migration.php +++ b/src/Migration.php @@ -50,6 +50,8 @@ class Migration */ private $migrationTable; + private $transaction = false; + /** * Migration constructor. * @@ -69,10 +71,14 @@ public function __construct(UriInterface $uri, $folder, $requiredBase = true, $m $this->migrationTable = $migrationTable; } + public function withTransactionEnabled($enabled = true) + { + $this->transaction = $enabled; + return $this; + } + /** - * @param $scheme - * @param $className - * @return $this + * @param $class */ public static function registerDatabase($class) { @@ -327,9 +333,22 @@ protected function migrate($upVersion, $increment, $force) call_user_func_array($this->callableProgress, ['migrate', $currentVersion, $fileInfo]); } - $this->getDbCommand()->setVersion($currentVersion, Migration::VERSION_STATUS_PARTIAL . ' ' . ($increment>0 ? 'up' : 'down')); - $this->getDbCommand()->executeSql($fileInfo["content"]); - $this->getDbCommand()->setVersion($currentVersion, Migration::VERSION_STATUS_COMPLETE); + try { + if ($this->transaction) { + $this->getDbDriver()->beginTransaction(); + } + $this->getDbCommand()->setVersion($currentVersion, Migration::VERSION_STATUS_PARTIAL . ' ' . ($increment>0 ? 'up' : 'down')); + $this->getDbCommand()->executeSql($fileInfo["content"]); + $this->getDbCommand()->setVersion($currentVersion, Migration::VERSION_STATUS_COMPLETE); + if ($this->transaction) { + $this->getDbDriver()->commitTransaction(); + } + } catch (\Exception $e) { + if ($this->transaction) { + $this->getDbDriver()->rollbackTransaction(); + } + throw $e; + } $currentVersion = $currentVersion + $increment; } } diff --git a/tests/MigrationTest.php b/tests/MigrationTest.php index 2d435f9..68269ee 100644 --- a/tests/MigrationTest.php +++ b/tests/MigrationTest.php @@ -1,6 +1,7 @@ __DIR__ . '/dirstructure/migrations/up/00001.sql', "description" => "this is a test", "exists" => true, - "checksum" => "b937afa57e363c9244fa30844dd11d312694f697", - "content" => "-- @description: this is a test\n\nselect * from mysql.users;\n", + "checksum" => "55249baf6b70c1d2e9c5362de133b2371d0dc989", + "content" => "-- @description: this is a test\n", ], $this->object->getFileContent(__DIR__ . '/dirstructure/migrations/up/00001.sql') ); @@ -127,8 +128,8 @@ public function testGetFileContent_2() "file" => __DIR__ . '/dirstructure/migrations/up/00002.sql', "description" => "another test", "exists" => true, - "checksum" => "fd8ab8176291c2dcbf0d91564405e0f98f0cd77e", - "content" => "-- @description: another test\n\nselect * from dual;", + "checksum" => "f20c73a5eb4d29e2f8edae4409a2ccc2b02c6f67", + "content" => "-- @description: another test\n", ], $this->object->getFileContent(__DIR__ . '/dirstructure/migrations/up/00002.sql') ); @@ -141,10 +142,41 @@ public function testGetFileContent_3() "file" => __DIR__ . '/dirstructure/migrations/up/00003.sql', "description" => "no description provided. Pro tip: use `-- @description:` to define one.", "exists" => true, - "checksum" => "73faaa68e2f60c11e75a9ccc18528e0ffa15127a", - "content" => "select something from sometable;", + "checksum" => "da39a3ee5e6b4b0d3255bfef95601890afd80709", + "content" => "", ], $this->object->getFileContent(__DIR__ . '/dirstructure/migrations/up/00003.sql') ); } + + public function testReset() + { + $this->expectException(\PDOException::class); + Migration::registerDatabase(SqliteDatabase::class); + $this->object = new Migration(new Uri('sqlite:///tmp/test.db'), __DIR__ . '/dirstructure2'); + $this->object->reset(); + } + + public function testResetWithoutTransactionCheck() + { + try { + Migration::registerDatabase(SqliteDatabase::class); + $this->object = new Migration(new Uri('sqlite:///tmp/test.db'), __DIR__ . '/dirstructure2'); + $this->object->reset(); + } catch (\PDOException $ex) { + $this->assertEquals(["version" => '2', "status" => "partial up"], $this->object->getCurrentVersion()); + } + } + + public function testResetWithTransactionCheck() + { + try { + Migration::registerDatabase(SqliteDatabase::class); + $this->object = new Migration(new Uri('sqlite:///tmp/test.db'), __DIR__ . '/dirstructure2'); + $this->object->withTransactionEnabled(); + $this->object->reset(); + } catch (\PDOException $ex) { + $this->assertEquals(["version" => '1', "status" => "complete"], $this->object->getCurrentVersion()); + } + } } diff --git a/tests/dirstructure/migrations/up/00001.sql b/tests/dirstructure/migrations/up/00001.sql index bba8da9..16d573f 100644 --- a/tests/dirstructure/migrations/up/00001.sql +++ b/tests/dirstructure/migrations/up/00001.sql @@ -1,3 +1 @@ -- @description: this is a test - -select * from mysql.users; diff --git a/tests/dirstructure/migrations/up/00002.sql b/tests/dirstructure/migrations/up/00002.sql index 3f8ff69..d4f1bbe 100644 --- a/tests/dirstructure/migrations/up/00002.sql +++ b/tests/dirstructure/migrations/up/00002.sql @@ -1,3 +1 @@ -- @description: another test - -select * from dual; \ No newline at end of file diff --git a/tests/dirstructure/migrations/up/00003.sql b/tests/dirstructure/migrations/up/00003.sql index 3f17ec4..e69de29 100644 --- a/tests/dirstructure/migrations/up/00003.sql +++ b/tests/dirstructure/migrations/up/00003.sql @@ -1 +0,0 @@ -select something from sometable; \ No newline at end of file diff --git a/tests/dirstructure2/base.sql b/tests/dirstructure2/base.sql new file mode 100644 index 0000000..e69de29 diff --git a/tests/dirstructure2/migrations/down/0000.sql b/tests/dirstructure2/migrations/down/0000.sql new file mode 100644 index 0000000..e69de29 diff --git a/tests/dirstructure2/migrations/down/00001.sql b/tests/dirstructure2/migrations/down/00001.sql new file mode 100644 index 0000000..e69de29 diff --git a/tests/dirstructure2/migrations/down/00002.sql b/tests/dirstructure2/migrations/down/00002.sql new file mode 100644 index 0000000..e69de29 diff --git a/tests/dirstructure2/migrations/up/00001.sql b/tests/dirstructure2/migrations/up/00001.sql new file mode 100644 index 0000000..e69de29 diff --git a/tests/dirstructure2/migrations/up/00002.sql b/tests/dirstructure2/migrations/up/00002.sql new file mode 100644 index 0000000..760589c --- /dev/null +++ b/tests/dirstructure2/migrations/up/00002.sql @@ -0,0 +1 @@ +error \ No newline at end of file diff --git a/tests/dirstructure2/migrations/up/00003.sql b/tests/dirstructure2/migrations/up/00003.sql new file mode 100644 index 0000000..e69de29