From eb60f057d9514208213bc720e174646815e3405e Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 12:49:05 +0200 Subject: [PATCH 01/18] feat(admin-tools): add check-app-migrations to detect schema drift Apps can have migrations recorded in oc_migrations whose schema changes are not in the database, e.g. Talk failing with an unknown column after the 31 to 32 to 33 upgrade. Checking the applied rows alone misses that. The read-only script lists migrations that were never applied and ones that are applied but not in effect, by statically extracting addColumn, createTable, addIndex, dropColumn and similar from changeSchema() and comparing them with the live schema. Code it cannot parse is reported as not verifiable instead of guessed. The DB connection is taken from the Nextcloud config; output contains only versions and table, column and index names. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 30 + admin-tools/check-app-migrations.php | 1266 ++++++++++++++++++++++++++ 2 files changed, 1296 insertions(+) create mode 100755 admin-tools/check-app-migrations.php diff --git a/admin-tools/README.md b/admin-tools/README.md index fb97539..2280aa3 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -93,6 +93,36 @@ Update email addresses in Nextcloud mail app configuration when a customer's ema **Note:** Users may need to refresh their mail app to see the changes. +### check-app-migrations.php + +Read-only check of an app's migrations against the database. Reports migrations that were **never applied** (no row in `oc_migrations`) and migrations that are **applied but not in effect** (row exists, but the columns, tables or indexes they create are missing, or what they drop is still there). Example: Talk failing with `Unknown column 'r.last_pinned_id'` although `23000Date20251030090219` is recorded as applied. + +The expected migrations are the `Version*.php` files in the app's `lib/Migration`, i.e. what is deployed there. No git is used. Schema effects (`addColumn`, `createTable`, `addIndex`, `setPrimaryKey`, `dropColumn`, `dropTable`, `dropIndex`, `renameColumn`) are extracted statically from `changeSchema()`. `hasColumn`-style guards are understood. Effects that a later applied migration overrides are skipped. Anything it cannot parse (data changes in `preSchemaChange`/`postSchemaChange`, loops, helper calls, non-guard conditions, computed names) is listed under "Not verifiable" and never guessed. + +The database connection is taken from the Nextcloud config (`config/config.php` plus `config/*.config.php`); nothing is passed on the command line. + +```bash +# On the instance: the app directory is found automatically +./check-app-migrations.php --app spreed --db + +# sqlite dev instance whose datadirectory is a container path +./check-app-migrations.php --db --sqlite-file data/owncloud.db + +# Every app that has rows in oc_migrations (core included; apps not on disk are skipped) +./check-app-migrations.php --all --db + +# Check elsewhere from dumps taken in the pod +./check-app-migrations.php --db --dump-applied > applied.txt +./check-app-migrations.php --db --dump-schema > schema.txt +./check-app-migrations.php --applied applied.txt --schema schema.txt +``` + +**Options:** `--app` (default `spreed`), `--all` (all apps; clean apps get one line, details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR`, `--sqlite-file FILE`, `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). + +**Exit codes:** 0 nothing wrong, 1 never applied or not in effect found, 2 usage/input error. Output contains only versions and table, column and index names. Sessions are read-only (`SET SESSION TRANSACTION READ ONLY` / `PRAGMA query_only`), and connection errors are reported by code only so no credentials leak. + +**Requires:** PHP with `pdo_mysql` or `pdo_sqlite` (for `--db`). No git needed. + ## Standard Interface diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php new file mode 100755 index 0000000..a4092d8 --- /dev/null +++ b/admin-tools/check-app-migrations.php @@ -0,0 +1,1266 @@ +#!/usr/bin/env php + applied.txt + check-app-migrations.php --db --dump-schema > schema.txt + +Options: + --app APP App id (default: spreed) + --all Check every app that has rows in the migrations table (with --db); + clean apps get one line, details only for apps with findings + --app-path DIR App directory (default: found via apps_paths of the Nextcloud + config, else apps/, apps-external/, custom_apps/) + --db Read applied list and schema from the database + --applied FILE Applied versions, one per line (e.g. 23000Date20251030090219) + --schema FILE Schema dump as produced by --dump-schema + --dump-applied Print the applied versions of APP and exit + --dump-schema Print the live schema (table/column/index names) and exit + --strict Treat "not verifiable" migrations as findings too + --verbose Also list verified and superseded schema effects + -h, --help This help + +Database connection: taken from the Nextcloud config (config/config.php and +config/*.config.php), nothing is passed on the command line or printed. + --config DIR Nextcloud config directory (default: /config) + --sqlite-file F sqlite only: use this file instead of /owncloud.db + +Expected migrations: the Version*.php files in DIR/lib/Migration, i.e. what is +deployed in the app directory. + +Schema dump format, whitespace separated, real (prefixed) table names: + column + index
(primary key: PRIMARY) + +TXT; +} + +function fail(string $message, int $code = EXIT_USAGE): never { + fwrite(STDERR, "[ERROR] $message\n"); + exit($code); +} + +// Never let PHP print exception messages: PDO messages can carry user/host. +set_exception_handler(static function (Throwable $e): void { + fwrite(STDERR, '[ERROR] unexpected ' . get_class($e) . ' (code ' . (string)$e->getCode() . ') at ' + . basename($e->getFile()) . ':' . $e->getLine() . "\n"); + exit(EXIT_USAGE); +}); +set_error_handler(static function (int $no, string $str, string $file, int $line): bool { + fwrite(STDERR, "[ERROR] php error $no in " . basename($file) . ":$line\n"); + exit(EXIT_USAGE); +}); + +// --------------------------------------------------------------------------- +// Expected migrations +// --------------------------------------------------------------------------- + +/** @return array version => php source from the app directory */ +function dirMigrations(string $dir): array { + $result = []; + foreach (glob("$dir/Version*.php") ?: [] as $path) { + if (preg_match('/^Version(\d+Date\d+)\.php$/', basename($path), $m) === 1) { + $result[$m[1]] = (string)file_get_contents($path); + } + } + uksort($result, 'compareVersions'); + return $result; +} + +function compareVersions(string $a, string $b): int { + return [(int)$a, $a] <=> [(int)$b, $b]; +} + +// --------------------------------------------------------------------------- +// Live state: applied list and schema +// --------------------------------------------------------------------------- + +/** + * Schema model: ['columns' => [table => [col => true]], 'indexes' => [table => [name => [cols]]]] + * Names are lower-cased and stripped of the table prefix. + */ +function newSchema(): array { + return ['columns' => [], 'indexes' => []]; +} + +function stripPrefix(string $table, string $prefix): ?string { + if ($prefix !== '' && strncmp($table, $prefix, strlen($prefix)) !== 0) { + return null; + } + return strtolower(substr($table, strlen($prefix))); +} + +function addColumnTo(array &$schema, string $table, string $column): void { + $schema['columns'][$table][strtolower($column)] = true; +} + +function addIndexTo(array &$schema, string $table, string $name, array $cols): void { + $schema['indexes'][$table][strtolower($name) === 'primary' ? 'PRIMARY' : $name] = array_map('strtolower', $cols); +} + +function readAppliedFile(string $file): array { + if (!is_readable($file)) { + fail("cannot read applied file $file"); + } + $result = []; + foreach (file($file, FILE_IGNORE_NEW_LINES) as $no => $line) { + $line = trim($line); + if ($line === '' || $line[0] === '#') { + continue; + } + if (preg_match('/^\d+Date\d+$/', $line) !== 1) { + fail('applied file: line ' . ($no + 1) . ' is not a migration version'); + } + $result[$line] = true; + } + return $result; +} + +function readSchemaFile(string $file, string $prefix): array { + if (!is_readable($file)) { + fail("cannot read schema file $file"); + } + $schema = newSchema(); + foreach (file($file, FILE_IGNORE_NEW_LINES) as $no => $line) { + $line = trim($line); + if ($line === '' || $line[0] === '#') { + continue; + } + $f = preg_split('/\s+/', $line); + $table = isset($f[1]) ? stripPrefix($f[1], $prefix) : null; + if ($f[0] === 'column' && count($f) === 3) { + if ($table !== null) { + addColumnTo($schema, $table, $f[2]); + } + } elseif ($f[0] === 'index' && count($f) === 4) { + if ($table !== null) { + addIndexTo($schema, $table, $f[2], explode(',', $f[3])); + } + } else { + fail('schema file: line ' . ($no + 1) . ' is malformed'); + } + } + return $schema; +} + +/** @return array only the keys needed for the DB connection */ +function loadNcConfig(string $dir, bool $required = true): array { + $files = []; + if (is_file("$dir/config.php")) { + $files[] = "$dir/config.php"; + } + $extra = glob("$dir/*.config.php") ?: []; + sort($extra); + $files = array_merge($files, $extra); + if ($files === [] && !$required) { + return []; + } + if ($files === []) { + fail("no Nextcloud config found in $dir (use --config)"); + } + // Some configs reference OC::$SERVERROOT; Nextcloud is not booted here, so provide just that. + if (!class_exists('OC', false)) { + eval('class OC { public static string $SERVERROOT = ' . var_export(dirname(__DIR__, 2), true) . '; }'); + } + $load = static function (string $file): array { + $CONFIG = []; + include $file; + return is_array($CONFIG) ? $CONFIG : []; + }; + $config = []; + foreach ($files as $file) { + $config = array_replace($config, $load($file)); + } + $keys = ['dbtype', 'dbhost', 'dbname', 'dbuser', 'dbpassword', 'dbtableprefix', 'datadirectory', 'apps_paths']; + return array_intersect_key($config, array_flip($keys)); +} + +/** Directory of the app: apps_paths from the config (also mapped below the NC root), then the usual places. */ +function findAppDir(string $app, string $ncRoot, array $config, bool $required = true): ?string { + $roots = []; + foreach ((array)($config['apps_paths'] ?? []) as $entry) { + if (is_array($entry) && isset($entry['path'])) { + $roots[] = (string)$entry['path']; + $roots[] = $ncRoot . '/' . basename((string)$entry['path']); + } + } + array_push($roots, "$ncRoot/apps", "$ncRoot/apps-external", "$ncRoot/custom_apps"); + foreach ($roots as $root) { + if (is_file("$root/$app/appinfo/info.xml")) { + return "$root/$app"; + } + } + if (!$required) { + return null; + } + fail("app $app not found in apps_paths or $ncRoot/{apps,apps-external,custom_apps} (use --app-path)"); +} + +function connectDb(array $config, ?string $sqliteFile): PDO { + $type = (string)($config['dbtype'] ?? ''); + if ($type === 'sqlite' || $type === 'sqlite3') { + $file = $sqliteFile ?? rtrim((string)($config['datadirectory'] ?? ''), '/') . '/' . ($config['dbname'] ?? 'owncloud') . '.db'; + if (!is_file($file)) { + fail('sqlite database file not found (use --sqlite-file)'); + } + $pdo = new PDO('sqlite:' . $file); + $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); + $pdo->exec('PRAGMA query_only = ON'); + return $pdo; + } + if ($type === 'mysql') { + if (!extension_loaded('pdo_mysql')) { + fail('PHP extension pdo_mysql is not available'); + } + // dbhost is "host", "host:port", "/path/to.sock" or "host:/path/to.sock" + $host = (string)($config['dbhost'] ?? 'localhost'); + $dsn = 'mysql:dbname=' . (string)($config['dbname'] ?? '') . ';charset=utf8mb4;'; + if (preg_match('#^(.*?):?(/.*)$#', $host, $m) === 1) { + $dsn .= 'unix_socket=' . $m[2]; + } elseif (preg_match('/^(.+):(\d+)$/', $host, $m) === 1) { + $dsn .= 'host=' . $m[1] . ';port=' . $m[2]; + } else { + $dsn .= 'host=' . $host; + } + try { + $pdo = new PDO($dsn, (string)($config['dbuser'] ?? ''), (string)($config['dbpassword'] ?? '')); + $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); + $pdo->exec('SET SESSION TRANSACTION READ ONLY'); + } catch (PDOException $e) { + fail('database connection failed (code ' . (string)$e->getCode() . ')'); + } + return $pdo; + } + fail('unsupported dbtype in Nextcloud config (mysql and sqlite only)'); +} + +function dbApplied(PDO $pdo, string $app, string $prefix): array { + $stmt = $pdo->prepare('SELECT version FROM ' . $prefix . 'migrations WHERE app = ?'); + $stmt->execute([$app]); + $result = []; + foreach ($stmt->fetchAll(PDO::FETCH_COLUMN) as $version) { + $result[(string)$version] = true; + } + return $result; +} + +/** @return array> app => versions */ +function dbAppliedAll(PDO $pdo, string $prefix): array { + $result = []; + foreach ($pdo->query('SELECT app, version FROM ' . $prefix . 'migrations')->fetchAll(PDO::FETCH_NUM) as [$app, $version]) { + $result[(string)$app][(string)$version] = true; + } + ksort($result); + return $result; +} + +function dbSchema(PDO $pdo, string $prefix): array { + $schema = newSchema(); + if ($pdo->getAttribute(PDO::ATTR_DRIVER_NAME) === 'sqlite') { + $tables = $pdo->query("SELECT name FROM sqlite_master WHERE type = 'table'")->fetchAll(PDO::FETCH_COLUMN); + foreach ($tables as $real) { + $table = stripPrefix((string)$real, $prefix); + if ($table === null) { + continue; + } + $quoted = '"' . str_replace('"', '""', (string)$real) . '"'; + $pk = []; + foreach ($pdo->query("PRAGMA table_info($quoted)")->fetchAll(PDO::FETCH_ASSOC) as $col) { + addColumnTo($schema, $table, (string)$col['name']); + if ((int)$col['pk'] > 0) { + $pk[(int)$col['pk']] = (string)$col['name']; + } + } + if ($pk !== []) { + ksort($pk); + addIndexTo($schema, $table, 'PRIMARY', array_values($pk)); + } + foreach ($pdo->query("PRAGMA index_list($quoted)")->fetchAll(PDO::FETCH_ASSOC) as $idx) { + if ($idx['origin'] === 'pk') { + continue; + } + $iq = '"' . str_replace('"', '""', (string)$idx['name']) . '"'; + $cols = array_column($pdo->query("PRAGMA index_info($iq)")->fetchAll(PDO::FETCH_ASSOC), 'name'); + addIndexTo($schema, $table, (string)$idx['name'], array_map('strval', $cols)); + } + } + return $schema; + } + + $rows = $pdo->query( + 'SELECT TABLE_NAME AS t, COLUMN_NAME AS c FROM information_schema.COLUMNS WHERE TABLE_SCHEMA = DATABASE()' + )->fetchAll(PDO::FETCH_NUM); + foreach ($rows as [$real, $col]) { + $table = stripPrefix((string)$real, $prefix); + if ($table !== null) { + addColumnTo($schema, $table, (string)$col); + } + } + $rows = $pdo->query( + 'SELECT TABLE_NAME, INDEX_NAME, COLUMN_NAME FROM information_schema.STATISTICS ' + . 'WHERE TABLE_SCHEMA = DATABASE() ORDER BY TABLE_NAME, INDEX_NAME, SEQ_IN_INDEX' + )->fetchAll(PDO::FETCH_NUM); + $grouped = []; + foreach ($rows as [$real, $index, $col]) { + $table = stripPrefix((string)$real, $prefix); + if ($table !== null) { + $grouped[$table][(string)$index][] = (string)$col; + } + } + foreach ($grouped as $table => $indexes) { + foreach ($indexes as $name => $cols) { + addIndexTo($schema, $table, (string)$name, $cols); + } + } + return $schema; +} + +function dumpSchema(PDO $pdo, string $prefix): void { + // Dump with the prefix kept so that the file is self-describing. + $schema = dbSchema($pdo, $prefix); + foreach ($schema['columns'] as $table => $cols) { + foreach (array_keys($cols) as $col) { + echo "column $prefix$table $col\n"; + } + } + foreach ($schema['indexes'] as $table => $indexes) { + foreach ($indexes as $name => $cols) { + echo "index $prefix$table $name " . implode(',', $cols) . "\n"; + } + } +} + +// --------------------------------------------------------------------------- +// Static extraction of schema effects from a migration +// --------------------------------------------------------------------------- + +/** + * Walks the token stream of changeSchema() and collects schema effects. + * Anything it cannot interpret is recorded as a reason and never guessed. + */ +final class EffectExtractor { + private const LOOPS = [T_FOREACH, T_FOR, T_WHILE, T_DO, T_SWITCH, T_TRY, T_MATCH]; + private const GUARD_CALLS = ['hasColumn', 'hasIndex', 'hasTable', 'hasPrimaryKey', 'hasUniqueConstraint']; + private const IGNORED_TABLE_CALLS = [ + 'hasColumn', 'hasIndex', 'hasPrimaryKey', 'hasUniqueConstraint', 'getName', 'addOption', 'setComment', + ]; + + /** @var list */ + private array $T = []; + /** @var array */ + private array $consts = []; + /** @var array */ + private array $vars = []; + /** @var list> */ + public array $effects = []; + /** @var array */ + public array $reasons = []; + private bool $conditional = false; + /** @var array tables that code we could not interpret may have changed */ + private array $touched = []; + /** @var array effect keys that an early-return has*() guard already speaks about */ + private array $guarded = []; + + /** + * @return array{effects:list>,reasons:list,touched:list} + */ + public static function extract(string $source): array { + $self = new self(); + $self->T = []; + foreach (token_get_all($source) as $tok) { + if (is_array($tok)) { + if (in_array($tok[0], [T_WHITESPACE, T_COMMENT, T_DOC_COMMENT], true)) { + continue; + } + $self->T[] = ['t' => $tok[0], 's' => $tok[1], 'l' => $tok[2]]; + } else { + $last = $self->T === [] ? 1 : $self->T[count($self->T) - 1]['l']; + $self->T[] = ['t' => null, 's' => $tok, 'l' => $last]; + } + } + $self->collectConsts(); + $methods = $self->findMethods(); + + foreach (['preSchemaChange', 'postSchemaChange'] as $name) { + if (isset($methods[$name]) && !$self->isTrivial(...$methods[$name])) { + $self->reasons["data changes in $name() are not verifiable"] = true; + } + } + if (isset($methods['changeSchema'])) { + $self->vars = ['$schema' => ['k' => 'schema']]; + $self->walk($methods['changeSchema'][0], $methods['changeSchema'][1], true); + } + return [ + 'effects' => $self->effects, + 'reasons' => array_keys($self->reasons), + 'touched' => array_keys($self->touched), + ]; + } + + private function collectConsts(): void { + $n = count($this->T); + for ($i = 0; $i < $n; $i++) { + if ($this->T[$i]['t'] !== T_CONST) { + continue; + } + for ($j = $i + 1; $j < $n && $this->T[$j]['s'] !== ';'; $j++) { + if ($this->T[$j]['t'] === T_STRING && ($this->T[$j + 1]['s'] ?? '') === '=' + && ($this->T[$j + 2]['t'] ?? null) === T_CONSTANT_ENCAPSED_STRING + && ($this->T[$j + 3]['s'] ?? '') === ';') { + $this->consts[$this->T[$j]['s']] = substr($this->T[$j + 2]['s'], 1, -1); + break; + } + } + } + } + + /** @return array method => [first body token, end (exclusive)] */ + private function findMethods(): array { + $methods = []; + $n = count($this->T); + for ($i = 0; $i < $n; $i++) { + if ($this->T[$i]['t'] !== T_FUNCTION || ($this->T[$i + 1]['t'] ?? null) !== T_STRING) { + continue; + } + $name = $this->T[$i + 1]['s']; + $j = $i + 2; + if (($this->T[$j]['s'] ?? '') !== '(') { + continue; + } + $j = $this->close($j) + 1; + while ($j < $n && $this->T[$j]['s'] !== '{' && $this->T[$j]['s'] !== ';') { + $j++; + } + if ($j < $n && $this->T[$j]['s'] === '{') { + $end = $this->close($j); + $methods[$name] = [$j + 1, $end]; + $i = $end; + } + } + return $methods; + } + + private function isTrivial(int $from, int $to): bool { + $s = ''; + for ($i = $from; $i < $to; $i++) { + $s .= $this->T[$i]['s']; + } + return in_array($s, ['', 'return;'], true); + } + + private function isOpen(int $i): bool { + return in_array($this->T[$i]['s'], ['(', '[', '{'], true) + || in_array($this->T[$i]['t'], [T_CURLY_OPEN, T_DOLLAR_OPEN_CURLY_BRACES, T_ATTRIBUTE], true); + } + + private function close(int $open): int { + $depth = 0; + $n = count($this->T); + for ($i = $open; $i < $n; $i++) { + if ($this->isOpen($i)) { + $depth++; + } elseif (in_array($this->T[$i]['s'], [')', ']', '}'], true)) { + $depth--; + if ($depth === 0) { + return $i; + } + } + } + return $n - 1; + } + + private function reason(string $text, int $tokenIndex): void { + $this->reasons[$text . ' (line ' . ($this->T[min($tokenIndex, count($this->T) - 1)]['l']) . ')'] = true; + } + + /** Remember which tables a range of uninterpreted code may touch: bound table variables and string literals. */ + private function touch(int $from, int $to): void { + for ($i = $from; $i < $to && $i < count($this->T); $i++) { + $t = $this->T[$i]; + if ($t['t'] === T_VARIABLE && ($this->vars[$t['s']]['k'] ?? '') === 'table' && isset($this->vars[$t['s']]['name'])) { + $this->touched[$this->vars[$t['s']]['name']] = true; + } elseif ($t['t'] === T_CONSTANT_ENCAPSED_STRING) { + $this->touched[strtolower(substr($t['s'], 1, -1))] = true; + } + } + } + + private function mentions(int $from, int $to): bool { + for ($i = $from; $i < $to; $i++) { + if ($this->T[$i]['t'] === T_VARIABLE && isset($this->vars[$this->T[$i]['s']]) + && $this->vars[$this->T[$i]['s']]['k'] !== 'other') { + return true; + } + } + return false; + } + + private function walk(int $i, int $end, bool $top): void { + while ($i < $end) { + $tok = $this->T[$i]; + if ($tok['t'] === T_IF) { + $i = $this->handleIf($i); + } elseif (in_array($tok['t'], self::LOOPS, true)) { + $i = $this->skipConstruct($i, $end); + } elseif ($tok['s'] === '{') { + $close = $this->close($i); + $this->walk($i + 1, $close, false); + $i = $close + 1; + } elseif ($tok['t'] === T_RETURN && $top) { + return; + } else { + $stmtEnd = $this->statementEnd($i, $end); + $this->statement($i, $stmtEnd); + $i = $stmtEnd + 1; + } + } + } + + private function statementEnd(int $i, int $end): int { + for ($j = $i; $j < $end; $j++) { + if ($this->isOpen($j)) { + $j = $this->close($j); + } elseif ($this->T[$j]['s'] === ';') { + return $j; + } + } + return $end; + } + + private function skipConstruct(int $i, int $end): int { + $start = $i; + $j = $i + 1; + if ($this->T[$i]['t'] === T_DO) { + // do { } while (...); + $j = $this->close($j) + 1; + $j = $this->statementEnd($j, $end) + 1; + } else { + while ($j < $end && $this->T[$j]['s'] !== '{' && $this->T[$j]['s'] !== ';') { + $j = $this->isOpen($j) ? $this->close($j) + 1 : $j + 1; + } + if ($j < $end && $this->T[$j]['s'] === '{') { + $j = $this->close($j) + 1; + while ($j < $end && in_array($this->T[$j]['t'], [T_CATCH, T_FINALLY], true)) { + while ($j < $end && $this->T[$j]['s'] !== '{') { + $j++; + } + $j = $this->close($j) + 1; + } + } else { + $j++; + } + } + if ($this->mentions($start, $j)) { + $this->touch($start, $j); + $this->reason('schema changes inside ' . strtolower(token_name((int)$this->T[$start]['t'])) . ' are not verifiable', $start); + } + return $j; + } + + /** @return int index after the whole if/elseif/else chain */ + private function handleIf(int $i): int { + $branches = []; + $j = $i; + $hasElse = false; + while (true) { + $isElse = $this->T[$j]['t'] === T_ELSE && ($this->T[$j + 1]['t'] ?? null) !== T_IF; + if ($this->T[$j]['t'] === T_ELSE && !$isElse) { + $j++; // "else if" + } + if ($isElse) { + $hasElse = true; + $condFrom = $condTo = $j; + $j++; + } else { + $condFrom = $j + 2; + $condClose = $this->close($j + 1); + $condTo = $condClose; + $j = $condClose + 1; + } + if (($this->T[$j]['s'] ?? '') === '{') { + $bodyEnd = $this->close($j); + $branches[] = [$condFrom, $condTo, $j + 1, $bodyEnd]; + $j = $bodyEnd + 1; + } else { + $bodyEnd = $this->statementEnd($j, count($this->T)); + $branches[] = [$condFrom, $condTo, $j, $bodyEnd]; + $j = $bodyEnd + 1; + } + $nextT = $this->T[$j]['t'] ?? null; + if ($nextT !== T_ELSE && $nextT !== T_ELSEIF) { + break; + } + if ($nextT === T_ELSEIF) { + // "elseif (" behaves like "else if (": normalise by treating as branch start + $this->T[$j]['t'] = T_IF; + } + } + + $single = count($branches) === 1; + if ($single && $this->isGuard($branches[0][0], $branches[0][1])) { + [, , $from, $to] = $branches[0]; + if ($this->conditional) { + if ($this->mentions($from, $to)) { + $this->touch($from, $to); + $this->reasons['some schema changes follow a conditional early return and are not verifiable'] = true; + } + return $j; + } + $this->walk($from, $to, false); + for ($k = $from; $k < $to; $k++) { + if ($this->T[$k]['t'] === T_RETURN) { + $this->conditional = true; + $this->collectGuardedKeys($branches[0][0], $branches[0][1]); + break; + } + } + return $j; + } + foreach ($branches as [$cf, $ct, $bf, $bt]) { + if ($this->mentions($cf, $ct) || $this->mentions($bf, $bt)) { + foreach ($branches as [, , $tf, $tt]) { + $this->touch($tf, $tt); + } + $this->reason('schema changes under a condition that is not a plain has*() guard are not verifiable', $i); + break; + } + } + return $j; + } + + /** + * "if (hasTable('x')) return;" followed by createTable('x') leaves 'x' in place either way, + * so effects on exactly the guarded object stay verifiable; everything else does not. + */ + private function collectGuardedKeys(int $from, int $to): void { + for ($i = $from; $i + 4 < $to; $i++) { + if ($this->T[$i]['t'] !== T_VARIABLE || $this->T[$i + 1]['t'] !== T_OBJECT_OPERATOR + || ($this->T[$i + 3]['s'] ?? '') !== '(') { + continue; + } + $recv = $this->vars[$this->T[$i]['s']] ?? ['k' => 'other']; + $method = $this->T[$i + 2]['s']; + $arg = $this->str([$i + 4, $i + 5]); + if ($arg === null) { + continue; + } + $arg = strtolower($arg); + if ($recv['k'] === 'schema' && $method === 'hasTable') { + $this->guarded["tbl:$arg"] = true; + } elseif ($recv['k'] === 'table' && isset($recv['name'])) { + if ($method === 'hasColumn') { + $this->guarded["col:{$recv['name']}.$arg"] = true; + } elseif ($method === 'hasIndex') { + $this->guarded["idx:{$recv['name']}:$arg"] = true; + } + } + } + } + + private function isGuard(int $from, int $to): bool { + for ($i = $from; $i < $to; $i++) { + $t = $this->T[$i]; + if ($t['t'] === T_VARIABLE) { + if (!isset($this->vars[$t['s']]) || $this->vars[$t['s']]['k'] === 'other') { + return false; + } + } elseif ($t['t'] === T_STRING && ($this->T[$i + 1]['s'] ?? '') === '(') { + $prev = $this->T[$i - 1]['t'] ?? null; + if ($prev !== T_OBJECT_OPERATOR || !in_array($t['s'], self::GUARD_CALLS, true)) { + return false; + } + } + } + return true; + } + + /** @return list */ + private function splitArgs(int $open, int $close): array { + $args = []; + $start = $open + 1; + for ($i = $open + 1; $i < $close; $i++) { + if ($this->isOpen($i)) { + $i = $this->close($i); + } elseif ($this->T[$i]['s'] === ',') { + $args[] = [$start, $i]; + $start = $i + 1; + } + } + if ($start < $close) { + $args[] = [$start, $close]; + } + return $args; + } + + private function str(array $range): ?string { + [$a, $b] = $range; + if ($b - $a === 1 && $this->T[$a]['t'] === T_CONSTANT_ENCAPSED_STRING) { + $raw = $this->T[$a]['s']; + if ($raw[0] === '"' && str_contains($raw, '$')) { + return null; + } + return substr($raw, 1, -1); + } + if ($b - $a === 3 && $this->T[$a + 1]['t'] === T_DOUBLE_COLON && $this->T[$a + 2]['t'] === T_STRING + && in_array(strtolower($this->T[$a]['s']), ['self', 'static'], true)) { + return $this->consts[$this->T[$a + 2]['s']] ?? null; + } + return null; + } + + /** @return ?list */ + private function strList(array $range): ?array { + [$a, $b] = $range; + if ($this->T[$a]['s'] === '[' && $this->close($a) === $b - 1) { + $open = $a; + $close = $b - 1; + } elseif (strtolower($this->T[$a]['s']) === 'array' && ($this->T[$a + 1]['s'] ?? '') === '(' && $this->close($a + 1) === $b - 1) { + $open = $a + 1; + $close = $b - 1; + } else { + $single = $this->str($range); + return $single === null ? null : [$single]; + } + $out = []; + foreach ($this->splitArgs($open, $close) as $arg) { + $s = $this->str($arg); + if ($s === null) { + return null; + } + $out[] = $s; + } + return $out; + } + + private function statement(int $from, int $to): void { + if ($from >= $to) { + return; + } + $first = $this->T[$from]; + $touches = $this->mentions($from, $to); + + if ($first['t'] === T_RETURN) { + return; + } + $assignTo = null; + $chainFrom = $from; + if ($first['t'] === T_VARIABLE && ($this->T[$from + 1]['s'] ?? '') === '=') { + $assignTo = $first['s']; + $chainFrom = $from + 2; + } + $base = $this->T[$chainFrom] ?? null; + if ($base === null || $base['t'] !== T_VARIABLE) { + if ($touches) { + $this->touch($from, $to); + $this->reason('statement touching the schema is not understood', $from); + } + if ($assignTo !== null) { + $this->vars[$assignTo] = ['k' => 'other']; + } + return; + } + if ($base['s'] === '$schemaClosure') { + return; + } + if (!isset($this->vars[$base['s']]) || $this->vars[$base['s']]['k'] === 'other') { + if ($touches) { + $this->touch($from, $to); + $this->reason('schema passed to code that is not parsed', $from); + } + if ($assignTo !== null) { + $this->vars[$assignTo] = ['k' => 'other']; + } + return; + } + + // Parse the call chain: $var->call(...)->call(...) + $recv = $this->vars[$base['s']]; + $i = $chainFrom + 1; + while ($i < $to) { + if ($this->T[$i]['t'] !== T_OBJECT_OPERATOR || ($this->T[$i + 1]['t'] ?? null) !== T_STRING + || ($this->T[$i + 2]['s'] ?? '') !== '(') { + $this->touch($from, $to); + $this->reason('chain on the schema is not understood', $i); + $recv = ['k' => 'other']; + break; + } + $method = $this->T[$i + 1]['s']; + $close = $this->close($i + 2); + $args = $this->splitArgs($i + 2, $close); + $recv = $this->call($recv, $method, $args, $i); + $i = $close + 1; + } + if ($assignTo !== null) { + $this->vars[$assignTo] = $recv; + } + } + + /** + * @param array{k:string,name?:?string} $recv + * @param list $args + * @return array{k:string,name?:?string} + */ + private function call(array $recv, string $method, array $args, int $at): array { + $other = ['k' => 'other']; + $name = isset($args[0]) ? $this->str($args[0]) : null; + + if ($recv['k'] === 'schema') { + switch ($method) { + case 'getTable': + case 'createTable': + if ($method === 'createTable') { + $this->record(['op' => 'createTable', 'table' => $name], $at, [$name]); + } + return ['k' => 'table', 'name' => $name === null ? null : strtolower($name)]; + case 'dropTable': + $this->record(['op' => 'dropTable', 'table' => $name], $at, [$name]); + return $other; + case 'hasTable': + return $other; + default: + $this->touched[$name === null ? '*' : strtolower($name)] = true; + $this->reason("schema->$method() is not verifiable", $at); + return $other; + } + } + + if ($recv['k'] === 'column') { + if (str_starts_with($method, 'set')) { + $this->reason('column attribute change on ' . ($recv['name'] ?? '?') . ' is not verifiable', $at); + } + return $recv; + } + + if ($recv['k'] !== 'table') { + return $other; + } + $table = $recv['name'] ?? null; + $t = $table === null ? null : $table; + switch ($method) { + case 'addColumn': + $this->record(['op' => 'addColumn', 'table' => $t, 'column' => $name], $at, [$t, $name]); + return ['k' => 'column', 'name' => $t . '.' . $name]; + case 'dropColumn': + $this->record(['op' => 'dropColumn', 'table' => $t, 'column' => $name], $at, [$t, $name]); + return $other; + case 'renameColumn': + $new = isset($args[1]) ? $this->str($args[1]) : null; + $this->record(['op' => 'dropColumn', 'table' => $t, 'column' => $name], $at, [$t, $name]); + $this->record(['op' => 'addColumn', 'table' => $t, 'column' => $new], $at, [$t, $new]); + return $other; + case 'getColumn': + return ['k' => 'column', 'name' => $t . '.' . $name]; + case 'changeColumn': + case 'modifyColumn': + $this->reason("column attribute change on $t.$name is not verifiable", $at); + return $other; + case 'addIndex': + case 'addUniqueIndex': + $cols = isset($args[0]) ? $this->strList($args[0]) : null; + $idx = isset($args[1]) ? $this->str($args[1]) : null; + $this->record(['op' => 'addIndex', 'table' => $t, 'columns' => $cols, 'index' => $idx], $at, [$t, $cols]); + return $other; + case 'setPrimaryKey': + $cols = isset($args[0]) ? $this->strList($args[0]) : null; + $this->record(['op' => 'setPrimaryKey', 'table' => $t, 'columns' => $cols], $at, [$t, $cols]); + return $other; + case 'dropPrimaryKey': + $this->record(['op' => 'dropPrimaryKey', 'table' => $t], $at, [$t]); + return $other; + case 'dropIndex': + case 'removeUniqueConstraint': + $this->record(['op' => 'dropIndex', 'table' => $t, 'index' => $name], $at, [$t, $name]); + return $other; + default: + if (!in_array($method, self::IGNORED_TABLE_CALLS, true)) { + $this->reason("table->$method() is not verifiable", $at); + } + return $other; + } + } + + /** @param array $effect @param list $required */ + private function record(array $effect, int $at, array $required): void { + foreach ($required as $value) { + if ($value === null) { + $this->reason($effect['op'] . ' with a name that is computed at runtime is not verifiable', $at); + return; + } + } + foreach (['table', 'column', 'index'] as $key) { + if (isset($effect[$key])) { + $effect[$key] = strtolower((string)$effect[$key]); + } + } + if (isset($effect['columns'])) { + $effect['columns'] = array_map('strtolower', $effect['columns']); + } + if ($this->conditional && !isset($this->guarded[effectKey($effect)])) { + $this->touched[$effect['table']] = true; + $this->reasons['some schema changes follow a conditional early return and are not verifiable'] = true; + return; + } + $effect['line'] = $this->T[$at]['l']; + $this->effects[] = $effect; + } +} + +// --------------------------------------------------------------------------- +// Verification against the live schema +// --------------------------------------------------------------------------- + +function effectKey(array $e): string { + return match ($e['op']) { + 'addColumn', 'dropColumn' => "col:{$e['table']}.{$e['column']}", + 'createTable', 'dropTable' => "tbl:{$e['table']}", + 'setPrimaryKey', 'dropPrimaryKey' => "idx:{$e['table']}:PRIMARY", + 'addIndex', 'dropIndex' => isset($e['index']) + ? "idx:{$e['table']}:{$e['index']}" + : "idxc:{$e['table']}:" . implode(',', $e['columns'] ?? []), + }; +} + +function describeEffect(array $e): string { + return match ($e['op']) { + 'addColumn', 'dropColumn' => "{$e['op']} {$e['table']}.{$e['column']}", + 'createTable', 'dropTable' => "{$e['op']} {$e['table']}", + 'setPrimaryKey' => "setPrimaryKey {$e['table']}(" . implode(',', $e['columns']) . ')', + 'dropPrimaryKey' => "dropPrimaryKey {$e['table']}", + 'addIndex' => "addIndex {$e['table']}." . ($e['index'] ?? '(' . implode(',', $e['columns']) . ')'), + 'dropIndex' => "dropIndex {$e['table']}.{$e['index']}", + }; +} + +/** True if a later effect F makes the state asserted by earlier effect E irrelevant. */ +function supersedes(array $later, array $earlier): bool { + if (effectKey($later) === effectKey($earlier)) { + return true; + } + return in_array($later['op'], ['createTable', 'dropTable'], true) && $later['table'] === $earlier['table']; +} + +/** @return ?string problem description, null if the effect is in place */ +function checkEffect(array $e, array $schema): ?string { + $table = $e['table']; + $tableExists = isset($schema['columns'][$table]); + $indexes = $schema['indexes'][$table] ?? []; + switch ($e['op']) { + case 'createTable': + return $tableExists ? null : 'table missing'; + case 'dropTable': + return $tableExists ? 'table still present' : null; + case 'addColumn': + if (!$tableExists) { + return 'table missing'; + } + return isset($schema['columns'][$table][$e['column']]) ? null : 'column missing'; + case 'dropColumn': + return $tableExists && isset($schema['columns'][$table][$e['column']]) ? 'column still present' : null; + case 'setPrimaryKey': + if (!$tableExists) { + return 'table missing'; + } + return ($indexes['PRIMARY'] ?? null) === $e['columns'] ? null : 'primary key missing or different'; + case 'dropPrimaryKey': + return isset($indexes['PRIMARY']) ? 'primary key still present' : null; + case 'addIndex': + if (!$tableExists) { + return 'table missing'; + } + foreach ($indexes as $name => $cols) { + if (isset($e['index']) && strtolower((string)$name) === $e['index']) { + return null; + } + // Doctrine drops an index that a wider index starting with the same columns makes + // redundant, so such a covering index counts as in effect. + if (isset($e['columns']) && array_slice($cols, 0, count($e['columns'])) === $e['columns']) { + return null; + } + } + return 'index missing'; + case 'dropIndex': + foreach ($indexes as $name => $cols) { + if (strtolower((string)$name) === $e['index']) { + return 'index still present'; + } + } + return null; + } + return null; +} + +/** Migration directory of an app; core keeps its migrations in core/Migrations. */ +function migrationDir(string $app, string $ncRoot, ?string $appPath, array $config): ?string { + if ($app === 'core') { + return "$ncRoot/core/Migrations"; + } + $dir = $appPath ?? findAppDir($app, $ncRoot, $config, false); + return $dir === null ? null : "$dir/lib/Migration"; +} + +/** + * Compare one app's migrations with the live state and print the report. + * @param array $expected version => php source + * @param array $applied + * @return bool true if something is missing or not in effect + */ +function checkApp(string $app, array $expected, array $applied, array $schema, string $prefix, string $source, bool $verbose, bool $strict, bool $compact): bool { + $neverApplied = []; + $notInApp = []; + foreach (array_keys($expected) as $v) { + if (!isset($applied[$v])) { + $neverApplied[] = $v; + } + } + foreach (array_keys($applied) as $v) { + if (!isset($expected[$v])) { + $notInApp[] = $v; + } + } + usort($notInApp, 'compareVersions'); + + // Extract effects of applied migrations, in execution order. + $extracted = []; + foreach ($expected as $v => $src) { + if (isset($applied[$v])) { + $extracted[$v] = EffectExtractor::extract($src); + } + } + $flat = []; + foreach ($extracted as $v => $x) { + foreach ($x['effects'] as $e) { + $flat[] = [$v, $e]; + } + } + + $notInEffect = []; + $verified = []; + $superseded = []; + foreach ($flat as $pos => [$v, $e]) { + $isSuperseded = false; + for ($later = $pos + 1; $later < count($flat); $later++) { + if (supersedes($flat[$later][1], $e)) { + $superseded[$v][] = describeEffect($e) . ' (by ' . $flat[$later][0] . ')'; + $isSuperseded = true; + break; + } + } + if ($isSuperseded) { + continue; + } + // A later migration with code we cannot interpret may have changed this table again. + foreach ($extracted as $laterVersion => $x) { + if (compareVersions($laterVersion, $v) > 0 + && (in_array($e['table'], $x['touched'], true) || in_array('*', $x['touched'], true))) { + $superseded[$v][] = describeEffect($e) . " (table may be changed by $laterVersion, not verifiable)"; + continue 2; + } + } + $problem = checkEffect($e, $schema); + if ($problem !== null) { + // One line per missing table instead of one per column/index of it. + $line = $problem === 'table missing' ? "table {$e['table']} missing" : describeEffect($e) . ': ' . $problem; + if (!in_array($line, $notInEffect[$v] ?? [], true)) { + $notInEffect[$v][] = $line; + } + } else { + $verified[$v][] = describeEffect($e); + } + } + + $unverifiable = []; + foreach ($extracted as $v => $x) { + if ($x['reasons'] !== []) { + $unverifiable[$v] = $x['reasons']; + } + } + + $bad = $neverApplied !== [] || $notInEffect !== [] || ($strict && $unverifiable !== []); + if ($compact && !$bad) { + echo "app=$app OK expected=" . count($expected) . ' not_verifiable=' . count($unverifiable) . "\n"; + if (!$verbose) { + return false; + } + } else { + echo "app=$app prefix=$prefix source=$source\n"; + echo 'expected=' . count($expected) . ' applied=' . count($applied) . "\n\n"; + } + + echo '== Never applied (' . count($neverApplied) . ") ==\n"; + foreach ($neverApplied as $v) { + echo "app='$app', version='$v'\n"; + } + echo "\n== Applied but not in effect (" . count($notInEffect) . ") ==\n"; + foreach ($notInEffect as $v => $problems) { + echo "app='$app', version='$v'\n"; + foreach ($problems as $p) { + echo " $p\n"; + } + } + echo "\n== Not verifiable (" . count($unverifiable) . ") ==\n"; + foreach ($unverifiable as $v => $reasons) { + if ($compact && !$verbose) { + break; + } + echo "app='$app', version='$v'" . (isset($verified[$v]) || isset($notInEffect[$v]) ? ' (partially verified)' : '') . "\n"; + foreach ($reasons as $r) { + echo " $r\n"; + } + } + if ($notInApp !== []) { + echo "\n== Applied but not in the app directory (" . count($notInApp) . ", informational) ==\n"; + foreach ($notInApp as $v) { + echo "app='$app', version='$v'\n"; + } + } + if ($verbose) { + echo "\n== Verified in effect ==\n"; + foreach ($verified as $v => $list) { + echo "$v\n"; + foreach ($list as $d) { + echo " $d\n"; + } + } + echo "\n== Not checked: superseded or possibly changed by a later migration ==\n"; + foreach ($superseded as $v => $list) { + echo "$v\n"; + foreach ($list as $d) { + echo " $d\n"; + } + } + } + $nVerified = array_sum(array_map('count', $verified)); + echo "\nsummary: never_applied=" . count($neverApplied) . ' not_in_effect=' . count($notInEffect) + . ' not_verifiable=' . count($unverifiable) . " effects_verified=$nVerified\n"; + + return $bad; +} + +// --------------------------------------------------------------------------- +// Main +// --------------------------------------------------------------------------- + +function main(array $argv): int { + $opts = getopt('h', [ + 'app:', 'app-path:', 'db', 'applied:', 'schema:', 'dump-applied', 'dump-schema', + 'strict', 'verbose', 'help', 'config:', 'sqlite-file:', 'all' + ], $rest); + if (isset($opts['h']) || isset($opts['help'])) { + echo usage(); + return EXIT_OK; + } + if ($rest < count($argv) - 1) { + fail("unknown or malformed arguments\n\n" . usage()); + } + $all = isset($opts['all']); + if ($all && (isset($opts['app']) || isset($opts['app-path']) || !isset($opts['db']))) { + fail('--all needs --db and cannot be combined with --app or --app-path'); + } + $app = (string)($opts['app'] ?? 'spreed'); + if (preg_match('/^[a-z0-9_]+$/', $app) !== 1) { + fail('invalid --app'); + } + $useDb = isset($opts['db']); + $ncRoot = dirname(__DIR__, 2); + $config = []; + $prefix = 'oc_'; + if ($useDb) { + $config = loadNcConfig((string)($opts['config'] ?? "$ncRoot/config")); + $prefix = (string)($config['dbtableprefix'] ?? 'oc_'); + } + $sqliteFile = isset($opts['sqlite-file']) ? (string)$opts['sqlite-file'] : null; + + if (isset($opts['dump-schema']) || isset($opts['dump-applied'])) { + if (!$useDb) { + fail('--dump-* requires --db'); + } + $pdo = connectDb($config, $sqliteFile); + if (isset($opts['dump-applied'])) { + foreach (array_keys(dbApplied($pdo, $app, $prefix)) as $v) { + echo $v . "\n"; + } + } else { + dumpSchema($pdo, $prefix); + } + return EXIT_OK; + } + + if ($useDb === isset($opts['applied']) || $useDb === isset($opts['schema'])) { + fail("use either --db or both --applied and --schema\n\n" . usage()); + } + + $verbose = isset($opts['verbose']); + $strict = isset($opts['strict']); + $appPath = isset($opts['app-path']) ? (string)$opts['app-path'] : null; + if ($appPath !== null && !is_dir($appPath)) { + fail('app directory not found: ' . $appPath); + } + $cfgForLookup = $config ?: loadNcConfig("$ncRoot/config", false); + + if ($useDb) { + $pdo = connectDb($config, $sqliteFile); + $schema = dbSchema($pdo, $prefix); + $source = 'database'; + $appliedByApp = $all ? dbAppliedAll($pdo, $prefix) : [$app => dbApplied($pdo, $app, $prefix)]; + } else { + $schema = readSchemaFile((string)$opts['schema'], $prefix); + $source = 'files'; + $appliedByApp = [$app => readAppliedFile((string)$opts['applied'])]; + } + + $bad = false; + $skipped = []; + foreach ($appliedByApp as $name => $applied) { + $dir = migrationDir($name, $ncRoot, $all ? null : $appPath, $cfgForLookup); + if ($dir === null || !is_dir($dir)) { + if (!$all) { + fail("no migration directory for app $name (use --app-path)"); + } + $skipped[] = $name; + continue; + } + $expected = dirMigrations($dir); + if ($expected === [] && !$all) { + fail("no migrations found in $dir"); + } + $bad = checkApp($name, $expected, $applied, $schema, $prefix, $source, $verbose, $strict, $all) || $bad; + if ($all) { + echo "\n"; + } + } + if ($skipped !== []) { + echo 'skipped (not on disk, e.g. disabled or removed apps): ' . implode(', ', $skipped) . "\n"; + } + return $bad ? EXIT_FINDINGS : EXIT_OK; +} + +exit(main($argv)); From 152f2e08f7fb64df9718ab59c97328eaa39eaa3d Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 12:53:58 +0200 Subject: [PATCH 02/18] feat(admin-tools): limit check-app-migrations to an upgrade window Findings from migrations that predate an upgrade are noise when the question is whether the 31 to 32 to 33 upgrade ran correctly. --since-root takes an older Nextcloud tree and reports only migrations that do not exist there; --since-date filters by the date stamp in the migration version. Never applied, not in effect and not verifiable are all limited to that scope. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 6 +++- admin-tools/check-app-migrations.php | 43 ++++++++++++++++++++++++---- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/admin-tools/README.md b/admin-tools/README.md index 2280aa3..7418a07 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -111,13 +111,17 @@ The database connection is taken from the Nextcloud config (`config/config.php` # Every app that has rows in oc_migrations (core included; apps not on disk are skipped) ./check-app-migrations.php --all --db +# Only what an upgrade from 31 should have run: point at a 31 Nextcloud tree +./check-app-migrations.php --all --db --since-root /path/to/nextcloud-31 +./check-app-migrations.php --all --db --since-date 20250101 # or by migration date + # Check elsewhere from dumps taken in the pod ./check-app-migrations.php --db --dump-applied > applied.txt ./check-app-migrations.php --db --dump-schema > schema.txt ./check-app-migrations.php --applied applied.txt --schema schema.txt ``` -**Options:** `--app` (default `spreed`), `--all` (all apps; clean apps get one line, details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR`, `--sqlite-file FILE`, `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). +**Options:** `--app` (default `spreed`), `--all` (all apps; clean apps get one line, details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR`, `--sqlite-file FILE`, `--since-root DIR` (only migrations that do not exist in that older tree; apps missing there count fully), `--since-date YYYYMMDD` (only migrations dated on or after), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). **Exit codes:** 0 nothing wrong, 1 never applied or not in effect found, 2 usage/input error. Output contains only versions and table, column and index names. Sessions are read-only (`SET SESSION TRANSACTION READ ONLY` / `PRAGMA query_only`), and connection errors are reported by code only so no credentials leak. diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index a4092d8..ef61c23 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -45,6 +45,9 @@ function usage(): string { --schema FILE Schema dump as produced by --dump-schema --dump-applied Print the applied versions of APP and exit --dump-schema Print the live schema (table/column/index names) and exit + --since-root DIR Only migrations missing from this older Nextcloud tree (e.g. a 31 + checkout): what an upgrade from it should have run + --since-date D Only migrations dated on or after D (YYYYMMDD) --strict Treat "not verifiable" migrations as findings too --verbose Also list verified and superseded schema effects -h, --help This help @@ -1018,6 +1021,34 @@ function checkEffect(array $e, array $schema): ?string { return null; } +/** + * Limit the report to migrations that belong to an upgrade window: + * --since-root DIR Nextcloud tree from before the upgrade (e.g. the 31 release); + * migrations that already exist there are out of scope + * --since-date D only migrations whose Date stamp is on or after D (YYYYMMDD) + */ +function scopeFilter(string $app, array $opts, string $ncRoot): ?Closure { + $old = null; + if (isset($opts['since-root'])) { + $root = rtrim((string)$opts['since-root'], '/'); + $dir = migrationDir($app, $root, null, []); + $old = $dir !== null && is_dir($dir) ? dirMigrations($dir) : []; + } + $since = isset($opts['since-date']) ? (string)$opts['since-date'] : null; + if ($since !== null && preg_match('/^\d{8}$/', $since) !== 1) { + fail('--since-date must be YYYYMMDD'); + } + if ($old === null && $since === null) { + return null; + } + return static function (string $version) use ($old, $since): bool { + if ($old !== null && isset($old[$version])) { + return false; + } + return $since === null || (preg_match('/Date(\d{8})/', $version, $m) === 1 && $m[1] >= $since); + }; +} + /** Migration directory of an app; core keeps its migrations in core/Migrations. */ function migrationDir(string $app, string $ncRoot, ?string $appPath, array $config): ?string { if ($app === 'core') { @@ -1033,11 +1064,11 @@ function migrationDir(string $app, string $ncRoot, ?string $appPath, array $conf * @param array $applied * @return bool true if something is missing or not in effect */ -function checkApp(string $app, array $expected, array $applied, array $schema, string $prefix, string $source, bool $verbose, bool $strict, bool $compact): bool { +function checkApp(string $app, array $expected, array $applied, array $schema, string $prefix, string $source, bool $verbose, bool $strict, bool $compact, ?Closure $inScope = null): bool { $neverApplied = []; $notInApp = []; foreach (array_keys($expected) as $v) { - if (!isset($applied[$v])) { + if (!isset($applied[$v]) && ($inScope === null || $inScope($v))) { $neverApplied[] = $v; } } @@ -1089,7 +1120,7 @@ function checkApp(string $app, array $expected, array $applied, array $schema, s if ($problem !== null) { // One line per missing table instead of one per column/index of it. $line = $problem === 'table missing' ? "table {$e['table']} missing" : describeEffect($e) . ': ' . $problem; - if (!in_array($line, $notInEffect[$v] ?? [], true)) { + if (($inScope === null || $inScope($v)) && !in_array($line, $notInEffect[$v] ?? [], true)) { $notInEffect[$v][] = $line; } } else { @@ -1099,7 +1130,7 @@ function checkApp(string $app, array $expected, array $applied, array $schema, s $unverifiable = []; foreach ($extracted as $v => $x) { - if ($x['reasons'] !== []) { + if ($x['reasons'] !== [] && ($inScope === null || $inScope($v))) { $unverifiable[$v] = $x['reasons']; } } @@ -1172,7 +1203,7 @@ function checkApp(string $app, array $expected, array $applied, array $schema, s function main(array $argv): int { $opts = getopt('h', [ 'app:', 'app-path:', 'db', 'applied:', 'schema:', 'dump-applied', 'dump-schema', - 'strict', 'verbose', 'help', 'config:', 'sqlite-file:', 'all' + 'strict', 'verbose', 'help', 'config:', 'sqlite-file:', 'all', 'since-root:', 'since-date:' ], $rest); if (isset($opts['h']) || isset($opts['help'])) { echo usage(); @@ -1252,7 +1283,7 @@ function main(array $argv): int { if ($expected === [] && !$all) { fail("no migrations found in $dir"); } - $bad = checkApp($name, $expected, $applied, $schema, $prefix, $source, $verbose, $strict, $all) || $bad; + $bad = checkApp($name, $expected, $applied, $schema, $prefix, $source, $verbose, $strict, $all, scopeFilter($name, $opts, $ncRoot)) || $bad; if ($all) { echo "\n"; } From a5e7f03bfa0a0f60dbd8345a0b6d942b6a406b2f Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 12:56:18 +0200 Subject: [PATCH 03/18] feat(admin-tools): print occ commands to repair check-app-migrations findings Admins need the exact commands to fix what the check reports. When migrations are missing or not in effect, the report now ends with the occ migrations:execute commands in execution order and the steps around them. The script still only prints them and changes nothing. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 2 ++ admin-tools/check-app-migrations.php | 32 ++++++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/admin-tools/README.md b/admin-tools/README.md index 7418a07..fc2e9c0 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -123,6 +123,8 @@ The database connection is taken from the Nextcloud config (`config/config.php` **Options:** `--app` (default `spreed`), `--all` (all apps; clean apps get one line, details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR`, `--sqlite-file FILE`, `--since-root DIR` (only migrations that do not exist in that older tree; apps missing there count fully), `--since-date YYYYMMDD` (only migrations dated on or after), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). +When something is found, the report ends with a "Suggested fix" block: the `occ migrations:execute ` commands in execution order, plus the steps around them (backup, maintenance mode, the temporary debug config that `migrations:execute` needs). The script only prints them and never runs anything. + **Exit codes:** 0 nothing wrong, 1 never applied or not in effect found, 2 usage/input error. Output contains only versions and table, column and index names. Sessions are read-only (`SET SESSION TRANSACTION READ ONLY` / `PRAGMA query_only`), and connection errors are reported by code only so no credentials leak. **Requires:** PHP with `pdo_mysql` or `pdo_sqlite` (for `--db`). No git needed. diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index ef61c23..8d87ecf 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -1049,6 +1049,27 @@ function scopeFilter(string $app, array $opts, string $ncRoot): ?Closure { }; } +/** + * Print the commands an admin has to run to repair the findings. Nothing is executed here. + * @param array> $fixes app => versions in execution order + */ +function printFix(array $fixes): void { + if ($fixes === []) { + return; + } + echo "== Suggested fix (run by an admin, this script changes nothing) ==\n"; + echo "# Before: take a DB backup/snapshot and enable maintenance mode: occ maintenance:mode --on\n"; + echo "# migrations:execute exists only with debug on; use a temporary config file and remove it afterwards:\n"; + echo "# printf ' true];\\n' > config/zz-debug.config.php\n"; + echo "# Run in this order; only migrations that are missing or not in effect are listed:\n"; + foreach ($fixes as $app => $versions) { + foreach ($versions as $v) { + echo "occ migrations:execute $app $v\n"; + } + } + echo "# After: rm config/zz-debug.config.php; occ maintenance:mode --off; then re-run this check\n\n"; +} + /** Migration directory of an app; core keeps its migrations in core/Migrations. */ function migrationDir(string $app, string $ncRoot, ?string $appPath, array $config): ?string { if ($app === 'core') { @@ -1064,7 +1085,7 @@ function migrationDir(string $app, string $ncRoot, ?string $appPath, array $conf * @param array $applied * @return bool true if something is missing or not in effect */ -function checkApp(string $app, array $expected, array $applied, array $schema, string $prefix, string $source, bool $verbose, bool $strict, bool $compact, ?Closure $inScope = null): bool { +function checkApp(string $app, array $expected, array $applied, array $schema, string $prefix, string $source, bool $verbose, bool $strict, bool $compact, ?Closure $inScope = null, array &$fixes = []): bool { $neverApplied = []; $notInApp = []; foreach (array_keys($expected) as $v) { @@ -1136,6 +1157,11 @@ function checkApp(string $app, array $expected, array $applied, array $schema, s } $bad = $neverApplied !== [] || $notInEffect !== [] || ($strict && $unverifiable !== []); + $toRun = array_values(array_unique(array_merge($neverApplied, array_keys($notInEffect)))); + usort($toRun, 'compareVersions'); + if ($toRun !== []) { + $fixes[$app] = $toRun; + } if ($compact && !$bad) { echo "app=$app OK expected=" . count($expected) . ' not_verifiable=' . count($unverifiable) . "\n"; if (!$verbose) { @@ -1270,6 +1296,7 @@ function main(array $argv): int { $bad = false; $skipped = []; + $fixes = []; foreach ($appliedByApp as $name => $applied) { $dir = migrationDir($name, $ncRoot, $all ? null : $appPath, $cfgForLookup); if ($dir === null || !is_dir($dir)) { @@ -1283,11 +1310,12 @@ function main(array $argv): int { if ($expected === [] && !$all) { fail("no migrations found in $dir"); } - $bad = checkApp($name, $expected, $applied, $schema, $prefix, $source, $verbose, $strict, $all, scopeFilter($name, $opts, $ncRoot)) || $bad; + $bad = checkApp($name, $expected, $applied, $schema, $prefix, $source, $verbose, $strict, $all, scopeFilter($name, $opts, $ncRoot), $fixes) || $bad; if ($all) { echo "\n"; } } + printFix($fixes); if ($skipped !== []) { echo 'skipped (not on disk, e.g. disabled or removed apps): ' . implode(', ', $skipped) . "\n"; } From a4eed948cded77bda905c1b249e8376993a3b877 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 13:05:24 +0200 Subject: [PATCH 04/18] feat(admin-tools): scope check-app-migrations by Nextcloud version An upgrade window is easier to name by Nextcloud version than by a date or an old tree. --since-nc N limits the report to migrations written after stable N was cut from master, so --since-nc 31 covers the 31 to 32 to 33 path. The cut dates come from nextcloud/server (merge-base of stableN and master) and replace --since-root and --since-date. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 7 ++-- admin-tools/check-app-migrations.php | 54 ++++++++++++++-------------- 2 files changed, 30 insertions(+), 31 deletions(-) diff --git a/admin-tools/README.md b/admin-tools/README.md index fc2e9c0..fa4bbe5 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -111,9 +111,8 @@ The database connection is taken from the Nextcloud config (`config/config.php` # Every app that has rows in oc_migrations (core included; apps not on disk are skipped) ./check-app-migrations.php --all --db -# Only what an upgrade from 31 should have run: point at a 31 Nextcloud tree -./check-app-migrations.php --all --db --since-root /path/to/nextcloud-31 -./check-app-migrations.php --all --db --since-date 20250101 # or by migration date +# Only what an upgrade from Nextcloud 31 (31 -> 32 -> 33) should have run +./check-app-migrations.php --all --db --since-nc 31 # Check elsewhere from dumps taken in the pod ./check-app-migrations.php --db --dump-applied > applied.txt @@ -121,7 +120,7 @@ The database connection is taken from the Nextcloud config (`config/config.php` ./check-app-migrations.php --applied applied.txt --schema schema.txt ``` -**Options:** `--app` (default `spreed`), `--all` (all apps; clean apps get one line, details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR`, `--sqlite-file FILE`, `--since-root DIR` (only migrations that do not exist in that older tree; apps missing there count fully), `--since-date YYYYMMDD` (only migrations dated on or after), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). +**Options:** `--app` (default `spreed`), `--all` (all apps; clean apps get one line, details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR`, `--sqlite-file FILE`, `--since-nc N` (only migrations written after Nextcloud N was branched, e.g. `31`; derived from the date stamp in the migration version, so approximate for apps that are branched on their own schedule), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). When something is found, the report ends with a "Suggested fix" block: the `occ migrations:execute ` commands in execution order, plus the steps around them (backup, maintenance mode, the temporary debug config that `migrations:execute` needs). The script only prints them and never runs anything. diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index 8d87ecf..ca4ba63 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -45,9 +45,9 @@ function usage(): string { --schema FILE Schema dump as produced by --dump-schema --dump-applied Print the applied versions of APP and exit --dump-schema Print the live schema (table/column/index names) and exit - --since-root DIR Only migrations missing from this older Nextcloud tree (e.g. a 31 - checkout): what an upgrade from it should have run - --since-date D Only migrations dated on or after D (YYYYMMDD) + --since-nc N Only migrations written after Nextcloud N was branched (stable N cut from + master), i.e. what an upgrade from N to a later release should have run, + e.g. --since-nc 31 for the 31 -> 32 -> 33 path. Known: 28 to 33. --strict Treat "not verifiable" migrations as findings too --verbose Also list verified and superseded schema effects -h, --help This help @@ -1022,31 +1022,31 @@ function checkEffect(array $e, array $schema): ?string { } /** - * Limit the report to migrations that belong to an upgrade window: - * --since-root DIR Nextcloud tree from before the upgrade (e.g. the 31 release); - * migrations that already exist there are out of scope - * --since-date D only migrations whose Date stamp is on or after D (YYYYMMDD) + * Date (YYYYMMDD) on which stable was cut from master in nextcloud/server + * (merge-base of stable and master). Migrations are stamped with the date they were written, so + * one dated on or after this day is not part of Nextcloud N. This is an approximation: apps are + * branched on their own schedule, so a migration written shortly before an app's own cut can be missed. */ -function scopeFilter(string $app, array $opts, string $ncRoot): ?Closure { - $old = null; - if (isset($opts['since-root'])) { - $root = rtrim((string)$opts['since-root'], '/'); - $dir = migrationDir($app, $root, null, []); - $old = $dir !== null && is_dir($dir) ? dirMigrations($dir) : []; - } - $since = isset($opts['since-date']) ? (string)$opts['since-date'] : null; - if ($since !== null && preg_match('/^\d{8}$/', $since) !== 1) { - fail('--since-date must be YYYYMMDD'); - } - if ($old === null && $since === null) { +const NC_BRANCH_DATES = [ + 28 => '20231123', + 29 => '20240328', + 30 => '20240814', + 31 => '20250123', + 32 => '20250904', + 33 => '20260122', +]; + +/** Limit the report to migrations that an upgrade from the given Nextcloud major version should have run. */ +function scopeFilter(array $opts): ?Closure { + if (!isset($opts['since-nc'])) { return null; } - return static function (string $version) use ($old, $since): bool { - if ($old !== null && isset($old[$version])) { - return false; - } - return $since === null || (preg_match('/Date(\d{8})/', $version, $m) === 1 && $m[1] >= $since); - }; + $nc = (int)$opts['since-nc']; + if (!isset(NC_BRANCH_DATES[$nc])) { + fail('--since-nc must be one of ' . implode(', ', array_keys(NC_BRANCH_DATES))); + } + $since = NC_BRANCH_DATES[$nc]; + return static fn (string $version): bool => preg_match('/Date(\d{8})/', $version, $m) === 1 && $m[1] >= $since; } /** @@ -1229,7 +1229,7 @@ function checkApp(string $app, array $expected, array $applied, array $schema, s function main(array $argv): int { $opts = getopt('h', [ 'app:', 'app-path:', 'db', 'applied:', 'schema:', 'dump-applied', 'dump-schema', - 'strict', 'verbose', 'help', 'config:', 'sqlite-file:', 'all', 'since-root:', 'since-date:' + 'strict', 'verbose', 'help', 'config:', 'sqlite-file:', 'all', 'since-nc:' ], $rest); if (isset($opts['h']) || isset($opts['help'])) { echo usage(); @@ -1310,7 +1310,7 @@ function main(array $argv): int { if ($expected === [] && !$all) { fail("no migrations found in $dir"); } - $bad = checkApp($name, $expected, $applied, $schema, $prefix, $source, $verbose, $strict, $all, scopeFilter($name, $opts, $ncRoot), $fixes) || $bad; + $bad = checkApp($name, $expected, $applied, $schema, $prefix, $source, $verbose, $strict, $all, scopeFilter($opts), $fixes) || $bad; if ($all) { echo "\n"; } From 5b537b09a7db730a1ce8dfdcb9a9130803525940 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 13:07:42 +0200 Subject: [PATCH 05/18] feat(admin-tools): structure the check-app-migrations --all report The per-app output of --all was hard to scan on a live instance: clean apps and apps with findings were interleaved and repeated headers. --all now prints a header with the scope, a table of the apps with findings, the details only for those apps, one wrapped list of clean apps, and the suggested fix at the end. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 2 +- admin-tools/check-app-migrations.php | 128 +++++++++++++++++++++++---- 2 files changed, 111 insertions(+), 19 deletions(-) diff --git a/admin-tools/README.md b/admin-tools/README.md index fa4bbe5..4c69412 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -120,7 +120,7 @@ The database connection is taken from the Nextcloud config (`config/config.php` ./check-app-migrations.php --applied applied.txt --schema schema.txt ``` -**Options:** `--app` (default `spreed`), `--all` (all apps; clean apps get one line, details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR`, `--sqlite-file FILE`, `--since-nc N` (only migrations written after Nextcloud N was branched, e.g. `31`; derived from the date stamp in the migration version, so approximate for apps that are branched on their own schedule), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). +**Options:** `--app` (default `spreed`), `--all` (all apps; an overview table and details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR`, `--sqlite-file FILE`, `--since-nc N` (only migrations written after Nextcloud N was branched, e.g. `31`; derived from the date stamp in the migration version, so approximate for apps that are branched on their own schedule), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). When something is found, the report ends with a "Suggested fix" block: the `occ migrations:execute ` commands in execution order, plus the steps around them (backup, maintenance mode, the temporary debug config that `migrations:execute` needs). The script only prints them and never runs anything. diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index ca4ba63..5234872 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -37,7 +37,7 @@ function usage(): string { Options: --app APP App id (default: spreed) --all Check every app that has rows in the migrations table (with --db); - clean apps get one line, details only for apps with findings + an overview table and details only for apps with findings --app-path DIR App directory (default: found via apps_paths of the Nextcloud config, else apps/, apps-external/, custom_apps/) --db Read applied list and schema from the database @@ -584,7 +584,7 @@ private function skipConstruct(int $i, int $end): int { } if ($this->mentions($start, $j)) { $this->touch($start, $j); - $this->reason('schema changes inside ' . strtolower(token_name((int)$this->T[$start]['t'])) . ' are not verifiable', $start); + $this->reason('schema changes inside ' . strtolower(substr(token_name((int)$this->T[$start]['t']), 2)) . ' are not verifiable', $start); } return $j; } @@ -1083,9 +1083,10 @@ function migrationDir(string $app, string $ncRoot, ?string $appPath, array $conf * Compare one app's migrations with the live state and print the report. * @param array $expected version => php source * @param array $applied - * @return bool true if something is missing or not in effect + * @return array{bad:bool,never:int,noeffect:int,unverifiable:int,verified:int,expected:int,text:string} + * text is the compact (--all) block for this app, empty when the app is clean */ -function checkApp(string $app, array $expected, array $applied, array $schema, string $prefix, string $source, bool $verbose, bool $strict, bool $compact, ?Closure $inScope = null, array &$fixes = []): bool { +function checkApp(string $app, array $expected, array $applied, array $schema, string $prefix, string $source, bool $verbose, bool $strict, bool $compact, ?Closure $inScope = null, array &$fixes = []): array { $neverApplied = []; $notInApp = []; foreach (array_keys($expected) as $v) { @@ -1162,15 +1163,23 @@ function checkApp(string $app, array $expected, array $applied, array $schema, s if ($toRun !== []) { $fixes[$app] = $toRun; } - if ($compact && !$bad) { - echo "app=$app OK expected=" . count($expected) . ' not_verifiable=' . count($unverifiable) . "\n"; - if (!$verbose) { - return false; + $stats = [ + 'bad' => $bad, + 'never' => count($neverApplied), + 'noeffect' => count($notInEffect), + 'unverifiable' => count($unverifiable), + 'verified' => array_sum(array_map('count', $verified)), + 'expected' => count($expected), + 'text' => '', + ]; + if ($compact) { + if ($bad || $verbose) { + $stats['text'] = compactBlock($app, $neverApplied, $notInEffect, $unverifiable, $notInApp, $verified, $superseded, $verbose); } - } else { - echo "app=$app prefix=$prefix source=$source\n"; - echo 'expected=' . count($expected) . ' applied=' . count($applied) . "\n\n"; + return $stats; } + echo "app=$app prefix=$prefix source=$source\n"; + echo 'expected=' . count($expected) . ' applied=' . count($applied) . "\n\n"; echo '== Never applied (' . count($neverApplied) . ") ==\n"; foreach ($neverApplied as $v) { @@ -1185,9 +1194,6 @@ function checkApp(string $app, array $expected, array $applied, array $schema, s } echo "\n== Not verifiable (" . count($unverifiable) . ") ==\n"; foreach ($unverifiable as $v => $reasons) { - if ($compact && !$verbose) { - break; - } echo "app='$app', version='$v'" . (isset($verified[$v]) || isset($notInEffect[$v]) ? ' (partially verified)' : '') . "\n"; foreach ($reasons as $r) { echo " $r\n"; @@ -1219,7 +1225,88 @@ function checkApp(string $app, array $expected, array $applied, array $schema, s echo "\nsummary: never_applied=" . count($neverApplied) . ' not_in_effect=' . count($notInEffect) . ' not_verifiable=' . count($unverifiable) . " effects_verified=$nVerified\n"; - return $bad; + return $stats; +} + +/** + * Overview for --all: totals, a table of the apps with findings, their details, then the clean apps. + * @param array> $results + */ +function printOverview(array $results, string $prefix, string $source, array $opts): void { + $bad = array_filter($results, static fn (array $r): bool => $r['bad']); + $ok = array_diff_key($results, $bad); + $scope = isset($opts['since-nc']) ? 'migrations after Nextcloud ' . $opts['since-nc'] . ' branched' : 'all migrations'; + echo "Migration check: source=$source prefix=$prefix scope=$scope\n"; + echo 'Apps checked: ' . count($results) . ', with findings: ' . count($bad) . ', clean: ' . count($ok) . "\n\n"; + + if ($bad !== []) { + $width = max(array_map('strlen', array_keys($bad))); + echo "== Apps with findings ==\n"; + printf(" %-{$width}s %12s %13s %14s\n", 'app', 'never applied', 'not in effect', 'not verifiable'); + foreach ($bad as $name => $r) { + printf(" %-{$width}s %12d %13d %14d\n", $name, $r['never'], $r['noeffect'], $r['unverifiable']); + } + echo "\n== Details ==\n"; + foreach ($bad as $r) { + echo $r['text'] . "\n"; + } + } + $detailsOfClean = array_filter($ok, static fn (array $r): bool => $r['text'] !== ''); + foreach ($detailsOfClean as $r) { + echo $r['text'] . "\n"; + } + if ($ok !== []) { + echo '== Clean apps (' . count($ok) . ") ==\n"; + echo wordwrap(' ' . implode(', ', array_keys($ok)), 100, "\n ") . "\n\n"; + } +} + +/** Per-app detail block for --all: only what is wrong, versions with their problems underneath. */ +function compactBlock(string $app, array $never, array $notInEffect, array $unverifiable, array $notInApp, array $verified, array $superseded, bool $verbose): string { + $out = "$app\n"; + if ($never !== []) { + $out .= " Never applied:\n"; + foreach ($never as $v) { + $out .= " $v\n"; + } + } + if ($notInEffect !== []) { + $out .= " Applied but not in effect:\n"; + foreach ($notInEffect as $v => $problems) { + $out .= " $v\n"; + foreach ($problems as $p) { + $out .= " - $p\n"; + } + } + } + if ($unverifiable !== []) { + if ($verbose) { + $out .= " Not verifiable:\n"; + foreach ($unverifiable as $v => $reasons) { + $out .= " $v" . (isset($verified[$v]) || isset($notInEffect[$v]) ? ' (partially verified)' : '') . "\n"; + foreach ($reasons as $r) { + $out .= " - $r\n"; + } + } + } else { + $out .= ' Not verifiable: ' . count($unverifiable) . " (use --verbose to list)\n"; + } + } + if ($notInApp !== []) { + $out .= ' Applied but not in the app directory (informational): ' . implode(', ', $notInApp) . "\n"; + } + if ($verbose) { + foreach (['Verified in effect' => $verified, 'Not checked (superseded or possibly changed later)' => $superseded] as $title => $groups) { + $out .= " $title:\n"; + foreach ($groups as $v => $list) { + $out .= " $v\n"; + foreach ($list as $d) { + $out .= " - $d\n"; + } + } + } + } + return $out; } // --------------------------------------------------------------------------- @@ -1297,6 +1384,7 @@ function main(array $argv): int { $bad = false; $skipped = []; $fixes = []; + $results = []; foreach ($appliedByApp as $name => $applied) { $dir = migrationDir($name, $ncRoot, $all ? null : $appPath, $cfgForLookup); if ($dir === null || !is_dir($dir)) { @@ -1310,14 +1398,18 @@ function main(array $argv): int { if ($expected === [] && !$all) { fail("no migrations found in $dir"); } - $bad = checkApp($name, $expected, $applied, $schema, $prefix, $source, $verbose, $strict, $all, scopeFilter($opts), $fixes) || $bad; + $result = checkApp($name, $expected, $applied, $schema, $prefix, $source, $verbose, $strict, $all, scopeFilter($opts), $fixes); + $bad = $result['bad'] || $bad; if ($all) { - echo "\n"; + $results[$name] = $result; } } + if ($all) { + printOverview($results, $prefix, $source, $opts); + } printFix($fixes); if ($skipped !== []) { - echo 'skipped (not on disk, e.g. disabled or removed apps): ' . implode(', ', $skipped) . "\n"; + echo 'Skipped (not on disk, e.g. disabled or removed apps): ' . implode(', ', $skipped) . "\n"; } return $bad ? EXIT_FINDINGS : EXIT_OK; } From 74c2cd68bd03d8f92ce4da241f6974843dff41bc Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 13:15:16 +0200 Subject: [PATCH 06/18] feat(admin-tools): use NC_debug for the suggested migrations:execute migrations:execute is only registered with debug on. Nextcloud reads NC_-prefixed environment variables as system config, so NC_debug=true enables it for a single command without writing a config file or turning debug on for the instance. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 2 +- admin-tools/check-app-migrations.php | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/admin-tools/README.md b/admin-tools/README.md index 4c69412..7cc4393 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -122,7 +122,7 @@ The database connection is taken from the Nextcloud config (`config/config.php` **Options:** `--app` (default `spreed`), `--all` (all apps; an overview table and details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR`, `--sqlite-file FILE`, `--since-nc N` (only migrations written after Nextcloud N was branched, e.g. `31`; derived from the date stamp in the migration version, so approximate for apps that are branched on their own schedule), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). -When something is found, the report ends with a "Suggested fix" block: the `occ migrations:execute ` commands in execution order, plus the steps around them (backup, maintenance mode, the temporary debug config that `migrations:execute` needs). The script only prints them and never runs anything. +When something is found, the report ends with a "Suggested fix" block: the `occ migrations:execute ` commands in execution order, plus the steps around them (backup, maintenance mode). `migrations:execute` only exists with debug on, so the commands are prefixed with `NC_debug=true`, which enables it for that one command and not for the instance. The script only prints them and never runs anything. **Exit codes:** 0 nothing wrong, 1 never applied or not in effect found, 2 usage/input error. Output contains only versions and table, column and index names. Sessions are read-only (`SET SESSION TRANSACTION READ ONLY` / `PRAGMA query_only`), and connection errors are reported by code only so no credentials leak. diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index 5234872..7674674 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -1059,15 +1059,14 @@ function printFix(array $fixes): void { } echo "== Suggested fix (run by an admin, this script changes nothing) ==\n"; echo "# Before: take a DB backup/snapshot and enable maintenance mode: occ maintenance:mode --on\n"; - echo "# migrations:execute exists only with debug on; use a temporary config file and remove it afterwards:\n"; - echo "# printf ' true];\\n' > config/zz-debug.config.php\n"; + echo "# migrations:execute is only available with debug on; NC_debug=true enables it for this one command only\n"; echo "# Run in this order; only migrations that are missing or not in effect are listed:\n"; foreach ($fixes as $app => $versions) { foreach ($versions as $v) { - echo "occ migrations:execute $app $v\n"; + echo "NC_debug=true occ migrations:execute $app $v\n"; } } - echo "# After: rm config/zz-debug.config.php; occ maintenance:mode --off; then re-run this check\n\n"; + echo "# After: occ maintenance:mode --off; then re-run this check\n\n"; } /** Migration directory of an app; core keeps its migrations in core/Migrations. */ From 2ede128f6af9dbe423481a099e19ea62a9ba7334 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 13:22:10 +0200 Subject: [PATCH 07/18] feat(admin-tools): honor NEXTCLOUD_CONFIG_DIR in check-app-migrations The config directory default is already the config/ of the Nextcloud root the script lives in, so --config is optional. Also respect NEXTCLOUD_CONFIG_DIR like Nextcloud itself does; --config still wins. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 2 +- admin-tools/check-app-migrations.php | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/admin-tools/README.md b/admin-tools/README.md index 7cc4393..4fafaca 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -120,7 +120,7 @@ The database connection is taken from the Nextcloud config (`config/config.php` ./check-app-migrations.php --applied applied.txt --schema schema.txt ``` -**Options:** `--app` (default `spreed`), `--all` (all apps; an overview table and details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR`, `--sqlite-file FILE`, `--since-nc N` (only migrations written after Nextcloud N was branched, e.g. `31`; derived from the date stamp in the migration version, so approximate for apps that are branched on their own schedule), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). +**Options:** `--app` (default `spreed`), `--all` (all apps; an overview table and details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR` (default: `NEXTCLOUD_CONFIG_DIR`, else the `config/` of the Nextcloud root the script lives in, not the current directory), `--sqlite-file FILE`, `--since-nc N` (only migrations written after Nextcloud N was branched, e.g. `31`; derived from the date stamp in the migration version, so approximate for apps that are branched on their own schedule), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). When something is found, the report ends with a "Suggested fix" block: the `occ migrations:execute ` commands in execution order, plus the steps around them (backup, maintenance mode). `migrations:execute` only exists with debug on, so the commands are prefixed with `NC_debug=true`, which enables it for that one command and not for the instance. The script only prints them and never runs anything. diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index 7674674..b01d730 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -54,7 +54,8 @@ function usage(): string { Database connection: taken from the Nextcloud config (config/config.php and config/*.config.php), nothing is passed on the command line or printed. - --config DIR Nextcloud config directory (default: /config) + --config DIR Nextcloud config directory (default: $NEXTCLOUD_CONFIG_DIR, else the + config/ of the Nextcloud root this script lives in; not the current directory) --sqlite-file F sqlite only: use this file instead of /owncloud.db Expected migrations: the Version*.php files in DIR/lib/Migration, i.e. what is @@ -1337,7 +1338,7 @@ function main(array $argv): int { $config = []; $prefix = 'oc_'; if ($useDb) { - $config = loadNcConfig((string)($opts['config'] ?? "$ncRoot/config")); + $config = loadNcConfig((string)(($opts['config'] ?? '') ?: getenv('NEXTCLOUD_CONFIG_DIR') ?: "$ncRoot/config")); $prefix = (string)($config['dbtableprefix'] ?? 'oc_'); } $sqliteFile = isset($opts['sqlite-file']) ? (string)$opts['sqlite-file'] : null; From f269647dcf9d134cf6ca55b7365e2c415c987564 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 13:45:10 +0200 Subject: [PATCH 08/18] fix(admin-tools): do not suggest re-running migrations that return null A migration whose changeSchema() ends with return null makes MigrationService::executeStep() skip the schema migration, so its changes are never applied and executing it again cannot create them. Talk 22001Date20250927174738 is such a migration. check-app-migrations now reports these as not verifiable with that reason instead of listing them as not in effect and in the suggested fix. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 2 +- admin-tools/check-app-migrations.php | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/admin-tools/README.md b/admin-tools/README.md index 4fafaca..c770bc1 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -97,7 +97,7 @@ Update email addresses in Nextcloud mail app configuration when a customer's ema Read-only check of an app's migrations against the database. Reports migrations that were **never applied** (no row in `oc_migrations`) and migrations that are **applied but not in effect** (row exists, but the columns, tables or indexes they create are missing, or what they drop is still there). Example: Talk failing with `Unknown column 'r.last_pinned_id'` although `23000Date20251030090219` is recorded as applied. -The expected migrations are the `Version*.php` files in the app's `lib/Migration`, i.e. what is deployed there. No git is used. Schema effects (`addColumn`, `createTable`, `addIndex`, `setPrimaryKey`, `dropColumn`, `dropTable`, `dropIndex`, `renameColumn`) are extracted statically from `changeSchema()`. `hasColumn`-style guards are understood. Effects that a later applied migration overrides are skipped. Anything it cannot parse (data changes in `preSchemaChange`/`postSchemaChange`, loops, helper calls, non-guard conditions, computed names) is listed under "Not verifiable" and never guessed. +The expected migrations are the `Version*.php` files in the app's `lib/Migration`, i.e. what is deployed there. No git is used. Schema effects (`addColumn`, `createTable`, `addIndex`, `setPrimaryKey`, `dropColumn`, `dropTable`, `dropIndex`, `renameColumn`) are extracted statically from `changeSchema()`. `hasColumn`-style guards are understood. Effects that a later applied migration overrides are skipped. A `changeSchema()` that ends with `return null;` is reported as not verifiable with a note: Nextcloud applies the schema only when a schema wrapper is returned, so such a migration never makes its changes (a defect in the app) and re-running it cannot fix them. Anything it cannot parse (data changes in `preSchemaChange`/`postSchemaChange`, loops, helper calls, non-guard conditions, computed names) is listed under "Not verifiable" and never guessed. The database connection is taken from the Nextcloud config (`config/config.php` plus `config/*.config.php`); nothing is passed on the command line. diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index b01d730..bb981b5 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -389,6 +389,8 @@ final class EffectExtractor { /** @var array */ public array $reasons = []; private bool $conditional = false; + /** True when changeSchema() ends with "return null;": Nextcloud then applies none of its changes. */ + private bool $returnsNull = false; /** @var array tables that code we could not interpret may have changed */ private array $touched = []; /** @var array effect keys that an early-return has*() guard already speaks about */ @@ -423,6 +425,12 @@ public static function extract(string $source): array { $self->vars = ['$schema' => ['k' => 'schema']]; $self->walk($methods['changeSchema'][0], $methods['changeSchema'][1], true); } + if ($self->returnsNull && $self->effects !== []) { + // MigrationService::executeStep() only migrates when a schema wrapper is returned, so these + // effects never happen and re-running the migration cannot create them. + $self->effects = []; + $self->reasons['changeSchema() ends with "return null;": Nextcloud discards its schema changes, they are never applied (defect in the app)'] = true; + } return [ 'effects' => $self->effects, 'reasons' => array_keys($self->reasons), @@ -540,6 +548,7 @@ private function walk(int $i, int $end, bool $top): void { $this->walk($i + 1, $close, false); $i = $close + 1; } elseif ($tok['t'] === T_RETURN && $top) { + $this->returnsNull = strtolower($this->T[$i + 1]['s'] ?? '') === 'null' && ($this->T[$i + 2]['s'] ?? '') === ';'; return; } else { $stmtEnd = $this->statementEnd($i, $end); From 3380c0b647f07899e714b257520f369e6a1f518e Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 13:48:43 +0200 Subject: [PATCH 09/18] fix(admin-tools): only treat a migration as discarded if every return is null A changeSchema() that returns the schema from a nested branch and ends with return null for the nothing-to-do case was wrongly reported as discarding its changes, e.g. core 33000Date20251013110519. Require that all returns are return null. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 2 +- admin-tools/check-app-migrations.php | 31 +++++++++++++++++++++++++--- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/admin-tools/README.md b/admin-tools/README.md index c770bc1..f23e464 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -97,7 +97,7 @@ Update email addresses in Nextcloud mail app configuration when a customer's ema Read-only check of an app's migrations against the database. Reports migrations that were **never applied** (no row in `oc_migrations`) and migrations that are **applied but not in effect** (row exists, but the columns, tables or indexes they create are missing, or what they drop is still there). Example: Talk failing with `Unknown column 'r.last_pinned_id'` although `23000Date20251030090219` is recorded as applied. -The expected migrations are the `Version*.php` files in the app's `lib/Migration`, i.e. what is deployed there. No git is used. Schema effects (`addColumn`, `createTable`, `addIndex`, `setPrimaryKey`, `dropColumn`, `dropTable`, `dropIndex`, `renameColumn`) are extracted statically from `changeSchema()`. `hasColumn`-style guards are understood. Effects that a later applied migration overrides are skipped. A `changeSchema()` that ends with `return null;` is reported as not verifiable with a note: Nextcloud applies the schema only when a schema wrapper is returned, so such a migration never makes its changes (a defect in the app) and re-running it cannot fix them. Anything it cannot parse (data changes in `preSchemaChange`/`postSchemaChange`, loops, helper calls, non-guard conditions, computed names) is listed under "Not verifiable" and never guessed. +The expected migrations are the `Version*.php` files in the app's `lib/Migration`, i.e. what is deployed there. No git is used. Schema effects (`addColumn`, `createTable`, `addIndex`, `setPrimaryKey`, `dropColumn`, `dropTable`, `dropIndex`, `renameColumn`) are extracted statically from `changeSchema()`. `hasColumn`-style guards are understood. Effects that a later applied migration overrides are skipped. A `changeSchema()` whose every `return` is `return null;` is reported as not verifiable with a note: Nextcloud applies the schema only when a schema wrapper is returned, so such a migration never makes its changes (a defect in the app) and re-running it cannot fix them. Anything it cannot parse (data changes in `preSchemaChange`/`postSchemaChange`, loops, helper calls, non-guard conditions, computed names) is listed under "Not verifiable" and never guessed. The database connection is taken from the Nextcloud config (`config/config.php` plus `config/*.config.php`); nothing is passed on the command line. diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index bb981b5..782debc 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -389,7 +389,7 @@ final class EffectExtractor { /** @var array */ public array $reasons = []; private bool $conditional = false; - /** True when changeSchema() ends with "return null;": Nextcloud then applies none of its changes. */ + /** True when every return in changeSchema() is "return null;": Nextcloud then applies none of its changes. */ private bool $returnsNull = false; /** @var array tables that code we could not interpret may have changed */ private array $touched = []; @@ -425,11 +425,14 @@ public static function extract(string $source): array { $self->vars = ['$schema' => ['k' => 'schema']]; $self->walk($methods['changeSchema'][0], $methods['changeSchema'][1], true); } + if (isset($methods['changeSchema'])) { + $self->returnsNull = $self->onlyReturnsNull(...$methods['changeSchema']); + } if ($self->returnsNull && $self->effects !== []) { // MigrationService::executeStep() only migrates when a schema wrapper is returned, so these // effects never happen and re-running the migration cannot create them. $self->effects = []; - $self->reasons['changeSchema() ends with "return null;": Nextcloud discards its schema changes, they are never applied (defect in the app)'] = true; + $self->reasons['changeSchema() only ever returns null: Nextcloud discards its schema changes, they are never applied (defect in the app)'] = true; } return [ 'effects' => $self->effects, @@ -481,6 +484,29 @@ private function findMethods(): array { return $methods; } + /** True if the range has at least one return and every return is "return null;" (no schema is ever handed back). */ + private function onlyReturnsNull(int $from, int $to): bool { + $returns = 0; + for ($i = $from; $i < $to; $i++) { + if ($this->T[$i]['t'] === T_FUNCTION || $this->T[$i]['t'] === T_FN) { + // closures have their own returns + $j = $i + 1; + while ($j < $to && $this->T[$j]['s'] !== '{' && $this->T[$j]['s'] !== ';') { + $j = $this->isOpen($j) ? $this->close($j) + 1 : $j + 1; + } + $i = ($j < $to && $this->T[$j]['s'] === '{') ? $this->close($j) : $j; + continue; + } + if ($this->T[$i]['t'] === T_RETURN) { + $returns++; + if (strtolower($this->T[$i + 1]['s'] ?? '') !== 'null' || ($this->T[$i + 2]['s'] ?? '') !== ';') { + return false; + } + } + } + return $returns > 0; + } + private function isTrivial(int $from, int $to): bool { $s = ''; for ($i = $from; $i < $to; $i++) { @@ -548,7 +574,6 @@ private function walk(int $i, int $end, bool $top): void { $this->walk($i + 1, $close, false); $i = $close + 1; } elseif ($tok['t'] === T_RETURN && $top) { - $this->returnsNull = strtolower($this->T[$i + 1]['s'] ?? '') === 'null' && ($this->T[$i + 2]['s'] ?? '') === ';'; return; } else { $stmtEnd = $this->statementEnd($i, $end); From b9b6e5b7f08c5f559871f0a0703bbd43c0c06eb1 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 14:04:45 +0200 Subject: [PATCH 10/18] style(admin-tools): satisfy php-cs-fixer and editorconfig in check-app-migrations The usage heredoc started lines with spaces, which editorconfig-checker rejects in a tab-indented file; prefix them with a marker that is stripped at runtime. Also remove an over-indented docblock continuation line that php-cs-fixer rewrote. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/check-app-migrations.php | 82 ++++++++++++++-------------- 1 file changed, 42 insertions(+), 40 deletions(-) diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index 782debc..fb7f786 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -25,47 +25,49 @@ const EXIT_USAGE = 2; function usage(): string { - return <<<'TXT' -Usage: - check-app-migrations.php [--app APP] [--app-path DIR] --db - check-app-migrations.php --all --db - check-app-migrations.php [--app APP] [--app-path DIR] \ - --applied FILE --schema FILE - check-app-migrations.php [--app APP] --db --dump-applied > applied.txt - check-app-migrations.php --db --dump-schema > schema.txt - -Options: - --app APP App id (default: spreed) - --all Check every app that has rows in the migrations table (with --db); - an overview table and details only for apps with findings - --app-path DIR App directory (default: found via apps_paths of the Nextcloud - config, else apps/, apps-external/, custom_apps/) - --db Read applied list and schema from the database - --applied FILE Applied versions, one per line (e.g. 23000Date20251030090219) - --schema FILE Schema dump as produced by --dump-schema - --dump-applied Print the applied versions of APP and exit - --dump-schema Print the live schema (table/column/index names) and exit - --since-nc N Only migrations written after Nextcloud N was branched (stable N cut from - master), i.e. what an upgrade from N to a later release should have run, - e.g. --since-nc 31 for the 31 -> 32 -> 33 path. Known: 28 to 33. - --strict Treat "not verifiable" migrations as findings too - --verbose Also list verified and superseded schema effects - -h, --help This help - -Database connection: taken from the Nextcloud config (config/config.php and -config/*.config.php), nothing is passed on the command line or printed. - --config DIR Nextcloud config directory (default: $NEXTCLOUD_CONFIG_DIR, else the - config/ of the Nextcloud root this script lives in; not the current directory) - --sqlite-file F sqlite only: use this file instead of /owncloud.db - -Expected migrations: the Version*.php files in DIR/lib/Migration, i.e. what is -deployed in the app directory. - -Schema dump format, whitespace separated, real (prefixed) table names: - column
- index
(primary key: PRIMARY) + $text = <<<'TXT' +|Usage: +| check-app-migrations.php [--app APP] [--app-path DIR] --db +| check-app-migrations.php --all --db +| check-app-migrations.php [--app APP] [--app-path DIR] \ +| --applied FILE --schema FILE +| check-app-migrations.php [--app APP] --db --dump-applied > applied.txt +| check-app-migrations.php --db --dump-schema > schema.txt + +|Options: +| --app APP App id (default: spreed) +| --all Check every app that has rows in the migrations table (with --db); +| an overview table and details only for apps with findings +| --app-path DIR App directory (default: found via apps_paths of the Nextcloud +| config, else apps/, apps-external/, custom_apps/) +| --db Read applied list and schema from the database +| --applied FILE Applied versions, one per line (e.g. 23000Date20251030090219) +| --schema FILE Schema dump as produced by --dump-schema +| --dump-applied Print the applied versions of APP and exit +| --dump-schema Print the live schema (table/column/index names) and exit +| --since-nc N Only migrations written after Nextcloud N was branched (stable N cut from +| master), i.e. what an upgrade from N to a later release should have run, +| e.g. --since-nc 31 for the 31 -> 32 -> 33 path. Known: 28 to 33. +| --strict Treat "not verifiable" migrations as findings too +| --verbose Also list verified and superseded schema effects +| -h, --help This help + +|Database connection: taken from the Nextcloud config (config/config.php and +|config/*.config.php), nothing is passed on the command line or printed. +| --config DIR Nextcloud config directory (default: $NEXTCLOUD_CONFIG_DIR, else the +| config/ of the Nextcloud root this script lives in; not the current directory) +| --sqlite-file F sqlite only: use this file instead of /owncloud.db + +|Expected migrations: the Version*.php files in DIR/lib/Migration, i.e. what is +|deployed in the app directory. + +|Schema dump format, whitespace separated, real (prefixed) table names: +| column
+| index
(primary key: PRIMARY) TXT; + // The leading | keeps indentation in the source free of spaces (tabs only, see .editorconfig). + return preg_replace('/^\|/m', '', $text); } function fail(string $message, int $code = EXIT_USAGE): never { @@ -1115,10 +1117,10 @@ function migrationDir(string $app, string $ncRoot, ?string $appPath, array $conf /** * Compare one app's migrations with the live state and print the report. + * The returned text is the compact --all block for the app, empty when the app is clean. * @param array $expected version => php source * @param array $applied * @return array{bad:bool,never:int,noeffect:int,unverifiable:int,verified:int,expected:int,text:string} - * text is the compact (--all) block for this app, empty when the app is clean */ function checkApp(string $app, array $expected, array $applied, array $schema, string $prefix, string $source, bool $verbose, bool $strict, bool $compact, ?Closure $inScope = null, array &$fixes = []): array { $neverApplied = []; From 147d965ab709424dc2fb76f6f2398eaf8b8b274d Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 14:34:03 +0200 Subject: [PATCH 11/18] fix(admin-tools): address review findings of check-app-migrations False drift findings and unsafe replay advice came from several places where the static analysis guessed too much. Guards are only trusted in the exact has*() forms whose body creates or removes the tested object; other conditions, early returns under runtime conditions, helpers that receive the schema, computed names and a closure bound to another variable name make the affected effects not verifiable and mark the tables as possibly changed. Index checks compare uniqueness and columns; dumps carry the table prefix and the unique flag. The suggested fix no longer lists replays that would drop something a later migration re-added or repeat data changes; those go to a review list. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 6 +- admin-tools/check-app-migrations.php | 348 ++++++++++++++++++++------- 2 files changed, 266 insertions(+), 88 deletions(-) diff --git a/admin-tools/README.md b/admin-tools/README.md index f23e464..3c738d1 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -97,7 +97,7 @@ Update email addresses in Nextcloud mail app configuration when a customer's ema Read-only check of an app's migrations against the database. Reports migrations that were **never applied** (no row in `oc_migrations`) and migrations that are **applied but not in effect** (row exists, but the columns, tables or indexes they create are missing, or what they drop is still there). Example: Talk failing with `Unknown column 'r.last_pinned_id'` although `23000Date20251030090219` is recorded as applied. -The expected migrations are the `Version*.php` files in the app's `lib/Migration`, i.e. what is deployed there. No git is used. Schema effects (`addColumn`, `createTable`, `addIndex`, `setPrimaryKey`, `dropColumn`, `dropTable`, `dropIndex`, `renameColumn`) are extracted statically from `changeSchema()`. `hasColumn`-style guards are understood. Effects that a later applied migration overrides are skipped. A `changeSchema()` whose every `return` is `return null;` is reported as not verifiable with a note: Nextcloud applies the schema only when a schema wrapper is returned, so such a migration never makes its changes (a defect in the app) and re-running it cannot fix them. Anything it cannot parse (data changes in `preSchemaChange`/`postSchemaChange`, loops, helper calls, non-guard conditions, computed names) is listed under "Not verifiable" and never guessed. +The expected migrations are the `Version*.php` files in the app's `lib/Migration`, i.e. what is deployed there. No git is used. Schema effects (`addColumn`, `createTable`, `addIndex`, `setPrimaryKey`, `dropColumn`, `dropTable`, `dropIndex`, `renameColumn`) are extracted statically from `changeSchema()`. `hasColumn`-style guards are understood. Effects that a later applied migration overrides are skipped. A `changeSchema()` whose every `return` is `return null;` is reported as not verifiable with a note: Nextcloud applies the schema only when a schema wrapper is returned, so such a migration never makes its changes (a defect in the app) and re-running it cannot fix them. `if` guards are only trusted in the exact forms `if (!hasX('n')) { add n }` and `if (hasX('n')) { drop n }` (and `hasTable('t')` for effects on that table); other conditions, `if (false)`, early returns under runtime conditions, helper calls that receive the schema and computed names make the affected migrations (and earlier effects on the same tables) not verifiable. Indexes are compared with their uniqueness. Anything it cannot parse (data changes in `preSchemaChange`/`postSchemaChange`, loops, helper calls, non-guard conditions, computed names) is listed under "Not verifiable" and never guessed. The database connection is taken from the Nextcloud config (`config/config.php` plus `config/*.config.php`); nothing is passed on the command line. @@ -122,7 +122,9 @@ The database connection is taken from the Nextcloud config (`config/config.php` **Options:** `--app` (default `spreed`), `--all` (all apps; an overview table and details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR` (default: `NEXTCLOUD_CONFIG_DIR`, else the `config/` of the Nextcloud root the script lives in, not the current directory), `--sqlite-file FILE`, `--since-nc N` (only migrations written after Nextcloud N was branched, e.g. `31`; derived from the date stamp in the migration version, so approximate for apps that are branched on their own schedule), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). -When something is found, the report ends with a "Suggested fix" block: the `occ migrations:execute ` commands in execution order, plus the steps around them (backup, maintenance mode). `migrations:execute` only exists with debug on, so the commands are prefixed with `NC_debug=true`, which enables it for that one command and not for the instance. The script only prints them and never runs anything. +When something is found, the report ends with a "Suggested fix" block. A replay runs the whole migration again, not only the missing part, so the block only lists `NC_debug=true occ migrations:execute ` for migrations that are safe to run: never applied ones, and applied ones without a drop that a later migration undid and without data changes in `preSchemaChange()`/`postSchemaChange()`. The rest is listed under "Review by hand" with the reason. `migrations:execute` only exists with debug on, so `NC_debug=true` enables it for that one command and not for the instance. The script only prints all this and never runs anything. + +The schema dump starts with a `prefix=` line, so a dump from an instance with another table prefix works as is; `--prefix` is for hand-made files. **Exit codes:** 0 nothing wrong, 1 never applied or not in effect found, 2 usage/input error. Output contains only versions and table, column and index names. Sessions are read-only (`SET SESSION TRANSACTION READ ONLY` / `PRAGMA query_only`), and connection errors are reported by code only so no credentials leak. diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index fb7f786..faf1dd6 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -43,6 +43,7 @@ function usage(): string { | --db Read applied list and schema from the database | --applied FILE Applied versions, one per line (e.g. 23000Date20251030090219) | --schema FILE Schema dump as produced by --dump-schema +| --prefix P Table prefix for --applied/--schema files without a prefix= line (default: oc_) | --dump-applied Print the applied versions of APP and exit | --dump-schema Print the live schema (table/column/index names) and exit | --since-nc N Only migrations written after Nextcloud N was branched (stable N cut from @@ -62,8 +63,9 @@ function usage(): string { |deployed in the app directory. |Schema dump format, whitespace separated, real (prefixed) table names: +| prefix=
(first line of a dump; --prefix overrides) | column
-| index
(primary key: PRIMARY) +| index
[unique] (primary key: PRIMARY) TXT; // The leading | keeps indentation in the source free of spaces (tabs only, see .editorconfig). @@ -111,7 +113,7 @@ function compareVersions(string $a, string $b): int { // --------------------------------------------------------------------------- /** - * Schema model: ['columns' => [table => [col => true]], 'indexes' => [table => [name => [cols]]]] + * Schema model: ['columns' => [table => [col => true]], 'indexes' => [table => [name => ['cols' => [..], 'unique' => bool]]]] * Names are lower-cased and stripped of the table prefix. */ function newSchema(): array { @@ -129,8 +131,12 @@ function addColumnTo(array &$schema, string $table, string $column): void { $schema['columns'][$table][strtolower($column)] = true; } -function addIndexTo(array &$schema, string $table, string $name, array $cols): void { - $schema['indexes'][$table][strtolower($name) === 'primary' ? 'PRIMARY' : $name] = array_map('strtolower', $cols); +function addIndexTo(array &$schema, string $table, string $name, array $cols, bool $unique): void { + $isPrimary = strtolower($name) === 'primary'; + $schema['indexes'][$table][$isPrimary ? 'PRIMARY' : $name] = [ + 'cols' => array_map('strtolower', $cols), + 'unique' => $unique || $isPrimary, + ]; } function readAppliedFile(string $file): array { @@ -151,6 +157,19 @@ function readAppliedFile(string $file): array { return $result; } +/** Table prefix recorded in a schema dump ("prefix=oc_" line), null for hand-made files without one. */ +function schemaFilePrefix(string $file): ?string { + if (!is_readable($file)) { + fail("cannot read schema file $file"); + } + foreach (file($file, FILE_IGNORE_NEW_LINES) as $line) { + if (str_starts_with(trim($line), 'prefix=')) { + return substr(trim($line), strlen('prefix=')); + } + } + return null; +} + function readSchemaFile(string $file, string $prefix): array { if (!is_readable($file)) { fail("cannot read schema file $file"); @@ -158,7 +177,7 @@ function readSchemaFile(string $file, string $prefix): array { $schema = newSchema(); foreach (file($file, FILE_IGNORE_NEW_LINES) as $no => $line) { $line = trim($line); - if ($line === '' || $line[0] === '#') { + if ($line === '' || $line[0] === '#' || str_starts_with($line, 'prefix=')) { continue; } $f = preg_split('/\s+/', $line); @@ -167,9 +186,9 @@ function readSchemaFile(string $file, string $prefix): array { if ($table !== null) { addColumnTo($schema, $table, $f[2]); } - } elseif ($f[0] === 'index' && count($f) === 4) { + } elseif ($f[0] === 'index' && (count($f) === 4 || (count($f) === 5 && $f[4] === 'unique'))) { if ($table !== null) { - addIndexTo($schema, $table, $f[2], explode(',', $f[3])); + addIndexTo($schema, $table, $f[2], explode(',', $f[3]), count($f) === 5); } } else { fail('schema file: line ' . ($no + 1) . ' is malformed'); @@ -308,7 +327,7 @@ function dbSchema(PDO $pdo, string $prefix): array { } if ($pk !== []) { ksort($pk); - addIndexTo($schema, $table, 'PRIMARY', array_values($pk)); + addIndexTo($schema, $table, 'PRIMARY', array_values($pk), true); } foreach ($pdo->query("PRAGMA index_list($quoted)")->fetchAll(PDO::FETCH_ASSOC) as $idx) { if ($idx['origin'] === 'pk') { @@ -316,7 +335,7 @@ function dbSchema(PDO $pdo, string $prefix): array { } $iq = '"' . str_replace('"', '""', (string)$idx['name']) . '"'; $cols = array_column($pdo->query("PRAGMA index_info($iq)")->fetchAll(PDO::FETCH_ASSOC), 'name'); - addIndexTo($schema, $table, (string)$idx['name'], array_map('strval', $cols)); + addIndexTo($schema, $table, (string)$idx['name'], array_map('strval', $cols), (int)$idx['unique'] === 1); } } return $schema; @@ -332,19 +351,21 @@ function dbSchema(PDO $pdo, string $prefix): array { } } $rows = $pdo->query( - 'SELECT TABLE_NAME, INDEX_NAME, COLUMN_NAME FROM information_schema.STATISTICS ' + 'SELECT TABLE_NAME, INDEX_NAME, COLUMN_NAME, NON_UNIQUE FROM information_schema.STATISTICS ' . 'WHERE TABLE_SCHEMA = DATABASE() ORDER BY TABLE_NAME, INDEX_NAME, SEQ_IN_INDEX' )->fetchAll(PDO::FETCH_NUM); $grouped = []; - foreach ($rows as [$real, $index, $col]) { + $unique = []; + foreach ($rows as [$real, $index, $col, $nonUnique]) { $table = stripPrefix((string)$real, $prefix); if ($table !== null) { $grouped[$table][(string)$index][] = (string)$col; + $unique[$table][(string)$index] = (int)$nonUnique === 0; } } foreach ($grouped as $table => $indexes) { foreach ($indexes as $name => $cols) { - addIndexTo($schema, $table, (string)$name, $cols); + addIndexTo($schema, $table, (string)$name, $cols, $unique[$table][$name]); } } return $schema; @@ -353,14 +374,15 @@ function dbSchema(PDO $pdo, string $prefix): array { function dumpSchema(PDO $pdo, string $prefix): void { // Dump with the prefix kept so that the file is self-describing. $schema = dbSchema($pdo, $prefix); + echo "prefix=$prefix\n"; foreach ($schema['columns'] as $table => $cols) { foreach (array_keys($cols) as $col) { echo "column $prefix$table $col\n"; } } foreach ($schema['indexes'] as $table => $indexes) { - foreach ($indexes as $name => $cols) { - echo "index $prefix$table $name " . implode(',', $cols) . "\n"; + foreach ($indexes as $name => $idx) { + echo "index $prefix$table $name " . implode(',', $idx['cols']) . ($idx['unique'] ? ' unique' : '') . "\n"; } } } @@ -397,6 +419,8 @@ final class EffectExtractor { private array $touched = []; /** @var array effect keys that an early-return has*() guard already speaks about */ private array $guarded = []; + /** @var array method => [index of "(", index of ")"] */ + private array $params = []; /** * @return array{effects:list>,reasons:list,touched:list} @@ -424,7 +448,8 @@ public static function extract(string $source): array { } } if (isset($methods['changeSchema'])) { - $self->vars = ['$schema' => ['k' => 'schema']]; + $closureVar = $self->closureParam(); + $self->vars = $closureVar === null ? [] : [$closureVar => ['k' => 'closure']]; $self->walk($methods['changeSchema'][0], $methods['changeSchema'][1], true); } if (isset($methods['changeSchema'])) { @@ -443,6 +468,31 @@ public static function extract(string $source): array { ]; } + /** Name of the Closure parameter of changeSchema() ($schemaClosure by convention). */ + private function closureParam(): ?string { + [$open, $close] = $this->params['changeSchema']; + $args = $this->splitArgs($open, $close); + foreach ($args as [$a, $b]) { + for ($i = $a; $i < $b; $i++) { + if ($this->T[$i]['t'] === T_STRING && strtolower($this->T[$i]['s']) === 'closure') { + for ($j = $i; $j < $b; $j++) { + if ($this->T[$j]['t'] === T_VARIABLE) { + return $this->T[$j]['s']; + } + } + } + } + } + if (isset($args[1])) { + for ($j = $args[1][0]; $j < $args[1][1]; $j++) { + if ($this->T[$j]['t'] === T_VARIABLE) { + return $this->T[$j]['s']; + } + } + } + return null; + } + private function collectConsts(): void { $n = count($this->T); for ($i = 0; $i < $n; $i++) { @@ -473,13 +523,15 @@ private function findMethods(): array { if (($this->T[$j]['s'] ?? '') !== '(') { continue; } - $j = $this->close($j) + 1; + $parenClose = $this->close($j); + $j = $parenClose + 1; while ($j < $n && $this->T[$j]['s'] !== '{' && $this->T[$j]['s'] !== ';') { $j++; } if ($j < $n && $this->T[$j]['s'] === '{') { $end = $this->close($j); $methods[$name] = [$j + 1, $end]; + $this->params[$name] = [$i + 2, $parenClose]; $i = $end; } } @@ -542,16 +594,37 @@ private function reason(string $text, int $tokenIndex): void { $this->reasons[$text . ' (line ' . ($this->T[min($tokenIndex, count($this->T) - 1)]['l']) . ')'] = true; } - /** Remember which tables a range of uninterpreted code may touch: bound table variables and string literals. */ + /** + * Remember which tables a range of uninterpreted code may touch: bound table variables and string + * literals; when the code gets the schema or a table of unknown name but names no literal, any table. + */ private function touch(int $from, int $to): void { + $literal = false; + $anyTable = false; for ($i = $from; $i < $to && $i < count($this->T); $i++) { $t = $this->T[$i]; - if ($t['t'] === T_VARIABLE && ($this->vars[$t['s']]['k'] ?? '') === 'table' && isset($this->vars[$t['s']]['name'])) { - $this->touched[$this->vars[$t['s']]['name']] = true; + if ($t['t'] === T_RETURN) { + // handing the schema back does not change a table + $i = $this->statementEnd($i, $to); + continue; + } + if ($t['t'] === T_VARIABLE && isset($this->vars[$t['s']])) { + $var = $this->vars[$t['s']]; + if ($var['k'] === 'table' && isset($var['name'])) { + $this->touched[$var['name']] = true; + } elseif ($var['k'] === 'column') { + $this->touched[strstr((string)($var['name'] ?? ''), '.', true) ?: '*'] = true; + } elseif ($var['k'] !== 'other') { + $anyTable = true; + } } elseif ($t['t'] === T_CONSTANT_ENCAPSED_STRING) { $this->touched[strtolower(substr($t['s'], 1, -1))] = true; + $literal = true; } } + if ($anyTable && !$literal) { + $this->touched['*'] = true; + } } private function mentions(int $from, int $to): bool { @@ -619,6 +692,12 @@ private function skipConstruct(int $i, int $end): int { $j++; } } + for ($k = $start; $k < $j; $k++) { + if ($this->T[$k]['t'] === T_RETURN) { + $this->conditional = true; + break; + } + } if ($this->mentions($start, $j)) { $this->touch($start, $j); $this->reason('schema changes inside ' . strtolower(substr(token_name((int)$this->T[$start]['t']), 2)) . ' are not verifiable', $start); @@ -665,8 +744,8 @@ private function handleIf(int $i): int { } } - $single = count($branches) === 1; - if ($single && $this->isGuard($branches[0][0], $branches[0][1])) { + $guard = count($branches) === 1 ? $this->parseGuard($branches[0][0], $branches[0][1]) : null; + if ($guard !== null) { [, , $from, $to] = $branches[0]; if ($this->conditional) { if ($this->mentions($from, $to)) { @@ -675,16 +754,27 @@ private function handleIf(int $i): int { } return $j; } + $before = count($this->effects); $this->walk($from, $to, false); + $this->validateGuarded($guard, $before); for ($k = $from; $k < $to; $k++) { if ($this->T[$k]['t'] === T_RETURN) { $this->conditional = true; - $this->collectGuardedKeys($branches[0][0], $branches[0][1]); + $this->guarded[$guard['key']] = true; break; } } return $j; } + foreach ($branches as [$cf, $ct, $bf, $bt]) { + for ($k = $bf; $k < $bt; $k++) { + if ($this->T[$k]['t'] === T_RETURN) { + // the rest of changeSchema() may be skipped at runtime + $this->conditional = true; + break 2; + } + } + } foreach ($branches as [$cf, $ct, $bf, $bt]) { if ($this->mentions($cf, $ct) || $this->mentions($bf, $bt)) { foreach ($branches as [, , $tf, $tt]) { @@ -698,49 +788,75 @@ private function handleIf(int $i): int { } /** - * "if (hasTable('x')) return;" followed by createTable('x') leaves 'x' in place either way, - * so effects on exactly the guarded object stay verifiable; everything else does not. + * Recognise exactly "[!]$x->hasColumn|hasIndex|hasUniqueConstraint|hasTable|hasPrimaryKey('name')". + * The key is the effect key the guard speaks about; table is set for hasTable guards, whose body may + * touch anything of that table. + * @return ?array{neg:bool,key:string,table:?string} */ - private function collectGuardedKeys(int $from, int $to): void { - for ($i = $from; $i + 4 < $to; $i++) { - if ($this->T[$i]['t'] !== T_VARIABLE || $this->T[$i + 1]['t'] !== T_OBJECT_OPERATOR - || ($this->T[$i + 3]['s'] ?? '') !== '(') { - continue; - } - $recv = $this->vars[$this->T[$i]['s']] ?? ['k' => 'other']; - $method = $this->T[$i + 2]['s']; - $arg = $this->str([$i + 4, $i + 5]); - if ($arg === null) { - continue; - } - $arg = strtolower($arg); - if ($recv['k'] === 'schema' && $method === 'hasTable') { - $this->guarded["tbl:$arg"] = true; - } elseif ($recv['k'] === 'table' && isset($recv['name'])) { - if ($method === 'hasColumn') { - $this->guarded["col:{$recv['name']}.$arg"] = true; - } elseif ($method === 'hasIndex') { - $this->guarded["idx:{$recv['name']}:$arg"] = true; - } - } + private function parseGuard(int $from, int $to): ?array { + $i = $from; + $neg = ($this->T[$i]['s'] ?? '') === '!'; + if ($neg) { + $i++; + } + if (($this->T[$i]['t'] ?? null) !== T_VARIABLE || ($this->T[$i + 1]['t'] ?? null) !== T_OBJECT_OPERATOR + || ($this->T[$i + 2]['t'] ?? null) !== T_STRING || ($this->T[$i + 3]['s'] ?? '') !== '(') { + return null; } + $recv = $this->vars[$this->T[$i]['s']] ?? ['k' => 'other']; + $method = $this->T[$i + 2]['s']; + $close = $this->close($i + 3); + if ($close + 1 !== $to || !in_array($method, self::GUARD_CALLS, true)) { + return null; + } + $args = $this->splitArgs($i + 3, $close); + $arg = isset($args[0]) ? $this->str($args[0]) : null; + if ($recv['k'] === 'schema' && $method === 'hasTable' && $arg !== null) { + $name = strtolower($arg); + return ['neg' => $neg, 'key' => "tbl:$name", 'table' => $name]; + } + if ($recv['k'] !== 'table' || !isset($recv['name'])) { + return null; + } + $t = $recv['name']; + if ($method === 'hasColumn' && $arg !== null) { + return ['neg' => $neg, 'key' => "col:$t." . strtolower($arg), 'table' => null]; + } + if (($method === 'hasIndex' || $method === 'hasUniqueConstraint') && $arg !== null) { + return ['neg' => $neg, 'key' => "idx:$t:" . strtolower($arg), 'table' => null]; + } + if ($method === 'hasPrimaryKey' && $args === []) { + return ['neg' => $neg, 'key' => "idx:$t:PRIMARY", 'table' => null]; + } + return null; } - private function isGuard(int $from, int $to): bool { - for ($i = $from; $i < $to; $i++) { - $t = $this->T[$i]; - if ($t['t'] === T_VARIABLE) { - if (!isset($this->vars[$t['s']]) || $this->vars[$t['s']]['k'] === 'other') { - return false; - } - } elseif ($t['t'] === T_STRING && ($this->T[$i + 1]['s'] ?? '') === '(') { - $prev = $this->T[$i - 1]['t'] ?? null; - if ($prev !== T_OBJECT_OPERATOR || !in_array($t['s'], self::GUARD_CALLS, true)) { - return false; + /** + * A guard only vouches for effects that create (guard "!hasX") or remove (guard "hasX") exactly the + * object it tests; any other effect in its body need not have happened. + * @param array{neg:bool,key:string,table:?string} $guard + */ + private function validateGuarded(array $guard, int $before): void { + $kept = array_slice($this->effects, 0, $before); + foreach (array_slice($this->effects, $before) as $e) { + $adds = in_array($e['op'], ['addColumn', 'createTable', 'addIndex', 'setPrimaryKey'], true); + if ($guard['table'] !== null) { + // "if (hasTable('x')) { ... }" changes x only when it exists; "if (!hasTable('x')) { ... }" creates it + $ok = $e['table'] === $guard['table']; + if ($ok && !$guard['neg']) { + $e['onlyIfTable'] = true; } + } else { + $ok = $adds === $guard['neg'] && effectKey($e) === $guard['key']; + } + if ($ok) { + $kept[] = $e; + } else { + $this->touched[$e['table']] = true; + $this->reasons['schema changes under a has*() guard that does not guarantee them are not verifiable'] = true; } } - return true; + $this->effects = $kept; } /** @return list */ @@ -828,9 +944,6 @@ private function statement(int $from, int $to): void { } return; } - if ($base['s'] === '$schemaClosure') { - return; - } if (!isset($this->vars[$base['s']]) || $this->vars[$base['s']]['k'] === 'other') { if ($touches) { $this->touch($from, $to); @@ -845,6 +958,19 @@ private function statement(int $from, int $to): void { // Parse the call chain: $var->call(...)->call(...) $recv = $this->vars[$base['s']]; $i = $chainFrom + 1; + if ($recv['k'] === 'closure') { + // $schema = $schemaClosure(); binds whatever variable name the migration chose + if (($this->T[$i]['s'] ?? '') !== '(') { + $this->touch($from, $to); + $this->reason('the schema closure is used in a way that is not understood', $from); + if ($assignTo !== null) { + $this->vars[$assignTo] = ['k' => 'other']; + } + return; + } + $recv = ['k' => 'schema']; + $i = $this->close($i) + 1; + } while ($i < $to) { if ($this->T[$i]['t'] !== T_OBJECT_OPERATOR || ($this->T[$i + 1]['t'] ?? null) !== T_STRING || ($this->T[$i + 2]['s'] ?? '') !== '(') { @@ -927,7 +1053,10 @@ private function call(array $recv, string $method, array $args, int $at): array case 'addUniqueIndex': $cols = isset($args[0]) ? $this->strList($args[0]) : null; $idx = isset($args[1]) ? $this->str($args[1]) : null; - $this->record(['op' => 'addIndex', 'table' => $t, 'columns' => $cols, 'index' => $idx], $at, [$t, $cols]); + $this->record([ + 'op' => 'addIndex', 'table' => $t, 'columns' => $cols, 'index' => $idx, + 'unique' => $method === 'addUniqueIndex', + ], $at, [$t, $cols]); return $other; case 'setPrimaryKey': $cols = isset($args[0]) ? $this->strList($args[0]) : null; @@ -952,6 +1081,7 @@ private function call(array $recv, string $method, array $args, int $at): array private function record(array $effect, int $at, array $required): void { foreach ($required as $value) { if ($value === null) { + $this->touched[($effect['table'] ?? null) ?: '*'] = true; $this->reason($effect['op'] . ' with a name that is computed at runtime is not verifiable', $at); return; } @@ -1008,11 +1138,29 @@ function supersedes(array $later, array $earlier): bool { return in_array($later['op'], ['createTable', 'dropTable'], true) && $later['table'] === $earlier['table']; } +/** + * Does a live index provide what addIndex()/addUniqueIndex() asked for? Unique needs exactly these columns + * and uniqueness (a wider unique index does not make the narrower combination unique). A plain index is + * also provided by a wider one starting with the same columns: Doctrine drops the redundant narrower + * index when the wider one is added. + * @param array{cols:list,unique:bool} $idx + * @param list $columns + */ +function indexSatisfies(array $idx, array $columns, bool $unique): bool { + if ($unique) { + return $idx['unique'] && $idx['cols'] === $columns; + } + return array_slice($idx['cols'], 0, count($columns)) === $columns; +} + /** @return ?string problem description, null if the effect is in place */ function checkEffect(array $e, array $schema): ?string { $table = $e['table']; $tableExists = isset($schema['columns'][$table]); $indexes = $schema['indexes'][$table] ?? []; + if (!$tableExists && ($e['onlyIfTable'] ?? false)) { + return null; + } switch ($e['op']) { case 'createTable': return $tableExists ? null : 'table missing'; @@ -1029,26 +1177,23 @@ function checkEffect(array $e, array $schema): ?string { if (!$tableExists) { return 'table missing'; } - return ($indexes['PRIMARY'] ?? null) === $e['columns'] ? null : 'primary key missing or different'; + return ($indexes['PRIMARY']['cols'] ?? null) === $e['columns'] ? null : 'primary key missing or different'; case 'dropPrimaryKey': return isset($indexes['PRIMARY']) ? 'primary key still present' : null; case 'addIndex': if (!$tableExists) { return 'table missing'; } - foreach ($indexes as $name => $cols) { - if (isset($e['index']) && strtolower((string)$name) === $e['index']) { - return null; - } - // Doctrine drops an index that a wider index starting with the same columns makes - // redundant, so such a covering index counts as in effect. - if (isset($e['columns']) && array_slice($cols, 0, count($e['columns'])) === $e['columns']) { + $sameName = false; + foreach ($indexes as $name => $idx) { + if (indexSatisfies($idx, $e['columns'], $e['unique'] ?? false)) { return null; } + $sameName = $sameName || (isset($e['index']) && strtolower((string)$name) === $e['index']); } - return 'index missing'; + return $sameName ? 'index exists with other columns or without being unique' : 'index missing'; case 'dropIndex': - foreach ($indexes as $name => $cols) { + foreach ($indexes as $name => $idx) { if (strtolower((string)$name) === $e['index']) { return 'index still present'; } @@ -1087,23 +1232,37 @@ function scopeFilter(array $opts): ?Closure { } /** - * Print the commands an admin has to run to repair the findings. Nothing is executed here. - * @param array> $fixes app => versions in execution order + * Print what an admin can do about the findings. Nothing is executed here. + * @param array,review:array}> $fixes */ function printFix(array $fixes): void { if ($fixes === []) { return; } + $run = array_filter($fixes, static fn (array $f): bool => $f['run'] !== []); + $review = array_filter($fixes, static fn (array $f): bool => $f['review'] !== []); echo "== Suggested fix (run by an admin, this script changes nothing) ==\n"; - echo "# Before: take a DB backup/snapshot and enable maintenance mode: occ maintenance:mode --on\n"; - echo "# migrations:execute is only available with debug on; NC_debug=true enables it for this one command only\n"; - echo "# Run in this order; only migrations that are missing or not in effect are listed:\n"; - foreach ($fixes as $app => $versions) { - foreach ($versions as $v) { - echo "NC_debug=true occ migrations:execute $app $v\n"; + echo "# A replay runs the whole migration again, not only the part that is missing.\n"; + if ($run !== []) { + echo "# Before: take a DB backup/snapshot and enable maintenance mode: occ maintenance:mode --on\n"; + echo "# migrations:execute is only available with debug on; NC_debug=true enables it for this one command only\n"; + echo "# In this order:\n"; + foreach ($run as $app => $f) { + foreach ($f['run'] as $v) { + echo "NC_debug=true occ migrations:execute $app $v\n"; + } + } + echo "# After: occ maintenance:mode --off; then re-run this check\n"; + } + if ($review !== []) { + echo "# Review by hand, no command suggested because a replay is not harmless:\n"; + foreach ($review as $app => $f) { + foreach ($f['review'] as $v => $why) { + echo "# $app $v: $why\n"; + } } } - echo "# After: occ maintenance:mode --off; then re-run this check\n\n"; + echo "\n"; } /** Migration directory of an app; core keeps its migrations in core/Migrations. */ @@ -1194,10 +1353,26 @@ function checkApp(string $app, array $expected, array $applied, array $schema, s } $bad = $neverApplied !== [] || $notInEffect !== [] || ($strict && $unverifiable !== []); - $toRun = array_values(array_unique(array_merge($neverApplied, array_keys($notInEffect)))); - usort($toRun, 'compareVersions'); - if ($toRun !== []) { - $fixes[$app] = $toRun; + // Never applied migrations just run for the first time. Replaying an applied one repeats everything + // it does: drops (of things a later migration may have re-added) and data changes. Those need a human. + $run = $neverApplied; + $review = []; + foreach (array_keys($notInEffect) as $v) { + // A drop that a later migration undid (re-added column, recreated table) must not run again; + // a drop that is itself what is missing is the repair. + $destructive = array_filter($superseded[$v] ?? [], static fn (string $d): bool => str_starts_with($d, 'drop')); + $dataChanges = array_filter($extracted[$v]['reasons'], static fn (string $r): bool => str_starts_with($r, 'data changes')); + if ($destructive !== []) { + $review[$v] = 'it contains ' . implode(', ', array_map(static fn (string $d): string => preg_replace('/ \(.*$/', '', $d), $destructive)) . ' which a later migration undid; a replay would drop it again'; + } elseif ($dataChanges !== []) { + $review[$v] = 'it changes data in pre/postSchemaChange(); a replay would repeat that'; + } else { + $run[] = $v; + } + } + usort($run, 'compareVersions'); + if ($run !== [] || $review !== []) { + $fixes[$app] = ['run' => $run, 'review' => $review]; } $stats = [ 'bad' => $bad, @@ -1352,7 +1527,7 @@ function compactBlock(string $app, array $never, array $notInEffect, array $unve function main(array $argv): int { $opts = getopt('h', [ 'app:', 'app-path:', 'db', 'applied:', 'schema:', 'dump-applied', 'dump-schema', - 'strict', 'verbose', 'help', 'config:', 'sqlite-file:', 'all', 'since-nc:' + 'strict', 'verbose', 'help', 'config:', 'sqlite-file:', 'all', 'since-nc:', 'prefix:' ], $rest); if (isset($opts['h']) || isset($opts['help'])) { echo usage(); @@ -1412,6 +1587,7 @@ function main(array $argv): int { $source = 'database'; $appliedByApp = $all ? dbAppliedAll($pdo, $prefix) : [$app => dbApplied($pdo, $app, $prefix)]; } else { + $prefix = isset($opts['prefix']) ? (string)$opts['prefix'] : (schemaFilePrefix((string)$opts['schema']) ?? 'oc_'); $schema = readSchemaFile((string)$opts['schema'], $prefix); $source = 'files'; $appliedByApp = [$app => readAppliedFile((string)$opts['applied'])]; From 9a7c11944ebfc4e7afbf5fcd0d46bed46f5dbb6b Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 14:45:55 +0200 Subject: [PATCH 12/18] feat(admin-tools): make suggested migrations:execute results visible migrations:execute prints nothing on success, which makes a run hard to tell from a no-op. Each suggested command now ends with an OK line that only prints when the exit code is 0, and the block ends with the exact check command to verify the result. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 2 +- admin-tools/check-app-migrations.php | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/admin-tools/README.md b/admin-tools/README.md index 3c738d1..ba75216 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -122,7 +122,7 @@ The database connection is taken from the Nextcloud config (`config/config.php` **Options:** `--app` (default `spreed`), `--all` (all apps; an overview table and details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR` (default: `NEXTCLOUD_CONFIG_DIR`, else the `config/` of the Nextcloud root the script lives in, not the current directory), `--sqlite-file FILE`, `--since-nc N` (only migrations written after Nextcloud N was branched, e.g. `31`; derived from the date stamp in the migration version, so approximate for apps that are branched on their own schedule), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). -When something is found, the report ends with a "Suggested fix" block. A replay runs the whole migration again, not only the missing part, so the block only lists `NC_debug=true occ migrations:execute ` for migrations that are safe to run: never applied ones, and applied ones without a drop that a later migration undid and without data changes in `preSchemaChange()`/`postSchemaChange()`. The rest is listed under "Review by hand" with the reason. `migrations:execute` only exists with debug on, so `NC_debug=true` enables it for that one command and not for the instance. The script only prints all this and never runs anything. +When something is found, the report ends with a "Suggested fix" block. A replay runs the whole migration again, not only the missing part, so the block only lists `NC_debug=true occ migrations:execute ` for migrations that are safe to run: never applied ones, and applied ones without a drop that a later migration undid and without data changes in `preSchemaChange()`/`postSchemaChange()`. The rest is listed under "Review by hand" with the reason. `migrations:execute` only exists with debug on, so `NC_debug=true` enables it for that one command and not for the instance. `migrations:execute` prints nothing on success, so each suggested command ends with `&& echo "OK: ..."`, and the block ends with the matching re-check command. The script only prints all this and never runs anything. The schema dump starts with a `prefix=` line, so a dump from an instance with another table prefix works as is; `--prefix` is for hand-made files. diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index faf1dd6..ebdc86e 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -1235,7 +1235,7 @@ function scopeFilter(array $opts): ?Closure { * Print what an admin can do about the findings. Nothing is executed here. * @param array,review:array}> $fixes */ -function printFix(array $fixes): void { +function printFix(array $fixes, string $recheck): void { if ($fixes === []) { return; } @@ -1243,16 +1243,19 @@ function printFix(array $fixes): void { $review = array_filter($fixes, static fn (array $f): bool => $f['review'] !== []); echo "== Suggested fix (run by an admin, this script changes nothing) ==\n"; echo "# A replay runs the whole migration again, not only the part that is missing.\n"; + echo "# migrations:execute is silent on success; the OK line and exit code 0 mean it worked, a failure prints an error.\n"; if ($run !== []) { echo "# Before: take a DB backup/snapshot and enable maintenance mode: occ maintenance:mode --on\n"; echo "# migrations:execute is only available with debug on; NC_debug=true enables it for this one command only\n"; echo "# In this order:\n"; foreach ($run as $app => $f) { foreach ($f['run'] as $v) { - echo "NC_debug=true occ migrations:execute $app $v\n"; + // migrations:execute prints nothing on success, so make the exit code visible + echo "NC_debug=true occ migrations:execute $app $v && echo \"OK: $app $v\"\n"; } } - echo "# After: occ maintenance:mode --off; then re-run this check\n"; + echo "# After: occ maintenance:mode --off; then check the result (expect no findings left):\n"; + echo "$recheck\n"; } if ($review !== []) { echo "# Review by hand, no command suggested because a replay is not harmless:\n"; @@ -1619,7 +1622,8 @@ function main(array $argv): int { if ($all) { printOverview($results, $prefix, $source, $opts); } - printFix($fixes); + $recheck = $argv[0] . ($all ? ' --all --db' : ' --app ' . $app . ' --db') . (isset($opts['since-nc']) ? ' --since-nc ' . (int)$opts['since-nc'] : ''); + printFix($fixes, $recheck); if ($skipped !== []) { echo 'Skipped (not on disk, e.g. disabled or removed apps): ' . implode(', ', $skipped) . "\n"; } From cde2254675ee2a506fa7da4d60d575e906d329b4 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 14:50:39 +0200 Subject: [PATCH 13/18] fix(admin-tools): address second review round of check-app-migrations Running or replaying a migration executes all of it against the current schema, so the suggested fix now vets never applied migrations too and sends a migration to manual review when it has code the check cannot interpret, an unguarded operation that is already in place, or a drop that a later migration undid or may have changed again. Uninterpreted code is treated more conservatively: return expressions that get the schema, conditional return null, unsupported table methods and helpers that receive the schema mark the tables as possibly changed, also within the same migration; string literals no longer narrow that. Known schema methods only affect the tables they name, and a guard that creates a column also vouches for an index on it. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 4 +- admin-tools/check-app-migrations.php | 198 ++++++++++++++++++++------- 2 files changed, 151 insertions(+), 51 deletions(-) diff --git a/admin-tools/README.md b/admin-tools/README.md index ba75216..e17f234 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -97,7 +97,7 @@ Update email addresses in Nextcloud mail app configuration when a customer's ema Read-only check of an app's migrations against the database. Reports migrations that were **never applied** (no row in `oc_migrations`) and migrations that are **applied but not in effect** (row exists, but the columns, tables or indexes they create are missing, or what they drop is still there). Example: Talk failing with `Unknown column 'r.last_pinned_id'` although `23000Date20251030090219` is recorded as applied. -The expected migrations are the `Version*.php` files in the app's `lib/Migration`, i.e. what is deployed there. No git is used. Schema effects (`addColumn`, `createTable`, `addIndex`, `setPrimaryKey`, `dropColumn`, `dropTable`, `dropIndex`, `renameColumn`) are extracted statically from `changeSchema()`. `hasColumn`-style guards are understood. Effects that a later applied migration overrides are skipped. A `changeSchema()` whose every `return` is `return null;` is reported as not verifiable with a note: Nextcloud applies the schema only when a schema wrapper is returned, so such a migration never makes its changes (a defect in the app) and re-running it cannot fix them. `if` guards are only trusted in the exact forms `if (!hasX('n')) { add n }` and `if (hasX('n')) { drop n }` (and `hasTable('t')` for effects on that table); other conditions, `if (false)`, early returns under runtime conditions, helper calls that receive the schema and computed names make the affected migrations (and earlier effects on the same tables) not verifiable. Indexes are compared with their uniqueness. Anything it cannot parse (data changes in `preSchemaChange`/`postSchemaChange`, loops, helper calls, non-guard conditions, computed names) is listed under "Not verifiable" and never guessed. +The expected migrations are the `Version*.php` files in the app's `lib/Migration`, i.e. what is deployed there. No git is used. Schema effects (`addColumn`, `createTable`, `addIndex`, `setPrimaryKey`, `dropColumn`, `dropTable`, `dropIndex`, `renameColumn`) are extracted statically from `changeSchema()`. `hasColumn`-style guards are understood. Effects that a later applied migration overrides are skipped. A `changeSchema()` whose every `return` is `return null;` is reported as not verifiable with a note: Nextcloud applies the schema only when a schema wrapper is returned, so such a migration never makes its changes (a defect in the app) and re-running it cannot fix them. `if` guards are only trusted in the exact forms `if (!hasX('n')) { add n }` and `if (hasX('n')) { drop n }` (and `hasTable('t')` for effects on that table); other conditions, `if (false)`, conditional or computed `return` values, helper calls that receive the schema (any table may change), unsupported table methods and computed names make the affected migrations, and earlier effects on the tables concerned, not verifiable. Indexes are compared with their uniqueness. Anything it cannot parse (data changes in `preSchemaChange`/`postSchemaChange`, loops, helper calls, non-guard conditions, computed names) is listed under "Not verifiable" and never guessed. The database connection is taken from the Nextcloud config (`config/config.php` plus `config/*.config.php`); nothing is passed on the command line. @@ -122,7 +122,7 @@ The database connection is taken from the Nextcloud config (`config/config.php` **Options:** `--app` (default `spreed`), `--all` (all apps; an overview table and details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR` (default: `NEXTCLOUD_CONFIG_DIR`, else the `config/` of the Nextcloud root the script lives in, not the current directory), `--sqlite-file FILE`, `--since-nc N` (only migrations written after Nextcloud N was branched, e.g. `31`; derived from the date stamp in the migration version, so approximate for apps that are branched on their own schedule), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). -When something is found, the report ends with a "Suggested fix" block. A replay runs the whole migration again, not only the missing part, so the block only lists `NC_debug=true occ migrations:execute ` for migrations that are safe to run: never applied ones, and applied ones without a drop that a later migration undid and without data changes in `preSchemaChange()`/`postSchemaChange()`. The rest is listed under "Review by hand" with the reason. `migrations:execute` only exists with debug on, so `NC_debug=true` enables it for that one command and not for the instance. `migrations:execute` prints nothing on success, so each suggested command ends with `&& echo "OK: ..."`, and the block ends with the matching re-check command. The script only prints all this and never runs anything. +When something is found, the report ends with a "Suggested fix" block. A run executes the whole migration against the current schema, not only the part that is missing, so a command is only suggested when that is clearly safe: the migration has no code the check cannot interpret (data changes in `preSchemaChange()`/`postSchemaChange()` are fine for a migration that was never applied), none of its unguarded operations is already in place (it would fail), and it drops nothing that a later migration re-added or may have changed again. This applies to never applied migrations too. Everything else is listed under "Review by hand" with the reason. `migrations:execute` only exists with debug on, so `NC_debug=true` enables it for that one command and not for the instance. `migrations:execute` prints nothing on success, so each suggested command ends with `&& echo "OK: ..."`, and the block ends with the matching re-check command. The script only prints all this and never runs anything. The schema dump starts with a `prefix=` line, so a dump from an instance with another table prefix works as is; `--prefix` is for hand-made files. diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index ebdc86e..72f52e9 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -415,15 +415,17 @@ final class EffectExtractor { private bool $conditional = false; /** True when every return in changeSchema() is "return null;": Nextcloud then applies none of its changes. */ private bool $returnsNull = false; - /** @var array tables that code we could not interpret may have changed */ + /** @var array tables that code we could not interpret may have changed, also within this migration */ private array $touched = []; + /** @var array tables a condition made uncertain for later migrations (not for this migration's own effects) */ + private array $touchedLater = []; /** @var array effect keys that an early-return has*() guard already speaks about */ private array $guarded = []; /** @var array method => [index of "(", index of ")"] */ private array $params = []; /** - * @return array{effects:list>,reasons:list,touched:list} + * @return array{effects:list>,reasons:list,touched:list,touchedSelf:list} */ public static function extract(string $source): array { $self = new self(); @@ -464,7 +466,8 @@ public static function extract(string $source): array { return [ 'effects' => $self->effects, 'reasons' => array_keys($self->reasons), - 'touched' => array_keys($self->touched), + 'touched' => array_keys($self->touched + $self->touchedLater), + 'touchedSelf' => array_keys($self->touched), ]; } @@ -595,35 +598,31 @@ private function reason(string $text, int $tokenIndex): void { } /** - * Remember which tables a range of uninterpreted code may touch: bound table variables and string - * literals; when the code gets the schema or a table of unknown name but names no literal, any table. + * Remember which tables a range of uninterpreted code may touch: the table of a bound table or column + * variable, and any table when it gets the full schema or a table of unknown name (string literals + * passed along say nothing about what a helper does). */ private function touch(int $from, int $to): void { - $literal = false; - $anyTable = false; for ($i = $from; $i < $to && $i < count($this->T); $i++) { $t = $this->T[$i]; if ($t['t'] === T_RETURN) { - // handing the schema back does not change a table - $i = $this->statementEnd($i, $to); + $end = $this->statementEnd($i, $to); + if ($end - $i === 2 && ($this->T[$i + 1]['t'] === T_VARIABLE || strtolower($this->T[$i + 1]['s']) === 'null')) { + $i = $end; // "return $schema;" hands the schema back, it does not change a table + } continue; } - if ($t['t'] === T_VARIABLE && isset($this->vars[$t['s']])) { - $var = $this->vars[$t['s']]; - if ($var['k'] === 'table' && isset($var['name'])) { - $this->touched[$var['name']] = true; - } elseif ($var['k'] === 'column') { - $this->touched[strstr((string)($var['name'] ?? ''), '.', true) ?: '*'] = true; - } elseif ($var['k'] !== 'other') { - $anyTable = true; - } - } elseif ($t['t'] === T_CONSTANT_ENCAPSED_STRING) { - $this->touched[strtolower(substr($t['s'], 1, -1))] = true; - $literal = true; + if ($t['t'] !== T_VARIABLE || !isset($this->vars[$t['s']])) { + continue; + } + $var = $this->vars[$t['s']]; + if ($var['k'] === 'table' && isset($var['name'])) { + $this->touched[$var['name']] = true; + } elseif ($var['k'] === 'column') { + $this->touched[strstr((string)($var['name'] ?? ''), '.', true) ?: '*'] = true; + } elseif ($var['k'] !== 'other') { + $this->touched['*'] = true; } - } - if ($anyTable && !$literal) { - $this->touched['*'] = true; } } @@ -649,6 +648,7 @@ private function walk(int $i, int $end, bool $top): void { $this->walk($i + 1, $close, false); $i = $close + 1; } elseif ($tok['t'] === T_RETURN && $top) { + $this->inspectReturn($i, $this->statementEnd($i, $end)); return; } else { $stmtEnd = $this->statementEnd($i, $end); @@ -658,6 +658,21 @@ private function walk(int $i, int $end, bool $top): void { } } + /** "return null;" and "return $schema;" are fine; any other expression that gets the schema is uninterpreted. */ + private function inspectReturn(int $from, int $to): void { + $single = $to - $from === 2; + if ($single && (strtolower($this->T[$from + 1]['s']) === 'null' + || ($this->T[$from + 1]['t'] === T_VARIABLE && ($this->vars[$this->T[$from + 1]['s']]['k'] ?? '') === 'schema'))) { + return; + } + if ($this->mentions($from + 1, $to)) { + // e.g. "return $changed ? $schema : null": earlier effects may be thrown away + $this->discardEffects(); + $this->touch($from + 1, $to); + $this->reason('return expression that gets the schema is not understood', $from); + } + } + private function statementEnd(int $i, int $end): int { for ($j = $i; $j < $end; $j++) { if ($this->isOpen($j)) { @@ -744,6 +759,17 @@ private function handleIf(int $i): int { } } + // "return null" under a condition discards everything recorded before it when it triggers + foreach ($branches as [, , $bf, $bt]) { + for ($k = $bf; $k + 2 < $bt + 1; $k++) { + if ($this->T[$k]['t'] === T_RETURN && strtolower($this->T[$k + 1]['s'] ?? '') === 'null' + && ($this->T[$k + 2]['s'] ?? '') === ';') { + $this->discardEffects(); + break 2; + } + } + } + $guard = count($branches) === 1 ? $this->parseGuard($branches[0][0], $branches[0][1]) : null; if ($guard !== null) { [, , $from, $to] = $branches[0]; @@ -787,11 +813,22 @@ private function handleIf(int $i): int { return $j; } + /** Effects recorded so far may not happen at runtime (a later condition can return null), so stop asserting them. */ + private function discardEffects(): void { + foreach ($this->effects as $e) { + $this->touched[$e['table']] = true; + } + if ($this->effects !== []) { + $this->reasons['a conditional "return null" can discard earlier schema changes of this migration, they are not verifiable'] = true; + } + $this->effects = []; + } + /** * Recognise exactly "[!]$x->hasColumn|hasIndex|hasUniqueConstraint|hasTable|hasPrimaryKey('name')". * The key is the effect key the guard speaks about; table is set for hasTable guards, whose body may * touch anything of that table. - * @return ?array{neg:bool,key:string,table:?string} + * @return ?array{neg:bool,key:string,table:?string,column?:array{0:string,1:string}} */ private function parseGuard(int $from, int $to): ?array { $i = $from; @@ -820,7 +857,7 @@ private function parseGuard(int $from, int $to): ?array { } $t = $recv['name']; if ($method === 'hasColumn' && $arg !== null) { - return ['neg' => $neg, 'key' => "col:$t." . strtolower($arg), 'table' => null]; + return ['neg' => $neg, 'key' => "col:$t." . strtolower($arg), 'table' => null, 'column' => [$t, strtolower($arg)]]; } if (($method === 'hasIndex' || $method === 'hasUniqueConstraint') && $arg !== null) { return ['neg' => $neg, 'key' => "idx:$t:" . strtolower($arg), 'table' => null]; @@ -834,7 +871,7 @@ private function parseGuard(int $from, int $to): ?array { /** * A guard only vouches for effects that create (guard "!hasX") or remove (guard "hasX") exactly the * object it tests; any other effect in its body need not have happened. - * @param array{neg:bool,key:string,table:?string} $guard + * @param array{neg:bool,key:string,table:?string,column?:array{0:string,1:string}} $guard */ private function validateGuarded(array $guard, int $before): void { $kept = array_slice($this->effects, 0, $before); @@ -848,11 +885,17 @@ private function validateGuarded(array $guard, int $before): void { } } else { $ok = $adds === $guard['neg'] && effectKey($e) === $guard['key']; + // "if (!hasColumn('c')) { addColumn('c'); addIndex(['c', ...]) }": the index belongs to the new column + if (!$ok && $guard['neg'] && isset($guard['column']) && $e['op'] === 'addIndex' + && $e['table'] === $guard['column'][0] && in_array($guard['column'][1], $e['columns'], true)) { + $ok = true; + } } if ($ok) { + $e['guarded'] = true; $kept[] = $e; } else { - $this->touched[$e['table']] = true; + $this->touchedLater[$e['table']] = true; $this->reasons['schema changes under a has*() guard that does not guarantee them are not verifiable'] = true; } } @@ -925,6 +968,7 @@ private function statement(int $from, int $to): void { $touches = $this->mentions($from, $to); if ($first['t'] === T_RETURN) { + $this->inspectReturn($from, $to); return; } $assignTo = null; @@ -1013,7 +1057,20 @@ private function call(array $recv, string $method, array $args, int $at): array case 'hasTable': return $other; default: - $this->touched[$name === null ? '*' : strtolower($name)] = true; + // these only affect the table(s) named in their arguments, anything else may affect any table + $tableArgs = match ($method) { + 'dropAutoincrementColumn' => [0], + 'renameTable' => [0, 1], + default => null, + }; + if ($tableArgs === null) { + $this->touched['*'] = true; + } else { + foreach ($tableArgs as $n) { + $arg = isset($args[$n]) ? $this->str($args[$n]) : null; + $this->touched[$arg === null ? '*' : strtolower($arg)] = true; + } + } $this->reason("schema->$method() is not verifiable", $at); return $other; } @@ -1047,6 +1104,7 @@ private function call(array $recv, string $method, array $args, int $at): array return ['k' => 'column', 'name' => $t . '.' . $name]; case 'changeColumn': case 'modifyColumn': + $this->touched[$t ?? '*'] = true; $this->reason("column attribute change on $t.$name is not verifiable", $at); return $other; case 'addIndex': @@ -1071,6 +1129,7 @@ private function call(array $recv, string $method, array $args, int $at): array return $other; default: if (!in_array($method, self::IGNORED_TABLE_CALLS, true)) { + $this->touched[$t ?? '*'] = true; $this->reason("table->$method() is not verifiable", $at); } return $other; @@ -1094,10 +1153,13 @@ private function record(array $effect, int $at, array $required): void { if (isset($effect['columns'])) { $effect['columns'] = array_map('strtolower', $effect['columns']); } - if ($this->conditional && !isset($this->guarded[effectKey($effect)])) { - $this->touched[$effect['table']] = true; - $this->reasons['some schema changes follow a conditional early return and are not verifiable'] = true; - return; + if ($this->conditional) { + if (!isset($this->guarded[effectKey($effect)])) { + $this->touchedLater[$effect['table']] = true; + $this->reasons['some schema changes follow a conditional early return and are not verifiable'] = true; + return; + } + $effect['guarded'] = true; } $effect['line'] = $this->T[$at]['l']; $this->effects[] = $effect; @@ -1231,6 +1293,42 @@ function scopeFilter(array $opts): ?Closure { return static fn (string $version): bool => preg_match('/Date(\d{8})/', $version, $m) === 1 && $m[1] >= $since; } +/** + * Why running this migration (again) is not obviously safe, null if it is. A run executes the whole + * migration against the current schema, not only the part that is missing. + * @param array{effects:list>,reasons:list,touched:list} $x + * @param array>,reasons:list,touched:list}> $applied extracted applied migrations + * @param list}> $flat effects of the applied migrations in execution order + */ +function replayRisk(string $v, array $x, bool $wasApplied, array $applied, array $flat, array $schema): ?string { + foreach ($x['reasons'] as $r) { + if (str_starts_with($r, 'changeSchema() only ever returns null') + || (!$wasApplied && str_starts_with($r, 'data changes'))) { + continue; // harmless, or the data changes simply run for the first time + } + return 'it contains code this check cannot verify (' . preg_replace('/ \(line \d+\)$/', '', $r) . ')'; + } + foreach ($x['effects'] as $e) { + if (!($e['guarded'] ?? false) && checkEffect($e, $schema) === null) { + return 'a run would fail: ' . describeEffect($e) . ' is already in place and the migration does not check for it'; + } + if (!str_starts_with($e['op'], 'drop')) { + continue; + } + foreach ($flat as [$laterVersion, $later]) { + if (compareVersions($laterVersion, $v) > 0 && supersedes($later, $e)) { + return 'it contains ' . describeEffect($e) . ' which a later migration (' . $laterVersion . ') undid; a run would drop it again'; + } + } + foreach ($applied as $laterVersion => $later) { + if (compareVersions($laterVersion, $v) > 0 && (in_array($e['table'], $later['touched'], true) || in_array('*', $later['touched'], true))) { + return 'it contains ' . describeEffect($e) . ' and migration ' . $laterVersion . ' may have changed that table again (not verifiable)'; + } + } + } + return null; +} + /** * Print what an admin can do about the findings. Nothing is executed here. * @param array,review:array}> $fixes @@ -1258,7 +1356,7 @@ function printFix(array $fixes, string $recheck): void { echo "$recheck\n"; } if ($review !== []) { - echo "# Review by hand, no command suggested because a replay is not harmless:\n"; + echo "# Review by hand, no command suggested because running it is not obviously safe:\n"; foreach ($review as $app => $f) { foreach ($f['review'] as $v => $why) { echo "# $app $v: $why\n"; @@ -1330,9 +1428,10 @@ function checkApp(string $app, array $expected, array $applied, array $schema, s } // A later migration with code we cannot interpret may have changed this table again. foreach ($extracted as $laterVersion => $x) { - if (compareVersions($laterVersion, $v) > 0 - && (in_array($e['table'], $x['touched'], true) || in_array('*', $x['touched'], true))) { - $superseded[$v][] = describeEffect($e) . " (table may be changed by $laterVersion, not verifiable)"; + $touched = $laterVersion === $v ? $x['touchedSelf'] : $x['touched']; + if (compareVersions($laterVersion, $v) >= 0 + && (in_array($e['table'], $touched, true) || in_array('*', $touched, true))) { + $superseded[$v][] = describeEffect($e) . ($laterVersion === $v ? ' (table may be changed again by code of this migration, not verifiable)' : " (table may be changed by $laterVersion, not verifiable)"); continue 2; } } @@ -1356,21 +1455,22 @@ function checkApp(string $app, array $expected, array $applied, array $schema, s } $bad = $neverApplied !== [] || $notInEffect !== [] || ($strict && $unverifiable !== []); - // Never applied migrations just run for the first time. Replaying an applied one repeats everything - // it does: drops (of things a later migration may have re-added) and data changes. Those need a human. - $run = $neverApplied; + // Which migrations are safe to run again (or, for never applied ones, to run now)? + $run = []; $review = []; + $candidates = []; + foreach ($neverApplied as $v) { + $candidates[$v] = [EffectExtractor::extract($expected[$v]), false]; + } foreach (array_keys($notInEffect) as $v) { - // A drop that a later migration undid (re-added column, recreated table) must not run again; - // a drop that is itself what is missing is the repair. - $destructive = array_filter($superseded[$v] ?? [], static fn (string $d): bool => str_starts_with($d, 'drop')); - $dataChanges = array_filter($extracted[$v]['reasons'], static fn (string $r): bool => str_starts_with($r, 'data changes')); - if ($destructive !== []) { - $review[$v] = 'it contains ' . implode(', ', array_map(static fn (string $d): string => preg_replace('/ \(.*$/', '', $d), $destructive)) . ' which a later migration undid; a replay would drop it again'; - } elseif ($dataChanges !== []) { - $review[$v] = 'it changes data in pre/postSchemaChange(); a replay would repeat that'; - } else { + $candidates[$v] = [$extracted[$v], true]; + } + foreach ($candidates as $v => [$x, $wasApplied]) { + $why = replayRisk($v, $x, $wasApplied, $extracted, $flat, $schema); + if ($why === null) { $run[] = $v; + } else { + $review[$v] = $why; } } usort($run, 'compareVersions'); From 3e4c5ab9b983620fa7633d0df1ad75617dd80550 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 15:06:22 +0200 Subject: [PATCH 14/18] fix(admin-tools): vet replays against guards and the current schema A run executes a whole migration, so the suggested fix now evaluates the guards around the missing parts against the current schema, only calls an operation repeatable when its own object is guarded, and checks operation prerequisites such as taken index names and existing primary keys. Conflicts with later migrations apply to additions too, never applied migrations that only return null go to manual review, and conditions and computed index names count as uninterpreted code. The suggested commands form one && chain so nothing runs after a failure, and the re-check keeps the original arguments. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 2 +- admin-tools/check-app-migrations.php | 133 ++++++++++++++++++++++----- 2 files changed, 111 insertions(+), 24 deletions(-) diff --git a/admin-tools/README.md b/admin-tools/README.md index e17f234..af58289 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -122,7 +122,7 @@ The database connection is taken from the Nextcloud config (`config/config.php` **Options:** `--app` (default `spreed`), `--all` (all apps; an overview table and details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR` (default: `NEXTCLOUD_CONFIG_DIR`, else the `config/` of the Nextcloud root the script lives in, not the current directory), `--sqlite-file FILE`, `--since-nc N` (only migrations written after Nextcloud N was branched, e.g. `31`; derived from the date stamp in the migration version, so approximate for apps that are branched on their own schedule), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). -When something is found, the report ends with a "Suggested fix" block. A run executes the whole migration against the current schema, not only the part that is missing, so a command is only suggested when that is clearly safe: the migration has no code the check cannot interpret (data changes in `preSchemaChange()`/`postSchemaChange()` are fine for a migration that was never applied), none of its unguarded operations is already in place (it would fail), and it drops nothing that a later migration re-added or may have changed again. This applies to never applied migrations too. Everything else is listed under "Review by hand" with the reason. `migrations:execute` only exists with debug on, so `NC_debug=true` enables it for that one command and not for the instance. `migrations:execute` prints nothing on success, so each suggested command ends with `&& echo "OK: ..."`, and the block ends with the matching re-check command. The script only prints all this and never runs anything. +When something is found, the report ends with a "Suggested fix" block. A run executes the whole migration against the current schema, not only the part that is missing, so a command is only suggested when that is clearly safe, for never applied migrations as well as applied ones: the migration has no code the check cannot interpret (data changes in `preSchemaChange()`/`postSchemaChange()` are fine for a never applied migration, which just runs for the first time), its guards (`if (!hasColumn(..))` and so on) would let the missing part run now, no unguarded operation is already in place or collides with an existing index name or primary key (it would fail), and no later migration undid or may have changed again what it does. Everything else is listed under "Review by hand" with the reason. The commands form one `&&` chain, so nothing runs after a failed migration. `migrations:execute` only exists with debug on, so `NC_debug=true` enables it for that one command and not for the instance. `migrations:execute` prints nothing on success, so each suggested command ends with `&& echo "OK: ..."`, and the block ends with the same check command with the arguments you used. The script only prints all this and never runs anything. The schema dump starts with a `prefix=` line, so a dump from an instance with another table prefix works as is; `--prefix` is for hand-made files. diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index 72f52e9..1fa7828 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -419,8 +419,10 @@ final class EffectExtractor { private array $touched = []; /** @var array tables a condition made uncertain for later migrations (not for this migration's own effects) */ private array $touchedLater = []; - /** @var array effect keys that an early-return has*() guard already speaks about */ + /** @var array> early-return has*() guards by the effect key they speak about; "neg" tells when the code after them runs */ private array $guarded = []; + /** @var list> guards of the if() blocks around the code being walked */ + private array $guardStack = []; /** @var array method => [index of "(", index of ")"] */ private array $params = []; @@ -781,12 +783,15 @@ private function handleIf(int $i): int { return $j; } $before = count($this->effects); + $this->guardStack[] = $guard; $this->walk($from, $to, false); + array_pop($this->guardStack); $this->validateGuarded($guard, $before); for ($k = $from; $k < $to; $k++) { if ($this->T[$k]['t'] === T_RETURN) { $this->conditional = true; - $this->guarded[$guard['key']] = true; + // the code after "if (X) { return; }" runs when X is false + $this->guarded[$guard['key']] = ['neg' => !$guard['neg']] + $guard; break; } } @@ -803,7 +808,8 @@ private function handleIf(int $i): int { } foreach ($branches as [$cf, $ct, $bf, $bt]) { if ($this->mentions($cf, $ct) || $this->mentions($bf, $bt)) { - foreach ($branches as [, , $tf, $tt]) { + foreach ($branches as [$tcf, $tct, $tf, $tt]) { + $this->touch($tcf, $tct); $this->touch($tf, $tt); } $this->reason('schema changes under a condition that is not a plain has*() guard are not verifiable', $i); @@ -892,7 +898,6 @@ private function validateGuarded(array $guard, int $before): void { } } if ($ok) { - $e['guarded'] = true; $kept[] = $e; } else { $this->touchedLater[$e['table']] = true; @@ -937,6 +942,10 @@ private function str(array $range): ?string { } /** @return ?list */ + private function isNullLiteral(array $range): bool { + return $range[1] - $range[0] === 1 && strtolower($this->T[$range[0]]['s']) === 'null'; + } + private function strList(array $range): ?array { [$a, $b] = $range; if ($this->T[$a]['s'] === '[' && $this->close($a) === $b - 1) { @@ -1111,10 +1120,12 @@ private function call(array $recv, string $method, array $args, int $at): array case 'addUniqueIndex': $cols = isset($args[0]) ? $this->strList($args[0]) : null; $idx = isset($args[1]) ? $this->str($args[1]) : null; + // an omitted or literal null name is fine, a name that is computed is not + $computedName = isset($args[1]) && $idx === null && !$this->isNullLiteral($args[1]); $this->record([ 'op' => 'addIndex', 'table' => $t, 'columns' => $cols, 'index' => $idx, 'unique' => $method === 'addUniqueIndex', - ], $at, [$t, $cols]); + ], $at, [$t, $cols, $computedName ? null : true]); return $other; case 'setPrimaryKey': $cols = isset($args[0]) ? $this->strList($args[0]) : null; @@ -1159,8 +1170,25 @@ private function record(array $effect, int $at, array $required): void { $this->reasons['some schema changes follow a conditional early return and are not verifiable'] = true; return; } - $effect['guarded'] = true; } + $key = effectKey($effect); + $guards = $this->guardStack; + if ($this->conditional) { + $guards[] = $this->guarded[$key]; + } + // An operation can be repeated without failing only if its own object is checked, or the whole + // block is skipped when the table exists ("if (!hasTable('t')) { create t and fill it }"). + $adds = in_array($effect['op'], ['addColumn', 'createTable', 'addIndex', 'setPrimaryKey'], true); + $protected = false; + foreach ($guards as $g) { + if ($g['table'] !== null) { + $protected = $protected || ($g['neg'] && $g['table'] === $effect['table']); + } else { + $protected = $protected || ($g['key'] === $key && $adds === $g['neg']); + } + } + $effect['guards'] = $guards; + $effect['protected'] = $protected; $effect['line'] = $this->T[$at]['l']; $this->effects[] = $effect; } @@ -1293,31 +1321,81 @@ function scopeFilter(array $opts): ?Closure { return static fn (string $version): bool => preg_match('/Date(\d{8})/', $version, $m) === 1 && $m[1] >= $since; } +/** Does the object a guard tests exist in the current schema? */ +function guardObjectExists(string $key, array $schema): bool { + if (str_starts_with($key, 'tbl:')) { + return isset($schema['columns'][substr($key, 4)]); + } + if (str_starts_with($key, 'col:')) { + [$table, $column] = explode('.', substr($key, 4), 2); + return isset($schema['columns'][$table][$column]); + } + // idx:
: + [, $table, $name] = explode(':', $key, 3); + foreach ($schema['indexes'][$table] ?? [] as $existing => $idx) { + if (strtolower((string)$existing) === strtolower($name)) { + return true; + } + } + return false; +} + +function isAddOp(array $e): bool { + return in_array($e['op'], ['addColumn', 'createTable', 'addIndex', 'setPrimaryKey'], true); +} + /** * Why running this migration (again) is not obviously safe, null if it is. A run executes the whole - * migration against the current schema, not only the part that is missing. + * migration against the current schema, not only the part that is missing, so besides code the check + * cannot interpret it has to be clear that every operation can run now, that the guards around the + * missing parts let them run, and that no later migration undid what the run would do. * @param array{effects:list>,reasons:list,touched:list} $x * @param array>,reasons:list,touched:list}> $applied extracted applied migrations * @param list}> $flat effects of the applied migrations in execution order */ function replayRisk(string $v, array $x, bool $wasApplied, array $applied, array $flat, array $schema): ?string { foreach ($x['reasons'] as $r) { - if (str_starts_with($r, 'changeSchema() only ever returns null') - || (!$wasApplied && str_starts_with($r, 'data changes'))) { - continue; // harmless, or the data changes simply run for the first time + if (!$wasApplied && str_starts_with($r, 'data changes')) { + continue; // simply runs for the first time + } + if (str_starts_with($r, 'changeSchema() only ever returns null')) { + return 'it only returns null, so Nextcloud would just record it as applied without changing the schema'; } return 'it contains code this check cannot verify (' . preg_replace('/ \(line \d+\)$/', '', $r) . ')'; } - foreach ($x['effects'] as $e) { - if (!($e['guarded'] ?? false) && checkEffect($e, $schema) === null) { - return 'a run would fail: ' . describeEffect($e) . ' is already in place and the migration does not check for it'; - } - if (!str_starts_with($e['op'], 'drop')) { - continue; + foreach ($x['effects'] as $pos => $e) { + $problem = checkEffect($e, $schema); + if ($problem === null) { + if (!$e['protected']) { + return 'a run would fail: ' . describeEffect($e) . ' is already in place and the migration does not check for it'; + } + } else { + foreach ($e['guards'] as $g) { + if ($g['neg'] === guardObjectExists($g['key'], $schema)) { + return 'its guard around ' . describeEffect($e) . ' would skip the block now, so a run would not repair it'; + } + } + $earlierDrops = array_slice($x['effects'], 0, $pos); + if ($e['op'] === 'addIndex' && isset($e['index'])) { + foreach ($schema['indexes'][$e['table']] ?? [] as $existing => $idx) { + $dropped = array_filter($earlierDrops, static fn (array $d): bool => $d['op'] === 'dropIndex' && $d['table'] === $e['table'] && $d['index'] === $e['index']); + if (strtolower((string)$existing) === $e['index'] && $dropped === []) { + return 'a run would fail: the index name of ' . describeEffect($e) . ' is taken by a different index'; + } + } + } + if ($e['op'] === 'setPrimaryKey' && isset($schema['indexes'][$e['table']]['PRIMARY']) + && array_filter($earlierDrops, static fn (array $d): bool => $d['op'] === 'dropPrimaryKey' && $d['table'] === $e['table']) === []) { + return 'a run would fail: ' . $e['table'] . ' already has a different primary key'; + } } foreach ($flat as [$laterVersion, $later]) { - if (compareVersions($laterVersion, $v) > 0 && supersedes($later, $e)) { - return 'it contains ' . describeEffect($e) . ' which a later migration (' . $laterVersion . ') undid; a run would drop it again'; + if (compareVersions($laterVersion, $v) <= 0 || !supersedes($later, $e)) { + continue; + } + $tableLevel = in_array($later['op'], ['createTable', 'dropTable'], true); + if ($tableLevel || isAddOp($later) !== isAddOp($e)) { + return 'it contains ' . describeEffect($e) . ' which a later migration (' . $laterVersion . ') undid with ' . describeEffect($later) . '; a run would redo it'; } } foreach ($applied as $laterVersion => $later) { @@ -1333,7 +1411,7 @@ function replayRisk(string $v, array $x, bool $wasApplied, array $applied, array * Print what an admin can do about the findings. Nothing is executed here. * @param array,review:array}> $fixes */ -function printFix(array $fixes, string $recheck): void { +function printFix(array $fixes, string $recheck, bool $fromFiles): void { if ($fixes === []) { return; } @@ -1345,14 +1423,19 @@ function printFix(array $fixes, string $recheck): void { if ($run !== []) { echo "# Before: take a DB backup/snapshot and enable maintenance mode: occ maintenance:mode --on\n"; echo "# migrations:execute is only available with debug on; NC_debug=true enables it for this one command only\n"; - echo "# In this order:\n"; + echo "# In this order, one chain: nothing runs after a failed migration:\n"; + $lines = []; foreach ($run as $app => $f) { foreach ($f['run'] as $v) { // migrations:execute prints nothing on success, so make the exit code visible - echo "NC_debug=true occ migrations:execute $app $v && echo \"OK: $app $v\"\n"; + $lines[] = "NC_debug=true occ migrations:execute $app $v && echo \"OK: $app $v\""; } } + echo implode(" \\\n\t&& ", $lines) . "\n"; echo "# After: occ maintenance:mode --off; then check the result (expect no findings left):\n"; + if ($fromFiles) { + echo "# (this run read dump files: take new ones with --dump-applied and --dump-schema first)\n"; + } echo "$recheck\n"; } if ($review !== []) { @@ -1722,8 +1805,12 @@ function main(array $argv): int { if ($all) { printOverview($results, $prefix, $source, $opts); } - $recheck = $argv[0] . ($all ? ' --all --db' : ' --app ' . $app . ' --db') . (isset($opts['since-nc']) ? ' --since-nc ' . (int)$opts['since-nc'] : ''); - printFix($fixes, $recheck); + // the same check again: same arguments, quoted for a shell + $recheck = implode(' ', array_map( + static fn (string $a): string => preg_match('/^[A-Za-z0-9_.\/=:@%+,-]+$/', $a) === 1 ? $a : escapeshellarg($a), + $argv + )); + printFix($fixes, $recheck, !$useDb); if ($skipped !== []) { echo 'Skipped (not on disk, e.g. disabled or removed apps): ' . implode(', ', $skipped) . "\n"; } From f3a4608b998929788193679ff9545c2853149b44 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 15:20:52 +0200 Subject: [PATCH 15/18] fix(admin-tools): treat opaque code in changeSchema as unsafe to replay Code in changeSchema() that never touches a schema object, such as a DB statement, was ignored, so a migration that also adds a missing column could get a replay command that repeats the statement. Any call other than on $output and a few pure functions now counts as code the check cannot interpret and sends the migration to manual review. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 2 +- admin-tools/check-app-migrations.php | 34 ++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/admin-tools/README.md b/admin-tools/README.md index af58289..56692d3 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -122,7 +122,7 @@ The database connection is taken from the Nextcloud config (`config/config.php` **Options:** `--app` (default `spreed`), `--all` (all apps; an overview table and details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR` (default: `NEXTCLOUD_CONFIG_DIR`, else the `config/` of the Nextcloud root the script lives in, not the current directory), `--sqlite-file FILE`, `--since-nc N` (only migrations written after Nextcloud N was branched, e.g. `31`; derived from the date stamp in the migration version, so approximate for apps that are branched on their own schedule), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). -When something is found, the report ends with a "Suggested fix" block. A run executes the whole migration against the current schema, not only the part that is missing, so a command is only suggested when that is clearly safe, for never applied migrations as well as applied ones: the migration has no code the check cannot interpret (data changes in `preSchemaChange()`/`postSchemaChange()` are fine for a never applied migration, which just runs for the first time), its guards (`if (!hasColumn(..))` and so on) would let the missing part run now, no unguarded operation is already in place or collides with an existing index name or primary key (it would fail), and no later migration undid or may have changed again what it does. Everything else is listed under "Review by hand" with the reason. The commands form one `&&` chain, so nothing runs after a failed migration. `migrations:execute` only exists with debug on, so `NC_debug=true` enables it for that one command and not for the instance. `migrations:execute` prints nothing on success, so each suggested command ends with `&& echo "OK: ..."`, and the block ends with the same check command with the arguments you used. The script only prints all this and never runs anything. +When something is found, the report ends with a "Suggested fix" block. A run executes the whole migration against the current schema, not only the part that is missing, so a command is only suggested when that is clearly safe, for never applied migrations as well as applied ones: the migration has no code the check cannot interpret, including calls in `changeSchema()` that never touch the schema, such as DB statements (data changes in `preSchemaChange()`/`postSchemaChange()` are fine for a never applied migration, which just runs for the first time), its guards (`if (!hasColumn(..))` and so on) would let the missing part run now, no unguarded operation is already in place or collides with an existing index name or primary key (it would fail), and no later migration undid or may have changed again what it does. Everything else is listed under "Review by hand" with the reason. The commands form one `&&` chain, so nothing runs after a failed migration. `migrations:execute` only exists with debug on, so `NC_debug=true` enables it for that one command and not for the instance. `migrations:execute` prints nothing on success, so each suggested command ends with `&& echo "OK: ..."`, and the block ends with the same check command with the arguments you used. The script only prints all this and never runs anything. The schema dump starts with a `prefix=` line, so a dump from an instance with another table prefix works as is; `--prefix` is for hand-made files. diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index 1fa7828..48b2957 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -398,6 +398,7 @@ function dumpSchema(PDO $pdo, string $prefix): void { final class EffectExtractor { private const LOOPS = [T_FOREACH, T_FOR, T_WHILE, T_DO, T_SWITCH, T_TRY, T_MATCH]; private const GUARD_CALLS = ['hasColumn', 'hasIndex', 'hasTable', 'hasPrimaryKey', 'hasUniqueConstraint']; + private const PURE_FUNCTIONS = ['isset', 'empty', 'count', 'in_array', 'is_array', 'is_null', 'strtolower', 'strtoupper']; private const IGNORED_TABLE_CALLS = [ 'hasColumn', 'hasIndex', 'hasPrimaryKey', 'hasUniqueConstraint', 'getName', 'addOption', 'setComment', ]; @@ -628,6 +629,24 @@ private function touch(int $from, int $to): void { } } + /** + * Code in changeSchema() that never touches the schema can still do things a replay would repeat + * (a DB statement, a config change). Any call other than on $output and a few pure functions counts. + */ + private function noteOpaque(int $from, int $to): void { + for ($i = $from; $i < $to && $i + 1 < count($this->T); $i++) { + if ($this->T[$i]['s'] === '$output' && ($this->T[$i + 1]['t'] ?? null) === T_OBJECT_OPERATOR) { + $i += 2; // $output->info(...) only prints + continue; + } + if ($this->T[$i + 1]['s'] === '(' && in_array($this->T[$i]['t'], [T_STRING, T_VARIABLE], true) + && !in_array(strtolower($this->T[$i]['s']), self::PURE_FUNCTIONS, true)) { + $this->reason('changeSchema() runs code that is not interpreted', $i); + return; + } + } + } + private function mentions(int $from, int $to): bool { for ($i = $from; $i < $to; $i++) { if ($this->T[$i]['t'] === T_VARIABLE && isset($this->vars[$this->T[$i]['s']]) @@ -667,6 +686,10 @@ private function inspectReturn(int $from, int $to): void { || ($this->T[$from + 1]['t'] === T_VARIABLE && ($this->vars[$this->T[$from + 1]['s']]['k'] ?? '') === 'schema'))) { return; } + if (!$this->mentions($from + 1, $to)) { + $this->noteOpaque($from + 1, $to); + return; + } if ($this->mentions($from + 1, $to)) { // e.g. "return $changed ? $schema : null": earlier effects may be thrown away $this->discardEffects(); @@ -715,6 +738,9 @@ private function skipConstruct(int $i, int $end): int { break; } } + if (!$this->mentions($start, $j)) { + $this->noteOpaque($start, $j); + } if ($this->mentions($start, $j)) { $this->touch($start, $j); $this->reason('schema changes inside ' . strtolower(substr(token_name((int)$this->T[$start]['t']), 2)) . ' are not verifiable', $start); @@ -807,6 +833,10 @@ private function handleIf(int $i): int { } } foreach ($branches as [$cf, $ct, $bf, $bt]) { + if (!$this->mentions($cf, $ct) && !$this->mentions($bf, $bt)) { + $this->noteOpaque($cf, $ct); + $this->noteOpaque($bf, $bt); + } if ($this->mentions($cf, $ct) || $this->mentions($bf, $bt)) { foreach ($branches as [$tcf, $tct, $tf, $tt]) { $this->touch($tcf, $tct); @@ -991,6 +1021,8 @@ private function statement(int $from, int $to): void { if ($touches) { $this->touch($from, $to); $this->reason('statement touching the schema is not understood', $from); + } else { + $this->noteOpaque($from, $to); } if ($assignTo !== null) { $this->vars[$assignTo] = ['k' => 'other']; @@ -1001,6 +1033,8 @@ private function statement(int $from, int $to): void { if ($touches) { $this->touch($from, $to); $this->reason('schema passed to code that is not parsed', $from); + } else { + $this->noteOpaque($from, $to); } if ($assignTo !== null) { $this->vars[$assignTo] = ['k' => 'other']; From 30bc06ce21c5f886a775d8a6461ae600b1142e72 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 15:38:02 +0200 Subject: [PATCH 16/18] fix(admin-tools): validate app ids from the database and quote commands With --all the app id comes from the migrations table without the check that --app gets. An id with shell syntax or ../ that matched a path could reach the generated command or the app directory lookup. Ids that are not plain app ids are now ignored and counted, and the app, version and success message in the suggested commands are shell-quoted. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 2 +- admin-tools/check-app-migrations.php | 22 +++++++++++++++++----- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/admin-tools/README.md b/admin-tools/README.md index 56692d3..a38ee4a 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -122,7 +122,7 @@ The database connection is taken from the Nextcloud config (`config/config.php` **Options:** `--app` (default `spreed`), `--all` (all apps; an overview table and details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR` (default: `NEXTCLOUD_CONFIG_DIR`, else the `config/` of the Nextcloud root the script lives in, not the current directory), `--sqlite-file FILE`, `--since-nc N` (only migrations written after Nextcloud N was branched, e.g. `31`; derived from the date stamp in the migration version, so approximate for apps that are branched on their own schedule), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). -When something is found, the report ends with a "Suggested fix" block. A run executes the whole migration against the current schema, not only the part that is missing, so a command is only suggested when that is clearly safe, for never applied migrations as well as applied ones: the migration has no code the check cannot interpret, including calls in `changeSchema()` that never touch the schema, such as DB statements (data changes in `preSchemaChange()`/`postSchemaChange()` are fine for a never applied migration, which just runs for the first time), its guards (`if (!hasColumn(..))` and so on) would let the missing part run now, no unguarded operation is already in place or collides with an existing index name or primary key (it would fail), and no later migration undid or may have changed again what it does. Everything else is listed under "Review by hand" with the reason. The commands form one `&&` chain, so nothing runs after a failed migration. `migrations:execute` only exists with debug on, so `NC_debug=true` enables it for that one command and not for the instance. `migrations:execute` prints nothing on success, so each suggested command ends with `&& echo "OK: ..."`, and the block ends with the same check command with the arguments you used. The script only prints all this and never runs anything. +When something is found, the report ends with a "Suggested fix" block. A run executes the whole migration against the current schema, not only the part that is missing, so a command is only suggested when that is clearly safe, for never applied migrations as well as applied ones: the migration has no code the check cannot interpret, including calls in `changeSchema()` that never touch the schema, such as DB statements (data changes in `preSchemaChange()`/`postSchemaChange()` are fine for a never applied migration, which just runs for the first time), its guards (`if (!hasColumn(..))` and so on) would let the missing part run now, no unguarded operation is already in place or collides with an existing index name or primary key (it would fail), and no later migration undid or may have changed again what it does. Everything else is listed under "Review by hand" with the reason. App ids read from the migrations table by `--all` must be plain ids (lowercase letters, digits, underscore); others are ignored and counted, and every word in the generated commands is shell-quoted. The commands form one `&&` chain, so nothing runs after a failed migration. `migrations:execute` only exists with debug on, so `NC_debug=true` enables it for that one command and not for the instance. `migrations:execute` prints nothing on success, so each suggested command ends with `&& echo "OK: ..."`, and the block ends with the same check command with the arguments you used. The script only prints all this and never runs anything. The schema dump starts with a `prefix=` line, so a dump from an instance with another table prefix works as is; `--prefix` is for hand-made files. diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index 48b2957..ff39339 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -1441,6 +1441,11 @@ function replayRisk(string $v, array $x, bool $wasApplied, array $applied, array return null; } +/** A word for a shell command line: left alone when it only has safe characters, quoted otherwise. */ +function shellWord(string $word): string { + return preg_match('/^[A-Za-z0-9_.\/=:@%+,-]+$/', $word) === 1 ? $word : escapeshellarg($word); +} + /** * Print what an admin can do about the findings. Nothing is executed here. * @param array,review:array}> $fixes @@ -1462,7 +1467,8 @@ function printFix(array $fixes, string $recheck, bool $fromFiles): void { foreach ($run as $app => $f) { foreach ($f['run'] as $v) { // migrations:execute prints nothing on success, so make the exit code visible - $lines[] = "NC_debug=true occ migrations:execute $app $v && echo \"OK: $app $v\""; + $lines[] = 'NC_debug=true occ migrations:execute ' . shellWord((string)$app) . ' ' . shellWord($v) + . ' && echo ' . escapeshellarg("OK: $app $v"); } } echo implode(" \\\n\t&& ", $lines) . "\n"; @@ -1817,7 +1823,13 @@ function main(array $argv): int { $skipped = []; $fixes = []; $results = []; + $invalid = 0; foreach ($appliedByApp as $name => $applied) { + if (preg_match('/^[a-z0-9_]+$/', (string)$name) !== 1) { + // an id from the database that is not a plain app id never reaches a path or a command + $invalid++; + continue; + } $dir = migrationDir($name, $ncRoot, $all ? null : $appPath, $cfgForLookup); if ($dir === null || !is_dir($dir)) { if (!$all) { @@ -1840,11 +1852,11 @@ function main(array $argv): int { printOverview($results, $prefix, $source, $opts); } // the same check again: same arguments, quoted for a shell - $recheck = implode(' ', array_map( - static fn (string $a): string => preg_match('/^[A-Za-z0-9_.\/=:@%+,-]+$/', $a) === 1 ? $a : escapeshellarg($a), - $argv - )); + $recheck = implode(' ', array_map('shellWord', $argv)); printFix($fixes, $recheck, !$useDb); + if ($invalid > 0) { + echo "Ignored $invalid app id(s) in the migrations table that are not plain app ids (lowercase letters, digits, underscore)\n"; + } if ($skipped !== []) { echo 'Skipped (not on disk, e.g. disabled or removed apps): ' . implode(', ', $skipped) . "\n"; } From 964c811d377fa2242ecca537ddf465c269c4eac4 Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Thu, 8 Oct 2026 15:51:40 +0200 Subject: [PATCH 17/18] fix(admin-tools): close remaining gaps in check-app-migrations replay vetting Qualified function calls (T_NAME_*) and calls inside schema call arguments now count as code the check cannot interpret. A return value that is neither the schema nor null, and return null inside a skipped loop or try block, discard the effects recorded before them. A guard proof no longer survives another early return that can bypass the effect, and a run is only suggested when the table and the index columns it needs exist or are created earlier in the same migration. Signed-off-by: Misha M.-Kupriyanov --- admin-tools/README.md | 4 +- admin-tools/check-app-migrations.php | 92 ++++++++++++++++++++-------- 2 files changed, 70 insertions(+), 26 deletions(-) diff --git a/admin-tools/README.md b/admin-tools/README.md index a38ee4a..153daf5 100644 --- a/admin-tools/README.md +++ b/admin-tools/README.md @@ -97,7 +97,7 @@ Update email addresses in Nextcloud mail app configuration when a customer's ema Read-only check of an app's migrations against the database. Reports migrations that were **never applied** (no row in `oc_migrations`) and migrations that are **applied but not in effect** (row exists, but the columns, tables or indexes they create are missing, or what they drop is still there). Example: Talk failing with `Unknown column 'r.last_pinned_id'` although `23000Date20251030090219` is recorded as applied. -The expected migrations are the `Version*.php` files in the app's `lib/Migration`, i.e. what is deployed there. No git is used. Schema effects (`addColumn`, `createTable`, `addIndex`, `setPrimaryKey`, `dropColumn`, `dropTable`, `dropIndex`, `renameColumn`) are extracted statically from `changeSchema()`. `hasColumn`-style guards are understood. Effects that a later applied migration overrides are skipped. A `changeSchema()` whose every `return` is `return null;` is reported as not verifiable with a note: Nextcloud applies the schema only when a schema wrapper is returned, so such a migration never makes its changes (a defect in the app) and re-running it cannot fix them. `if` guards are only trusted in the exact forms `if (!hasX('n')) { add n }` and `if (hasX('n')) { drop n }` (and `hasTable('t')` for effects on that table); other conditions, `if (false)`, conditional or computed `return` values, helper calls that receive the schema (any table may change), unsupported table methods and computed names make the affected migrations, and earlier effects on the tables concerned, not verifiable. Indexes are compared with their uniqueness. Anything it cannot parse (data changes in `preSchemaChange`/`postSchemaChange`, loops, helper calls, non-guard conditions, computed names) is listed under "Not verifiable" and never guessed. +The expected migrations are the `Version*.php` files in the app's `lib/Migration`, i.e. what is deployed there. No git is used. Schema effects (`addColumn`, `createTable`, `addIndex`, `setPrimaryKey`, `dropColumn`, `dropTable`, `dropIndex`, `renameColumn`) are extracted statically from `changeSchema()`. `hasColumn`-style guards are understood. Effects that a later applied migration overrides are skipped. A `changeSchema()` whose every `return` is `return null;` is reported as not verifiable with a note: Nextcloud applies the schema only when a schema wrapper is returned, so such a migration never makes its changes (a defect in the app) and re-running it cannot fix them. `if` guards are only trusted in the exact forms `if (!hasX('n')) { add n }` and `if (hasX('n')) { drop n }` (and `hasTable('t')` for effects on that table); other conditions, `if (false)`, conditional, computed or unknown `return` values (including `return null` inside loops and `try` blocks), helper calls that receive the schema (any table may change), unsupported table methods and computed names make the affected migrations, and earlier effects on the tables concerned, not verifiable. Indexes are compared with their uniqueness. Anything it cannot parse (data changes in `preSchemaChange`/`postSchemaChange`, loops, helper calls, non-guard conditions, computed names) is listed under "Not verifiable" and never guessed. The database connection is taken from the Nextcloud config (`config/config.php` plus `config/*.config.php`); nothing is passed on the command line. @@ -122,7 +122,7 @@ The database connection is taken from the Nextcloud config (`config/config.php` **Options:** `--app` (default `spreed`), `--all` (all apps; an overview table and details only for apps with findings, `--verbose` shows the rest), `--app-path` (default: looked up via `apps_paths` in the Nextcloud config, else `apps/`, `apps-external/`, `custom_apps/`), `--config DIR` (default: `NEXTCLOUD_CONFIG_DIR`, else the `config/` of the Nextcloud root the script lives in, not the current directory), `--sqlite-file FILE`, `--since-nc N` (only migrations written after Nextcloud N was branched, e.g. `31`; derived from the date stamp in the migration version, so approximate for apps that are branched on their own schedule), `--strict` (also fail on "not verifiable"), `--verbose` (list verified and superseded effects). -When something is found, the report ends with a "Suggested fix" block. A run executes the whole migration against the current schema, not only the part that is missing, so a command is only suggested when that is clearly safe, for never applied migrations as well as applied ones: the migration has no code the check cannot interpret, including calls in `changeSchema()` that never touch the schema, such as DB statements (data changes in `preSchemaChange()`/`postSchemaChange()` are fine for a never applied migration, which just runs for the first time), its guards (`if (!hasColumn(..))` and so on) would let the missing part run now, no unguarded operation is already in place or collides with an existing index name or primary key (it would fail), and no later migration undid or may have changed again what it does. Everything else is listed under "Review by hand" with the reason. App ids read from the migrations table by `--all` must be plain ids (lowercase letters, digits, underscore); others are ignored and counted, and every word in the generated commands is shell-quoted. The commands form one `&&` chain, so nothing runs after a failed migration. `migrations:execute` only exists with debug on, so `NC_debug=true` enables it for that one command and not for the instance. `migrations:execute` prints nothing on success, so each suggested command ends with `&& echo "OK: ..."`, and the block ends with the same check command with the arguments you used. The script only prints all this and never runs anything. +When something is found, the report ends with a "Suggested fix" block. A run executes the whole migration against the current schema, not only the part that is missing, so a command is only suggested when that is clearly safe, for never applied migrations as well as applied ones: the migration has no code the check cannot interpret, including calls in `changeSchema()` that never touch the schema, such as DB statements (data changes in `preSchemaChange()`/`postSchemaChange()` are fine for a never applied migration, which just runs for the first time), its guards (`if (!hasColumn(..))` and so on) would let the missing part run now, no unguarded operation is already in place or collides with an existing index name or primary key (it would fail), the tables and columns it needs exist or are created earlier in the same migration, and no later migration undid or may have changed again what it does. Everything else is listed under "Review by hand" with the reason. App ids read from the migrations table by `--all` must be plain ids (lowercase letters, digits, underscore); others are ignored and counted, and every word in the generated commands is shell-quoted. The commands form one `&&` chain, so nothing runs after a failed migration. `migrations:execute` only exists with debug on, so `NC_debug=true` enables it for that one command and not for the instance. `migrations:execute` prints nothing on success, so each suggested command ends with `&& echo "OK: ..."`, and the block ends with the same check command with the arguments you used. The script only prints all this and never runs anything. The schema dump starts with a `prefix=` line, so a dump from an instance with another table prefix works as is; `--prefix` is for hand-made files. diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index ff39339..08fdddb 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -630,21 +630,45 @@ private function touch(int $from, int $to): void { } /** - * Code in changeSchema() that never touches the schema can still do things a replay would repeat - * (a DB statement, a config change). Any call other than on $output and a few pure functions counts. + * Index of the first call in the range that a replay would repeat without us knowing what it does: + * any call other than on $output and a few pure functions, however the name is written. */ - private function noteOpaque(int $from, int $to): void { + private function findOpaque(int $from, int $to): ?int { for ($i = $from; $i < $to && $i + 1 < count($this->T); $i++) { if ($this->T[$i]['s'] === '$output' && ($this->T[$i + 1]['t'] ?? null) === T_OBJECT_OPERATOR) { $i += 2; // $output->info(...) only prints continue; } - if ($this->T[$i + 1]['s'] === '(' && in_array($this->T[$i]['t'], [T_STRING, T_VARIABLE], true) + if ($this->T[$i + 1]['s'] === '(' + && in_array($this->T[$i]['t'], [T_STRING, T_VARIABLE, T_NAME_QUALIFIED, T_NAME_FULLY_QUALIFIED, T_NAME_RELATIVE], true) && !in_array(strtolower($this->T[$i]['s']), self::PURE_FUNCTIONS, true)) { - $this->reason('changeSchema() runs code that is not interpreted', $i); - return; + return $i; } } + return null; + } + + /** Code in changeSchema() that never touches the schema can still do things a replay would repeat (a DB statement). */ + private function noteOpaque(int $from, int $to): void { + $i = $this->findOpaque($from, $to); + if ($i !== null) { + $this->reason('changeSchema() runs code that is not interpreted', $i); + } + } + + /** A return we cannot place: the code after it only runs conditionally and earlier guard proofs no longer hold. */ + private function unknownEarlyReturn(): void { + $this->conditional = true; + $this->guarded = []; + } + + private function rangeReturnsNull(int $from, int $to): bool { + for ($k = $from; $k + 2 <= $to && $k + 2 < count($this->T); $k++) { + if ($this->T[$k]['t'] === T_RETURN && strtolower($this->T[$k + 1]['s'] ?? '') === 'null' && ($this->T[$k + 2]['s'] ?? '') === ';') { + return true; + } + } + return false; } private function mentions(int $from, int $to): bool { @@ -679,23 +703,20 @@ private function walk(int $i, int $end, bool $top): void { } } - /** "return null;" and "return $schema;" are fine; any other expression that gets the schema is uninterpreted. */ + /** "return null;" and "return $schema;" are fine; any other return value is not understood and may discard the changes. */ private function inspectReturn(int $from, int $to): void { $single = $to - $from === 2; if ($single && (strtolower($this->T[$from + 1]['s']) === 'null' || ($this->T[$from + 1]['t'] === T_VARIABLE && ($this->vars[$this->T[$from + 1]['s']]['k'] ?? '') === 'schema'))) { return; } - if (!$this->mentions($from + 1, $to)) { - $this->noteOpaque($from + 1, $to); - return; - } + $this->noteOpaque($from + 1, $to); if ($this->mentions($from + 1, $to)) { - // e.g. "return $changed ? $schema : null": earlier effects may be thrown away - $this->discardEffects(); $this->touch($from + 1, $to); - $this->reason('return expression that gets the schema is not understood', $from); } + // e.g. "return $changed ? $schema : null" or "return $result": earlier effects may be thrown away + $this->discardEffects(); + $this->reason('return value that is not the schema or null is not understood', $from); } private function statementEnd(int $i, int $end): int { @@ -734,7 +755,10 @@ private function skipConstruct(int $i, int $end): int { } for ($k = $start; $k < $j; $k++) { if ($this->T[$k]['t'] === T_RETURN) { - $this->conditional = true; + $this->unknownEarlyReturn(); + if ($this->rangeReturnsNull($start, $j)) { + $this->discardEffects(); + } break; } } @@ -789,12 +813,9 @@ private function handleIf(int $i): int { // "return null" under a condition discards everything recorded before it when it triggers foreach ($branches as [, , $bf, $bt]) { - for ($k = $bf; $k + 2 < $bt + 1; $k++) { - if ($this->T[$k]['t'] === T_RETURN && strtolower($this->T[$k + 1]['s'] ?? '') === 'null' - && ($this->T[$k + 2]['s'] ?? '') === ';') { - $this->discardEffects(); - break 2; - } + if ($this->rangeReturnsNull($bf, $bt)) { + $this->discardEffects(); + break; } } @@ -826,8 +847,8 @@ private function handleIf(int $i): int { foreach ($branches as [$cf, $ct, $bf, $bt]) { for ($k = $bf; $k < $bt; $k++) { if ($this->T[$k]['t'] === T_RETURN) { - // the rest of changeSchema() may be skipped at runtime - $this->conditional = true; + // the rest of changeSchema() may be skipped at runtime, whatever a guard proved before + $this->unknownEarlyReturn(); break 2; } } @@ -1069,6 +1090,12 @@ private function statement(int $from, int $to): void { $method = $this->T[$i + 1]['s']; $close = $this->close($i + 2); $args = $this->splitArgs($i + 2, $close); + $opaque = $this->findOpaque($i + 2, $close); + if ($opaque !== null) { + // e.g. addColumn('c', ..., ['default' => $this->compute($table)]) + $this->touch($i + 2, $close); + $this->reason('changeSchema() runs code that is not interpreted', $opaque); + } $recv = $this->call($recv, $method, $args, $i); $i = $close + 1; } @@ -1208,7 +1235,8 @@ private function record(array $effect, int $at, array $required): void { $key = effectKey($effect); $guards = $this->guardStack; if ($this->conditional) { - $guards[] = $this->guarded[$key]; + // every early-return guard seen so far has to let the code through + $guards = array_merge($guards, array_values($this->guarded)); } // An operation can be repeated without failing only if its own object is checked, or the whole // block is skipped when the table exists ("if (!hasTable('t')) { create t and fill it }"). @@ -1410,6 +1438,22 @@ function replayRisk(string $v, array $x, bool $wasApplied, array $applied, array } } $earlierDrops = array_slice($x['effects'], 0, $pos); + // the table and the columns an operation needs must exist, or be created by an earlier step of this migration + if (in_array($e['op'], ['addColumn', 'addIndex', 'setPrimaryKey'], true)) { + $createdTables = array_column(array_filter($earlierDrops, static fn (array $d): bool => $d['op'] === 'createTable'), 'table'); + if (!isset($schema['columns'][$e['table']]) && !in_array($e['table'], $createdTables, true)) { + return 'a run would fail: table ' . $e['table'] . ' does not exist for ' . describeEffect($e); + } + $addedColumns = array_map( + static fn (array $d): string => $d['table'] . '.' . $d['column'], + array_filter($earlierDrops, static fn (array $d): bool => $d['op'] === 'addColumn') + ); + foreach ($e['columns'] ?? [] as $column) { + if (!isset($schema['columns'][$e['table']][$column]) && !in_array($e['table'] . '.' . $column, $addedColumns, true)) { + return 'a run would fail: column ' . $e['table'] . '.' . $column . ' does not exist for ' . describeEffect($e); + } + } + } if ($e['op'] === 'addIndex' && isset($e['index'])) { foreach ($schema['indexes'][$e['table']] ?? [] as $existing => $idx) { $dropped = array_filter($earlierDrops, static fn (array $d): bool => $d['op'] === 'dropIndex' && $d['table'] === $e['table'] && $d['index'] === $e['index']); From a481cae8c13c8d6c83d537d3432c150c6882dbfb Mon Sep 17 00:00:00 2001 From: "Misha M.-Kupriyanov" Date: Tue, 13 Oct 2026 12:49:34 +0200 Subject: [PATCH 18/18] fix(admin-tools): print ./occ in check-app-migrations suggested fix --- admin-tools/check-app-migrations.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/admin-tools/check-app-migrations.php b/admin-tools/check-app-migrations.php index 08fdddb..b296d5a 100755 --- a/admin-tools/check-app-migrations.php +++ b/admin-tools/check-app-migrations.php @@ -1504,19 +1504,19 @@ function printFix(array $fixes, string $recheck, bool $fromFiles): void { echo "# A replay runs the whole migration again, not only the part that is missing.\n"; echo "# migrations:execute is silent on success; the OK line and exit code 0 mean it worked, a failure prints an error.\n"; if ($run !== []) { - echo "# Before: take a DB backup/snapshot and enable maintenance mode: occ maintenance:mode --on\n"; + echo "# Before: take a DB backup/snapshot and enable maintenance mode: ./occ maintenance:mode --on\n"; echo "# migrations:execute is only available with debug on; NC_debug=true enables it for this one command only\n"; echo "# In this order, one chain: nothing runs after a failed migration:\n"; $lines = []; foreach ($run as $app => $f) { foreach ($f['run'] as $v) { // migrations:execute prints nothing on success, so make the exit code visible - $lines[] = 'NC_debug=true occ migrations:execute ' . shellWord((string)$app) . ' ' . shellWord($v) + $lines[] = 'NC_debug=true ./occ migrations:execute ' . shellWord((string)$app) . ' ' . shellWord($v) . ' && echo ' . escapeshellarg("OK: $app $v"); } } echo implode(" \\\n\t&& ", $lines) . "\n"; - echo "# After: occ maintenance:mode --off; then check the result (expect no findings left):\n"; + echo "# After: ./occ maintenance:mode --off; then check the result (expect no findings left):\n"; if ($fromFiles) { echo "# (this run read dump files: take new ones with --dump-applied and --dump-schema first)\n"; }