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

Merge "ActorNormalization should require a DB connection."

Browse source
This commit is contained in:
jenkins-bot 2021-03-10 22:05:17 +00:00 committed by Gerrit Code Review
commit 600f75010c
13 changed files with 142 additions and 60 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
includes/ActorMigration.php
View file

@ -265,7 +265,7 @@ class ActorMigration {
// foreign.
return $this->actorStoreFactory
->getActorNormalization( $db->getDomainID() )
->findActorId( $user );
->findActorId( $user, $db );
}
/**

5
includes/Revision/RevisionStore.php
View file

@ -2203,7 +2203,10 @@ class RevisionStore
}
if ( !$user && $actorID ) {
try {
$user = $this->actorStore->getActorById( $actorID );
$user = $this->actorStore->getActorById(
$actorID,
$this->getDBConnectionRefForQueryFlags( self::READ_NORMAL )
);
} catch ( InvalidArgumentException $ex ) {
$user = null;
}

2
includes/filerepo/file/LocalFile.php
View file

@ -1538,7 +1538,7 @@ class LocalFile extends File {
$dbw->startAtomic( __METHOD__ );
$actorId = $actorNormalizaton->acquireActorId( $user );
$actorId = $actorNormalizaton->acquireActorId( $user, $dbw );
$this->user = $user;
# Test to see if the row exists using INSERT IGNORE

24
includes/user/ActorNormalization.php
View file

@ -21,7 +21,6 @@
namespace MediaWiki\User;
use CannotCreateActorException;
use IDBAccessObject;
use InvalidArgumentException;
use stdClass;
use Wikimedia\Rdbms\IDatabase;
@ -30,7 +29,7 @@ use Wikimedia\Rdbms\IDatabase;
* Service for dealing with the actor table.
* @since 1.36
*/
interface ActorNormalization extends IDBAccessObject {
interface ActorNormalization {
/**
* Instantiate a new UserIdentity object based on a $row from the actor table.
@ -70,26 +69,32 @@ interface ActorNormalization extends IDBAccessObject {
* Find the actor_id for the given name.
*
* @param string $name
* @param int $queryFlags
* @param IDatabase $db The database connection to operate on.
* The database must correspond to the wiki this ActorNormalization is bound to.
* @return int|null
*/
public function findActorIdByName( string $name, int $queryFlags = self::READ_NORMAL ): ?int;
public function findActorIdByName( string $name, IDatabase $db ): ?int;
/**
* Find the actor_id of the given $user.
*
* @param UserIdentity $user
* @param int $queryFlags
* @param IDatabase $db The database connection to operate on.
* The database must correspond to the wiki this ActorNormalization is bound to.
* @return int|null
*/
public function findActorId( UserIdentity $user, int $queryFlags = self::READ_NORMAL ): ?int;
public function findActorId( UserIdentity $user, IDatabase $db ): ?int;
/**
* Attempt to assign an actor ID to the given $user
* If it is already assigned, return the existing ID.
*
* @param UserIdentity $user
* @param IDatabase|null $dbw
* @param IDatabase|null $dbw The database connection to acquire the ID from.
* The database must correspond to the wiki this ActorNormalization is bound to.
* If not given, a connection for this ActorNormalization's wiki will be created.
* Not providing a database connection triggers a deprecation warning!
* In the future, this parameter will be required.
* @return int greater then 0
* @throws CannotCreateActorException if no actor ID has been assigned to this $user
*/
@ -99,10 +104,11 @@ interface ActorNormalization extends IDBAccessObject {
* Find an actor by $id.
*
* @param int $actorId
* @param int $queryFlags one of IDBAccessObject constants
* @param IDatabase $db The database connection to operate on.
* The database must correspond to the wiki this ActorNormalization is bound to.
* @return UserIdentity|null Returns null if no actor with this $actorId exists in the database.
*/
public function getActorById( int $actorId, int $queryFlags = self::READ_NORMAL ): ?UserIdentity;
public function getActorById( int $actorId, IDatabase $db ): ?UserIdentity;
/**
* In case all reasonable attempts of initializing a proper actor from the

70
includes/user/ActorStore.php
View file

@ -207,10 +207,11 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
* Find an actor by $id.
*
* @param int $actorId
* @param int $queryFlags one of IDBAccessObject constants
* @param IDatabase $db The database connection to operate on.
* The database must correspond to ActorStore's wiki ID.
* @return UserIdentity|null Returns null if no actor with this $actorId exists in the database.
*/
public function getActorById( int $actorId, int $queryFlags = self::READ_NORMAL ): ?UserIdentity {
public function getActorById( int $actorId, IDatabase $db ): ?UserIdentity {
if ( !$actorId ) {
return null;
}
@ -220,7 +221,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
return $cachedValue[0];
}
return $this->newSelectQueryBuilder( $queryFlags )
return $this->newSelectQueryBuilder( $db )
->caller( __METHOD__ )
->conds( [ 'actor_id' => $actorId ] )
->fetchUserIdentity();
@ -249,7 +250,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
return $cachedValue[0];
}
return $this->newSelectQueryBuilder( $queryFlags )
return $this->newSelectQueryBuilderForQueryFlags( $queryFlags )
->caller( __METHOD__ )
->userNames( $name )
->fetchUserIdentity();
@ -272,7 +273,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
return $cachedValue[0];
}
return $this->newSelectQueryBuilder( $queryFlags )
return $this->newSelectQueryBuilderForQueryFlags( $queryFlags )
->caller( __METHOD__ )
->userIds( $userId )
->fetchUserIdentity();
@ -298,10 +299,11 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
* Find the actor_id of the given $user.
*
* @param UserIdentity $user
* @param int $queryFlags
* @param IDatabase $db The database connection to operate on.
* The database must correspond to ActorStore's wiki ID.
* @return int|null
*/
public function findActorId( UserIdentity $user, int $queryFlags = self::READ_NORMAL ): ?int {
public function findActorId( UserIdentity $user, IDatabase $db ): ?int {
// TODO: we want to assert this user belongs to the correct wiki,
// but User objects are always local and we used to use them
// on a non-local DB connection. We need to first deprecate this
@ -318,8 +320,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
return null;
}
[ $db, $options ] = $this->getDBConnectionRefForQueryFlags( $queryFlags );
$id = $this->findActorIdInternal( $name, $db, $options );
$id = $this->findActorIdInternal( $name, $db );
if ( $id ) {
$this->attachActorId( $user, $id );
@ -332,18 +333,18 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
* Find the actor_id of the given $name.
*
* @param string $name
* @param int $queryFlags
* @param IDatabase $db The database connection to operate on.
* The database must correspond to ActorStore's wiki ID.
* @return int|null
*/
public function findActorIdByName( $name, int $queryFlags = self::READ_NORMAL ): ?int {
public function findActorIdByName( $name, IDatabase $db ): ?int {
// NOTE: $name may be user-supplied, need full normalization
$name = $this->normalizeUserName( $name, UserNameUtils::RIGOR_VALID );
if ( !$name ) {
return null;
}
[ $db, $options ] = $this->getDBConnectionRefForQueryFlags( $queryFlags );
$id = $this->findActorIdInternal( $name, $db, $options );
$id = $this->findActorIdInternal( $name, $db );
return $id;
}
@ -352,7 +353,8 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
* Find actor_id of the given $user using the passed $db connection.
*
* @param string $name
* @param IDatabase $db
* @param IDatabase $db The database connection to operate on.
* The database must correspond to ActorStore's wiki ID.
* @param array $queryOptions
* @return int|null
*/
@ -396,7 +398,12 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
* If it is already assigned, return the existing ID.
*
* @param UserIdentity $user
* @param IDatabase|null $dbw
* @param IDatabase|null $dbw The database connection to acquire the ID from.
* The database must correspond to ActorStore's wiki ID.
* If not given, an appropriate database connection will acquired from the
* LoadBalancer provided to the constructor.
* Not providing a database connection triggers a deprecation warning!
* In the future, this parameter will be required.
* @return int greater then 0
* @throws CannotCreateActorException if no actor ID has been assigned to this $user
*/
@ -404,6 +411,12 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
if ( $dbw ) {
$this->checkDatabaseDomain( $dbw );
} else {
// TODO: Remove after fixing it in all extensions and seeing it live for one train.
// Does not need full deprecation since this method is new in 1.36.
wfDeprecatedMsg(
'Calling acquireActorId() without the $dbw parameter is deprecated',
'1.36'
);
[ $dbw, ] = $this->getDBConnectionRefForQueryFlags( self::READ_LATEST );
}
// TODO: we want to assert this user belongs to the correct wiki,
@ -530,7 +543,9 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
return $actor;
}
$actor = new UserIdentityValue( 0, self::UNKNOWN_USER_NAME, 0, $this->wikiId );
$this->acquireActorId( $actor );
[ $db, ] = $this->getDBConnectionRefForQueryFlags( self::READ_LATEST );
$this->acquireActorId( $actor, $db );
return $actor;
}
@ -540,8 +555,27 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
* @param int $queryFlags one of IDBAccessObject constants
* @return UserSelectQueryBuilder
*/
public function newSelectQueryBuilder( int $queryFlags = self::READ_NORMAL ): UserSelectQueryBuilder {
private function newSelectQueryBuilderForQueryFlags( $queryFlags ): UserSelectQueryBuilder {
[ $db, $options ] = $this->getDBConnectionRefForQueryFlags( $queryFlags );
return ( new UserSelectQueryBuilder( $db, $this ) )->options( $options );
$queryBuilder = $this->newSelectQueryBuilder( $db );
$queryBuilder->options( $options );
return $queryBuilder;
}
/**
* Returns a specialized SelectQueryBuilder for querying the UserIdentity objects.
*
* @param IDatabase|null $db The database connection to perform the query on.
* The database must correspond to ActorStore's wiki ID.
* If not given, an appropriate database connection will acquired from the
* LoadBalancer provided to the constructor.
* @return UserSelectQueryBuilder
*/
public function newSelectQueryBuilder( IDatabase $db = null ): UserSelectQueryBuilder {
if ( !$db ) {
[ $db, ] = $this->getDBConnectionRefForQueryFlags( self::READ_NORMAL );
}
return new UserSelectQueryBuilder( $db, $this );
}
}

6
includes/user/UserIdentityLookup.php
View file

@ -22,6 +22,7 @@ namespace MediaWiki\User;
use IDBAccessObject;
use InvalidArgumentException;
use Wikimedia\Rdbms\IDatabase;
/**
* Service for looking up UserIdentity
@ -59,8 +60,9 @@ interface UserIdentityLookup extends IDBAccessObject {
/**
* Returns a specialized SelectQueryBuilder for querying the UserIdentity objects.
*
* @param int $queryFlags one of IDBAccessObject constants
* @param IDatabase|null $db The database connection to perform the query on.
* If not given, a connection to the default database will be used.
* @return UserSelectQueryBuilder
*/
public function newSelectQueryBuilder( int $queryFlags = self::READ_NORMAL ): UserSelectQueryBuilder;
public function newSelectQueryBuilder( IDatabase $db = null ): UserSelectQueryBuilder;
}

3
maintenance/findMissingActors.php
View file

@ -142,7 +142,8 @@ class FindMissingActors extends Maintenance {
$this->fatalError( "Unknown user: '$name'" );
}
$actorId = $this->actorNormalization->acquireActorId( $user );
$dbw = $this->loadBalancer->getConnectionRef( DB_MASTER );
$actorId = $this->actorNormalization->acquireActorId( $user, $dbw );
if ( !$actorId ) {
$this->fatalError( "Failed to acquire an actor ID for user '$user'" );

3
maintenance/reassignEdits.php
View file

@ -130,7 +130,8 @@ class ReassignEdits extends Maintenance {
$total = $cur + $del + $rec;
$this->output( "\nTotal entries to change: {$total}\n" );
$toActorId = MediaWikiServices::getInstance()->getActorNormalization()->acquireActorId( $to );
$toActorId =
MediaWikiServices::getInstance()->getActorNormalization()->acquireActorId( $to, $dbw );
if ( !$report ) {
if ( $total ) {
# Reassign edits

3
tests/phpunit/includes/ActorMigrationTest.php
View file

@ -514,7 +514,8 @@ class ActorMigrationTest extends MediaWikiLangTestCase {
*/
public function testInsertRoundTrip( $table, $key, $pk, $usesTemp ) {
$u = $this->getTestUser()->getUser();
$actorId = $this->getServiceContainer()->getActorNormalization()->acquireActorId( $u );
$actorId =
$this->getServiceContainer()->getActorNormalization()->acquireActorId( $u, $this->db );
$user = new UserIdentityValue( $u->getId(), $u->getName(), $actorId );
$stageNames = [

2
tests/phpunit/includes/page/PageArchiveTest.php
View file

@ -170,7 +170,7 @@ class PageArchiveTest extends MediaWikiIntegrationTestCase {
'ar_user' => null,
'ar_user_text' => $this->ipEditor,
'ar_actor' => (string)$this->getServiceContainer()->getActorNormalization()
->acquireActorId( new UserIdentityValue( 0, $this->ipEditor, 0 ) ),
->acquireActorId( new UserIdentityValue( 0, $this->ipEditor, 0 ), $this->db ),
'ar_len' => '11',
'ar_deleted' => '0',
'ar_rev_id' => strval( $this->ipRev->getId() ),

8
tests/phpunit/includes/specialpage/ChangesListSpecialPageTest.php
View file

@ -262,7 +262,7 @@ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase
public function testRcHidemyselfFilter() {
$actorNormalization = $this->getServiceContainer()->getActorNormalization();
$user = $this->getTestUser()->getUser();
$actorId = $actorNormalization->acquireActorId( $user );
$actorId = $actorNormalization->acquireActorId( $user, $this->db );
$this->assertConditions(
[ # expected
"NOT((rc_actor = {$actorId}))",
@ -275,7 +275,7 @@ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase
);
$user = User::newFromName( '10.11.12.13', false );
$actorId = $actorNormalization->acquireActorId( $user );
$actorId = $actorNormalization->acquireActorId( $user, $this->db );
$this->assertConditions(
[ # expected
"NOT((rc_actor = {$actorId}))",
@ -291,7 +291,7 @@ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase
public function testRcHidebyothersFilter() {
$actorNormalization = $this->getServiceContainer()->getActorNormalization();
$user = $this->getTestUser()->getUser();
$actorId = $actorNormalization->acquireActorId( $user );
$actorId = $actorNormalization->acquireActorId( $user, $this->db );
$this->assertConditions(
[ # expected
"(rc_actor = {$actorId})",
@ -304,7 +304,7 @@ class ChangesListSpecialPageTest extends AbstractChangesListSpecialPageTestCase
);
$user = User::newFromName( '10.11.12.13', false );
$actorId = $actorNormalization->acquireActorId( $user );
$actorId = $actorNormalization->acquireActorId( $user, $this->db );
$this->assertConditions(
[ # expected
"(rc_actor = {$actorId})",

2
tests/phpunit/includes/user/UserTest.php
View file

@ -1120,7 +1120,7 @@ class UserTest extends MediaWikiIntegrationTestCase {
// Anon user. Can't load by only user ID when that's 0.
$user = User::newFromName( '192.168.12.34', false );
// Make sure an actor ID exists
MediaWikiServices::getInstance()->getActorNormalization()->acquireActorId( $user );
MediaWikiServices::getInstance()->getActorNormalization()->acquireActorId( $user, $this->db );
$test = User::newFromAnyId( null, '192.168.12.34', null );
$this->assertSame( $user->getId(), $test->getId() );

72
tests/phpunit/integration/includes/user/ActorStoreTest.php
View file

@ -11,6 +11,7 @@ use MediaWiki\User\ActorStore;
use MediaWiki\User\UserIdentity;
use MediaWiki\User\UserIdentityValue;
use MediaWiki\User\UserNameUtils;
use MediaWiki\User\UserSelectQueryBuilder;
use stdClass;
use Wikimedia\Assert\PreconditionException;
@ -21,27 +22,46 @@ use Wikimedia\Assert\PreconditionException;
*/
class ActorStoreTest extends ActorStoreTestBase {
public function provideGetActorByMethods() {
public function provideGetActorById() {
yield 'getActorById, registered' => [
'getActorById', // $method
42, // $argument
new UserIdentityValue( 24, 'TestUser', 42 ), // $expected
];
yield 'getActorById, anon' => [
'getActorById', // $method
43, // $argument
new UserIdentityValue( 0, self::IP, 43 ), // $expected
];
yield 'getActorById, non-existent' => [
'getActorById', // $method
4321231, // $argument
null, // $expected
];
yield 'getActorById, zero' => [
'getActorById', // $method
0, // $argument
null, // $expected
];
}
/**
* @dataProvider provideGetActorById
* @covers ::getActorById
* @covers ::getUserIdentityByName
* @covers ::getUserIdentityByUserId
*/
public function testGetActorById( $argument, ?UserIdentity $expected ) {
$actor = $this->getStore()->getActorById( $argument, $this->db );
if ( $expected ) {
$this->assertNotNull( $actor );
$this->assertSameActors( $expected, $actor );
// test caching
$cachedActor = $this->getStore()->getActorById( $argument, $this->db );
$this->assertSame( $actor, $cachedActor );
} else {
$this->assertNull( $actor );
}
}
public function provideGetActorByMethods() {
yield 'getUserIdentityByName, registered' => [
'getUserIdentityByName', // $method
'TestUser', // $argument
@ -115,16 +135,16 @@ class ActorStoreTest extends ActorStoreTestBase {
*/
public function testSequentialCacheRetrieval( UserIdentity $expected ) {
// ensure UserIdentity is cached
$actorId = $this->getStore()->findActorId( $expected );
$actorId = $this->getStore()->findActorId( $expected, $this->db );
$this->assertSame( $expected->getActorId(), $actorId );
$cachedActorId = $this->getStore()->findActorId( $expected );
$cachedActorId = $this->getStore()->findActorId( $expected, $this->db );
$this->assertSame( $actorId, $cachedActorId );
$cachedActorId = $this->getStore()->acquireActorId( $expected );
$cachedActorId = $this->getStore()->acquireActorId( $expected, $this->db );
$this->assertSame( $actorId, $cachedActorId );
$cached = $this->getStore()->getActorById( $actorId );
$cached = $this->getStore()->getActorById( $actorId, $this->db );
$this->assertNotNull( $cached );
$this->assertSameActors( $expected, $cached );
@ -145,7 +165,7 @@ class ActorStoreTest extends ActorStoreTestBase {
*/
public function testGetActorByIdRealUser() {
$user = $this->getTestUser()->getUser();
$actor = $this->getStore()->getActorById( $user->getActorId() );
$actor = $this->getStore()->getActorById( $user->getActorId(), $this->db );
$this->assertSameActors( $user, $actor );
}
@ -400,12 +420,12 @@ class ActorStoreTest extends ActorStoreTestBase {
$this->executeWithForeignStore(
$wikiId,
function ( ActorStore $store ) use ( $expected, $actor ) {
$this->assertSame( $expected, $store->findActorId( $actor ) );
$this->assertSame( $expected, $store->findActorId( $actor, $this->db ) );
$this->assertSame( $expected ?: 0, $actor->getActorId( $actor->getWikiId() ) );
}
);
} else {
$this->assertSame( $expected, $this->getStore()->findActorId( $actor ) );
$this->assertSame( $expected, $this->getStore()->findActorId( $actor, $this->db ) );
$this->assertSame( $expected ?: 0, $actor->getActorId( $actor->getWikiId() ) );
}
}
@ -417,7 +437,8 @@ class ActorStoreTest extends ActorStoreTestBase {
$this->markTestSkipped();
$this->expectException( PreconditionException::class );
$this->getStore()->findActorId(
new UserIdentityValue( 0, self::IP, 0, 'acmewiki' )
new UserIdentityValue( 0, self::IP, 0, 'acmewiki' ),
$this->db
);
}
@ -461,7 +482,7 @@ class ActorStoreTest extends ActorStoreTestBase {
* @covers ::findActorId
*/
public function testFindActorIdByName( $name, $expected ) {
$this->assertSame( $expected, $this->getStore()->findActorIdByName( $name ) );
$this->assertSame( $expected, $this->getStore()->findActorIdByName( $name, $this->db ) );
}
public function provideAcquireActorId() {
@ -482,7 +503,7 @@ class ActorStoreTest extends ActorStoreTestBase {
*/
public function testAcquireActorId( callable $userCallback ) {
$user = $userCallback( $this->getServiceContainer() );
$actorId = $this->getStore()->acquireActorId( $user );
$actorId = $this->getStore()->acquireActorId( $user, $this->db );
$this->assertTrue( $actorId > 0 );
$this->assertSame( $actorId, $user->getActorId() );
}
@ -505,7 +526,7 @@ class ActorStoreTest extends ActorStoreTestBase {
$this->executeWithForeignStore(
'acmewiki',
function ( ActorStore $store ) use ( $user ) {
$actorId = $store->acquireActorId( $user );
$actorId = $store->acquireActorId( $user, $this->db );
$this->assertTrue( $actorId > 0 );
$this->assertSame( $actorId, $user->getActorId( $user->getWikiId() ) );
}
@ -539,7 +560,7 @@ class ActorStoreTest extends ActorStoreTestBase {
*/
public function testAcquireActorId_canNotCreate( UserIdentityValue $actor ) {
$this->expectException( CannotCreateActorException::class );
$this->getStore()->acquireActorId( $actor );
$this->getStore()->acquireActorId( $actor, $this->db );
}
public function provideAcquireActorId_existing() {
@ -558,7 +579,7 @@ class ActorStoreTest extends ActorStoreTestBase {
* @covers ::acquireActorId
*/
public function testAcquireActorId_existing( UserIdentityValue $actor, int $expected ) {
$this->assertSame( $expected, $this->getStore()->acquireActorId( $actor ) );
$this->assertSame( $expected, $this->getStore()->acquireActorId( $actor, $this->db ) );
}
public function testAcquireActorId_domain_mismatch() {
@ -576,7 +597,8 @@ class ActorStoreTest extends ActorStoreTestBase {
$this->markTestSkipped();
$this->expectException( PreconditionException::class );
$this->getStore()->acquireActorId(
new UserIdentityValue( 0, self::IP, 0, 'acmewiki' )
new UserIdentityValue( 0, self::IP, 0, 'acmewiki' ),
$this->db
);
}
@ -610,4 +632,16 @@ class ActorStoreTest extends ActorStoreTestBase {
$this->assertSame( $expected, $store->normalizeUserName( $name, $rigor ) );
}
public function testNewSelectQueryBuilderWithoutDB() {
$store = $this->getStore();
$queryBuilder = $store->newSelectQueryBuilder();
$this->assertInstanceOf( UserSelectQueryBuilder::class, $queryBuilder );
}
public function testNewSelectQueryBuilderWithDB() {
$store = $this->getStore();
$queryBuilder = $store->newSelectQueryBuilder( $this->db );
$this->assertInstanceOf( UserSelectQueryBuilder::class, $queryBuilder );
}
}