refactor(config): replace IConfig with IAppConfig for improved type safety and clarity

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 @@
 	public function __construct(
 		protected $appName,
 		private ConfigService $configService,
-		private IConfig $config,
+		private IAppConfig $appConfig,
 		private LoggerInterface $logger,
 		IRequest $request,
 	) {
@@ -46,11 +46,11 @@
 	 * 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,25 @@
 			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 (err) {
+					return new DataResponse('Invalid value for ' . $configKey, Http::STATUS_BAD_REQUEST);
+				}
+
+			case Constants::CONFIG_KEY_CREATIONALLOWEDGROUPS:
+				try {
+					$this->appConfig->setAppValueArray($configKey, $configValue);
+				} catch (err) {
+					return new DataResponse('Invalid value for ' . $configKey, Http::STATUS_BAD_REQUEST);
+				}
+		}
 
 		return new DataResponse();
 	}

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;
 	}
 
 	/**

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;
 	}
 
 	/**

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);
 	}
 
 	/**

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\AppFramework\Services\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->setAppValueArray($key, $value);
+			} else {
+				$bool = is_bool($value) ? $value : filter_var($value, FILTER_VALIDATE_BOOLEAN);
+				$config->setAppValueBool($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)->setAppValueBool(Constants::CONFIG_KEY_ALLOWPERMITALL, false);
 
 		$resp = $this->http->request(
 			'GET',

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\AppFramework\Services\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->setAppValueArray($key, $value);
+			} else {
+				$bool = is_bool($value) ? $value : filter_var($value, FILTER_VALIDATE_BOOLEAN);
+				$config->setAppValueBool($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->setAppValueBool(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->setAppValueArray($key, $value);
+			} else {
+				$bool = is_bool($value) ? $value : filter_var($value, FILTER_VALIDATE_BOOLEAN);
+				$config->setAppValueBool($key, $bool);
+			}
 		}
 
 		$formMapper = \OCP\Server::get(FormMapper::class);

tests/Integration/IntegrationBase.php

@@ -9,8 +9,8 @@
 
 use OCA\Forms\AppInfo\Application;
 use OCA\Forms\Constants;
+use OCP\AppFramework\Services\IAppConfig;
 use OCP\DB\QueryBuilder\IQueryBuilder;
-use OCP\IConfig;
 use OCP\IDBConnection;
 use OCP\IUserManager;
 use Test\TestCase;
@@ -19,8 +19,7 @@
  * @group DB
  */
 class IntegrationBase extends TestCase {
-	/** @var Array */
-	protected $testForms;
+	protected array $testForms;
 
 	/**
 	 * Users that are needed by this test case
@@ -35,9 +34,9 @@ class IntegrationBase extends TestCase {
 	public function setUp(): void {
 		parent::setUp();
 
-		$config = \OCP\Server::get(IConfig::class);
+		$appConfig = \OCP\AppFramework\Services::get(IAppConfig::class);
 		foreach (Constants::CONFIG_KEYS as $key) {
-			$config->deleteAppValue(Application::APP_ID, $key);
+			$appConfig->deleteAppValue($key);
 		}
 
 		$userManager = \OCP\Server::get(IUserManager::class);

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!'));
 	}