Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

rdbms: fix table prefixing in "FOR UPDATE" clause generation in Postgres

Browse source 
Make SqlPlatform::tableNameWithAlias() include the unqualified table
name as an alias if doing so is not redundant. This assures that the
default alias from JoinGroupBase::addJoin(), equal to the unqualified
table name, will be usable in SQL (regardless of table prefixes).

Clean up use of identifier quotes for sqlite_master tables. The called
methods expect unqualified names and a passthrough exception already
exists for sqlite_* tables.

Use "block_target.bt_user" directly in ApiQueryBlocks and BlockPager,
instead of using addIdentifierQuotes(). The "block_target" alias is
automatically added to the SQL by the rdbms layer when it's not clearly
redundant, so it is always safe to use block_target.bt_user. Also, there
is no reason for aliases to include quote characters. They are supposed
to be simple alphanumerics like column names. This makes it easy for
tableNameWithAlias() to avoid redundant aliases by checking tableName().

Avoid unneeded quotes around pg_catalog.* table names in the Postgres
installer. The relevant documentation of methods like selectField() is
that the table names be unqualified (no quotes nor dots), though dots
are still supported internally for compatibility reasons and ease of
querying schemas like pg_catalog and information_schema.

Change-Id: Ic7d7826da31f49915141692cb3bd84ed1e872e96
This commit is contained in:
Aaron Schulz 2024-12-10 06:17:24 -08:00 committed by Reedy
parent fd31ca4e79
commit 4659cbcccc
9 changed files with 55 additions and 26 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
includes/api/ApiQueryBlocks.php
View file

