Skip to content

Commit 705590e

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 ac7f429 commit 705590e

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
@@ -66,6 +66,16 @@ class AppConfig implements IAppConfig {
6666
private array $valueTypes = []; // type for all config values
6767
private bool $fastLoaded = false;
6868
private bool $lazyLoaded = false;
69+
/**
70+
* Tracks whether the NC-only columns (`type`, `lazy`) exist in the `appconfig` table.
71+
* Set to false on first load when a DBException::REASON_INVALID_FIELD_NAME is caught,
72+
* which happens during an ownCloud → Nextcloud migration before the schema steps have run.
73+
*
74+
* Every SELECT that reads those columns and every INSERT/UPDATE that writes them must
75+
* guard with `if ($this->migrationCompleted)` so they degrade gracefully.
76+
* If you add a new query that touches NC-only columns, add the same guard.
77+
*/
78+
private bool $migrationCompleted = true;
6979
/** @var array<string, array{entries: array<string, Entry>, aliases: array<string, string>, strictness: Strictness}> ['app_id' => ['strictness' => ConfigLexiconStrictness, 'entries' => ['config_key' => ConfigLexiconEntry[]]] */
7080
private array $configLexiconDetails = [];
7181
private bool $ignoreLexiconAliases = false;
@@ -834,10 +844,12 @@ private function setTypedValue(
834844
$insert = $this->connection->getQueryBuilder();
835845
$insert->insert('appconfig')
836846
->setValue('appid', $insert->createNamedParameter($app))
837-
->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
838-
->setValue('type', $insert->createNamedParameter($type, IQueryBuilder::PARAM_INT))
839847
->setValue('configkey', $insert->createNamedParameter($key))
840848
->setValue('configvalue', $insert->createNamedParameter($value));
849+
if ($this->migrationCompleted) {
850+
$insert->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
851+
->setValue('type', $insert->createNamedParameter($type, IQueryBuilder::PARAM_INT));
852+
}
841853
$insert->executeStatement();
842854
$inserted = true;
843855
} catch (DBException $e) {
@@ -897,10 +909,12 @@ private function setTypedValue(
897909
$update = $this->connection->getQueryBuilder();
898910
$update->update('appconfig')
899911
->set('configvalue', $update->createNamedParameter($value))
900-
->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
901-
->set('type', $update->createNamedParameter($type, IQueryBuilder::PARAM_INT))
902912
->where($update->expr()->eq('appid', $update->createNamedParameter($app)))
903913
->andWhere($update->expr()->eq('configkey', $update->createNamedParameter($key)));
914+
if ($this->migrationCompleted) {
915+
$update->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
916+
->set('type', $update->createNamedParameter($type, IQueryBuilder::PARAM_INT));
917+
}
904918

905919
$update->executeStatement();
906920
}
@@ -1354,23 +1368,39 @@ private function loadConfig(?string $app = null, bool $lazy = false): void {
13541368

13551369
// Otherwise no cache available and we need to fetch from database
13561370
$qb = $this->connection->getQueryBuilder();
1357-
$qb->from('appconfig')
1358-
->select('appid', 'configkey', 'configvalue', 'type');
1371+
$qb->from('appconfig');
13591372

1360-
if ($lazy === false) {
1361-
$qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)));
1373+
if (!$this->migrationCompleted) {
1374+
$qb->select('appid', 'configkey', 'configvalue');
13621375
} else {
1363-
if ($loadLazyOnly) {
1364-
$qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT)));
1376+
$qb->select('appid', 'configkey', 'configvalue', 'type');
1377+
1378+
if ($lazy === false) {
1379+
$qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)));
1380+
} else {
1381+
if ($loadLazyOnly) {
1382+
$qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT)));
1383+
}
1384+
$qb->addSelect('lazy');
1385+
}
1386+
}
1387+
1388+
try {
1389+
$result = $qb->executeQuery();
1390+
} catch (DBException $e) {
1391+
if ($e->getReason() !== DBException::REASON_INVALID_FIELD_NAME || !$this->migrationCompleted) {
1392+
throw $e;
13651393
}
1366-
$qb->addSelect('lazy');
1394+
// columns 'type' and 'lazy' don't exist yet (ownCloud migration)
1395+
$this->migrationCompleted = false;
1396+
$this->loadConfig($app, $lazy);
1397+
return;
13671398
}
13681399

