You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(admin-tools): add check-app-migrations to detect schema drift - #183
Adds a read-only admin tool that reports app migrations which were never applied and ones recorded in oc_migrations whose schema changes are missing from the database, e.g. Talk failing with an unknown column after the 31 to 32 to 33 upgrade.
It connects through the Nextcloud config, extracts addColumn, createTable, addIndex, dropColumn and similar from changeSchema() and compares them with the live schema; code it cannot parse is reported as not verifiable. Output has only versions and table, column and index names. Tested on sqlite only, the MySQL path is untested here.
Null returns can discard previously recorded effects
admin-tools/check-app-migrations.php:715
A return null inside an unsupported loop or try block can discard effects recorded before it, but this only marks later statements conditional. For example, addColumn(...), followed by foreach ([true] as $discard) { if ($discard) { return null; } } and a final return $schema;, produces a missing-column finding and a replay suggestion with no unverifiable reason. Replaying still discards the schema. Inspect all enclosing-method return paths in skipped constructs and invalidate earlier effects when a null or unknown return can discard them.
Do not treat conditional index effects as definitely applied
admin-tools/check-app-migrations.php:898
An index inside if (!hasColumn('c')) is not guaranteed to exist after the migration. If c already existed without the index, the block correctly skipped, yet this exception makes the checker report the index as applied but missing. Remove the exception so this additional index effect is reported as not verifiable rather than definite schema drift.
Track schema-mutating helpers in recognized call arguments
admin-tools/check-app-migrations.php:1038
Arguments of recognized calls are skipped without checking for schema-mutating helpers. For example, $table->addColumn('c', 'integer', $this->options($schema)) records only the column, with no unverifiable reason or touched-table marker, although the helper receives the whole schema. Earlier effects can then be misreported and a replay suggested for unparsed code. Mark argument expressions receiving tracked schema, table, or column objects as uncertain before accepting the call's effects.
Invalidate dependent indexes after column changes
admin-tools/check-app-migrations.php:1228
Dropping or renaming a column also invalidates earlier index and primary-key definitions containing it. For example, addIndex(['old'], 'idx') followed by an applied renameColumn('old', 'new') can leave a valid idx(new), but this code still checks for idx(old) and reports false drift. Treat dependent index definitions as superseded by a later column drop; renames already produce a drop effect.
Verify ordering before replaying opaque data migrations
admin-tools/check-app-migrations.php:1360
Running a data migration for the first time is not necessarily safe when newer migrations have already run. This exemption accepts an older never-applied postSchemaChange() even if a later data migration is recorded; for a data-only migration, the subsequent effects loop is empty, so no ordering check occurs. The suggested command can transform data in the wrong order. Require manual review for opaque data transformations unless their ordering is known to be safe, using the complete applied-version list.
Validate prerequisites before suggesting replay commands
admin-tools/check-app-migrations.php:1367
A missing effect is not necessarily executable. If a migration only calls $schema->getTable('t')->addColumn(...) and t is absent, checkEffect() returns table missing, but replayRisk() accepts it and suggests a command that fails at getTable(). Index additions likewise require their columns to exist. Check prerequisites against the ordered repair operations, accounting for earlier table and column creation, and require manual review when prerequisites are unmet.
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Withhold replay for fall-through or false schema returns
admin-tools/check-app-migrations.php:567
When changeSchema() has no explicit return, this returns false and leaves its extracted schema effects eligible for verification and replay. An untyped method falls through to null, so Nextcloud discards its changes; replay cannot repair them. Mark this case as not verifiable and withhold replay. Also account conservatively for fall-through paths rather than assuming that the presence of a return elsewhere proves a schema wrapper is returned.
Treat nonliteral null returns as unverifiable schema effects
admin-tools/check-app-migrations.php:692
$result = null; return $result; reaches this branch without recording uncertainty, and onlyReturnsNull() does not recognize it either. Schema effects remain asserted and replay can be recommended, although Nextcloud will discard the changes again. Treat returns other than literal null or a known schema variable as not verifiable, and discard their asserted effects.
Inspect schema method argument expressions for hidden effects
admin-tools/check-app-migrations.php:1073
Recognized schema methods skip argument expressions. For example, addColumn('c', 'integer', ['default' => $this->calculateDefault($schema)]) records no uncertainty, even though the helper can change other tables or write data. The migration can then be incorrectly verified or recommended for replay. Inspect these arguments for opaque calls and tracked schema references, and record their possible effects before accepting the method's effect.
Invalidate dependent indexes and keys after column renames
admin-tools/check-app-migrations.php:1262
renameColumn is represented as a column drop and add, but neither supersedes earlier index or primary-key expectations using the old column. If one applied migration indexes items.old and a later one renames it to new, the live index uses new, yet this check reports drift against the old definition. Invalidate dependent index and primary-key expectations when their column is removed or renamed.
Validate table and column prerequisites before recommending replay
admin-tools/check-app-migrations.php:1401
A missing effect does not establish that the operation can run. A migration that calls getTable('items')->addColumn(...) is recommended for replay even when items is absent. An index-only migration is also recommended when its indexed columns are missing. Both commands fail instead of repairing the schema. Validate table and column prerequisites against the state produced by preceding operations and guards; send unmet prerequisites to "Review by hand".
Withhold replay when newer applied migration source is unavailable
admin-tools/check-app-migrations.php:1586
Applied versions missing from the app directory are absent from both $extracted and $flat, so replay safety ignores them. If a newer applied migration dropped a table but its source is no longer deployed, an earlier create-table migration can be recommended for replay, recreating deliberately removed schema. Withhold replay whenever a newer applied migration has unavailable source, since its effects cannot be checked.
Reject all unparsed positional arguments
admin-tools/check-app-migrations.php:1756
getopt() sets $rest to the first unparsed argument's index. With one trailing positional argument, $rest === count($argv) - 1, so this condition silently accepts it and continues the check. Compare against count($argv) to reject any unparsed argument as malformed usage.
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Recognize qualified function calls as opaque side effects
admin-tools/check-app-migrations.php:643
This check only recognizes T_STRING and T_VARIABLE, so it misses qualified function calls such as \Vendor\updateData(). PHP uses T_NAME_QUALIFIED, T_NAME_FULLY_QUALIFIED, or T_NAME_RELATIVE for these names. An applied migration can therefore receive a replay suggestion that repeats an unreported side effect. Include qualified-name tokens in the opaque-call check.
Reject unknown return values and discard their effects
admin-tools/check-app-migrations.php:692
A return expression that does not mention a tracked schema variable is accepted without proving that it returns the modified wrapper. For example, adding a column and then doing $result = null; return $result; produces no unverifiable reason and can receive a replay suggestion, although Nextcloud discards the changes again. Mark unknown return values as not verifiable and withdraw the effects they may discard.
Discard effects before skipped loops with null returns
admin-tools/check-app-migrations.php:740
A return null; inside a skipped loop only makes subsequent effects conditional; earlier effects remain asserted. For example, addColumn(...), followed by foreach ([1] as $item) { return null; } and a final return $schema;, retains the column effect without an unverifiable reason. Nextcloud discards that change, so a missing column produces a false drift finding and an ineffective replay suggestion. Discard earlier effects when skipped constructs contain null returns, as handleIf() already does.
Inspect schema call arguments for opaque side effects
admin-tools/check-app-migrations.php:1073
Recognized schema calls skip their argument expressions. For example, addColumn('counter', 'integer', ['default' => $this->runDataChange()]) produces no unverifiable reason, so a missing column can trigger a replay suggestion that repeats an uninterpreted data change. Helpers receiving schema objects here also escape touched-table tracking. Inspect argument ranges for opaque calls and mark tables passed to unparsed code as uncertain before accepting the schema call.
Invalidate guard proofs bypassed by early returns
admin-tools/check-app-migrations.php:1205
An entry in $guarded remains trusted after another early return intervenes. For example, if ($table->hasColumn('x')) { return null; }, then if (true) { return null; }, followed by addColumn('x', ...) and a final return $schema;, retains the column effect without an unverifiable reason. When x is missing, the checker suggests replay even though execution always exits before adding it. Invalidate earlier guard proofs when another early return can bypass the effect, or track every condition needed to reach it.
Require parent tables and index columns before replay
admin-tools/check-app-migrations.php:1407
A missing parent table is accepted as a repairable column or index effect without checking prerequisites. A migration containing only getTable('room')->addColumn('x', ...) receives a replay command when room is absent, but execution fails at getTable(). Require prerequisite tables and index columns to exist or be supplied by earlier operations in the proposed execution sequence. Otherwise, put the migration under "Review by hand".
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
…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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
… 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 <kupriyanov@strato.de>
…p-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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
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 <kupriyanov@strato.de>
… 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 <kupriyanov@strato.de>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a read-only admin tool that reports app migrations which were never applied and ones recorded in oc_migrations whose schema changes are missing from the database, e.g. Talk failing with an unknown column after the 31 to 32 to 33 upgrade.
It connects through the Nextcloud config, extracts addColumn, createTable, addIndex, dropColumn and similar from changeSchema() and compares them with the live schema; code it cannot parse is reported as not verifiable. Output has only versions and table, column and index names. Tested on sqlite only, the MySQL path is untested here.