From 434d5a6321329ec53110faf441fb825c609347ac Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Wed, 28 Feb 2018 12:56:34 -0800 Subject: [PATCH] rdbms: allow construction of Database objects without connecting * Database::factory() supports a $connect parameter, that defaults to NEW_CONNECTED (current behavior) but can also be NEW_UNCONNECTED. * Add tests asserting the type of various instances returned from Database::factory(). * Clean up sqlite "conn" field handling to handle cases of it not being set, just as other classes do. * Add some comments about the return type of doQuery(). Change-Id: Ic0837cfdb35326c2045133d664abd29043d48c03 --- includes/libs/rdbms/database/Database.php | 96 ++++++++++++------- .../libs/rdbms/database/DatabaseMysqli.php | 9 +- .../libs/rdbms/database/DatabaseSqlite.php | 78 +++++++++------ .../libs/rdbms/database/DatabaseTest.php | 28 +++++- 4 files changed, 148 insertions(+), 63 deletions(-) diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 2c59963c6d..8596690411 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -33,6 +33,7 @@ use Wikimedia\Timestamp\ConvertibleTimestamp; use Wikimedia; use BagOStuff; use HashBagOStuff; +use LogicException; use InvalidArgumentException; use Exception; use RuntimeException; @@ -62,19 +63,24 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware /** @var string Whether lock granularity is on the level of the entire database */ const ATTR_DB_LEVEL_LOCKING = 'db-level-locking'; + /** @var int New Database instance will not be connected yet when returned */ + const NEW_UNCONNECTED = 0; + /** @var int New Database instance will already be connected when returned */ + const NEW_CONNECTED = 1; + /** @var string SQL query */ protected $lastQuery = ''; /** @var float|bool UNIX timestamp of last write query */ protected $lastWriteTime = false; /** @var string|bool */ protected $phpError = false; - /** @var string */ + /** @var string Server that this instance is currently connected to */ protected $server; - /** @var string */ + /** @var string User that this instance is currently connected under the name of */ protected $user; - /** @var string */ + /** @var string Password used to establish the current connection */ protected $password; - /** @var string */ + /** @var string Database that this instance is currently connected to */ protected $dbName; /** @var array[] $aliases Map of (table => (dbname, schema, prefix) map) */ protected $tableAliases = []; @@ -82,7 +88,8 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware protected $cliMode; /** @var string Agent name for query profiling */ protected $agent; - + /** @var array Parameters used by initConnection() to establish a connection */ + protected $connectionParams = []; /** @var BagOStuff APC cache */ protected $srvCache; /** @var LoggerInterface */ @@ -244,18 +251,13 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware protected $nonNativeInsertSelectBatchSize = 10000; /** - * Constructor and database handle and attempt to connect to the DB server - * - * IDatabase classes should not be constructed directly in external - * code. Database::factory() should be used instead. - * + * @note: exceptions for missing libraries/drivers should be thrown in initConnection() * @param array $params Parameters passed from Database::factory() */ - function __construct( array $params ) { - $server = $params['host']; - $user = $params['user']; - $password = $params['password']; - $dbName = $params['dbname']; + protected function __construct( array $params ) { + foreach ( [ 'host', 'user', 'password', 'dbname' ] as $name ) { + $this->connectionParams[$name] = $params[$name]; + } $this->schema = $params['schema']; $this->tablePrefix = $params['tablePrefix']; @@ -291,13 +293,22 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware // Set initial dummy domain until open() sets the final DB/prefix $this->currentDomain = DatabaseDomain::newUnspecified(); + } - if ( $user ) { - $this->open( $server, $user, $password, $dbName ); - } elseif ( $this->requiresDatabaseUser() ) { - throw new InvalidArgumentException( "No database user provided." ); + /** + * Initialize the connection to the database over the wire (or to local files) + * + * @throws LogicException + * @throws InvalidArgumentException + * @throws DBConnectionError + * @since 1.31 + */ + final public function initConnection() { + if ( $this->isOpen() ) { + throw new LogicException( __METHOD__ . ': already connected.' ); } - + // Establish the connection + $this->doInitConnection(); // Set the domain object after open() sets the relevant fields if ( $this->dbName != '' ) { // Domains with server scope but a table prefix are not used by IDatabase classes @@ -305,6 +316,26 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } } + /** + * Actually connect to the database over the wire (or to local files) + * + * @throws InvalidArgumentException + * @throws DBConnectionError + * @since 1.31 + */ + protected function doInitConnection() { + if ( strlen( $this->connectionParams['user'] ) ) { + $this->open( + $this->connectionParams['host'], + $this->connectionParams['user'], + $this->connectionParams['password'], + $this->connectionParams['dbname'] + ); + } else { + throw new InvalidArgumentException( "No database user provided." ); + } + } + /** * Construct a Database subclass instance given a database type and parameters * @@ -343,11 +374,12 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware * - agent: Optional name used to identify the end-user in query profiling/logging. * - srvCache: Optional BagOStuff instance to an APC-style cache. * - nonNativeInsertSelectBatchSize: Optional batch size for non-native INSERT SELECT emulation. + * @param int $connect One of the class constants (NEW_CONNECTED, NEW_UNCONNECTED) [optional] * @return Database|null If the database driver or extension cannot be found * @throws InvalidArgumentException If the database driver or extension cannot be found * @since 1.18 */ - final public static function factory( $dbType, $p = [] ) { + final public static function factory( $dbType, $p = [], $connect = self::NEW_CONNECTED ) { $class = self::getClass( $dbType, isset( $p['driver'] ) ? $p['driver'] : null ); if ( class_exists( $class ) && is_subclass_of( $class, IDatabase::class ) ) { @@ -380,7 +412,11 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware }; } + /** @var Database $conn */ $conn = new $class( $p ); + if ( $connect == self::NEW_CONNECTED ) { + $conn->initConnection(); + } } else { $conn = null; } @@ -859,11 +895,13 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware } /** - * The DBMS-dependent part of query() + * Run a query and return a DBMS-dependent wrapper (that has all IResultWrapper methods) + * + * This might return things, such as mysqli_result, that do not formally implement + * IResultWrapper, but nonetheless implement all of its methods correctly * * @param string $sql SQL query. - * @return ResultWrapper|bool Result object to feed to fetchObject, - * fetchRow, ...; or false on failure + * @return IResultWrapper|bool Iterator to feed to fetchObject/fetchRow; false on failure */ abstract protected function doQuery( $sql ); @@ -3865,14 +3903,6 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->tableAliases = $aliases; } - /** - * @return bool Whether a DB user is required to access the DB - * @since 1.28 - */ - protected function requiresDatabaseUser() { - return true; - } - /** * Get the underlying binding connection handle * @@ -3880,7 +3910,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware * This catches broken callers than catch and ignore disconnection exceptions. * Unlike checking isOpen(), this is safe to call inside of open(). * - * @return resource|object + * @return mixed * @throws DBUnexpectedError * @since 1.26 */ diff --git a/includes/libs/rdbms/database/DatabaseMysqli.php b/includes/libs/rdbms/database/DatabaseMysqli.php index 984e1c0d26..0a5450cc5e 100644 --- a/includes/libs/rdbms/database/DatabaseMysqli.php +++ b/includes/libs/rdbms/database/DatabaseMysqli.php @@ -37,7 +37,7 @@ use stdClass; class DatabaseMysqli extends DatabaseMysqlBase { /** * @param string $sql - * @return resource + * @return mysqli_result */ protected function doQuery( $sql ) { $conn = $this->getBindingHandle(); @@ -332,6 +332,13 @@ class DatabaseMysqli extends DatabaseMysqlBase { return (string)$this->conn; } } + + /** + * @return mysqli + */ + protected function getBindingHandle() { + return parent::getBindingHandle(); + } } class_alias( DatabaseMysqli::class, 'DatabaseMysqli' ); diff --git a/includes/libs/rdbms/database/DatabaseSqlite.php b/includes/libs/rdbms/database/DatabaseSqlite.php index 83c88149b0..d0d62e9edb 100644 --- a/includes/libs/rdbms/database/DatabaseSqlite.php +++ b/includes/libs/rdbms/database/DatabaseSqlite.php @@ -69,24 +69,13 @@ class DatabaseSqlite extends Database { */ function __construct( array $p ) { if ( isset( $p['dbFilePath'] ) ) { - parent::__construct( $p ); - // Standalone .sqlite file mode. - // Super doesn't open when $user is false, but we can work with $dbName, - // which is derived from the file path in this case. - $this->openFile( $p['dbFilePath'] ); - $lockDomain = md5( $p['dbFilePath'] ); - } elseif ( !isset( $p['dbDirectory'] ) ) { - throw new InvalidArgumentException( "Need 'dbDirectory' or 'dbFilePath' parameter." ); - } else { + $this->dbPath = $p['dbFilePath']; + $lockDomain = md5( $this->dbPath ); + } elseif ( isset( $p['dbDirectory'] ) ) { $this->dbDir = $p['dbDirectory']; - $this->dbName = $p['dbname']; - $lockDomain = $this->dbName; - // Stock wiki mode using standard file names per DB. - parent::__construct( $p ); - // Super doesn't open when $user is false, but we can work with $dbName - if ( $p['dbname'] && !$this->isOpen() ) { - $this->open( $p['host'], $p['user'], $p['password'], $p['dbname'] ); - } + $lockDomain = $p['dbname']; + } else { + throw new InvalidArgumentException( "Need 'dbDirectory' or 'dbFilePath' parameter." ); } $this->trxMode = isset( $p['trxMode'] ) ? strtoupper( $p['trxMode'] ) : null; @@ -101,6 +90,8 @@ class DatabaseSqlite extends Database { 'domain' => $lockDomain, 'lockDirectory' => "{$this->dbDir}/locks" ] ); + + parent::__construct( $p ); } protected static function getAttributes() { @@ -126,6 +117,28 @@ class DatabaseSqlite extends Database { return $db; } + protected function doInitConnection() { + if ( $this->dbPath !== null ) { + // Standalone .sqlite file mode. + $this->openFile( $this->dbPath ); + } elseif ( $this->dbDir !== null ) { + // Stock wiki mode using standard file names per DB + if ( strlen( $this->connectionParams['dbname'] ) ) { + $this->open( + $this->connectionParams['host'], + $this->connectionParams['user'], + $this->connectionParams['password'], + $this->connectionParams['dbname'] + ); + } else { + // Caller will manually call open() later? + $this->connLogger->debug( __METHOD__ . ': no database opened.' ); + } + } else { + throw new InvalidArgumentException( "Need 'dbDirectory' or 'dbFilePath' parameter." ); + } + } + /** * @return string */ @@ -146,7 +159,7 @@ class DatabaseSqlite extends Database { * NOTE: only $dbName is used, the other parameters are irrelevant for SQLite databases * * @param string $server - * @param string $user + * @param string $user Unused * @param string $pass * @param string $dbName * @@ -162,6 +175,10 @@ class DatabaseSqlite extends Database { } $this->openFile( $fileName ); + if ( $this->conn ) { + $this->dbName = $dbName; + } + return (bool)$this->conn; } @@ -192,7 +209,7 @@ class DatabaseSqlite extends Database { throw new DBConnectionError( $this, $err ); } - $this->opened = !!$this->conn; + $this->opened = is_object( $this->conn ); if ( $this->opened ) { # Set error codes only, don't raise exceptions $this->conn->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_SILENT ); @@ -315,7 +332,7 @@ class DatabaseSqlite extends Database { * @return bool|ResultWrapper */ protected function doQuery( $sql ) { - $res = $this->conn->query( $sql ); + $res = $this->getBindingHandle()->query( $sql ); if ( $res === false ) { return false; } @@ -451,7 +468,7 @@ class DatabaseSqlite extends Database { */ function insertId() { // PDO::lastInsertId yields a string :( - return intval( $this->conn->lastInsertId() ); + return intval( $this->getBindingHandle()->lastInsertId() ); } /** @@ -724,7 +741,7 @@ class DatabaseSqlite extends Database { * @return string Version information from the database */ function getServerVersion() { - $ver = $this->conn->getAttribute( PDO::ATTR_SERVER_VERSION ); + $ver = $this->getBindingHandle()->getAttribute( PDO::ATTR_SERVER_VERSION ); return $ver; } @@ -814,7 +831,7 @@ class DatabaseSqlite extends Database { ); return "x'" . bin2hex( (string)$s ) . "'"; } else { - return $this->conn->quote( (string)$s ); + return $this->getBindingHandle()->quote( (string)$s ); } } @@ -1059,15 +1076,20 @@ class DatabaseSqlite extends Database { } } - protected function requiresDatabaseUser() { - return false; // just a file - } - /** * @return string */ public function __toString() { - return 'SQLite ' . (string)$this->conn->getAttribute( PDO::ATTR_SERVER_VERSION ); + return is_object( $this->conn ) + ? 'SQLite ' . (string)$this->conn->getAttribute( PDO::ATTR_SERVER_VERSION ) + : '(not connected)'; + } + + /** + * @return PDO + */ + protected function getBindingHandle() { + return parent::getBindingHandle(); } } diff --git a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php index 6adbc758ad..85574b743e 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DatabaseTest.php @@ -1,11 +1,14 @@ db = new DatabaseTestHelper( __CLASS__ . '::' . $this->getName() ); } + /** + * @dataProvider provideAddQuotes + * @covers Wikimedia\Rdbms\Database::factory + */ + public function testFactory() { + $m = Database::NEW_UNCONNECTED; // no-connect mode + $p = [ 'host' => 'localhost', 'user' => 'me', 'password' => 'myself', 'dbname' => 'i' ]; + + $this->assertInstanceOf( DatabaseMysqli::class, Database::factory( 'mysqli', $p, $m ) ); + $this->assertInstanceOf( DatabaseMysqli::class, Database::factory( 'MySqli', $p, $m ) ); + $this->assertInstanceOf( DatabaseMysqli::class, Database::factory( 'MySQLi', $p, $m ) ); + $this->assertInstanceOf( DatabasePostgres::class, Database::factory( 'postgres', $p, $m ) ); + $this->assertInstanceOf( DatabasePostgres::class, Database::factory( 'Postgres', $p, $m ) ); + + $x = $p + [ 'port' => 10000, 'UseWindowsAuth' => false ]; + $this->assertInstanceOf( DatabaseMssql::class, Database::factory( 'mssql', $x, $m ) ); + + $x = $p + [ 'dbFilePath' => 'some/file.sqlite' ]; + $this->assertInstanceOf( DatabaseSqlite::class, Database::factory( 'sqlite', $x, $m ) ); + $x = $p + [ 'dbDirectory' => 'some/file' ]; + $this->assertInstanceOf( DatabaseSqlite::class, Database::factory( 'sqlite', $x, $m ) ); + } + public static function provideAddQuotes() { return [ [ null, 'NULL' ], -- 2.20.1