@ -235,7 +235,7 @@ class ApiQueryBlocks extends ApiQueryBase {
if ( !$this->getAuthority()->isAllowed( 'hideuser' ) ) {
$this->addWhere(
$this->hideUserUtils->getExpression( $db, $db->tableName( 'block_target' ) . '.bt_user' )
$this->hideUserUtils->getExpression( $db, 'block_target.bt_user' )
);
}

10
includes/installer/PostgresInstaller.php
View file

@ -216,7 +216,7 @@ class PostgresInstaller extends DatabaseInstaller {
$conn = $status->getDB();
$superuser = $this->getVar( '_InstallUser' );
$row = $conn->selectRow( '"pg_catalog"."pg_roles"', '*',
$row = $conn->selectRow( 'pg_catalog.pg_roles', '*',
[ 'rolname' => $superuser ], __METHOD__ );
return $row;
@ -247,9 +247,9 @@ class PostgresInstaller extends DatabaseInstaller {
return false;
}
$conn = $status->getDB();
$installerId = $conn->selectField( '"pg_catalog"."pg_roles"', 'oid',
$installerId = $conn->selectField( 'pg_catalog.pg_roles', 'oid',
[ 'rolname' => $this->getVar( '_InstallUser' ) ], __METHOD__ );
$webId = $conn->selectField( '"pg_catalog"."pg_roles"', 'oid',
$webId = $conn->selectField( 'pg_catalog.pg_roles', 'oid',
[ 'rolname' => $this->getVar( 'wgDBuser' ) ], __METHOD__ );
return $this->isRoleMember( $conn, $installerId, $webId, $this->maxRoleSearchDepth );
@ -269,7 +269,7 @@ class PostgresInstaller extends DatabaseInstaller {
return true;
}
// Get all members of the given group
$res = $conn->select( '"pg_catalog"."pg_auth_members"', [ 'member' ],
$res = $conn->select( 'pg_catalog.pg_auth_members', [ 'member' ],
[ 'roleid' => $group ], __METHOD__ );
foreach ( $res as $row ) {
if ( $row->member == $targetMember ) {
@ -319,7 +319,7 @@ class PostgresInstaller extends DatabaseInstaller {
$conn = $status->getDB();
$dbName = $this->getVar( 'wgDBname' );
$exists = (bool)$conn->selectField( '"pg_catalog"."pg_database"', '1',
$exists = (bool)$conn->selectField( 'pg_catalog.pg_database', '1',
[ 'datname' => $dbName ], __METHOD__ );
if ( !$exists ) {
$safedb = $conn->addIdentifierQuotes( $dbName );

2
includes/installer/SqliteInstaller.php
View file

@ -334,7 +334,7 @@ EOT;
$module = DatabaseSqlite::getFulltextSearchModule();
$searchIndexSql = (string)$this->db->newSelectQueryBuilder()
->select( 'sql' )
->from( $this->db->addIdentifierQuotes( 'sqlite_master' ) )
->from( 'sqlite_master' )
->where( [ 'tbl_name' => $this->db->tableName( 'searchindex', 'raw' ) ] )
->caller( __METHOD__ )->fetchField();
$fts3tTable = ( stristr( $searchIndexSql, 'fts' ) !== false );

23
includes/libs/rdbms/platform/PostgresPlatform.php
View file

@ -85,24 +85,23 @@ class PostgresPlatform extends SQLPlatform {
reset( $toCheck );
while ( $toCheck ) {
$alias = key( $toCheck );
$name = $toCheck[$alias];
$table = $toCheck[$alias];
unset( $toCheck[$alias] );
$hasAlias = !is_numeric( $alias );
if ( !$hasAlias && is_string( $name ) ) {
$alias = $name;
if ( !is_string( $alias ) ) {
// No alias? Set it equal to the table name
$alias = $table;
}
if ( !isset( $join_conds[$alias] ) ||
!preg_match( '/^(?:LEFT|RIGHT|FULL)(?: OUTER)? JOIN$/i', $join_conds[$alias][0] )
) {
if ( is_array( $name ) ) {
if ( is_array( $table ) ) {
// It's a parenthesized group, process all the tables inside the group.
$toCheck = array_merge( $toCheck, $name );
$toCheck = array_merge( $toCheck, $table );
} else {
// Quote alias names so $this->tableName() won't mangle them
$options['FOR UPDATE'][] = $hasAlias ?
$this->addIdentifierQuotes( $alias ) : $alias;
// If an alias is declared, then any FOR UPDATE FOR must use it
$options['FOR UPDATE'][] = $alias;
}
}
}
@ -135,8 +134,10 @@ class PostgresPlatform extends SQLPlatform {
$preLimitTail .= $this->makeOrderBy( $options );
if ( isset( $options['FOR UPDATE'] ) ) {
$postLimitTail .= ' FOR UPDATE OF ' .
implode( ', ', array_map( [ $this, 'tableName' ], $options['FOR UPDATE'] ) );
$postLimitTail .= ' FOR UPDATE OF ' . implode(
', ',
array_map( [ $this, 'addIdentifierQuotes' ], $options['FOR UPDATE'] )
);
} elseif ( isset( $noKeyOptions['FOR UPDATE'] ) ) {
$postLimitTail .= ' FOR UPDATE';
}

19
includes/libs/rdbms/platform/SQLPlatform.php
View file

@ -90,7 +90,7 @@ class SQLPlatform implements ISQLPlatform {
public function addIdentifierQuotes( $s ) {
if ( strcspn( $s, "\0\"`'." ) !== strlen( $s ) ) {
throw new DBLanguageError(
"Identifier must not contain quote, dot or null characters"
"Identifier must not contain quote, dot or null characters: got '$s'"
);
}
$quoteChar = $this->getIdentifierQuoteChar();
@ -1027,15 +1027,24 @@ class SQLPlatform implements ISQLPlatform {
throw new InvalidArgumentException( "Table must be a string or Subquery" );
}
if ( $alias === false || $alias === $table ) {
if ( $alias === false ) {
if ( $table instanceof Subquery ) {
throw new InvalidArgumentException( "Subquery table missing alias" );
}
return $quotedTable;
$quotedTableWithAnyAlias = $quotedTable;
} elseif (
$alias === $table &&
(
str_contains( $alias, '.' ) ||
$this->tableName( $alias, 'raw' ) === $table
)
) {
$quotedTableWithAnyAlias = $quotedTable;
} else {
return $quotedTable . ' ' . $this->addIdentifierQuotes( $alias );
$quotedTableWithAnyAlias = $quotedTable . ' ' . $this->addIdentifierQuotes( $alias );
}
return $quotedTableWithAnyAlias;
}
public function tableName( string $name, $format = 'quoted' ) {

2
includes/search/SearchSqlite.php
View file

@ -38,7 +38,7 @@ class SearchSqlite extends SearchDatabase {
$dbr = $this->dbProvider->getReplicaDatabase();
$sql = (string)$dbr->newSelectQueryBuilder()
->select( 'sql' )
->from( $dbr->addIdentifierQuotes( 'sqlite_master' ) )
->from( 'sqlite_master' )
->where( [ 'tbl_name' => $dbr->tableName( 'searchindex', 'raw' ) ] )
->caller( __METHOD__ )->fetchField();

2
includes/specials/pagers/BlockListPager.php
View file

@ -487,7 +487,7 @@ class BlockListPager extends TablePager {
# With multiblocks we can't just rely on bl_deleted in the row being formatted
$info['fields']['hu_deleted'] = $this->hideUserUtils->getExpression(
$db,
$db->tableName( 'block_target' ) . '.bt_user',
'block_target.bt_user',
HideUserUtils::HIDDEN_USERS );
return $info;
}

5
tests/phpunit/includes/Revision/RevisionStoreDbTest.php
View file

@ -987,6 +987,11 @@ class RevisionStoreDbTest extends MediaWikiIntegrationTestCase {
$this->assertSame( $revRecord->getId(), $storeRecord->getId() );
$this->assertTrue( $storeRecord->getSlot( SlotRecord::MAIN )->getContent()->equals( $content ) );
$this->assertSame( __METHOD__, $storeRecord->getComment()->text );
$storeRecord = $store->getRevisionByPageId( $page->getId(), 0, IDBAccessObject::READ_LOCKING );
$this->assertSame( $revRecord->getId(), $storeRecord->getId() );
$this->assertTrue( $storeRecord->getSlot( SlotRecord::MAIN )->getContent()->equals( $content ) );
$this->assertSame( __METHOD__, $storeRecord->getComment()->text );
}
/**

16
tests/phpunit/unit/includes/libs/rdbms/database/DatabaseMySQLTest.php
View file

@ -541,6 +541,20 @@ class DatabaseMySQLTest extends TestCase {
$wdb->platform = new MySQLPlatform( new AddQuoterMock() );
/** @var IDatabase $db */
$db->setTableAliases( [
'meow' => [ 'dbname' => 'feline', 'schema' => null, 'prefix' => '' ]
] );
$sql = $db->newSelectQueryBuilder()
->select( 'field' )
->from( 'meow' )
->where( [ 'a' => 'x' ] )
->caller( __METHOD__ )->getSQL();
$this->assertSameSql(
"SELECT field FROM `feline`.`meow` `meow` WHERE a = 'x' ",
$sql
);
$db->setTableAliases( [
'meow' => [ 'dbname' => 'feline', 'schema' => null, 'prefix' => 'cat_' ]
] );
@ -551,7 +565,7 @@ class DatabaseMySQLTest extends TestCase {
->caller( __METHOD__ )->getSQL();
$this->assertSameSql(
"SELECT field FROM `feline`.`cat_meow` WHERE a = 'x' ",
"SELECT field FROM `feline`.`cat_meow` `meow` WHERE a = 'x' ",
$sql
);