Skip to content

Commit c354f8e

Browse files
committed
fix(SchemaChecker): also replay migrations for disabled apps
- otherwise tables from disabled apps rendered as 'unexpected' - should only be true for actually removed apps - do not exit with 1 if there are only findings in disabled apps Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
1 parent 8752268 commit c354f8e

3 files changed

Lines changed: 114 additions & 7 deletions

File tree

core/Command/Db/CheckSchema.php

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,19 +36,39 @@ protected function configure(): void {
3636
protected function execute(InputInterface $input, OutputInterface $output): int {
3737
$onlyTable = $input->getArgument('table');
3838
$findings = $this->schemaChecker->getFindings($onlyTable);
39+
['blocking' => $blocking, 'byDisabledApp' => $byDisabledApp] = $this->schemaChecker->partitionFindings($findings);
3940

4041
if ($input->getOption('output') === self::OUTPUT_FORMAT_PLAIN) {
4142
if ($findings === []) {
4243
$output->writeln('<info>The live database schema matches the expected schema.</info>');
4344
} else {
44-
foreach ($findings as $finding) {
45+
foreach ($blocking as $finding) {
4546
$output->writeln('<comment>' . $this->schemaChecker->formatFinding($finding) . '</comment>');
4647
}
48+
$this->printDisabledAppFindings($byDisabledApp, $output);
4749
}
4850
} else {
4951
$this->writeArrayInOutputFormat($input, $output, $findings);
5052
}
5153

52-
return $findings === [] ? 0 : 1;
54+
return $blocking === [] ? 0 : 1;
55+
}
56+
57+
/**
58+
* @param array<string, list<array{table: string, type: string, name?: string, changes?: list<string>, app: ?string, enabled: bool}>> $byDisabledApp
59+
*/
60+
private function printDisabledAppFindings(array $byDisabledApp, OutputInterface $output): void {
61+
if ($byDisabledApp === []) {
62+
return;
63+
}
64+
65+
$output->writeln('Disabled apps (not affecting exit code):');
66+
$output->writeln('If the schema for a disabled app differs from what is expected, this might indicate the app was updated since it was disabled. Missing migrations will be applied once the app is enabled again.');
67+
foreach ($byDisabledApp as $app => $appFindings) {
68+
$output->writeln(" {$app}:");
69+
foreach ($appFindings as $finding) {
70+
$output->writeln(' - <comment>' . $this->schemaChecker->formatFinding($finding) . '</comment>');
71+
}
72+
}
5373
}
5474
}

core/Command/Upgrade.php

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,10 +252,23 @@ private function checkSchema(OutputInterface $output): void {
252252
return;
253253
}
254254

255+
['blocking' => $blocking, 'byDisabledApp' => $byDisabledApp] = $this->schemaChecker->partitionFindings($findings);
256+
255257
$output->writeln('<comment>The database schema does not match what is expected for the installed version:</comment>');
256-
foreach ($findings as $finding) {
258+
foreach ($blocking as $finding) {
257259
$output->writeln(' - ' . $this->schemaChecker->formatFinding($finding));
258260
}
261+
262+
if ($byDisabledApp !== []) {
263+
$output->writeln('<comment>Disabled apps:</comment>');
264+
$output->writeln('If the schema for a disabled app differs from what is expected, this might indicate the app was updated since it was disabled. Missing migrations will be applied once the app is enabled again.');
265+
foreach ($byDisabledApp as $app => $appFindings) {
266+
$output->writeln(" {$app}:");
267+
foreach ($appFindings as $finding) {
268+
$output->writeln(' - ' . $this->schemaChecker->formatFinding($finding));
269+
}
270+
}
271+
}
259272
$output->writeln('<comment>Run "occ db:schema:check" for details.</comment>');
260273
}
261274
}

lib/private/DB/SchemaChecker.php

Lines changed: 78 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,9 @@
1414
use Doctrine\DBAL\Schema\TableDiff;
1515
use Doctrine\DBAL\Types\Types;
1616
use OC\Migration\NullOutput;
17+
use OCP\App\AppPathNotFoundException;
1718
use OCP\App\IAppManager;
19+
use OCP\IAppConfig;
1820

