diff --git a/core/Command/Db/CheckSchema.php b/core/Command/Db/CheckSchema.php index 696aa971ccf74..77cd26e3a9b8f 100644 --- a/core/Command/Db/CheckSchema.php +++ b/core/Command/Db/CheckSchema.php @@ -39,15 +39,15 @@ protected function execute(InputInterface $input, OutputInterface $output): int ['blocking' => $blocking, 'byDisabledApp' => $byDisabledApp] = $this->schemaChecker->partitionFindings($findings); if ($input->getOption('output') === self::OUTPUT_FORMAT_PLAIN) { - if ($findings === []) { + if ($blocking === []) { $output->writeln('The live database schema matches the expected schema.'); } else { foreach ($blocking as $finding) { $output->writeln('' . $this->schemaChecker->formatFinding($finding) . ''); } - if ($output->isVerbose()) { - $this->printDisabledAppFindings($byDisabledApp, $output); - } + } + if ($output->isVerbose()) { + $this->printDisabledAppFindings($byDisabledApp, $output); } } else { $this->writeArrayInOutputFormat($input, $output, $findings); diff --git a/lib/private/DB/SchemaChecker.php b/lib/private/DB/SchemaChecker.php index 15f6e0271caf4..227077ed5f2ff 100644 --- a/lib/private/DB/SchemaChecker.php +++ b/lib/private/DB/SchemaChecker.php @@ -12,11 +12,17 @@ use Doctrine\DBAL\Schema\Schema; use Doctrine\DBAL\Schema\SchemaDiff; use Doctrine\DBAL\Schema\TableDiff; +use Doctrine\DBAL\Types\StringType; +use Doctrine\DBAL\Types\Type; use Doctrine\DBAL\Types\Types; use OC\Migration\NullOutput; use OCP\App\AppPathNotFoundException; use OCP\App\IAppManager; +use OCP\DB\Events\AddMissingIndicesEvent; +use OCP\EventDispatcher\IEventDispatcher; use OCP\IAppConfig; +use OCP\IDBConnection; +use Psr\Log\LoggerInterface; /** * Compares the live database schema against the schema expected for the @@ -28,6 +34,8 @@ public function __construct( private readonly Connection $connection, private readonly IAppConfig $appConfig, private readonly IAppManager $appManager, + private readonly IEventDispatcher $eventDispatcher, + private readonly LoggerInterface $logger, ) { } @@ -60,6 +68,7 @@ public function getFindings(?string $onlyTable = null): array { $this->addMigrationsTable($expectedSchema); $this->materializeUniqueConstraints($expectedSchema); + $this->normalizeLongStringColumns($expectedSchema); $liveSchema = $this->connection->createSchema(); @@ -70,6 +79,8 @@ public function getFindings(?string $onlyTable = null): array { $comparator = $this->connection->createSchemaManager()->createComparator(); $diff = $comparator->compareSchemas($liveSchema, $expectedSchema); + $optionalIndexNames = $this->getOptionalIndexNames(); + $findings = array_filter($this->buildFindings($diff), fn (array $finding): bool => !$this->isOptionalIndexFinding($finding, $optionalIndexNames)); return array_map(function (array $finding) use ($disabledAppTableOwners, $enabledApps): array { $app = $disabledAppTableOwners[$finding['table']] ?? null; @@ -84,7 +95,7 @@ public function getFindings(?string $onlyTable = null): array { $finding['enabled'] = $app === null || $app === 'core' || isset($enabledApps[$app]); } return $finding; - }, $this->buildFindings($diff)); + }, array_values($findings)); } /** @@ -170,13 +181,18 @@ private function applyDisabledMigrations(string $app, Schema $schema, array &$di } $this->applyMigrations($app, $schema); - } catch (\Throwable) { - return; - } - - foreach ($schema->getTables() as $table) { - if (!isset($existingTables[$table->getName()])) { - $disabledAppTableOwners[$table->getName()] = $app; + } catch (\Throwable $e) { + $this->logger->warning('Could not replay migrations for disabled app {app}', [ + 'app' => $app, + 'exception' => $e, + ]); + } finally { + // Attribute whatever was applied before a failure too, so it + // isn't misreported as blocking drift owned by no app. + foreach ($schema->getTables() as $table) { + if (!isset($existingTables[$table->getName()])) { + $disabledAppTableOwners[$table->getName()] = $app; + } } } } @@ -237,6 +253,65 @@ private function materializeUniqueConstraints(Schema $schema): void { } } + /** + * Migrator::getDiff() (see lib/private/DB/Migrator.php) rewrites any + * STRING column longer than 4000 characters to TEXT before it generates + * DDL, for consistency between the supported databases. That rewrite + * only happens when a migration is actually applied, never when it is + * replayed here to build the expected schema - so without repeating it, + * any such column would forever be reported as a type mismatch. + */ + private function normalizeLongStringColumns(Schema $schema): void { + foreach ($schema->getTables() as $table) { + foreach ($table->getColumns() as $column) { + if ($column->getType() instanceof StringType && $column->getLength() > 4000) { + $column->setType(Type::getType(Types::TEXT)); + $column->setLength(null); + } + } + } + } + + /** + * Apps can register indices that are only ever created or renamed via + * occ db:add-missing-indices (AddMissingIndicesEvent), not through a + * versioned migration. Since running that command is optional, whether + * such an index exists on the live schema depends on whether an admin + * ever ran it - it is not itself a sign of drift in either direction. + * Collect their names here so findings about them can be filtered out + * entirely, rather than reported as missing/unexpected index findings. + * + * @return array> table name => set of index names + */ + private function getOptionalIndexNames(): array { + $event = new AddMissingIndicesEvent(); + $this->eventDispatcher->dispatchTyped($event); + + $names = []; + foreach ($event->getMissingIndices() as $missingIndex) { + $table = $this->connection->getPrefix() . $missingIndex['tableName']; + $names[$table][$missingIndex['indexName']] = true; + } + foreach ($event->getIndicesToReplace() as $toReplace) { + $table = $this->connection->getPrefix() . $toReplace['tableName']; + $names[$table][$toReplace['newIndexName']] = true; + foreach ($toReplace['oldIndexNames'] as $oldIndexName) { + $names[$table][$oldIndexName] = true; + } + } + + return $names; + } + + /** + * @param array{table: string, type: string, name?: string, changes?: list} $finding + * @param array> $optionalIndexNames table name => set of index names, as returned by getOptionalIndexNames() + */ + private function isOptionalIndexFinding(array $finding, array $optionalIndexNames): bool { + return ($finding['type'] === 'missing_index' || $finding['type'] === 'unexpected_index') + && isset($optionalIndexNames[$finding['table']][$finding['name']]); + } + private function keepOnlyTable(Schema $schema, string $tableName): void { foreach ($schema->getTables() as $table) { if ($table->getName() !== $tableName) { @@ -324,7 +399,7 @@ private function getChangedColumnProperties(ColumnDiff $columnDiff): array { if ($columnDiff->hasNotNullChanged()) { $changes[] = 'nullable'; } - if ($columnDiff->hasDefaultChanged()) { + if ($columnDiff->hasDefaultChanged() && !$this->isIgnorableTextDefaultDiff($columnDiff)) { $changes[] = 'default'; } if ($columnDiff->hasAutoIncrementChanged()) { @@ -342,4 +417,20 @@ private function getChangedColumnProperties(ColumnDiff $columnDiff): array { return $changes; } + + /** + * MySQL and MariaDB silently ignore a literal DEFAULT clause on TEXT and + * BLOB columns - only NULL is ever actually stored for them. A migration + * that declares such a default therefore always disagrees with the live + * schema on these platforms, even though nothing has actually drifted. + */ + private function isIgnorableTextDefaultDiff(ColumnDiff $columnDiff): bool { + if (!in_array($this->connection->getDatabaseProvider(), [IDBConnection::PLATFORM_MYSQL, IDBConnection::PLATFORM_MARIADB], true)) { + return false; + } + + $typeName = Type::getTypeRegistry()->lookupName($columnDiff->getNewColumn()->getType()); + + return in_array($typeName, [Types::TEXT, Types::BLOB], true); + } } diff --git a/tests/lib/DB/SchemaCheckerTest.php b/tests/lib/DB/SchemaCheckerTest.php new file mode 100644 index 0000000000000..463f2d078bd09 --- /dev/null +++ b/tests/lib/DB/SchemaCheckerTest.php @@ -0,0 +1,164 @@ +connection = $this->createMock(Connection::class); + $this->appConfig = $this->createMock(IAppConfig::class); + $this->appManager = $this->createMock(IAppManager::class); + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); + $this->logger = $this->createMock(LoggerInterface::class); + + $this->schemaChecker = new SchemaChecker( + $this->connection, + $this->appConfig, + $this->appManager, + $this->eventDispatcher, + $this->logger, + ); + } + + public static function dataFormatFinding(): array { + return [ + 'missing_table' => [['table' => 'oc_foo', 'type' => 'missing_table'], "missing table 'oc_foo'"], + 'unexpected_table' => [['table' => 'oc_foo', 'type' => 'unexpected_table'], "unexpected table 'oc_foo'"], + 'missing_column' => [['table' => 'oc_foo', 'type' => 'missing_column', 'name' => 'bar'], "oc_foo: missing column 'bar'"], + 'unexpected_column' => [['table' => 'oc_foo', 'type' => 'unexpected_column', 'name' => 'bar'], "oc_foo: unexpected column 'bar'"], + 'modified_column' => [['table' => 'oc_foo', 'type' => 'modified_column', 'name' => 'bar', 'changes' => ['type', 'default']], "oc_foo: column 'bar' differs in: type, default"], + 'missing_index' => [['table' => 'oc_foo', 'type' => 'missing_index', 'name' => 'bar_idx'], "oc_foo: missing index 'bar_idx'"], + 'unexpected_index' => [['table' => 'oc_foo', 'type' => 'unexpected_index', 'name' => 'bar_idx'], "oc_foo: unexpected index 'bar_idx'"], + 'unknown' => [['table' => 'oc_foo', 'type' => 'something_else'], "oc_foo: unknown finding 'something_else'"], + ]; + } + + #[DataProvider('dataFormatFinding')] + public function testFormatFinding(array $finding, string $expected): void { + $this->assertSame($expected, $this->schemaChecker->formatFinding($finding)); + } + + public function testPartitionFindingsSplitsBlockingAndDisabled(): void { + $blockingFinding = ['table' => 'oc_foo', 'type' => 'missing_column', 'name' => 'a', 'app' => 'core', 'enabled' => true]; + $disabledAppFinding = ['table' => 'oc_bar', 'type' => 'missing_column', 'name' => 'b', 'app' => 'files', 'enabled' => false]; + $unattributedFinding = ['table' => 'oc_baz', 'type' => 'unexpected_table', 'app' => null, 'enabled' => false]; + + $result = $this->schemaChecker->partitionFindings([ + $blockingFinding, + $disabledAppFinding, + $unattributedFinding, + ]); + + $this->assertSame([$blockingFinding], $result['blocking']); + $this->assertSame(['files' => [$disabledAppFinding]], array_intersect_key($result['byDisabledApp'], ['files' => true])); + $this->assertSame(['(unknown app)' => [$unattributedFinding]], array_intersect_key($result['byDisabledApp'], ['(unknown app)' => true])); + } + + public function testNormalizeLongStringColumnsRewritesOnlyColumnsOverTheLimit(): void { + $schema = new Schema(); + $table = $schema->createTable('oc_test'); + $table->addColumn('short_col', Types::STRING, ['length' => 255]); + $table->addColumn('long_col', Types::STRING, ['length' => 4001]); + + self::invokePrivate($this->schemaChecker, 'normalizeLongStringColumns', [$schema]); + + $this->assertSame(Types::STRING, Type::getTypeRegistry()->lookupName($table->getColumn('short_col')->getType())); + $this->assertSame(255, $table->getColumn('short_col')->getLength()); + + $this->assertInstanceOf(TextType::class, $table->getColumn('long_col')->getType()); + $this->assertNull($table->getColumn('long_col')->getLength()); + } + + public static function dataIsIgnorableTextDefaultDiff(): array { + return [ + 'mysql text' => [IDBConnection::PLATFORM_MYSQL, Types::TEXT, true], + 'mariadb blob' => [IDBConnection::PLATFORM_MARIADB, Types::BLOB, true], + 'mysql string is not ignorable' => [IDBConnection::PLATFORM_MYSQL, Types::STRING, false], + 'sqlite text is not ignorable' => [IDBConnection::PLATFORM_SQLITE, Types::TEXT, false], + ]; + } + + #[DataProvider('dataIsIgnorableTextDefaultDiff')] + public function testIsIgnorableTextDefaultDiff(string $provider, string $typeName, bool $expected): void { + $this->connection->method('getDatabaseProvider')->willReturn($provider); + + $column = new Column('some_col', Type::getType($typeName)); + $columnDiff = new ColumnDiff('some_col', $column, ['default'], $column); + + $result = self::invokePrivate($this->schemaChecker, 'isIgnorableTextDefaultDiff', [$columnDiff]); + + $this->assertSame($expected, $result); + } + + public function testGetOptionalIndexNamesCollectsMissingAndReplacedIndices(): void { + $this->connection->method('getPrefix')->willReturn('oc_'); + $this->eventDispatcher->method('dispatchTyped') + ->willReturnCallback(function (AddMissingIndicesEvent $event): void { + $event->addMissingIndex('foo', 'foo_idx', ['col']); + $event->replaceIndex('bar', ['old_idx'], 'new_idx', ['col'], false); + }); + + $names = self::invokePrivate($this->schemaChecker, 'getOptionalIndexNames'); + + $this->assertSame([ + 'oc_foo' => ['foo_idx' => true], + 'oc_bar' => ['new_idx' => true, 'old_idx' => true], + ], $names); + } + + public static function dataIsOptionalIndexFinding(): array { + $optionalIndexNames = ['oc_foo' => ['foo_idx' => true]]; + + return [ + 'matching missing_index' => [['table' => 'oc_foo', 'type' => 'missing_index', 'name' => 'foo_idx'], $optionalIndexNames, true], + 'matching unexpected_index' => [['table' => 'oc_foo', 'type' => 'unexpected_index', 'name' => 'foo_idx'], $optionalIndexNames, true], + 'different index name' => [['table' => 'oc_foo', 'type' => 'missing_index', 'name' => 'other_idx'], $optionalIndexNames, false], + 'different table' => [['table' => 'oc_bar', 'type' => 'missing_index', 'name' => 'foo_idx'], $optionalIndexNames, false], + 'non-index finding type' => [['table' => 'oc_foo', 'type' => 'missing_column', 'name' => 'foo_idx'], $optionalIndexNames, false], + ]; + } + + /** + * Optional-index findings must be filtered out entirely (never reach + * partitionFindings() or --output=json), not just hidden from plain-text + * output - Settings already has a dedicated admin-overview surface for them. + */ + #[DataProvider('dataIsOptionalIndexFinding')] + public function testIsOptionalIndexFinding(array $finding, array $optionalIndexNames, bool $expected): void { + $result = self::invokePrivate($this->schemaChecker, 'isOptionalIndexFinding', [$finding, $optionalIndexNames]); + + $this->assertSame($expected, $result); + } +}