From 3d721b7d6be64447f5d03a4315b96bfe7ca38603 Mon Sep 17 00:00:00 2001 From: blaipr Date: Tue, 18 Aug 2026 23:17:10 +0200 Subject: [PATCH] fix: apply the CustomFieldData migration as one statement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `40024210101.sql` gives CustomFieldData the identity it should always have had, dropping the surrogate `id` and making (moduleId, itemId, definitionId) the primary key. It did so as two statements, and DDL commits as it goes, so those are two commits: drop column id -> succeeds add primary key -> ERROR 1062 Duplicate entry '1-1-1' for key 'PRIMARY' retry -> ERROR 1091 Can't DROP COLUMN `id` Any installation holding a duplicate of that triple — which the surrogate key allowed — loses the column, keeps its old database version because `UpgradeDatabase::apply()` throws before writing it, and then fails every retry on the already-applied statement before reaching the one that failed. The upgrade can neither be finished nor repeated. As one ALTER the server applies both or neither: the duplicate still refuses it, the table is left as it was, and once the operator clears the duplicates the same file succeeds. `MigrationIsAtomicTest` runs the real file through the real parser against a real server, on a table shaped the way an installation's is before the migration. The unit test hard-coded both statements and errored the moment they became one, which is what it is there for; it now pins the single statement and points at the new test for the reason. --- schemas/40024210101.sql | 15 +- .../Domain/Upgrade/MigrationIsAtomicTest.php | 190 ++++++++++++++++++ .../Upgrade/Services/UpgradeDatabaseTest.php | 13 +- 3 files changed, 209 insertions(+), 9 deletions(-) create mode 100644 tests/Integration/Domain/Upgrade/MigrationIsAtomicTest.php diff --git a/schemas/40024210101.sql b/schemas/40024210101.sql index 4c8e3623b..5845e64c9 100644 --- a/schemas/40024210101.sql +++ b/schemas/40024210101.sql @@ -1,7 +1,16 @@ +-- CustomFieldData's identity is the row it describes, not a surrogate id. +-- +-- Both changes are one ALTER on purpose. As two statements the drop committed on its own — DDL +-- always does — and the primary key then failed on any installation holding a duplicate +-- (moduleId, itemId, definitionId), which the old surrogate key allowed. That left the column +-- gone, the database version unchanged because the upgrade throws before writing it, and every +-- retry failing on `Can't DROP COLUMN id` before it could reach the statement that had actually +-- failed. The upgrade could not be completed and could not be repeated. +-- +-- As one statement the server applies both or neither, so a duplicate leaves the table untouched: +-- the operator clears the duplicates and runs the upgrade again. DELIMITER $$ alter table CustomFieldData - drop column id$$ - -alter table CustomFieldData + drop column id, add primary key (moduleId, itemId, definitionId)$$ diff --git a/tests/Integration/Domain/Upgrade/MigrationIsAtomicTest.php b/tests/Integration/Domain/Upgrade/MigrationIsAtomicTest.php new file mode 100644 index 000000000..79c722ac2 --- /dev/null +++ b/tests/Integration/Domain/Upgrade/MigrationIsAtomicTest.php @@ -0,0 +1,190 @@ +. + */ + +namespace SP\Tests\Integration\Domain\Upgrade; + +use PDO; +use PDOException; +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\TestCase; +use SP\Infrastructure\Database\MysqlFileParser; +use SP\Infrastructure\File\FileHandler; + +use function SP\Tests\getDbHandler; + +/** + * A schema migration that stops half way has to be one somebody can run again. + * + * `UpgradeDatabase::apply()` runs the statements of a version's file one at a time and writes the + * new database version only once they have all succeeded — which is right, and which is also why + * the statements themselves have to be able to fail together. DDL commits as it goes, whatever + * the application does around it, so two `ALTER`s in a file are two commits: the first stands + * even when the second is refused. + * + * `40024210101.sql` gives `CustomFieldData` the identity it should always have had, dropping the + * surrogate `id` and making `(moduleId, itemId, definitionId)` the primary key. Written as two + * statements the drop committed alone, and the key then failed on any installation holding a + * duplicate of that triple — which the surrogate key had allowed. The column was gone, the + * version was unchanged, and the retry died on `Can't DROP COLUMN id` before reaching the + * statement that had actually failed: an upgrade that could neither be finished nor repeated. + * + * This runs the real file against a real server, on a table shaped the way an installation's is + * before the migration, holding the duplicate that provokes the failure. + */ +#[Group('integration')] +final class MigrationIsAtomicTest extends TestCase +{ + private const VERSION_FILE = REAL_APP_ROOT . '/schemas/40024210101.sql'; + private const SCHEMA = 'syspass_migration_probe'; + + private PDO $pdo; + + protected function setUp(): void + { + parent::setUp(); + + $this->pdo = getDbHandler()->getConnection(); + + // A scratch schema: this rewrites a table, and the fixture database belongs to every + // other test in the run. + $this->pdo->exec(sprintf('DROP DATABASE IF EXISTS `%s`', self::SCHEMA)); + $this->pdo->exec(sprintf('CREATE DATABASE `%s`', self::SCHEMA)); + $this->pdo->exec(sprintf('USE `%s`', self::SCHEMA)); + + // CustomFieldData as it stands before this migration. + $this->pdo->exec( + 'CREATE TABLE `CustomFieldData` ( + `id` int(11) NOT NULL AUTO_INCREMENT, + `moduleId` int(10) unsigned NOT NULL, + `itemId` int(10) unsigned NOT NULL, + `definitionId` int(10) unsigned NOT NULL, + `data` longblob, + `key` varbinary(1000), + PRIMARY KEY (`id`) + ) ENGINE = InnoDB' + ); + } + + protected function tearDown(): void + { + $this->pdo->exec(sprintf('DROP DATABASE IF EXISTS `%s`', self::SCHEMA)); + + parent::tearDown(); + } + + /** + * A duplicate leaves the table exactly as it was, so the upgrade can be run again. + */ + #[Test] + public function aRefusedMigrationLeavesTheTableAloneAndCanBeRepeated(): void + { + $this->givenARowTwice(); + + $before = $this->columns(); + + $failed = $this->applyMigration(); + + self::assertNotNull($failed, 'a duplicate of the new key must stop this migration'); + self::assertStringContainsString('Duplicate entry', $failed); + + self::assertSame( + $before, + $this->columns(), + 'the migration was refused, so the table must be as it was — with `id` still there, or ' + . 'the operator has nothing left to run again' + ); + + // What the operator does next: clear the duplicate, run the upgrade again. + $this->pdo->exec('DELETE FROM `CustomFieldData` LIMIT 1'); + + self::assertNull($this->applyMigration(), 'the same file must succeed once the data allows it'); + self::assertSame( + ['moduleId', 'itemId', 'definitionId'], + $this->primaryKeyColumns(), + 'the row is identified by what it describes' + ); + } + + /** + * And with nothing in the way it simply applies. + */ + #[Test] + public function theMigrationAppliesToATableThatAllowsIt(): void + { + $this->pdo->exec('INSERT INTO `CustomFieldData` (`moduleId`, `itemId`, `definitionId`) VALUES (1, 1, 1)'); + + self::assertNull($this->applyMigration()); + self::assertSame(['moduleId', 'itemId', 'definitionId'], $this->primaryKeyColumns()); + self::assertNotContains('id', $this->columns(), 'the surrogate key is gone'); + } + + private function givenARowTwice(): void + { + $this->pdo->exec( + 'INSERT INTO `CustomFieldData` (`moduleId`, `itemId`, `definitionId`) VALUES (1, 1, 1), (1, 1, 1)' + ); + } + + /** + * Runs the version file the way UpgradeDatabase does — the real statements, one at a time, + * stopping at the first refusal. + * + * @return string|null the error, or null when every statement applied + */ + private function applyMigration(): ?string + { + foreach ((new MysqlFileParser(new FileHandler(self::VERSION_FILE)))->parse('$$') as $query) { + try { + $this->pdo->exec($query); + } catch (PDOException $e) { + return $e->getMessage(); + } + } + + return null; + } + + /** + * @return string[] + */ + private function columns(): array + { + $statement = $this->pdo->query('SHOW COLUMNS FROM `CustomFieldData`'); + + return array_column($statement->fetchAll(PDO::FETCH_ASSOC), 'Field'); + } + + /** + * @return string[] + */ + private function primaryKeyColumns(): array + { + $statement = $this->pdo->query("SHOW KEYS FROM `CustomFieldData` WHERE `Key_name` = 'PRIMARY'"); + + return array_column($statement->fetchAll(PDO::FETCH_ASSOC), 'Column_name'); + } +} diff --git a/tests/Unit/Domain/Upgrade/Services/UpgradeDatabaseTest.php b/tests/Unit/Domain/Upgrade/Services/UpgradeDatabaseTest.php index 712437288..0c129b211 100644 --- a/tests/Unit/Domain/Upgrade/Services/UpgradeDatabaseTest.php +++ b/tests/Unit/Domain/Upgrade/Services/UpgradeDatabaseTest.php @@ -55,14 +55,15 @@ public function testUpgrade() { $configData = $this->createMock(ConfigDataInterface::class); - $this->database->expects($this->exactly(2)) + // One statement, not two. DDL commits as it goes, so as a pair the drop stood on its own + // when the primary key was refused — and the retry then died on `Can't DROP COLUMN id` + // before reaching the statement that had failed. MigrationIsAtomicTest runs the file + // against a real server and holds it to failing as a whole. + $this->database->expects($this->once()) ->method('runQueryRaw') ->with( - ... - self::withConsecutive( - ['alter table CustomFieldData drop column id'], - ['alter table CustomFieldData add primary key (moduleId, itemId, definitionId)'] - ) + 'alter table CustomFieldData drop column id, ' + . 'add primary key (moduleId, itemId, definitionId)' ); $configData->expects($this->once())