LockManagerGroupFactory to replace singletons
authorAryeh Gregor <ayg@aryeh.name>
Thu, 15 Aug 2019 18:07:36 +0000 (21:07 +0300)
committerAryeh Gregor <ayg@aryeh.name>
Tue, 27 Aug 2019 10:19:15 +0000 (13:19 +0300)
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
includes/ForkController.php
includes/MediaWikiServices.php
includes/ServiceWiring.php
includes/filebackend/lockmanager/LockManagerGroup.php
includes/filebackend/lockmanager/LockManagerGroupFactory.php [new file with mode: 0644]
tests/parser/ParserTestRunner.php
tests/phpunit/unit/includes/filebackend/lockmanager/LockManagerGroupFactoryTest.php [new file with mode: 0644]
tests/phpunit/unit/includes/filebackend/lockmanager/LockManagerGroupTest.php [new file with mode: 0644]

index 95297fb..cc65f09 100644 (file)
@@ -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',
index 85f3a7d..af06a88 100644 (file)
@@ -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();
index 6013aaf..3b80e58 100644 (file)
@@ -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
index b307264..cc68cb6 100644 (file)
@@ -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() );
        },
index 957af3e..7da3753 100644 (file)
@@ -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 (file)
index 0000000..54bbc3d
--- /dev/null
@@ -0,0 +1,54 @@
+<?php
+
+namespace MediaWiki\FileBackend\LockManager;
+
+use LockManagerGroup;
+use Wikimedia\Rdbms\LBFactory;
+
+/**
+ * Service to construct LockManagerGroups.
+ */
+class LockManagerGroupFactory {
+       /** @var string */
+       private $defaultDomain;
+
+       /** @var array */
+       private $lockManagerConfigs;
+
+       /** @var LBFactory */
+       private $lbFactory;
+
+       /** @var LockManagerGroup[] (domain => 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];
+       }
+}
index 36c3fe2..c8b8ef9 100644 (file)
@@ -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 (file)
index 0000000..38fcf29
--- /dev/null
@@ -0,0 +1,34 @@
+<?php
+
+use MediaWiki\FileBackend\LockManager\LockManagerGroupFactory;
+use Wikimedia\Rdbms\LBFactory;
+
+/**
+ * @covers MediaWiki\FileBackend\LockManager\LockManagerGroupFactory
+ * @todo Should we somehow test that the LockManagerGroup objects are as we expect? How do we do
+ *   that without getting into testing LockManagerGroup itself?
+ */
+class LockManagerGroupFactoryTest extends MediaWikiUnitTestCase {
+       public function testGetLockManagerGroup() {
+               $mockLbFactory = $this->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 (file)
index 0000000..79baac9
--- /dev/null
@@ -0,0 +1,88 @@
+<?php
+
+use Wikimedia\Rdbms\LBFactory;
+use Wikimedia\TestingAccessWrapper;
+
+/**
+ * Since this is a unit test, we don't test the singleton() or destroySingletons() methods. We also
+ * can't test get() with a valid argument, because that winds up calling static methods of
+ * ObjectCache and LoggerFactory that aren't yet compatible with proper unit tests. Those will be
+ * tested in the integration test for now.
+ *
+ * @covers LockManagerGroup
+ */
+class LockManagerGroupTest extends MediaWikiUnitTestCase {
+       private function getMockLBFactory() {
+               $mock = $this->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();
+       }
+}