1921
/**
2022
* Compares the live database schema against the schema expected for the
@@ -24,19 +26,33 @@
2426
class SchemaChecker {
2527
public function __construct(
2628
private readonly Connection $connection,
29+
private readonly IAppConfig $appConfig,
2730
private readonly IAppManager $appManager,
2831
) {
2932
}
3033

3134
/**
32-
* @return list<array{table: string, type: string, name?: string, changes?: list<string>}>
35+
* @return list<array{table: string, type: string, name?: string, changes?: list<string>, app: ?string, enabled: bool}>
3336
*/
3437
public function getFindings(?string $onlyTable = null): array {
3538
$expectedSchema = new Schema();
39+
$enabledApps = array_flip($this->appManager->getEnabledApps());
40+
3641
$this->applyMigrations('core', $expectedSchema);
37-
foreach ($this->appManager->getEnabledApps() as $app) {
42+
43+
// Enabled apps are already autoloaded at boot, no extra class loading needed.
44+
foreach (array_keys($enabledApps) as $app) {
3845
$this->applyMigrations($app, $expectedSchema);
3946
}
47+
48+
// Disabled apps keep their tables, so replay their migrations too.
49+
$disabledApps = array_diff(array_keys($this->appConfig->getAppInstalledVersions()), array_keys($enabledApps));
50+
// Table name => owning disabled app, so its findings can be marked non-blocking below.
51+
$disabledAppTableOwners = [];
52+
foreach ($disabledApps as $app) {
53+
$this->applyDisabledMigrations($app, $expectedSchema, $disabledAppTableOwners);
54+
}
55+
4056
$this->addMigrationsTable($expectedSchema);
4157
$this->materializeUniqueConstraints($expectedSchema);
4258

@@ -50,11 +66,17 @@ public function getFindings(?string $onlyTable = null): array {
5066
$comparator = $this->connection->createSchemaManager()->createComparator();
5167
$diff = $comparator->compareSchemas($liveSchema, $expectedSchema);
5268

53-
return $this->buildFindings($diff);
69+
return array_map(function (array $finding) use ($disabledAppTableOwners, $enabledApps): array {
70+
$app = $disabledAppTableOwners[$finding['table']] ?? null;
71+
$finding['app'] = $app;
72+
// Only tables owned by a disabled app are non-blocking.
73+
$finding['enabled'] = $app === null || $app === 'core' || isset($enabledApps[$app]);
74+
return $finding;
75+
}, $this->buildFindings($diff));
5476
}
5577

5678
/**
57-
* @param array{table: string, type: string, name?: string, changes?: list<string>} $finding
79+
* @param array{table: string, type: string, name?: string, changes?: list<string>, app?: ?string, enabled?: bool} $finding
5880
*/
5981
public function formatFinding(array $finding): string {
6082
return match ($finding['type']) {
@@ -69,6 +91,26 @@ public function formatFinding(array $finding): string {
6991
};
7092
}
7193

94+
/**
95+
* Splits findings into blocking ones (from core or an enabled app) and
96+
* non-blocking ones, grouped by the disabled app that owns them.
97+
*
98+
* @param list<array{table: string, type: string, name?: string, changes?: list<string>, app: ?string, enabled: bool}> $findings
99+
* @return array{blocking: list<array{table: string, type: string, name?: string, changes?: list<string>, app: ?string, enabled: bool}>, byDisabledApp: array<string, list<array{table: string, type: string, name?: string, changes?: list<string>, app: ?string, enabled: bool}>>}
100+
*/
101+
public function partitionFindings(array $findings): array {
102+
$blocking = [];
103+
$byDisabledApp = [];
104+
foreach ($findings as $finding) {
105+
if ($finding['enabled']) {
106+
$blocking[] = $finding;
107+
} else {
108+
$byDisabledApp[$finding['app']][] = $finding;
109+
}
110+
}
111+
return ['blocking' => $blocking, 'byDisabledApp' => $byDisabledApp];
112+
}
113+
72114
private function applyMigrations(string $app, Schema $schema): void {
73115
$output = new NullOutput();
74116
$ms = new MigrationService($app, $this->connection, $output);
@@ -80,6 +122,38 @@ private function applyMigrations(string $app, Schema $schema): void {
80122
}
81123
}
82124

125+
/**
126+
* @param array<string, string> $disabledAppTableOwners table name => owning app id, updated in place
127+
*/
128+
private function applyDisabledMigrations(string $app, Schema $schema, array &$disabledAppTableOwners): void {
129+
try {
130+
$appPath = $this->appManager->getAppPath($app);
131+
} catch (AppPathNotFoundException) {
132+
// Installed, but code is gone: no migrations to replay.
133+
return;
134+
}
135+
136+
// Disabled apps are not autoloaded on boot. Load only the migration
137+
// classes themselves directly from disk, rather than registering
138+
// the whole app for PSR-4 autoloading.
139+
foreach (glob($appPath . '/lib/Migration/Version*.php') ?: [] as $file) {
140+
require_once $file;
141+
}
142+
143+
$existingTables = [];
144+
foreach ($schema->getTables() as $table) {
145+
$existingTables[$table->getName()] = true;
146+
}
147+
148+
$this->applyMigrations($app, $schema);
149+
150+
foreach ($schema->getTables() as $table) {
151+
if (!isset($existingTables[$table->getName()])) {
152+
$disabledAppTableOwners[$table->getName()] = $app;
153+
}
154+
}
155+
}
156+
83157
/**
84158
* The migrations bookkeeping table is created directly by MigrationService
85159
* outside of any app's changeSchema(), so replaying migrations never

0 commit comments

Comments
 (0)