diff --git a/changelog/unreleased/41753 b/changelog/unreleased/41753 new file mode 100644 index 000000000000..bd29bbc719ac --- /dev/null +++ b/changelog/unreleased/41753 @@ -0,0 +1,13 @@ +Change: Expose createLocal() on ICacheFactory + +The cache factory has always been able to hand out a cache from the host local +tier, but the method was missing from the public ICacheFactory interface, so +core had to ask for it defensively and apps had no way to use it at all. It is +now part of the interface, which lets values that are only meaningful on the +machine that produced them be kept out of the cache shared between the nodes of +an installation. + +Note for app developers: a class implementing OCP\ICacheFactory has to declare +createLocal() from this release on. + +https://github.com/owncloud/core/pull/41753 diff --git a/lib/private/App/AppManager.php b/lib/private/App/AppManager.php index b1480cd73590..4e73066b8a4a 100644 --- a/lib/private/App/AppManager.php +++ b/lib/private/App/AppManager.php @@ -31,8 +31,7 @@ namespace OC\App; use OC_App; -use OC\Memcache\ArrayCache; -use OC\Memcache\NullCache; +use OC\Memcache\LocalCacheFactory; use OC\Installer; use OCP\App\AppNotFoundException; use OCP\App\IAppManager; @@ -120,14 +119,9 @@ public function __construct( $this->dispatcher = $dispatcher; $this->config = $config; - // TODO we have no public API for this - if (\method_exists($this->memCacheFactory, 'createLocal')) { - /* @phan-suppress-next-line PhanUndeclaredMethod */ - $this->appInfo = $this->memCacheFactory->createLocal('app-info'); - } - if ($this->appInfo === null || $this->appInfo instanceof NullCache) { - $this->appInfo = new ArrayCache('app-info'); - } + // app info describes the apps installed on this machine, so it belongs in + // the host local tier + $this->appInfo = LocalCacheFactory::create($this->memCacheFactory, 'app-info'); } /** diff --git a/lib/private/Memcache/LocalCacheFactory.php b/lib/private/Memcache/LocalCacheFactory.php index 003cbeec2155..0e3ac480beaa 100644 --- a/lib/private/Memcache/LocalCacheFactory.php +++ b/lib/private/Memcache/LocalCacheFactory.php @@ -33,9 +33,8 @@ * consume what any single node (or anything else able to talk to the backend) * wrote there. * - * ICacheFactory does not declare createLocal(), so the tier is requested - * defensively and a request scoped ArrayCache is used whenever a usable local - * cache cannot be obtained. + * A request scoped ArrayCache is used whenever a usable local cache cannot be + * obtained. * * @package OC\Memcache */ @@ -51,11 +50,7 @@ class LocalCacheFactory { * @return ICache */ public static function create(ICacheFactory $factory, $prefix = '') { - $cache = null; - if (\method_exists($factory, 'createLocal')) { - /* @phan-suppress-next-line PhanUndeclaredMethod */ - $cache = $factory->createLocal($prefix); - } + $cache = $factory->createLocal($prefix); if (!$cache instanceof ICache || $cache instanceof NullCache) { $cache = new ArrayCache($prefix); } diff --git a/lib/public/ICacheFactory.php b/lib/public/ICacheFactory.php index 977bb31c42ee..988e741d1f55 100644 --- a/lib/public/ICacheFactory.php +++ b/lib/public/ICacheFactory.php @@ -40,6 +40,23 @@ interface ICacheFactory { */ public function create($prefix = ''); + /** + * Get a memory cache instance in the host local tier + * + * Unlike the cache returned by create(), this cache is not shared between the + * nodes of an installation. Values which are only meaningful on the machine + * which produced them - resolved paths on disk, results of inspecting the + * local installation - belong here and not in the shared cache, where every + * node would consume what any single node wrote. + * + * All entries added trough the cache instance will be namespaced by $prefix to prevent collisions between apps + * + * @param string $prefix + * @return \OCP\ICache + * @since 11.0.0 + */ + public function createLocal($prefix = ''); + /** * Check if any memory cache backend is available * diff --git a/tests/lib/Memcache/LocalCacheFactoryTest.php b/tests/lib/Memcache/LocalCacheFactoryTest.php index 374e54b79b63..a6d866681771 100644 --- a/tests/lib/Memcache/LocalCacheFactoryTest.php +++ b/tests/lib/Memcache/LocalCacheFactoryTest.php @@ -77,20 +77,12 @@ public function testNeverFallsBackToTheDistributedTier(): void { $this->assertNull($factory->createDistributed('test')->get('key')); } - public function testFallsBackWhenCreateLocalIsNotImplemented(): void { - // ICacheFactory does not declare createLocal(), so a bare interface - // mock has no such method - $factory = $this->createMock(ICacheFactory::class); - - $this->assertInstanceOf(ArrayCache::class, LocalCacheFactory::create($factory, 'test')); - } - /** * @dataProvider unusableLocalCacheProvider * @param mixed $returnValue */ public function testFallsBackWhenCreateLocalReturnsSomethingUnusable($returnValue): void { - $factory = $this->createMock(FixedCacheFactory::class); + $factory = $this->createMock(ICacheFactory::class); $factory->method('createLocal')->willReturn($returnValue); $this->assertInstanceOf(ArrayCache::class, LocalCacheFactory::create($factory, 'test')); @@ -103,6 +95,17 @@ public function unusableLocalCacheProvider() { ]; } + /** + * The helper calls createLocal() unconditionally, which is only safe as long + * as every ICacheFactory has to provide it. + */ + public function testTheLocalTierIsPartOfThePublicApi(): void { + $this->assertTrue( + (new \ReflectionClass(ICacheFactory::class))->hasMethod('createLocal'), + 'ICacheFactory declares createLocal()' + ); + } + public function testPassesThroughThePrefix(): void { $factory = new FixedCacheFactory($this->createMock(ICache::class));