nextcloud / nextcloud/server

db:schema:check: tables of apps whose code was removed (but still registered) are reported as blocking findings instead of non-blocking

Open
#64,440 2 comments 2 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
PHP
Stars
36.9k
Forks
5.2k
Avg merge
2d 3h
Merged PRs (30d)
713

Description

Bug description

occ db:schema:check is designed to only treat findings from core and
enabled apps as blocking, while findings belonging to a disabled app are
collected separately and excluded from the exit code
(SchemaChecker::partitionFindings()).

However, when an app is registered as installed (has an installed_version
app-config entry) but its code directory no longer exists — e.g. it was
previously used and its files were removed without a full occ app:remove,
or its code became unavailable during a major-version upgrade while its
appconfig/tables were left in place — SchemaChecker::applyDisabledMigrations()
hits AppPathNotFoundException and simply returns without replaying any
migrations for that app:

try {
    $appPath = $this->appManager->getAppPath($app);
} catch (AppPathNotFoundException) {
    // Installed, but code is gone: no migrations to replay.
    return;
}

Because no tables get added to the in-memory expected schema for that app,
every live table still owned by it is reported as unexpected_table and —
critically — is not attributed to any app ($disabledAppTableOwners stays
empty for it). In getFindings() this makes enabled evaluate to true:

$finding['enabled'] = $app === null || $app === 'core' || isset($enabledApps[$app]);

So orphaned tables from a long-gone app end up in the blocking bucket,
printed as plain findings and affecting the exit code — exactly like a real
core/enabled-app schema problem — instead of the non-blocking "Disabled
apps" section the command is explicitly designed to produce for this case.

The same silent-failure path (catch (\Throwable) { return; } around
applyMigrations() inside applyDisabledMigrations()) can also swallow a
migration class that fails to load because it references other classes from
the same app that aren't autoloaded (disabled apps only get their
lib/Migration/*.php files require_onced directly, not the full PSR-4
autoload). This produces the same misclassification for a present but
disabled
app, without any indication of why.

A related, separate false positive (same command)

Independent of the above: occ db:schema:check can report a missing_index
for an index a shipped app has intentionally made redundant via
AddMissingIndicesEvent::replaceIndex(), when no migration ever formally
drops the old index.

Example: the activity app's initial migration
(Version2006Date20170808154933) creates both activity_object
(object_type, object_id) and activity_object_user
(affecteduser, object_type, object_id, timestamp). A later
AddMissingIndicesListener calls:

$event->replaceIndex('activity', ['activity_object'], 'activity_object_user', [...], false);

...to retire activity_object in favor of the superseding
activity_object_user. Since no migration was ever added to drop
activity_object, db:schema:check's migration replay still expects it,
and reports oc_activity: missing index 'activity_object' on any instance
where that index is already gone (e.g. via historical cleanup, or simply
never created because only the superseding migration entry applies) — even
though this is the intended, up-to-date state.

Confirmed on a test instance that the index is absent and its replacement
is present:

SELECT indexname, indexdef FROM pg_indexes
WHERE tablename = 'oc_activity' AND indexname LIKE 'activity_object%';

      indexname       |                                                        indexdef
-----------------------+-------------------------------------------------------------------------------------------------------------------------
 activity_object_user | CREATE INDEX activity_object_user ON public.oc_activity USING btree (affecteduser, object_type, object_id, "timestamp")
(1 row)

db:add-missing-indices correctly does nothing in this state (the
replacement index already exists), consistent with this being a stale
expectation in db:schema:check rather than an actual gap.

Steps to reproduce

  1. Install and enable an app; let it create its tables.
  2. Remove the app's code from the apps directory without running
    occ app:remove (or let it become unavailable through some other means),
    leaving its appconfig installed_version entry and its tables intact.
  3. Run occ db:schema:check.

Alternative reproduction for the index case:

  1. On an instance where the activity app's oc_activity table has
    activity_object_user but not activity_object (e.g. after running
    occ db:add-missing-indices, or on any instance where the old index was
    already cleaned up historically), run occ db:schema:check.

Expected behavior

  • Tables belonging to an app that is "installed" per app-config but has no
    resolvable code path should be treated like any other disabled-app
    finding: collected separately, excluded from the exit code, and ideally
    flagged explicitly as "app code missing" rather than silently merged into
    the same bucket as core/enabled-app problems.
  • An index retired exclusively through
    AddMissingIndicesEvent::replaceIndex() should not be permanently
    reported as missing_index by db:schema:check once the replacement
    index is present.

Actual behavior

Both cases produce plain, unlabeled unexpected_table / missing_index
findings in the blocking output of occ db:schema:check, indistinguishable
from genuine schema drift, with no indication that the underlying cause is
a removed app or an intentionally superseded index.

Environment

  • Nextcloud Server version: 35.0.0
  • Database: PostgreSQL
  • Reproducible independently of OS/web server/PHP version — the issue is in
    SchemaChecker's logic and the activity app's own migration/listener
    setup.

Additional context

Traced via the following source files:

  • lib/private/DB/SchemaChecker.php (getFindings(),
    applyDisabledMigrations(), partitionFindings())
  • core/Command/Db/AddMissingIndices.php
  • nextcloud/activity: lib/Listener/AddMissingIndicesListener.php,
    lib/Migration/Version2006Date20170808154933.php

Happy to provide more detail or test a patch.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in lib/private/DB/SchemaChecker.php, reading getFindings(), applyDisabledMigrations(), and partitionFindings(), then compare the related behavior in core/Command/Db/AddMissingIndices.php. Review nextcloud/activity's AddMissingIndicesListener.php and Version2006Date20170808154933.php for the superseded index case. Done means removed-app tables and intentionally replaced indexes no longer appear as blocking schema findings, with the remaining findings still reported correctly.

Written by the indexing model from the issue text.

Assessment

Tech stack
php, postgresql
Domain
cli, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.