From 750a215074b86986e95ad5175dcbb7633b84fca6 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/Controller/ConfigController.php | 31 ++++++++--- .../Version0010Date20190000000007.php | 8 +-- .../Version010200Date20200323141300.php | 8 +-- lib/Service/ConfigService.php | 14 ++--- .../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 ++++++++++++++----- 9 files changed, 120 insertions(+), 66 deletions(-) diff --git a/lib/Controller/ConfigController.php b/lib/Controller/ConfigController.php index 3152f53b8..fcd77bcc1 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 @@ -61,8 +61,27 @@ public function updateAppConfig(string $configKey, $configValue): DataResponse { return new DataResponse('Unknown appConfig key: ' . $configKey, Http::STATUS_BAD_REQUEST); } - // Set on DB - $this->config->setAppValue($this->appName, $configKey, json_encode($configValue)); + // Set on DB with typed setters + switch ($configKey) { + case Constants::CONFIG_KEY_ALLOWPERMITALL: + case Constants::CONFIG_KEY_ALLOWPUBLICLINK: + case Constants::CONFIG_KEY_ALLOWSHOWTOALL: + case Constants::CONFIG_KEY_RESTRICTCREATION: + try { + $this->appConfig->setAppValueBool($configKey, $configValue); + } catch (\InvalidArgumentException $e) { + return new DataResponse('Invalid value for ' . $configKey, Http::STATUS_BAD_REQUEST); + } + break; + + case Constants::CONFIG_KEY_CREATIONALLOWEDGROUPS: + try { + $this->appConfig->setAppValueArray($configKey, $configValue); + } catch (\InvalidArgumentException $e) { + return new DataResponse('Invalid value for ' . $configKey, Http::STATUS_BAD_REQUEST); + } + break; + } return new DataResponse(); } diff --git a/lib/Migration/Version0010Date20190000000007.php b/lib/Migration/Version0010Date20190000000007.php index 50768eba7..de00a2fe3 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; @@ -22,16 +21,11 @@ class Version0010Date20190000000007 extends SimpleMigrationStep { /** @var IDBConnection */ protected $connection; - /** @var IConfig */ - protected $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 3d4200353..dead4301f 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; @@ -26,9 +25,6 @@ class Version010200Date20200323141300 extends SimpleMigrationStep { /** @var IDBConnection */ protected $connection; - /** @var IConfig */ - protected $config; - /** Map of questionTypes to change */ private $questionTypeMap = [ 'radiogroup' => 'multiple_unique', @@ -40,11 +36,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 593315c9e..568e128aa 100644 --- a/lib/Service/ConfigService.php +++ b/lib/Service/ConfigService.php @@ -10,7 +10,7 @@ use OCA\Forms\Constants; -use OCP\IConfig; +use OCP\AppFramework\Services\IAppConfig; use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; @@ -21,7 +21,7 @@ class ConfigService { public function __construct( protected string $appName, - private IConfig $config, + private IAppConfig $appConfig, private IGroupManager $groupManager, IUserSession $userSession, ) { @@ -32,22 +32,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); } /** diff --git a/tests/Integration/Api/RespectAdminSettingsTest.php b/tests/Integration/Api/RespectAdminSettingsTest.php index 09d8ff7d1..0bcc76de2 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 @@ -221,9 +221,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( @@ -322,7 +327,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 7b2fc271d..ce1700edf 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; @@ -35,9 +35,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 7e5020c6a..d6308006c 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; @@ -27,7 +27,7 @@ class ConfigControllerTest extends TestCase { /** @var ConfigService */ private $configService; - /** @var IConfig|MockObject */ + /** @var IAppConfig|MockObject */ private $config; /** @var LoggerInterface|MockObject */ @@ -40,7 +40,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); @@ -69,18 +69,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' @@ -100,9 +100,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)); } @@ -112,7 +118,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 d0ff7a420..d0d0eabb8 100644 --- a/tests/Unit/Service/ConfigServiceTest.php +++ b/tests/Unit/Service/ConfigServiceTest.php @@ -9,7 +9,7 @@ use OCA\Forms\Service\ConfigService; -use OCP\IConfig; +use OCP\AppFramework\Services\IAppConfig; use OCP\IGroup; use OCP\IGroupManager; use OCP\IUser; @@ -24,7 +24,7 @@ class ConfigServiceTest extends TestCase { /** @var ConfigService */ private $configService; - /** @var IConfig|MockObject */ + /** @var IAppConfig|MockObject */ private $config; /** @var IGroupManager|MockObject */ @@ -36,7 +36,7 @@ class ConfigServiceTest extends TestCase { public function setUp(): void { parent::setUp(); - $this->config = $this->createMock(IConfig::class); + $this->config = $this->createMock(IAppConfig::class); $this->groupManager = $this->createMock(IGroupManager::class); $userSession = $this->createMock(IUserSession::class); @@ -99,11 +99,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. + // Mock typed getters on IAppConfig $this->config->expects($this->any()) - ->method('getAppValue') - ->will($this->returnCallback(function ($appName, $configKey, $defaultVal) use ($strConfig) { - return $strConfig[$configKey]; + ->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->config->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); })); @@ -152,8 +164,14 @@ 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) { + ->method('getAppValueBool') + ->will($this->returnCallback(function ($configKey, $defaultVal) { + return $defaultVal; + })); + + $this->config->expects($this->any()) + ->method('getAppValueArray') + ->will($this->returnCallback(function ($configKey, $defaultVal) { return $defaultVal; })); @@ -164,20 +182,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 @@ -193,11 +211,17 @@ public static function dataCanCreateForms() { */ public function testCanCreateForms(array $config, bool $expected) { $this->config->expects($this->any()) - ->method('getAppValue') - ->will($this->returnCallback(function ($appName, $configKey, $defaultVal) use ($config) { + ->method('getAppValueBool') + ->will($this->returnCallback(function ($configKey, $defaultVal) use ($config) { return $config[$configKey]; })); + $this->config->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)