Skip to content

Commit b2c9273

Browse files
miaulalalamiaulalala
committed
fix(appconfig,userconfig): restore pre-migration fallback for ownCloud migration
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: #57340 Signed-off-by: Anna Larch <anna@nextcloud.com> AI-Assisted-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 50ddee1 commit b2c9273

4 files changed

Lines changed: 454 additions & 29 deletions

File tree

‎lib/private/AppConfig.php‎

Lines changed: 43 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,16 @@ class AppConfig implements IAppConfig {
6767
private array $valueTypes = []; // type for all config values
6868
private bool $fastLoaded = false;
6969
private bool $lazyLoaded = false;
70+
/**
71+
* Tracks whether the NC-only columns (`type`, `lazy`) exist in the `appconfig` table.
72+
* Set to false on first load when a DBException::REASON_INVALID_FIELD_NAME is caught,
73+
* which happens during an ownCloud → Nextcloud migration before the schema steps have run.
74+
*
75+
* Every SELECT that reads those columns and every INSERT/UPDATE that writes them must
76+
* guard with `if ($this->migrationCompleted)` so they degrade gracefully.
77+
* If you add a new query that touches NC-only columns, add the same guard.
78+
*/
79+
private bool $migrationCompleted = true;
7080
/** @var array<string, array{entries: array<string, Entry>, aliases: array<string, string>, strictness: Strictness}> ['app_id' => ['strictness' => ConfigLexiconStrictness, 'entries' => ['config_key' => ConfigLexiconEntry[]]] */
7181
private array $configLexiconDetails = [];
7282
private bool $ignoreLexiconAliases = false;
@@ -854,10 +864,12 @@ private function setTypedValue(
854864
$insert = $this->connection->getQueryBuilder();
855865
$insert->insert('appconfig')
856866
->setValue('appid', $insert->createNamedParameter($app))
857-
->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
858-
->setValue('type', $insert->createNamedParameter($type, IQueryBuilder::PARAM_INT))
859867
->setValue('configkey', $insert->createNamedParameter($key))
860868
->setValue('configvalue', $insert->createNamedParameter($value));
869+
if ($this->migrationCompleted) {
870+
$insert->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
871+
->setValue('type', $insert->createNamedParameter($type, IQueryBuilder::PARAM_INT));
872+
}
861873
$insert->executeStatement();
862874
$inserted = true;
863875
} catch (DBException $e) {
@@ -917,10 +929,12 @@ private function setTypedValue(
917929
$update = $this->connection->getQueryBuilder();
918930
$update->update('appconfig')
919931
->set('configvalue', $update->createNamedParameter($value))
920-
->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
921-
->set('type', $update->createNamedParameter($type, IQueryBuilder::PARAM_INT))
922932
->where($update->expr()->eq('appid', $update->createNamedParameter($app)))
923933
->andWhere($update->expr()->eq('configkey', $update->createNamedParameter($key)));
934+
if ($this->migrationCompleted) {
935+
$update->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
936+
->set('type', $update->createNamedParameter($type, IQueryBuilder::PARAM_INT));
937+
}
924938

925939
$update->executeStatement();
926940
}
@@ -1383,23 +1397,39 @@ private function loadConfig(?string $app = null, bool $lazy = false): void {
13831397

13841398
// Otherwise no cache available and we need to fetch from database
13851399
$qb = $this->connection->getQueryBuilder();
1386-
$qb->from('appconfig')
1387-
->select('appid', 'configkey', 'configvalue', 'type');
1400+
$qb->from('appconfig');
13881401

1389-
if ($lazy === false) {
1390-
$qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)));
1402+
if (!$this->migrationCompleted) {
1403+
$qb->select('appid', 'configkey', 'configvalue');
13911404
} else {
1392-
if ($loadLazyOnly) {
1393-
$qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT)));
1405+
$qb->select('appid', 'configkey', 'configvalue', 'type');
1406+
1407+
if ($lazy === false) {
1408+
$qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)));
1409+
} else {
1410+
if ($loadLazyOnly) {
1411+
$qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT)));
1412+
}
1413+
$qb->addSelect('lazy');
1414+
}
1415+
}
1416+
1417+
try {
1418+
$result = $qb->executeQuery();
1419+
} catch (DBException $e) {
1420+
if ($e->getReason() !== DBException::REASON_INVALID_FIELD_NAME || !$this->migrationCompleted) {
1421+
throw $e;
13941422
}
1395-
$qb->addSelect('lazy');
1423+
// columns 'type' and 'lazy' don't exist yet (ownCloud migration)
1424+
$this->migrationCompleted = false;
1425+
$this->loadConfig($app, $lazy);
1426+
return;
13961427
}
13971428

