Make WikiMap::isCurrentWikiDbDomain() more rigorous
authorAaron Schulz <aschulz@wikimedia.org>
Thu, 28 Mar 2019 22:37:52 +0000 (15:37 -0700)
committerAaron Schulz <aschulz@wikimedia.org>
Fri, 29 Mar 2019 19:03:28 +0000 (19:03 +0000)
This already requires a DB domain ID, so there is no reason to have
hacks for trying to handle a wiki ID being passed in instead. If the
provided domain has a schema, it should not simply be ignored in the
comparison.

Change-Id: I9ced7a46fa05f32843a9a7d17391c5d0576b099c

includes/WikiMap.php
includes/jobqueue/JobQueue.php
tests/phpunit/includes/WikiMapTest.php
tests/phpunit/includes/jobqueue/JobQueueMemoryTest.php
tests/phpunit/includes/jobqueue/JobQueueTest.php

index dbad4b0..8b000f2 100644 (file)
@@ -245,8 +245,12 @@ class WikiMap {
        /**
         * Get the wiki ID of a database domain
         *
-        * This is like DatabaseDomain::getId() without encoding (for legacy reasons)
-        * and without the schema if it merely set to the generic value "mediawiki"
+        * This is like DatabaseDomain::getId() without encoding (for legacy reasons) and
+        * without the schema if it is the generic installer default of "mediawiki"/"dbo"
+        *
+        * @see $wgDBmwschema
+        * @see PostgresInstaller
+        * @see MssqlInstaller
         *
         * @param string|DatabaseDomain $domain
         * @return string
@@ -254,15 +258,17 @@ class WikiMap {
         */
        public static function getWikiIdFromDbDomain( $domain ) {
                $domain = DatabaseDomain::newFromId( $domain );
-
+               // Since the schema was not always part of the wiki ID, try to maintain backwards
+               // compatibility with some common cases. Assume that if the DB domain schema is just
+               // the installer default then it is probably the case that the schema is the same for
+               // all wikis in the farm. Historically, any wiki farm had to make the database/prefix
+               // combination unique per wiki. Ommit the schema if it does not seem wiki specific.
                if ( !in_array( $domain->getSchema(), [ null, 'mediawiki', 'dbo' ], true ) ) {
-                       // Include the schema if it is set and is not the default placeholder.
                        // This means a site admin may have specifically taylored the schemas.
-                       // Domain IDs might use the form <DB>-<project>-<language>, meaning that
-                       // the schema portion must be accounted for to disambiguate wikis.
+                       // Domain IDs might use the form <DB>-<project>- or <DB>-<project>-<language>_,
+                       // meaning that the schema portion must be accounted for to disambiguate wikis.
                        return "{$domain->getDatabase()}-{$domain->getSchema()}-{$domain->getTablePrefix()}";
                }
-
                // Note that if this wiki ID is passed a a domain ID to LoadBalancer, then it can
                // handle the schema by assuming the generic "mediawiki" schema if needed.
                return strlen( $domain->getTablePrefix() )
@@ -291,30 +297,16 @@ class WikiMap {
 
        /**
         * @param DatabaseDomain|string $domain
-        * @return bool Whether $domain has the same DB/prefix as the current wiki
+        * @return bool Whether $domain matches the DB domain of the current wiki
         * @since 1.33
         */
        public static function isCurrentWikiDbDomain( $domain ) {
-               $domain = DatabaseDomain::newFromId( $domain );
-               $curDomain = self::getCurrentWikiDbDomain();
-
-               if ( !in_array( $curDomain->getSchema(), [ null, 'mediawiki', 'dbo' ], true ) ) {
-                       // Include the schema if it is set and is not the default placeholder.
-                       // This means a site admin may have specifically taylored the schemas.
-                       // Domain IDs might use the form <DB>-<project>-<language>, meaning that
-                       // the schema portion must be accounted for to disambiguate wikis.
-                       return $curDomain->equals( $domain );
-               }
-
-               return (
-                       $curDomain->getDatabase() === $domain->getDatabase() &&
-                       $curDomain->getTablePrefix() === $domain->getTablePrefix()
-               );
+               return self::getCurrentWikiDbDomain()->equals( DatabaseDomain::newFromId( $domain ) );
        }
 
        /**
         * @param string $wikiId
-        * @return bool Whether $wikiId has the same DB/prefix as the current wiki
+        * @return bool Whether $wikiId matches the wiki ID of the current wiki
         * @since 1.33
         */
        public static function isCurrentWikiId( $wikiId ) {
index 660352a..fdcd65c 100644 (file)
@@ -126,7 +126,7 @@ abstract class JobQueue {
         * @deprecated 1.33
         */
        final public function getWiki() {
-               return $this->domain;
+               return WikiMap::getWikiIdFromDbDomain( $this->domain );
        }
 
        /**
index df05671..1608b9c 100644 (file)
@@ -260,16 +260,19 @@ class WikiMapTest extends MediaWikiLangTestCase {
         * @covers WikiMap::getCurrentWikiDbDomain()
         */
        public function testIsCurrentWikiDomain() {
-               $this->assertTrue( WikiMap::isCurrentWikiDbDomain( wfWikiID() ) );
+               $this->setMwGlobals( 'wgDBmwschema', 'mediawiki' );
 
-               $localDomain = DatabaseDomain::newFromId( wfWikiID() );
+               $localDomain = WikiMap::getCurrentWikiDbDomain()->getId();
+               $this->assertTrue( WikiMap::isCurrentWikiDbDomain( $localDomain ) );
+
+               $localDomain = DatabaseDomain::newFromId( $localDomain );
                $domain1 = new DatabaseDomain(
                        $localDomain->getDatabase(), 'someschema', $localDomain->getTablePrefix() );
                $domain2 = new DatabaseDomain(
                        $localDomain->getDatabase(), null, $localDomain->getTablePrefix() );
 
-               $this->assertTrue( WikiMap::isCurrentWikiDbDomain( $domain1 ), 'Schema ignored' );
-               $this->assertTrue( WikiMap::isCurrentWikiDbDomain( $domain2 ), 'Schema ignored' );
+               $this->assertFalse( WikiMap::isCurrentWikiDbDomain( $domain1 ), 'Schema not ignored' );
+               $this->assertFalse( WikiMap::isCurrentWikiDbDomain( $domain2 ), 'Null schema not ignored' );
 
                $this->assertTrue( WikiMap::isCurrentWikiDbDomain( WikiMap::getCurrentWikiDbDomain() ) );
        }
index b2e7ea4..232b46a 100644 (file)
@@ -18,7 +18,7 @@ class JobQueueMemoryTest extends PHPUnit\Framework\TestCase {
        private function newJobQueue() {
                return JobQueue::factory( [
                        'class' => JobQueueMemory::class,
-                       'wiki' => wfWikiID(),
+                       'domain' => WikiMap::getCurrentWikiDbDomain()->getId(),
                        'type' => 'null',
                ] );
        }
index 0421fe7..d38c6c7 100644 (file)
@@ -31,7 +31,7 @@ class JobQueueTest extends MediaWikiTestCase {
                        $baseConfig = [ 'class' => JobQueueDBSingle::class ];
                }
                $baseConfig['type'] = 'null';
-               $baseConfig['wiki'] = wfWikiID();
+               $baseConfig['domain'] = WikiMap::getCurrentWikiDbDomain()->getId();
                $variants = [
                        'queueRand' => [ 'order' => 'random', 'claimTTL' => 0 ],
                        'queueRandTTL' => [ 'order' => 'random', 'claimTTL' => 10 ],
@@ -75,7 +75,10 @@ class JobQueueTest extends MediaWikiTestCase {
                        $this->markTestSkipped( $desc );
                }
                $this->assertEquals( wfWikiID(), $queue->getWiki(), "Proper wiki ID ($desc)" );
-               $this->assertEquals( wfWikiID(), $queue->getDomain(), "Proper wiki ID ($desc)" );
+               $this->assertEquals(
+                       WikiMap::getCurrentWikiDbDomain()->getId(),
+                       $queue->getDomain(),
+                       "Proper wiki ID ($desc)" );
        }
 
        /**