From 343ceb56f9e17849fa66b57c8e670bd7f19f020b Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Wed, 27 May 2026 18:08:01 +0200 Subject: [PATCH] fix(appconfig,userconfig): restore pre-migration fallback for ownCloud migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AppConfig and UserConfig unconditionally queried NC-only columns (type, lazy, flags, indexed) that don't exist in ownCloud's database schema, breaking ownCloud → Nextcloud upgrades entirely before the schema migration steps could run. Restore the fallback pattern in both classes: on first loadConfig() call, if a DBException with REASON_INVALID_FIELD_NAME is thrown, set $migrationCompleted = false and retry selecting only the columns present in ownCloud's schema. INSERT and UPDATE statements also omit NC-only columns when $migrationCompleted is false. The catch block also guards against infinite recursion: if $migrationCompleted is already false when the exception fires, the exception is re-thrown instead of triggering another recursive call. Fixes: https://github.com/nextcloud/server/issues/57340 Signed-off-by: Anna Larch AI-Assisted-By: Claude Sonnet 4.6 --- lib/private/AppConfig.php | 56 ++++-- lib/private/Config/UserConfig.php | 62 ++++-- tests/lib/AppConfigMigrationFallbackTest.php | 185 ++++++++++++++++++ .../UserConfigMigrationFallbackTest.php | 180 +++++++++++++++++ 4 files changed, 454 insertions(+), 29 deletions(-) create mode 100644 tests/lib/AppConfigMigrationFallbackTest.php create mode 100644 tests/lib/Config/UserConfigMigrationFallbackTest.php diff --git a/lib/private/AppConfig.php b/lib/private/AppConfig.php index e4da80b94d94d..55820575a5440 100644 --- a/lib/private/AppConfig.php +++ b/lib/private/AppConfig.php @@ -67,6 +67,16 @@ class AppConfig implements IAppConfig { private array $valueTypes = []; // type for all config values private bool $fastLoaded = false; private bool $lazyLoaded = false; + /** + * Tracks whether the NC-only columns (`type`, `lazy`) exist in the `appconfig` table. + * Set to false on first load when a DBException::REASON_INVALID_FIELD_NAME is caught, + * which happens during an ownCloud → Nextcloud migration before the schema steps have run. + * + * Every SELECT that reads those columns and every INSERT/UPDATE that writes them must + * guard with `if ($this->migrationCompleted)` so they degrade gracefully. + * If you add a new query that touches NC-only columns, add the same guard. + */ + private bool $migrationCompleted = true; /** @var array, aliases: array, strictness: Strictness}> ['app_id' => ['strictness' => ConfigLexiconStrictness, 'entries' => ['config_key' => ConfigLexiconEntry[]]] */ private array $configLexiconDetails = []; private bool $ignoreLexiconAliases = false; @@ -827,10 +837,12 @@ private function setTypedValue( $insert = $this->connection->getQueryBuilder(); $insert->insert('appconfig') ->setValue('appid', $insert->createNamedParameter($app)) - ->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT)) - ->setValue('type', $insert->createNamedParameter($type, IQueryBuilder::PARAM_INT)) ->setValue('configkey', $insert->createNamedParameter($key)) ->setValue('configvalue', $insert->createNamedParameter($value)); + if ($this->migrationCompleted) { + $insert->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT)) + ->setValue('type', $insert->createNamedParameter($type, IQueryBuilder::PARAM_INT)); + } $insert->executeStatement(); $inserted = true; } catch (DBException $e) { @@ -890,10 +902,12 @@ private function setTypedValue( $update = $this->connection->getQueryBuilder(); $update->update('appconfig') ->set('configvalue', $update->createNamedParameter($value)) - ->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT)) - ->set('type', $update->createNamedParameter($type, IQueryBuilder::PARAM_INT)) ->where($update->expr()->eq('appid', $update->createNamedParameter($app))) ->andWhere($update->expr()->eq('configkey', $update->createNamedParameter($key))); + if ($this->migrationCompleted) { + $update->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT)) + ->set('type', $update->createNamedParameter($type, IQueryBuilder::PARAM_INT)); + } $update->executeStatement(); } @@ -1347,23 +1361,39 @@ private function loadConfig(?string $app = null, bool $lazy = false): void { // Otherwise no cache available and we need to fetch from database $qb = $this->connection->getQueryBuilder(); - $qb->from('appconfig') - ->select('appid', 'configkey', 'configvalue', 'type'); + $qb->from('appconfig'); - if ($lazy === false) { - $qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT))); + if (!$this->migrationCompleted) { + $qb->select('appid', 'configkey', 'configvalue'); } else { - if ($loadLazyOnly) { - $qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT))); + $qb->select('appid', 'configkey', 'configvalue', 'type'); + + if ($lazy === false) { + $qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT))); + } else { + if ($loadLazyOnly) { + $qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT))); + } + $qb->addSelect('lazy'); + } + } + + try { + $result = $qb->executeQuery(); + } catch (DBException $e) { + if ($e->getReason() !== DBException::REASON_INVALID_FIELD_NAME || !$this->migrationCompleted) { + throw $e; } - $qb->addSelect('lazy'); + // columns 'type' and 'lazy' don't exist yet (ownCloud migration) + $this->migrationCompleted = false; + $this->loadConfig($app, $lazy); + return; } - $result = $qb->executeQuery(); $rows = $result->fetchAll(); foreach ($rows as $row) { // most of the time, 'lazy' is not in the select because its value is already known - if ($lazy && ((int)$row['lazy']) === 1) { + if ($this->migrationCompleted && $lazy && ((int)$row['lazy']) === 1) { $this->lazyCache[$row['appid']][$row['configkey']] = $row['configvalue'] ?? ''; } else { $this->fastCache[$row['appid']][$row['configkey']] = $row['configvalue'] ?? ''; diff --git a/lib/private/Config/UserConfig.php b/lib/private/Config/UserConfig.php index 1c6b819571e92..e496769accc08 100644 --- a/lib/private/Config/UserConfig.php +++ b/lib/private/Config/UserConfig.php @@ -70,6 +70,16 @@ class UserConfig implements IUserConfig { private array $configLexiconDetails = []; private bool $ignoreLexiconAliases = false; private array $strictnessApplied = []; + /** + * Tracks whether the NC-only columns (`type`, `lazy`, `flags`, `indexed`) exist in the + * `preferences` table. Set to false on first load when a DBException::REASON_INVALID_FIELD_NAME + * is caught, which happens during an ownCloud → Nextcloud migration before the schema steps run. + * + * Every SELECT that reads those columns and every INSERT/UPDATE that writes them must + * guard with `if ($this->migrationCompleted)` so they degrade gracefully. + * If you add a new query that touches NC-only columns, add the same guard. + */ + private bool $migrationCompleted = true; public function __construct( protected IDBConnection $connection, @@ -1148,12 +1158,14 @@ private function setTypedValue( $insert->insert('preferences') ->setValue('userid', $insert->createNamedParameter($userId)) ->setValue('appid', $insert->createNamedParameter($app)) - ->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT)) - ->setValue('type', $insert->createNamedParameter($type->value, IQueryBuilder::PARAM_INT)) - ->setValue('flags', $insert->createNamedParameter($flags, IQueryBuilder::PARAM_INT)) - ->setValue('indexed', $insert->createNamedParameter($indexed)) ->setValue('configkey', $insert->createNamedParameter($key)) ->setValue('configvalue', $insert->createNamedParameter($value)); + if ($this->migrationCompleted) { + $insert->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT)) + ->setValue('type', $insert->createNamedParameter($type->value, IQueryBuilder::PARAM_INT)) + ->setValue('flags', $insert->createNamedParameter($flags, IQueryBuilder::PARAM_INT)) + ->setValue('indexed', $insert->createNamedParameter($indexed)); + } $insert->executeStatement(); $inserted = true; } catch (DBException $e) { @@ -1205,13 +1217,15 @@ private function setTypedValue( $update = $this->connection->getQueryBuilder(); $update->update('preferences') ->set('configvalue', $update->createNamedParameter($value)) - ->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT)) - ->set('type', $update->createNamedParameter($type->value, IQueryBuilder::PARAM_INT)) - ->set('flags', $update->createNamedParameter($flags, IQueryBuilder::PARAM_INT)) - ->set('indexed', $update->createNamedParameter($indexed)) ->where($update->expr()->eq('userid', $update->createNamedParameter($userId))) ->andWhere($update->expr()->eq('appid', $update->createNamedParameter($app))) ->andWhere($update->expr()->eq('configkey', $update->createNamedParameter($key))); + if ($this->migrationCompleted) { + $update->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT)) + ->set('type', $update->createNamedParameter($type->value, IQueryBuilder::PARAM_INT)) + ->set('flags', $update->createNamedParameter($flags, IQueryBuilder::PARAM_INT)) + ->set('indexed', $update->createNamedParameter($indexed)); + } $update->executeStatement(); } @@ -1762,25 +1776,41 @@ private function loadConfig(string $userId, ?bool $lazy = false): void { $qb = $this->connection->getQueryBuilder(); $qb->from('preferences'); - $qb->select('appid', 'configkey', 'configvalue', 'type', 'flags'); $qb->where($qb->expr()->eq('userid', $qb->createNamedParameter($userId))); - // we only need value from lazy when loadConfig does not specify it - if ($lazy !== null) { - $qb->andWhere($qb->expr()->eq('lazy', $qb->createNamedParameter($lazy ? 1 : 0, IQueryBuilder::PARAM_INT))); + if (!$this->migrationCompleted) { + $qb->select('appid', 'configkey', 'configvalue'); } else { - $qb->addSelect('lazy'); + $qb->select('appid', 'configkey', 'configvalue', 'type', 'flags'); + + // we only need value from lazy when loadConfig does not specify it + if ($lazy !== null) { + $qb->andWhere($qb->expr()->eq('lazy', $qb->createNamedParameter($lazy ? 1 : 0, IQueryBuilder::PARAM_INT))); + } else { + $qb->addSelect('lazy'); + } + } + + try { + $result = $qb->executeQuery(); + } catch (DBException $e) { + if ($e->getReason() !== DBException::REASON_INVALID_FIELD_NAME || !$this->migrationCompleted) { + throw $e; + } + // columns 'type', 'lazy', 'flags', 'indexed' don't exist yet (ownCloud migration) + $this->migrationCompleted = false; + $this->loadConfig($userId, $lazy); + return; } - $result = $qb->executeQuery(); $rows = $result->fetchAll(); foreach ($rows as $row) { - if (($row['lazy'] ?? ($lazy ?? 0) ? 1 : 0) === 1) { + if ($this->migrationCompleted && (($row['lazy'] ?? ($lazy ?? 0) ? 1 : 0) === 1)) { $this->lazyCache[$userId][$row['appid']][$row['configkey']] = $row['configvalue'] ?? ''; } else { $this->fastCache[$userId][$row['appid']][$row['configkey']] = $row['configvalue'] ?? ''; } - $this->valueDetails[$userId][$row['appid']][$row['configkey']] = ['type' => ValueType::from((int)($row['type'] ?? 0)), 'flags' => (int)$row['flags']]; + $this->valueDetails[$userId][$row['appid']][$row['configkey']] = ['type' => ValueType::from((int)($row['type'] ?? 0)), 'flags' => (int)($row['flags'] ?? 0)]; } $result->closeCursor(); $this->setAsLoaded($userId, $lazy); diff --git a/tests/lib/AppConfigMigrationFallbackTest.php b/tests/lib/AppConfigMigrationFallbackTest.php new file mode 100644 index 0000000000000..63e7a96bca27c --- /dev/null +++ b/tests/lib/AppConfigMigrationFallbackTest.php @@ -0,0 +1,185 @@ +connection = $this->createMock(IDBConnection::class); + $this->config = $this->createMock(IConfig::class); + $this->configManager = $this->createMock(ConfigManager::class); + $this->presetManager = $this->createMock(PresetManager::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->crypto = $this->createMock(ICrypto::class); + $this->cacheFactory = $this->createMock(CacheFactory::class); + } + + private function getAppConfig(): AppConfig { + $this->config->method('getSystemValueBool') + ->with('cache_app_config', true) + ->willReturn(true); + $this->cacheFactory->method('isLocalCacheAvailable')->willReturn(false); + + return new AppConfig( + $this->connection, + $this->config, + $this->configManager, + $this->presetManager, + $this->logger, + $this->crypto, + $this->cacheFactory, + ); + } + + private function createInvalidFieldNameException(): DBException { + $driverException = $this->createMock(\Doctrine\DBAL\Driver\Exception::class); + $dbalException = new InvalidFieldNameException($driverException, null); + return DbalException::wrap($dbalException); + } + + private function createMockQueryBuilder(): IQueryBuilder&MockObject { + $expression = $this->createMock(IExpressionBuilder::class); + $qb = $this->createMock(IQueryBuilder::class); + $qb->method('from')->willReturn($qb); + $qb->method('select')->willReturn($qb); + $qb->method('addSelect')->willReturn($qb); + $qb->method('where')->willReturn($qb); + $qb->method('andWhere')->willReturn($qb); + $qb->method('set')->willReturn($qb); + $qb->method('expr')->willReturn($expression); + $qb->method('insert')->willReturn($qb); + $qb->method('setValue')->willReturn($qb); + $qb->method('createNamedParameter')->willReturn('?'); + return $qb; + } + + /** + * Test that loadConfig retries without type/lazy columns on InvalidFieldNameException. + */ + public function testLoadConfigFallsBackOnMissingColumns(): void { + $exception = $this->createInvalidFieldNameException(); + + $successResult = $this->createMock(IResult::class); + $successResult->method('fetchAll')->willReturn([ + ['appid' => 'core', 'configkey' => 'vendor', 'configvalue' => 'owncloud'], + ['appid' => 'core', 'configkey' => 'installedat', 'configvalue' => '1234567890'], + ]); + + $qb = $this->createMockQueryBuilder(); + // First call throws (columns missing), second call succeeds (fallback query) + $qb->method('executeQuery') + ->willReturnOnConsecutiveCalls( + $this->throwException($exception), + $successResult, + ); + + $this->connection->method('getQueryBuilder')->willReturn($qb); + + $appConfig = $this->getAppConfig(); + + // getValueString triggers loadConfig internally + $value = $appConfig->getValueString('core', 'vendor'); + $this->assertSame('owncloud', $value); + } + + /** + * Test that non-INVALID_FIELD_NAME exceptions are re-thrown, not swallowed. + */ + public function testLoadConfigRethrowsOtherExceptions(): void { + $driverException = $this->createMock(\Doctrine\DBAL\Driver\Exception::class); + $dbalException = new \Doctrine\DBAL\Exception\SyntaxErrorException($driverException, null); + $exception = DbalException::wrap($dbalException); + + $qb = $this->createMockQueryBuilder(); + $qb->method('executeQuery')->willThrowException($exception); + + $this->connection->method('getQueryBuilder')->willReturn($qb); + + $appConfig = $this->getAppConfig(); + + $this->expectException(DBException::class); + $appConfig->getValueString('core', 'vendor'); + } + + /** + * Test that insert omits lazy/type columns when migration is not completed. + */ + public function testInsertOmitsNewColumnsInFallbackMode(): void { + $exception = $this->createInvalidFieldNameException(); + + $loadResult = $this->createMock(IResult::class); + $loadResult->method('fetchAll')->willReturn([]); + + $qb = $this->createMockQueryBuilder(); + + $callCount = 0; + $qb->method('executeQuery') + ->willReturnCallback(function () use ($exception, $loadResult, &$callCount) { + $callCount++; + if ($callCount === 1) { + throw $exception; + } + return $loadResult; + }); + + // Verify insert() is called (meaning we reached the insert path) + $qb->expects(self::once())->method('insert')->with('appconfig')->willReturn($qb); + $qb->method('executeStatement')->willReturn(1); + + // Track which columns are set via setValue + $setColumns = []; + $qb->method('setValue') + ->willReturnCallback(function (string $column) use ($qb, &$setColumns) { + $setColumns[] = $column; + return $qb; + }); + + $this->connection->method('getQueryBuilder')->willReturn($qb); + + $appConfig = $this->getAppConfig(); + $appConfig->setValueString('core', 'vendor', 'owncloud'); + + $this->assertContains('appid', $setColumns); + $this->assertContains('configkey', $setColumns); + $this->assertContains('configvalue', $setColumns); + $this->assertNotContains('lazy', $setColumns, 'lazy column should be omitted in fallback mode'); + $this->assertNotContains('type', $setColumns, 'type column should be omitted in fallback mode'); + } +} diff --git a/tests/lib/Config/UserConfigMigrationFallbackTest.php b/tests/lib/Config/UserConfigMigrationFallbackTest.php new file mode 100644 index 0000000000000..4d1ec8850cf8d --- /dev/null +++ b/tests/lib/Config/UserConfigMigrationFallbackTest.php @@ -0,0 +1,180 @@ +connection = $this->createMock(IDBConnection::class); + $this->config = $this->createMock(IConfig::class); + $this->configManager = $this->createMock(ConfigManager::class); + $this->presetManager = $this->createMock(PresetManager::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->crypto = $this->createMock(ICrypto::class); + $this->dispatcher = $this->createMock(IEventDispatcher::class); + } + + private function getUserConfig(): UserConfig { + return new UserConfig( + $this->connection, + $this->config, + $this->configManager, + $this->presetManager, + $this->logger, + $this->crypto, + $this->dispatcher, + ); + } + + private function createInvalidFieldNameException(): DBException { + $driverException = $this->createMock(\Doctrine\DBAL\Driver\Exception::class); + $dbalException = new InvalidFieldNameException($driverException, null); + return DbalException::wrap($dbalException); + } + + private function createMockQueryBuilder(): IQueryBuilder&MockObject { + $expression = $this->createMock(IExpressionBuilder::class); + $qb = $this->createMock(IQueryBuilder::class); + $qb->method('from')->willReturn($qb); + $qb->method('select')->willReturn($qb); + $qb->method('addSelect')->willReturn($qb); + $qb->method('where')->willReturn($qb); + $qb->method('andWhere')->willReturn($qb); + $qb->method('set')->willReturn($qb); + $qb->method('expr')->willReturn($expression); + $qb->method('insert')->willReturn($qb); + $qb->method('setValue')->willReturn($qb); + $qb->method('createNamedParameter')->willReturn('?'); + return $qb; + } + + /** + * Test that loadConfig retries without new columns on InvalidFieldNameException. + */ + public function testLoadConfigFallsBackOnMissingColumns(): void { + $exception = $this->createInvalidFieldNameException(); + + $successResult = $this->createMock(IResult::class); + $successResult->method('fetchAll')->willReturn([ + ['appid' => 'settings', 'configkey' => 'email', 'configvalue' => 'user@example.com'], + ]); + + $qb = $this->createMockQueryBuilder(); + $qb->method('executeQuery') + ->willReturnOnConsecutiveCalls( + $this->throwException($exception), + $successResult, + ); + + $this->connection->method('getQueryBuilder')->willReturn($qb); + + $userConfig = $this->getUserConfig(); + + $value = $userConfig->getValueString('user1', 'settings', 'email'); + $this->assertSame('user@example.com', $value); + } + + /** + * Test that non-INVALID_FIELD_NAME exceptions are re-thrown. + */ + public function testLoadConfigRethrowsOtherExceptions(): void { + $driverException = $this->createMock(\Doctrine\DBAL\Driver\Exception::class); + $dbalException = new \Doctrine\DBAL\Exception\SyntaxErrorException($driverException, null); + $exception = DbalException::wrap($dbalException); + + $qb = $this->createMockQueryBuilder(); + $qb->method('executeQuery')->willThrowException($exception); + + $this->connection->method('getQueryBuilder')->willReturn($qb); + + $userConfig = $this->getUserConfig(); + + $this->expectException(DBException::class); + $userConfig->getValueString('user1', 'settings', 'email'); + } + + /** + * Test that insert omits new columns when migration is not completed. + */ + public function testInsertOmitsNewColumnsInFallbackMode(): void { + $exception = $this->createInvalidFieldNameException(); + + $loadResult = $this->createMock(IResult::class); + $loadResult->method('fetchAll')->willReturn([]); + + $qb = $this->createMockQueryBuilder(); + + $callCount = 0; + $qb->method('executeQuery') + ->willReturnCallback(function () use ($exception, $loadResult, &$callCount) { + $callCount++; + if ($callCount === 1) { + throw $exception; + } + return $loadResult; + }); + + $qb->expects(self::once())->method('insert')->with('preferences')->willReturn($qb); + $qb->method('executeStatement')->willReturn(1); + + $setColumns = []; + $qb->method('setValue') + ->willReturnCallback(function (string $column) use ($qb, &$setColumns) { + $setColumns[] = $column; + return $qb; + }); + + $this->connection->method('getQueryBuilder')->willReturn($qb); + + $userConfig = $this->getUserConfig(); + $userConfig->setValueString('user1', 'settings', 'email', 'user@example.com'); + + $this->assertContains('userid', $setColumns); + $this->assertContains('appid', $setColumns); + $this->assertContains('configkey', $setColumns); + $this->assertContains('configvalue', $setColumns); + $this->assertNotContains('lazy', $setColumns, 'lazy column should be omitted in fallback mode'); + $this->assertNotContains('type', $setColumns, 'type column should be omitted in fallback mode'); + $this->assertNotContains('flags', $setColumns, 'flags column should be omitted in fallback mode'); + $this->assertNotContains('indexed', $setColumns, 'indexed column should be omitted in fallback mode'); + } +}