From d2f05f57a1b489203a058d712df11b20b9b795c9 Mon Sep 17 00:00:00 2001 From: blaipr Date: Tue, 18 Aug 2026 23:28:29 +0200 Subject: [PATCH] test: hold the upgrade path to being repeatable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two guards over the way #822 stopped being repeatable, both of which fail today if the thing they describe is undone. A migration that is refused part way has to leave the operator something to run again. `40024210101.sql` gave CustomFieldData its natural primary key as two statements; DDL commits as it goes, so the drop stood alone when the key was refused on a duplicate, and every retry then died on `Can't DROP COLUMN id` before reaching the statement that had failed. Each version file must now be a single statement, or wholly inside one transaction with no DDL in it — DDL would commit and end the transaction under the statements after it — or written so that running it twice is harmless. Both files satisfy it, one each way. `Upgrade::getTargetUpgradeHandlers()` yields a handler's versions in the order `getAttributes()` returns them, which is the order they are written in the class. Nothing sorts them. They ascend today, so the schema change runs before the data migration that needs it; a version added at the top of the list would run first. Out-of-order migrations are not something an installation reports. The messages say what to do about it rather than only what is wrong, because whoever sees them is adding a migration and has a choice of three ways to satisfy the rule. CLAUDE.md records the pattern these two guard, which covers half of the recent fixes: a guard that is not where the change happens — a limit tested against a row already read, a counter written back from a stale read, a hash stored outside the transaction holding what it describes, two commits made out of one logical change. Where the codebase gets it right the guard and the change are one statement. The suite counts there are refreshed to match. --- CLAUDE.md | 19 +- .../Upgrade/UpgradePathCannotRegressTest.php | 236 ++++++++++++++++++ 2 files changed, 254 insertions(+), 1 deletion(-) create mode 100644 tests/Unit/Domain/Upgrade/UpgradePathCannotRegressTest.php diff --git a/CLAUDE.md b/CLAUDE.md index fbff4ba6c..532a709a0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -101,7 +101,7 @@ docker compose exec -e DB_SERVER=db -e DB_NAME=syspass -e DB_USER=root -e DB_PAS -w /var/www/html app vendor/bin/phpunit -c tests/phpunit.xml --testsuite integration --no-coverage ``` -Both pass: **3955 unit** + **942 integration**. The integration suite includes the +Both pass: **3973 unit** + **972 integration**. The integration suite includes the end-to-end CLI command tests (`tests/Integration/Infrastructure/Adapter/In/Cli/`, real DI container + real DB via `CliTestCase`, per-test config under `/tmp/syspass-cli-tests`). Test-environment gotchas (the image provides these): @@ -390,6 +390,23 @@ Key constraints: A record of what repeatedly turned out to be broken, because the pattern predicts the next one better than any coverage number does. +**A guard that is not where the change happens.** The commonest shape here, and the one that reads +as correct in review. A public link's view limit and the temporary master password's fifty-attempt +cap were both tested in PHP against a row that had already been read, so two requests arriving +together both passed — and the attempt counter was written back as `$attempts + 1`, an absolute +value worked out from that same stale read, so guesses in parallel advanced it by one between them. +The master password's rotation re-encrypted every secret inside a transaction and then stored the +hash describing them outside it, leaving a vault nobody could open if those last two writes failed. +`40024210101.sql` made two commits out of one logical change, and DDL commits as it goes, so a +refused second statement left an upgrade that could be neither finished nor repeated. + +Where the codebase gets this right it is always the same move — the guard and the change are one +statement: `UserPassRecover::toggleUsedByHash()` consumes a reset token with `used = 0` in its +`WHERE` and throws when it affects nothing, `InstallThrottle` holds an exclusive `flock` across its +whole read-modify-write, `countViews + 1` is arithmetic the server does. **Ask where the decision is +taken and where the change lands; if they are not the same statement, work out what fits between +them.** + **The wiring, not the code.** php-di skips a constructor parameter that has a default *even when the container has a binding for its type*, silently. `Init::$sessionKeyService` was null that way, so `reKey()` — and the `session_regenerate_id()` inside it — never ran, and session identifiers were diff --git a/tests/Unit/Domain/Upgrade/UpgradePathCannotRegressTest.php b/tests/Unit/Domain/Upgrade/UpgradePathCannotRegressTest.php new file mode 100644 index 000000000..f420e07e1 --- /dev/null +++ b/tests/Unit/Domain/Upgrade/UpgradePathCannotRegressTest.php @@ -0,0 +1,236 @@ +. + */ + +namespace SP\Tests\Unit\Domain\Upgrade; + +use PHPUnit\Framework\Attributes\DataProvider; +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\TestCase; +use ReflectionClass; +use SP\Domain\Common\Providers\Version; +use SP\Domain\Upgrade\Services\UpgradeConfigText; +use SP\Domain\Upgrade\Services\UpgradeDatabase; +use SP\Domain\Common\Attributes\UpgradeVersion; +use SP\Infrastructure\Database\MysqlFileParser; +use SP\Infrastructure\File\FileHandler; + +/** + * An upgrade is the one thing a user runs on data they cannot afford to lose, on an installation + * that is already broken enough to need it. Both of these guard a way it stopped being repeatable. + * + * The migration rule comes from `40024210101.sql`, which gave `CustomFieldData` its natural + * primary key as two statements. DDL commits as it goes, so the drop stood alone when the key was + * refused on a duplicate, and the retry then died on `Can't DROP COLUMN id` before it could reach + * the statement that had failed — `UpgradeDatabase::apply()` writes the new version only after + * every statement succeeds, so the version stayed old and the file was re-run from the top for + * ever. It is one `ALTER` now, and `MigrationIsAtomicTest` holds it to failing as a whole against + * a real server. This holds the *next* migration to being written the same way. + * + * The ordering rule is about how those files are reached. `Upgrade::getTargetUpgradeHandlers()` + * yields a handler's versions in the order `getAttributes()` returns them, which is the order they + * are written in the class — nothing sorts them. Today they ascend, so the schema change runs + * before the data migration that needs it. A version added at the top of the list would run first, + * and out-of-order migrations are not something an installation reports; they are something it + * survives or does not. + */ +#[Group('unitary')] +class UpgradePathCannotRegressTest extends TestCase +{ + private const SCHEMAS = REAL_APP_ROOT . '/schemas'; + + /** + * Every handler that carries version attributes. + * + * @return array + */ + public static function handlerProvider(): array + { + return [ + 'UpgradeDatabase' => [UpgradeDatabase::class], + 'UpgradeConfigText' => [UpgradeConfigText::class], + ]; + } + + /** + * A handler's versions are written in the order they must be applied. + */ + #[Test] + #[DataProvider('handlerProvider')] + public function versionsAreDeclaredInTheOrderTheyApply(string $handler): void + { + $versions = self::versionsOf($handler); + + self::assertNotEmpty($versions, sprintf('%s carries no version, so it can never run', $handler)); + + $sorted = $versions; + usort( + $sorted, + static fn(string $a, string $b) => version_compare( + (string)Version::normalizeVersionForCompare($a), + (string)Version::normalizeVersionForCompare($b) + ) + ); + + self::assertSame( + $sorted, + $versions, + sprintf( + '%s applies its versions in the order they are written, because nothing sorts them. ' + . 'Written out of order they are applied out of order, and a schema change that ' + . 'runs after the data migration needing it does not announce itself.', + $handler + ) + ); + } + + /** + * No two handlers claim the same version twice over in a way that hides which runs first. + * + * They may share a version — `400.24240101` is both a row migration and a config one — and the + * order between handlers is then registration order in `CoreDefinitions`. What must not happen + * is a handler claiming the same version more than once, where the repeat is silently applied + * twice. + */ + #[Test] + #[DataProvider('handlerProvider')] + public function noHandlerClaimsAVersionTwice(string $handler): void + { + $versions = self::versionsOf($handler); + + self::assertSame( + array_values(array_unique($versions)), + $versions, + sprintf('%s would apply a repeated version more than once', $handler) + ); + } + + /** + * Every version file, and the statements it holds. + * + * @return array + */ + public static function migrationProvider(): array + { + $cases = []; + + foreach (glob(self::SCHEMAS . '/*.sql') ?: [] as $path) { + // dbstructure.sql builds a database rather than upgrading one: nothing has been + // applied when it runs, so a failure leaves nothing half-done. + if (basename($path) === 'dbstructure.sql') { + continue; + } + + $cases[basename($path)] = [$path]; + } + + return $cases; + } + + /** + * A migration that is refused part way leaves the operator something to run again. + * + * Three ways to satisfy it, and each is a real answer rather than a formality: + * + * - one statement, which the server applies whole; + * - every statement inside one transaction, which only works while none of them is DDL, + * because DDL commits and would end the transaction under the ones that follow; + * - DDL written so that re-running it is harmless (`IF NOT EXISTS`, `IF EXISTS`). + */ + #[Test] + #[DataProvider('migrationProvider')] + public function aMigrationCanBeRunAgainAfterItIsRefused(string $path): void + { + $statements = iterator_to_array((new MysqlFileParser(new FileHandler($path)))->parse('$$'), false); + + self::assertNotEmpty($statements, sprintf('%s holds no statements', basename($path))); + + if (count($statements) === 1) { + return; + } + + $ddl = array_values( + array_filter( + $statements, + static fn(string $s) => preg_match('/^\s*(alter|create|drop|rename|truncate)\b/i', $s) === 1 + ) + ); + + if ($ddl === []) { + self::assertTrue( + self::isWrappedInATransaction($statements), + sprintf( + '%s applies several statements and none of them is DDL, so it can be one ' + . 'transaction — and has to be, or a failure part way leaves the rows half ' + . 'migrated with the version unchanged.', + basename($path) + ) + ); + + return; + } + + foreach ($ddl as $statement) { + self::assertMatchesRegularExpression( + '/\bIF\s+(NOT\s+)?EXISTS\b/i', + $statement, + sprintf( + "%s applies more than one statement and one of them is DDL, which commits on " + . "its own — so a later failure cannot be undone and the retry meets a change " + . "that is already there. Make it a single statement, or write the DDL so that " + . "running it twice is harmless.\n\n%s", + basename($path), + $statement + ) + ); + } + } + + /** + * @param string[] $statements + */ + private static function isWrappedInATransaction(array $statements): bool + { + $first = strtolower(trim((string)reset($statements))); + $last = strtolower(trim((string)end($statements))); + + return (str_starts_with($first, 'start transaction') || str_starts_with($first, 'begin')) + && str_starts_with($last, 'commit'); + } + + /** + * @param class-string $handler + * + * @return string[] + */ + private static function versionsOf(string $handler): array + { + return array_map( + static fn(\ReflectionAttribute $attribute) => $attribute->newInstance()->version, + (new ReflectionClass($handler))->getAttributes(UpgradeVersion::class) + ); + } +}