From eb603040de8dcc164128080ba669df0ea930faee Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Thu, 21 Mar 2019 09:10:23 -0700 Subject: [PATCH] rdbms: add and enforce DB_REPLICA/DB_MASTER roles in DBConnRef Even if there is only one database, if a DB_REPLICA connection reference is obtained and an attempt is made to write it then it will still fail. This can make it easier to spot incorrect database usage even in a simple vanilla developer environment. Note that methods like ILoadBalancer::getConnectionRef() can be used for local connections as well as foreign ones. Change-Id: I5523daad1bdd64624d3e0dd41bfd8d078b1078b0 --- autoload.php | 1 + includes/libs/rdbms/database/DBConnRef.php | 79 ++++++++- includes/libs/rdbms/database/Database.php | 8 +- includes/libs/rdbms/database/IDatabase.php | 2 + .../rdbms/database/MaintainableDBConnRef.php | 14 ++ .../rdbms/exception/DBReadOnlyRoleError.php | 30 ++++ .../libs/rdbms/loadbalancer/ILoadBalancer.php | 4 +- .../libs/rdbms/loadbalancer/LoadBalancer.php | 25 ++- .../phpunit/includes/db/LoadBalancerTest.php | 152 +++++++++++++----- .../libs/rdbms/database/DBConnRefTest.php | 65 +++++++- 10 files changed, 321 insertions(+), 59 deletions(-) create mode 100644 includes/libs/rdbms/exception/DBReadOnlyRoleError.php diff --git a/autoload.php b/autoload.php index bb1b3b2ad3..8de4afd034 100644 --- a/autoload.php +++ b/autoload.php @@ -1636,6 +1636,7 @@ $wgAutoloadLocalClasses = [ 'Wikimedia\\Rdbms\\DBQueryError' => __DIR__ . '/includes/libs/rdbms/exception/DBQueryError.php', 'Wikimedia\\Rdbms\\DBQueryTimeoutError' => __DIR__ . '/includes/libs/rdbms/exception/DBQueryTimeoutError.php', 'Wikimedia\\Rdbms\\DBReadOnlyError' => __DIR__ . '/includes/libs/rdbms/exception/DBReadOnlyError.php', + 'Wikimedia\\Rdbms\\DBReadOnlyRoleError' => __DIR__ . '/includes/libs/rdbms/exception/DBReadOnlyRoleError.php', 'Wikimedia\\Rdbms\\DBReplicationWaitError' => __DIR__ . '/includes/libs/rdbms/exception/DBReplicationWaitError.php', 'Wikimedia\\Rdbms\\DBTransactionError' => __DIR__ . '/includes/libs/rdbms/exception/DBTransactionError.php', 'Wikimedia\\Rdbms\\DBTransactionSizeError' => __DIR__ . '/includes/libs/rdbms/exception/DBTransactionSizeError.php', diff --git a/includes/libs/rdbms/database/DBConnRef.php b/includes/libs/rdbms/database/DBConnRef.php index cf582b7e99..f3ab1c5842 100644 --- a/includes/libs/rdbms/database/DBConnRef.php +++ b/includes/libs/rdbms/database/DBConnRef.php @@ -8,7 +8,6 @@ use InvalidArgumentException; * Helper class to handle automatically marking connections as reusable (via RAII pattern) * as well handling deferring the actual network connection until the handle is used * - * @note: proxy methods are defined explicitly to avoid interface errors * @ingroup Database * @since 1.22 */ @@ -19,6 +18,8 @@ class DBConnRef implements IDatabase { private $conn; /** @var array|null N-tuple of (server index, group, DatabaseDomain|string) */ private $params; + /** @var int One of DB_MASTER/DB_REPLICA */ + private $role; const FLD_INDEX = 0; const FLD_GROUP = 1; @@ -27,10 +28,13 @@ class DBConnRef implements IDatabase { /** * @param ILoadBalancer $lb Connection manager for $conn - * @param Database|array $conn Database handle or (server index, query groups, domain, flags) + * @param Database|array $conn Database or (server index, query groups, domain, flags) + * @param int $role The type of connection asked for; one of DB_MASTER/DB_REPLICA + * @internal This method should not be called outside of LoadBalancer */ - public function __construct( ILoadBalancer $lb, $conn ) { + public function __construct( ILoadBalancer $lb, $conn, $role ) { $this->lb = $lb; + $this->role = $role; if ( $conn instanceof Database ) { $this->conn = $conn; // live handle } elseif ( is_array( $conn ) && count( $conn ) >= 4 && $conn[self::FLD_DOMAIN] !== false ) { @@ -49,6 +53,14 @@ class DBConnRef implements IDatabase { return $this->conn->$name( ...$arguments ); } + /** + * @return int DB_MASTER when this *requires* the master DB, otherwise DB_REPLICA + * @since 1.33 + */ + public function getReferenceRole() { + return $this->role; + } + public function getServerInfo() { return $this->__call( __FUNCTION__, func_get_args() ); } @@ -255,7 +267,11 @@ class DBConnRef implements IDatabase { } public function query( $sql, $fname = __METHOD__, $flags = 0 ) { - return $this->__call( __FUNCTION__, func_get_args() ); + if ( $this->role !== ILoadBalancer::DB_MASTER ) { + $flags |= IDatabase::QUERY_REPLICA_ROLE; + } + + return $this->__call( __FUNCTION__, [ $sql, $fname, $flags ] ); } public function freeResult( $res ) { @@ -310,6 +326,8 @@ class DBConnRef implements IDatabase { public function lockForUpdate( $table, $conds = '', $fname = __METHOD__, $options = [], $join_conds = [] ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } @@ -326,10 +344,14 @@ class DBConnRef implements IDatabase { } public function insert( $table, $a, $fname = __METHOD__, $options = [] ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } public function update( $table, $values, $conds, $fname = __METHOD__, $options = [] ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } @@ -435,26 +457,36 @@ class DBConnRef implements IDatabase { } public function nextSequenceValue( $seqName ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } public function replace( $table, $uniqueIndexes, $rows, $fname = __METHOD__ ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } public function upsert( $table, array $rows, $uniqueIndexes, array $set, $fname = __METHOD__ ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } public function deleteJoin( $delTable, $joinTable, $delVar, $joinVar, $conds, $fname = __METHOD__ ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } public function delete( $table, $conds, $fname = __METHOD__ ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } @@ -462,6 +494,8 @@ class DBConnRef implements IDatabase { $destTable, $srcTable, $varMap, $conds, $fname = __METHOD__, $insertOptions = [], $selectOptions = [], $selectJoinConds = [] ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } @@ -529,18 +563,21 @@ class DBConnRef implements IDatabase { } public function onTransactionResolution( callable $callback, $fname = __METHOD__ ) { + // DB_REPLICA role: caller might want to refresh cache after a REPEATABLE-READ snapshot return $this->__call( __FUNCTION__, func_get_args() ); } public function onTransactionCommitOrIdle( callable $callback, $fname = __METHOD__ ) { + // DB_REPLICA role: caller might want to refresh cache after a REPEATABLE-READ snapshot return $this->__call( __FUNCTION__, func_get_args() ); } public function onTransactionIdle( callable $callback, $fname = __METHOD__ ) { - return $this->__call( __FUNCTION__, func_get_args() ); + return $this->onTransactionCommitOrIdle( $callback, $fname ); } public function onTransactionPreCommitOrIdle( callable $callback, $fname = __METHOD__ ) { + // DB_REPLICA role: caller might want to refresh cache after a cache mutex is released return $this->__call( __FUNCTION__, func_get_args() ); } @@ -551,20 +588,24 @@ class DBConnRef implements IDatabase { public function startAtomic( $fname = __METHOD__, $cancelable = IDatabase::ATOMIC_NOT_CANCELABLE ) { + // Don't call assertRoleAllowsWrites(); caller might want a REPEATABLE-READ snapshot return $this->__call( __FUNCTION__, func_get_args() ); } public function endAtomic( $fname = __METHOD__ ) { + // Don't call assertRoleAllowsWrites(); caller might want a REPEATABLE-READ snapshot return $this->__call( __FUNCTION__, func_get_args() ); } public function cancelAtomic( $fname = __METHOD__, AtomicSectionIdentifier $sectionId = null ) { + // Don't call assertRoleAllowsWrites(); caller might want a REPEATABLE-READ snapshot return $this->__call( __FUNCTION__, func_get_args() ); } public function doAtomicSection( $fname, callable $callback, $cancelable = self::ATOMIC_NOT_CANCELABLE ) { + // Don't call assertRoleAllowsWrites(); caller might want a REPEATABLE-READ snapshot return $this->__call( __FUNCTION__, func_get_args() ); } @@ -627,18 +668,26 @@ class DBConnRef implements IDatabase { } public function lockIsFree( $lockName, $method ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } public function lock( $lockName, $method, $timeout = 5 ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } public function unlock( $lockName, $method ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } public function getScopedLockAndFlush( $lockKey, $fname, $timeout ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } @@ -674,6 +723,26 @@ class DBConnRef implements IDatabase { return $this->__call( __FUNCTION__, func_get_args() ); } + /** + * Error out if the role is not DB_MASTER + * + * Note that the underlying connection may or may not itself be read-only. + * It could even be to a writable master (both server-side and to the application). + * This error is meant for the case when a DB_REPLICA handle was requested but a + * a write was attempted on that handle regardless. + * + * In configurations where the master DB has some generic read load or is the only server, + * DB_MASTER/DB_REPLICA will sometimes (or always) use the same connection to the master DB. + * This does not effect the role of DBConnRef instances. + * @throws DBReadOnlyRoleError + */ + protected function assertRoleAllowsWrites() { + // DB_MASTER is "prima facie" writable + if ( $this->role !== ILoadBalancer::DB_MASTER ) { + throw new DBReadOnlyRoleError( $this->conn, "Cannot write with role DB_REPLICA" ); + } + } + /** * Clean up the connection when out of scope */ diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 27392050ce..b4a61d57e7 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -1032,7 +1032,7 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware */ protected function assertIsWritableMaster() { if ( $this->getLBInfo( 'replica' ) === true ) { - throw new DBUnexpectedError( + throw new DBReadOnlyRoleError( $this, 'Write operations are not allowed on replica database connections.' ); @@ -1194,7 +1194,6 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $flags = (int)$flags; // b/c; this field used to be a bool $ignoreErrors = $this->hasFlags( $flags, self::QUERY_SILENCE_ERRORS ); - $pseudoPermanent = $this->hasFlags( $flags, self::QUERY_PSEUDO_PERMANENT ); $priorTransaction = $this->trxLevel; $priorWritesPending = $this->writesOrCallbacksPending(); @@ -1206,8 +1205,13 @@ abstract class Database implements IDatabase, IMaintainableDatabase, LoggerAware $this->assertIsWritableMaster(); # Do not treat temporary table writes as "meaningful writes" that need committing. # Profile them as reads. Integration tests can override this behavior via $flags. + $pseudoPermanent = $this->hasFlags( $flags, self::QUERY_PSEUDO_PERMANENT ); $tableType = $this->registerTempTableWrite( $sql, $pseudoPermanent ); $isEffectiveWrite = ( $tableType !== self::TEMP_NORMAL ); + # DBConnRef uses QUERY_REPLICA_ROLE to enforce the replica role for raw SQL queries + if ( $isEffectiveWrite && $this->hasFlags( $flags, self::QUERY_REPLICA_ROLE ) ) { + throw new DBReadOnlyRoleError( $this, "Cannot write; target role is DB_REPLICA" ); + } } else { $isEffectiveWrite = false; } diff --git a/includes/libs/rdbms/database/IDatabase.php b/includes/libs/rdbms/database/IDatabase.php index b4440d6dfa..d858dd41e7 100644 --- a/includes/libs/rdbms/database/IDatabase.php +++ b/includes/libs/rdbms/database/IDatabase.php @@ -113,6 +113,8 @@ interface IDatabase { * permanent as far as write tracking is concerned. This is useful for testing. */ const QUERY_PSEUDO_PERMANENT = 2; + /** @var int Enforce that a query does not make effective writes */ + const QUERY_REPLICA_ROLE = 4; /** @var bool Parameter to unionQueries() for UNION ALL */ const UNION_ALL = true; diff --git a/includes/libs/rdbms/database/MaintainableDBConnRef.php b/includes/libs/rdbms/database/MaintainableDBConnRef.php index 8a2c795933..10a08978f8 100644 --- a/includes/libs/rdbms/database/MaintainableDBConnRef.php +++ b/includes/libs/rdbms/database/MaintainableDBConnRef.php @@ -30,6 +30,8 @@ class MaintainableDBConnRef extends DBConnRef implements IMaintainableDatabase { $fname = false, callable $inputCallback = null ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } @@ -40,14 +42,20 @@ class MaintainableDBConnRef extends DBConnRef implements IMaintainableDatabase { $fname = __METHOD__, callable $inputCallback = null ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } public function dropTable( $tableName, $fName = __METHOD__ ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } public function deadlockLoop() { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } @@ -66,6 +74,8 @@ class MaintainableDBConnRef extends DBConnRef implements IMaintainableDatabase { public function duplicateTableStructure( $oldName, $newName, $temporary = false, $fname = __METHOD__ ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } @@ -74,10 +84,14 @@ class MaintainableDBConnRef extends DBConnRef implements IMaintainableDatabase { } public function lockTables( array $read, array $write, $method ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } public function unlockTables( $method ) { + $this->assertRoleAllowsWrites(); + return $this->__call( __FUNCTION__, func_get_args() ); } diff --git a/includes/libs/rdbms/exception/DBReadOnlyRoleError.php b/includes/libs/rdbms/exception/DBReadOnlyRoleError.php new file mode 100644 index 0000000000..4d565ba11a --- /dev/null +++ b/includes/libs/rdbms/exception/DBReadOnlyRoleError.php @@ -0,0 +1,30 @@ +resolveDomainID( $domain ); + $role = $this->getRoleFromIndex( $i ); - return new DBConnRef( $this, $this->getConnection( $db, $groups, $domain, $flags ) ); + return new DBConnRef( $this, $this->getConnection( $i, $groups, $domain, $flags ), $role ); } - public function getLazyConnectionRef( $db, $groups = [], $domain = false, $flags = 0 ) { + public function getLazyConnectionRef( $i, $groups = [], $domain = false, $flags = 0 ) { $domain = $this->resolveDomainID( $domain ); + $role = $this->getRoleFromIndex( $i ); - return new DBConnRef( $this, [ $db, $groups, $domain, $flags ] ); + return new DBConnRef( $this, [ $i, $groups, $domain, $flags ], $role ); } - public function getMaintenanceConnectionRef( $db, $groups = [], $domain = false, $flags = 0 ) { + public function getMaintenanceConnectionRef( $i, $groups = [], $domain = false, $flags = 0 ) { $domain = $this->resolveDomainID( $domain ); + $role = $this->getRoleFromIndex( $i ); return new MaintainableDBConnRef( - $this, $this->getConnection( $db, $groups, $domain, $flags ) ); + $this, $this->getConnection( $i, $groups, $domain, $flags ), $role ); + } + + /** + * @param int $i Server index or DB_MASTER/DB_REPLICA + * @return int One of DB_MASTER/DB_REPLICA + */ + private function getRoleFromIndex( $i ) { + return ( $i === self::DB_MASTER || $i === $this->getWriterIndex() ) + ? self::DB_MASTER + : self::DB_REPLICA; } public function openConnection( $i, $domain = false, $flags = 0 ) { diff --git a/tests/phpunit/includes/db/LoadBalancerTest.php b/tests/phpunit/includes/db/LoadBalancerTest.php index 2c4e6b421b..2f4e68af32 100644 --- a/tests/phpunit/includes/db/LoadBalancerTest.php +++ b/tests/phpunit/includes/db/LoadBalancerTest.php @@ -32,7 +32,7 @@ use Wikimedia\Rdbms\LoadMonitorNull; * @covers \Wikimedia\Rdbms\LoadBalancer */ class LoadBalancerTest extends MediaWikiTestCase { - private function makeServerConfig() { + private function makeServerConfig( $flags = DBO_DEFAULT ) { global $wgDBserver, $wgDBname, $wgDBuser, $wgDBpassword, $wgDBtype, $wgSQLiteDataDir; return [ @@ -44,7 +44,7 @@ class LoadBalancerTest extends MediaWikiTestCase { 'type' => $wgDBtype, 'dbDirectory' => $wgSQLiteDataDir, 'load' => 0, - 'flags' => DBO_TRX // REPEATABLE-READ for consistency + 'flags' => $flags ]; } @@ -57,7 +57,8 @@ class LoadBalancerTest extends MediaWikiTestCase { $called = false; $lb = new LoadBalancer( [ - 'servers' => [ $this->makeServerConfig() ], + // Simulate web request with DBO_TRX + 'servers' => [ $this->makeServerConfig( DBO_TRX ) ], 'queryLogger' => MediaWiki\Logger\LoggerFactory::getInstance( 'DBQuery' ), 'localDomain' => new DatabaseDomain( $wgDBname, null, $this->dbPrefix() ), 'chronologyCallback' => function () use ( &$called ) { @@ -71,8 +72,8 @@ class LoadBalancerTest extends MediaWikiTestCase { $this->assertSame( 'my_test_wiki', $lb->resolveDomainID( 'my_test_wiki' ) ); $this->assertSame( $ld->getId(), $lb->resolveDomainID( false ) ); $this->assertSame( $ld->getId(), $lb->resolveDomainID( $ld ) ); - $this->assertFalse( $called ); + $dbw = $lb->getConnection( DB_MASTER ); $this->assertTrue( $called ); $this->assertTrue( $dbw->getLBInfo( 'master' ), 'master shows as master' ); @@ -106,39 +107,10 @@ class LoadBalancerTest extends MediaWikiTestCase { } public function testWithReplica() { - global $wgDBserver, $wgDBname, $wgDBuser, $wgDBpassword, $wgDBtype, $wgSQLiteDataDir; - - $servers = [ - [ // master - 'host' => $wgDBserver, - 'dbname' => $wgDBname, - 'tablePrefix' => $this->dbPrefix(), - 'user' => $wgDBuser, - 'password' => $wgDBpassword, - 'type' => $wgDBtype, - 'dbDirectory' => $wgSQLiteDataDir, - 'load' => 0, - 'flags' => DBO_TRX // REPEATABLE-READ for consistency - ], - [ // emulated replica - 'host' => $wgDBserver, - 'dbname' => $wgDBname, - 'tablePrefix' => $this->dbPrefix(), - 'user' => $wgDBuser, - 'password' => $wgDBpassword, - 'type' => $wgDBtype, - 'dbDirectory' => $wgSQLiteDataDir, - 'load' => 100, - 'flags' => DBO_TRX // REPEATABLE-READ for consistency - ] - ]; + global $wgDBserver; - $lb = new LoadBalancer( [ - 'servers' => $servers, - 'localDomain' => new DatabaseDomain( $wgDBname, null, $this->dbPrefix() ), - 'queryLogger' => MediaWiki\Logger\LoggerFactory::getInstance( 'DBQuery' ), - 'loadMonitorClass' => LoadMonitorNull::class - ] ); + // Simulate web request with DBO_TRX + $lb = $this->newMultiServerLocalLoadBalancer( DBO_TRX ); $dbw = $lb->getConnection( DB_MASTER ); $this->assertTrue( $dbw->getLBInfo( 'master' ), 'master shows as master' ); @@ -180,6 +152,51 @@ class LoadBalancerTest extends MediaWikiTestCase { $lb->closeAll(); } + private function newSingleServerLocalLoadBalancer() { + global $wgDBname; + + return new LoadBalancer( [ + 'servers' => [ $this->makeServerConfig() ], + 'localDomain' => new DatabaseDomain( $wgDBname, null, $this->dbPrefix() ) + ] ); + } + + private function newMultiServerLocalLoadBalancer( $flags = DBO_DEFAULT ) { + global $wgDBserver, $wgDBname, $wgDBuser, $wgDBpassword, $wgDBtype, $wgSQLiteDataDir; + + $servers = [ + [ // master + 'host' => $wgDBserver, + 'dbname' => $wgDBname, + 'tablePrefix' => $this->dbPrefix(), + 'user' => $wgDBuser, + 'password' => $wgDBpassword, + 'type' => $wgDBtype, + 'dbDirectory' => $wgSQLiteDataDir, + 'load' => 0, + 'flags' => $flags + ], + [ // emulated replica + 'host' => $wgDBserver, + 'dbname' => $wgDBname, + 'tablePrefix' => $this->dbPrefix(), + 'user' => $wgDBuser, + 'password' => $wgDBpassword, + 'type' => $wgDBtype, + 'dbDirectory' => $wgSQLiteDataDir, + 'load' => 100, + 'flags' => $flags + ] + ]; + + return new LoadBalancer( [ + 'servers' => $servers, + 'localDomain' => new DatabaseDomain( $wgDBname, null, $this->dbPrefix() ), + 'queryLogger' => MediaWiki\Logger\LoggerFactory::getInstance( 'DBQuery' ), + 'loadMonitorClass' => LoadMonitorNull::class + ] ); + } + private function assertWriteForbidden( Database $db ) { try { $db->delete( 'some_table', [ 'id' => 57634126 ], __METHOD__ ); @@ -295,11 +312,16 @@ class LoadBalancerTest extends MediaWikiTestCase { $i = $lb->getWriterIndex(); $this->assertEquals( null, $lb->getAnyOpenConnection( $i ) ); + $conn1 = $lb->getConnection( $i ); $this->assertNotEquals( null, $conn1 ); $this->assertEquals( $conn1, $lb->getAnyOpenConnection( $i ) ); + $this->assertFalse( $conn1->getFlag( DBO_TRX ) ); + $conn2 = $lb->getConnection( $i, [], false, $lb::CONN_TRX_AUTOCOMMIT ); $this->assertNotEquals( null, $conn2 ); + $this->assertFalse( $conn2->getFlag( DBO_TRX ) ); + if ( $lb->getServerAttributes( $i )[Database::ATTR_DB_LEVEL_LOCKING] ) { $this->assertEquals( null, $lb->getAnyOpenConnection( $i, $lb::CONN_TRX_AUTOCOMMIT ) ); @@ -343,7 +365,7 @@ class LoadBalancerTest extends MediaWikiTestCase { 'type' => $wgDBtype, 'dbDirectory' => $wgSQLiteDataDir, 'load' => 0, - 'flags' => DBO_TRX // REPEATABLE-READ for consistency + 'flags' => DBO_TRX // simulate a web request with DBO_TRX ], ]; @@ -416,4 +438,60 @@ class LoadBalancerTest extends MediaWikiTestCase { $conn1->close(); $conn2->close(); } + + public function testDBConnRefReadsMasterAndReplicaRoles() { + $lb = $this->newSingleServerLocalLoadBalancer(); + + $rConn = $lb->getConnectionRef( DB_REPLICA ); + $wConn = $lb->getConnectionRef( DB_MASTER ); + $wConn2 = $lb->getConnectionRef( 0 ); + + $v = [ 'value' => '1', '1' ]; + $sql = 'SELECT MAX(1) AS value'; + foreach ( [ $rConn, $wConn, $wConn2 ] as $conn ) { + $conn->clearFlag( $conn::DBO_TRX ); + + $res = $conn->query( $sql, __METHOD__ ); + $this->assertEquals( $v, $conn->fetchRow( $res ) ); + + $res = $conn->query( $sql, __METHOD__, $conn::QUERY_REPLICA_ROLE ); + $this->assertEquals( $v, $conn->fetchRow( $res ) ); + } + + $wConn->getScopedLockAndFlush( 'key', __METHOD__, 1 ); + $wConn2->getScopedLockAndFlush( 'key2', __METHOD__, 1 ); + } + + /** + * @expectedException \Wikimedia\Rdbms\DBReadOnlyRoleError + */ + public function testDBConnRefWritesReplicaRole() { + $lb = $this->newSingleServerLocalLoadBalancer(); + + $rConn = $lb->getConnectionRef( DB_REPLICA ); + + $rConn->query( 'DELETE FROM sometesttable WHERE 1=0' ); + } + + /** + * @expectedException \Wikimedia\Rdbms\DBReadOnlyRoleError + */ + public function testDBConnRefWritesReplicaRoleIndex() { + $lb = $this->newMultiServerLocalLoadBalancer(); + + $rConn = $lb->getConnectionRef( 1 ); + + $rConn->query( 'DELETE FROM sometesttable WHERE 1=0' ); + } + + /** + * @expectedException \Wikimedia\Rdbms\DBReadOnlyRoleError + */ + public function testDBConnRefWritesReplicaRoleInsert() { + $lb = $this->newMultiServerLocalLoadBalancer(); + + $rConn = $lb->getConnectionRef( DB_REPLICA ); + + $rConn->insert( 'test', [ 't' => 1 ], __METHOD__ ); + } } diff --git a/tests/phpunit/includes/libs/rdbms/database/DBConnRefTest.php b/tests/phpunit/includes/libs/rdbms/database/DBConnRefTest.php index 9b72b95dc3..33e5c3b3fb 100644 --- a/tests/phpunit/includes/libs/rdbms/database/DBConnRefTest.php +++ b/tests/phpunit/includes/libs/rdbms/database/DBConnRefTest.php @@ -75,12 +75,12 @@ class DBConnRefTest extends PHPUnit\Framework\TestCase { */ private function getDBConnRef( ILoadBalancer $lb = null ) { $lb = $lb ?: $this->getLoadBalancerMock(); - return new DBConnRef( $lb, $this->getDatabaseMock() ); + return new DBConnRef( $lb, $this->getDatabaseMock(), DB_MASTER ); } public function testConstruct() { $lb = $this->getLoadBalancerMock(); - $ref = new DBConnRef( $lb, $this->getDatabaseMock() ); + $ref = new DBConnRef( $lb, $this->getDatabaseMock(), DB_MASTER ); $this->assertInstanceOf( ResultWrapper::class, $ref->select( 'whatever', '*' ) ); } @@ -99,10 +99,19 @@ class DBConnRefTest extends PHPUnit\Framework\TestCase { $ref = new DBConnRef( $lb, - [ DB_MASTER, [ 'test' ], 'dummy', ILoadBalancer::CONN_TRX_AUTOCOMMIT ] + [ DB_MASTER, [ 'test' ], 'dummy', ILoadBalancer::CONN_TRX_AUTOCOMMIT ], + DB_MASTER ); $this->assertInstanceOf( ResultWrapper::class, $ref->select( 'whatever', '*' ) ); + $this->assertEquals( DB_MASTER, $ref->getReferenceRole() ); + + $ref2 = new DBConnRef( + $lb, + [ DB_MASTER, [ 'test' ], 'dummy', ILoadBalancer::CONN_TRX_AUTOCOMMIT ], + DB_REPLICA + ); + $this->assertEquals( DB_REPLICA, $ref2->getReferenceRole() ); } public function testDestruct() { @@ -124,7 +133,7 @@ class DBConnRefTest extends PHPUnit\Framework\TestCase { $this->setExpectedException( InvalidArgumentException::class, '' ); $lb = $this->getLoadBalancerMock(); - new DBConnRef( $lb, 17 ); // bad constructor argument + new DBConnRef( $lb, 17, DB_REPLICA ); // bad constructor argument } /** @@ -137,7 +146,7 @@ class DBConnRefTest extends PHPUnit\Framework\TestCase { $lb->expects( $this->never() ) ->method( 'getConnection' ); - $ref = new DBConnRef( $lb, [ DB_REPLICA, [], 'dummy', 0 ] ); + $ref = new DBConnRef( $lb, [ DB_REPLICA, [], 'dummy', 0 ], DB_REPLICA ); $this->assertSame( 'dummy', $ref->getDomainID() ); } @@ -156,7 +165,7 @@ class DBConnRefTest extends PHPUnit\Framework\TestCase { $this->assertInternalType( 'string', $ref->__toString() ); $lb = $this->getLoadBalancerMock(); - $ref = new DBConnRef( $lb, [ DB_MASTER, [], 'test', 0 ] ); + $ref = new DBConnRef( $lb, [ DB_MASTER, [], 'test', 0 ], DB_MASTER ); $this->assertInternalType( 'string', $ref->__toString() ); } @@ -166,7 +175,49 @@ class DBConnRefTest extends PHPUnit\Framework\TestCase { */ public function testClose() { $lb = $this->getLoadBalancerMock(); - $ref = new DBConnRef( $lb, [ DB_REPLICA, [], 'dummy', 0 ] ); + $ref = new DBConnRef( $lb, [ DB_REPLICA, [], 'dummy', 0 ], DB_MASTER ); $ref->close(); } + + /** + * @covers Wikimedia\Rdbms\DBConnRef::getReferenceRole + */ + public function testGetReferenceRole() { + $lb = $this->getLoadBalancerMock(); + $ref = new DBConnRef( $lb, [ DB_REPLICA, [], 'dummy', 0 ], DB_REPLICA ); + $this->assertSame( DB_REPLICA, $ref->getReferenceRole() ); + + $ref = new DBConnRef( $lb, [ DB_MASTER, [], 'dummy', 0 ], DB_MASTER ); + $this->assertSame( DB_MASTER, $ref->getReferenceRole() ); + + $ref = new DBConnRef( $lb, [ 1, [], 'dummy', 0 ], DB_REPLICA ); + $this->assertSame( DB_REPLICA, $ref->getReferenceRole() ); + + $ref = new DBConnRef( $lb, [ 0, [], 'dummy', 0 ], DB_MASTER ); + $this->assertSame( DB_MASTER, $ref->getReferenceRole() ); + } + + /** + * @covers Wikimedia\Rdbms\DBConnRef::getReferenceRole + * @expectedException Wikimedia\Rdbms\DBReadOnlyRoleError + * @dataProvider provideRoleExceptions + */ + public function testRoleExceptions( $method, $args ) { + $lb = $this->getLoadBalancerMock(); + $ref = new DBConnRef( $lb, [ DB_REPLICA, [], 'dummy', 0 ], DB_REPLICA ); + $ref->$method( ...$args ); + } + + function provideRoleExceptions() { + return [ + [ 'insert', [ 'table', [ 'a' => 1 ] ] ], + [ 'update', [ 'table', [ 'a' => 1 ], [ 'a' => 2 ] ] ], + [ 'delete', [ 'table', [ 'a' => 1 ] ] ], + [ 'replace', [ 'table', [ 'a' ], [ 'a' => 1 ] ] ], + [ 'upsert', [ 'table', [ 'a' => 1 ], [ 'a' ], [ 'a = a + 1' ] ] ], + [ 'lock', [ 'k', 'method' ] ], + [ 'unlock', [ 'k', 'method' ] ], + [ 'getScopedLockAndFlush', [ 'k', 'method', 1 ] ] + ]; + } } -- 2.20.1