Skip to content

Commit 343ceb5

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 51df4e9 commit 343ceb5

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;
@@ -827,10 +837,12 @@ private function setTypedValue(
827837
$insert = $this->connection->getQueryBuilder();
828838
$insert->insert('appconfig')
829839
->setValue('appid', $insert->createNamedParameter($app))
830-
->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
831-
->setValue('type', $insert->createNamedParameter($type, IQueryBuilder::PARAM_INT))
832840
->setValue('configkey', $insert->createNamedParameter($key))
833841
->setValue('configvalue', $insert->createNamedParameter($value));
842+
if ($this->migrationCompleted) {
843+
$insert->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
844+
->setValue('type', $insert->createNamedParameter($type, IQueryBuilder::PARAM_INT));
845+
}
834846
$insert->executeStatement();
835847
$inserted = true;
836848
} catch (DBException $e) {
@@ -890,10 +902,12 @@ private function setTypedValue(
890902
$update = $this->connection->getQueryBuilder();
891903
$update->update('appconfig')
892904
->set('configvalue', $update->createNamedParameter($value))
893-
->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
894-
->set('type', $update->createNamedParameter($type, IQueryBuilder::PARAM_INT))
895905
->where($update->expr()->eq('appid', $update->createNamedParameter($app)))
896906
->andWhere($update->expr()->eq('configkey', $update->createNamedParameter($key)));
907+
if ($this->migrationCompleted) {
908+
$update->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
909+
->set('type', $update->createNamedParameter($type, IQueryBuilder::PARAM_INT));
910+
}
897911

898912
$update->executeStatement();
899913
}
@@ -1347,23 +1361,39 @@ private function loadConfig(?string $app = null, bool $lazy = false): void {
13471361

13481362
// Otherwise no cache available and we need to fetch from database
13491363
$qb = $this->connection->getQueryBuilder();
1350-
$qb->from('appconfig')
1351-
->select('appid', 'configkey', 'configvalue', 'type');
1364+
$qb->from('appconfig');
13521365

1353-
if ($lazy === false) {
1354-
$qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)));
1366+
if (!$this->migrationCompleted) {
1367+
$qb->select('appid', 'configkey', 'configvalue');
13551368
} else {
1356-
if ($loadLazyOnly) {
1357-
$qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT)));
1369+
$qb->select('appid', 'configkey', 'configvalue', 'type');
1370+
1371+
if ($lazy === false) {
1372+
$qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)));
1373+
} else {
1374+
if ($loadLazyOnly) {
1375+
$qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT)));
1376+
}
1377+
$qb->addSelect('lazy');
1378+
}
1379+
}
1380+
1381+
try {
1382+
$result = $qb->executeQuery();
1383+
} catch (DBException $e) {
1384+
if ($e->getReason() !== DBException::REASON_INVALID_FIELD_NAME || !$this->migrationCompleted) {
1385+
throw $e;
13581386
}
1359-
$qb->addSelect('lazy');
1387+
// columns 'type' and 'lazy' don't exist yet (ownCloud migration)
1388+
$this->migrationCompleted = false;
1389+
$this->loadConfig($app, $lazy);
1390+
return;
13601391
}
13611392

