Skip to content

Commit bdc8d9f

Browse files
oleksandr-ncoleksandr-nc
committed
perf: load the app config values lazily
Since Nextcloud 29 every request loads the non-lazy app config rows of every app. All of this app's values were read and written through the deprecated IConfig::getAppValue() and setAppValue(), and setAppValue() always stores a key as non-lazy, although only dashboard_enabled and issue_notifications_enabled are read on every request, in Application::register(). Every read and write of an app config key now goes through the typed methods of IAppConfig, and every delete through its deleteAppValue(): the app-scoped IAppConfig in the app's code, and OCP\IAppConfig in the two migrations. The OAuth client id and secret, the default link token, use_popup, link_preview_enabled, the two allow_default_link_token_* switches and the default token's user_name and user_displayname pass lazy: true on every read and write, because a read without it can return the default once the key is lazy, and a write without it makes the key non-lazy again. dashboard_enabled and issue_notifications_enabled stay non-lazy. Stored values keep their format, and the secrets stay encrypted with ICrypto. Besides the three secrets, the two admin endpoints wrote any key the request carried. Written lazily, the server's own enabled key would make the app look disabled, so setAdminConfig() now accepts only the six switches of the admin settings and setSensitiveAdminConfig() only the three secrets, both only as strings, and they answer 400 without writing anything otherwise. A new migration marks the lazy keys lazy on existing installs with IAppConfig::updateLazy(). The unit tests pin the flag and the default of each call, which of the app's keys, the server's own keys and an unknown key each endpoint accepts, and the keys the migration marks. Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
1 parent 0d695a0 commit bdc8d9f

18 files changed

Lines changed: 875 additions & 58 deletions

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
1010

1111
## [Unreleased]
1212

13+
### Changed
14+
15+
- Stop loading most of the app's settings on every request
16+
1317
## 3.3.0 - 2026-10-02
1418

1519
### Added

‎lib/AppInfo/Application.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
use OCP\AppFramework\Bootstrap\IBootContext;
2020
use OCP\AppFramework\Bootstrap\IBootstrap;
2121
use OCP\AppFramework\Bootstrap\IRegistrationContext;
22+
use OCP\AppFramework\Services\IAppConfig;
2223

2324
use OCP\Collaboration\Reference\RenderReferenceEvent;
2425