1369-
$result = $qb->executeQuery();
13701400
$rows = $result->fetchAll();
13711401
foreach ($rows as $row) {
13721402
// most of the time, 'lazy' is not in the select because its value is already known
1373-
if ($lazy && ((int)$row['lazy']) === 1) {
1403+
if ($this->migrationCompleted && $lazy && ((int)$row['lazy']) === 1) {
13741404
$this->lazyCache[$row['appid']][$row['configkey']] = $row['configvalue'] ?? '';
13751405
} else {
13761406
$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
@@ -69,6 +69,16 @@ class UserConfig implements IUserConfig {
6969
private array $configLexiconDetails = [];
7070
private bool $ignoreLexiconAliases = false;
7171
private array $strictnessApplied = [];
72+
/**
73+
* Tracks whether the NC-only columns (`type`, `lazy`, `flags`, `indexed`) exist in the
74+
* `preferences` table. Set to false on first load when a DBException::REASON_INVALID_FIELD_NAME
75+
* is caught, which happens during an ownCloud → Nextcloud migration before the schema steps run.
76+
*
77+
* Every SELECT that reads those columns and every INSERT/UPDATE that writes them must
78+
* guard with `if ($this->migrationCompleted)` so they degrade gracefully.
79+
* If you add a new query that touches NC-only columns, add the same guard.
80+
*/
81+
private bool $migrationCompleted = true;
7282

7383
public function __construct(
7484
protected IDBConnection $connection,
@@ -1155,12 +1165,14 @@ private function setTypedValue(
11551165
$insert->insert('preferences')
11561166
->setValue('userid', $insert->createNamedParameter($userId))
11571167
->setValue('appid', $insert->createNamedParameter($app))
1158-
->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
1159-
->setValue('type', $insert->createNamedParameter($type->value, IQueryBuilder::PARAM_INT))
1160-
->setValue('flags', $insert->createNamedParameter($flags, IQueryBuilder::PARAM_INT))
1161-
->setValue('indexed', $insert->createNamedParameter($indexed))
11621168
->setValue('configkey', $insert->createNamedParameter($key))
11631169
->setValue('configvalue', $insert->createNamedParameter($value));
1170+
if ($this->migrationCompleted) {
1171+
$insert->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
1172+
->setValue('type', $insert->createNamedParameter($type->value, IQueryBuilder::PARAM_INT))
1173+
->setValue('flags', $insert->createNamedParameter($flags, IQueryBuilder::PARAM_INT))
1174+
->setValue('indexed', $insert->createNamedParameter($indexed));
1175+
}
11641176
$insert->executeStatement();
11651177
$inserted = true;
11661178
} catch (DBException $e) {
@@ -1212,13 +1224,15 @@ private function setTypedValue(
12121224
$update = $this->connection->getQueryBuilder();
12131225
$update->update('preferences')
12141226
->set('configvalue', $update->createNamedParameter($value))
1215-
->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
1216-
->set('type', $update->createNamedParameter($type->value, IQueryBuilder::PARAM_INT))
1217-
->set('flags', $update->createNamedParameter($flags, IQueryBuilder::PARAM_INT))
1218-
->set('indexed', $update->createNamedParameter($indexed))
12191227
->where($update->expr()->eq('userid', $update->createNamedParameter($userId)))
12201228
->andWhere($update->expr()->eq('appid', $update->createNamedParameter($app)))
12211229
->andWhere($update->expr()->eq('configkey', $update->createNamedParameter($key)));
1230+
if ($this->migrationCompleted) {
1231+
$update->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
1232+
->set('type', $update->createNamedParameter($type->value, IQueryBuilder::PARAM_INT))
1233+
->set('flags', $update->createNamedParameter($flags, IQueryBuilder::PARAM_INT))
1234+
->set('indexed', $update->createNamedParameter($indexed));
1235+
}
12221236

12231237
$update->executeStatement();
12241238
}
@@ -1769,25 +1783,41 @@ private function loadConfig(string $userId, ?bool $lazy = false): void {
17691783

17701784
$qb = $this->connection->getQueryBuilder();
17711785
$qb->from('preferences');
1772-
$qb->select('appid', 'configkey', 'configvalue', 'type', 'flags');
17731786
$qb->where($qb->expr()->eq('userid', $qb->createNamedParameter($userId)));
17741787

1775-
// we only need value from lazy when loadConfig does not specify it
1776-
if ($lazy !== null) {
1777-
$qb->andWhere($qb->expr()->eq('lazy', $qb->createNamedParameter($lazy ? 1 : 0, IQueryBuilder::PARAM_INT)));
1788+
if (!$this->migrationCompleted) {
1789+
$qb->select('appid', 'configkey', 'configvalue');
17781790
} else {
1779-
$qb->addSelect('lazy');
1791+
$qb->select('appid', 'configkey', 'configvalue', 'type', 'flags');
1792+
1793+
// we only need value from lazy when loadConfig does not specify it
1794+
if ($lazy !== null) {
1795+
$qb->andWhere($qb->expr()->eq('lazy', $qb->createNamedParameter($lazy ? 1 : 0, IQueryBuilder::PARAM_INT)));
1796+
} else {
1797+
$qb->addSelect('lazy');
1798+
}
1799+
}
1800+
1801+
try {
1802+
$result = $qb->executeQuery();
1803+
} catch (DBException $e) {
1804+
if ($e->getReason() !== DBException::REASON_INVALID_FIELD_NAME || !$this->migrationCompleted) {
1805+
throw $e;
1806+
}
1807+
// columns 'type', 'lazy', 'flags', 'indexed' don't exist yet (ownCloud migration)
1808+
$this->migrationCompleted = false;
1809+
$this->loadConfig($userId, $lazy);
1810+
return;
17801811
}
17811812

1782-
$result = $qb->executeQuery();
17831813
$rows = $result->fetchAll();
17841814
foreach ($rows as $row) {
1785-
if (($row['lazy'] ?? ($lazy ?? 0) ? 1 : 0) === 1) {
1815+
if ($this->migrationCompleted && (($row['lazy'] ?? ($lazy ?? 0) ? 1 : 0) === 1)) {
17861816
$this->lazyCache[$userId][$row['appid']][$row['configkey']] = $row['configvalue'] ?? '';
17871817
} else {
17881818
$this->fastCache[$userId][$row['appid']][$row['configkey']] = $row['configvalue'] ?? '';
17891819
}
1790-
$this->valueDetails[$userId][$row['appid']][$row['configkey']] = ['type' => ValueType::from((int)($row['type'] ?? 0)), 'flags' => (int)$row['flags']];
1820+
$this->valueDetails[$userId][$row['appid']][$row['configkey']] = ['type' => ValueType::from((int)($row['type'] ?? 0)), 'flags' => (int)($row['flags'] ?? 0)];
17911821
}
17921822
$result->closeCursor();
17931823
$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)