From e9f0cbb49bc6e48d433946b030ec340f87d3fa28 Mon Sep 17 00:00:00 2001 From: HugoFara Date: Thu, 27 Aug 2026 11:32:41 +0200 Subject: [PATCH] fix(db): restore a foreign key its own rows violate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit addMissingForeignKeys() adds a constraint with a plain ALTER TABLE, and InnoDB validates the existing rows when it does. That contradicts the premise of the repair: the reason an install is missing a constraint is that it spent time without one, so the rows it would have prevented are already there. With FOREIGN_KEY_CHECKS on, InnoDB answers errno 1452 and refuses precisely the keys most worth putting back — and the failure is only logged, so the constraint is lost without anything saying so. It worked anywhere it was actually reached, because update() and Restore::run() both set FOREIGN_KEY_CHECKS = 0 around their own migration runs, and reconcileForeignKeys()' docblock already describes adding under that setting as the intended behaviour. Nothing established it; the method inherited it from whichever caller happened to be on the stack. Establish it here instead. The caller cannot know the setting is needed — it is a property of what this method does, not of when it is called. Found through MigrationsTest::testForeignKeysSurviveDropAndRestore, which called restoreForeignKeys() directly, without the ambient setting, and lost texttags.fk_texttags_text_tag on every run where the test database happened to hold rows violating it. Reproduced on the test schema: texttags rows: 3 text_tags rows: 0 orphans: 3 ADD CONSTRAINT, FOREIGN_KEY_CHECKS=1 -> ERROR 1452 ADD CONSTRAINT, FOREIGN_KEY_CHECKS=0 -> added The test read as flaky because it is self-concealing: a run that fails to restore the key leaves it dropped, so the next run captures a "before" set without it and passes. The new test covers the case directly, with a deliberately violating row. --- .../Infrastructure/Database/Migrations.php | 38 +++++++++++++ .../backend/Core/Database/MigrationsTest.php | 53 +++++++++++++++++++ 2 files changed, 91 insertions(+) diff --git a/src/Shared/Infrastructure/Database/Migrations.php b/src/Shared/Infrastructure/Database/Migrations.php index fd88cc39f..a44ac051e 100644 --- a/src/Shared/Infrastructure/Database/Migrations.php +++ b/src/Shared/Infrastructure/Database/Migrations.php @@ -232,6 +232,44 @@ public static function getMissingForeignKeys(): array * @return int How many were created */ private static function addMissingForeignKeys(array $keys): int + { + // Rows a missing constraint would have prevented are already in the + // database — that is the whole premise of repairing an install that + // lost its keys. InnoDB validates existing rows when a constraint is + // added, so with checks on it refuses the very keys most worth putting + // back, and the failure is only logged: the constraint is quietly lost. + // + // update() and Restore both happen to set this already, so the repair + // worked from there and nowhere else. Establish it here instead of + // depending on the caller, since the caller cannot know it is needed. + $checksWereOn = ((int) Connection::preparedFetchValue( + 'SELECT @@FOREIGN_KEY_CHECKS AS value' + )) === 1; + if ($checksWereOn) { + Connection::execute('SET FOREIGN_KEY_CHECKS = 0'); + } + + try { + return self::addForeignKeysUnchecked($keys); + } finally { + if ($checksWereOn) { + Connection::execute('SET FOREIGN_KEY_CHECKS = 1'); + } + } + } + + /** + * Add the absent constraints, assuming FK checks are already off. + * + * @param array, + * refTable: string, refColumns: array, + * onUpdate: string, onDelete: string + * }> $keys Foreign keys to add if missing + * + * @return int How many were added + */ + private static function addForeignKeysUnchecked(array $keys): int { $existing = []; foreach (self::captureForeignKeys() as $key) { diff --git a/tests/backend/Core/Database/MigrationsTest.php b/tests/backend/Core/Database/MigrationsTest.php index 7f372916e..6814fce0f 100644 --- a/tests/backend/Core/Database/MigrationsTest.php +++ b/tests/backend/Core/Database/MigrationsTest.php @@ -660,6 +660,59 @@ public function testRecordMigrationPromotesFailureToApplied(): void Connection::preparedExecute("DELETE FROM _migrations WHERE filename = ?", [$testFilename]); } + public function testAConstraintIsRestoredEvenWhenItsRowsViolateIt(): void + { + if (!self::$dbConnected) { + $this->markTestSkipped('Database connection required'); + } + + // The premise of repairing an install that lost its keys is that rows + // the constraint would have prevented are already there. InnoDB + // validates existing rows when a constraint is added, so with FK checks + // on it refuses exactly the keys most worth restoring (errno 1452) and + // addMissingForeignKeys() only logs it — the constraint is lost. + $before = Migrations::captureForeignKeys(); + $target = null; + foreach ($before as $key) { + if ($key['table'] === 'texttags' && count($key['columns']) === 1) { + $target = $key; + break; + } + } + if ($target === null) { + $this->markTestSkipped('No single-column texttags constraint to exercise'); + } + + Connection::execute('SET FOREIGN_KEY_CHECKS = 0'); + Connection::preparedExecute( + 'INSERT IGNORE INTO texttags (TtTxID, TtT2ID) VALUES (?, ?)', + [999123, 999123] + ); + Connection::execute('SET FOREIGN_KEY_CHECKS = 1'); + + try { + Migrations::dropAllForeignKeys(); + Migrations::restoreForeignKeys($before); + + $names = array_map( + static fn(array $k): string => $k['table'] . '.' . $k['name'], + Migrations::captureForeignKeys() + ); + $this->assertContains( + $target['table'] . '.' . $target['name'], + $names, + 'A constraint its own rows violate must still be restored' + ); + } finally { + Connection::execute('SET FOREIGN_KEY_CHECKS = 0'); + Connection::preparedExecute( + 'DELETE FROM texttags WHERE TtTxID = ? AND TtT2ID = ?', + [999123, 999123] + ); + Connection::execute('SET FOREIGN_KEY_CHECKS = 1'); + } + } + public function testForeignKeysSurviveDropAndRestore(): void { if (!self::$dbConnected) {