From 32c7a9bf46b8ba8d81ed534ecb9bba29c4e50610 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 16 Sep 2016 11:32:23 -0700 Subject: [PATCH] Make LoadBalancer domain handling more robust * Cleanup setDomainPrefix() methods to handle existing LBs/DBs. * This also makes it set just the prefix as the prefix rather than as the whole domain. * Fix regression from 789a54a5f1 where "read" mode of tablePrefix()/dbSchema() still set the field. * Make getConnectionRef() always have the domain set. * Add a check in openConnection() for explicit local connections, treating $domain as false and avoiding openForeignConnection(). Bug: T145840 Change-Id: Idf392bd9992a215c4fa3bddf275562f3916596aa --- includes/db/CloneDatabase.php | 6 ----- includes/db/Database.php | 18 +++++++++------ includes/libs/rdbms/lbfactory/LBFactory.php | 15 ++++++++----- .../libs/rdbms/loadbalancer/LoadBalancer.php | 13 +++++++++-- tests/phpunit/includes/db/DatabaseTest.php | 22 +++++++++++++++++++ 5 files changed, 53 insertions(+), 21 deletions(-) diff --git a/includes/db/CloneDatabase.php b/includes/db/CloneDatabase.php index ee82bdf5df..2af742e971 100644 --- a/includes/db/CloneDatabase.php +++ b/includes/db/CloneDatabase.php @@ -132,12 +132,6 @@ class CloneDatabase { $lbFactory = wfGetLBFactory(); $lbFactory->setDomainPrefix( $prefix ); - $lbFactory->forEachLB( function( LoadBalancer $lb ) use ( $prefix ) { - $lb->setDomainPrefix( $prefix ); - $lb->forEachOpenConnection( function ( IDatabase $db ) use ( $prefix ) { - $db->tablePrefix( $prefix ); - } ); - } ); $wgDBprefix = $prefix; } } diff --git a/includes/db/Database.php b/includes/db/Database.php index e908824645..3d9b41a5b9 100644 --- a/includes/db/Database.php +++ b/includes/db/Database.php @@ -93,9 +93,9 @@ abstract class DatabaseBase implements IDatabase, LoggerAwareInterface { protected $mTrxEndCallbacksSuppressed = false; /** @var string */ - protected $mTablePrefix; + protected $mTablePrefix = ''; /** @var string */ - protected $mSchema; + protected $mSchema = ''; /** @var integer */ protected $mFlags; /** @var array */ @@ -367,7 +367,7 @@ abstract class DatabaseBase implements IDatabase, LoggerAwareInterface { $p['variables'] = isset( $p['variables'] ) ? $p['variables'] : []; $p['tablePrefix'] = isset( $p['tablePrefix'] ) ? $p['tablePrefix'] : ''; if ( !isset( $p['schema'] ) ) { - $p['schema'] = isset( $defaultSchemas[$dbType] ) ? $defaultSchemas[$dbType] : null; + $p['schema'] = isset( $defaultSchemas[$dbType] ) ? $defaultSchemas[$dbType] : ''; } $p['foreign'] = isset( $p['foreign'] ) ? $p['foreign'] : false; @@ -466,14 +466,18 @@ abstract class DatabaseBase implements IDatabase, LoggerAwareInterface { public function tablePrefix( $prefix = null ) { $old = $this->mTablePrefix; - $this->mTablePrefix = $prefix; + if ( $prefix !== null ) { + $this->mTablePrefix = $prefix; + } return $old; } public function dbSchema( $schema = null ) { $old = $this->mSchema; - $this->mSchema = $schema; + if ( $schema !== null ) { + $this->mSchema = $schema; + } return $old; } @@ -699,7 +703,7 @@ abstract class DatabaseBase implements IDatabase, LoggerAwareInterface { } public function getWikiID() { - if ( $this->mTablePrefix ) { + if ( $this->mTablePrefix != '' ) { return "{$this->mDBname}-{$this->mTablePrefix}"; } else { return $this->mDBname; @@ -1884,7 +1888,7 @@ abstract class DatabaseBase implements IDatabase, LoggerAwareInterface { # In dbs that support it, $database may actually be the schema # but that doesn't affect any of the functionality here $prefix = ''; - $schema = null; + $schema = ''; } else { list( $table ) = $dbDetails; if ( isset( $this->tableAliases[$table] ) ) { diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php b/includes/libs/rdbms/lbfactory/LBFactory.php index b6f331701c..e011ff0671 100644 --- a/includes/libs/rdbms/lbfactory/LBFactory.php +++ b/includes/libs/rdbms/lbfactory/LBFactory.php @@ -633,15 +633,18 @@ abstract class LBFactory { } /** - * Define a new local domain (for testing) + * Set a new table prefix for the existing local domain ID for testing * - * Caller should make sure no local connection are open to the old local domain - * - * @param string $domain + * @param string $prefix * @since 1.28 */ - public function setDomainPrefix( $domain ) { - $this->localDomain = $domain; + public function setDomainPrefix( $prefix ) { + list( $dbName, ) = explode( '-', $this->localDomain, 2 ); + $this->localDomain = "{$dbName}-{$prefix}"; + + $this->forEachLB( function( LoadBalancer $lb ) use ( $prefix ) { + $lb->setDomainPrefix( $prefix ); + } ); } /** diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index db69de1597..57c905facc 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -515,7 +515,7 @@ class LoadBalancer implements ILoadBalancer { } if ( $domain === $this->localDomain ) { - $domain = false; + $domain = false; // local connection requested } $groups = ( $groups === false || $groups === [] ) @@ -627,6 +627,8 @@ class LoadBalancer implements ILoadBalancer { * @since 1.22 */ public function getConnectionRef( $db, $groups = [], $domain = false ) { + $domain = ( $domain !== false ) ? $domain : $this->localDomain; + return new DBConnRef( $this, $this->getConnection( $db, $groups, $domain ) ); } @@ -650,6 +652,10 @@ class LoadBalancer implements ILoadBalancer { } public function openConnection( $i, $domain = false ) { + if ( $domain === $this->localDomain ) { + $domain = false; // local connection requested + } + if ( $domain !== false ) { $conn = $this->openForeignConnection( $i, $domain ); } elseif ( isset( $this->mConns['local'][$i][0] ) ) { @@ -1607,7 +1613,10 @@ class LoadBalancer implements ILoadBalancer { */ public function setDomainPrefix( $prefix ) { list( $dbName, ) = explode( '-', $this->localDomain, 2 ); - $this->localDomain = "{$dbName}-{$prefix}"; + + $this->forEachOpenConnection( function ( IDatabase $db ) use ( $prefix ) { + $db->tablePrefix( $prefix ); + } ); } } diff --git a/tests/phpunit/includes/db/DatabaseTest.php b/tests/phpunit/includes/db/DatabaseTest.php index d4be6e4fa0..134caf4abb 100644 --- a/tests/phpunit/includes/db/DatabaseTest.php +++ b/tests/phpunit/includes/db/DatabaseTest.php @@ -427,4 +427,26 @@ class DatabaseTest extends MediaWikiTestCase { $this->assertEquals( $origSsl, $db->getFlag( DBO_SSL ) ); $this->assertEquals( $origTrx, $db->getFlag( DBO_TRX ) ); } + + /** + * @covers DatabaseBase::tablePrefix() + * @covers DatabaseBase::dbSchema() + */ + public function testMutators() { + $old = $this->db->tablePrefix(); + $this->assertType( 'string', $old, 'Prefix is string' ); + $this->assertEquals( $old, $this->db->tablePrefix(), "Prefix unchanged" ); + $this->assertEquals( $old, $this->db->tablePrefix( 'xxx' ) ); + $this->assertEquals( 'xxx', $this->db->tablePrefix(), "Prefix set" ); + $this->db->tablePrefix( $old ); + $this->assertNotEquals( 'xxx', $this->db->tablePrefix() ); + + $old = $this->db->dbSchema(); + $this->assertType( 'string', $old, 'Schema is string' ); + $this->assertEquals( $old, $this->db->dbSchema(), "Schema unchanged" ); + $this->assertEquals( $old, $this->db->dbSchema( 'xxx' ) ); + $this->assertEquals( 'xxx', $this->db->dbSchema(), "Schema set" ); + $this->db->dbSchema( $old ); + $this->assertNotEquals( 'xxx', $this->db->dbSchema() ); + } } -- 2.20.1