From f76523933be601c45a71b91934437cdcacaeca45 Mon Sep 17 00:00:00 2001 From: Christian Hartmann Date: Sun, 3 May 2026 00:05:02 +0200 Subject: [PATCH] refactor(config): replace IConfig with IAppConfig for improved type safety and clarity Signed-off-by: Christian Hartmann --- lib/Constants.php | 10 ++++ lib/Controller/ConfigController.php | 28 +++++++--- .../Version0010Date20190000000007.php | 6 +-- .../Version010200Date20200323141300.php | 6 +-- lib/Service/ConfigService.php | 10 ++-- .../Api/RespectAdminSettingsTest.php | 13 +++-- tests/Integration/DB/SharedFormsTest.php | 24 ++++++--- tests/Integration/IntegrationBase.php | 6 +-- .../Unit/Controller/ConfigControllerTest.php | 30 +++++++---- tests/Unit/Service/ConfigServiceTest.php | 52 ++++++++++++++----- 10 files changed, 125 insertions(+), 60 deletions(-) diff --git a/lib/Constants.php b/lib/Constants.php index fa0c347db..d17c3c934 100644 --- a/lib/Constants.php +++ b/lib/Constants.php @@ -29,6 +29,16 @@ class Constants { self::CONFIG_KEY_ALLOWCONFIRMATIONEMAIL, self::CONFIG_KEY_CONFIRMATIONEMAILRATELIMIT, ]; + public const CONFIG_KEY_TYPES = [ + self::CONFIG_KEY_ALLOWPERMITALL => 'bool', + self::CONFIG_KEY_ALLOWPUBLICLINK => 'bool', + self::CONFIG_KEY_ALLOWSHOWTOALL => 'bool', + self::CONFIG_KEY_RESTRICTCREATION => 'bool', + self::CONFIG_KEY_ALLOWCONFIRMATIONEMAIL => 'bool', + self::CONFIG_KEY_CREATIONALLOWEDGROUPS => 'array', + self::CONFIG_KEY_CONFIRMATIONEMAILRATELIMIT => 'int', + ]; + /** * Maximum String lengths, the database is set to store. diff --git a/lib/Controller/ConfigController.php b/lib/Controller/ConfigController.php index 5b9393c72..a4db73d36 100644 --- a/lib/Controller/ConfigController.php +++ b/lib/Controller/ConfigController.php @@ -15,7 +15,7 @@ use OCP\AppFramework\Http\Attribute\FrontpageRoute; use OCP\AppFramework\Http\Attribute\OpenAPI; use OCP\AppFramework\Http\DataResponse; -use OCP\IConfig; +use OCP\AppFramework\Services\IAppConfig; use OCP\IRequest; use Psr\Log\LoggerInterface; @@ -25,7 +25,7 @@ class ConfigController extends ApiController { public function __construct( protected $appName, private ConfigService $configService, - private IConfig $config, + private IAppConfig $appConfig, private LoggerInterface $logger, IRequest $request, ) { @@ -46,11 +46,11 @@ public function getAppConfig(): DataResponse { * Admin required, thus not checking separately. * * @param string $configKey AppConfig Key to store - * @param mixed $configValues Corresponding AppConfig Value + * @param mixed $configValue Corresponding AppConfig Value * */ #[FrontpageRoute(verb: 'PATCH', url: '/config')] - public function updateAppConfig(string $configKey, $configValue): DataResponse { + public function updateAppConfig(string $configKey, mixed $configValue): DataResponse { $this->logger->debug('Updating AppConfig: {configKey} => {configValue}', [ 'configKey' => $configKey, 'configValue' => $configValue @@ -65,8 +65,24 @@ public function updateAppConfig(string $configKey, $configValue): DataResponse { return new DataResponse('Mail server is not configured', Http::STATUS_BAD_REQUEST); } - // Set on DB - $this->config->setAppValue($this->appName, $configKey, json_encode($configValue)); + // Set on DB with typed setters + try { + switch (Constants::CONFIG_KEY_TYPES[$configKey]) { + case 'bool': + $this->appConfig->setAppValueBool($configKey, $configValue); + break; + + case 'array': + $this->appConfig->setAppValueArray($configKey, $configValue); + break; + + case 'int': + $this->appConfig->setAppValueInt($configKey, $configValue); + break; + } + } catch (\InvalidArgumentException) { + return new DataResponse('Invalid value for ' . $configKey, Http::STATUS_BAD_REQUEST); + } return new DataResponse(); } diff --git a/lib/Migration/Version0010Date20190000000007.php b/lib/Migration/Version0010Date20190000000007.php index 52b57eec0..286aa490d 100644 --- a/lib/Migration/Version0010Date20190000000007.php +++ b/lib/Migration/Version0010Date20190000000007.php @@ -8,7 +8,6 @@ namespace OCA\Forms\Migration; use OCP\DB\ISchemaWrapper; -use OCP\IConfig; use OCP\IDBConnection; use OCP\Migration\IOutput; use OCP\Migration\SimpleMigrationStep; @@ -20,15 +19,12 @@ class Version0010Date20190000000007 extends SimpleMigrationStep { protected IDBConnection $connection; - protected IConfig $config; /** * @param IDBConnection $connection - * @param IConfig $config */ - public function __construct(IDBConnection $connection, IConfig $config) { + public function __construct(IDBConnection $connection) { $this->connection = $connection; - $this->config = $config; } /** diff --git a/lib/Migration/Version010200Date20200323141300.php b/lib/Migration/Version010200Date20200323141300.php index ecb13cd0d..746a57bdf 100644 --- a/lib/Migration/Version010200Date20200323141300.php +++ b/lib/Migration/Version010200Date20200323141300.php @@ -11,7 +11,6 @@ use OCP\DB\ISchemaWrapper; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\DB\Types; -use OCP\IConfig; use OCP\IDBConnection; use OCP\Migration\IOutput; @@ -24,7 +23,6 @@ class Version010200Date20200323141300 extends SimpleMigrationStep { protected IDBConnection $connection; - protected IConfig $config; /** Map of questionTypes to change */ private array $questionTypeMap = [ @@ -37,11 +35,9 @@ class Version010200Date20200323141300 extends SimpleMigrationStep { /** * @param IDBConnection $connection - * @param IConfig $config */ - public function __construct(IDBConnection $connection, IConfig $config) { + public function __construct(IDBConnection $connection) { $this->connection = $connection; - $this->config = $config; } /** diff --git a/lib/Service/ConfigService.php b/lib/Service/ConfigService.php index 9bb6a19b8..ac120eebf 100644 --- a/lib/Service/ConfigService.php +++ b/lib/Service/ConfigService.php @@ -34,22 +34,22 @@ public function __construct( * Load the single values, decode, have default values */ public function getAllowPermitAll(): bool { - return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWPERMITALL, 'true')); + return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_ALLOWPERMITALL, true); } public function getAllowPublicLink(): bool { - return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWPUBLICLINK, 'true')); + return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_ALLOWPUBLICLINK, true); } public function getAllowShowToAll() : bool { - return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWSHOWTOALL, 'true')); + return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_ALLOWSHOWTOALL, true); } private function getUnformattedCreationAllowedGroups(): array { - return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_CREATIONALLOWEDGROUPS, '[]')); + return $this->appConfig->getAppValueArray(Constants::CONFIG_KEY_CREATIONALLOWEDGROUPS, []); } public function getCreationAllowedGroups(): array { return $this->formatGroupsForMultiselect($this->getUnformattedCreationAllowedGroups()); } public function getRestrictCreation(): bool { - return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_RESTRICTCREATION, 'false')); + return $this->appConfig->getAppValueBool(Constants::CONFIG_KEY_RESTRICTCREATION, false); } public function getAllowConfirmationEmail(): bool { diff --git a/tests/Integration/Api/RespectAdminSettingsTest.php b/tests/Integration/Api/RespectAdminSettingsTest.php index 14937c86b..2c8b8dbd1 100644 --- a/tests/Integration/Api/RespectAdminSettingsTest.php +++ b/tests/Integration/Api/RespectAdminSettingsTest.php @@ -11,7 +11,7 @@ use OCA\Forms\AppInfo\Application; use OCA\Forms\Constants; use OCA\Forms\Tests\Integration\IntegrationBase; -use OCP\IConfig; +use OCP\IAppConfig; /** * This tests that the API respects all admin settings @@ -224,9 +224,14 @@ public function testAllowUpdate(): void { * @dataProvider forbidUpdateAdminSettingsData */ public function testForbidUpdate(array $accessValue, array $adminConfigKeys): void { - $config = \OCP\Server::get(IConfig::class); + $config = \OCP\Server::get(IAppConfig::class); foreach ($adminConfigKeys as $key => $value) { - $config->setAppValue(Application::APP_ID, $key, $value); + if (is_array($value)) { + $config->setValueArray(Application::APP_ID, $key, $value); + } else { + $bool = is_bool($value) ? $value : filter_var($value, FILTER_VALIDATE_BOOLEAN); + $config->setValueBool(Application::APP_ID, $key, $bool); + } } $resp = $this->http->request( @@ -325,7 +330,7 @@ public function testListFormsAllowed(): void { */ public function testListFormsNoAdminPermission(): void { // Disable global access - \OCP\Server::get(IConfig::class)->setAppValue(Application::APP_ID, Constants::CONFIG_KEY_ALLOWPERMITALL, 'false'); + \OCP\Server::get(IAppConfig::class)->setValueBool(Application::APP_ID, Constants::CONFIG_KEY_ALLOWPERMITALL, false); $resp = $this->http->request( 'GET', diff --git a/tests/Integration/DB/SharedFormsTest.php b/tests/Integration/DB/SharedFormsTest.php index 29bdb9f8f..30ee9fb2a 100644 --- a/tests/Integration/DB/SharedFormsTest.php +++ b/tests/Integration/DB/SharedFormsTest.php @@ -12,7 +12,7 @@ use OCA\Forms\Constants; use OCA\Forms\Db\FormMapper; use OCA\Forms\Tests\Integration\IntegrationBase; -use OCP\IConfig; +use OCP\IAppConfig; /** * @group DB @@ -180,9 +180,14 @@ public function testPublicSharedForms() { * @dataProvider dataForbidPublicShowAccess */ public function testShowNoSharedFormsIfDisabled(array $configValues) { - $config = \OCP\Server::get(IConfig::class); + $config = \OCP\Server::get(IAppConfig::class); foreach ($configValues as $key => $value) { - $config->setAppValue(Application::APP_ID, $key, json_encode($value)); + if (is_array($value)) { + $config->setValueArray(Application::APP_ID, $key, $value); + } else { + $bool = is_bool($value) ? $value : filter_var($value, FILTER_VALIDATE_BOOLEAN); + $config->setValueBool(Application::APP_ID, $key, $bool); + } } $formMapper = \OCP\Server::get(FormMapper::class); @@ -199,8 +204,8 @@ public function testShowNoSharedFormsIfDisabled(array $configValues) { * 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) */ public function testAllowPublicAccessOnDeniedPublicVisibility(): void { - $config = \OCP\Server::get(IConfig::class); - $config->setAppValue(Application::APP_ID, Constants::CONFIG_KEY_ALLOWSHOWTOALL, json_encode(false)); + $config = \OCP\Server::get(IAppConfig::class); + $config->setValueBool(Application::APP_ID, Constants::CONFIG_KEY_ALLOWSHOWTOALL, false); $formMapper = \OCP\Server::get(FormMapper::class); $forms = $formMapper->findSharedForms('user1', filterShown: false); @@ -216,9 +221,14 @@ public function testAllowPublicAccessOnDeniedPublicVisibility(): void { * @dataProvider dataForbidPublicAccess */ public function testShowNoSharedFormsAccessIfDisabled(array $configValues): void { - $config = \OCP\Server::get(IConfig::class); + $config = \OCP\Server::get(IAppConfig::class); foreach ($configValues as $key => $value) { - $config->setAppValue(Application::APP_ID, $key, json_encode($value)); + if (is_array($value)) { + $config->setValueArray(Application::APP_ID, $key, $value); + } else { + $bool = is_bool($value) ? $value : filter_var($value, FILTER_VALIDATE_BOOLEAN); + $config->setValueBool(Application::APP_ID, $key, $bool); + } } $formMapper = \OCP\Server::get(FormMapper::class); diff --git a/tests/Integration/IntegrationBase.php b/tests/Integration/IntegrationBase.php index fc3b22401..ce2886759 100644 --- a/tests/Integration/IntegrationBase.php +++ b/tests/Integration/IntegrationBase.php @@ -10,7 +10,7 @@ use OCA\Forms\AppInfo\Application; use OCA\Forms\Constants; use OCP\DB\QueryBuilder\IQueryBuilder; -use OCP\IConfig; +use OCP\IAppConfig; use OCP\IDBConnection; use OCP\IUserManager; use Test\TestCase; @@ -34,9 +34,9 @@ class IntegrationBase extends TestCase { public function setUp(): void { parent::setUp(); - $config = \OCP\Server::get(IConfig::class); + $config = \OCP\Server::get(IAppConfig::class); foreach (Constants::CONFIG_KEYS as $key) { - $config->deleteAppValue(Application::APP_ID, $key); + $config->deleteKey(Application::APP_ID, $key); } $userManager = \OCP\Server::get(IUserManager::class); diff --git a/tests/Unit/Controller/ConfigControllerTest.php b/tests/Unit/Controller/ConfigControllerTest.php index 7ad170b36..2ea1d9456 100644 --- a/tests/Unit/Controller/ConfigControllerTest.php +++ b/tests/Unit/Controller/ConfigControllerTest.php @@ -11,7 +11,7 @@ use OCA\Forms\Service\ConfigService; use OCP\AppFramework\Http\DataResponse; -use OCP\IConfig; +use OCP\AppFramework\Services\IAppConfig; use OCP\IRequest; use PHPUnit\Framework\MockObject\MockObject; @@ -23,7 +23,7 @@ class ConfigControllerTest extends TestCase { private ConfigController $configController; private ConfigService|MockObject $configService; - private IConfig|MockObject $config; + private IAppConfig|MockObject $config; private LoggerInterface|MockObject $logger; private IRequest|MockObject $request; @@ -31,7 +31,7 @@ public function setUp(): void { parent::setUp(); $this->configService = $this->createMock(ConfigService::class); - $this->config = $this->createMock(IConfig::class); + $this->config = $this->createMock(IAppConfig::class); $this->logger = $this->createMock(LoggerInterface::class); $this->request = $this->createMock(IRequest::class); @@ -60,18 +60,18 @@ public function testGetAppConfig() { public static function dataUpdateAppConfig() { return [ - 'booleanConfig' => [ + 'booleanAllowPermitAll' => [ 'configKey' => 'allowPermitAll', 'configValue' => true, 'strConfig' => 'true' ], - 'booleanConfig' => [ + 'booleanAllowShowToAll' => [ 'configKey' => 'allowShowToAll', 'configValue' => true, 'strConfig' => 'true' ], - 'arrayConfig' => [ - 'configKey' => 'allowPermitAll', + 'arrayCreationAllowedGroups' => [ + 'configKey' => 'creationAllowedGroups', 'configValue' => [ 'admin', 'group1' @@ -91,9 +91,15 @@ public function testUpdateAppConfig(string $configKey, $configValue, string $str $this->logger->expects($this->once()) ->method('debug'); - $this->config->expects($this->once()) - ->method('setAppValue') - ->with('forms', $configKey, $strConfig); + if (is_array($configValue)) { + $this->config->expects($this->once()) + ->method('setAppValueArray') + ->with($configKey, $configValue); + } else { + $this->config->expects($this->once()) + ->method('setAppValueBool') + ->with($configKey, $configValue); + } $this->assertEquals(new DataResponse(), $this->configController->updateAppConfig($configKey, $configValue)); } @@ -103,7 +109,9 @@ public function testUpdateAppConfig_unknownKey() { ->method('debug'); $this->config->expects($this->never()) - ->method('setAppValue'); + ->method('setAppValueBool'); + $this->config->expects($this->never()) + ->method('setAppValueArray'); $this->assertEquals(new DataResponse('Unknown appConfig key: someUnknownKey', 400), $this->configController->updateAppConfig('someUnknownKey', 'storeThisValue!')); } diff --git a/tests/Unit/Service/ConfigServiceTest.php b/tests/Unit/Service/ConfigServiceTest.php index 713052d97..b47743185 100644 --- a/tests/Unit/Service/ConfigServiceTest.php +++ b/tests/Unit/Service/ConfigServiceTest.php @@ -100,11 +100,23 @@ public static function dataGetAppConfig() { * @param array $expected */ public function testGetAppConfig(array $strConfig, array $groupDisplayNames, array $expected) { - // Default Values are set within getAppValue, thus returning this one. - $this->config->expects($this->any()) - ->method('getAppValue') - ->will($this->returnCallback(function ($appName, $configKey, $defaultVal) use ($strConfig) { - return $strConfig[$configKey] ?? $defaultVal; + // Mock typed getters on IAppConfig + $this->appConfig->expects($this->any()) + ->method('getAppValueBool') + ->will($this->returnCallback(function ($configKey, $defaultVal) use ($strConfig) { + if (!array_key_exists($configKey, $strConfig)) { + return $defaultVal; + } + return filter_var($strConfig[$configKey], FILTER_VALIDATE_BOOLEAN); + })); + + $this->appConfig->expects($this->any()) + ->method('getAppValueArray') + ->will($this->returnCallback(function ($configKey, $defaultVal) use ($strConfig) { + if (!array_key_exists($configKey, $strConfig)) { + return $defaultVal; + } + return json_decode($strConfig[$configKey], true); })); $this->config->expects($this->any()) @@ -166,9 +178,15 @@ public static function dataGetAppConfig_Default() { */ public function testGetAppConfig_Default(array $expected) { // Default Values are set within getAppValue, thus returning this one. - $this->config->expects($this->any()) - ->method('getAppValue') - ->will($this->returnCallback(function ($appName, $configKey, $defaultVal) { + $this->appConfig->expects($this->any()) + ->method('getAppValueBool') + ->will($this->returnCallback(function ($configKey, $defaultVal) { + return $defaultVal; + })); + + $this->appConfig->expects($this->any()) + ->method('getAppValueArray') + ->will($this->returnCallback(function ($configKey, $defaultVal) { return $defaultVal; })); @@ -191,20 +209,20 @@ public static function dataCanCreateForms() { return [ 'notRestriced' => [ 'config' => [ - 'restrictCreation' => 'false', + 'restrictCreation' => false, ], 'expected' => true ], 'restrictedGroupAllowed' => [ 'config' => [ - 'restrictCreation' => 'true', + 'restrictCreation' => true, 'creationAllowedGroups' => '["usersGroup","notUsersGroup"]' ], 'expected' => true ], 'restrictedNotInGroup' => [ 'config' => [ - 'restrictCreation' => 'true', + 'restrictCreation' => true, 'creationAllowedGroups' => '["notUsersGroup"]' ], 'expected' => false @@ -219,12 +237,18 @@ public static function dataCanCreateForms() { * @param bool $expected */ public function testCanCreateForms(array $config, bool $expected) { - $this->config->expects($this->any()) - ->method('getAppValue') - ->will($this->returnCallback(function ($appName, $configKey, $defaultVal) use ($config) { + $this->appConfig->expects($this->any()) + ->method('getAppValueBool') + ->will($this->returnCallback(function ($configKey, $defaultVal) use ($config) { return $config[$configKey]; })); + $this->appConfig->expects($this->any()) + ->method('getAppValueArray') + ->will($this->returnCallback(function ($configKey, $defaultVal) use ($config) { + return isset($config[$configKey]) ? json_decode($config[$configKey], true) : $defaultVal; + })); + $this->groupManager->expects($this->any()) ->method('getUserGroupIds') ->with($this->currentUser)