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 <opensource@fthiessen.de>

lib/private/AppConfig.php

@@ -14,6 +14,7 @@ use JsonException;
 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 @@ class AppConfig implements IAppConfig {
 		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();
+			});
 		}
 	}
 

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<ICache> $localCacheClass
+	 * @var class-string<ICache> $localCacheClass
 	 */
-	private ?string $localCacheClass;
+	protected string $localCacheClass;
 
 	/**
-	 * @var ?class-string<ICache> $distributedCacheClass
+	 * @var class-string<ICache> $distributedCacheClass
 	 */
-	private ?string $distributedCacheClass;
+	protected string $distributedCacheClass;
 
 	/**
-	 * @var ?class-string<IMemcache> $lockingCacheClass
+	 * @var class-string<IMemcache> $lockingCacheClass
 	 */
-	private ?string $lockingCacheClass;
-
-	private string $logFile;
-
-	private IProfiler $profiler;
+	protected string $lockingCacheClass;
 
 	/**
-	 * @param Closure $globalPrefixClosure
-	 * @param LoggerInterface $logger
 	 * @param ?class-string<ICache> $localCacheClass
 	 * @param ?class-string<ICache> $distributedCacheClass
 	 * @param ?class-string<IMemcache> $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 @@ class Factory implements ICacheFactory {
 				]), $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 @@ class Factory implements ICacheFactory {
 				]), $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<IMemcache> */
 		$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 @@ class Factory implements ICacheFactory {
 	 * @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 @@ class Factory implements ICacheFactory {
 	 * @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 @@ class Factory implements ICacheFactory {
 	 * @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 @@ class Factory implements ICacheFactory {
 	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();
+	}
 }

lib/private/Server.php

@@ -585,62 +585,37 @@ class Server extends ServerContainer implements IServerContainer {
 
 		$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);
 

tests/lib/App/AppManagerTest.php

@@ -233,28 +233,25 @@ class AppManagerTest extends TestCase {
 			$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 @@ class AppManagerTest extends TestCase {
 			->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'));

tests/lib/AppConfigIntegrationTest.php

@@ -11,10 +11,10 @@ use InvalidArgumentException;
 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 @@ class AppConfigIntegrationTest extends TestCase {
 		$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

tests/lib/AppConfigTest.php

@@ -10,11 +10,11 @@ namespace Test;
 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 @@ class AppConfigTest extends TestCase {
 		$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 @@ class AppConfigTest extends TestCase {
 			->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);
 		}
 

tests/lib/Memcache/FactoryTest.php

@@ -12,6 +12,7 @@ use OC\Memcache\Factory;
 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 @@ class FactoryTest extends \Test\TestCase {
 		$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 @@ class FactoryTest extends \Test\TestCase {
 
 		$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);