Merge "rdbms: specify DB name and table prefix even for the local domain"

diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
index e80b952..b01b23f 100644 (file)
--- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php
+++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
@@ -146,17 +146,10 @@ class LoadBalancer implements ILoadBalancer {
                        }
                }
 
-               $this->localDomain = isset( $params['localDomain'] )
+               $localDomain = isset( $params['localDomain'] )
                        ? DatabaseDomain::newFromId( $params['localDomain'] )
                        : DatabaseDomain::newUnspecified();
-               // In case a caller assumes that the domain ID is simply <db>-<prefix>, which is almost
-               // always true, gracefully handle the case when they fail to account for escaping.
-               if ( $this->localDomain->getTablePrefix() != '' ) {
-                       $this->localDomainIdAlias =
-                               $this->localDomain->getDatabase() . '-' . $this->localDomain->getTablePrefix();
-               } else {
-                       $this->localDomainIdAlias = $this->localDomain->getDatabase();
-               }
+               $this->setLocalDomain( $localDomain );
 
                $this->mWaitTimeout = isset( $params['waitTimeout'] ) ? $params['waitTimeout'] : 10;
 
@@ -821,7 +814,11 @@ class LoadBalancer implements ILoadBalancer {
                                $server = $this->mServers[$i];
                                $server['serverIndex'] = $i;
                                $server['autoCommitOnly'] = $autoCommit;
-                               $conn = $this->reallyOpenConnection( $server, false );
+                               if ( $this->localDomain->getDatabase() !== null ) {
+                                       // Use the local domain table prefix if the local domain is specified
+                                       $server['tablePrefix'] = $this->localDomain->getTablePrefix();
+                               }
+                               $conn = $this->reallyOpenConnection( $server, $this->localDomain->getDatabase() );
                                $host = $this->getServerName( $i );
                                if ( $conn->isOpen() ) {
                                        $this->connLogger->debug( "Connected to database $i at '$host'." );
@@ -899,8 +896,6 @@ class LoadBalancer implements ILoadBalancer {
                        // Reuse a free connection from another domain
                        $conn = reset( $this->mConns[$connFreeKey][$i] );
                        $oldDomain = key( $this->mConns[$connFreeKey][$i] );
-                       // The empty string as a DB name means "don't care".
-                       // DatabaseMysqlBase::open() already handle this on connection.
                        if ( strlen( $dbName ) && !$conn->selectDB( $dbName ) ) {
                                $this->mLastError = "Error selecting database '$dbName' on server " .
                                        $conn->getServer() . " from client host {$this->host}";
@@ -909,7 +904,8 @@ class LoadBalancer implements ILoadBalancer {
                        } else {
                                $conn->tablePrefix( $prefix );
                                unset( $this->mConns[$connFreeKey][$i][$oldDomain] );
-                               $this->mConns[$connInUseKey][$i][$domain] = $conn;
+                               // Note that if $domain is an empty string, getDomainID() might not match it
+                               $this->mConns[$connInUseKey][$i][$conn->getDomainId()] = $conn;
                                $this->connLogger->debug( __METHOD__ .
                                        ": reusing free connection from $oldDomain for $domain" );
                        }
@@ -929,8 +925,9 @@ class LoadBalancer implements ILoadBalancer {
                                $this->errorConnection = $conn;
                                $conn = false;
                        } else {
-                               $conn->tablePrefix( $prefix );
-                               $this->mConns[$connInUseKey][$i][$domain] = $conn;
+                               $conn->tablePrefix( $prefix ); // as specified
+                               // Note that if $domain is an empty string, getDomainID() might not match it
+                               $this->mConns[$connInUseKey][$i][$conn->getDomainID()] = $conn;
                                $this->connLogger->debug( __METHOD__ . ": opened new connection for $i/$domain" );
                        }
                }
@@ -960,22 +957,22 @@ class LoadBalancer implements ILoadBalancer {
        }
 
        /**
-        * Really opens a connection. Uncached.
+        * Open a new network connection to a server (uncached)
+        *
         * Returns a Database object whether or not the connection was successful.
-        * @access private
         *
         * @param array $server
-        * @param string|bool $dbNameOverride Use "" to not select any database
+        * @param string|null $dbNameOverride Use "" to not select any database
         * @return Database
         * @throws DBAccessError
         * @throws InvalidArgumentException
         */
-       protected function reallyOpenConnection( array $server, $dbNameOverride = false ) {
+       protected function reallyOpenConnection( array $server, $dbNameOverride ) {
                if ( $this->disabled ) {
                        throw new DBAccessError();
                }
 
-               if ( $dbNameOverride !== false ) {
+               if ( $dbNameOverride !== null ) {
                        $server['dbname'] = $dbNameOverride;
                }
 
@@ -1691,17 +1688,35 @@ class LoadBalancer implements ILoadBalancer {
                                "Foreign domain connections are still in use ($domains)." );
                }
 
-               $this->localDomain = new DatabaseDomain(
+               $oldDomain = $this->localDomain->getId();
+               $this->setLocalDomain( new DatabaseDomain(
                        $this->localDomain->getDatabase(),
                        null,
                        $prefix
-               );
+               ) );
 
-               $this->forEachOpenConnection( function ( IDatabase $db ) use ( $prefix ) {
-                       $db->tablePrefix( $prefix );
+               $this->forEachOpenConnection( function ( IDatabase $db ) use ( $prefix, $oldDomain ) {
+                       if ( !$db->getLBInfo( 'foreign' ) ) {
+                               $db->tablePrefix( $prefix );
+                       }
                } );
        }
 
+       /**
+        * @param DatabaseDomain $domain
+        */
+       private function setLocalDomain( DatabaseDomain $domain ) {
+               $this->localDomain = $domain;
+               // In case a caller assumes that the domain ID is simply <db>-<prefix>, which is almost
+               // always true, gracefully handle the case when they fail to account for escaping.
+               if ( $this->localDomain->getTablePrefix() != '' ) {
+                       $this->localDomainIdAlias =
+                               $this->localDomain->getDatabase() . '-' . $this->localDomain->getTablePrefix();
+               } else {
+                       $this->localDomainIdAlias = $this->localDomain->getDatabase();
+               }
+       }
+
        /**
         * Make PHP ignore user aborts/disconnects until the returned
         * value leaves scope. This returns null and does nothing in CLI mode.

diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancerSingle.php b/includes/libs/rdbms/loadbalancer/LoadBalancerSingle.php
index 79d250f..c737563 100644 (file)
--- a/includes/libs/rdbms/loadbalancer/LoadBalancerSingle.php
+++ b/includes/libs/rdbms/loadbalancer/LoadBalancerSingle.php
@@ -72,7 +72,7 @@ class LoadBalancerSingle extends LoadBalancer {
                return new static( [ 'connection' => $db ] + $params );
        }
 
-       protected function reallyOpenConnection( array $server, $dbNameOverride = false ) {
+       protected function reallyOpenConnection( array $server, $dbNameOverride ) {
                return $this->db;
        }
 }

diff --git a/tests/phpunit/includes/Storage/RevisionStoreDbTest.php b/tests/phpunit/includes/Storage/RevisionStoreDbTest.php
index e33de20..8185670 100644 (file)
--- a/tests/phpunit/includes/Storage/RevisionStoreDbTest.php
+++ b/tests/phpunit/includes/Storage/RevisionStoreDbTest.php
@@ -44,7 +44,7 @@ class RevisionStoreDbTest extends MediaWikiTestCase {
                        ->getMock();
 
                $lb->method( 'reallyOpenConnection' )->willReturnCallback(
-                       function ( array $server, $dbNameOverride = false ) {
+                       function ( array $server, $dbNameOverride ) {
                                return $this->getDatabaseMock( $server );
                        }
                );

diff --git a/tests/phpunit/includes/db/LBFactoryTest.php b/tests/phpunit/includes/db/LBFactoryTest.php
index e46a0dd..e52dac6 100644 (file)
--- a/tests/phpunit/includes/db/LBFactoryTest.php
+++ b/tests/phpunit/includes/db/LBFactoryTest.php
@@ -27,6 +27,7 @@ use Wikimedia\Rdbms\LBFactorySimple;
 use Wikimedia\Rdbms\LBFactoryMulti;
 use Wikimedia\Rdbms\ChronologyProtector;
 use Wikimedia\Rdbms\MySQLMasterPos;
+use Wikimedia\Rdbms\DatabaseDomain;
 
 /**
  * @group Database
@@ -314,7 +315,8 @@ class LBFactoryTest extends MediaWikiTestCase {
        }
 
        private function newLBFactoryMulti( array $baseOverride = [], array $serverOverride = [] ) {
-               global $wgDBserver, $wgDBuser, $wgDBpassword, $wgDBname, $wgDBtype, $wgSQLiteDataDir;
+               global $wgDBserver, $wgDBuser, $wgDBpassword, $wgDBname, $wgDBprefix, $wgDBtype;
+               global $wgSQLiteDataDir;
 
                return new LBFactoryMulti( $baseOverride + [
                        'sectionsByDB' => [],
@@ -325,6 +327,7 @@ class LBFactoryTest extends MediaWikiTestCase {
                        ],
                        'serverTemplate' => $serverOverride + [
                                'dbname' => $wgDBname,
+                               'tablePrefix' => $wgDBprefix,
                                'user' => $wgDBuser,
                                'password' => $wgDBpassword,
                                'type' => $wgDBtype,
@@ -335,7 +338,7 @@ class LBFactoryTest extends MediaWikiTestCase {
                                'test-db1' => $wgDBserver,
                        ],
                        'loadMonitorClass' => 'LoadMonitorNull',
-                       'localDomain' => wfWikiID()
+                       'localDomain' => new DatabaseDomain( $wgDBname, null, $wgDBprefix )
                ] );
        }
 
@@ -361,7 +364,7 @@ class LBFactoryTest extends MediaWikiTestCase {
                if ( $wgDBtype !== 'sqlite' ) {
                        $db = $lb->getConnectionRef( DB_MASTER );
                        $this->assertEquals(
-                               $wgDBname,
+                               wfWikiID(),
                                $db->getDomainID()
                        );
                        unset( $db );
@@ -369,34 +372,45 @@ class LBFactoryTest extends MediaWikiTestCase {
 
                /** @var Database $db */
                $db = $lb->getConnection( DB_MASTER, [], '' );
-               $lb->reuseConnection( $db ); // don't care
 
+               $this->assertEquals(
+                       $wgDBname,
+                       $db->getDomainId(),
+                       'Main domain ID handle used; same DB name'
+               );
+               $this->assertEquals(
+                       $wgDBname,
+                       $db->getDBname(),
+                       'Main domain ID handle used; same DB name'
+               );
                $this->assertEquals(
                        '',
-                       $db->getDomainID()
+                       $db->tablePrefix(),
+                       'Main domain ID handle used; prefix is empty though'
                );
-
                $this->assertEquals(
                        $this->quoteTable( $db, 'page' ),
                        $db->tableName( 'page' ),
                        "Correct full table name"
                );
-
                $this->assertEquals(
                        $this->quoteTable( $db, $wgDBname ) . '.' . $this->quoteTable( $db, 'page' ),
                        $db->tableName( "$wgDBname.page" ),
                        "Correct full table name"
                );
-
                $this->assertEquals(
                        $this->quoteTable( $db, 'nice_db' ) . '.' . $this->quoteTable( $db, 'page' ),
                        $db->tableName( 'nice_db.page' ),
                        "Correct full table name"
                );
 
+               $lb->reuseConnection( $db ); // don't care
+
+               $db = $lb->getConnection( DB_MASTER ); // local domain connection
                $factory->setDomainPrefix( 'my_' );
+
                $this->assertEquals(
-                       '',
+                       "$wgDBname-my_",
                        $db->getDomainID()
                );
                $this->assertEquals(
@@ -415,7 +429,7 @@ class LBFactoryTest extends MediaWikiTestCase {
        }
 
        public function testTrickyDomain() {
-               global $wgDBtype;
+               global $wgDBtype, $wgDBname;
 
                if ( $wgDBtype === 'sqlite' ) {
                        $tmpDir = $this->getNewTempDirectory();
@@ -427,18 +441,17 @@ class LBFactoryTest extends MediaWikiTestCase {
                        $dbPath = null;
                }
 
-               $dbname = 'unittest-domain';
+               $dbname = 'unittest-domain'; // explodes if DB is selected
                $factory = $this->newLBFactoryMulti(
-                       [ 'localDomain' => $dbname ],
-                       [ 'dbname' => $dbname, 'dbFilePath' => $dbPath ]
+                       [ 'localDomain' => ( new DatabaseDomain( $dbname, null, '' ) )->getId() ],
+                       [ 'dbFilePath' => $dbPath ]
                );
                $lb = $factory->getMainLB();
                /** @var Database $db */
                $db = $lb->getConnection( DB_MASTER, [], '' );
-               $lb->reuseConnection( $db ); // don't care
 
                $this->assertEquals(
-                       '',
+                       $wgDBname,
                        $db->getDomainID()
                );
 
@@ -460,7 +473,10 @@ class LBFactoryTest extends MediaWikiTestCase {
                        "Correct full table name"
                );
 
+               $lb->reuseConnection( $db ); // don't care
+
                $factory->setDomainPrefix( 'my_' );
+               $db = $lb->getConnection( DB_MASTER, [], "$wgDBname-my_" );
 
                $this->assertEquals(
                        $this->quoteTable( $db, 'my_page' ),
@@ -472,7 +488,6 @@ class LBFactoryTest extends MediaWikiTestCase {
                        $db->tableName( 'other_nice_db.page' ),
                        "Correct full table name"
                );
-
                $this->assertEquals(
                        $this->quoteTable( $db, 'garbage-db' ) . '.' . $this->quoteTable( $db, 'page' ),
                        $db->tableName( 'garbage-db.page' ),
@@ -494,6 +509,8 @@ class LBFactoryTest extends MediaWikiTestCase {
                        \MediaWiki\restoreWarnings();
                }
 
+               $lb->reuseConnection( $db ); // don't care
+
                $factory->closeAll();
                $factory->destroy();
        }

diff --git a/tests/phpunit/includes/jobqueue/JobQueueTest.php b/tests/phpunit/includes/jobqueue/JobQueueTest.php
index 7b34b59..bd21dc8 100644 (file)
--- a/tests/phpunit/includes/jobqueue/JobQueueTest.php
+++ b/tests/phpunit/includes/jobqueue/JobQueueTest.php
@@ -1,5 +1,7 @@
 <?php
 
+use MediaWiki\MediaWikiServices;
+
 /**
  * @group JobQueue
  * @group medium
@@ -26,7 +28,7 @@ class JobQueueTest extends MediaWikiTestCase {
                        }
                        $baseConfig = $wgJobTypeConf[$name];
                } else {
-                       $baseConfig = [ 'class' => 'JobQueueDB' ];
+                       $baseConfig = [ 'class' => 'JobQueueDBSingle' ];
                }
                $baseConfig['type'] = 'null';
                $baseConfig['wiki'] = wfWikiID();
@@ -381,3 +383,11 @@ class JobQueueTest extends MediaWikiTestCase {
                        [ 'lives' => 0, 'usleep' => 0, 'removeDuplicates' => 1, 'i' => $i ] + $rootJob );
        }
 }
+
+class JobQueueDBSingle extends JobQueueDB {
+       protected function getDB( $index ) {
+               $lb = MediaWikiServices::getInstance()->getDBLoadBalancer();
+               // Override to not use CONN_TRX_AUTO so that we see the same temporary `job` table
+               return $lb->getConnection( $index, [], $this->wiki );
+       }
+}