Merge pull request #63701 from nextcloud/fix/63351/schema-checker-disabled

fix(SchemaChecker): replay migrations for disabled apps in 'expected' schema

Author: Antreesy

core/Command/Db/CheckSchema.php

@@ -36,19 +36,41 @@ protected function configure(): void {
 	protected function execute(InputInterface $input, OutputInterface $output): int {
 		$onlyTable = $input->getArgument('table');
 		$findings = $this->schemaChecker->getFindings($onlyTable);
+		['blocking' => $blocking, 'byDisabledApp' => $byDisabledApp] = $this->schemaChecker->partitionFindings($findings);
 
 		if ($input->getOption('output') === self::OUTPUT_FORMAT_PLAIN) {
 			if ($findings === []) {
 				$output->writeln('<info>The live database schema matches the expected schema.</info>');
 			} else {
-				foreach ($findings as $finding) {
+				foreach ($blocking as $finding) {
 					$output->writeln('<comment>' . $this->schemaChecker->formatFinding($finding) . '</comment>');
 				}
+				if ($output->isVerbose()) {
+					$this->printDisabledAppFindings($byDisabledApp, $output);
+				}
 			}
 		} else {
 			$this->writeArrayInOutputFormat($input, $output, $findings);
 		}
 
-		return $findings === [] ? 0 : 1;
+		return $blocking === [] ? 0 : 1;
+	}
+
+	/**
+	 * @param array<string, list<array{table: string, type: string, name?: string, changes?: list<string>, app: ?string, enabled: bool}>> $byDisabledApp
+	 */
+	private function printDisabledAppFindings(array $byDisabledApp, OutputInterface $output): void {
+		if ($byDisabledApp === []) {
+			return;
+		}
+
+		$output->writeln('Disabled apps (not affecting exit code):');
+		$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.');
+		foreach ($byDisabledApp as $app => $appFindings) {
+			$output->writeln("  {$app}:");
+			foreach ($appFindings as $finding) {
+				$output->writeln('    - <comment>' . $this->schemaChecker->formatFinding($finding) . '</comment>');
+			}
+		}
 	}
 }

core/Command/Upgrade.php

@@ -252,10 +252,15 @@ private function checkSchema(OutputInterface $output): void {
 			return;
 		}
 
+		['blocking' => $blocking] = $this->schemaChecker->partitionFindings($findings);
+		if ($blocking === []) {
+			return;
+		}
+
 		$output->writeln('<comment>The database schema does not match what is expected for the installed version:</comment>');
-		foreach ($findings as $finding) {
+		foreach ($blocking as $finding) {
 			$output->writeln('  - ' . $this->schemaChecker->formatFinding($finding));
 		}
-		$output->writeln('<comment>Run "occ db:schema:check" for details.</comment>');
+		$output->writeln('<comment>Run "occ db:schema:check -v" for details.</comment>');
 	}
 }

lib/private/DB/SchemaChecker.php

@@ -14,7 +14,9 @@
 use Doctrine\DBAL\Schema\TableDiff;
 use Doctrine\DBAL\Types\Types;
 use OC\Migration\NullOutput;
+use OCP\App\AppPathNotFoundException;
 use OCP\App\IAppManager;
