diff --git a/apps/files_sharing/tests/TestCase.php b/apps/files_sharing/tests/TestCase.php index 29bacbe45974..89dfaffc725d 100644 --- a/apps/files_sharing/tests/TestCase.php +++ b/apps/files_sharing/tests/TestCase.php @@ -188,14 +188,6 @@ protected static function resetStorage() { $localCache->clear(); } $property->setAccessible(false); - $property = $storage->getProperty('distributedCache'); - $property->setAccessible(true); - /** @var ICache $localCache */ - $distributedCache = $property->getValue(); - if ($distributedCache instanceof ICache) { - $distributedCache->clear(); - } - $property->setAccessible(false); } /** diff --git a/changelog/unreleased/41734 b/changelog/unreleased/41734 new file mode 100644 index 000000000000..b126871f9925 --- /dev/null +++ b/changelog/unreleased/41734 @@ -0,0 +1,11 @@ +Change: Keep host local caches in the local cache tier + +The image paths of the active theme and the mimetype id map were stored in the +distributed memory cache although both are derived from the files and the +database of a single instance. They now use the host local cache tier and their +entries expire, so a stale entry is scoped to one node and no longer lives +forever. The repair step for mimetypes deletes rows from the mimetype table and +now clears the mimetype cache afterwards, and occ upgrade clears both cache +tiers instead of only the distributed one. + +https://github.com/owncloud/core/pull/41734 diff --git a/changelog/unreleased/41735 b/changelog/unreleased/41735 new file mode 100644 index 000000000000..599c250e4d1d --- /dev/null +++ b/changelog/unreleased/41735 @@ -0,0 +1,11 @@ +Bugfix: Clear stale integrity check results when rescanning + +The code integrity checker stores one cache entry per checked scope, but a +rescan only removed the entry holding the combined results. The per app entries +had no expiry either, so a verdict about an app was cached indefinitely and was +served even after the app had been repaired or replaced. Rescanning now clears +all of them, the entries expire, and the results are kept in the host local +cache tier - they describe the files on disk of one host and are of no use to +another. + +https://github.com/owncloud/core/pull/41735 diff --git a/changelog/unreleased/41736 b/changelog/unreleased/41736 new file mode 100644 index 000000000000..442e49f65d74 --- /dev/null +++ b/changelog/unreleased/41736 @@ -0,0 +1,10 @@ +Change: Drop the distributed cache in front of the storages table + +The mapping between the string and the numeric id of a storage was cached in +the distributed cache for five minutes on top of the cache that already +memoizes it for the duration of the request. A storage marked unavailable was +therefore still considered available by the other nodes until that entry +expired. The mapping is now looked up in the database once per request, so a +change of the availability is seen everywhere immediately. + +https://github.com/owncloud/core/pull/41736 diff --git a/core/Command/Upgrade.php b/core/Command/Upgrade.php index 19d529975f2f..549cae3a5b0d 100644 --- a/core/Command/Upgrade.php +++ b/core/Command/Upgrade.php @@ -30,6 +30,7 @@ namespace OC\Core\Command; use OC\Console\TimestampFormatter; +use OC\Memcache\LocalCacheFactory; use OC\Updater; use OCP\IConfig; use OCP\ILogger; @@ -277,10 +278,10 @@ function ($success) use ($output) { } // Clear caches after successful upgrade. - // Caches were created before the upgrade, so the cache prefix will be the old one - // TODO: Note that only the "create" method is available in the interface. It isn't - // possible to create local or distributed caches explicitly - $this->cacheFactory->create()->clear(); + // Caches were created before the upgrade, so the cache prefix will be the old one. + // Note that clearing the local cache only reaches the node running occ - other + // nodes rely on the TTLs the individual caches set on their entries. + $this->clearCaches(); return self::ERROR_SUCCESS; } elseif ($this->config->getSystemValue('maintenance', false)) { //Possible scenario: ownCloud core is updated but an app failed @@ -296,6 +297,15 @@ function ($success) use ($output) { } } + /** + * Clear both cache tiers - host local values (image paths, mime types, ...) + * live in the local tier, everything else in the distributed one. + */ + private function clearCaches() { + $this->cacheFactory->create()->clear(); + LocalCacheFactory::create($this->cacheFactory)->clear(); + } + /** * Perform a post upgrade check (specific to the command line tool) * diff --git a/lib/private/Files/Cache/Storage.php b/lib/private/Files/Cache/Storage.php index 3c328f156f25..39d140ce8720 100644 --- a/lib/private/Files/Cache/Storage.php +++ b/lib/private/Files/Cache/Storage.php @@ -27,7 +27,6 @@ namespace OC\Files\Cache; use OC\Cache\CappedMemoryCache; -use OCP\ICache; /** * Handle the mapping between the string and numeric storage ids @@ -38,6 +37,11 @@ * * A mapping between the two storage ids is stored in the database and accessible trough this class * + * The mapping is memoized for the duration of the request only. It used to be + * cached in the distributed cache on top of that, which bought about 0.1ms per + * lookup at the price of setAvailability() changes taking up to five minutes to + * be seen by the other nodes. + * * @package OC\Files\Cache */ class Storage { @@ -47,11 +51,6 @@ class Storage { /** @var CappedMemoryCache */ protected static $localCache = null; - /** @var ICache */ - private static $distributedCache = null; - - private static $distributedCacheTTL = 300; // 5 Min - /** * @param \OC\Files\Storage\Storage|string $storage * @param bool $isAvailable @@ -83,13 +82,6 @@ public function __construct($storage, $isAvailable = true) { // local cache has been initialized by self::getStorageById self::$localCache->set($this->storageId, $storageData); - - // distributed cache may need initializing - self::getDistributedCache()->set( - $this->storageId, - $storageData, - self::$distributedCacheTTL - ); } else { if ($row = self::getStorageById($this->storageId)) { $this->numericId = (int)$row['numeric_id']; @@ -101,7 +93,7 @@ public function __construct($storage, $isAvailable = true) { } /** - * query the local cache, a distributed cache and the db for a storageid + * query the request scoped cache and the db for a storageid * @param string $storageId * @return array|false */ @@ -110,38 +102,9 @@ public static function getStorageById($storageId) { self::$localCache = new CappedMemoryCache(); } $result = self::$localCache->get($storageId); - if ($result === null || !isset($result['numeric_id'])) { - $result = self::getStorageByIdFromCache($storageId); - self::$localCache->set($storageId, $result); - } - return $result; - } - - /** - * @return ICache - */ - private static function getDistributedCache() { - if (self::$distributedCache === null) { - self::$distributedCache = - \OC::$server->getMemCacheFactory()->create('getStorageById'); - } - return self::$distributedCache; - } - - /** - * query the distributed cache for a storageid - * @param string $storageId - * @return array|false - */ - private static function getStorageByIdFromCache($storageId) { - $result = self::getDistributedCache()->get($storageId); if ($result === null || !isset($result['numeric_id'])) { $result = self::getStorageByIdFromDb($storageId); - self::getDistributedCache()->set( - $storageId, - $result, - self::$distributedCacheTTL - ); + self::$localCache->set($storageId, $result); } return $result; } @@ -162,12 +125,9 @@ private static function getStorageByIdFromDb($storageId) { } private static function unsetCache($storageId) { - // delete from local cache if (self::$localCache !== null) { self::$localCache->remove($storageId); } - // delete from distributed cache - self::getDistributedCache()->remove($storageId); } /** diff --git a/lib/private/Files/Type/Loader.php b/lib/private/Files/Type/Loader.php index 3fd774575c12..4bc49f1ee020 100644 --- a/lib/private/Files/Type/Loader.php +++ b/lib/private/Files/Type/Loader.php @@ -22,6 +22,7 @@ namespace OC\Files\Type; use Doctrine\DBAL\Exception; +use OC\Memcache\LocalCacheFactory; use OCP\Files\IMimeTypeLoader; use OCP\IDBConnection; use OCP\ICacheFactory; @@ -38,6 +39,13 @@ class Loader implements IMimeTypeLoader { public const CACHE_PREFIX_FOR_ID = ':id:'; public const CACHE_PREFIX_FOR_MIME = ':mime:'; + /** + * The mimetype table only ever grows, so a cached entry cannot become + * wrong - but reset() only clears the cache of the node it runs on, so + * entries need to expire for the others to pick up a repair. + */ + public const CACHE_TTL = 24 * 3600; + /** @var IDBConnection */ private $dbConnection; @@ -55,7 +63,7 @@ class Loader implements IMimeTypeLoader { */ public function __construct(IDBConnection $dbConnection, ICacheFactory $cacheFactory) { $this->dbConnection = $dbConnection; - $this->memcache = $cacheFactory->create('mimetypes'); + $this->memcache = LocalCacheFactory::create($cacheFactory, 'mimetypes'); $this->mimetypes = []; $this->mimetypeIds = []; } @@ -171,8 +179,8 @@ protected function store($mimetype) { $r->free(); // update cache - $this->memcache->set(self::CACHE_PREFIX_FOR_ID . $row['id'], $mimetype); - $this->memcache->set(self::CACHE_PREFIX_FOR_MIME . $mimetype, $row['id']); + $this->memcache->set(self::CACHE_PREFIX_FOR_ID . $row['id'], $mimetype, self::CACHE_TTL); + $this->memcache->set(self::CACHE_PREFIX_FOR_MIME . $mimetype, $row['id'], self::CACHE_TTL); // update local vars $this->mimetypes[$row['id']] = $mimetype; @@ -232,8 +240,8 @@ private function getIdFromDB($mimetype) { $id = $row['id']; // update cache - $this->memcache->set(self::CACHE_PREFIX_FOR_ID . $row['id'], $row['mimetype']); - $this->memcache->set(self::CACHE_PREFIX_FOR_MIME . $row['mimetype'], $row['id']); + $this->memcache->set(self::CACHE_PREFIX_FOR_ID . $row['id'], $row['mimetype'], self::CACHE_TTL); + $this->memcache->set(self::CACHE_PREFIX_FOR_MIME . $row['mimetype'], $row['id'], self::CACHE_TTL); // update local vars $this->mimetypes[$row['id']] = $row['mimetype']; @@ -266,8 +274,8 @@ private function getMimetypeFromDB($id) { $mimetype = $row['mimetype']; // update cache - $this->memcache->set(self::CACHE_PREFIX_FOR_ID . $row['id'], $row['mimetype']); - $this->memcache->set(self::CACHE_PREFIX_FOR_MIME . $row['mimetype'], $row['id']); + $this->memcache->set(self::CACHE_PREFIX_FOR_ID . $row['id'], $row['mimetype'], self::CACHE_TTL); + $this->memcache->set(self::CACHE_PREFIX_FOR_MIME . $row['mimetype'], $row['id'], self::CACHE_TTL); // update local vars $this->mimetypes[$row['id']] = $row['mimetype']; diff --git a/lib/private/IntegrityCheck/Checker.php b/lib/private/IntegrityCheck/Checker.php index edd9a3b018c7..143fe54c3608 100644 --- a/lib/private/IntegrityCheck/Checker.php +++ b/lib/private/IntegrityCheck/Checker.php @@ -55,6 +55,14 @@ */ class Checker implements OnDiskHasher { public const CACHE_KEY = 'oc.integritycheck.checker'; + + /** + * The results describe the files on disk of this instance, so cached entries + * have to expire for a node that never runs a check itself to notice a + * repaired or replaced installation. + */ + public const CACHE_TTL = 24 * 3600; + /** @var EnvironmentHelper */ private $environmentHelper; /** @var AppLocator */ @@ -104,7 +112,9 @@ public function __construct( $this->fileAccessHelper = $fileAccessHelper; $this->appLocator = $appLocator; $this->config = $config; - $this->cache = $cacheFactory ? $cacheFactory->create(self::CACHE_KEY) : new \OC\Memcache\NullCache(); + $this->cache = $cacheFactory + ? \OC\Memcache\LocalCacheFactory::create($cacheFactory, self::CACHE_KEY) + : new \OC\Memcache\NullCache(); $this->appManager = $appManager; $this->tempManager = $tempManager; $this->verifier = $verifier; @@ -400,17 +410,21 @@ private function storeResults($scope, array $result) { $this->setAppValue(self::CACHE_KEY, \json_encode($resultArray)); //Set cache for each app - $this->cache->set($scope, \json_encode($resultArray)); - $this->cache->set(self::CACHE_KEY, \json_encode($resultArray)); + $this->cache->set($scope, \json_encode($resultArray), self::CACHE_TTL); + $this->cache->set(self::CACHE_KEY, \json_encode($resultArray), self::CACHE_TTL); } /** + * Clean previous results for a proper rescanning. Otherwise a stale verdict + * would be served instead of the one the rescan is about to produce. * - * Clean previous results for a proper rescanning. Otherwise + * storeResults() writes one entry per scope in addition to CACHE_KEY, so the + * whole prefix is cleared - removing CACHE_KEY alone left every per app entry + * behind. */ private function cleanResults() { $this->deleteAppValue(self::CACHE_KEY); - $this->cache->remove(self::CACHE_KEY); + $this->cache->clear(); } /** diff --git a/lib/private/Repair.php b/lib/private/Repair.php index dc5310a740dc..63248aa43aec 100644 --- a/lib/private/Repair.php +++ b/lib/private/Repair.php @@ -126,7 +126,7 @@ public function addStep($repairStep) { */ public static function getRepairSteps() { return [ - new RepairMimeTypes(\OC::$server->getConfig()), + new RepairMimeTypes(\OC::$server->getConfig(), \OC::$server->getMimeTypeLoader()), new RepairMismatchFileCachePath( \OC::$server->getDatabaseConnection(), \OC::$server->getMimeTypeLoader(), diff --git a/lib/private/Repair/RepairMimeTypes.php b/lib/private/Repair/RepairMimeTypes.php index 2afbeefce8ea..e06c032a9ce8 100644 --- a/lib/private/Repair/RepairMimeTypes.php +++ b/lib/private/Repair/RepairMimeTypes.php @@ -29,6 +29,7 @@ namespace OC\Repair; +use OCP\Files\IMimeTypeLoader; use OCP\Migration\IOutput; use OCP\Migration\IRepairStep; @@ -38,6 +39,11 @@ class RepairMimeTypes implements IRepairStep { */ protected $config; + /** + * @var IMimeTypeLoader + */ + protected $mimeTypeLoader; + /** * @var int */ @@ -45,9 +51,11 @@ class RepairMimeTypes implements IRepairStep { /** * @param \OCP\IConfig $config + * @param IMimeTypeLoader|null $mimeTypeLoader */ - public function __construct($config) { + public function __construct($config, IMimeTypeLoader $mimeTypeLoader = null) { $this->config = $config; + $this->mimeTypeLoader = $mimeTypeLoader ?? \OC::$server->getMimeTypeLoader(); } public function getName() { @@ -313,12 +321,15 @@ private function introduceRichDocumentsMimeTypes() { */ public function run(IOutput $out) { $ocVersionFromBeforeUpdate = $this->config->getSystemValue('version', '0.0.0'); + $repaired = false; // NOTE TO DEVELOPERS: when adding new mime types, please make sure to // add a version comparison to avoid doing it every time // only update mime types if necessary as it can be expensive if (\version_compare($ocVersionFromBeforeUpdate, '8.2.0', '<')) { + $repaired = true; + $this->fixOfficeMimeTypes(); $out->info('Fixed office mime types'); @@ -346,6 +357,7 @@ public function run(IOutput $out) { // Mimetype updates from #19272 if (\version_compare($ocVersionFromBeforeUpdate, '8.2.0.8', '<')) { + $repaired = true; $this->introduceJavaMimeType(); $out->info('Fixed java/class mime types'); @@ -360,8 +372,17 @@ public function run(IOutput $out) { } if (\version_compare($ocVersionFromBeforeUpdate, '9.0.0.10', '<')) { + $repaired = true; $this->introduceRichDocumentsMimeTypes(); $out->info('Fixed richdocuments additional office mime types'); } + + if ($repaired) { + // rows have been inserted into and deleted from the mimetypes table, + // so anything holding on to the old id <-> mimetype mapping has to + // let go of it + $this->mimeTypeLoader->reset(); + $out->info('Cleared the mime type cache'); + } } } diff --git a/lib/private/URLGenerator.php b/lib/private/URLGenerator.php index 34abd493fc9e..48c3aa696808 100644 --- a/lib/private/URLGenerator.php +++ b/lib/private/URLGenerator.php @@ -32,8 +32,10 @@ namespace OC; use OC\Helper\EnvironmentHelper; +use OC\Memcache\LocalCacheFactory; use OCP\Theme\ITheme; use OC_Defaults; +use OCP\ICache; use OCP\ICacheFactory; use OCP\IConfig; use OCP\IURLGenerator; @@ -44,10 +46,18 @@ * Class to generate URLs */ class URLGenerator implements IURLGenerator { + /** + * Image paths depend on the server root and the theme, so they are cached + * per node. The TTL only exists as a safety net - nothing invalidates these + * entries, so without it a moved installation would keep serving the old + * paths for the lifetime of the cache. + */ + private const IMAGE_PATH_TTL = 24 * 3600; + /** @var IConfig */ private $config; - /** @var ICacheFactory */ - private $cacheFactory; + /** @var ICache */ + private $imagePathCache; /** @var IRouter */ private $router; /** @var ITheme */ @@ -69,7 +79,7 @@ public function __construct( EnvironmentHelper $environmentHelper ) { $this->config = $config; - $this->cacheFactory = $cacheFactory; + $this->imagePathCache = LocalCacheFactory::create($cacheFactory, 'imagePath'); $this->router = $router; $this->environmentHelper = $environmentHelper; $this->theme = \OC_Util::getTheme(); @@ -159,16 +169,15 @@ public function linkTo($app, $file, $args = []) { * Returns the path to the image. */ public function imagePath($app, $image) { - $cache = $this->cacheFactory->create('imagePath'); $cacheKey = $this->theme->getName().'-'.$app.'-'.$image; - if ($key = $cache->get($cacheKey)) { + if ($key = $this->imagePathCache->get($cacheKey)) { return $key; } $path = $this->getImagePath($app, $image); if ($path !== '' && $path !== null) { - $cache->set($cacheKey, $path); + $this->imagePathCache->set($cacheKey, $path, self::IMAGE_PATH_TTL); return $path; } else { throw new RuntimeException( diff --git a/tests/lib/Files/Cache/StorageTest.php b/tests/lib/Files/Cache/StorageTest.php new file mode 100644 index 000000000000..a6a7d1a118cc --- /dev/null +++ b/tests/lib/Files/Cache/StorageTest.php @@ -0,0 +1,99 @@ + + * + * @copyright Copyright (c) 2026, ownCloud GmbH + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +namespace Test\Files\Cache; + +use OC\Files\Cache\Storage; +use Test\Memcache\FixedCacheFactory; +use Test\TestCase; + +/** + * Tests for the string id <-> numeric id mapping of the storages table. + * + * @group DB + */ +class StorageTest extends TestCase { + /** @var string */ + private $storageId; + + protected function setUp(): void { + parent::setUp(); + $this->storageId = 'test::' . $this->getUniqueID(); + } + + protected function tearDown(): void { + Storage::remove($this->storageId); + parent::tearDown(); + } + + public function testStorageIsInsertedOnce() { + $storage = new Storage($this->storageId); + $numericId = $storage->getNumericId(); + + $this->assertSame($numericId, (new Storage($this->storageId))->getNumericId()); + } + + public function testAvailabilityChangeIsVisibleImmediately() { + $storage = new Storage($this->storageId); + $this->assertTrue($storage->getAvailability()['available']); + + $storage->setAvailability(false); + + // a separately constructed instance - as another request would build it - + // has to see the change + $this->assertFalse((new Storage($this->storageId))->getAvailability()['available']); + } + + /** + * The mapping used to be cached in the distributed cache for five minutes on + * top of the request scoped memoization, which meant a storage marked + * unavailable on one node kept being reported as available by the others until + * the entry expired. It also let anything able to write to that cache remap a + * string storage id onto the numeric id - and hence the file cache - of + * another storage. + */ + public function testMappingIsNotCachedBeyondTheRequest() { + $this->assertFalse( + (new \ReflectionClass(Storage::class))->hasProperty('distributedCache'), + 'the mapping is no longer kept in a memory cache' + ); + + // nothing in this class reaches for a cache factory any more + $cacheFactory = $this->createMock(FixedCacheFactory::class); + $cacheFactory->expects($this->never())->method('create'); + $cacheFactory->expects($this->never())->method('createLocal'); + $cacheFactory->expects($this->never())->method('createDistributed'); + $this->overwriteService('MemCacheFactory', $cacheFactory); + + try { + $storage = new Storage($this->storageId); + $this->assertSame( + $storage->getNumericId(), + Storage::getNumericStorageId($this->storageId) + ); + $storage->setAvailability(false); + Storage::remove($this->storageId); + $this->assertFalse(Storage::exists($this->storageId)); + } finally { + $this->restoreService('MemCacheFactory'); + } + } +} diff --git a/tests/lib/Files/Type/LoaderTest.php b/tests/lib/Files/Type/LoaderTest.php index 5fd7efef4b3b..124e50a42bda 100644 --- a/tests/lib/Files/Type/LoaderTest.php +++ b/tests/lib/Files/Type/LoaderTest.php @@ -23,8 +23,8 @@ use OC\Files\Type\Loader; use OCP\IDBConnection; -use OCP\ICacheFactory; use OCP\ICache; +use Test\Memcache\FixedCacheFactory; class LoaderTest extends \Test\TestCase { /** @var IDBConnection */ @@ -33,16 +33,15 @@ class LoaderTest extends \Test\TestCase { protected $loader; /** @var ICache */ protected $memcache; + /** @var FixedCacheFactory */ + protected $cacheFactory; protected function setUp(): void { $this->memcache = $this->createMock(ICache::class); - $cacheFactoryMock = $this->createMock(ICacheFactory::class); - $cacheFactoryMock->expects($this->once()) - ->method('create') - ->willReturn($this->memcache); + $this->cacheFactory = new FixedCacheFactory($this->memcache); $this->db = \OC::$server->getDatabaseConnection(); - $this->loader = new Loader($this->db, $cacheFactoryMock); + $this->loader = new Loader($this->db, $this->cacheFactory); } protected function tearDown(): void { @@ -99,4 +98,37 @@ public function testStoreExists() { $this->assertEquals($mimetypeId, $mimetypeId2); } + + /** + * The mimetype map is derived from the database of this instance, so it + * belongs in the local tier - not in a distributed cache that anything able + * to reach it could remap. + */ + public function testUsesTheLocalCacheTier() { + $cacheFactory = $this->createMock(FixedCacheFactory::class); + $cacheFactory->expects($this->once()) + ->method('createLocal') + ->with('mimetypes') + ->willReturn($this->createMock(ICache::class)); + $cacheFactory->expects($this->never())->method('createDistributed'); + $cacheFactory->expects($this->never())->method('create'); + + new Loader($this->db, $cacheFactory); + } + + /** + * Nothing invalidates the cache of the other nodes, so entries have to + * expire on their own. + */ + public function testCachedEntriesExpire() { + $this->memcache->expects($this->atLeastOnce()) + ->method('set') + ->with( + $this->anything(), + $this->anything(), + Loader::CACHE_TTL + ); + + $this->loader->getId('testing/mymimetype'); + } } diff --git a/tests/lib/IntegrityCheck/CheckerTest.php b/tests/lib/IntegrityCheck/CheckerTest.php index 8cc07152f474..3a28796c8e9a 100644 --- a/tests/lib/IntegrityCheck/CheckerTest.php +++ b/tests/lib/IntegrityCheck/CheckerTest.php @@ -27,11 +27,12 @@ use OC\IntegrityCheck\Helpers\FileAccessHelper; use OC\IntegrityCheck\Verifier\Verifier; use OC\IntegrityCheck\Verifier\VerificationResult; +use OC\Memcache\ArrayCache; use OC\Memcache\NullCache; use OC\Memcache\Redis; use OCP\App\IAppManager; -use OCP\ICacheFactory; use OCP\IConfig; +use Test\Memcache\FixedCacheFactory; use Test\TestCase; /** @@ -48,7 +49,7 @@ class CheckerTest extends TestCase { private $fileAccessHelper; /** @var IConfig | \PHPUnit\Framework\MockObject\MockObject */ private $config; - /** @var ICacheFactory | \PHPUnit\Framework\MockObject\MockObject */ + /** @var FixedCacheFactory */ private $cacheFactory; /** @var IAppManager | \PHPUnit\Framework\MockObject\MockObject */ private $appManager; @@ -61,7 +62,7 @@ public function setUp(): void { $this->fileAccessHelper = $this->createMock(FileAccessHelper::class); $this->appLocator = $this->createMock(AppLocator::class); $this->config = $this->createMock(IConfig::class); - $this->cacheFactory = $this->createMock(ICacheFactory::class); + $this->cacheFactory = new FixedCacheFactory(new NullCache()); $this->appManager = $this->createMock(IAppManager::class); $this->verifier = $this->createMock(Verifier::class); @@ -87,12 +88,6 @@ public function setUp(): void { ->method('getAllApps') ->willReturn([]); - $this->cacheFactory - ->expects($this->any()) - ->method('create') - ->with('oc.integritycheck.checker') - ->willReturn(new NullCache()); - $this->checker = new Checker( $this->environmentHelper, $this->fileAccessHelper, @@ -103,6 +98,69 @@ public function setUp(): void { \OC::$server->getTempManager(), $this->verifier ); + + $this->assertSame([Checker::CACHE_KEY], $this->cacheFactory->getRequestedPrefixes()); + } + + /** + * The results describe the files on disk of this host, so they belong in the + * host local cache tier - not in a distributed one where another host could + * hand back a verdict about an installation it cannot see. + */ + public function testUsesTheLocalCacheTier() { + $cacheFactory = $this->createMock(FixedCacheFactory::class); + $cacheFactory->expects($this->once()) + ->method('createLocal') + ->with(Checker::CACHE_KEY) + ->willReturn(new NullCache()); + $cacheFactory->expects($this->never())->method('createDistributed'); + $cacheFactory->expects($this->never())->method('create'); + + new Checker( + $this->environmentHelper, + $this->fileAccessHelper, + $this->appLocator, + $this->config, + $cacheFactory, + $this->appManager, + \OC::$server->getTempManager(), + $this->verifier + ); + } + + /** + * storeResults() writes one entry per scope next to CACHE_KEY, and a rescan + * has to invalidate all of them - removing CACHE_KEY alone left every per app + * verdict cached forever. + */ + public function testRescanningDropsThePerAppResults() { + $cache = new ArrayCache(); + $checker = new Checker( + $this->environmentHelper, + $this->fileAccessHelper, + $this->appLocator, + $this->config, + new FixedCacheFactory($cache), + $this->appManager, + \OC::$server->getTempManager(), + $this->verifier + ); + + $this->environmentHelper->method('getChannel')->willReturn('stable'); + $this->environmentHelper->method('getServerRoot')->willReturn(\OC::$SERVERROOT); + $this->verifier->method('verify')->willReturn(VerificationResult::passed()); + + $cache->set('SomeApp', '{"SomeApp":[]}'); + $cache->set('SomeOtherApp', '{"SomeOtherApp":[]}'); + $cache->set(Checker::CACHE_KEY, '{"SomeApp":[]}'); + + $checker->runInstanceVerification(); + + // only the results of this run are left - the verification passed, so + // there is nothing to report + $this->assertNull($cache->get('SomeApp')); + $this->assertNull($cache->get('SomeOtherApp')); + $this->assertSame('[]', $cache->get(Checker::CACHE_KEY)); } public function testIgnoredAppSignatureWithoutSignatureData() { @@ -630,12 +688,7 @@ public function testVerifyCachedAppSignatureCheck() { $redisObj->method('get') ->with('SomeApp') ->willReturn('[]'); - $cacheFactory = $this->createMock(ICacheFactory::class); - $cacheFactory - ->expects($this->any()) - ->method('create') - ->with('oc.integritycheck.checker') - ->will($this->returnValue($redisObj)); + $cacheFactory = new FixedCacheFactory($redisObj); $checker = new Checker( $this->environmentHelper, $this->fileAccessHelper, @@ -654,12 +707,7 @@ public function testAppNotCachedSignatureCheck() { $redisObj->method('get') ->with('SomeApp') ->willReturn(null); - $cacheFactory = $this->createMock(ICacheFactory::class); - $cacheFactory - ->expects($this->any()) - ->method('create') - ->with('oc.integritycheck.checker') - ->will($this->returnValue($redisObj)); + $cacheFactory = new FixedCacheFactory($redisObj); $checker = new Checker( $this->environmentHelper, $this->fileAccessHelper, @@ -702,10 +750,7 @@ public function testHasPassedCheckWithExceptionResult() { ] ])); - $cacheFactory = $this->createMock(ICacheFactory::class); - $cacheFactory->expects($this->any()) - ->method('create') - ->willReturn(new NullCache()); + $cacheFactory = new FixedCacheFactory(new NullCache()); $checker = new Checker( $this->environmentHelper, @@ -732,10 +777,7 @@ public function testHasPassedCheckWithEmptyResults() { ->with('core', 'oc.integritycheck.checker', '{}') ->willReturn('{}'); - $cacheFactory = $this->createMock(ICacheFactory::class); - $cacheFactory->expects($this->any()) - ->method('create') - ->willReturn(new NullCache()); + $cacheFactory = new FixedCacheFactory(new NullCache()); $checker = new Checker( $this->environmentHelper, @@ -766,10 +808,7 @@ public function testHasPassedCheckWithFileMissing() { ] ])); - $cacheFactory = $this->createMock(ICacheFactory::class); - $cacheFactory->expects($this->any()) - ->method('create') - ->willReturn(new NullCache()); + $cacheFactory = new FixedCacheFactory(new NullCache()); $checker = new Checker( $this->environmentHelper, diff --git a/tests/lib/Repair/RepairMimeTypesTest.php b/tests/lib/Repair/RepairMimeTypesTest.php index e922543b727d..4cb9624ff129 100644 --- a/tests/lib/Repair/RepairMimeTypesTest.php +++ b/tests/lib/Repair/RepairMimeTypesTest.php @@ -51,7 +51,7 @@ protected function setUp(): void { $this->storage = new Temporary([]); - $this->repair = new RepairMimeTypes($config); + $this->repair = new RepairMimeTypes($config, $this->mimetypeLoader); } protected function tearDown(): void { @@ -461,6 +461,45 @@ public function testRenameFontsMimeTypesWhenExist() { $this->assertNull($this->getMimeTypeIdFromDB('font/opentype')); } + /** + * The repair step deletes rows from the mimetypes table, so it has to drop + * the id <-> mimetype mapping the loader caches - otherwise the mapping of + * a deleted mimetype survives the repair. + */ + public function testMimeTypeCacheIsResetAfterRepair() { + /** @var IMimeTypeLoader | MockObject $loader */ + $loader = $this->createMock(IMimeTypeLoader::class); + $loader->expects($this->once())->method('reset'); + + /** @var IConfig | MockObject $config */ + $config = $this->createMock(IConfig::class); + $config->method('getSystemValue')->with('version')->willReturn('8.0.0.0'); + + /** @var IOutput | MockObject $outputMock */ + $outputMock = $this->createMock(IOutput::class); + + (new RepairMimeTypes($config, $loader))->run($outputMock); + } + + /** + * Nothing was changed, so there is no reason to throw the mimetype cache of + * every node away. + */ + public function testMimeTypeCacheIsKeptWhenNothingIsRepaired() { + /** @var IMimeTypeLoader | MockObject $loader */ + $loader = $this->createMock(IMimeTypeLoader::class); + $loader->expects($this->never())->method('reset'); + + /** @var IConfig | MockObject $config */ + $config = $this->createMock(IConfig::class); + $config->method('getSystemValue')->with('version')->willReturn('10.0.0.0'); + + /** @var IOutput | MockObject $outputMock */ + $outputMock = $this->createMock(IOutput::class); + + (new RepairMimeTypes($config, $loader))->run($outputMock); + } + /** * Test that nothing happens and no error happens when all mimetypes are * already correct and no old ones exist.. diff --git a/tests/lib/UrlGeneratorTest.php b/tests/lib/UrlGeneratorTest.php index c3456dd315d3..cf32987eed28 100644 --- a/tests/lib/UrlGeneratorTest.php +++ b/tests/lib/UrlGeneratorTest.php @@ -8,11 +8,12 @@ namespace Test; use OC\Helper\EnvironmentHelper; +use OC\Memcache\ArrayCache; use OC\URLGenerator; -use OCP\ICacheFactory; use OCP\IConfig; use OCP\IURLGenerator; use OCP\Route\IRouter; +use Test\Memcache\FixedCacheFactory; /** * Class UrlGeneratorTest @@ -26,10 +27,14 @@ class UrlGeneratorTest extends TestCase { /** @var EnvironmentHelper | \PHPUnit\Framework\MockObject\MockObject */ private $environmentHelper; + /** @var ArrayCache */ + private $imagePathCache; + public function setUp(): void { parent::setUp(); $config = $this->createMock(IConfig::class); - $cacheFactory = $this->createMock(ICacheFactory::class); + $this->imagePathCache = new ArrayCache(); + $cacheFactory = new FixedCacheFactory($this->imagePathCache); $this->router = $this->createMock(IRouter::class); $this->environmentHelper = $this->createMock(EnvironmentHelper::class); $this->urlGenerator = new URLGenerator( @@ -141,4 +146,60 @@ public function provideSubDirURLs() { ["apps/index.php", "http://localhost/owncloud/apps/index.php"], ]; } + + public function testImagePathIsCached() { + $this->environmentHelper->expects($this->any()) + ->method('getWebRoot') + ->willReturn('/owncloud'); + $this->environmentHelper->expects($this->any()) + ->method('getServerRoot') + ->willReturn(\OC::$SERVERROOT); + + $path = $this->urlGenerator->imagePath('', 'favicon.png'); + $this->assertEquals('/owncloud/core/img/favicon.png', $path); + + // the theme name is part of the key, so that switching themes does not + // serve the paths of the previous one + $theme = \OC_Util::getTheme()->getName(); + $this->assertEquals($path, $this->imagePathCache->get($theme . '--favicon.png')); + + // a second call is served from the cache - proven by handing out a + // value the filesystem would never produce + $this->imagePathCache->set($theme . '--favicon.png', '/from/the/cache.png'); + $this->assertEquals('/from/the/cache.png', $this->urlGenerator->imagePath('', 'favicon.png')); + } + + /** + * Image paths are host local, so they must not be stored in the distributed + * cache where another host - or anything else reaching it - could serve them + * back. + */ + public function testImagePathsUseTheLocalCacheTier() { + $cacheFactory = $this->createMock(FixedCacheFactory::class); + $cacheFactory->expects($this->once()) + ->method('createLocal') + ->with('imagePath') + ->willReturn(new ArrayCache()); + $cacheFactory->expects($this->never())->method('createDistributed'); + $cacheFactory->expects($this->never())->method('create'); + + new URLGenerator( + $this->createMock(IConfig::class), + $cacheFactory, + $this->router, + $this->environmentHelper + ); + } + + public function testImagePathThrowsForMissingImage() { + $this->environmentHelper->expects($this->any()) + ->method('getWebRoot') + ->willReturn('/owncloud'); + $this->environmentHelper->expects($this->any()) + ->method('getServerRoot') + ->willReturn(\OC::$SERVERROOT); + + $this->expectException(\RuntimeException::class); + $this->urlGenerator->imagePath('', 'this-image-does-not-exist.gif'); + } }