@@ -37,20 +38,22 @@ class Application extends App implements IBootstrap {
3738

3839
private ContainerInterface $container;
3940
private IConfig $config;
41+
private IAppConfig $appConfig;
4042

4143
public function __construct(array $urlParams = []) {
4244
parent::__construct(self::APP_ID, $urlParams);
4345

4446
$this->container = $this->getContainer();
4547
$this->config = $this->container->get(IConfig::class);
48+
$this->appConfig = $this->container->get(IAppConfig::class);
4649
}
4750

4851
public function register(IRegistrationContext $context): void {
49-
if ($this->config->getAppValue(self::APP_ID, 'dashboard_enabled', '1') === '1') {
52+
if ($this->appConfig->getAppValueString('dashboard_enabled', '1') === '1') {
5053
$context->registerDashboardWidget(GithubWidget::class);
5154
}
5255

53-
if ($this->config->getAppValue(self::APP_ID, 'issue_notifications_enabled', '1') === '1') {
56+
if ($this->appConfig->getAppValueString('issue_notifications_enabled', '1') === '1') {
5457
$context->registerNotifierService(GitHubNotifier::class);
5558
}
5659

‎lib/Controller/ConfigController.php‎

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
use OCP\AppFramework\Http\DataResponse;
2222
use OCP\AppFramework\Http\RedirectResponse;
2323
use OCP\AppFramework\Http\TemplateResponse;
24+
use OCP\AppFramework\Services\IAppConfig;
2425
use OCP\AppFramework\Services\IInitialState;
2526
use OCP\IConfig;
2627
use OCP\IL10N;
@@ -32,10 +33,27 @@
3233

3334
class ConfigController extends Controller {
3435

36+
/**
37+
* The switches of the admin settings, and whether each one is stored lazily.
38+
* dashboard_enabled and issue_notifications_enabled are read in Application::register(),
39+
* which runs on every request, so they stay non-lazy.
40+
*/
41+
private const ADMIN_SWITCHES = [
42+
'use_popup' => true,
43+
'link_preview_enabled' => true,
44+
'allow_default_link_token_to_anonymous' => true,
45+
'allow_default_link_token_to_guests' => true,
46+
'dashboard_enabled' => false,
47+
'issue_notifications_enabled' => false,
48+
];
49+
50+
private const SECRET_KEYS = ['client_id', 'client_secret', 'default_link_token'];
51+
3552
public function __construct(
3653
string $appName,
3754
IRequest $request,
3855
private IConfig $config,
56+
private IAppConfig $appConfig,
3957
private IURLGenerator $urlGenerator,
4058
private IL10N $l,
4159
private IInitialState $initialStateService,
@@ -108,13 +126,15 @@ public function setConfig(array $values): DataResponse {
108126
* @return DataResponse
109127
*/
110128
public function setAdminConfig(array $values): DataResponse {
129+
// only the switches, so that a request cannot write a key the server owns, such as "enabled"
111130
foreach ($values as $key => $value) {
112-
if (in_array($key, ['client_id', 'client_secret', 'default_link_token'], true)) {
131+
if (!array_key_exists($key, self::ADMIN_SWITCHES) || !is_string($value)) {
113132
return new DataResponse([], Http::STATUS_BAD_REQUEST);
114-
} else {
115-
$this->config->setAppValue(Application::APP_ID, $key, $value);
116133
}
117134
}
135+
foreach ($values as $key => $value) {
136+
$this->appConfig->setAppValueString($key, $value, lazy: self::ADMIN_SWITCHES[$key]);
137+
}
118138
return new DataResponse([]);
119139
}
120140

@@ -127,14 +147,15 @@ public function setAdminConfig(array $values): DataResponse {
127147
#[PasswordConfirmationRequired]
128148
public function setSensitiveAdminConfig(array $values): DataResponse {
129149
foreach ($values as $key => $value) {
130-
if (in_array($key, ['client_id', 'client_secret', 'default_link_token'], true)) {
131-
$this->secretService->setEncryptedAppValue($key, $value);
132-
if ($key === 'default_link_token') {
133-
$info = $this->githubAPIService->updateLinkTokenUserInfo($value);
134-
return new DataResponse($info);
135-
}
136-
} else {
137-
$this->config->setAppValue(Application::APP_ID, $key, $value);
150+
if (!in_array($key, self::SECRET_KEYS, true) || !is_string($value)) {
151+
return new DataResponse([], Http::STATUS_BAD_REQUEST);
152+
}
153+
}
154+
foreach ($values as $key => $value) {
155+
$this->secretService->setEncryptedAppValue($key, $value);
156+
if ($key === 'default_link_token') {
157+
$info = $this->githubAPIService->updateLinkTokenUserInfo($value);
158+
return new DataResponse($info);
138159
}
139160
}
140161
return new DataResponse([]);
@@ -184,7 +205,7 @@ public function oauthRedirect(string $code, string $state): RedirectResponse {
184205
$this->config->setUserValue($this->userId, Application::APP_ID, 'token_type', 'oauth');
185206
$userInfo = $this->storeUserInfo();
186207

187-
$usePopup = $this->config->getAppValue(Application::APP_ID, 'use_popup', '0') === '1';
208+
$usePopup = $this->appConfig->getAppValueString('use_popup', '0', lazy: true) === '1';
188209
if ($usePopup) {
189210
return new RedirectResponse(
190211
$this->urlGenerator->linkToRoute('integration_github.config.popupSuccessPage', [

‎lib/Dashboard/GithubWidget.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,14 @@
99

1010
use OCA\Github\AppInfo\Application;
1111
use OCA\Github\Service\GithubAPIService;
12+
use OCP\AppFramework\Services\IAppConfig;
1213
use OCP\AppFramework\Services\IInitialState;
1314
use OCP\Dashboard\IAPIWidget;
1415
use OCP\Dashboard\IButtonWidget;
1516
use OCP\Dashboard\IIconWidget;
1617
use OCP\Dashboard\IOptionWidget;
1718
use OCP\Dashboard\Model\WidgetButton;
1819
use OCP\Dashboard\Model\WidgetOptions;
19-
use OCP\IConfig;
2020
use OCP\IL10N;
2121
use OCP\IURLGenerator;
2222

@@ -26,7 +26,7 @@ class GithubWidget implements IAPIWidget, IButtonWidget, IIconWidget, IOptionWid
2626

2727
public function __construct(
2828
private IL10N $l10n,
29-
private IConfig $config,
29+
private IAppConfig $appConfig,
3030
private IURLGenerator $urlGenerator,
3131
private IInitialState $initialStateService,
3232
private GithubAPIService $githubAPIService,
@@ -82,10 +82,10 @@ public function getUrl(): ?string {
8282
* @inheritDoc
8383
*/
8484
public function load(): void {
85-
$clientID = $this->config->getAppValue(Application::APP_ID, 'client_id');
86-
$clientSecret = $this->config->getAppValue(Application::APP_ID, 'client_secret');
85+
$clientID = $this->appConfig->getAppValueString('client_id', lazy: true);
86+
$clientSecret = $this->appConfig->getAppValueString('client_secret', lazy: true);
8787
$oauthPossible = $clientID !== '' && $clientSecret !== '';
88-
$usePopup = $this->config->getAppValue(Application::APP_ID, 'use_popup', '0');
88+
$usePopup = $this->appConfig->getAppValueString('use_popup', '0', lazy: true);
8989

9090
$userConfig = [
9191
'oauth_is_possible' => $oauthPossible,

‎lib/Migration/Version030010Date20241002015812.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
use Closure;
1212
use OCA\Github\AppInfo\Application;
1313
use OCP\DB\QueryBuilder\IQueryBuilder;
14-
use OCP\IConfig;
14+
use OCP\IAppConfig;
1515
use OCP\IDBConnection;
1616
use OCP\Migration\IOutput;
1717
use OCP\Migration\SimpleMigrationStep;
@@ -22,7 +22,7 @@ class Version030010Date20241002015812 extends SimpleMigrationStep {
2222
public function __construct(
2323
private IDBConnection $connection,
2424
private ICrypto $crypto,
25-
private IConfig $config,
25+
private IAppConfig $appConfig,
2626
) {
2727
}
2828

@@ -34,10 +34,10 @@ public function __construct(
3434
public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options) {
3535
// client_id, client_secret, default_link_token
3636
foreach (['client_id', 'client_secret', 'default_link_token'] as $key) {
37-
$value = $this->config->getAppValue(Application::APP_ID, $key);
37+
$value = $this->appConfig->getValueString(Application::APP_ID, $key, lazy: true);
3838
if ($value !== '') {
3939
$encryptedValue = $this->crypto->encrypt($value);
40-
$this->config->setAppValue(Application::APP_ID, $key, $encryptedValue);
40+
$this->appConfig->setValueString(Application::APP_ID, $key, $encryptedValue, lazy: true);
4141
}
4242
}
4343

‎lib/Migration/Version030301Date20261021102507.php‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\Github\Migration;
11+
12+
use Closure;
13+
use OCA\Github\AppInfo\Application;
14+
use OCP\DB\ISchemaWrapper;
15+
use OCP\IAppConfig;
16+
use OCP\Migration\IOutput;
17+
use OCP\Migration\SimpleMigrationStep;
18+
19+
class Version030301Date20261021102507 extends SimpleMigrationStep {
20+
21+
/**
22+
* The app config keys that are only read by the app's own pages, settings, providers,
23+
* OAuth flow and API requests. dashboard_enabled and issue_notifications_enabled are
24+
* read on every request in Application::register() and stay non-lazy.
25+
*/
26+
private const LAZY_APP_CONFIG_KEYS = [
27+
'client_id',
28+
'client_secret',
29+
'default_link_token',
30+
'use_popup',
31+
'link_preview_enabled',
32+
'allow_default_link_token_to_anonymous',
33+
'allow_default_link_token_to_guests',
34+
'user_name',
35+
'user_displayname',
36+
];
37+
38+
public function __construct(
39+
private IAppConfig $appConfig,
40+
) {
41+
}
42+
43+
/**
44+
* @param IOutput $output
45+
* @param Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper`
46+
* @param array $options
47+
*
48+
* @return null|ISchemaWrapper
49+
*/
50+
public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options) {
51+
// mark the values stored as non-lazy by earlier versions as lazy
52+
foreach (self::LAZY_APP_CONFIG_KEYS as $key) {
53+
$this->appConfig->updateLazy(Application::APP_ID, $key, true);
54+
}
55+
56+
return null;
57+
}
58+
}

‎lib/Reference/GithubCodeReferenceProvider.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use OC\Collaboration\Reference\ReferenceManager;
1111
use OCA\Github\AppInfo\Application;
1212
use OCA\Github\Service\GithubAPIService;
13+
use OCP\AppFramework\Services\IAppConfig;
1314
use OCP\Collaboration\Reference\ADiscoverableReferenceProvider;
1415
use OCP\Collaboration\Reference\IReference;
1516
use OCP\Collaboration\Reference\Reference;
@@ -24,6 +25,7 @@ class GithubCodeReferenceProvider extends ADiscoverableReferenceProvider {
2425
public function __construct(
2526
private GithubAPIService $githubAPIService,
2627
private IConfig $config,
28+
private IAppConfig $appConfig,
2729
private IL10N $l10n,
2830
private IURLGenerator $urlGenerator,
2931
private ReferenceManager $referenceManager,
@@ -71,7 +73,7 @@ public function matchReference(string $referenceText): bool {
7173
return false;
7274
}
7375
}
74-
$adminLinkPreviewEnabled = $this->config->getAppValue(Application::APP_ID, 'link_preview_enabled', '1') === '1';
76+
$adminLinkPreviewEnabled = $this->appConfig->getAppValueString('link_preview_enabled', '1', lazy: true) === '1';
7577
if (!$adminLinkPreviewEnabled) {
7678
return false;
7779
}

‎lib/Reference/GithubIssuePrReferenceProvider.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use OC\Collaboration\Reference\ReferenceManager;
1313
use OCA\Github\AppInfo\Application;
1414
use OCA\Github\Service\GithubAPIService;
15+
use OCP\AppFramework\Services\IAppConfig;
1516
use OCP\Collaboration\Reference\ADiscoverableReferenceProvider;
1617
use OCP\Collaboration\Reference\IReference;
1718
use OCP\Collaboration\Reference\ISearchableReferenceProvider;
@@ -31,6 +32,7 @@ class GithubIssuePrReferenceProvider extends ADiscoverableReferenceProvider impl
3132
public function __construct(
3233
private GithubAPIService $githubAPIService,
3334
private IConfig $config,
35+
private IAppConfig $appConfig,
3436
private IL10N $l10n,
3537
private IURLGenerator $urlGenerator,
3638
private ReferenceManager $referenceManager,
@@ -85,7 +87,7 @@ public function matchReference(string $referenceText): bool {
8587
return false;
8688
}
8789
}
88-
$adminLinkPreviewEnabled = $this->config->getAppValue(Application::APP_ID, 'link_preview_enabled', '1') === '1';
90+
$adminLinkPreviewEnabled = $this->appConfig->getAppValueString('link_preview_enabled', '1', lazy: true) === '1';
8991
if (!$adminLinkPreviewEnabled) {
9092
return false;
9193
}

‎lib/Service/GithubAPIService.php‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
use GuzzleHttp\Exception\ClientException;
1414
use GuzzleHttp\Exception\ServerException;
1515
use OCA\Github\AppInfo\Application;
16+
use OCP\AppFramework\Services\IAppConfig;
1617
use OCP\Dashboard\Model\WidgetItem;
1718
use OCP\Http\Client\IClient;
1819
use OCP\Http\Client\IClientService;
@@ -37,6 +38,7 @@ public function __construct(
3738
private LoggerInterface $logger,
3839
private IL10N $l10n,
3940
private IConfig $config,
41+
private IAppConfig $appConfig,
4042
private IURLGenerator $urlGenerator,
4143
private IUserManager $userManager,
4244
private IManager $notificationManager,
@@ -377,15 +379,15 @@ public function getCommitInfo(?string $userId, string $owner, string $repo, stri
377379
public function updateLinkTokenUserInfo(string $defaultLinkToken): array {
378380
$info = $this->request(null, 'user', [], 'GET', true, 30, $defaultLinkToken);
379381
if (isset($info['login'], $info['name'])) {
380-
$this->config->setAppValue(Application::APP_ID, 'user_name', $info['login']);
381-
$this->config->setAppValue(Application::APP_ID, 'user_displayname', $info['name']);
382+
$this->appConfig->setAppValueString('user_name', $info['login'], lazy: true);
383+
$this->appConfig->setAppValueString('user_displayname', $info['name'], lazy: true);
382384
return [
383385
'user_name' => $info['login'],
384386
'user_displayname' => $info['name'],
385387
];
386388
} else {
387-
$this->config->deleteAppValue(Application::APP_ID, 'user_name');
388-
$this->config->deleteAppValue(Application::APP_ID, 'user_displayname');
389+
$this->appConfig->deleteAppValue('user_name');
390+
$this->appConfig->deleteAppValue('user_displayname');
389391
return [
390392
'user_name' => '',
391393
'user_displayname' => '',
@@ -394,7 +396,7 @@ public function updateLinkTokenUserInfo(string $defaultLinkToken): array {
394396
}
395397

396398
public function checkNewNotifications() {
397-
$notificationEnabled = $this->config->getAppValue(Application::APP_ID, 'issue_notifications_enabled', '1') === '1';
399+
$notificationEnabled = $this->appConfig->getAppValueString('issue_notifications_enabled', '1') === '1';
398400
if (!$notificationEnabled) {
399401
return;
400402
}

0 commit comments

Comments
 (0)