+use OCP\IAppConfig;
 
 /**
  * Compares the live database schema against the schema expected for the
@@ -24,19 +26,33 @@
 class SchemaChecker {
 	public function __construct(
 		private readonly Connection $connection,
+		private readonly IAppConfig $appConfig,
 		private readonly IAppManager $appManager,
 	) {
 	}
 
 	/**
-	 * @return list<array{table: string, type: string, name?: string, changes?: list<string>}>
+	 * @return list<array{table: string, type: string, name?: string, changes?: list<string>, app: ?string, enabled: bool}>
 	 */
 	public function getFindings(?string $onlyTable = null): array {
 		$expectedSchema = new Schema();
+		$enabledApps = array_flip($this->appManager->getEnabledApps());
+
 		$this->applyMigrations('core', $expectedSchema);
-		foreach ($this->appManager->getEnabledApps() as $app) {
+
+		// Enabled apps are already autoloaded at boot, no extra class loading needed.
+		foreach (array_keys($enabledApps) as $app) {
 			$this->applyMigrations($app, $expectedSchema);
 		}
+
+		// Disabled apps keep their tables, so replay their migrations too.
+		$disabledApps = array_diff(array_keys($this->appConfig->getAppInstalledVersions()), array_keys($enabledApps));
+		// Table name => owning disabled app, so its findings can be marked non-blocking below.
+		$disabledAppTableOwners = [];
+		foreach ($disabledApps as $app) {
+			$this->applyDisabledMigrations($app, $expectedSchema, $disabledAppTableOwners);
+		}
+
 		$this->addMigrationsTable($expectedSchema);
 		$this->materializeUniqueConstraints($expectedSchema);
 
@@ -50,11 +66,17 @@ public function getFindings(?string $onlyTable = null): array {
 		$comparator = $this->connection->createSchemaManager()->createComparator();
 		$diff = $comparator->compareSchemas($liveSchema, $expectedSchema);
 
-		return $this->buildFindings($diff);
+		return array_map(function (array $finding) use ($disabledAppTableOwners, $enabledApps): array {
+			$app = $disabledAppTableOwners[$finding['table']] ?? null;
+			$finding['app'] = $app;
+			// Only tables owned by a disabled app are non-blocking.
+			$finding['enabled'] = $app === null || $app === 'core' || isset($enabledApps[$app]);
+			return $finding;
+		}, $this->buildFindings($diff));
 	}
 
 	/**
-	 * @param array{table: string, type: string, name?: string, changes?: list<string>} $finding
+	 * @param array{table: string, type: string, name?: string, changes?: list<string>, app?: ?string, enabled?: bool} $finding
 	 */
 	public function formatFinding(array $finding): string {
 		return match ($finding['type']) {
@@ -69,6 +91,26 @@ public function formatFinding(array $finding): string {
 		};
 	}
 
+	/**
+	 * Splits findings into blocking ones (from core or an enabled app) and
+	 * non-blocking ones, grouped by the disabled app that owns them.
+	 *
+	 * @param list<array{table: string, type: string, name?: string, changes?: list<string>, app: ?string, enabled: bool}> $findings
+	 * @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}>>}
+	 */
+	public function partitionFindings(array $findings): array {
+		$blocking = [];
+		$byDisabledApp = [];
+		foreach ($findings as $finding) {
+			if ($finding['enabled']) {
+				$blocking[] = $finding;
+			} else {
+				$byDisabledApp[$finding['app']][] = $finding;
+			}
+		}
+		return ['blocking' => $blocking, 'byDisabledApp' => $byDisabledApp];
+	}
+
 	private function applyMigrations(string $app, Schema $schema): void {
 		$output = new NullOutput();
 		$ms = new MigrationService($app, $this->connection, $output);
@@ -80,6 +122,64 @@ private function applyMigrations(string $app, Schema $schema): void {
 		}
 	}
 
+	/**
+	 * @param array<string, string> $disabledAppTableOwners table name => owning app id, updated in place
+	 */
+	private function applyDisabledMigrations(string $app, Schema $schema, array &$disabledAppTableOwners): void {
+		try {
+			$appPath = $this->appManager->getAppPath($app);
+		} catch (AppPathNotFoundException) {
+			// Installed, but code is gone: no migrations to replay.
+			return;
+		}
+
+		$existingTables = [];
+		foreach ($schema->getTables() as $table) {
+			$existingTables[$table->getName()] = true;
+		}
+
+		try {
+			// Disabled apps are not autoloaded on boot. Load only the migration
+			// classes themselves directly from disk, rather than registering
+			// the whole app for PSR-4 autoloading.
+			foreach ($this->findMigrationFiles($appPath . '/lib/Migration') as $file) {
+				require_once $file;
+			}
+
+			$this->applyMigrations($app, $schema);
+		} catch (\Throwable) {
+			return;
+		}
+
+		foreach ($schema->getTables() as $table) {
+			if (!isset($existingTables[$table->getName()])) {
+				$disabledAppTableOwners[$table->getName()] = $app;
+			}
+		}
+	}
+
+	/**
+	 * Copied from MigrationService::findMigrations(), minus the class-name mapping.
+	 *
+	 * @return list<string>
+	 */
+	private function findMigrationFiles(string $directory): array {
+		$directory = realpath($directory);
+		if ($directory === false || !file_exists($directory) || !is_dir($directory)) {
+			return [];
+		}
+
+		$iterator = new \RegexIterator(
+			new \RecursiveIteratorIterator(
+				new \RecursiveDirectoryIterator($directory, \FilesystemIterator::SKIP_DOTS),
+				\RecursiveIteratorIterator::LEAVES_ONLY
+			),
+			'#^.+\\/Version[^\\/]{1,255}\\.php$#i',
+			\RegexIterator::GET_MATCH);
+
+		return array_keys(iterator_to_array($iterator));
+	}
+
 	/**
 	 * The migrations bookkeeping table is created directly by MigrationService
 	 * outside of any app's changeSchema(), so replaying migrations never