1398-
$result = $qb->executeQuery();
13991429
$rows = $result->fetchAll();
14001430
foreach ($rows as $row) {
14011431
// most of the time, 'lazy' is not in the select because its value is already known
1402-
if ($lazy && ((int)$row['lazy']) === 1) {
1432+
if ($this->migrationCompleted && $lazy && ((int)$row['lazy']) === 1) {
14031433
$this->lazyCache[$row['appid']][$row['configkey']] = $row['configvalue'] ?? '';
14041434
} else {
14051435
$this->fastCache[$row['appid']][$row['configkey']] = $row['configvalue'] ?? '';

‎lib/private/Config/UserConfig.php‎

Lines changed: 46 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,16 @@ class UserConfig implements IUserConfig {
7070
private array $configLexiconDetails = [];
7171
private bool $ignoreLexiconAliases = false;
7272
private array $strictnessApplied = [];
73+
/**
74+
* Tracks whether the NC-only columns (`type`, `lazy`, `flags`, `indexed`) exist in the
75+
* `preferences` table. Set to false on first load when a DBException::REASON_INVALID_FIELD_NAME
76+
* is caught, which happens during an ownCloud → Nextcloud migration before the schema steps run.
77+
*
78+
* Every SELECT that reads those columns and every INSERT/UPDATE that writes them must
79+
* guard with `if ($this->migrationCompleted)` so they degrade gracefully.
80+
* If you add a new query that touches NC-only columns, add the same guard.
81+
*/
82+
private bool $migrationCompleted = true;
7383

7484
public function __construct(
7585
protected IDBConnection $connection,
@@ -1183,12 +1193,14 @@ private function setTypedValue(
11831193
$insert->insert('preferences')
11841194
->setValue('userid', $insert->createNamedParameter($userId))
11851195
->setValue('appid', $insert->createNamedParameter($app))
1186-
->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
1187-
->setValue('type', $insert->createNamedParameter($type->value, IQueryBuilder::PARAM_INT))
1188-
->setValue('flags', $insert->createNamedParameter($flags, IQueryBuilder::PARAM_INT))
1189-
->setValue('indexed', $insert->createNamedParameter($indexed))
11901196
->setValue('configkey', $insert->createNamedParameter($key))
11911197
->setValue('configvalue', $insert->createNamedParameter($value));
1198+
if ($this->migrationCompleted) {
1199+
$insert->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
1200+
->setValue('type', $insert->createNamedParameter($type->value, IQueryBuilder::PARAM_INT))
1201+
->setValue('flags', $insert->createNamedParameter($flags, IQueryBuilder::PARAM_INT))
1202+
->setValue('indexed', $insert->createNamedParameter($indexed));
1203+
}
11921204
$insert->executeStatement();
11931205
$inserted = true;
11941206
} catch (DBException $e) {
@@ -1240,13 +1252,15 @@ private function setTypedValue(
12401252
$update = $this->connection->getQueryBuilder();
12411253
$update->update('preferences')
12421254
->set('configvalue', $update->createNamedParameter($value))
1243-
->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
1244-
->set('type', $update->createNamedParameter($type->value, IQueryBuilder::PARAM_INT))
1245-
->set('flags', $update->createNamedParameter($flags, IQueryBuilder::PARAM_INT))
1246-
->set('indexed', $update->createNamedParameter($indexed))
12471255
->where($update->expr()->eq('userid', $update->createNamedParameter($userId)))
12481256
->andWhere($update->expr()->eq('appid', $update->createNamedParameter($app)))
12491257
->andWhere($update->expr()->eq('configkey', $update->createNamedParameter($key)));
1258+
if ($this->migrationCompleted) {
1259+
$update->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
1260+
->set('type', $update->createNamedParameter($type->value, IQueryBuilder::PARAM_INT))
1261+
->set('flags', $update->createNamedParameter($flags, IQueryBuilder::PARAM_INT))
1262+
->set('indexed', $update->createNamedParameter($indexed));
1263+
}
12501264

12511265
$update->executeStatement();
12521266
}
@@ -1810,25 +1824,41 @@ private function loadConfig(string $userId, ?bool $lazy = false): void {
18101824

18111825
$qb = $this->connection->getQueryBuilder();
18121826
$qb->from('preferences');
1813-
$qb->select('appid', 'configkey', 'configvalue', 'type', 'flags');
18141827
$qb->where($qb->expr()->eq('userid', $qb->createNamedParameter($userId)));
18151828

1816-
// we only need value from lazy when loadConfig does not specify it
1817-
if ($lazy !== null) {
1818-
$qb->andWhere($qb->expr()->eq('lazy', $qb->createNamedParameter($lazy ? 1 : 0, IQueryBuilder::PARAM_INT)));
1829+
if (!$this->migrationCompleted) {
1830+
$qb->select('appid', 'configkey', 'configvalue');
18191831
} else {
1820-
$qb->addSelect('lazy');
1832+
$qb->select('appid', 'configkey', 'configvalue', 'type', 'flags');
1833+
1834+
// we only need value from lazy when loadConfig does not specify it
1835+
if ($lazy !== null) {
1836+
$qb->andWhere($qb->expr()->eq('lazy', $qb->createNamedParameter($lazy ? 1 : 0, IQueryBuilder::PARAM_INT)));
1837+
} else {
1838+
$qb->addSelect('lazy');
1839+
}
1840+
}
1841+
1842+
try {
1843+
$result = $qb->executeQuery();
1844+
} catch (DBException $e) {
1845+
if ($e->getReason() !== DBException::REASON_INVALID_FIELD_NAME || !$this->migrationCompleted) {
1846+
throw $e;
1847+
}
1848+
// columns 'type', 'lazy', 'flags', 'indexed' don't exist yet (ownCloud migration)
1849+
$this->migrationCompleted = false;
1850+
$this->loadConfig($userId, $lazy);
1851+
return;
18211852
}
18221853

1823-
$result = $qb->executeQuery();
18241854
$rows = $result->fetchAll();
18251855
foreach ($rows as $row) {
1826-
if (($row['lazy'] ?? ($lazy ?? 0) ? 1 : 0) === 1) {
1856+
if ($this->migrationCompleted && (($row['lazy'] ?? ($lazy ?? 0) ? 1 : 0) === 1)) {
18271857
$this->lazyCache[$userId][$row['appid']][$row['configkey']] = $row['configvalue'] ?? '';
18281858
} else {
18291859
$this->fastCache[$userId][$row['appid']][$row['configkey']] = $row['configvalue'] ?? '';
18301860
}
1831-
$this->valueDetails[$userId][$row['appid']][$row['configkey']] = ['type' => ValueType::from((int)($row['type'] ?? 0)), 'flags' => (int)$row['flags']];
1861+
$this->valueDetails[$userId][$row['appid']][$row['configkey']] = ['type' => ValueType::from((int)($row['type'] ?? 0)), 'flags' => (int)($row['flags'] ?? 0)];
18321862
}
18331863
$result->closeCursor();
18341864
$this->setAsLoaded($userId, $lazy);

‎tests/lib/AppConfigMigrationFallbackTest.php‎

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
/**
5+
* SPDX-FileCopyrightText: 2025 Nextcloud GmbH and Nextcloud contributors
6+
* SPDX-License-Identifier: AGPL-3.0-or-later
7+
*/
8+
namespace Test;
9+
10+
use Doctrine\DBAL\Exception\InvalidFieldNameException;
11+
use OC\AppConfig;
12+
use OC\Config\ConfigManager;
13+
use OC\Config\PresetManager;
14+
use OC\DB\Exceptions\DbalException;
15+
use OC\Memcache\Factory as CacheFactory;
16+
use OCP\DB\Exception as DBException;
17+
use OCP\DB\IResult;
18+
use OCP\DB\QueryBuilder\IExpressionBuilder;
19+
use OCP\DB\QueryBuilder\IQueryBuilder;
20+
use OCP\IConfig;
21+
use OCP\IDBConnection;
22+
use OCP\Security\ICrypto;
23+
use PHPUnit\Framework\MockObject\MockObject;
24+
use Psr\Log\LoggerInterface;
25+
26+
/**
27+
* Tests the ownCloud migration fallback in AppConfig.
28+
*
29+
* When migrating from ownCloud, the appconfig table lacks 'type' and 'lazy'
30+
* columns. AppConfig::loadConfig() must catch the resulting DBException and
31+
* retry with a query that only selects columns present in ownCloud's schema.
32+
*/
33+
class AppConfigMigrationFallbackTest extends TestCase {
34+
private IConfig&MockObject $config;
35+
private IDBConnection&MockObject $connection;
36+
private ConfigManager&MockObject $configManager;
37+
private PresetManager&MockObject $presetManager;
38+
private LoggerInterface&MockObject $logger;
39+
private ICrypto&MockObject $crypto;
40+
private CacheFactory&MockObject $cacheFactory;
41+
42+
protected function setUp(): void {
43+
parent::setUp();
44+
45+
$this->connection = $this->createMock(IDBConnection::class);
46+
$this->config = $this->createMock(IConfig::class);
47+
$this->configManager = $this->createMock(ConfigManager::class);
48+
$this->presetManager = $this->createMock(PresetManager::class);
49+
$this->logger = $this->createMock(LoggerInterface::class);
50+
$this->crypto = $this->createMock(ICrypto::class);
51+
$this->cacheFactory = $this->createMock(CacheFactory::class);
52+
}
53+
54+
private function getAppConfig(): AppConfig {
55+
$this->config->method('getSystemValueBool')
56+
->with('cache_app_config', true)
57+
->willReturn(true);
58+
$this->cacheFactory->method('isLocalCacheAvailable')->willReturn(false);
59+
60+
return new AppConfig(
61+
$this->connection,
62+
$this->config,
63+
$this->configManager,
64+
$this->presetManager,
65+
$this->logger,
66+
$this->crypto,
67+
$this->cacheFactory,
68+
);
69+
}
70+
71+
private function createInvalidFieldNameException(): DBException {
72+
$driverException = $this->createMock(\Doctrine\DBAL\Driver\Exception::class);
73+
$dbalException = new InvalidFieldNameException($driverException, null);
74+
return DbalException::wrap($dbalException);
75+
}
76+
77+
private function createMockQueryBuilder(): IQueryBuilder&MockObject {
78+
$expression = $this->createMock(IExpressionBuilder::class);
79+
$qb = $this->createMock(IQueryBuilder::class);
80+
$qb->method('from')->willReturn($qb);
81+
$qb->method('select')->willReturn($qb);
82+
$qb->method('addSelect')->willReturn($qb);
83+
$qb->method('where')->willReturn($qb);
84+
$qb->method('andWhere')->willReturn($qb);
85+
$qb->method('set')->willReturn($qb);
86+
$qb->method('expr')->willReturn($expression);
87+
$qb->method('insert')->willReturn($qb);
88+
$qb->method('setValue')->willReturn($qb);
89+
$qb->method('createNamedParameter')->willReturn('?');
90+
return $qb;
91+
}
92+
93+
/**
94+
* Test that loadConfig retries without type/lazy columns on InvalidFieldNameException.
95+
*/
96+
public function testLoadConfigFallsBackOnMissingColumns(): void {
97+
$exception = $this->createInvalidFieldNameException();
98+
99+
$successResult = $this->createMock(IResult::class);
100+
$successResult->method('fetchAll')->willReturn([
101+
['appid' => 'core', 'configkey' => 'vendor', 'configvalue' => 'owncloud'],
102+
['appid' => 'core', 'configkey' => 'installedat', 'configvalue' => '1234567890'],
103+
]);
104+
105+
$qb = $this->createMockQueryBuilder();
106+
// First call throws (columns missing), second call succeeds (fallback query)
107+
$qb->method('executeQuery')
108+
->willReturnOnConsecutiveCalls(
109+
$this->throwException($exception),
110+
$successResult,
111+
);
112+
113+
$this->connection->method('getQueryBuilder')->willReturn($qb);
114+
115+
$appConfig = $this->getAppConfig();
116+
117+
// getValueString triggers loadConfig internally
118+
$value = $appConfig->getValueString('core', 'vendor');
119+
$this->assertSame('owncloud', $value);
120+
}
121+
122+
/**
123+
* Test that non-INVALID_FIELD_NAME exceptions are re-thrown, not swallowed.
124+
*/
125+
public function testLoadConfigRethrowsOtherExceptions(): void {
126+
$driverException = $this->createMock(\Doctrine\DBAL\Driver\Exception::class);
127+
$dbalException = new \Doctrine\DBAL\Exception\SyntaxErrorException($driverException, null);
128+
$exception = DbalException::wrap($dbalException);
129+
130+
$qb = $this->createMockQueryBuilder();
131+
$qb->method('executeQuery')->willThrowException($exception);
132+
133+
$this->connection->method('getQueryBuilder')->willReturn($qb);
134+
135+
$appConfig = $this->getAppConfig();
136+
137+
$this->expectException(DBException::class);
138+
$appConfig->getValueString('core', 'vendor');
139+
}
140+
141+
/**
142+
* Test that insert omits lazy/type columns when migration is not completed.
143+
*/
144+
public function testInsertOmitsNewColumnsInFallbackMode(): void {
145+
$exception = $this->createInvalidFieldNameException();
146+
147+
$loadResult = $this->createMock(IResult::class);
148+
$loadResult->method('fetchAll')->willReturn([]);
149+
150+
$qb = $this->createMockQueryBuilder();
151+
152+
$callCount = 0;
153+
$qb->method('executeQuery')
154+
->willReturnCallback(function () use ($exception, $loadResult, &$callCount) {
155+
$callCount++;
156+
if ($callCount === 1) {
157+
throw $exception;
158+
}
159+
return $loadResult;
160+
});
161+
162+
// Verify insert() is called (meaning we reached the insert path)
163+
$qb->expects(self::once())->method('insert')->with('appconfig')->willReturn($qb);
164+
$qb->method('executeStatement')->willReturn(1);
165+
166+
// Track which columns are set via setValue
167+
$setColumns = [];
168+
$qb->method('setValue')
169+
->willReturnCallback(function (string $column) use ($qb, &$setColumns) {
170+
$setColumns[] = $column;
171+
return $qb;
172+
});
173+
174+
$this->connection->method('getQueryBuilder')->willReturn($qb);
175+
176+
$appConfig = $this->getAppConfig();
177+
$appConfig->setValueString('core', 'vendor', 'owncloud');
178+
179+
$this->assertContains('appid', $setColumns);
180+
$this->assertContains('configkey', $setColumns);
181+
$this->assertContains('configvalue', $setColumns);
182+
$this->assertNotContains('lazy', $setColumns, 'lazy column should be omitted in fallback mode');
183+
$this->assertNotContains('type', $setColumns, 'type column should be omitted in fallback mode');
184+
}
185+
}

0 commit comments

Comments
 (0)