Skip to content

Commit 79b808d

Browse files
Chartman123Chartman123
authored
Merge pull request #3321 from nextcloud/chore/appconfig
refactor(config): replace IConfig with IAppConfig for improved type safety and clarity
2 parents 4a97cb2 + f765239 commit 79b808d

10 files changed

Lines changed: 125 additions & 60 deletions

‎lib/Constants.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,16 @@ class Constants {
2929
self::CONFIG_KEY_ALLOWCONFIRMATIONEMAIL,
3030
self::CONFIG_KEY_CONFIRMATIONEMAILRATELIMIT,
3131
];
32+
public const CONFIG_KEY_TYPES = [
33+
self::CONFIG_KEY_ALLOWPERMITALL => 'bool',
34+
self::CONFIG_KEY_ALLOWPUBLICLINK => 'bool',
35+
self::CONFIG_KEY_ALLOWSHOWTOALL => 'bool',
36+
self::CONFIG_KEY_RESTRICTCREATION => 'bool',
37+
self::CONFIG_KEY_ALLOWCONFIRMATIONEMAIL => 'bool',
38+
self::CONFIG_KEY_CREATIONALLOWEDGROUPS => 'array',
39+
self::CONFIG_KEY_CONFIRMATIONEMAILRATELIMIT => 'int',
40+
];
41+
3242

