From 236a0941c0c67fff9eb5386da1cd64eb15d0bf6e Mon Sep 17 00:00:00 2001 From: Amir Sarabadani Date: Wed, 8 Dec 2021 13:08:11 +0100 Subject: [PATCH] rdmbs: Start of SQLPlatform to split out of Database This is the first step to split parts of Database that doesn't require a connection and are used for query parts. Bug: T299691 Change-Id: I140aa4328865994499926f898233867ce383908c --- autoload.php | 1 + includes/AutoLoader.php | 1 + .../libs/rdbms/database/DatabaseMysqlBase.php | 22 +++-- .../libs/rdbms/database/DatabasePostgres.php | 22 +++-- .../libs/rdbms/database/DatabaseSqlite.php | 16 ++-- includes/libs/rdbms/database/IDatabase.php | 34 +------ .../libs/rdbms/exception/DBLanguageError.php | 36 ++++++++ includes/libs/rdbms/platform/ISQLPlatform.php | 60 +++++++++++++ .../libs/rdbms/platform/MySQLPlatform.php | 38 ++++++++ .../libs/rdbms/platform/PostgresPlatform.php | 31 +++++++ includes/libs/rdbms/platform/SQLPlatform.php | 61 +++++++++++++ .../libs/rdbms/platform/SqlitePlatform.php | 31 +++++++ .../rdbms/database/DatabaseMysqlBaseTest.php | 82 +++-------------- .../libs/rdbms/platform/MySQLPlatformTest.php | 88 +++++++++++++++++++ 14 files changed, 402 insertions(+), 121 deletions(-) create mode 100644 includes/libs/rdbms/exception/DBLanguageError.php create mode 100644 includes/libs/rdbms/platform/ISQLPlatform.php create mode 100644 includes/libs/rdbms/platform/MySQLPlatform.php create mode 100644 includes/libs/rdbms/platform/PostgresPlatform.php create mode 100644 includes/libs/rdbms/platform/SQLPlatform.php create mode 100644 includes/libs/rdbms/platform/SqlitePlatform.php create mode 100644 tests/phpunit/unit/includes/libs/rdbms/platform/MySQLPlatformTest.php diff --git a/autoload.php b/autoload.php index dd9666b5728..d733d1519cb 100644 --- a/autoload.php +++ b/autoload.php @@ -1807,6 +1807,7 @@ $wgAutoloadLocalClasses = [ 'Wikimedia\\Rdbms\\DBConnectionError' => __DIR__ . '/includes/libs/rdbms/exception/DBConnectionError.php', 'Wikimedia\\Rdbms\\DBError' => __DIR__ . '/includes/libs/rdbms/exception/DBError.php', 'Wikimedia\\Rdbms\\DBExpectedError' => __DIR__ . '/includes/libs/rdbms/exception/DBExpectedError.php', + 'Wikimedia\\Rdbms\\DBLanguageError' => __DIR__ . '/includes/libs/rdbms/exception/DBLanguageError.php', 'Wikimedia\\Rdbms\\DBPrimaryPos' => __DIR__ . '/includes/libs/rdbms/database/position/DBPrimaryPos.php', 'Wikimedia\\Rdbms\\DBQueryDisconnectedError' => __DIR__ . '/includes/libs/rdbms/exception/DBQueryDisconnectedError.php', 'Wikimedia\\Rdbms\\DBQueryError' => __DIR__ . '/includes/libs/rdbms/exception/DBQueryError.php', diff --git a/includes/AutoLoader.php b/includes/AutoLoader.php index 4ee73e8e362..7de64be0f3c 100644 --- a/includes/AutoLoader.php +++ b/includes/AutoLoader.php @@ -179,6 +179,7 @@ class AutoLoader { 'MediaWiki\\Widget\\' => __DIR__ . '/widget/', 'Wikimedia\\' => __DIR__ . '/libs/', 'Wikimedia\\Http\\' => __DIR__ . '/libs/http/', + 'Wikimedia\\Rdbms\\Platform\\' => __DIR__ . '/libs/rdbms/platform/', 'Wikimedia\\UUID\\' => __DIR__ . '/libs/uuid/', ]; } diff --git a/includes/libs/rdbms/database/DatabaseMysqlBase.php b/includes/libs/rdbms/database/DatabaseMysqlBase.php index 72a0479b05d..3ebc7876288 100644 --- a/includes/libs/rdbms/database/DatabaseMysqlBase.php +++ b/includes/libs/rdbms/database/DatabaseMysqlBase.php @@ -26,6 +26,8 @@ use InvalidArgumentException; use RuntimeException; use stdClass; use Wikimedia\AtEase\AtEase; +use Wikimedia\Rdbms\Platform\ISQLPlatform; +use Wikimedia\Rdbms\Platform\MySQLPlatform; /** * Database abstraction object for MySQL. @@ -81,6 +83,9 @@ abstract class DatabaseMysqlBase extends Database { /** @var float Warn if lag estimates are made for transactions older than this many seconds */ private const LAG_STALE_WARN_THRESHOLD = 0.100; + /** @var ISQLPlatform */ + protected $platform; + /** * Additional $params include: * - lagDetectionMethod : set to one of (Seconds_Behind_Master,pt-heartbeat). @@ -114,6 +119,7 @@ abstract class DatabaseMysqlBase extends Database { $this->utf8Mode = !empty( $params['utf8Mode'] ); $this->insertSelectIsSafe = isset( $params['insertSelectIsSafe'] ) ? (bool)$params['insertSelectIsSafe'] : null; + $this->platform = new MySQLPlatform(); parent::__construct( $params ); } @@ -164,7 +170,7 @@ abstract class DatabaseMysqlBase extends Database { if ( !is_int( $val ) && !is_float( $val ) ) { $val = $this->addQuotes( $val ); } - $set[] = $this->addIdentifierQuotes( $var ) . ' = ' . $val; + $set[] = $this->platform->addIdentifierQuotes( $var ) . ' = ' . $val; } // @phan-suppress-next-next-line PhanRedundantCondition @@ -205,7 +211,7 @@ abstract class DatabaseMysqlBase extends Database { } if ( $database !== $this->getDBname() ) { - $sql = 'USE ' . $this->addIdentifierQuotes( $database ); + $sql = 'USE ' . $this->platform->addIdentifierQuotes( $database ); list( $res, $err, $errno ) = $this->executeQuery( $sql, __METHOD__, self::QUERY_IGNORE_DBO_TRX ); @@ -365,7 +371,7 @@ abstract class DatabaseMysqlBase extends Database { // If the database has been specified (such as for shared tables), use "FROM" if ( $database !== '' ) { - $encDatabase = $this->addIdentifierQuotes( $database ); + $encDatabase = $this->platform->addIdentifierQuotes( $database ); $sql = "SHOW TABLES FROM $encDatabase LIKE '$encLike'"; } else { $sql = "SHOW TABLES LIKE '$encLike'"; @@ -459,15 +465,15 @@ abstract class DatabaseMysqlBase extends Database { abstract protected function mysqlRealEscapeString( $s ); /** - * MySQL uses `backticks` for identifier quoting instead of the sql standard "double quotes". - * * @param string $s * @return string */ public function addIdentifierQuotes( $s ) { - // Characters in the range \u0001-\uFFFF are valid in a quoted identifier - // Remove NUL bytes and escape backticks by doubling - return '`' . str_replace( [ "\0", '`' ], [ '', '``' ], $s ) . '`'; + // Tests disabling constructor + if ( !$this->platform ) { + $this->platform = new MySQLPlatform(); + } + return $this->platform->addIdentifierQuotes( $s ); } /** diff --git a/includes/libs/rdbms/database/DatabasePostgres.php b/includes/libs/rdbms/database/DatabasePostgres.php index eaac75a6797..3c368ba417c 100644 --- a/includes/libs/rdbms/database/DatabasePostgres.php +++ b/includes/libs/rdbms/database/DatabasePostgres.php @@ -23,6 +23,8 @@ namespace Wikimedia\Rdbms; use RuntimeException; +use Wikimedia\Rdbms\Platform\ISQLPlatform; +use Wikimedia\Rdbms\Platform\PostgresPlatform; use Wikimedia\Timestamp\ConvertibleTimestamp; use Wikimedia\WaitConditionLoop; @@ -44,6 +46,9 @@ class DatabasePostgres extends Database { /** @var resource|null */ private $lastResultHandle; + /** @var ISQLPlatform */ + private $platform; + /** * @see Database::__construct() * @param array $params Additional parameters include: @@ -63,6 +68,7 @@ class DatabasePostgres extends Database { } parent::__construct( $params ); + $this->platform = new PostgresPlatform(); } public function getType() { @@ -129,7 +135,7 @@ class DatabasePostgres extends Database { ]; foreach ( $variables as $var => $val ) { $this->query( - 'SET ' . $this->addIdentifierQuotes( $var ) . ' = ' . $this->addQuotes( $val ), + 'SET ' . $this->platform->addIdentifierQuotes( $var ) . ' = ' . $this->addQuotes( $val ), __METHOD__, self::QUERY_NO_RETRY | self::QUERY_CHANGE_TRX ); @@ -456,7 +462,9 @@ __INDEXATTR__; $toCheck = array_merge( $toCheck, $name ); } else { // Quote alias names so $this->tableName() won't mangle them - $options['FOR UPDATE'][] = $hasAlias ? $this->addIdentifierQuotes( $alias ) : $alias; + $options['FOR UPDATE'][] = $hasAlias ? + // @phan-suppress-next-line PhanTypeMismatchArgumentNullable + $this->platform->addIdentifierQuotes( $alias ) : $alias; } } } @@ -674,8 +682,8 @@ __INDEXATTR__; public function duplicateTableStructure( $oldName, $newName, $temporary = false, $fname = __METHOD__ ) { - $newNameE = $this->addIdentifierQuotes( $newName ); - $oldNameE = $this->addIdentifierQuotes( $oldName ); + $newNameE = $this->platform->addIdentifierQuotes( $newName ); + $oldNameE = $this->platform->addIdentifierQuotes( $oldName ); $temporary = $temporary ? 'TEMPORARY' : ''; @@ -705,8 +713,8 @@ __INDEXATTR__; if ( $row ) { $field = $row->attname; $newSeq = "{$newName}_{$field}_seq"; - $fieldE = $this->addIdentifierQuotes( $field ); - $newSeqE = $this->addIdentifierQuotes( $newSeq ); + $fieldE = $this->platform->addIdentifierQuotes( $field ); + $newSeqE = $this->platform->addIdentifierQuotes( $newSeq ); $newSeqQ = $this->addQuotes( $newSeq ); $this->query( "CREATE $temporary SEQUENCE $newSeqE OWNED BY $newNameE.$fieldE", @@ -921,7 +929,7 @@ __INDEXATTR__; } else { // Prepend the desired schema to the search path (T17816) $search_path = $this->getSearchPath(); - array_unshift( $search_path, $this->addIdentifierQuotes( $desiredSchema ) ); + array_unshift( $search_path, $this->platform->addIdentifierQuotes( $desiredSchema ) ); $this->setSearchPath( $search_path ); $this->coreSchema = $desiredSchema; $this->queryLogger->debug( diff --git a/includes/libs/rdbms/database/DatabaseSqlite.php b/includes/libs/rdbms/database/DatabaseSqlite.php index 8f3e5b5717a..ef412b0684b 100644 --- a/includes/libs/rdbms/database/DatabaseSqlite.php +++ b/includes/libs/rdbms/database/DatabaseSqlite.php @@ -29,6 +29,8 @@ use NullLockManager; use PDO; use PDOException; use RuntimeException; +use Wikimedia\Rdbms\Platform\ISQLPlatform; +use Wikimedia\Rdbms\Platform\SqlitePlatform; /** * @ingroup Database @@ -67,6 +69,9 @@ class DatabaseSqlite extends Database { 'temp_store' => [ 'FILE', 'MEMORY' ] ]; + /** @var ISQLPlatform */ + private $platform; + /** * Additional params include: * - dbDirectory : directory containing the DB and the lock file directory @@ -89,6 +94,7 @@ class DatabaseSqlite extends Database { $this->trxMode = strtoupper( $params['trxMode'] ?? '' ); $this->lockMgr = $this->makeLockManager(); + $this->platform = new SqlitePlatform(); } protected static function getAttributes() { @@ -922,9 +928,9 @@ class DatabaseSqlite extends Database { $sqlCreateTable = $obj->sql; $sqlCreateTable = preg_replace( '/(?<=\W)"?' . - preg_quote( trim( $this->addIdentifierQuotes( $oldName ), '"' ), '/' ) . + preg_quote( trim( $this->platform->addIdentifierQuotes( $oldName ), '"' ), '/' ) . '"?(?=\W)/', - $this->addIdentifierQuotes( $newName ), + $this->platform->addIdentifierQuotes( $newName ), $sqlCreateTable, 1 ); @@ -966,8 +972,8 @@ class DatabaseSqlite extends Database { } // Try to come up with a new index name, given indexes have database scope in SQLite $indexName = $newName . '_' . $index->name; - $sqlIndex .= ' ' . $this->addIdentifierQuotes( $indexName ) . - ' ON ' . $this->addIdentifierQuotes( $newName ); + $sqlIndex .= ' ' . $this->platform->addIdentifierQuotes( $indexName ) . + ' ON ' . $this->platform->addIdentifierQuotes( $newName ); $indexInfo = $this->query( 'PRAGMA INDEX_INFO(' . $this->addQuotes( $index->name ) . ')', @@ -1046,7 +1052,7 @@ class DatabaseSqlite extends Database { $encSeqNames[] = $this->addQuotes( $this->tableName( $table, 'raw' ) ); } - $encMasterTable = $this->addIdentifierQuotes( 'sqlite_sequence' ); + $encMasterTable = $this->platform->addIdentifierQuotes( 'sqlite_sequence' ); $this->query( "DELETE FROM $encMasterTable WHERE name IN(" . implode( ',', $encSeqNames ) . ")", $fname, diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php index 7b2216f03c4..c840fe90e2a 100644 --- a/includes/libs/rdbms/database/IDatabase.php +++ b/includes/libs/rdbms/database/IDatabase.php @@ -22,6 +22,7 @@ namespace Wikimedia\Rdbms; use Exception; use InvalidArgumentException; use stdClass; +use Wikimedia\Rdbms\Platform\ISQLPlatform; use Wikimedia\ScopedCallback; /** @@ -35,7 +36,7 @@ use Wikimedia\ScopedCallback; * @note IDatabase and DBConnRef should be updated to reflect any changes * @ingroup Database */ -interface IDatabase { +interface IDatabase extends ISQLPlatform { /** @var int Callback triggered immediately due to no active transaction */ public const TRIGGER_IDLE = 1; /** @var int Callback triggered by COMMIT */ @@ -990,26 +991,6 @@ interface IDatabase { */ public function factorConds( $condsArray ); - /** - * @param string|int $field - * @return string - */ - public function bitNot( $field ); - - /** - * @param string|int $fieldLeft - * @param string|int $fieldRight - * @return string - */ - public function bitAnd( $fieldLeft, $fieldRight ); - - /** - * @param string|int $fieldLeft - * @param string|int $fieldRight - * @return string - */ - public function bitOr( $fieldLeft, $fieldRight ); - /** * Build a concatenation list to feed into a SQL query * @param string[] $stringList Raw SQL expression list; caller is responsible for escaping @@ -1210,17 +1191,6 @@ interface IDatabase { */ public function addQuotes( $s ); - /** - * Escape a SQL identifier (e.g. table, column, database) for use in a SQL query - * - * Depending on the database this will either be `backticks` or "double quotes" - * - * @param string $s - * @return string - * @since 1.33 - */ - public function addIdentifierQuotes( $s ); - /** * LIKE statement wrapper * diff --git a/includes/libs/rdbms/exception/DBLanguageError.php b/includes/libs/rdbms/exception/DBLanguageError.php new file mode 100644 index 00000000000..b6eda7c2216 --- /dev/null +++ b/includes/libs/rdbms/exception/DBLanguageError.php @@ -0,0 +1,36 @@ +getMockBuilder( DatabaseMysqli::class ) - ->disableOriginalConstructor() - ->onlyMethods( [] ) - ->getMock(); - - /** @var IDatabase $db */ - $quoted = $db->addIdentifierQuotes( $in ); - $this->assertEquals( $expected, $quoted ); - } - - /** - * Feeds testAddIdentifierQuotes - * - * Named per T22281 convention. - */ - public static function provideDiapers() { - return [ - // Format: expected, input - [ '``', '' ], - - // Yeah I really hate loosely typed PHP idiocies nowadays - [ '``', null ], - - // Dear codereviewer, guess what addIdentifierQuotes() - // will return with thoses: - [ '``', false ], - [ '`1`', true ], - - // We never know what could happen - [ '`0`', 0 ], - [ '`1`', 1 ], - - // Whatchout! Should probably use something more meaningful - [ "`'`", "'" ], # single quote - [ '`"`', '"' ], # double quote - [ '````', '`' ], # backtick - [ '`’`', '’' ], # apostrophe (look at your encyclopedia) - - // sneaky NUL bytes are lurking everywhere - [ '``', "\0" ], - [ '`xyzzy`', "\0x\0y\0z\0z\0y\0" ], - - // unicode chars - [ - "`\u{0001}a\u{FFFF}b`", - "\u{0001}a\u{FFFF}b" - ], - [ - "`\u{0001}\u{FFFF}`", - "\u{0001}\u{0000}\u{FFFF}\u{0000}" - ], - [ '`☃`', '☃' ], - [ '`メインページ`', 'メインページ' ], - [ '`Басты_бет`', 'Басты_бет' ], - - // Real world: - [ '`Alix`', 'Alix' ], # while( ! $recovered ) { sleep(); } - [ '`Backtick: ```', 'Backtick: `' ], - [ '`This is a test`', 'This is a test' ], - ]; - } - private function getMockForViews(): IMaintainableDatabase { $db = $this->getMockBuilder( DatabaseMysqli::class ) ->disableOriginalConstructor() @@ -734,13 +668,18 @@ class DatabaseMysqlBaseTest extends PHPUnit\Framework\TestCase { public function testIndexAliases() { $db = $this->getMockBuilder( DatabaseMysqli::class ) ->disableOriginalConstructor() - ->onlyMethods( [ 'mysqlRealEscapeString', 'dbSchema', 'tablePrefix' ] ) + ->onlyMethods( [ 'mysqlRealEscapeString', 'dbSchema', 'tablePrefix', 'addIdentifierQuotes' ] ) ->getMock(); $db->method( 'mysqlRealEscapeString' )->willReturnCallback( static function ( $s ) { return str_replace( "'", "\\'", $s ); } ); + $db->method( 'addIdentifierQuotes' )->willReturnCallback( + static function ( $s ) { + return ( new MySQLPlatform() )->addIdentifierQuotes( $s ); + } + ); /** @var IDatabase $db */ $db->setIndexAliases( [ 'a_b_idx' => 'a_c_idx' ] ); @@ -768,13 +707,18 @@ class DatabaseMysqlBaseTest extends PHPUnit\Framework\TestCase { public function testTableAliases() { $db = $this->getMockBuilder( DatabaseMysqli::class ) ->disableOriginalConstructor() - ->onlyMethods( [ 'mysqlRealEscapeString', 'dbSchema', 'tablePrefix' ] ) + ->onlyMethods( [ 'mysqlRealEscapeString', 'dbSchema', 'tablePrefix', 'addIdentifierQuotes' ] ) ->getMock(); $db->method( 'mysqlRealEscapeString' )->willReturnCallback( static function ( $s ) { return str_replace( "'", "\\'", $s ); } ); + $db->method( 'addIdentifierQuotes' )->willReturnCallback( + static function ( $s ) { + return ( new MySQLPlatform() )->addIdentifierQuotes( $s ); + } + ); /** @var IDatabase $db */ $db->setTableAliases( [ diff --git a/tests/phpunit/unit/includes/libs/rdbms/platform/MySQLPlatformTest.php b/tests/phpunit/unit/includes/libs/rdbms/platform/MySQLPlatformTest.php new file mode 100644 index 00000000000..aa7ccc49093 --- /dev/null +++ b/tests/phpunit/unit/includes/libs/rdbms/platform/MySQLPlatformTest.php @@ -0,0 +1,88 @@ +addIdentifierQuotes( $in ); + $this->assertEquals( $expected, $quoted ); + } + + /** + * Feeds testAddIdentifierQuotes + * + * Named per T22281 convention. + */ + public static function provideDiapers() { + return [ + // Format: expected, input + [ '``', '' ], + + // Yeah I really hate loosely typed PHP idiocies nowadays + [ '``', null ], + + // Dear codereviewer, guess what addIdentifierQuotes() + // will return with thoses: + [ '``', false ], + [ '`1`', true ], + + // We never know what could happen + [ '`0`', 0 ], + [ '`1`', 1 ], + + // Whatchout! Should probably use something more meaningful + [ "`'`", "'" ], # single quote + [ '`"`', '"' ], # double quote + [ '````', '`' ], # backtick + [ '`’`', '’' ], # apostrophe (look at your encyclopedia) + + // sneaky NUL bytes are lurking everywhere + [ '``', "\0" ], + [ '`xyzzy`', "\0x\0y\0z\0z\0y\0" ], + + // unicode chars + [ + "`\u{0001}a\u{FFFF}b`", + "\u{0001}a\u{FFFF}b" + ], + [ + "`\u{0001}\u{FFFF}`", + "\u{0001}\u{0000}\u{FFFF}\u{0000}" + ], + [ '`☃`', '☃' ], + [ '`メインページ`', 'メインページ' ], + [ '`Басты_бет`', 'Басты_бет' ], + + // Real world: + [ '`Alix`', 'Alix' ], # while( ! $recovered ) { sleep(); } + [ '`Backtick: ```', 'Backtick: `' ], + [ '`This is a test`', 'This is a test' ], + ]; + } +}