From 9778adf1c534ce91f3a93543edea6e5c29396b62 Mon Sep 17 00:00:00 2001 From: Dan Duvall Date: Fri, 10 Dec 2021 10:20:05 -0800 Subject: [PATCH] Refactor SettingsBuilder to use BagOStuff. Removed use of Psr\SimpleCache\CacheInterface in favor of BagOStuff, as the latter is a tried-and-true abstraction and the former offers no real upfront benefits since the caching patterns of SettingsBuilder are quite basic at this time. The simplicity of cache interface use is largely in part to the minimal probabilistic stampede protection implementation within CachedSource which is left untouched by this change. Bug: T294748 Change-Id: Ie59b37a8d5c7bf96225757fa9eb9d2c762476713 --- composer.json | 1 - includes/Settings/Cache/ArrayCache.php | 65 ----- .../Settings/Cache/CacheArgumentException.php | 12 - includes/Settings/Cache/CachedSource.php | 17 +- includes/Settings/Cache/SharedMemoryCache.php | 274 ------------------ includes/Settings/SettingsBuilder.php | 14 +- tests/common/TestsAutoLoader.php | 3 - .../Settings/Cache/ArrayCacheTest.php | 17 -- .../Cache/CacheInterfaceTestTrait.php | 94 ------ .../Settings/Cache/CachedSourceTest.php | 32 +- .../Settings/Cache/SharedMemoryCacheTest.php | 63 ---- .../includes/Settings/SettingsBuilderTest.php | 19 +- 12 files changed, 54 insertions(+), 557 deletions(-) delete mode 100644 includes/Settings/Cache/ArrayCache.php delete mode 100644 includes/Settings/Cache/CacheArgumentException.php delete mode 100644 includes/Settings/Cache/SharedMemoryCache.php delete mode 100644 tests/phpunit/unit/includes/Settings/Cache/ArrayCacheTest.php delete mode 100644 tests/phpunit/unit/includes/Settings/Cache/CacheInterfaceTestTrait.php delete mode 100644 tests/phpunit/unit/includes/Settings/Cache/SharedMemoryCacheTest.php diff --git a/composer.json b/composer.json index c2ef2213b4c..3818df57eca 100644 --- a/composer.json +++ b/composer.json @@ -44,7 +44,6 @@ "php": ">=7.2.22", "psr/container": "1.1.1", "psr/log": "1.1.4", - "psr/simple-cache": "1.0.1", "ralouphie/getallheaders": "3.0.3", "symfony/polyfill-php80": "1.23.1", "symfony/yaml": "5.3.6", diff --git a/includes/Settings/Cache/ArrayCache.php b/includes/Settings/Cache/ArrayCache.php deleted file mode 100644 index bb285feb8f0..00000000000 --- a/includes/Settings/Cache/ArrayCache.php +++ /dev/null @@ -1,65 +0,0 @@ -store = []; - return true; - } - - public function delete( $key ): bool { - unset( $this->store[$key] ); - return true; - } - - public function deleteMultiple( $keys ): bool { - foreach ( $keys as $key ) { - $this->delete( $key ); - } - return true; - } - - public function get( $key, $default = null ) { - if ( !array_key_exists( $key, $this->store ) ) { - return $default; - } - return $this->store[$key]; - } - - public function getMultiple( $keys, $default = null ) { - $results = []; - - foreach ( $keys as $key ) { - $results[$key] = $this->get( $key, $default ); - } - - return $results; - } - - public function has( $key ): bool { - return array_key_exists( $key, $this->store ); - } - - public function set( $key, $value, $_ttl = null ): bool { - $this->store[$key] = $value; - return true; - } - - public function setMultiple( $values, $ttl = null ): bool { - foreach ( $values as $key => $value ) { - $this->set( $key, $value, $ttl ); - } - return true; - } -} diff --git a/includes/Settings/Cache/CacheArgumentException.php b/includes/Settings/Cache/CacheArgumentException.php deleted file mode 100644 index 412d8044cf2..00000000000 --- a/includes/Settings/Cache/CacheArgumentException.php +++ /dev/null @@ -1,12 +0,0 @@ -cache = $cache; @@ -40,11 +40,14 @@ class CachedSource implements SettingsSource { * @return array */ public function load(): array { - $key = $this->source->getHashKey(); - $item = $this->cache->get( $key, null ); + $key = $this->cache->makeGlobalKey( + __CLASS__, + $this->source->getHashKey() + ); + $item = $this->cache->get( $key ); $miss = - $item === null || + $item === false || $this->expiresEarly( $item, $this->source->getExpiryWeight() ); if ( $miss ) { diff --git a/includes/Settings/Cache/SharedMemoryCache.php b/includes/Settings/Cache/SharedMemoryCache.php deleted file mode 100644 index 896e68da444..00000000000 --- a/includes/Settings/Cache/SharedMemoryCache.php +++ /dev/null @@ -1,274 +0,0 @@ -namespace = $namespace; - $this->ttl = $ttl; - - if ( !static::isSupported() ) { - throw new SettingsBuilderException( 'APCu is not enabled' ); - } - } - - /** - * Clear all items from this cache. - * - * @return bool True on success, false on failure. - */ - public function clear(): bool { - return apcu_delete( - new APCuIterator( - sprintf( '/^%s/', preg_quote( $this->qualifyKey( '' ) ) ), - APC_ITER_KEY - ) - ); - } - - /** - * Delete an item from the cache. - * - * @param string $key - * - * @return bool True if removed, false if not. - * - * @throws CacheArgumentException - */ - public function delete( $key ): bool { - return $this->deleteMultiple( [ $key ] ); - } - - /** - * Deletes multiple cache items. - * - * @param iterable $keys - * - * @return bool True if all were removed, false if not. - * - * @throws CacheArgumentException - */ - public function deleteMultiple( $keys ): bool { - if ( !( is_array( $keys ) || $keys instanceof Traversable ) ) { - throw new CacheArgumentException( - 'given cache $keys is not an iterable' - ); - } - - $apcuKeys = []; - - foreach ( $keys as $key ) { - $this->assertValidKey( $key ); - $apcuKeys[] = $this->qualifyKey( $key ); - } - - $deleted = apcu_delete( $apcuKeys ); - - if ( is_array( $deleted ) ) { - return count( $deleted ) === count( $keys ); - } - - return $deleted; - } - - /** - * Fetches a value from the cache. - * - * @param string $key - * @param mixed $default - * - * @return mixed - * - * @throws CacheArgumentException - */ - public function get( $key, $default = null ) { - $result = $default; - - foreach ( $this->getMultiple( [ $key ], $default ) as $value ) { - $result = $value; - } - - return $result; - } - - /** - * Fetches values from the cache. - * - * @param iterable $keys - * @param mixed $default - * - * @return iterable - * - * @throws CacheArgumentException - */ - public function getMultiple( $keys, $default = null ) { - if ( !( is_array( $keys ) || $keys instanceof Traversable ) ) { - throw new CacheArgumentException( - 'given cache $keys is not an iterable' - ); - } - - $results = []; - $keyMap = []; - - foreach ( $keys as $key ) { - $this->assertValidKey( $key ); - $keyMap[ $this->qualifyKey( $key ) ] = $key; - $results[$key] = $default; - } - - $apcuResults = apcu_fetch( array_keys( $keyMap ), $ok ) ?: []; - - if ( $ok ) { - foreach ( $apcuResults as $qualifiedKey => $value ) { - $results[ $keyMap[ $qualifiedKey ] ] = $value; - } - } - - return $results; - } - - /** - * Checks for the presence of a given item in the cache. - * - * @param string $key - * - * @return bool - * - * @throws CacheArgumentException - */ - public function has( $key ): bool { - $this->assertValidKey( $key ); - - return apcu_exists( $this->qualifyKey( $key ) ); - } - - /** - * Store a value in the cache. - * - * Note that while object values are possible given the underlying APCU - * serializer, caching of objects directly should be avoided due to - * incompatibilities in serialized representation from PHP version to PHP - * version and when simple differences in member scope are made (private - * to protected, etc.). Wherever possible, convert objects to arrays - * before storing. - * - * @param string $key - * @param mixed $value - * @param null|int|DateInterval $ttl Optional. By default, the $ttl - * argument passed to the constructor will be used. - * - * @return bool True on success, false on failure. - * - * @throws CacheArgumentException - */ - public function set( $key, $value, $ttl = null ): bool { - return $this->setMultiple( [ $key => $value ], $ttl ); - } - - /** - * Store multiple values in the cache. - * - * @param iterable $values An iterable of key => value pairs. - * @param null|int|DateInterval $ttl Optional. By default, the $ttl - * argument passed to the constructor will be used. - * - * @return bool True on success, false on failure. - * - * @throws CacheArgumentException - */ - public function setMultiple( $values, $ttl = null ): bool { - if ( $ttl === null ) { - $ttl = $this->ttl; - } - - if ( !( is_array( $values ) || $values instanceof Traversable ) ) { - throw new CacheArgumentException( - 'given cache $values is not an iterable' - ); - } - - // To be compliant with the PSR-16 interface, we must accept a - // DateInterval TTL. Convert it to an int. - if ( $ttl instanceof DateInterval ) { - $ttl = ( new DateTime( '@0' ) )->add( $ttl )->getTimestamp(); - } - - $apcuValues = []; - - foreach ( $values as $key => $value ) { - $this->assertValidKey( $key ); - $apcuValues[ $this->qualifyKey( $key ) ] = $value; - } - - $failures = apcu_store( $apcuValues, null, $ttl ); - - return empty( $failures ); - } - - /** - * Fully qualifies a relative key, prefixing it with the internal - * namespace and cache implementation version. - * - * @param string $key Relative key value - * - * @return string - */ - private function qualifyKey( string $key ): string { - return $this->namespace . '@' . self::VERSION . ':' . $key; - } - - /** - * @param string $key - * - * @throws CacheArgumentException - */ - private function assertValidKey( $key ) { - if ( !is_string( $key ) ) { - throw new CacheArgumentException( 'key must be a string' ); - } - - if ( empty( $key ) ) { - throw new CacheArgumentException( 'key cannot be an empty string' ); - } - } -} diff --git a/includes/Settings/SettingsBuilder.php b/includes/Settings/SettingsBuilder.php index 1cc4a924384..bab084e4fcc 100644 --- a/includes/Settings/SettingsBuilder.php +++ b/includes/Settings/SettingsBuilder.php @@ -2,6 +2,7 @@ namespace MediaWiki\Settings; +use BagOStuff; use ExtensionRegistry; use JsonSchema\Constraints\Constraint; use JsonSchema\Validator; @@ -13,7 +14,6 @@ use MediaWiki\Settings\Config\PhpIniSink; use MediaWiki\Settings\Source\ArraySource; use MediaWiki\Settings\Source\FileSource; use MediaWiki\Settings\Source\SettingsSource; -use Psr\SimpleCache\CacheInterface; use StatusValue; /** @@ -27,7 +27,7 @@ class SettingsBuilder { /** @var ExtensionRegistry */ private $extensionRegistry; - /** @var CacheInterface */ + /** @var BagOStuff */ private $cache; /** @var ConfigBuilder */ @@ -54,17 +54,17 @@ class SettingsBuilder { * @param ExtensionRegistry $extensionRegistry * @param ConfigBuilder $configSink * @param PhpIniSink $phpIniSink - * @param CacheInterface|null $cache PSR-16 compliant cache interface used - * to cache settings loaded from each source. The caller should beware - * that secrets contained in any source passed to {@link load} or {@link - * loadFile} will be cached as well. + * @param BagOStuff|null $cache BagOStuff used to cache settings loaded + * from each source. The caller should beware that secrets contained in + * any source passed to {@link load} or {@link loadFile} will be cached as + * well. */ public function __construct( string $baseDir, ExtensionRegistry $extensionRegistry, ConfigBuilder $configSink, PhpIniSink $phpIniSink, - CacheInterface $cache = null + BagOStuff $cache = null ) { $this->baseDir = $baseDir; $this->extensionRegistry = $extensionRegistry; diff --git a/tests/common/TestsAutoLoader.php b/tests/common/TestsAutoLoader.php index 8d45d45604d..a06c4b4f522 100644 --- a/tests/common/TestsAutoLoader.php +++ b/tests/common/TestsAutoLoader.php @@ -268,9 +268,6 @@ $wgAutoloadClasses += [ 'MediaWiki\Tests\Unit\Revision\RevisionSlotsTest' => "$testDir/phpunit/unit/includes/Revision/RevisionSlotsTest.php", 'MediaWiki\Tests\Unit\Revision\RevisionStoreRecordTest' => "$testDir/phpunit/unit/includes/Revision/RevisionStoreRecordTest.php", - # tests/phpunit/unit/includes/Settings/Cache - 'MediaWiki\Tests\Unit\Settings\Cache\CacheInterfaceTestTrait' => "$testDir/phpunit/unit/includes/Settings/Cache/CacheInterfaceTestTrait.php", - # tests/phpunit/unit/includes/Settings/Config 'MediaWiki\Tests\Unit\Settings\Config\ConfigSinkTestTrait' => "$testDir/phpunit/unit/includes/Settings/Config/ConfigSinkTestTrait.php", diff --git a/tests/phpunit/unit/includes/Settings/Cache/ArrayCacheTest.php b/tests/phpunit/unit/includes/Settings/Cache/ArrayCacheTest.php deleted file mode 100644 index 28335a638e8..00000000000 --- a/tests/phpunit/unit/includes/Settings/Cache/ArrayCacheTest.php +++ /dev/null @@ -1,17 +0,0 @@ -newCache(); - - $cache->set( 'one', 'fish1' ); - $cache->set( 'two', 'fish2', 60 ); - $cache->set( 'red', 'fish3', DateInterval::createFromDateString( '@60' ) ); - - $this->assertSame( 'fish1', $cache->get( 'one' ) ); - $this->assertSame( 'fish2', $cache->get( 'two' ) ); - $this->assertSame( 'fish3', $cache->get( 'red' ) ); - $this->assertSame( 'fish4', $cache->get( 'blue', 'fish4' ) ); - } - - public function testSetAndGetMultiple() { - $cache = $this->newCache(); - - $cache->setMultiple( - [ - 'one' => 'fish1', - 'two' => 'fish2', - 'red' => 'fish3', - ] - ); - - $this->assertSame( - [ - 'one' => 'fish1', - 'two' => 'fish2', - 'red' => 'fish3', - 'blue' => 'fish4', - ], - $cache->getMultiple( [ 'one', 'two', 'red', 'blue' ], 'fish4' ) - ); - } - - public function testDeleteAndHas() { - $cache = $this->newCache(); - - $cache->set( 'red', 'fish' ); - $cache->set( 'blue', 'fish' ); - - $cache->delete( 'red' ); - - $this->assertFalse( $cache->has( 'red' ) ); - $this->assertTrue( $cache->has( 'blue' ) ); - } - - public function testDeleteMultiple() { - $cache = $this->newCache(); - - $cache->set( 'one', 'fish' ); - $cache->set( 'two', 'fish' ); - $cache->set( 'red', 'fish' ); - $cache->set( 'blue', 'fish' ); - - $cache->deleteMultiple( [ 'two', 'red' ] ); - - $this->assertTrue( $cache->has( 'one' ) ); - $this->assertFalse( $cache->has( 'two' ) ); - $this->assertFalse( $cache->has( 'red' ) ); - $this->assertTrue( $cache->has( 'blue' ) ); - } - - public function testClear() { - $cache = $this->newCache(); - - $cache->set( 'one', 'fish' ); - $cache->set( 'two', 'fish' ); - $cache->set( 'red', 'fish' ); - $cache->set( 'blue', 'fish' ); - - $this->assertTrue( $cache->has( 'one' ) ); - $this->assertTrue( $cache->has( 'two' ) ); - $this->assertTrue( $cache->has( 'red' ) ); - $this->assertTrue( $cache->has( 'blue' ) ); - - $cache->clear(); - - $this->assertFalse( $cache->has( 'one' ) ); - $this->assertFalse( $cache->has( 'two' ) ); - $this->assertFalse( $cache->has( 'red' ) ); - $this->assertFalse( $cache->has( 'blue' ) ); - } -} diff --git a/tests/phpunit/unit/includes/Settings/Cache/CachedSourceTest.php b/tests/phpunit/unit/includes/Settings/Cache/CachedSourceTest.php index c884efd668a..d6e9386632c 100644 --- a/tests/phpunit/unit/includes/Settings/Cache/CachedSourceTest.php +++ b/tests/phpunit/unit/includes/Settings/Cache/CachedSourceTest.php @@ -2,34 +2,41 @@ namespace MediaWiki\Tests\Unit\Settings\Cache; +use BagOStuff; use MediaWiki\Settings\Cache\CacheableSource; use MediaWiki\Settings\Cache\CachedSource; use PHPUnit\Framework\TestCase; -use Psr\SimpleCache\CacheInterface; /** * @covers \MediaWiki\Settings\Cache\CachedSource */ class CachedSourceTest extends TestCase { public function testLoadWithMiss() { - $cache = $this->createMock( CacheInterface::class ); + $cache = $this->createMock( BagOStuff::class ); $source = $this->createMock( CacheableSource::class ); $cacheSource = new CachedSource( $cache, $source ); $settings = [ 'config' => [ 'Foo' => 'value' ] ]; - $key = 'abc123'; + $hashKey = 'abc123'; + $key = 'global:MediaWiki\Tests\Unit\Settings\Cache\CachedSourceTest:' . $hashKey; $ttl = 123; $source ->expects( $this->once() ) ->method( 'getHashKey' ) + ->willReturn( $hashKey ); + + $cache + ->expects( $this->once() ) + ->method( 'makeGlobalKey' ) + ->with( 'MediaWiki\Settings\Cache\CachedSource', $hashKey ) ->willReturn( $key ); $cache ->expects( $this->once() ) ->method( 'get' ) - ->with( $key, null ) - ->willReturn( null ); + ->with( $key ) + ->willReturn( false ); $source ->expects( $this->atLeastOnce() ) @@ -65,22 +72,29 @@ class CachedSourceTest extends TestCase { } public function testLoadWithHit() { - $cache = $this->createMock( CacheInterface::class ); + $cache = $this->createMock( BagOStuff::class ); $source = $this->createMock( CacheableSource::class ); $cacheSource = new CachedSource( $cache, $source ); $settings = [ 'config' => [ 'Foo' => 'value' ] ]; - $key = 'abc123'; + $hashKey = 'abc123'; + $key = 'global:MediaWiki\Tests\Unit\Settings\Cache\CachedSourceTest:' . $hashKey; $source ->expects( $this->once() ) ->method( 'getHashKey' ) - ->willReturn( 'abc123' ); + ->willReturn( $hashKey ); + + $cache + ->expects( $this->once() ) + ->method( 'makeGlobalKey' ) + ->with( 'MediaWiki\Settings\Cache\CachedSource', $hashKey ) + ->willReturn( $key ); $cache ->expects( $this->once() ) ->method( 'get' ) - ->with( $key, null ) + ->with( $key ) ->willReturn( [ 'value' => $settings, diff --git a/tests/phpunit/unit/includes/Settings/Cache/SharedMemoryCacheTest.php b/tests/phpunit/unit/includes/Settings/Cache/SharedMemoryCacheTest.php deleted file mode 100644 index 3f4798198fe..00000000000 --- a/tests/phpunit/unit/includes/Settings/Cache/SharedMemoryCacheTest.php +++ /dev/null @@ -1,63 +0,0 @@ -markTestSkipped( - 'skipped shared memory cache tests. set apc.enable_cli=on to run them' - ); - } - } - - public function tearDown(): void { - if ( SharedMemoryCache::isSupported() && ini_get( 'apc.enable_cli' ) ) { - $this->newCache()->clear(); - } - } - - public function testSetWithZeroLengthKey() { - $cache = $this->newCache(); - - $this->expectException( CacheArgumentException::class ); - - $cache->set( '', 'foo' ); - } - - public function testNamespacing() { - $cache1 = new SharedMemoryCache( 'phpunit.mediawiki.settings.one' ); - $cache2 = new SharedMemoryCache( 'phpunit.mediawiki.settings.two' ); - - $cache1->set( 'one', 'fish' ); - $cache1->set( 'two', 'fish' ); - $cache2->set( 'red', 'fish' ); - $cache2->set( 'blue', 'fish' ); - - try { - $cache1->clear(); - - $this->assertFalse( $cache1->has( 'one' ) ); - $this->assertFalse( $cache1->has( 'two' ) ); - - $this->assertTrue( $cache2->has( 'red' ) ); - $this->assertTrue( $cache2->has( 'blue' ) ); - - } finally { - $cache2->clear(); - } - } -} diff --git a/tests/phpunit/unit/includes/Settings/SettingsBuilderTest.php b/tests/phpunit/unit/includes/Settings/SettingsBuilderTest.php index 43cfeb4d431..7ab65bc14a9 100644 --- a/tests/phpunit/unit/includes/Settings/SettingsBuilderTest.php +++ b/tests/phpunit/unit/includes/Settings/SettingsBuilderTest.php @@ -2,6 +2,7 @@ namespace phpunit\unit\includes\Settings; +use BagOStuff; use ExtensionRegistry; use InvalidArgumentException; use MediaWiki\Settings\Cache\CacheableSource; @@ -11,7 +12,6 @@ use MediaWiki\Settings\Config\PhpIniSink; use MediaWiki\Settings\SettingsBuilder; use MediaWiki\Settings\SettingsBuilderException; use PHPUnit\Framework\TestCase; -use Psr\SimpleCache\CacheInterface; /** * @covers \MediaWiki\Settings\SettingsBuilder @@ -308,7 +308,7 @@ class SettingsBuilderTest extends TestCase { public function testLoadsCacheableSource() { $mockSource = $this->createMock( CacheableSource::class ); - $mockCache = $this->createMock( CacheInterface::class ); + $mockCache = $this->createMock( BagOStuff::class ); $configBuilder = new ArrayConfigBuilder(); $builder = $this ->newSettingsBuilder( [ @@ -317,17 +317,26 @@ class SettingsBuilderTest extends TestCase { ] ) ->load( $mockSource ); + $hashKey = 'abc123'; + $key = 'global:MediaWiki\Tests\Unit\Settings\Cache\CachedSourceTest:' . $hashKey; + // Mock a cache miss $mockSource ->expects( $this->once() ) ->method( 'getHashKey' ) - ->willReturn( 'abc123' ); + ->willReturn( $hashKey ); + + $mockCache + ->expects( $this->once() ) + ->method( 'makeGlobalKey' ) + ->with( 'MediaWiki\Settings\Cache\CachedSource', $hashKey ) + ->willReturn( $key ); $mockCache ->expects( $this->once() ) ->method( 'get' ) - ->with( 'abc123' ) - ->willReturn( null ); + ->with( $key ) + ->willReturn( false ); $mockSource ->expects( $this->once() )