3343
/**
3444
* Maximum String lengths, the database is set to store.

‎lib/Controller/ConfigController.php‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
use OCP\AppFramework\Http\Attribute\FrontpageRoute;
1616
use OCP\AppFramework\Http\Attribute\OpenAPI;
1717
use OCP\AppFramework\Http\DataResponse;
18-
use OCP\IConfig;
18+
use OCP\AppFramework\Services\IAppConfig;
1919
use OCP\IRequest;
2020

2121
use Psr\Log\LoggerInterface;
@@ -25,7 +25,7 @@ class ConfigController extends ApiController {
2525
public function __construct(
2626
protected $appName,
2727
private ConfigService $configService,
28-
private IConfig $config,
28+
private IAppConfig $appConfig,
2929
private LoggerInterface $logger,
3030
IRequest $request,
3131
) {
@@ -46,11 +46,11 @@ public function getAppConfig(): DataResponse {
4646
* Admin required, thus not checking separately.
4747
*
4848
* @param string $configKey AppConfig Key to store
49-
* @param mixed $configValues Corresponding AppConfig Value
49+
* @param mixed $configValue Corresponding AppConfig Value
5050
*
5151
*/
5252
#[FrontpageRoute(verb: 'PATCH', url: '/config')]
53-
public function updateAppConfig(string $configKey, $configValue): DataResponse {
53+
public function updateAppConfig(string $configKey, mixed $configValue): DataResponse {
5454
$this->logger->debug('Updating AppConfig: {configKey} => {configValue}', [
5555
'configKey' => $configKey,
5656
'configValue' => $configValue
@@ -65,8 +65,24 @@ public function updateAppConfig(string $configKey, $configValue): DataResponse {
6565
return new DataResponse('Mail server is not configured', Http::STATUS_BAD_REQUEST);
6666
}
6767

68-
// Set on DB
69-
$this->config->setAppValue($this->appName, $configKey, json_encode($configValue));
68+
// Set on DB with typed setters
69+
try {
70+
switch (Constants::CONFIG_KEY_TYPES[$configKey]) {
71+
case 'bool':
72+
$this->appConfig->setAppValueBool($configKey, $configValue);
73+
break;
74+
75+
case 'array':
76+
$this->appConfig->setAppValueArray($configKey, $configValue);
77+
break;
78+
79+
case 'int':
80+
$this->appConfig->setAppValueInt($configKey, $configValue);
81+
break;
82+
}
83+
} catch (\InvalidArgumentException) {
84+
return new DataResponse('Invalid value for ' . $configKey, Http::STATUS_BAD_REQUEST);
85+
}
7086

7187
return new DataResponse();
7288
}

‎lib/Migration/Version0010Date20190000000007.php‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
namespace OCA\Forms\Migration;
99

1010
use OCP\DB\ISchemaWrapper;
11-
use OCP\IConfig;
1211
use OCP\IDBConnection;
1312
use OCP\Migration\IOutput;
1413
use OCP\Migration\SimpleMigrationStep;
@@ -20,15 +19,12 @@
2019
class Version0010Date20190000000007 extends SimpleMigrationStep {
2120

2221
protected IDBConnection $connection;
23-
protected IConfig $config;
2422

2523
/**
2624
* @param IDBConnection $connection
27-
* @param IConfig $config
2825
*/
29-
public function __construct(IDBConnection $connection, IConfig $config) {
26+
public function __construct(IDBConnection $connection) {
3027
$this->connection = $connection;
31-
$this->config = $config;
3228
}
3329

3430
/**

‎lib/Migration/Version010200Date20200323141300.php‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
use OCP\DB\ISchemaWrapper;
1212
use OCP\DB\QueryBuilder\IQueryBuilder;
1313
use OCP\DB\Types;
14-
use OCP\IConfig;
1514
use OCP\IDBConnection;
1615
use OCP\Migration\IOutput;
1716

@@ -24,7 +23,6 @@
2423
class Version010200Date20200323141300 extends SimpleMigrationStep {
2524

2625
protected IDBConnection $connection;
27-
protected IConfig $config;
2826

2927
/** Map of questionTypes to change */
3028
private array $questionTypeMap = [
@@ -37,11 +35,9 @@ class Version010200Date20200323141300 extends SimpleMigrationStep {
3735

3836
/**
3937
* @param IDBConnection $connection
40-
* @param IConfig $config
4138
*/
42-
public function __construct(IDBConnection $connection, IConfig $config) {
39+
public function __construct(IDBConnection $connection) {
4340
$this->connection = $connection;
44-
$this->config = $config;
4541
}
4642

4743
/**

‎lib/Service/ConfigService.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,22 +34,22 @@ public function __construct(
3434
* Load the single values, decode, have default values
3535
*/
3636
public function getAllowPermitAll(): bool {
37-
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWPERMITALL, 'true'));
37+
return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_ALLOWPERMITALL, true);
3838
}
3939
public function getAllowPublicLink(): bool {
40-
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWPUBLICLINK, 'true'));
40+
return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_ALLOWPUBLICLINK, true);
4141
}
4242
public function getAllowShowToAll() : bool {
43-
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWSHOWTOALL, 'true'));
43+
return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_ALLOWSHOWTOALL, true);
4444
}
4545
private function getUnformattedCreationAllowedGroups(): array {
46-
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_CREATIONALLOWEDGROUPS, '[]'));
46+
return $this->appConfig->getAppValueArray(Constants::CONFIG_KEY_CREATIONALLOWEDGROUPS, []);
4747
}
4848
public function getCreationAllowedGroups(): array {
4949
return $this->formatGroupsForMultiselect($this->getUnformattedCreationAllowedGroups());
5050
}
5151
public function getRestrictCreation(): bool {
52-
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_RESTRICTCREATION, 'false'));
52+
return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_RESTRICTCREATION, false);
5353
}
5454

