From 1c85548f8635284b255dc6e0743e99cc408342e8 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 12 Aug 2025 14:57:07 +0200 Subject: [PATCH 1/7] feat(AppConfig): cache app config in local cache if available Signed-off-by: Ferdinand Thiessen --- build/psalm-baseline.xml | 5 - config/config.sample.php | 9 + lib/private/AllConfig.php | 2 +- lib/private/AppConfig.php | 171 +++++++++--------- ...gTest.php => AppConfigIntegrationTest.php} | 10 +- 5 files changed, 105 insertions(+), 92 deletions(-) rename tests/lib/{AppConfigTest.php => AppConfigIntegrationTest.php} (99%) diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 07df0637d57f4..114dc479f1ab5 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -3314,11 +3314,6 @@ - - - fastCache[$app][$key] ?? $default]]> - - diff --git a/config/config.sample.php b/config/config.sample.php index 37871669309e9..ea8766b970b7b 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -1792,6 +1792,15 @@ */ 'cache_chunk_gc_ttl' => 60*60*24, +/** + * Enable caching of the app config values. + * If enabled the app config will be cached locally for a short TTL, + * reducing database load significatly on larger setups. + * + * Defaults to ``true`` + */ +'cache_app_config' => true, + /** * Using Object Store with Nextcloud */ diff --git a/lib/private/AllConfig.php b/lib/private/AllConfig.php index c80ee52eb0dc6..c4a6989df1317 100644 --- a/lib/private/AllConfig.php +++ b/lib/private/AllConfig.php @@ -195,7 +195,7 @@ public function setAppValue($appName, $key, $value) { * @deprecated 29.0.0 Use {@see IAppConfig} directly */ public function getAppValue($appName, $key, $default = '') { - return \OC::$server->get(AppConfig::class)->getValue($appName, $key, $default); + return \OC::$server->get(AppConfig::class)->getValue($appName, $key, $default) ?? $default; } /** diff --git a/lib/private/AppConfig.php b/lib/private/AppConfig.php index cef612536d620..a245f3149d239 100644 --- a/lib/private/AppConfig.php +++ b/lib/private/AppConfig.php @@ -15,7 +15,6 @@ use OC\Config\ConfigManager; use OC\Config\PresetManager; use OCP\Config\Lexicon\Entry; -use OCP\Config\Lexicon\ILexicon; use OCP\Config\Lexicon\Strictness; use OCP\Config\ValueType; use OCP\DB\Exception as DBException; @@ -24,6 +23,8 @@ use OCP\Exceptions\AppConfigTypeConflictException; use OCP\Exceptions\AppConfigUnknownKeyException; use OCP\IAppConfig; +use OCP\ICache; +use OCP\ICacheFactory; use OCP\IConfig; use OCP\IDBConnection; use OCP\Security\ICrypto; @@ -53,10 +54,12 @@ class AppConfig implements IAppConfig { private const KEY_MAX_LENGTH = 64; private const ENCRYPTION_PREFIX = '$AppConfigEncryption$'; private const ENCRYPTION_PREFIX_LENGTH = 21; // strlen(self::ENCRYPTION_PREFIX) + private const LOCAL_CACHE_KEY = 'OC\\AppConfig'; + private const LOCAL_CACHE_TTL = 3; - /** @var array> ['app_id' => ['config_key' => 'config_value']] */ + /** @var array> ['app_id' => ['config_key' => 'config_value']] */ private array $fastCache = []; // cache for normal config keys - /** @var array> ['app_id' => ['config_key' => 'config_value']] */ + /** @var array> ['app_id' => ['config_key' => 'config_value']] */ private array $lazyCache = []; // cache for lazy config keys /** @var array> ['app_id' => ['config_key' => bitflag]] */ private array $valueTypes = []; // type for all config values @@ -67,6 +70,7 @@ class AppConfig implements IAppConfig { private bool $ignoreLexiconAliases = false; /** @var ?array */ private ?array $appVersionsCache = null; + private ?ICache $localCache = null; public function __construct( protected IDBConnection $connection, @@ -75,7 +79,11 @@ public function __construct( private readonly PresetManager $presetManager, protected LoggerInterface $logger, protected ICrypto $crypto, + readonly ICacheFactory $cacheFactory, ) { + if ($config->getSystemValueBool('cache_app_config', true) && $cacheFactory->isLocalCacheAvailable()) { + $this->localCache = $cacheFactory->createLocal(); + } } /** @@ -85,7 +93,7 @@ public function __construct( * @since 7.0.0 */ public function getApps(): array { - $this->loadConfigAll(); + $this->loadConfig(lazy: true); $apps = array_merge(array_keys($this->fastCache), array_keys($this->lazyCache)); sort($apps); @@ -103,7 +111,7 @@ public function getApps(): array { */ public function getKeys(string $app): array { $this->assertParams($app); - $this->loadConfigAll($app); + $this->loadConfig($app, true); $keys = array_merge(array_keys($this->fastCache[$app] ?? []), array_keys($this->lazyCache[$app] ?? [])); sort($keys); @@ -149,19 +157,16 @@ public function searchKeys(string $app, string $prefix = '', bool $lazy = false) */ public function hasKey(string $app, string $key, ?bool $lazy = false): bool { $this->assertParams($app, $key); - $this->loadConfig($app, $lazy); + $this->loadConfig($app, $lazy ?? true); $this->matchAndApplyLexiconDefinition($app, $key); + $hasLazy = isset($this->lazyCache[$app][$key]); + $hasFast = isset($this->fastCache[$app][$key]); if ($lazy === null) { - $appCache = $this->getAllValues($app); - return isset($appCache[$key]); - } - - if ($lazy) { - return isset($this->lazyCache[$app][$key]); + return $hasLazy || $hasFast; + } else { + return $lazy ? $hasLazy : $hasFast; } - - return isset($this->fastCache[$app][$key]); } /** @@ -175,7 +180,7 @@ public function hasKey(string $app, string $key, ?bool $lazy = false): bool { */ public function isSensitive(string $app, string $key, ?bool $lazy = false): bool { $this->assertParams($app, $key); - $this->loadConfig(null, $lazy); + $this->loadConfig(null, $lazy ?? true); $this->matchAndApplyLexiconDefinition($app, $key); if (!isset($this->valueTypes[$app][$key])) { @@ -227,7 +232,7 @@ public function isLazy(string $app, string $key): bool { public function getAllValues(string $app, string $prefix = '', bool $filtered = false): array { $this->assertParams($app, $prefix); // if we want to filter values, we need to get sensitivity - $this->loadConfigAll($app); + $this->loadConfig($app, true); // array_merge() will remove numeric keys (here config keys), so addition arrays instead $values = $this->formatAppValues($app, ($this->fastCache[$app] ?? []) + ($this->lazyCache[$app] ?? [])); $values = array_filter( @@ -479,7 +484,7 @@ private function getTypedValue( return $default; } - $this->loadConfig($app, $lazy); + $this->loadConfig($app, $lazy ?? true); /** * We ignore check if mixed type is requested. @@ -551,7 +556,7 @@ public function getValueType(string $app, string $key, ?bool $lazy = null): int } $this->assertParams($app, $key); - $this->loadConfig($app, $lazy); + $this->loadConfig($app, $lazy ?? true); if (!isset($this->valueTypes[$app][$key])) { throw new AppConfigUnknownKeyException('unknown config key'); @@ -788,7 +793,7 @@ private function setTypedValue( if (!$this->matchAndApplyLexiconDefinition($app, $key, $lazy, $type)) { return false; // returns false as database is not updated } - $this->loadConfig(null, $lazy); + $this->loadConfig(null, $lazy ?? true); $sensitive = $this->isTyped(self::VALUE_SENSITIVE, $type); $inserted = $refreshCache = false; @@ -803,7 +808,7 @@ private function setTypedValue( * no update if key is already known with set lazy status and value is * not different, unless sensitivity is switched from false to true. */ - if ($origValue === $this->getTypedValue($app, $key, $value, $lazy, $type) + if ($origValue === $this->getTypedValue($app, $key, $value, $lazy ?? true, $type) && (!$sensitive || $this->isSensitive($app, $key, $lazy))) { return false; } @@ -835,7 +840,7 @@ private function setTypedValue( if (!$inserted) { $currType = $this->valueTypes[$app][$key] ?? 0; if ($currType === 0) { // this might happen when switching lazy loading status - $this->loadConfigAll(); + $this->loadConfig(lazy: true); $currType = $this->valueTypes[$app][$key] ?? 0; } @@ -856,7 +861,7 @@ private function setTypedValue( && ($type | self::VALUE_SENSITIVE) !== ($currType | self::VALUE_SENSITIVE)) { try { $currType = $this->convertTypeToString($currType); - $type = $this->convertTypeToString($type); + $this->convertTypeToString($type); } catch (AppConfigIncorrectTypeException) { // can be ignored, this was just needed for a better exception message. } @@ -895,6 +900,7 @@ private function setTypedValue( $this->fastCache[$app][$key] = $value; } $this->valueTypes[$app][$key] = $type; + $this->clearLocalCache(); return true; } @@ -916,7 +922,7 @@ private function setTypedValue( */ public function updateType(string $app, string $key, int $type = self::VALUE_MIXED): bool { $this->assertParams($app, $key); - $this->loadConfigAll(); + $this->loadConfig(lazy: true); $this->matchAndApplyLexiconDefinition($app, $key); $this->isLazy($app, $key); // confirm key exists @@ -959,7 +965,7 @@ public function updateType(string $app, string $key, int $type = self::VALUE_MIX */ public function updateSensitive(string $app, string $key, bool $sensitive): bool { $this->assertParams($app, $key); - $this->loadConfigAll(); + $this->loadConfig(lazy: true); $this->matchAndApplyLexiconDefinition($app, $key); try { @@ -1019,7 +1025,7 @@ public function updateSensitive(string $app, string $key, bool $sensitive): bool */ public function updateLazy(string $app, string $key, bool $lazy): bool { $this->assertParams($app, $key); - $this->loadConfigAll(); + $this->loadConfig(lazy: true); $this->matchAndApplyLexiconDefinition($app, $key); try { @@ -1055,7 +1061,7 @@ public function updateLazy(string $app, string $key, bool $lazy): bool { */ public function getDetails(string $app, string $key): array { $this->assertParams($app, $key); - $this->loadConfigAll(); + $this->loadConfig(lazy: true); $this->matchAndApplyLexiconDefinition($app, $key); $lazy = $this->isLazy($app, $key); @@ -1198,6 +1204,7 @@ public function deleteKey(string $app, string $key): void { unset($this->lazyCache[$app][$key]); unset($this->fastCache[$app][$key]); unset($this->valueTypes[$app][$key]); + $this->clearLocalCache(); } /** @@ -1227,12 +1234,13 @@ public function deleteApp(string $app): void { public function clearCache(bool $reload = false): void { $this->lazyLoaded = $this->fastLoaded = false; $this->lazyCache = $this->fastCache = $this->valueTypes = $this->configLexiconDetails = []; + $this->localCache?->remove(self::LOCAL_CACHE_KEY); if (!$reload) { return; } - $this->loadConfigAll(); + $this->loadConfig(lazy: true); } @@ -1293,35 +1301,49 @@ private function assertParams(string $app = '', string $configKey = '', bool $al } } - private function loadConfigAll(?string $app = null): void { - $this->loadConfig($app, null); - } - /** * Load normal config or config set as lazy loaded * - * @param bool|null $lazy set to TRUE to load config set as lazy loaded, set to NULL to load all config + * @param bool $lazy set to TRUE to also load config values set as lazy loaded */ - private function loadConfig(?string $app = null, ?bool $lazy = false): void { + private function loadConfig(?string $app = null, bool $lazy = false): void { if ($this->isLoaded($lazy)) { return; } // if lazy is null or true, we debug log - if (($lazy ?? true) !== false && $app !== null) { + if ($lazy === true && $app !== null) { $exception = new \RuntimeException('The loading of lazy AppConfig values have been triggered by app "' . $app . '"'); $this->logger->debug($exception->getMessage(), ['exception' => $exception, 'app' => $app]); } - $qb = $this->connection->getQueryBuilder(); - $qb->from('appconfig'); + $loadLazyOnly = $lazy && $this->isLoaded(); - // we only need value from lazy when loadConfig does not specify it - $qb->select('appid', 'configkey', 'configvalue', 'type'); + /** @var array */ + $cacheContent = $this->localCache?->get(self::LOCAL_CACHE_KEY) ?? []; + $includesLazyValues = !empty($cacheContent) && !empty($cacheContent['lazyCache']); + if (!empty($cacheContent) && (!$lazy || $includesLazyValues)) { + $this->valueTypes = $cacheContent['valueTypes']; + $this->fastCache = $cacheContent['fastCache']; + $this->fastLoaded = !empty($this->fastCache); + if ($includesLazyValues) { + $this->lazyCache = $cacheContent['lazyCache']; + $this->lazyLoaded = !empty($this->lazyCache); + } + return; + } - if ($lazy !== null) { - $qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter($lazy ? 1 : 0, IQueryBuilder::PARAM_INT))); + // Otherwise no cache available and we need to fetch from database + $qb = $this->connection->getQueryBuilder(); + $qb->from('appconfig') + ->select('appid', 'configkey', 'configvalue', 'type'); + + if ($lazy === false) { + $qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT))); } else { + if ($loadLazyOnly) { + $qb->where($qb->expr()->eq('lazy', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT))); + } $qb->addSelect('lazy'); } @@ -1329,56 +1351,34 @@ private function loadConfig(?string $app = null, ?bool $lazy = false): void { $rows = $result->fetchAll(); foreach ($rows as $row) { // most of the time, 'lazy' is not in the select because its value is already known - if (($row['lazy'] ?? ($lazy ?? 0) ? 1 : 0) === 1) { + if ($lazy && ((int)$row['lazy']) === 1) { $this->lazyCache[$row['appid']][$row['configkey']] = $row['configvalue'] ?? ''; } else { $this->fastCache[$row['appid']][$row['configkey']] = $row['configvalue'] ?? ''; } $this->valueTypes[$row['appid']][$row['configkey']] = (int)($row['type'] ?? 0); } - $result->closeCursor(); - $this->setAsLoaded($lazy); - } - /** - * if $lazy is: - * - false: will returns true if fast config is loaded - * - true : will returns true if lazy config is loaded - * - null : will returns true if both config are loaded - * - * @param bool $lazy - * - * @return bool - */ - private function isLoaded(?bool $lazy): bool { - if ($lazy === null) { - return $this->lazyLoaded && $this->fastLoaded; - } + $result->closeCursor(); + $this->localCache?->set( + self::LOCAL_CACHE_KEY, + [ + 'fastCache' => $this->fastCache, + 'lazyCache' => $this->lazyCache, + 'valueTypes' => $this->valueTypes, + ], + self::LOCAL_CACHE_TTL, + ); - return $lazy ? $this->lazyLoaded : $this->fastLoaded; + $this->fastLoaded = true; + $this->lazyLoaded = $lazy; } /** - * if $lazy is: - * - false: set fast config as loaded - * - true : set lazy config as loaded - * - null : set both config as loaded - * - * @param bool $lazy + * @param bool $lazy - If set to true then also check if lazy values are loaded */ - private function setAsLoaded(?bool $lazy): void { - if ($lazy === null) { - $this->fastLoaded = true; - $this->lazyLoaded = true; - - return; - } - - if ($lazy) { - $this->lazyLoaded = true; - } else { - $this->fastLoaded = true; - } + private function isLoaded(bool $lazy = false): bool { + return $this->fastLoaded && (!$lazy || $this->lazyLoaded); } /** @@ -1388,7 +1388,7 @@ private function setAsLoaded(?bool $lazy): void { * @param string $key key * @param string $default = null, default value if the key does not exist * - * @return string the value or $default + * @return ?string the value or $default * @deprecated 29.0.0 use getValue*() * * This function gets a value from the appconfig table. If the key does @@ -1421,7 +1421,7 @@ public function setValue($app, $key, $value) { * or enabled (lazy=lazy-2) * * this solution would remove the loading of config values from disabled app - * unless calling the method {@see loadConfigAll()} + * unless calling the method. */ return $this->setTypedValue($app, $key, (string)$value, false, self::VALUE_MIXED); } @@ -1733,7 +1733,7 @@ private function matchAndApplyLexiconDefinition( * * @return bool TRUE if conflict can be fully ignored, FALSE if action should be not performed * @throws AppConfigUnknownKeyException if strictness implies exception - * @see ILexicon::getStrictness() + * @see \OCP\Config\Lexicon\ILexicon::getStrictness() */ private function applyLexiconStrictness( ?Strictness $strictness, @@ -1772,8 +1772,9 @@ public function getConfigDetailsFromLexicon(string $appId): array { $configLexicon = $bootstrapCoordinator->getRegistrationContext()?->getConfigLexicon($appId); foreach ($configLexicon?->getAppConfigs() ?? [] as $configEntry) { $entries[$configEntry->getKey()] = $configEntry; - if ($configEntry->getRename() !== null) { - $aliases[$configEntry->getRename()] = $configEntry->getKey(); + $newName = $configEntry->getRename(); + if ($newName !== null) { + $aliases[$newName] = $configEntry->getKey(); } } @@ -1819,4 +1820,8 @@ public function getAppInstalledVersions(bool $onlyEnabled = false): array { } return $this->appVersionsCache; } + + private function clearLocalCache(): void { + $this->localCache?->remove(self::LOCAL_CACHE_KEY); + } } diff --git a/tests/lib/AppConfigTest.php b/tests/lib/AppConfigIntegrationTest.php similarity index 99% rename from tests/lib/AppConfigTest.php rename to tests/lib/AppConfigIntegrationTest.php index 0ae917a1d9fdd..4c4154927081a 100644 --- a/tests/lib/AppConfigTest.php +++ b/tests/lib/AppConfigIntegrationTest.php @@ -14,20 +14,20 @@ use OCP\Exceptions\AppConfigTypeConflictException; use OCP\Exceptions\AppConfigUnknownKeyException; use OCP\IAppConfig; +use OCP\ICacheFactory; use OCP\IConfig; use OCP\IDBConnection; use OCP\Security\ICrypto; use OCP\Server; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; /** - * Class AppConfigTest - * * @group DB * * @package Test */ -class AppConfigTest extends TestCase { +class AppConfigIntegrationTest extends TestCase { protected IAppConfig $appConfig; protected IDBConnection $connection; private IConfig $config; @@ -35,6 +35,7 @@ class AppConfigTest extends TestCase { private PresetManager $presetManager; private LoggerInterface $logger; private ICrypto $crypto; + private ICacheFactory&MockObject $cacheFactory; private array $originalConfig; @@ -107,6 +108,8 @@ protected function setUp(): void { $this->presetManager = Server::get(PresetManager::class); $this->logger = Server::get(LoggerInterface::class); $this->crypto = Server::get(ICrypto::class); + $this->cacheFactory = $this->createMock(ICacheFactory::class); + $this->cacheFactory->method('isLocalCacheAvailable')->willReturn(false); // storing current config and emptying the data table $sql = $this->connection->getQueryBuilder(); @@ -200,6 +203,7 @@ private function generateAppConfig(bool $preLoading = true): IAppConfig { $this->presetManager, $this->logger, $this->crypto, + $this->cacheFactory, ); $msg = ' generateAppConfig() failed to confirm cache status'; From 9d320f84707e280da3a7262f0ceee56f714d589d Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Wed, 13 Aug 2025 17:34:20 +0200 Subject: [PATCH 2/7] test(AppConfig): add proper unit tests Signed-off-by: Ferdinand Thiessen --- tests/lib/AppConfigTest.php | 201 ++++++++++++++++++++++++++++++++++++ 1 file changed, 201 insertions(+) create mode 100644 tests/lib/AppConfigTest.php diff --git a/tests/lib/AppConfigTest.php b/tests/lib/AppConfigTest.php new file mode 100644 index 0000000000000..d7358c4a1e975 --- /dev/null +++ b/tests/lib/AppConfigTest.php @@ -0,0 +1,201 @@ +connection = $this->createMock(IDBConnection::class); + $this->config = $this->createMock(IConfig::class); + $this->configManager = $this->createMock(ConfigManager::class); + $this->presetManager = $this->createMock(PresetManager::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->crypto = $this->createMock(ICrypto::class); + $this->cacheFactory = $this->createMock(ICacheFactory::class); + $this->localCache = $this->createMock(ICache::class); + } + + protected function getAppConfig($cached = false): AppConfig { + $this->config->method('getSystemValueBool') + ->with('cache_app_config', $cached) + ->willReturn(true); + $this->cacheFactory->method('isLocalCacheAvailable')->willReturn($cached); + if ($cached) { + $this->cacheFactory->method('createLocal')->willReturn($this->localCache); + } + + return new AppConfig( + $this->connection, + $this->config, + $this->configManager, + $this->presetManager, + $this->logger, + $this->crypto, + $this->cacheFactory, + ); + } + + public function testCachedRead(): void { + $this->localCache->expects(self::once()) + ->method('get') + ->with('OC\\AppConfig') + ->willReturn([ + 'fastCache' => [ + 'appid' => [ + 'some-key' => 'some-value', + 'other-key' => 'other value' + ], + ], + 'valueTypes' => [ + 'appid' => [ + 'some-key' => AppConfig::VALUE_STRING, + 'other-key' => AppConfig::VALUE_STRING, + ], + ], + ]); + + $this->connection->expects(self::never())->method('getQueryBuilder'); + $config = $this->getAppConfig(true); + + + $this->assertSame('some-value', $config->getValueString('appid', 'some-key')); + $this->assertSame('other value', $config->getValueString('appid', 'other-key')); + $this->assertSame(AppConfig::VALUE_STRING, $config->getValueType('appid', 'some-key', false)); + } + + public function testCachedLazyRead(): void { + $this->localCache->expects(self::once()) + ->method('get') + ->with('OC\\AppConfig') + ->willReturn([ + 'fastCache' => [ + 'appid' => [ + 'fast-key' => 'fast value', + ], + ], + 'lazyCache' => [ + 'appid' => [ + 'lazy-key' => 'lazy value', + ], + ], + 'valueTypes' => [ + 'appid' => [ + 'some-key' => AppConfig::VALUE_STRING, + 'lazy-key' => AppConfig::VALUE_STRING, + ], + ], + ]); + + $this->connection->expects(self::never())->method('getQueryBuilder'); + $config = $this->getAppConfig(true); + + + $this->assertSame('fast value', $config->getValueString('appid', 'fast-key')); + $this->assertSame('lazy value', $config->getValueString('appid', 'lazy-key', '', true)); + } + + public function testOnlyFastKeyCached(): void { + $this->localCache->expects(self::atLeastOnce()) + ->method('get') + ->with('OC\\AppConfig') + ->willReturn([ + 'fastCache' => [ + 'appid' => [ + 'fast-key' => 'fast value', + ], + ], + 'valueTypes' => [ + 'appid' => [ + 'fast-key' => AppConfig::VALUE_STRING, + ], + ], + ]); + + $result = $this->createMock(IResult::class); + $result->method('fetchAll')->willReturn([ + ['lazy' => 1, 'appid' => 'appid', 'configkey' => 'lazy-key', 'configvalue' => 'lazy value'], + ]); + $expression = $this->createMock(IExpressionBuilder::class); + $queryBuilder = $this->createMock(IQueryBuilder::class); + $queryBuilder->method('from')->willReturn($queryBuilder); + $queryBuilder->method('expr')->willReturn($expression); + $queryBuilder->method('executeQuery')->willReturn($result); + + $this->connection->expects(self::once())->method('getQueryBuilder')->willReturn($queryBuilder); + $config = $this->getAppConfig(true); + + + $this->assertSame('fast value', $config->getValueString('appid', 'fast-key')); + $this->assertSame('lazy value', $config->getValueString('appid', 'lazy-key', '', true)); + } + + public function testWritesAreCached(): void { + $this->localCache->expects(self::atLeastOnce()) + ->method('get') + ->with('OC\\AppConfig') + ->willReturn([ + 'fastCache' => [ + 'appid' => [ + 'first-key' => 'first value', + ], + ], + 'valueTypes' => [ + 'appid' => [ + 'first-key' => AppConfig::VALUE_STRING, + ], + ], + ]); + + $expression = $this->createMock(IExpressionBuilder::class); + $queryBuilder = $this->createMock(IQueryBuilder::class); + $queryBuilder->expects(self::once()) + ->method('update') + ->with('appconfig', null) + ->willReturn($queryBuilder); + $queryBuilder->method('set')->willReturn($queryBuilder); + $queryBuilder->method('where')->willReturn($queryBuilder); + $queryBuilder->method('andWhere')->willReturn($queryBuilder); + $queryBuilder->method('expr')->willReturn($expression); + $this->connection->expects(self::once())->method('getQueryBuilder')->willReturn($queryBuilder); + + $config = $this->getAppConfig(true); + + $this->assertSame('first value', $config->getValueString('appid', 'first-key')); + $config->setValueString('appid', 'first-key', 'new value'); + $this->assertSame('new value', $config->getValueString('appid', 'first-key')); + } +} From d5e2432bcdb07162e9e8eb0b73e722d54e1cee15 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 12 Aug 2025 22:21:38 +0200 Subject: [PATCH 3/7] fix: resolve invalid usage of `AppConfig::getValue` Signed-off-by: Ferdinand Thiessen --- build/psalm-baseline.xml | 9 --------- core/Command/Encryption/Enable.php | 4 ++-- lib/private/AppConfig.php | 6 +++--- lib/private/Installer.php | 2 +- lib/private/legacy/OC_App.php | 2 +- tests/Core/Command/Encryption/EnableTest.php | 8 ++++---- 6 files changed, 11 insertions(+), 20 deletions(-) diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 114dc479f1ab5..7de2b7161b819 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -2980,9 +2980,6 @@ - - - @@ -4010,9 +4007,6 @@ - - - @@ -4389,9 +4383,6 @@ - - - diff --git a/core/Command/Encryption/Enable.php b/core/Command/Encryption/Enable.php index 2c476185692c5..02045fa1a4ba6 100644 --- a/core/Command/Encryption/Enable.php +++ b/core/Command/Encryption/Enable.php @@ -42,8 +42,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int $output->writeln('No encryption module is loaded'); return 1; } - $defaultModule = $this->config->getAppValue('core', 'default_encryption_module', null); - if ($defaultModule === null) { + $defaultModule = $this->config->getAppValue('core', 'default_encryption_module'); + if ($defaultModule === '') { $output->writeln('No default module is set'); return 1; } diff --git a/lib/private/AppConfig.php b/lib/private/AppConfig.php index a245f3149d239..d023e688c0070 100644 --- a/lib/private/AppConfig.php +++ b/lib/private/AppConfig.php @@ -1386,15 +1386,15 @@ private function isLoaded(bool $lazy = false): bool { * * @param string $app app * @param string $key key - * @param string $default = null, default value if the key does not exist + * @param string $default - Default value if the key does not exist * - * @return ?string the value or $default + * @return string the value or $default * @deprecated 29.0.0 use getValue*() * * This function gets a value from the appconfig table. If the key does * not exist the default value will be returned */ - public function getValue($app, $key, $default = null) { + public function getValue($app, $key, $default = '') { $this->loadConfig($app); $this->matchAndApplyLexiconDefinition($app, $key); diff --git a/lib/private/Installer.php b/lib/private/Installer.php index 91d20a129ae55..fd737d286ad81 100644 --- a/lib/private/Installer.php +++ b/lib/private/Installer.php @@ -546,7 +546,7 @@ public static function installShippedApps(bool $softErrors = false, ?IOutput $ou while (false !== ($filename = readdir($dir))) { if ($filename[0] !== '.' and is_dir($app_dir['path'] . "/$filename")) { if (file_exists($app_dir['path'] . "/$filename/appinfo/info.xml")) { - if ($config->getAppValue($filename, 'installed_version', null) === null) { + if ($config->getAppValue($filename, 'installed_version') === '') { $enabled = $appManager->isDefaultEnabled($filename); if (($enabled || in_array($filename, $appManager->getAlwaysEnabledApps())) && $config->getAppValue($filename, 'enabled') !== 'no') { diff --git a/lib/private/legacy/OC_App.php b/lib/private/legacy/OC_App.php index 10b78e2a7ef19..6dbef42594bf9 100644 --- a/lib/private/legacy/OC_App.php +++ b/lib/private/legacy/OC_App.php @@ -685,7 +685,7 @@ public static function updateApp(string $appId): bool { //set remote/public handlers if (array_key_exists('ocsid', $appData)) { \OC::$server->getConfig()->setAppValue($appId, 'ocsid', $appData['ocsid']); - } elseif (\OC::$server->getConfig()->getAppValue($appId, 'ocsid', null) !== null) { + } elseif (\OC::$server->getConfig()->getAppValue($appId, 'ocsid') !== '') { \OC::$server->getConfig()->deleteAppValue($appId, 'ocsid'); } foreach ($appData['remote'] as $name => $path) { diff --git a/tests/Core/Command/Encryption/EnableTest.php b/tests/Core/Command/Encryption/EnableTest.php index 32d1a7576f5f5..0e9655c29c778 100644 --- a/tests/Core/Command/Encryption/EnableTest.php +++ b/tests/Core/Command/Encryption/EnableTest.php @@ -48,9 +48,9 @@ protected function setUp(): void { public static function dataEnable(): array { return [ - ['no', null, [], true, 'Encryption enabled', 'No encryption module is loaded'], - ['yes', null, [], false, 'Encryption is already enabled', 'No encryption module is loaded'], - ['no', null, ['OC_TEST_MODULE' => []], true, 'Encryption enabled', 'No default module is set'], + ['no', '', [], true, 'Encryption enabled', 'No encryption module is loaded'], + ['yes', '', [], false, 'Encryption is already enabled', 'No encryption module is loaded'], + ['no', '', ['OC_TEST_MODULE' => []], true, 'Encryption enabled', 'No default module is set'], ['no', 'OC_NO_MODULE', ['OC_TEST_MODULE' => []], true, 'Encryption enabled', 'The current default module does not exist: OC_NO_MODULE'], ['no', 'OC_TEST_MODULE', ['OC_TEST_MODULE' => []], true, 'Encryption enabled', 'Default module: OC_TEST_MODULE'], ]; @@ -79,7 +79,7 @@ public function testEnable(string $oldStatus, ?string $defaultModule, array $ava ->method('getAppValue') ->willReturnMap([ ['core', 'encryption_enabled', 'no', $oldStatus], - ['core', 'default_encryption_module', null, $defaultModule], + ['core', 'default_encryption_module', '', $defaultModule], ]); } From 0ef58c96ab1adc249dea38950faecbde278c25f8 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 12 Aug 2025 22:20:15 +0200 Subject: [PATCH 4/7] refactor(Memcache\Factory): move prefix generation to the factory class This removes a circular dependency between AppConfig and cache factory. When a cache in the app config is used. Signed-off-by: Ferdinand Thiessen --- lib/private/AppConfig.php | 7 +- lib/private/Memcache/Factory.php | 169 ++++++++++++++----------- lib/private/Server.php | 73 ++++------- tests/lib/App/AppManagerTest.php | 21 ++- tests/lib/AppConfigIntegrationTest.php | 6 +- tests/lib/AppConfigTest.php | 9 +- tests/lib/Memcache/FactoryTest.php | 10 +- 7 files changed, 147 insertions(+), 148 deletions(-) diff --git a/lib/private/AppConfig.php b/lib/private/AppConfig.php index d023e688c0070..2fbc7fe38125f 100644 --- a/lib/private/AppConfig.php +++ b/lib/private/AppConfig.php @@ -14,6 +14,7 @@ use OC\AppFramework\Bootstrap\Coordinator; use OC\Config\ConfigManager; use OC\Config\PresetManager; +use OC\Memcache\Factory as CacheFactory; use OCP\Config\Lexicon\Entry; use OCP\Config\Lexicon\Strictness; use OCP\Config\ValueType; @@ -79,10 +80,12 @@ public function __construct( private readonly PresetManager $presetManager, protected LoggerInterface $logger, protected ICrypto $crypto, - readonly ICacheFactory $cacheFactory, + readonly CacheFactory $cacheFactory, ) { if ($config->getSystemValueBool('cache_app_config', true) && $cacheFactory->isLocalCacheAvailable()) { - $this->localCache = $cacheFactory->createLocal(); + $cacheFactory->withServerVersionPrefix(function (ICacheFactory $factory) { + $this->localCache = $factory->createLocal(); + }); } } diff --git a/lib/private/Memcache/Factory.php b/lib/private/Memcache/Factory.php index b54189937fc12..2a3fabc89f18e 100644 --- a/lib/private/Memcache/Factory.php +++ b/lib/private/Memcache/Factory.php @@ -7,72 +7,65 @@ */ namespace OC\Memcache; -use Closure; +use OC\SystemConfig; use OCP\Cache\CappedMemoryCache; +use OCP\IAppConfig; use OCP\ICache; use OCP\ICacheFactory; use OCP\IMemcache; use OCP\Profiler\IProfiler; +use OCP\ServerVersion; use Psr\Log\LoggerInterface; class Factory implements ICacheFactory { public const NULL_CACHE = NullCache::class; - private ?string $globalPrefix = null; - - private LoggerInterface $logger; - + protected ?string $globalPrefix = null; /** - * @var ?class-string $localCacheClass + * @var class-string $localCacheClass */ - private ?string $localCacheClass; + protected string $localCacheClass; /** - * @var ?class-string $distributedCacheClass + * @var class-string $distributedCacheClass */ - private ?string $distributedCacheClass; + protected string $distributedCacheClass; /** - * @var ?class-string $lockingCacheClass + * @var class-string $lockingCacheClass */ - private ?string $lockingCacheClass; - - private string $logFile; - - private IProfiler $profiler; + protected string $lockingCacheClass; /** - * @param Closure $globalPrefixClosure - * @param LoggerInterface $logger * @param ?class-string $localCacheClass * @param ?class-string $distributedCacheClass * @param ?class-string $lockingCacheClass - * @param string $logFile */ public function __construct( - private Closure $globalPrefixClosure, - LoggerInterface $logger, - IProfiler $profiler, + protected LoggerInterface $logger, + protected IProfiler $profiler, + protected ServerVersion $serverVersion, ?string $localCacheClass = null, ?string $distributedCacheClass = null, ?string $lockingCacheClass = null, - string $logFile = '', + protected string $logFile = '', ) { - $this->logFile = $logFile; - if (!$localCacheClass) { $localCacheClass = self::NULL_CACHE; } $localCacheClass = ltrim($localCacheClass, '\\'); + if (!$distributedCacheClass) { $distributedCacheClass = $localCacheClass; } - $distributedCacheClass = ltrim($distributedCacheClass, '\\'); $missingCacheMessage = 'Memcache {class} not available for {use} cache'; $missingCacheHint = 'Is the matching PHP module installed and enabled?'; - if (!class_exists($localCacheClass) || !$localCacheClass::isAvailable()) { + if (!class_exists($localCacheClass) + || !is_a($localCacheClass, ICache::class, true) + || !$localCacheClass::isAvailable() + ) { if (\OC::$CLI && !defined('PHPUNIT_RUN') && $localCacheClass === APCu::class) { // CLI should not fail if APCu is not available but fallback to NullCache. // This can be the case if APCu is used without apc.enable_cli=1. @@ -84,7 +77,11 @@ public function __construct( ]), $missingCacheHint); } } - if (!class_exists($distributedCacheClass) || !$distributedCacheClass::isAvailable()) { + + if (!class_exists($distributedCacheClass) + || !is_a($distributedCacheClass, ICache::class, true) + || !$distributedCacheClass::isAvailable() + ) { if (\OC::$CLI && !defined('PHPUNIT_RUN') && $distributedCacheClass === APCu::class) { // CLI should not fail if APCu is not available but fallback to NullCache. // This can be the case if APCu is used without apc.enable_cli=1. @@ -96,25 +93,51 @@ public function __construct( ]), $missingCacheHint); } } - if (!($lockingCacheClass && class_exists($lockingCacheClass) && $lockingCacheClass::isAvailable())) { + + if (!$lockingCacheClass + || !class_exists($lockingCacheClass) + || !is_a($lockingCacheClass, IMemcache::class, true) + || !$lockingCacheClass::isAvailable() + ) { // don't fall back since the fallback might not be suitable for storing lock $lockingCacheClass = self::NULL_CACHE; } + /** @var class-string */ $lockingCacheClass = ltrim($lockingCacheClass, '\\'); $this->localCacheClass = $localCacheClass; $this->distributedCacheClass = $distributedCacheClass; $this->lockingCacheClass = $lockingCacheClass; - $this->profiler = $profiler; } - private function getGlobalPrefix(): ?string { - if (is_null($this->globalPrefix)) { - $this->globalPrefix = ($this->globalPrefixClosure)(); + protected function getGlobalPrefix(): string { + if ($this->globalPrefix === null) { + $config = \OCP\Server::get(SystemConfig::class); + $versions = []; + if ($config->getValue('installed', false)) { + $appConfig = \OCP\Server::get(IAppConfig::class); + $versions = $appConfig->getAppInstalledVersions(); + } + $versions['core'] = implode('.', $this->serverVersion->getVersion()); + $this->globalPrefix = hash('xxh128', implode(',', $versions)); } return $this->globalPrefix; } + /** + * Override the global prefix for a specific closure. + * This should only be used internally for bootstrapping purpose! + * + * @param string $globalPrefix - The prefix to use during the closure execution + * @param \Closure $closure - The closure with the cache factory as the first parameter + */ + public function withServerVersionPrefix(\Closure $closure): void { + $backupPrefix = $this->globalPrefix; + $this->globalPrefix = hash('xxh128', implode('.', $this->serverVersion->getVersion())); + $closure($this); + $this->globalPrefix = $backupPrefix; + } + /** * create a cache instance for storing locks * @@ -122,22 +145,17 @@ private function getGlobalPrefix(): ?string { * @return IMemcache */ public function createLocking(string $prefix = ''): IMemcache { - $globalPrefix = $this->getGlobalPrefix(); - if (is_null($globalPrefix)) { - return new ArrayCache($prefix); - } - - assert($this->lockingCacheClass !== null); - $cache = new $this->lockingCacheClass($globalPrefix . '/' . $prefix); - if ($this->lockingCacheClass === Redis::class && $this->profiler->isEnabled()) { - // We only support the profiler with Redis - $cache = new ProfilerWrapperCache($cache, 'Locking'); - $this->profiler->add($cache); - } + $cache = new $this->lockingCacheClass($this->getGlobalPrefix() . '/' . $prefix); + if ($this->lockingCacheClass === Redis::class) { + if ($this->profiler->isEnabled()) { + // We only support the profiler with Redis + $cache = new ProfilerWrapperCache($cache, 'Locking'); + $this->profiler->add($cache); + } - if ($this->lockingCacheClass === Redis::class - && $this->logFile !== '' && is_writable(dirname($this->logFile)) && (!file_exists($this->logFile) || is_writable($this->logFile))) { - $cache = new LoggerWrapperCache($cache, $this->logFile); + if ($this->logFile !== '' && is_writable(dirname($this->logFile)) && (!file_exists($this->logFile) || is_writable($this->logFile))) { + $cache = new LoggerWrapperCache($cache, $this->logFile); + } } return $cache; } @@ -149,22 +167,17 @@ public function createLocking(string $prefix = ''): IMemcache { * @return ICache */ public function createDistributed(string $prefix = ''): ICache { - $globalPrefix = $this->getGlobalPrefix(); - if (is_null($globalPrefix)) { - return new ArrayCache($prefix); - } - - assert($this->distributedCacheClass !== null); - $cache = new $this->distributedCacheClass($globalPrefix . '/' . $prefix); - if ($this->distributedCacheClass === Redis::class && $this->profiler->isEnabled()) { - // We only support the profiler with Redis - $cache = new ProfilerWrapperCache($cache, 'Distributed'); - $this->profiler->add($cache); - } + $cache = new $this->distributedCacheClass($this->getGlobalPrefix() . '/' . $prefix); + if ($this->distributedCacheClass === Redis::class) { + if ($this->profiler->isEnabled()) { + // We only support the profiler with Redis + $cache = new ProfilerWrapperCache($cache, 'Distributed'); + $this->profiler->add($cache); + } - if ($this->distributedCacheClass === Redis::class && $this->logFile !== '' - && is_writable(dirname($this->logFile)) && (!file_exists($this->logFile) || is_writable($this->logFile))) { - $cache = new LoggerWrapperCache($cache, $this->logFile); + if ($this->logFile !== '' && is_writable(dirname($this->logFile)) && (!file_exists($this->logFile) || is_writable($this->logFile))) { + $cache = new LoggerWrapperCache($cache, $this->logFile); + } } return $cache; } @@ -176,22 +189,17 @@ public function createDistributed(string $prefix = ''): ICache { * @return ICache */ public function createLocal(string $prefix = ''): ICache { - $globalPrefix = $this->getGlobalPrefix(); - if (is_null($globalPrefix)) { - return new ArrayCache($prefix); - } - - assert($this->localCacheClass !== null); - $cache = new $this->localCacheClass($globalPrefix . '/' . $prefix); - if ($this->localCacheClass === Redis::class && $this->profiler->isEnabled()) { - // We only support the profiler with Redis - $cache = new ProfilerWrapperCache($cache, 'Local'); - $this->profiler->add($cache); - } + $cache = new $this->localCacheClass($this->getGlobalPrefix() . '/' . $prefix); + if ($this->localCacheClass === Redis::class) { + if ($this->profiler->isEnabled()) { + // We only support the profiler with Redis + $cache = new ProfilerWrapperCache($cache, 'Local'); + $this->profiler->add($cache); + } - if ($this->localCacheClass === Redis::class && $this->logFile !== '' - && is_writable(dirname($this->logFile)) && (!file_exists($this->logFile) || is_writable($this->logFile))) { - $cache = new LoggerWrapperCache($cache, $this->logFile); + if ($this->logFile !== '' && is_writable(dirname($this->logFile)) && (!file_exists($this->logFile) || is_writable($this->logFile))) { + $cache = new LoggerWrapperCache($cache, $this->logFile); + } } return $cache; } @@ -217,4 +225,11 @@ public function createInMemory(int $capacity = 512): ICache { public function isLocalCacheAvailable(): bool { return $this->localCacheClass !== self::NULL_CACHE; } + + public function clearAll(): void { + $this->createLocal()->clear(); + $this->createDistributed()->clear(); + $this->createLocking()->clear(); + $this->createInMemory()->clear(); + } } diff --git a/lib/private/Server.php b/lib/private/Server.php index 22cd13438b847..49bda19a738bc 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -585,62 +585,37 @@ public function __construct($webRoot, \OC\Config $config) { $this->registerAlias(IURLGenerator::class, URLGenerator::class); - $this->registerService(ICache::class, function ($c) { - return new Cache\File(); - }); - + $this->registerAlias(ICache::class, Cache\File::class); $this->registerService(Factory::class, function (Server $c) { $profiler = $c->get(IProfiler::class); - $arrayCacheFactory = new \OC\Memcache\Factory(fn () => '', $c->get(LoggerInterface::class), - $profiler, - ArrayCache::class, - ArrayCache::class, - ArrayCache::class - ); + $logger = $c->get(LoggerInterface::class); + $serverVersion = $c->get(ServerVersion::class); /** @var SystemConfig $config */ $config = $c->get(SystemConfig::class); - /** @var ServerVersion $serverVersion */ - $serverVersion = $c->get(ServerVersion::class); - - if ($config->getValue('installed', false) && !(defined('PHPUNIT_RUN') && PHPUNIT_RUN)) { - $logQuery = $config->getValue('log_query'); - $prefixClosure = function () use ($logQuery, $serverVersion): ?string { - if (!$logQuery) { - try { - $v = \OCP\Server::get(IAppConfig::class)->getAppInstalledVersions(true); - } catch (\Doctrine\DBAL\Exception $e) { - // Database service probably unavailable - // Probably related to https://github.com/nextcloud/server/issues/37424 - return null; - } - } else { - // If the log_query is enabled, we can not get the app versions - // as that does a query, which will be logged and the logging - // depends on redis and here we are back again in the same function. - $v = [ - 'log_query' => 'enabled', - ]; - } - $v['core'] = implode(',', $serverVersion->getVersion()); - $version = implode(',', array_keys($v)) . implode(',', $v); - $instanceId = \OC_Util::getInstanceId(); - $path = \OC::$SERVERROOT; - return md5($instanceId . '-' . $version . '-' . $path); - }; - return new \OC\Memcache\Factory($prefixClosure, - $c->get(LoggerInterface::class), + if (!$config->getValue('installed', false) || (defined('PHPUNIT_RUN') && PHPUNIT_RUN)) { + return new \OC\Memcache\Factory( + $logger, $profiler, - /** @psalm-taint-escape callable */ - $config->getValue('memcache.local', null), - /** @psalm-taint-escape callable */ - $config->getValue('memcache.distributed', null), - /** @psalm-taint-escape callable */ - $config->getValue('memcache.locking', null), - /** @psalm-taint-escape callable */ - $config->getValue('redis_log_file') + $serverVersion, + ArrayCache::class, + ArrayCache::class, + ArrayCache::class ); } - return $arrayCacheFactory; + + return new \OC\Memcache\Factory( + $logger, + $profiler, + $serverVersion, + /** @psalm-taint-escape callable */ + $config->getValue('memcache.local', null), + /** @psalm-taint-escape callable */ + $config->getValue('memcache.distributed', null), + /** @psalm-taint-escape callable */ + $config->getValue('memcache.locking', null), + /** @psalm-taint-escape callable */ + $config->getValue('redis_log_file') + ); }); $this->registerAlias(ICacheFactory::class, Factory::class); diff --git a/tests/lib/App/AppManagerTest.php b/tests/lib/App/AppManagerTest.php index 6637c529a1e53..9a4716aa8f10a 100644 --- a/tests/lib/App/AppManagerTest.php +++ b/tests/lib/App/AppManagerTest.php @@ -233,28 +233,25 @@ public function testEnableApp(): void { $this->manager->disableApp('files_trashbin'); } $this->eventDispatcher->expects($this->once())->method('dispatchTyped')->with(new AppEnableEvent('files_trashbin')); + $this->manager->enableApp('files_trashbin'); $this->assertEquals('yes', $this->appConfig->getValue('files_trashbin', 'enabled', 'no')); } public function testDisableApp(): void { $this->eventDispatcher->expects($this->once())->method('dispatchTyped')->with(new AppDisableEvent('files_trashbin')); + $this->manager->disableApp('files_trashbin'); $this->assertEquals('no', $this->appConfig->getValue('files_trashbin', 'enabled', 'no')); } public function testNotEnableIfNotInstalled(): void { - try { - $this->manager->enableApp('some_random_name_which_i_hope_is_not_an_app'); - $this->assertFalse(true, 'If this line is reached the expected exception is not thrown.'); - } catch (AppPathNotFoundException $e) { - // Exception is expected - $this->assertEquals('Could not find path for some_random_name_which_i_hope_is_not_an_app', $e->getMessage()); - } + $this->expectException(AppPathNotFoundException::class); + $this->expectExceptionMessage('Could not find path for some_random_name_which_i_hope_is_not_an_app'); + $this->appConfig->expects(self::never()) + ->method('setValue'); - $this->assertEquals('no', $this->appConfig->getValue( - 'some_random_name_which_i_hope_is_not_an_app', 'enabled', 'no' - )); + $this->manager->enableApp('some_random_name_which_i_hope_is_not_an_app'); } public function testEnableAppForGroups(): void { @@ -289,7 +286,9 @@ public function testEnableAppForGroups(): void { ->with('test') ->willReturn('apps/test'); - $this->eventDispatcher->expects($this->once())->method('dispatchTyped')->with(new AppEnableEvent('test', ['group1', 'group2'])); + $this->eventDispatcher->expects($this->once()) + ->method('dispatchTyped') + ->with(new AppEnableEvent('test', ['group1', 'group2'])); $manager->enableAppForGroups('test', $groups); $this->assertEquals('["group1","group2"]', $this->appConfig->getValue('test', 'enabled', 'no')); diff --git a/tests/lib/AppConfigIntegrationTest.php b/tests/lib/AppConfigIntegrationTest.php index 4c4154927081a..4f821e00a6354 100644 --- a/tests/lib/AppConfigIntegrationTest.php +++ b/tests/lib/AppConfigIntegrationTest.php @@ -11,10 +11,10 @@ use OC\AppConfig; use OC\Config\ConfigManager; use OC\Config\PresetManager; +use OC\Memcache\Factory as CacheFactory; use OCP\Exceptions\AppConfigTypeConflictException; use OCP\Exceptions\AppConfigUnknownKeyException; use OCP\IAppConfig; -use OCP\ICacheFactory; use OCP\IConfig; use OCP\IDBConnection; use OCP\Security\ICrypto; @@ -35,7 +35,7 @@ class AppConfigIntegrationTest extends TestCase { private PresetManager $presetManager; private LoggerInterface $logger; private ICrypto $crypto; - private ICacheFactory&MockObject $cacheFactory; + private CacheFactory&MockObject $cacheFactory; private array $originalConfig; @@ -108,7 +108,7 @@ protected function setUp(): void { $this->presetManager = Server::get(PresetManager::class); $this->logger = Server::get(LoggerInterface::class); $this->crypto = Server::get(ICrypto::class); - $this->cacheFactory = $this->createMock(ICacheFactory::class); + $this->cacheFactory = $this->createMock(CacheFactory::class); $this->cacheFactory->method('isLocalCacheAvailable')->willReturn(false); // storing current config and emptying the data table diff --git a/tests/lib/AppConfigTest.php b/tests/lib/AppConfigTest.php index d7358c4a1e975..f5e385a8405e3 100644 --- a/tests/lib/AppConfigTest.php +++ b/tests/lib/AppConfigTest.php @@ -10,11 +10,11 @@ use OC\AppConfig; use OC\Config\ConfigManager; use OC\Config\PresetManager; +use OC\Memcache\Factory as CacheFactory; use OCP\DB\IResult; use OCP\DB\QueryBuilder\IExpressionBuilder; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\ICache; -use OCP\ICacheFactory; use OCP\IConfig; use OCP\IDBConnection; use OCP\Security\ICrypto; @@ -33,7 +33,7 @@ class AppConfigTest extends TestCase { private PresetManager&MockObject $presetManager; private LoggerInterface&MockObject $logger; private ICrypto&MockObject $crypto; - private ICacheFactory&MockObject $cacheFactory; + private CacheFactory&MockObject $cacheFactory; private ICache&MockObject $localCache; protected function setUp(): void { @@ -45,7 +45,7 @@ protected function setUp(): void { $this->presetManager = $this->createMock(PresetManager::class); $this->logger = $this->createMock(LoggerInterface::class); $this->crypto = $this->createMock(ICrypto::class); - $this->cacheFactory = $this->createMock(ICacheFactory::class); + $this->cacheFactory = $this->createMock(CacheFactory::class); $this->localCache = $this->createMock(ICache::class); } @@ -55,6 +55,9 @@ protected function getAppConfig($cached = false): AppConfig { ->willReturn(true); $this->cacheFactory->method('isLocalCacheAvailable')->willReturn($cached); if ($cached) { + $this->cacheFactory->method('withServerVersionPrefix')->willReturnCallback(function (\Closure $closure): void { + $closure($this->cacheFactory); + }); $this->cacheFactory->method('createLocal')->willReturn($this->localCache); } diff --git a/tests/lib/Memcache/FactoryTest.php b/tests/lib/Memcache/FactoryTest.php index e16e079349f35..31500f31b65d5 100644 --- a/tests/lib/Memcache/FactoryTest.php +++ b/tests/lib/Memcache/FactoryTest.php @@ -12,6 +12,7 @@ use OC\Memcache\NullCache; use OCP\HintException; use OCP\Profiler\IProfiler; +use OCP\ServerVersion; use Psr\Log\LoggerInterface; class Test_Factory_Available_Cache1 extends NullCache { @@ -111,7 +112,8 @@ public function testCacheAvailability($localCache, $distributedCache, $lockingCa $expectedLocalCache, $expectedDistributedCache, $expectedLockingCache): void { $logger = $this->getMockBuilder(LoggerInterface::class)->getMock(); $profiler = $this->getMockBuilder(IProfiler::class)->getMock(); - $factory = new Factory(fn () => 'abc', $logger, $profiler, $localCache, $distributedCache, $lockingCache); + $serverVersion = $this->createMock(ServerVersion::class); + $factory = new Factory($logger, $profiler, $serverVersion, $localCache, $distributedCache, $lockingCache); $this->assertTrue(is_a($factory->createLocal(), $expectedLocalCache)); $this->assertTrue(is_a($factory->createDistributed(), $expectedDistributedCache)); $this->assertTrue(is_a($factory->createLocking(), $expectedLockingCache)); @@ -123,13 +125,15 @@ public function testCacheNotAvailableException($localCache, $distributedCache): $logger = $this->getMockBuilder(LoggerInterface::class)->getMock(); $profiler = $this->getMockBuilder(IProfiler::class)->getMock(); - new Factory(fn () => 'abc', $logger, $profiler, $localCache, $distributedCache); + $serverVersion = $this->createMock(ServerVersion::class); + new Factory($logger, $profiler, $serverVersion, $localCache, $distributedCache); } public function testCreateInMemory(): void { $logger = $this->getMockBuilder(LoggerInterface::class)->getMock(); $profiler = $this->getMockBuilder(IProfiler::class)->getMock(); - $factory = new Factory(fn () => 'abc', $logger, $profiler, null, null, null); + $serverVersion = $this->createMock(ServerVersion::class); + $factory = new Factory($logger, $profiler, $serverVersion, null, null, null); $cache = $factory->createInMemory(); $cache->set('test', 48); From 866ccae54282958d6d02ba01480afade26907deb Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Wed, 13 Aug 2025 18:04:24 +0200 Subject: [PATCH 5/7] docs(IConfig): fix wrong doc block type for `$key` on `setAppValue` Signed-off-by: Ferdinand Thiessen --- build/psalm-baseline.xml | 5 ----- lib/public/IConfig.php | 4 ++-- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/build/psalm-baseline.xml b/build/psalm-baseline.xml index 7de2b7161b819..1d6d14a503f54 100644 --- a/build/psalm-baseline.xml +++ b/build/psalm-baseline.xml @@ -3298,11 +3298,6 @@ - - - - - diff --git a/lib/public/IConfig.php b/lib/public/IConfig.php index 3bc64c5e13337..d22606672aae4 100644 --- a/lib/public/IConfig.php +++ b/lib/public/IConfig.php @@ -112,8 +112,8 @@ public function getAppKeys($appName); * Writes a new app wide value * * @param string $appName the appName that we want to store the value under - * @param string|float|int $key the key of the value, under which will be saved - * @param string $value the value that should be stored + * @param string $key the key of the value, under which will be saved + * @param string|float|int $value the value that should be stored * @return void * @since 6.0.0 * @deprecated 29.0.0 Use {@see IAppConfig} directly From 503f43f655ed0bf95714c77829c0a0ccb994b105 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Fri, 15 Aug 2025 15:59:05 +0200 Subject: [PATCH 6/7] test(cypress): clear cache after running OCC commands Signed-off-by: Ferdinand Thiessen --- cypress/support/commands.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/cypress/support/commands.ts b/cypress/support/commands.ts index ad486a8a8f7c5..b09a165451f09 100644 --- a/cypress/support/commands.ts +++ b/cypress/support/commands.ts @@ -246,3 +246,16 @@ Cypress.Commands.add('userFileExists', (user: string, path: string) => { return cy.runCommand(`stat --printf="%s" "data/${user}/files/${path}"`, { failOnNonZeroExit: true }) .then((exec) => Number.parseInt(exec.stdout || '0')) }) + +Cypress.Commands.add('runOccCommand', (command: string, options?: Partial) => { + return cy.runCommand(`php ./occ ${command}`, options) + .then((context) => + // OCC cannot clear the APCu cache + // eslint-disable-next-line cypress/no-unnecessary-waiting + cy.wait( + command.startsWith('app:') || command.startsWith('config:') + ? 3000 // clear APCu cache + : 0, + ).then(() => context), + ) +}) From c1583260bac8ccf87de0d86f9ecb54e589ae30df Mon Sep 17 00:00:00 2001 From: tomerqodo Date: Mon, 24 Nov 2025 09:48:06 +0200 Subject: [PATCH 7/7] Apply changes for benchmark PR --- cypress/support/commands.ts | 2 +- lib/private/AllConfig.php | 3 ++- lib/private/AppConfig.php | 31 ++++++++++++++++--------------- lib/private/Memcache/Factory.php | 2 +- 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/cypress/support/commands.ts b/cypress/support/commands.ts index b09a165451f09..d867435a432db 100644 --- a/cypress/support/commands.ts +++ b/cypress/support/commands.ts @@ -254,7 +254,7 @@ Cypress.Commands.add('runOccCommand', (command: string, options?: Partial context), ) diff --git a/lib/private/AllConfig.php b/lib/private/AllConfig.php index c4a6989df1317..cbf13862daa54 100644 --- a/lib/private/AllConfig.php +++ b/lib/private/AllConfig.php @@ -195,7 +195,8 @@ public function setAppValue($appName, $key, $value) { * @deprecated 29.0.0 Use {@see IAppConfig} directly */ public function getAppValue($appName, $key, $default = '') { - return \OC::$server->get(AppConfig::class)->getValue($appName, $key, $default) ?? $default; + $value = \OC::$server->get(AppConfig::class)->getValue($appName, $key, $default); + return $value ?? $default; } /** diff --git a/lib/private/AppConfig.php b/lib/private/AppConfig.php index 2fbc7fe38125f..8a011913b2649 100644 --- a/lib/private/AppConfig.php +++ b/lib/private/AppConfig.php @@ -56,7 +56,7 @@ class AppConfig implements IAppConfig { private const ENCRYPTION_PREFIX = '$AppConfigEncryption$'; private const ENCRYPTION_PREFIX_LENGTH = 21; // strlen(self::ENCRYPTION_PREFIX) private const LOCAL_CACHE_KEY = 'OC\\AppConfig'; - private const LOCAL_CACHE_TTL = 3; + private const LOCAL_CACHE_TTL = 300; /** @var array> ['app_id' => ['config_key' => 'config_value']] */ private array $fastCache = []; // cache for normal config keys @@ -160,7 +160,7 @@ public function searchKeys(string $app, string $prefix = '', bool $lazy = false) */ public function hasKey(string $app, string $key, ?bool $lazy = false): bool { $this->assertParams($app, $key); - $this->loadConfig($app, $lazy ?? true); + $this->loadConfig($app, $lazy !== false); $this->matchAndApplyLexiconDefinition($app, $key); $hasLazy = isset($this->lazyCache[$app][$key]); @@ -903,7 +903,6 @@ private function setTypedValue( $this->fastCache[$app][$key] = $value; } $this->valueTypes[$app][$key] = $type; - $this->clearLocalCache(); return true; } @@ -1224,7 +1223,7 @@ public function deleteApp(string $app): void { ->where($qb->expr()->eq('appid', $qb->createNamedParameter($app))); $qb->executeStatement(); - $this->clearCache(); + $this->clearLocalCache(); } /** @@ -1328,10 +1327,10 @@ private function loadConfig(?string $app = null, bool $lazy = false): void { if (!empty($cacheContent) && (!$lazy || $includesLazyValues)) { $this->valueTypes = $cacheContent['valueTypes']; $this->fastCache = $cacheContent['fastCache']; - $this->fastLoaded = !empty($this->fastCache); + $this->fastLoaded = true; if ($includesLazyValues) { $this->lazyCache = $cacheContent['lazyCache']; - $this->lazyLoaded = !empty($this->lazyCache); + $this->lazyLoaded = true; } return; } @@ -1363,15 +1362,17 @@ private function loadConfig(?string $app = null, bool $lazy = false): void { } $result->closeCursor(); - $this->localCache?->set( - self::LOCAL_CACHE_KEY, - [ - 'fastCache' => $this->fastCache, - 'lazyCache' => $this->lazyCache, - 'valueTypes' => $this->valueTypes, - ], - self::LOCAL_CACHE_TTL, - ); + if ($lazy) { + $this->localCache?->set( + self::LOCAL_CACHE_KEY, + [ + 'fastCache' => $this->fastCache, + 'lazyCache' => $this->lazyCache, + 'valueTypes' => $this->valueTypes, + ], + self::LOCAL_CACHE_TTL, + ); + } $this->fastLoaded = true; $this->lazyLoaded = $lazy; diff --git a/lib/private/Memcache/Factory.php b/lib/private/Memcache/Factory.php index 2a3fabc89f18e..9e34b0babc7a1 100644 --- a/lib/private/Memcache/Factory.php +++ b/lib/private/Memcache/Factory.php @@ -116,7 +116,7 @@ protected function getGlobalPrefix(): string { $versions = []; if ($config->getValue('installed', false)) { $appConfig = \OCP\Server::get(IAppConfig::class); - $versions = $appConfig->getAppInstalledVersions(); + $versions = $appConfig->getAppInstalledVersions(true); } $versions['core'] = implode('.', $this->serverVersion->getVersion()); $this->globalPrefix = hash('xxh128', implode(',', $versions));