From 5a6c18a0863fad234a513bcd61a2076f28433d7e Mon Sep 17 00:00:00 2001 From: Aryeh Gregor Date: Thu, 15 Aug 2019 21:07:36 +0300 Subject: [PATCH] LockManagerGroupFactory to replace singletons 100% test coverage of code that appears to be working and used, in both LockManagerGroupFactory and also LockManagerGroup. Where possible I wrote it as unit tests. One preexisting code path seems to be broken and I marked the test as skipped. Two methods look unused and perhaps not especially helpful, so I didn't write tests for them yet in case we want to just get rid of them instead. Change-Id: Iaa7354f31c451b87773468609c674a3bf1d4382f --- autoload.php | 1 + includes/ForkController.php | 1 - includes/MediaWikiServices.php | 9 ++ includes/ServiceWiring.php | 9 ++ .../lockmanager/LockManagerGroup.php | 90 +++++++++---------- .../lockmanager/LockManagerGroupFactory.php | 54 +++++++++++ tests/parser/ParserTestRunner.php | 2 +- .../LockManagerGroupFactoryTest.php | 34 +++++++ .../lockmanager/LockManagerGroupTest.php | 88 ++++++++++++++++++ 9 files changed, 238 insertions(+), 50 deletions(-) create mode 100644 includes/filebackend/lockmanager/LockManagerGroupFactory.php create mode 100644 tests/phpunit/unit/includes/filebackend/lockmanager/LockManagerGroupFactoryTest.php create mode 100644 tests/phpunit/unit/includes/filebackend/lockmanager/LockManagerGroupTest.php diff --git a/autoload.php b/autoload.php index 95297fbc64..cc65f09748 100644 --- a/autoload.php +++ b/autoload.php @@ -882,6 +882,7 @@ $wgAutoloadLocalClasses = [ 'MediaWiki\\Diff\\ComplexityException' => __DIR__ . '/includes/diff/ComplexityException.php', 'MediaWiki\\Diff\\WordAccumulator' => __DIR__ . '/includes/diff/WordAccumulator.php', 'MediaWiki\\FileBackend\\FSFile\\TempFSFileFactory' => __DIR__ . '/includes/libs/filebackend/fsfile/TempFSFileFactory.php', + 'MediaWiki\\FileBackend\\LockManager\\LockManagerGroupFactory' => __DIR__ . '/includes/filebackend/lockmanager/LockManagerGroupFactory.php', 'MediaWiki\\HeaderCallback' => __DIR__ . '/includes/HeaderCallback.php', 'MediaWiki\\Http\\HttpRequestFactory' => __DIR__ . '/includes/http/HttpRequestFactory.php', 'MediaWiki\\Installer\\InstallException' => __DIR__ . '/includes/installer/InstallException.php', diff --git a/includes/ForkController.php b/includes/ForkController.php index 85f3a7dc63..af06a88982 100644 --- a/includes/ForkController.php +++ b/includes/ForkController.php @@ -154,7 +154,6 @@ class ForkController { // Don't share DB, storage, or memcached connections MediaWikiServices::resetChildProcessServices(); FileBackendGroup::destroySingleton(); - LockManagerGroup::destroySingletons(); JobQueueGroup::destroySingletons(); ObjectCache::clear(); RedisConnectionPool::destroySingletons(); diff --git a/includes/MediaWikiServices.php b/includes/MediaWikiServices.php index 6013aafc9e..3b80e58444 100644 --- a/includes/MediaWikiServices.php +++ b/includes/MediaWikiServices.php @@ -17,6 +17,7 @@ use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface; use MediaWiki\Block\BlockManager; use MediaWiki\Block\BlockRestrictionStore; use MediaWiki\FileBackend\FSFile\TempFSFileFactory; +use MediaWiki\FileBackend\LockManager\LockManagerGroupFactory; use MediaWiki\Http\HttpRequestFactory; use MediaWiki\Page\MovePageFactory; use MediaWiki\Permissions\PermissionManager; @@ -658,6 +659,14 @@ class MediaWikiServices extends ServiceContainer { return $this->getService( 'LocalServerObjectCache' ); } + /** + * @since 1.34 + * @return LockManagerGroupFactory + */ + public function getLockManagerGroupFactory() : LockManagerGroupFactory { + return $this->getService( 'LockManagerGroupFactory' ); + } + /** * @since 1.32 * @return MagicWordFactory diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index b30726415e..cc68cb6485 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -45,6 +45,7 @@ use MediaWiki\Block\BlockRestrictionStore; use MediaWiki\Config\ConfigRepository; use MediaWiki\Config\ServiceOptions; use MediaWiki\FileBackend\FSFile\TempFSFileFactory; +use MediaWiki\FileBackend\LockManager\LockManagerGroupFactory; use MediaWiki\Http\HttpRequestFactory; use MediaWiki\Interwiki\ClassicInterwikiLookup; use MediaWiki\Interwiki\InterwikiLookup; @@ -289,6 +290,14 @@ return [ return \ObjectCache::newFromParams( $config->get( 'ObjectCaches' )[$cacheId] ); }, + 'LockManagerGroupFactory' => function ( MediaWikiServices $services ) : LockManagerGroupFactory { + return new LockManagerGroupFactory( + WikiMap::getCurrentWikiDbDomain()->getId(), + $services->getMainConfig()->get( 'LockManagers' ), + $services->getDBLoadBalancerFactory() + ); + }, + 'MagicWordFactory' => function ( MediaWikiServices $services ) : MagicWordFactory { return new MagicWordFactory( $services->getContentLanguage() ); }, diff --git a/includes/filebackend/lockmanager/LockManagerGroup.php b/includes/filebackend/lockmanager/LockManagerGroup.php index 957af3e4ae..7da3753c9b 100644 --- a/includes/filebackend/lockmanager/LockManagerGroup.php +++ b/includes/filebackend/lockmanager/LockManagerGroup.php @@ -22,6 +22,7 @@ */ use MediaWiki\MediaWikiServices; use MediaWiki\Logger\LoggerFactory; +use Wikimedia\Rdbms\LBFactory; /** * Class to handle file lock manager registration @@ -30,62 +31,27 @@ use MediaWiki\Logger\LoggerFactory; * @since 1.19 */ class LockManagerGroup { - /** @var LockManagerGroup[] (domain => LockManagerGroup) */ - protected static $instances = []; + /** @var string domain (usually wiki ID) */ + protected $domain; - protected $domain; // string; domain (usually wiki ID) + /** @var LBFactory */ + protected $lbFactory; /** @var array Array of (name => ('class' => ..., 'config' => ..., 'instance' => ...)) */ protected $managers = []; /** + * Do not call this directly. Use LockManagerGroupFactory. + * * @param string $domain Domain (usually wiki ID) + * @param array[] $lockManagerConfigs In format of $wgLockManagers + * @param LBFactory $lbFactory */ - protected function __construct( $domain ) { + public function __construct( $domain, array $lockManagerConfigs, LBFactory $lbFactory ) { $this->domain = $domain; - } - - /** - * @param bool|string $domain Domain (usually wiki ID). Default: false. - * @return LockManagerGroup - */ - public static function singleton( $domain = false ) { - if ( $domain === false ) { - $domain = WikiMap::getCurrentWikiDbDomain()->getId(); - } + $this->lbFactory = $lbFactory; - if ( !isset( self::$instances[$domain] ) ) { - self::$instances[$domain] = new self( $domain ); - self::$instances[$domain]->initFromGlobals(); - } - - return self::$instances[$domain]; - } - - /** - * Destroy the singleton instances - */ - public static function destroySingletons() { - self::$instances = []; - } - - /** - * Register lock managers from the global variables - */ - protected function initFromGlobals() { - global $wgLockManagers; - - $this->register( $wgLockManagers ); - } - - /** - * Register an array of file lock manager configurations - * - * @param array $configs - * @throws Exception - */ - protected function register( array $configs ) { - foreach ( $configs as $config ) { + foreach ( $lockManagerConfigs as $config ) { $config['domain'] = $this->domain; if ( !isset( $config['name'] ) ) { throw new Exception( "Cannot register a lock manager with no name." ); @@ -104,6 +70,26 @@ class LockManagerGroup { } } + /** + * @deprecated since 1.34, use LockManagerGroupFactory + * + * @param bool|string $domain Domain (usually wiki ID). Default: false. + * @return LockManagerGroup + */ + public static function singleton( $domain = false ) { + return MediaWikiServices::getInstance()->getLockManagerGroupFactory() + ->getLockManagerGroup( $domain ); + } + + /** + * Destroy the singleton instances + * + * @deprecated since 1.34, use resetServiceForTesting() on LockManagerGroupFactory + */ + public static function destroySingletons() { + MediaWikiServices::getInstance()->resetServiceForTesting( 'LockManagerGroupFactory' ); + } + /** * Get the lock manager object with a given name * @@ -120,8 +106,7 @@ class LockManagerGroup { $class = $this->managers[$name]['class']; $config = $this->managers[$name]['config']; if ( $class === DBLockManager::class ) { - $lbFactory = MediaWikiServices::getInstance()->getDBLoadBalancerFactory(); - $lb = $lbFactory->getMainLB( $config['domain'] ); + $lb = $this->lbFactory->getMainLB( $config['domain'] ); $config['dbServers']['localDBMaster'] = $lb->getLazyConnectionRef( DB_MASTER, [], @@ -132,6 +117,11 @@ class LockManagerGroup { } $config['logger'] = LoggerFactory::getInstance( 'LockManager' ); + // XXX Looks like phan is right, we are trying to instantiate an abstract class and it + // throws. Did this ever work? Presumably we need to detect the right subclass? Or + // should we just get rid of this? It looks like it never worked since it was first + // introduced by 0cf832a3394 in 2016, so if no one's complained until now, clearly it + // can't be very useful? // @phan-suppress-next-line PhanTypeInstantiateAbstract $this->managers[$name]['instance'] = new $class( $config ); } @@ -159,6 +149,8 @@ class LockManagerGroup { * Get the default lock manager configured for the site. * Returns NullLockManager if no lock manager could be found. * + * XXX This looks unused, should we just get rid of it? + * * @return LockManager */ public function getDefault() { @@ -172,6 +164,8 @@ class LockManagerGroup { * or at least some other effective configured lock manager. * Throws an exception if no lock manager could be found. * + * XXX This looks unused, should we just get rid of it? + * * @return LockManager * @throws Exception */ diff --git a/includes/filebackend/lockmanager/LockManagerGroupFactory.php b/includes/filebackend/lockmanager/LockManagerGroupFactory.php new file mode 100644 index 0000000000..54bbc3d17c --- /dev/null +++ b/includes/filebackend/lockmanager/LockManagerGroupFactory.php @@ -0,0 +1,54 @@ + LockManagerGroup) */ + private $instances = []; + + /** + * Do not call directly, use MediaWikiServices. + * + * @param string $defaultDomain + * @param array $lockManagerConfigs In format of $wgLockManagers + * @param LBFactory $lbFactory + */ + public function __construct( $defaultDomain, array $lockManagerConfigs, LBFactory $lbFactory ) { + $this->defaultDomain = $defaultDomain; + $this->lockManagerConfigs = $lockManagerConfigs; + $this->lbFactory = $lbFactory; + } + + /** + * @param string|bool $domain Domain (usually wiki ID). false for the default (normally the + * current wiki's domain). + * @return LockManagerGroup + */ + public function getLockManagerGroup( $domain = false ) : LockManagerGroup { + if ( $domain === false ) { + $domain = $this->defaultDomain; + } + + if ( !isset( $this->instances[$domain] ) ) { + $this->instances[$domain] = + new LockManagerGroup( $domain, $this->lockManagerConfigs, $this->lbFactory ); + } + + return $this->instances[$domain]; + } +} diff --git a/tests/parser/ParserTestRunner.php b/tests/parser/ParserTestRunner.php index 36c3fe20b0..c8b8ef92a1 100644 --- a/tests/parser/ParserTestRunner.php +++ b/tests/parser/ParserTestRunner.php @@ -313,7 +313,7 @@ class ParserTestRunner { 'class' => NullLockManager::class, ] ]; $reset = function () { - LockManagerGroup::destroySingletons(); + MediaWikiServices::getInstance()->resetServiceForTesting( 'LockManagerGroupFactory' ); }; $setup[] = $reset; $teardown[] = $reset; diff --git a/tests/phpunit/unit/includes/filebackend/lockmanager/LockManagerGroupFactoryTest.php b/tests/phpunit/unit/includes/filebackend/lockmanager/LockManagerGroupFactoryTest.php new file mode 100644 index 0000000000..38fcf29363 --- /dev/null +++ b/tests/phpunit/unit/includes/filebackend/lockmanager/LockManagerGroupFactoryTest.php @@ -0,0 +1,34 @@ +createMock( LBFactory::class ); + $mockLbFactory->expects( $this->never() )->method( $this->anything() ); + + $factory = new LockManagerGroupFactory( 'defaultDomain', [], $mockLbFactory ); + $lbmUnspecified = $factory->getLockManagerGroup(); + $lbmFalse = $factory->getLockManagerGroup( false ); + $lbmDefault = $factory->getLockManagerGroup( 'defaultDomain' ); + $lbmOther = $factory->getLockManagerGroup( 'otherDomain' ); + + $this->assertSame( $lbmUnspecified, $lbmFalse ); + $this->assertSame( $lbmFalse, $lbmDefault ); + $this->assertSame( $lbmDefault, $lbmUnspecified ); + $this->assertNotEquals( $lbmUnspecified, $lbmOther ); + $this->assertNotEquals( $lbmFalse, $lbmOther ); + $this->assertNotEquals( $lbmDefault, $lbmOther ); + + $this->assertSame( $lbmUnspecified, $factory->getLockManagerGroup() ); + $this->assertSame( $lbmFalse, $factory->getLockManagerGroup( false ) ); + $this->assertSame( $lbmDefault, $factory->getLockManagerGroup( 'defaultDomain' ) ); + $this->assertSame( $lbmOther, $factory->getLockManagerGroup( 'otherDomain' ) ); + } +} diff --git a/tests/phpunit/unit/includes/filebackend/lockmanager/LockManagerGroupTest.php b/tests/phpunit/unit/includes/filebackend/lockmanager/LockManagerGroupTest.php new file mode 100644 index 0000000000..79baac9a17 --- /dev/null +++ b/tests/phpunit/unit/includes/filebackend/lockmanager/LockManagerGroupTest.php @@ -0,0 +1,88 @@ +createMock( LBFactory::class ); + $mock->expects( $this->never() )->method( $this->anythingBut( '__destruct' ) ); + return $mock; + } + + public function testConstructorNoConfigs() { + new LockManagerGroup( 'domain', [], $this->getMockLBFactory() ); + $this->assertTrue( true, 'No exception thrown' ); + } + + public function testConstructorConfigWithNoName() { + $this->setExpectedException( Exception::class, + 'Cannot register a lock manager with no name.' ); + + new LockManagerGroup( 'domain', + [ [ 'name' => 'a', 'class' => 'b' ], [ 'class' => 'c' ] ], $this->getMockLBFactory() ); + } + + public function testConstructorConfigWithNoClass() { + $this->setExpectedException( Exception::class, + 'Cannot register lock manager `c` with no class.' ); + + new LockManagerGroup( 'domain', + [ [ 'name' => 'a', 'class' => 'b' ], [ 'name' => 'c' ] ], $this->getMockLBFactory() ); + } + + public function testGetUndefined() { + $this->setExpectedException( Exception::class, + 'No lock manager defined with the name `c`.' ); + + $lmg = new LockManagerGroup( 'domain', [ [ 'name' => 'a', 'class' => 'b' ] ], + $this->getMockLBFactory() ); + $lmg->get( 'c' ); + } + + public function testConfigUndefined() { + $this->setExpectedException( Exception::class, + 'No lock manager defined with the name `c`.' ); + + $lmg = new LockManagerGroup( 'domain', [ [ 'name' => 'a', 'class' => 'b' ] ], + $this->getMockLBFactory() ); + $lmg->config( 'c' ); + } + + public function testConfig() { + $lmg = new LockManagerGroup( 'domain', [ [ 'name' => 'a', 'class' => 'b', 'foo' => 'c' ] ], + $this->getMockLBFactory() ); + $this->assertSame( + [ 'class' => 'b', 'name' => 'a', 'foo' => 'c', 'domain' => 'domain' ], + $lmg->config( 'a' ) + ); + } + + public function testGetDefaultNull() { + $lmg = new LockManagerGroup( 'domain', [], $this->getMockLBFactory() ); + $expected = new NullLockManager( [] ); + $actual = $lmg->getDefault(); + // Have to get rid of the $sessions for equality check to work + TestingAccessWrapper::newFromObject( $actual )->session = null; + TestingAccessWrapper::newFromObject( $expected )->session = null; + $this->assertEquals( $expected, $actual ); + } + + public function testGetAnyException() { + // XXX Isn't the name 'getAny' misleading if we don't get whatever's available? + $this->setExpectedException( Exception::class, + 'No lock manager defined with the name `fsLockManager`.' ); + + $lmg = new LockManagerGroup( 'domain', [ [ 'name' => 'a', 'class' => 'b' ] ], + $this->getMockLBFactory() ); + $lmg->getAny(); + } +} -- 2.20.1