1362-
$result = $qb->executeQuery();
13631393
$rows = $result->fetchAll();
13641394
foreach ($rows as $row) {
13651395
// most of the time, 'lazy' is not in the select because its value is already known
1366-
if ($lazy && ((int)$row['lazy']) === 1) {
1396+
if ($this->migrationCompleted && $lazy && ((int)$row['lazy']) === 1) {
13671397
$this->lazyCache[$row['appid']][$row['configkey']] = $row['configvalue'] ?? '';
13681398
} else {
13691399
$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,
@@ -1148,12 +1158,14 @@ private function setTypedValue(
11481158
$insert->insert('preferences')
11491159
->setValue('userid', $insert->createNamedParameter($userId))
11501160
->setValue('appid', $insert->createNamedParameter($app))
1151-
->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
1152-
->setValue('type', $insert->createNamedParameter($type->value, IQueryBuilder::PARAM_INT))
1153-
->setValue('flags', $insert->createNamedParameter($flags, IQueryBuilder::PARAM_INT))
1154-
->setValue('indexed', $insert->createNamedParameter($indexed))
11551161
->setValue('configkey', $insert->createNamedParameter($key))
11561162
->setValue('configvalue', $insert->createNamedParameter($value));
1163+
if ($this->migrationCompleted) {
1164+
$insert->setValue('lazy', $insert->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
1165+
->setValue('type', $insert->createNamedParameter($type->value, IQueryBuilder::PARAM_INT))
1166+
->setValue('flags', $insert->createNamedParameter($flags, IQueryBuilder::PARAM_INT))
1167+
->setValue('indexed', $insert->createNamedParameter($indexed));
1168+
}
11571169
$insert->executeStatement();
11581170
$inserted = true;
11591171
} catch (DBException $e) {
@@ -1205,13 +1217,15 @@ private function setTypedValue(
12051217
$update = $this->connection->getQueryBuilder();
12061218
$update->update('preferences')
12071219
->set('configvalue', $update->createNamedParameter($value))
1208-
->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
1209-
->set('type', $update->createNamedParameter($type->value, IQueryBuilder::PARAM_INT))
1210-
->set('flags', $update->createNamedParameter($flags, IQueryBuilder::PARAM_INT))
1211-
->set('indexed', $update->createNamedParameter($indexed))
12121220
->where($update->expr()->eq('userid', $update->createNamedParameter($userId)))
12131221
->andWhere($update->expr()->eq('appid', $update->createNamedParameter($app)))
12141222
->andWhere($update->expr()->eq('configkey', $update->createNamedParameter($key)));
1223+
if ($this->migrationCompleted) {
1224+
$update->set('lazy', $update->createNamedParameter(($lazy) ? 1 : 0, IQueryBuilder::PARAM_INT))
1225+
->set('type', $update->createNamedParameter($type->value, IQueryBuilder::PARAM_INT))
1226+
->set('flags', $update->createNamedParameter($flags, IQueryBuilder::PARAM_INT))
1227+
->set('indexed', $update->createNamedParameter($indexed));
1228+
}
12151229

12161230
$update->executeStatement();
12171231
}
@@ -1762,25 +1776,41 @@ private function loadConfig(string $userId, ?bool $lazy = false): void {
17621776

17631777
$qb = $this->connection->getQueryBuilder();
17641778
$qb->from('preferences');
1765-
$qb->select('appid', 'configkey', 'configvalue', 'type', 'flags');
17661779
$qb->where($qb->expr()->eq('userid', $qb->createNamedParameter($userId)));
17671780

1768-
// we only need value from lazy when loadConfig does not specify it
1769-
if ($lazy !== null) {
1770-
$qb->andWhere($qb->expr()->eq('lazy', $qb->createNamedParameter($lazy ? 1 : 0, IQueryBuilder::PARAM_INT)));
1781+
if (!$this->migrationCompleted) {
1782+
$qb->select('appid', 'configkey', 'configvalue');
17711783
} else {
1772-
$qb->addSelect('lazy');
1784+
$qb->select('appid', 'configkey', 'configvalue', 'type', 'flags');
1785+
1786+
// we only need value from lazy when loadConfig does not specify it
1787+
if ($lazy !== null) {
1788+
$qb->andWhere($qb->expr()->eq('lazy', $qb->createNamedParameter($lazy ? 1 : 0, IQueryBuilder::PARAM_INT)));
1789+
} else {
1790+
$qb->addSelect('lazy');
1791+
}
1792+
}
1793+
1794+
try {
1795+
$result = $qb->executeQuery();
1796+
} catch (DBException $e) {
1797+
if ($e->getReason() !== DBException::REASON_INVALID_FIELD_NAME || !$this->migrationCompleted) {
1798+
throw $e;
1799+
}
1800+
// columns 'type', 'lazy', 'flags', 'indexed' don't exist yet (ownCloud migration)
1801+
$this->migrationCompleted = false;
1802+
$this->loadConfig($userId, $lazy);
1803+
return;
17731804
}
17741805

1775-
$result = $qb->executeQuery();
17761806
$rows = $result->fetchAll();
17771807
foreach ($rows as $row) {
1778-
if (($row['lazy'] ?? ($lazy ?? 0) ? 1 : 0) === 1) {
1808+
if ($this->migrationCompleted && (($row['lazy'] ?? ($lazy ?? 0) ? 1 : 0) === 1)) {
17791809
$this->lazyCache[$userId][$row['appid']][$row['configkey']] = $row['configvalue'] ?? '';
17801810
} else {
17811811
$this->fastCache[$userId][$row['appid']][$row['configkey']] = $row['configvalue'] ?? '';
17821812
}
1783-
$this->valueDetails[$userId][$row['appid']][$row['configkey']] = ['type' => ValueType::from((int)($row['type'] ?? 0)), 'flags' => (int)$row['flags']];
1813+
$this->valueDetails[$userId][$row['appid']][$row['configkey']] = ['type' => ValueType::from((int)($row['type'] ?? 0)), 'flags' => (int)($row['flags'] ?? 0)];
17841814
}
17851815
$result->closeCursor();
17861816
$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)