From 321640b117b775ba7feb26281922bfd7833b0618 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 21 Mar 2019 18:12:44 -0700 Subject: [PATCH] rdbms: use a direct "USE" query for doSelectDomain() for mysql This should give better error messages on failure. Bug: T212284 Change-Id: I55260c6e3db1770f01e3d6a6a363b917a57265be --- .../libs/rdbms/database/DatabaseMysqlBase.php | 33 +++++ .../libs/rdbms/database/DatabaseMysqli.php | 19 --- .../libs/rdbms/exception/DBQueryError.php | 2 +- tests/phpunit/includes/db/LBFactoryTest.php | 114 ++++++++++++++---- 4 files changed, 122 insertions(+), 46 deletions(-) diff --git a/includes/libs/rdbms/database/DatabaseMysqlBase.php b/includes/libs/rdbms/database/DatabaseMysqlBase.php index 25d78da18d..77bb6779fe 100644 --- a/includes/libs/rdbms/database/DatabaseMysqlBase.php +++ b/includes/libs/rdbms/database/DatabaseMysqlBase.php @@ -225,6 +225,39 @@ abstract class DatabaseMysqlBase extends Database { } } + protected function doSelectDomain( DatabaseDomain $domain ) { + if ( $domain->getSchema() !== null ) { + throw new DBExpectedError( $this, __CLASS__ . ": domain schemas are not supported." ); + } + + $database = $domain->getDatabase(); + // A null database means "don't care" so leave it as is and update the table prefix + if ( $database === null ) { + $this->currentDomain = new DatabaseDomain( + $this->currentDomain->getDatabase(), + null, + $domain->getTablePrefix() + ); + + return true; + } + + if ( $database !== $this->getDBname() ) { + $sql = 'USE ' . $this->addIdentifierQuotes( $database ); + $ret = $this->doQuery( $sql ); + if ( $ret === false ) { + $error = $this->lastError(); + $errno = $this->lastErrno(); + $this->reportQueryError( $error, $errno, $sql, __METHOD__ ); + } + } + + // Update that domain fields on success (no exception thrown) + $this->currentDomain = $domain; + + return true; + } + /** * Open a connection to a MySQL server * diff --git a/includes/libs/rdbms/database/DatabaseMysqli.php b/includes/libs/rdbms/database/DatabaseMysqli.php index 15eeccf02c..1a5cdab78d 100644 --- a/includes/libs/rdbms/database/DatabaseMysqli.php +++ b/includes/libs/rdbms/database/DatabaseMysqli.php @@ -178,25 +178,6 @@ class DatabaseMysqli extends DatabaseMysqlBase { return $conn->affected_rows; } - function doSelectDomain( DatabaseDomain $domain ) { - if ( $domain->getSchema() !== null ) { - throw new DBExpectedError( $this, __CLASS__ . ": domain schemas are not supported." ); - } - - $database = $domain->getDatabase(); - if ( $database !== $this->getDBname() ) { - $conn = $this->getBindingHandle(); - if ( !$conn->select_db( $database ) ) { - throw new DBExpectedError( $this, "Could not select database '$database'." ); - } - } - - // Update that domain fields on success (no exception thrown) - $this->currentDomain = $domain; - - return true; - } - /** * @param mysqli_result $res * @return bool diff --git a/includes/libs/rdbms/exception/DBQueryError.php b/includes/libs/rdbms/exception/DBQueryError.php index b1353b793e..9b664fc3d0 100644 --- a/includes/libs/rdbms/exception/DBQueryError.php +++ b/includes/libs/rdbms/exception/DBQueryError.php @@ -45,7 +45,7 @@ class DBQueryError extends DBExpectedError { public function __construct( IDatabase $db, $error, $errno, $sql, $fname, $message = null ) { if ( $message === null ) { if ( $db instanceof Database && $db->wasConnectionError( $errno ) ) { - $message = "A connection error occurred. \n" . + $message = "A connection error occurred during a query. \n" . "Query: $sql\n" . "Function: $fname\n" . "Error: $errno $error\n"; diff --git a/tests/phpunit/includes/db/LBFactoryTest.php b/tests/phpunit/includes/db/LBFactoryTest.php index 3d1bf59315..2559b67db5 100644 --- a/tests/phpunit/includes/db/LBFactoryTest.php +++ b/tests/phpunit/includes/db/LBFactoryTest.php @@ -438,6 +438,13 @@ class LBFactoryTest extends MediaWikiTestCase { ] ); } + /** + * @covers \Wikimedia\Rdbms\LoadBalancer::getConnection + * @covers \Wikimedia\Rdbms\DatabaseMysqlBase::doSelectDomain + * @covers \Wikimedia\Rdbms\DatabaseMysqlBase::selectDB + * @covers \Wikimedia\Rdbms\DatabaseMssql::selectDB + * @covers DatabaseOracle::selectDB + */ public function testNiceDomains() { global $wgDBname; @@ -518,6 +525,13 @@ class LBFactoryTest extends MediaWikiTestCase { $factory->destroy(); } + /** + * @covers \Wikimedia\Rdbms\LoadBalancer::getConnection + * @covers \Wikimedia\Rdbms\DatabaseMysqlBase::doSelectDomain + * @covers \Wikimedia\Rdbms\DatabaseMysqlBase::selectDB + * @covers \Wikimedia\Rdbms\DatabaseMssql::selectDB + * @covers DatabaseOracle::selectDB + */ public function testTrickyDomain() { global $wgDBname; @@ -530,7 +544,7 @@ class LBFactoryTest extends MediaWikiTestCase { $factory = $this->newLBFactoryMulti( [ 'localDomain' => ( new DatabaseDomain( $dbname, null, '' ) )->getId() ], [ - 'dbName' => 'do_not_select_me' // explodes if DB is selected + 'dbname' => 'do_not_select_me' // explodes if DB is selected ] ); $lb = $factory->getMainLB(); @@ -584,46 +598,94 @@ class LBFactoryTest extends MediaWikiTestCase { $factory->destroy(); } + /** + * @covers \Wikimedia\Rdbms\LoadBalancer::getConnection + * @covers \Wikimedia\Rdbms\DatabaseMysqlBase::doSelectDomain + * @covers \Wikimedia\Rdbms\DatabaseMysqlBase::selectDB + * @covers \Wikimedia\Rdbms\DatabaseMssql::selectDB + * @covers DatabaseOracle::selectDB + */ public function testInvalidSelectDB() { - // FIXME: fails under sqlite - $this->markTestSkippedIfDbType( 'sqlite' ); + if ( wfGetDB( DB_MASTER )->databasesAreIndependent() ) { + $this->markTestSkipped( "Not applicable per databasesAreIndependent()" ); + } + $dbname = 'unittest-domain'; // explodes if DB is selected $factory = $this->newLBFactoryMulti( [ 'localDomain' => ( new DatabaseDomain( $dbname, null, '' ) )->getId() ], [ - 'dbName' => 'do_not_select_me' // explodes if DB is selected + 'dbname' => 'do_not_select_me' // explodes if DB is selected ] ); $lb = $factory->getMainLB(); /** @var IDatabase $db */ $db = $lb->getConnection( DB_MASTER, [], '' ); - if ( $db->getType() === 'sqlite' ) { + \Wikimedia\suppressWarnings(); + try { $this->assertFalse( $db->selectDB( 'garbage-db' ) ); - } elseif ( $db->databasesAreIndependent() ) { - try { - $e = null; - $db->selectDB( 'garbage-db' ); - } catch ( \Wikimedia\Rdbms\DBConnectionError $e ) { - // expected - } - $this->assertInstanceOf( \Wikimedia\Rdbms\DBConnectionError::class, $e ); - $this->assertFalse( $db->isOpen() ); - } else { - \Wikimedia\suppressWarnings(); - try { - $this->assertFalse( $db->selectDB( 'garbage-db' ) ); - $this->fail( "No error thrown." ); - } catch ( \Wikimedia\Rdbms\DBExpectedError $e ) { - $this->assertEquals( - "Could not select database 'garbage-db'.", - $e->getMessage() - ); - } - \Wikimedia\restoreWarnings(); + $this->fail( "No error thrown." ); + } catch ( \Wikimedia\Rdbms\DBQueryError $e ) { + $this->assertRegExp( '/[\'"]garbage-db[\'"]/', $e->getMessage() ); + } + \Wikimedia\restoreWarnings(); + } + + /** + * @covers \Wikimedia\Rdbms\DatabaseSqlite::selectDB + * @covers \Wikimedia\Rdbms\DatabasePostgres::selectDB + * @expectedException \Wikimedia\Rdbms\DBConnectionError + */ + public function testInvalidSelectDBIndependant() { + $dbname = 'unittest-domain'; // explodes if DB is selected + $factory = $this->newLBFactoryMulti( + [ 'localDomain' => ( new DatabaseDomain( $dbname, null, '' ) )->getId() ], + [ + 'dbname' => 'do_not_select_me' // explodes if DB is selected + ] + ); + $lb = $factory->getMainLB(); + + if ( !wfGetDB( DB_MASTER )->databasesAreIndependent() ) { + $this->markTestSkipped( "Not applicable per databasesAreIndependent()" ); + } + + /** @var IDatabase $db */ + $lb->getConnection( DB_MASTER, [], '' ); + } + + /** + * @covers \Wikimedia\Rdbms\DatabaseSqlite::selectDB + * @covers \Wikimedia\Rdbms\DatabasePostgres::selectDB + * @expectedException \Wikimedia\Rdbms\DBConnectionError + */ + public function testInvalidSelectDBIndependant2() { + $dbname = 'unittest-domain'; // explodes if DB is selected + $factory = $this->newLBFactoryMulti( + [ 'localDomain' => ( new DatabaseDomain( $dbname, null, '' ) )->getId() ], + [ + 'dbname' => 'do_not_select_me' // explodes if DB is selected + ] + ); + $lb = $factory->getMainLB(); + + if ( !wfGetDB( DB_MASTER )->databasesAreIndependent() ) { + $this->markTestSkipped( "Not applicable per databasesAreIndependent()" ); } + + $db = $lb->getConnection( DB_MASTER ); + \Wikimedia\suppressWarnings(); + $db->selectDB( 'garbage-db' ); + \Wikimedia\restoreWarnings(); } + /** + * @covers \Wikimedia\Rdbms\LoadBalancer::getConnection + * @covers \Wikimedia\Rdbms\LoadBalancer::redefineLocalDomain + * @covers \Wikimedia\Rdbms\DatabaseMysqlBase::selectDB + * @covers \Wikimedia\Rdbms\DatabaseMssql::selectDB + * @covers DatabaseOracle::selectDB + */ public function testRedefineLocalDomain() { global $wgDBname; -- 2.20.1