5555
public function getAllowConfirmationEmail(): bool {

‎tests/Integration/Api/RespectAdminSettingsTest.php‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
use OCA\Forms\AppInfo\Application;
1212
use OCA\Forms\Constants;
1313
use OCA\Forms\Tests\Integration\IntegrationBase;
14-
use OCP\IConfig;
14+
use OCP\IAppConfig;
1515

1616
/**
1717
* This tests that the API respects all admin settings
@@ -224,9 +224,14 @@ public function testAllowUpdate(): void {
224224
* @dataProvider forbidUpdateAdminSettingsData
225225
*/
226226
public function testForbidUpdate(array $accessValue, array $adminConfigKeys): void {
227-
$config = \OCP\Server::get(IConfig::class);
227+
$config = \OCP\Server::get(IAppConfig::class);
228228
foreach ($adminConfigKeys as $key => $value) {
229-
$config->setAppValue(Application::APP_ID, $key, $value);
229+
if (is_array($value)) {
230+
$config->setValueArray(Application::APP_ID, $key, $value);
231+
} else {
232+
$bool = is_bool($value) ? $value : filter_var($value, FILTER_VALIDATE_BOOLEAN);
233+
$config->setValueBool(Application::APP_ID, $key, $bool);
234+
}
230235
}
231236

232237
$resp = $this->http->request(
@@ -325,7 +330,7 @@ public function testListFormsAllowed(): void {
325330
*/
326331
public function testListFormsNoAdminPermission(): void {
327332
// Disable global access
328-
\OCP\Server::get(IConfig::class)->setAppValue(Application::APP_ID, Constants::CONFIG_KEY_ALLOWPERMITALL, 'false');
333+
\OCP\Server::get(IAppConfig::class)->setValueBool(Application::APP_ID, Constants::CONFIG_KEY_ALLOWPERMITALL, false);
329334

330335
$resp = $this->http->request(
331336
'GET',

‎tests/Integration/DB/SharedFormsTest.php‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
use OCA\Forms\Constants;
1313
use OCA\Forms\Db\FormMapper;
1414
use OCA\Forms\Tests\Integration\IntegrationBase;
15-
use OCP\IConfig;
15+
use OCP\IAppConfig;
1616

1717
/**
1818
* @group DB
@@ -180,9 +180,14 @@ public function testPublicSharedForms() {
180180
* @dataProvider dataForbidPublicShowAccess
181181
*/
182182
public function testShowNoSharedFormsIfDisabled(array $configValues) {
183-
$config = \OCP\Server::get(IConfig::class);
183+
$config = \OCP\Server::get(IAppConfig::class);
184184
foreach ($configValues as $key => $value) {
185-
$config->setAppValue(Application::APP_ID, $key, json_encode($value));
185+
if (is_array($value)) {
186+
$config->setValueArray(Application::APP_ID, $key, $value);
187+
} else {
188+
$bool = is_bool($value) ? $value : filter_var($value, FILTER_VALIDATE_BOOLEAN);
189+
$config->setValueBool(Application::APP_ID, $key, $bool);
190+
}
186191
}
187192

188193
$formMapper = \OCP\Server::get(FormMapper::class);
@@ -199,8 +204,8 @@ public function testShowNoSharedFormsIfDisabled(array $configValues) {
199204
* Test that a form with public access can be accessed even if show permissions are not granted (can fill out but not see in sidebar)
200205
*/
201206
public function testAllowPublicAccessOnDeniedPublicVisibility(): void {
202-
$config = \OCP\Server::get(IConfig::class);
203-
$config->setAppValue(Application::APP_ID, Constants::CONFIG_KEY_ALLOWSHOWTOALL, json_encode(false));
207+
$config = \OCP\Server::get(IAppConfig::class);
208+
$config->setValueBool(Application::APP_ID, Constants::CONFIG_KEY_ALLOWSHOWTOALL, false);
204209

205210
$formMapper = \OCP\Server::get(FormMapper::class);
206211
$forms = $formMapper->findSharedForms('user1', filterShown: false);
@@ -216,9 +221,14 @@ public function testAllowPublicAccessOnDeniedPublicVisibility(): void {
216221
* @dataProvider dataForbidPublicAccess
217222
*/
218223
public function testShowNoSharedFormsAccessIfDisabled(array $configValues): void {
219-
$config = \OCP\Server::get(IConfig::class);
224+
$config = \OCP\Server::get(IAppConfig::class);
220225
foreach ($configValues as $key => $value) {
221-
$config->setAppValue(Application::APP_ID, $key, json_encode($value));
226+
if (is_array($value)) {
227+
$config->setValueArray(Application::APP_ID, $key, $value);
228+
} else {
229+
$bool = is_bool($value) ? $value : filter_var($value, FILTER_VALIDATE_BOOLEAN);
230+
$config->setValueBool(Application::APP_ID, $key, $bool);
231+
}
222232
}
223233

224234
$formMapper = \OCP\Server::get(FormMapper::class);

‎tests/Integration/IntegrationBase.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
use OCA\Forms\AppInfo\Application;
1111
use OCA\Forms\Constants;
1212
use OCP\DB\QueryBuilder\IQueryBuilder;
13-
use OCP\IConfig;
13+
use OCP\IAppConfig;
1414
use OCP\IDBConnection;
1515
use OCP\IUserManager;
1616
use Test\TestCase;
@@ -34,9 +34,9 @@ class IntegrationBase extends TestCase {
3434
public function setUp(): void {
3535
parent::setUp();
3636

37-
$config = \OCP\Server::get(IConfig::class);
37+
$config = \OCP\Server::get(IAppConfig::class);
3838
foreach (Constants::CONFIG_KEYS as $key) {
39-
$config->deleteAppValue(Application::APP_ID, $key);
39+
$config->deleteKey(Application::APP_ID, $key);
4040
}
4141

4242
$userManager = \OCP\Server::get(IUserManager::class);

‎tests/Unit/Controller/ConfigControllerTest.php‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111

1212
use OCA\Forms\Service\ConfigService;
1313
use OCP\AppFramework\Http\DataResponse;
14-
use OCP\IConfig;
14+
use OCP\AppFramework\Services\IAppConfig;
1515
use OCP\IRequest;
1616

1717
use PHPUnit\Framework\MockObject\MockObject;
@@ -23,15 +23,15 @@ class ConfigControllerTest extends TestCase {
2323

2424
private ConfigController $configController;
2525
private ConfigService|MockObject $configService;
26-
private IConfig|MockObject $config;
26+
private IAppConfig|MockObject $config;
2727
private LoggerInterface|MockObject $logger;
2828
private IRequest|MockObject $request;
2929

3030
public function setUp(): void {
3131
parent::setUp();
3232

3333
$this->configService = $this->createMock(ConfigService::class);
34-
$this->config = $this->createMock(IConfig::class);
34+
$this->config = $this->createMock(IAppConfig::class);
3535
$this->logger = $this->createMock(LoggerInterface::class);
3636
$this->request = $this->createMock(IRequest::class);
3737

@@ -60,18 +60,18 @@ public function testGetAppConfig() {
6060

6161
public static function dataUpdateAppConfig() {
6262
return [
63-
'booleanConfig' => [
63+
'booleanAllowPermitAll' => [
6464
'configKey' => 'allowPermitAll',
6565
'configValue' => true,
6666
'strConfig' => 'true'
6767
],
68-
'booleanConfig' => [
68+
'booleanAllowShowToAll' => [
6969
'configKey' => 'allowShowToAll',
7070
'configValue' => true,
7171
'strConfig' => 'true'
7272
],
73-
'arrayConfig' => [
74-
'configKey' => 'allowPermitAll',
73+
'arrayCreationAllowedGroups' => [
74+
'configKey' => 'creationAllowedGroups',
7575
'configValue' => [
7676
'admin',
7777
'group1'
@@ -91,9 +91,15 @@ public function testUpdateAppConfig(string $configKey, $configValue, string $str
9191
$this->logger->expects($this->once())
9292
->method('debug');
9393

94-
$this->config->expects($this->once())
95-
->method('setAppValue')
96-
->with('forms', $configKey, $strConfig);
94+
if (is_array($configValue)) {
95+
$this->config->expects($this->once())
96+
->method('setAppValueArray')
97+
->with($configKey, $configValue);
98+
} else {
99+
$this->config->expects($this->once())
100+
->method('setAppValueBool')
101+
->with($configKey, $configValue);
102+
}
97103

98104
$this->assertEquals(new DataResponse(), $this->configController->updateAppConfig($configKey, $configValue));
99105
}
@@ -103,7 +109,9 @@ public function testUpdateAppConfig_unknownKey() {
103109
->method('debug');
104110

105111
$this->config->expects($this->never())
106-
->method('setAppValue');
112+
->method('setAppValueBool');
113+
$this->config->expects($this->never())
114+
->method('setAppValueArray');
107115

108116
$this->assertEquals(new DataResponse('Unknown appConfig key: someUnknownKey', 400), $this->configController->updateAppConfig('someUnknownKey', 'storeThisValue!'));
109117
}

0 commit comments

Comments
 (0)