Aaron Schulz has uploaded a new change for review.
https://gerrit.wikimedia.org/r/311222
Change subject: Make database classes handle hyphens in $wgDBname
......................................................................
Make database classes handle hyphens in $wgDBname
* Add DatabaseDomain class to handle passing domains around.
It also can be cast to and from strings, which are of the same
format as wfWikiId() except with hyphens escaped.
* Make IDatabase::getDomainID() use these IDs so they can be
passed into LoadBalancer::getConnection() and friends without
breaking on sites with a hyphen in the DB name.
* Add more LBFactory unit tests for domains.
Bug: T145840
Change-Id: Icfed62b251af8cef706a899197c3ccdb730ef4d1
---
M autoload.php
M includes/db/loadbalancer/LBFactoryMW.php
M includes/libs/rdbms/database/DBConnRef.php
M includes/libs/rdbms/database/Database.php
A includes/libs/rdbms/database/DatabaseDomain.php
M includes/libs/rdbms/lbfactory/LBFactory.php
M includes/libs/rdbms/loadbalancer/LoadBalancer.php
M tests/phpunit/includes/db/LBFactoryTest.php
8 files changed, 381 insertions(+), 25 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/22/311222/1
diff --git a/autoload.php b/autoload.php
index 716e56d..035c152 100644
--- a/autoload.php
+++ b/autoload.php
@@ -318,6 +318,7 @@
'DataUpdate' => __DIR__ . '/includes/deferred/DataUpdate.php',
'Database' => __DIR__ . '/includes/libs/rdbms/database/Database.php',
'DatabaseBase' => __DIR__ .
'/includes/libs/rdbms/database/DatabaseBase.php',
+ 'DatabaseDomain' => __DIR__ .
'/includes/libs/rdbms/database/DatabaseDomain.php',
'DatabaseInstaller' => __DIR__ .
'/includes/installer/DatabaseInstaller.php',
'DatabaseLag' => __DIR__ . '/maintenance/lag.php',
'DatabaseLogEntry' => __DIR__ . '/includes/logging/LogEntry.php',
diff --git a/includes/db/loadbalancer/LBFactoryMW.php
b/includes/db/loadbalancer/LBFactoryMW.php
index 49d0624..16faeb7 100644
--- a/includes/db/loadbalancer/LBFactoryMW.php
+++ b/includes/db/loadbalancer/LBFactoryMW.php
@@ -37,10 +37,10 @@
* @TODO: inject objects via dependency framework
*/
public function __construct( array $conf ) {
- global $wgCommandLineMode, $wgSQLMode, $wgDBmysql5;
+ global $wgCommandLineMode, $wgSQLMode, $wgDBmysql5, $wgDBname,
$wgDBprefix;
$defaults = [
- 'localDomain' => wfWikiID(),
+ 'localDomain' => new DatabaseDomain( $wgDBname, null,
$wgDBprefix ),
'hostname' => wfHostname(),
'trxProfiler' =>
Profiler::instance()->getTransactionProfiler(),
'replLogger' => LoggerFactory::getInstance(
'DBReplication' ),
diff --git a/includes/libs/rdbms/database/DBConnRef.php
b/includes/libs/rdbms/database/DBConnRef.php
index 876ee30..f111fdf 100644
--- a/includes/libs/rdbms/database/DBConnRef.php
+++ b/includes/libs/rdbms/database/DBConnRef.php
@@ -14,22 +14,22 @@
/** @var IDatabase|null Live connection handle */
private $conn;
- /** @var array|null */
+ /** @var array|null N-tuple of (server index, group,
DatabaseDomain|string) */
private $params;
const FLD_INDEX = 0;
const FLD_GROUP = 1;
- const FLD_WIKI = 2;
+ const FLD_DOMAIN = 2;
/**
* @param ILoadBalancer $lb
- * @param IDatabase|array $conn Connection or (server index, group,
wiki ID)
+ * @param IDatabase|array $conn Connection or (server index, group,
DatabaseDomain|string)
*/
public function __construct( ILoadBalancer $lb, $conn ) {
$this->lb = $lb;
if ( $conn instanceof IDatabase ) {
$this->conn = $conn; // live handle
- } elseif ( count( $conn ) >= 3 && $conn[self::FLD_WIKI] !==
false ) {
+ } elseif ( count( $conn ) >= 3 && $conn[self::FLD_DOMAIN] !==
false ) {
$this->params = $conn;
} else {
throw new InvalidArgumentException( "Missing lazy
connection arguments." );
@@ -146,9 +146,10 @@
}
public function getDomainID() {
- if ( $this->conn === null ) {
- // Avoid triggering a connection
- return $this->params[self::FLD_WIKI];
+ if ( !$this->conn ) {
+ $domain = $this->params[self::FLD_DOMAIN];
+ // Avoid triggering a database connection
+ return $domain instanceof DatabaseDomain ?
$domain->getId() : $domain;
}
return $this->__call( __FUNCTION__, func_get_args() );
diff --git a/includes/libs/rdbms/database/Database.php
b/includes/libs/rdbms/database/Database.php
index 9a63b7f..f13db72 100644
--- a/includes/libs/rdbms/database/Database.php
+++ b/includes/libs/rdbms/database/Database.php
@@ -621,11 +621,7 @@
}
public function getDomainID() {
- if ( $this->mTablePrefix != '' ) {
- return "{$this->mDBname}-{$this->mTablePrefix}";
- } else {
- return $this->mDBname;
- }
+ return DatabaseDomain::newFromId( $this->mDBname, null,
$this->mTablePrefix )->getId();
}
final public function getWikiID() {
diff --git a/includes/libs/rdbms/database/DatabaseDomain.php
b/includes/libs/rdbms/database/DatabaseDomain.php
new file mode 100644
index 0000000..12f45c6
--- /dev/null
+++ b/includes/libs/rdbms/database/DatabaseDomain.php
@@ -0,0 +1,196 @@
+<?php
+/**
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ * @ingroup Database
+ */
+
+/**
+ * Class to handle database/prefix specification for IDatabase domains
+ */
+class DatabaseDomain {
+ /** @var string */
+ private $database;
+ /** @var string|null */
+ private $schema;
+ /** @var string */
+ private $prefix;
+
+ /** @var string Cache of convertToString() */
+ private $equivalentString;
+
+ /**
+ * @param string $database Database name
+ * @param string|null $schema Schema name
+ * @param string $prefix Table prefix
+ */
+ public function __construct( $database, $schema, $prefix ) {
+ if ( !is_string( $database ) || !is_string( $prefix ) ) {
+ throw new InvalidArgumentException( "Database and
prefix must be strings." );
+ }
+ $this->database = $database;
+ if ( $schema !== null && ( !is_string( $schema ) || !strlen(
$schema ) ) ) {
+ throw new InvalidArgumentException( "Schema must be
null or a non-empty string." );
+ }
+ $this->schema = $schema;
+ $this->prefix = $prefix;
+ $this->equivalentString = $this->convertToString();
+ }
+
+ /**
+ * @param DatabaseDomain|string $domain Result of
DatabaseDomain::toString()
+ * @return DatabaseDomain
+ */
+ public static function newFromId( $domain ) {
+ if ( $domain instanceof self ) {
+ return $domain;
+ }
+
+ $parts = array_map( [ __CLASS__, 'decode' ], explode( '-',
$domain ) );
+
+ $schema = null;
+ $prefix = '';
+
+ if ( count( $parts ) == 1 ) {
+ $database = $parts[0];
+ } elseif ( count( $parts ) == 2 ) {
+ list( $database, $prefix ) = $parts;
+ } elseif ( count( $parts ) == 3 ) {
+ list( $database, $schema, $prefix ) = $parts;
+ } else {
+ throw new InvalidArgumentException( "Domain has too few
or too many parts." );
+ }
+
+ return new self( $database, $schema, $prefix );
+ }
+
+ /**
+ * @return DatabaseDomain
+ */
+ public static function newUnspecified() {
+ return new self( '__UNSPECIFIED__', null, '' );
+ }
+
+ /**
+ * @param DatabaseDomain|string $other
+ * @return DatabaseDomain
+ */
+ public function equals( $other ) {
+ if ( $other instanceof DatabaseDomain ) {
+ return (
+ $this->getDatabase() === $other->getDatabase()
&&
+ $this->getSchema() === $other->getSchema() &&
+ $this->getTablePrefix() ===
$other->getTablePrefix()
+ );
+ }
+
+ return ( $this->equivalentString === $other );
+ }
+
+ /**
+ * @return string Database name
+ */
+ public function getDatabase() {
+ return $this->database;
+ }
+
+ /**
+ * @return string|null Database schema
+ */
+ public function getSchema() {
+ return $this->database;
+ }
+
+ /**
+ * @return string Table prefix
+ */
+ public function getTablePrefix() {
+ return $this->prefix;
+ }
+
+ /**
+ * @return string
+ */
+ public function getId() {
+ return $this->equivalentString;
+ }
+
+ /**
+ * @return string
+ */
+ private function convertToString() {
+ $parts = [ $this->database ];
+ if ( $this->schema !== null ) {
+ $parts[] = $this->schema;
+ }
+ if ( $this->prefix != '' ) {
+ $parts[] = $this->prefix;
+ }
+
+ return implode( '-', array_map( [ __CLASS__, 'encode' ], $parts
) );
+ }
+
+ private static function encode( $decoded ) {
+ $encoded = '';
+
+ $length = strlen( $decoded );
+ for ( $i = 0; $i < $length; ++$i ) {
+ $char = $decoded[$i];
+ if ( $char === '-' ) {
+ $encoded .= '?h';
+ } elseif ( $char === '?' ) {
+ $encoded .= '??';
+ } else {
+ $encoded .= $char;
+ }
+ }
+
+ return $encoded;
+ }
+
+ private static function decode( $encoded ) {
+ $decoded = '';
+
+ $length = strlen( $encoded );
+ for ( $i = 0; $i < $length; ++$i ) {
+ $char = $encoded[$i];
+ if ( $char === '?' ) {
+ $nextChar = isset( $encoded[$i + 1] ) ?
$encoded[$i + 1] : null;
+ if ( $nextChar === 'h' ) {
+ $decoded .= '-';
+ ++$i;
+ } elseif ( $nextChar === '@' ) {
+ $decoded .= '@';
+ ++$i;
+ } else {
+ $decoded .= $char;
+ }
+ } else {
+ $decoded .= $char;
+ }
+ }
+
+ return $decoded;
+ }
+
+ /**
+ * @return string
+ */
+ function __toString() {
+ return $this->getId();
+ }
+}
diff --git a/includes/libs/rdbms/lbfactory/LBFactory.php
b/includes/libs/rdbms/lbfactory/LBFactory.php
index 49fac6a..3ab7362 100644
--- a/includes/libs/rdbms/lbfactory/LBFactory.php
+++ b/includes/libs/rdbms/lbfactory/LBFactory.php
@@ -49,7 +49,7 @@
/** @var WANObjectCache */
protected $wanCache;
- /** @var string Local domain */
+ /** @var DatabaseDomain Local domain */
protected $localDomain;
/** @var string Local hostname of the app server */
protected $hostname;
@@ -79,7 +79,9 @@
* @param array $conf
*/
public function __construct( array $conf ) {
- $this->localDomain = isset( $conf['localDomain'] ) ?
$conf['localDomain'] : '';
+ $this->localDomain = isset( $conf['localDomain'] )
+ ? DatabaseDomain::newFromId( $conf['localDomain'] )
+ : DatabaseDomain::newUnspecified();
if ( isset( $conf['readOnlyReason'] ) && is_string(
$conf['readOnlyReason'] ) ) {
$this->readOnlyReason = $conf['readOnlyReason'];
@@ -638,8 +640,11 @@
* @since 1.28
*/
public function setDomainPrefix( $prefix ) {
- list( $dbName, ) = explode( '-', $this->localDomain, 2 );
- $this->localDomain = "{$dbName}-{$prefix}";
+ $this->localDomain = new DatabaseDomain(
+ $this->localDomain->getDatabase(),
+ null,
+ $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 57c905f..c673492 100644
--- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php
+++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php
@@ -84,8 +84,10 @@
private $trxRoundId = false;
/** @var array[] Map of (name => callable) */
private $trxRecurringCallbacks = [];
- /** @var string Local Domain ID and default for selectDB() calls */
+ /** @var DatabaseDomain Local Domain ID and default for selectDB()
calls */
private $localDomain;
+ /** @var string Alternate ID string for the domain instead of
DatabaseDomain::getId() */
+ private $localDomainIdAlias;
/** @var string Current server name */
private $host;
/** @var bool Whether this PHP instance is for a CLI script */
@@ -113,10 +115,18 @@
throw new InvalidArgumentException( __CLASS__ . ':
missing servers parameter' );
}
$this->mServers = $params['servers'];
+
+ $this->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.
+ $this->localDomainIdAlias =
+ $this->localDomain->getDatabase() . '-' .
$this->localDomain->getTablePrefix();
+
$this->mWaitTimeout = isset( $params['waitTimeout'] )
? $params['waitTimeout']
: self::POS_WAIT_TIMEOUT;
- $this->localDomain = isset( $params['localDomain'] ) ?
$params['localDomain'] : '';
$this->mReadIndex = -1;
$this->mConns = [
@@ -514,7 +524,7 @@
' with invalid server index' );
}
- if ( $domain === $this->localDomain ) {
+ if ( $this->localDomain->equals( $domain ) || $domain ===
$this->localDomainIdAlias ) {
$domain = false; // local connection requested
}
@@ -652,7 +662,7 @@
}
public function openConnection( $i, $domain = false ) {
- if ( $domain === $this->localDomain ) {
+ if ( $this->localDomain->equals( $domain ) || $domain ===
$this->localDomainIdAlias ) {
$domain = false; // local connection requested
}
@@ -708,7 +718,9 @@
* @return IDatabase
*/
private function openForeignConnection( $i, $domain ) {
- list( $dbName, $prefix ) = explode( '-', $domain, 2 ) + [ '',
'' ];
+ $domainInstance = DatabaseDomain::newFromId( $domain );
+ $dbName = $domainInstance->getDatabase();
+ $prefix = $domainInstance->getTablePrefix();
if ( isset( $this->mConns['foreignUsed'][$i][$domain] ) ) {
// Reuse an already-used connection
@@ -1612,8 +1624,11 @@
* @since 1.28
*/
public function setDomainPrefix( $prefix ) {
- list( $dbName, ) = explode( '-', $this->localDomain, 2 );
- $this->localDomain = "{$dbName}-{$prefix}";
+ $this->localDomain = new DatabaseDomain(
+ $this->localDomain->getDatabase(),
+ null,
+ $prefix
+ );
$this->forEachOpenConnection( function ( IDatabase $db ) use (
$prefix ) {
$db->tablePrefix( $prefix );
diff --git a/tests/phpunit/includes/db/LBFactoryTest.php
b/tests/phpunit/includes/db/LBFactoryTest.php
index 5affa9c..cac2d6d 100644
--- a/tests/phpunit/includes/db/LBFactoryTest.php
+++ b/tests/phpunit/includes/db/LBFactoryTest.php
@@ -216,4 +216,146 @@
$cp->shutdownLB( $lb );
$cp->shutdown();
}
+
+ private function newLBFactoryMulti( array $baseOverride = [], array
$serverOverride = [] ) {
+ global $wgDBserver, $wgDBuser, $wgDBpassword, $wgDBname,
$wgDBtype;
+
+ return new LBFactoryMulti( $baseOverride + [
+ 'sectionsByDB' => [],
+ 'sectionLoads' => [
+ 'DEFAULT' => [
+ 'test-db1' => 1,
+ ],
+ ],
+ 'serverTemplate' => $serverOverride + [
+ 'dbname' => $wgDBname,
+ 'user' => $wgDBuser,
+ 'password' => $wgDBpassword,
+ 'type' => $wgDBtype,
+ 'flags' => DBO_DEFAULT
+ ],
+ 'hostsByName' => [
+ 'test-db1' => $wgDBserver,
+ ],
+ 'loadMonitorClass' => 'LoadMonitorNull',
+ 'localDomain' => wfWikiID()
+ ] );
+ }
+
+ public function testNiceDomains() {
+ global $wgDBname;
+
+ $factory = $this->newLBFactoryMulti();
+ $lb = $factory->getMainLB();
+
+ $db = $lb->getConnectionRef( DB_MASTER );
+ $this->assertEquals(
+ $wgDBname,
+ $db->getDomainID()
+ );
+ unset( $db );
+
+ /** @var DatabaseBase $db */
+ $db = $lb->getConnection( DB_MASTER, [], '' );
+
+ $this->assertEquals(
+ '',
+ $db->getDomainID()
+ );
+
+ $this->assertEquals(
+ $db->addIdentifierQuotes( 'page' ),
+ $db->tableName( 'page' ),
+ "Correct full table name"
+ );
+
+ $this->assertEquals(
+ $db->addIdentifierQuotes( $wgDBname ) . '.' .
$db->addIdentifierQuotes( 'page' ),
+ $db->tableName( "$wgDBname.page" ),
+ "Correct full table name"
+ );
+
+ $this->assertEquals(
+ $db->addIdentifierQuotes( 'nice_db' ) . '.' .
$db->addIdentifierQuotes( 'page' ),
+ $db->tableName( 'nice_db.page' ),
+ "Correct full table name"
+ );
+
+ $factory->setDomainPrefix( 'my_' );
+ $this->assertEquals(
+ '',
+ $db->getDomainID()
+ );
+ $this->assertEquals(
+ $db->addIdentifierQuotes( 'my_page' ),
+ $db->tableName( 'page' ),
+ "Correct full table name"
+ );
+ $this->assertEquals(
+ $db->addIdentifierQuotes( 'other_nice_db' ) . '.' .
$db->addIdentifierQuotes( 'page' ),
+ $db->tableName( 'other_nice_db.page' ),
+ "Correct full table name"
+ );
+
+ $factory->closeAll();
+ $factory->destroy();
+ }
+
+ public function testTrickyDomain() {
+ $dbname = 'unittest-domain';
+ $factory = $this->newLBFactoryMulti(
+ [ 'localDomain' => $dbname ], [ 'dbname' => $dbname ] );
+ $lb = $factory->getMainLB();
+ /** @var DatabaseBase $db */
+ $db = $lb->getConnection( DB_MASTER, [], '' );
+
+ $this->assertEquals(
+ '',
+ $db->getDomainID()
+ );
+
+ $this->assertEquals(
+ $db->addIdentifierQuotes( 'page' ),
+ $db->tableName( 'page' ),
+ "Correct full table name"
+ );
+
+ $this->assertEquals(
+ $db->addIdentifierQuotes( $dbname ) . '.' .
$db->addIdentifierQuotes( 'page' ),
+ $db->tableName( "$dbname.page" ),
+ "Correct full table name"
+ );
+
+ $this->assertEquals(
+ $db->addIdentifierQuotes( 'nice_db' ) . '.' .
$db->addIdentifierQuotes( 'page' ),
+ $db->tableName( 'nice_db.page' ),
+ "Correct full table name"
+ );
+
+ $factory->setDomainPrefix( 'my_' );
+
+ $this->assertEquals(
+ $db->addIdentifierQuotes( 'my_page' ),
+ $db->tableName( 'page' ),
+ "Correct full table name"
+ );
+ $this->assertEquals(
+ $db->addIdentifierQuotes( 'other_nice_db' ) . '.' .
$db->addIdentifierQuotes( 'page' ),
+ $db->tableName( 'other_nice_db.page' ),
+ "Correct full table name"
+ );
+
+ \MediaWiki\suppressWarnings();
+ $this->assertFalse( $db->selectDB( 'garbage-db' ) );
+ \MediaWiki\restoreWarnings();
+
+ $this->assertEquals(
+ $db->addIdentifierQuotes( 'garbage-db' ) . '.' .
$db->addIdentifierQuotes( 'page' ),
+ $db->tableName( 'garbage-db.page' ),
+ "Correct full table name"
+ );
+
+ $factory->closeAll();
+ $factory->destroy();
+ }
}
--
To view, visit https://gerrit.wikimedia.org/r/311222
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icfed62b251af8cef706a899197c3ccdb730ef4d1
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Aaron Schulz <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
