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

Merge "Keep ActorStore caches consistent on user rename"

Browse source
This commit is contained in:
jenkins-bot 2021-04-26 21:33:59 +00:00 committed by Gerrit Code Review
commit ef5a13550a
5 changed files with 317 additions and 84 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

146
includes/user/ActorCache.php Normal file
View file

@ -0,0 +1,146 @@
<?php
/**
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License along
* with this program; if not, write to the Free Software Foundation, Inc.,
* 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
* http://www.gnu.org/copyleft/gpl.html
*
* @file
*/
namespace MediaWiki\User;
/**
* Simple in-memory cache for UserIdentity objects indexed by user ID,
* actor ID and user name.
*
* We cannot just use MapCacheLRU for this because of eviction semantics:
* we need to be able to remove UserIdentity from the cache even if
* user ID or user name has changed, so we track the most accessed VALUES
* in the cache, not keys, and evict them alongside with all their indexes.
*
* @since 1.37
* @internal for use by ActorStore
* @package MediaWiki\User
*/
class ActorCache {
/** @var string Key by actor ID */
public const KEY_ACTOR_ID = 'actorId';
/** @var string Key by user ID */
public const KEY_USER_ID = 'userId';
/** @var string Key by user name */
public const KEY_USER_NAME = 'name';
/** @var int */
private $maxSize;
/**
* @var array[][] Contains 3 keys, KEY_ACTOR_ID, KEY_USER_ID, and KEY_USER_NAME,
* each of which has a value of an array of arrays with actor ids and UserIdentity objects,
* keyed with the corresponding actor id/user id/user name
*/
private $cache = [ self::KEY_ACTOR_ID => [], self::KEY_USER_NAME => [], self::KEY_USER_ID => [] ];
/**
* @param int $maxSize hold up to this many UserIdentity values
*/
public function __construct( int $maxSize ) {
$this->maxSize = $maxSize;
}
/**
* Get user identity which has $keyType equal to $keyValue
* @param string $keyType one of self::KEY_* constants.
* @param string|int $keyValue
* @return UserIdentity|null
*/
public function getActor( string $keyType, $keyValue ): ?UserIdentity {
return $this->getCachedValue( $keyType, $keyValue )['actor'] ?? null;
}
/**
* Get actor ID of the user which has $keyType equal to $keyValue.
* @param string $keyType one of self::KEY_* constants.
* @param string|int $keyValue
* @return int|null
*/
public function getActorId( string $keyType, $keyValue ): ?int {
return $this->getCachedValue( $keyType, $keyValue )['actorId'] ?? null;
}
/**
* Add $actor with $actorId to the cache.
* @param int $actorId
* @param UserIdentity $actor
*/
public function add( int $actorId, UserIdentity $actor ) {
while ( count( $this->cache[self::KEY_ACTOR_ID] ) >= $this->maxSize ) {
reset( $this->cache[self::KEY_ACTOR_ID] );
$evictId = key( $this->cache[self::KEY_ACTOR_ID] );
$this->remove( $this->cache[self::KEY_ACTOR_ID][$evictId]['actor'] );
}
$value = [ 'actorId' => $actorId, 'actor' => $actor ];
$this->cache[self::KEY_ACTOR_ID][$actorId] = $value;
$userId = $actor->getId( $actor->getWikiId() );
if ( $userId ) {
$this->cache[self::KEY_USER_ID][$userId] = $value;
}
$this->cache[self::KEY_USER_NAME][$actor->getName()] = $value;
}
/**
* Remove $actor from cache.
* @param UserIdentity $actor
*/
public function remove( UserIdentity $actor ) {
$oldByName = $this->cache[self::KEY_USER_NAME][$actor->getName()] ?? null;
$oldByUserId = $this->cache[self::KEY_USER_ID][$actor->getId( $actor->getWikiId() )] ?? null;
if ( $oldByName ) {
unset( $this->cache[self::KEY_USER_ID][$oldByName['actor']->getId( $oldByName['actor']->getWikiId() )] );
unset( $this->cache[self::KEY_ACTOR_ID][$oldByName['actorId']] );
}
if ( $oldByUserId ) {
unset( $this->cache[self::KEY_USER_NAME][$oldByUserId['actor']->getName()] );
unset( $this->cache[self::KEY_ACTOR_ID][$oldByUserId['actorId']] );
}
unset( $this->cache[self::KEY_USER_NAME][$actor->getName()] );
unset( $this->cache[self::KEY_USER_ID][$actor->getId( $actor->getWikiId() )] );
}
/**
* @param string $keyType one of self::KEY_* constants.
* @param string|int $keyValue
* @return array|null [ 'actor' => UserIdentity, 'actorId' => int ]
*/
private function getCachedValue( string $keyType, $keyValue ): ?array {
if ( isset( $this->cache[$keyType][$keyValue] ) ) {
$cached = $this->cache[$keyType][$keyValue];
$this->ping( $cached['actorId'] );
return $cached;
}
return null;
}
/**
* Record the actor with $actorId was recently used.
* @param int $actorId
*/
private function ping( int $actorId ) {
$item = $this->cache[self::KEY_ACTOR_ID][$actorId];
unset( $this->cache[self::KEY_ACTOR_ID][$actorId] );
$this->cache[self::KEY_ACTOR_ID][$actorId] = $item;
}
}

121
includes/user/ActorStore.php
View file

@ -24,7 +24,6 @@ use CannotCreateActorException;
use DBAccessObjectUtils;
use ExternalUserNames;
use InvalidArgumentException;
use MapCacheLRU;
use MediaWiki\DAO\WikiAwareEntity;
use Psr\Log\LoggerInterface;
use stdClass;
@ -59,14 +58,8 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
/** @var string|false */
private $wikiId;
/** @var MapCacheLRU int actor ID => [ UserIdentity, int actor ID ] */
private $actorsByActorId;
/** @var MapCacheLRU int user ID => [ UserIdentity, int actor ID ] */
private $actorsByUserId;
/** @var MapCacheLRU string user name => [ UserIdentity, int actor ID ] */
private $actorsByName;
/** @var ActorCache */
private $cache;
/**
* @param ILoadBalancer $loadBalancer
@ -88,9 +81,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
$this->logger = $logger;
$this->wikiId = $wikiId;
$this->actorsByActorId = new MapCacheLRU( self::LOCAL_CACHE_SIZE );
$this->actorsByUserId = new MapCacheLRU( self::LOCAL_CACHE_SIZE );
$this->actorsByName = new MapCacheLRU( self::LOCAL_CACHE_SIZE );
$this->cache = new ActorCache( self::LOCAL_CACHE_SIZE );
}
/**
@ -130,7 +121,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
}
$actor = new UserIdentityValue( $userId, $row->actor_name, $this->wikiId );
$this->addUserIdentityToCache( $actorId, $actor );
$this->cache->add( $actorId, $actor );
return $actor;
}
@ -186,34 +177,16 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
$this->wikiId
);
$this->addUserIdentityToCache( $actorId, $actor );
$this->cache->add( $actorId, $actor );
return $actor;
}
/**
* @param int $actorId
* @param UserIdentity $actor
* @internal for use in User object only
*/
private function addUserIdentityToCache( int $actorId, UserIdentity $actor ) {
$this->actorsByActorId->set( $actorId, [ $actor, $actorId ] );
$userId = $actor->getId( $this->wikiId );
if ( $userId ) {
$this->actorsByUserId->set( $userId, [ $actor, $actorId ] );
}
$this->actorsByName->set( $actor->getName(), [ $actor, $actorId ] );
}
/**
* @param int $actorId
* @param UserIdentity $actor
*/
private function deleteUserIdentityFromCache( int $actorId, UserIdentity $actor ) {
$this->actorsByActorId->clear( $actorId );
$userId = $actor->getId( $this->wikiId );
if ( $userId ) {
$this->actorsByUserId->clear( $userId );
}
$this->actorsByName->clear( $actor->getName() );
public function deleteUserIdentityFromCache( UserIdentity $actor ) {
$this->cache->remove( $actor );
}
/**
@ -231,26 +204,18 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
return null;
}
$cachedValue = $this->actorsByActorId->get( $actorId );
if ( $cachedValue ) {
return $cachedValue[0];
}
$actor = $this->newSelectQueryBuilder( $db )
->caller( __METHOD__ )
->conds( [ 'actor_id' => $actorId ] )
->fetchUserIdentity();
// The actor ID mostly comes from DB, so if we can't find an actor by ID,
// it's most likely due to lagged replica and not cause it doesn't actually exist.
// Probably we just inserted it? Try master.
if ( !$actor ) {
$actor = $this->newSelectQueryBuilderForQueryFlags( self::READ_LATEST )
return $this->cache->getActor( ActorCache::KEY_ACTOR_ID, $actorId ) ??
$this->newSelectQueryBuilder( $db )
->caller( __METHOD__ )
->conds( [ 'actor_id' => $actorId ] )
->fetchUserIdentity() ??
// The actor ID mostly comes from DB, so if we can't find an actor by ID,
// it's most likely due to lagged replica and not cause it doesn't actually exist.
// Probably we just inserted it? Try master.
$this->newSelectQueryBuilderForQueryFlags( self::READ_LATEST )
->caller( __METHOD__ )
->conds( [ 'actor_id' => $actorId ] )
->fetchUserIdentity();
}
return $actor;
}
/**
@ -273,15 +238,11 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
);
}
$cachedValue = $this->actorsByName->get( $normalizedName );
if ( $cachedValue ) {
return $cachedValue[0];
}
return $this->newSelectQueryBuilderForQueryFlags( $queryFlags )
->caller( __METHOD__ )
->userNames( $normalizedName )
->fetchUserIdentity();
return $this->cache->getActor( ActorCache::KEY_USER_NAME, $normalizedName ) ??
$this->newSelectQueryBuilderForQueryFlags( $queryFlags )
->caller( __METHOD__ )
->userNames( $normalizedName )
->fetchUserIdentity();
}
/**
@ -296,15 +257,11 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
return null;
}
$cachedValue = $this->actorsByUserId->get( $userId );
if ( $cachedValue ) {
return $cachedValue[0];
}
return $this->newSelectQueryBuilderForQueryFlags( $queryFlags )
->caller( __METHOD__ )
->userIds( $userId )
->fetchUserIdentity();
return $this->cache->getActor( ActorCache::KEY_USER_ID, $userId ) ??
$this->newSelectQueryBuilderForQueryFlags( $queryFlags )
->caller( __METHOD__ )
->userIds( $userId )
->fetchUserIdentity();
}
/**
@ -388,9 +345,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
return null;
}
$id = $this->findActorIdInternal( $name, $db );
return $id;
return $this->findActorIdInternal( $name, $db );
}
/**
@ -412,9 +367,9 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
// User always thinks it's local, so we could end up fetching the ID
// from the wrong database.
$cachedValue = $this->actorsByName->get( $name );
$cachedValue = $this->cache->getActorId( ActorCache::KEY_USER_NAME, $name );
if ( $cachedValue ) {
return $cachedValue[1];
return $cachedValue;
}
$row = $db->selectRow(
@ -504,7 +459,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
$this->attachActorId( $user, $actorId, true );
// Cache row we've just created
$cachedUserIdentity = $this->newActorFromRowFields( $userId, $userName, $actorId );
$this->setUpRollbackHandler( $dbw, $actorId, $cachedUserIdentity, $user );
$this->setUpRollbackHandler( $dbw, $cachedUserIdentity, $user );
return $actorId;
}
@ -544,7 +499,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
$this->attachActorId( $user, $actorId, true );
// Cache row we've just created
$cachedUserIdentity = $this->newActorFromRowFields( $userId, $userName, $actorId );
$this->setUpRollbackHandler( $dbw, $actorId, $cachedUserIdentity, $user );
$this->setUpRollbackHandler( $dbw, $cachedUserIdentity, $user );
return $actorId;
}
@ -573,7 +528,7 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
$existingActorId = $this->findActorIdInternal( $userName, $dbw );
if ( $existingActorId ) {
// It certainly will be cached if we just found it.
$existingActor = $this->actorsByActorId->get( $existingActorId )[0];
$existingActor = $this->cache->getActor( ActorCache::KEY_ACTOR_ID, $existingActorId );
// If we already have an existing actor with a matching user ID
// just return it, nothing to do here.
@ -605,11 +560,11 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
}
$actorId = $dbw->insertId() ?: $existingActorId;
$this->deleteUserIdentityFromCache( $actorId, $user );
$this->cache->remove( $user );
$this->attachActorId( $user, $actorId, true );
// Cache row we've just created
$cachedUserIdentity = $this->newActorFromRowFields( $userId, $userName, $actorId );
$this->setUpRollbackHandler( $dbw, $actorId, $cachedUserIdentity, $user );
$this->setUpRollbackHandler( $dbw, $cachedUserIdentity, $user );
return $actorId;
}
@ -674,13 +629,11 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
* Clear in-process caches if transaction gets rolled back.
*
* @param IDatabase $dbw
* @param int $actorId
* @param UserIdentity $cachedActor
* @param UserIdentity $originalActor
*/
private function setUpRollbackHandler(
IDatabase $dbw,
int $actorId,
UserIdentity $cachedActor,
UserIdentity $originalActor
) {
@ -688,9 +641,9 @@ class ActorStore implements UserIdentityLookup, ActorNormalization {
// If called within a transaction and it was rolled back, the cached actor ID
// becomes invalid, so cache needs to be invalidated as well. See T277795.
$dbw->onTransactionResolution(
function ( int $trigger ) use ( $actorId, $cachedActor, $originalActor ) {
function ( int $trigger ) use ( $cachedActor, $originalActor ) {
if ( $trigger === IDatabase::TRIGGER_ROLLBACK ) {
$this->deleteUserIdentityFromCache( $actorId, $cachedActor );
$this->cache->remove( $cachedActor );
$this->detachActorId( $originalActor );
}
} );

1
includes/user/User.php
View file

@ -3426,6 +3426,7 @@ class User implements Authority, IDBAccessObject, UserIdentity, UserEmailContact
[ 'actor_user' => $this->mId ],
$fname
);
MediaWikiServices::getInstance()->getActorStore()->deleteUserIdentityFromCache( $this );
} );
$this->mTouched = $newTouched;

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

@ -750,6 +750,23 @@ class ActorStoreTest extends ActorStoreTestBase {
$this->assertSameActors( $anotherActor, $store->getActorById( $anotherActorId, $this->db ) );
}
public function testUserRenameUpdatesActor() {
$user = $this->getMutableTestUser()->getUser();
$oldName = $user->getName();
$store = $this->getStore();
$actorId = $store->findActorId( $user, $this->db );
$this->assertTrue( $actorId > 0 );
$user->setName( 'NewName' );
$user->saveSettings();
$this->assertSameActors( $user, $store->getActorById( $actorId, $this->db ) );
$this->assertSameActors( $user, $store->getUserIdentityByName( 'NewName' ) );
$this->assertSameActors( $user, $store->getUserIdentityByUserId( $user->getId() ) );
$this->assertNull( $store->getUserIdentityByName( $oldName ) );
}
/**
* @covers ::acquireSystemActorId
*/

116
tests/phpunit/unit/includes/user/ActorCacheTest.php Normal file
View file

@ -0,0 +1,116 @@
<?php
namespace MediaWiki\Tests\Unit\User;
use MediaWiki\User\ActorCache;
use MediaWiki\User\UserIdentity;
use MediaWiki\User\UserIdentityValue;
use MediaWikiUnitTestCase;
/**
* @covers \MediaWiki\User\ActorCache
* @package MediaWiki\Tests\Unit\User
*/
class ActorCacheTest extends MediaWikiUnitTestCase {
/**
* @param ActorCache $cache
* @param int $actorId
* @param UserIdentity $actor
*/
private function assertCacheContains( ActorCache $cache, int $actorId, UserIdentity $actor ) {
$this->assertSame( $actor,
$cache->getActor( ActorCache::KEY_ACTOR_ID, $actorId ) );
$this->assertSame( $actor,
$cache->getActor( ActorCache::KEY_USER_NAME, $actor->getName() ) );
$this->assertSame( $actor,
$cache->getActor( ActorCache::KEY_USER_ID, $actor->getId( $actor->getWikiId() ) ) );
$this->assertSame( $actorId,
$cache->getActorId( ActorCache::KEY_ACTOR_ID, 1 ) );
$this->assertSame( $actorId,
$cache->getActorId( ActorCache::KEY_USER_NAME, $actor->getName() ) );
$this->assertSame( $actorId,
$cache->getActorId( ActorCache::KEY_USER_ID, $actor->getId( $actor->getWikiId() ) ) );
}
/**
* @param ActorCache $cache
* @param int $actorId
* @param string $actorName
* @param int $userId
*/
private function assertCacheNotContains( ActorCache $cache, int $actorId, string $actorName, int $userId ) {
$this->assertNull( $cache->getActor( ActorCache::KEY_ACTOR_ID, $actorId ) );
$this->assertNull( $cache->getActor( ActorCache::KEY_USER_NAME, $actorName ) );
$this->assertNull( $cache->getActor( ActorCache::KEY_USER_ID, $userId ) );
$this->assertNull( $cache->getActorId( ActorCache::KEY_ACTOR_ID, $actorId ) );
$this->assertNull( $cache->getActorId( ActorCache::KEY_USER_NAME, $actorName ) );
$this->assertNull( $cache->getActorId( ActorCache::KEY_USER_ID, $userId ) );
}
public function provideGetActor() {
yield 'local' => [
'actor' => new UserIdentityValue( 10, 'Hello' ),
];
yield 'foreign' => [
'actor' => new UserIdentityValue( 10, 'Hello', 'acmewiki' ),
];
}
/**
* @dataProvider provideGetActor
*/
public function testGetActor( UserIdentity $actor ) {
$cache = new ActorCache( 5 );
$cache->add( 1, $actor );
$this->assertCacheContains( $cache, 1, $actor );
}
public function provideRemove() {
yield 'Same actor' => [
'addedActor' => new UserIdentityValue( 10, 'Hello' ),
'removedActor' => new UserIdentityValue( 10, 'Hello' )
];
yield 'Different name' => [
'addedActor' => new UserIdentityValue( 10, 'Hello' ),
'removedActor' => new UserIdentityValue( 10, 'Goodbye' )
];
yield 'Different user ID' => [
'addedActor' => new UserIdentityValue( 10, 'Hello' ),
'removedActor' => new UserIdentityValue( 11, 'Hello' )
];
}
/**
* @dataProvider provideRemove
*/
public function testRemove( UserIdentity $added, UserIdentity $removed ) {
$cache = new ActorCache( 5 );
$cache->add( 1, $added );
$this->assertCacheContains( $cache, 1, $added );
$cache->remove( $removed );
$this->assertCacheNotContains( $cache, 1, $added->getName(), $added->getId() );
$this->assertCacheNotContains( $cache, 1, $removed->getName(), $removed->getId() );
}
public function testEvict() {
$cache = new ActorCache( 1 );
$actor1 = new UserIdentityValue( 10, 'Hello' );
$cache->add( 2, new UserIdentityValue( 12, 'Goodbye' ) );
$cache->add( 1, $actor1 );
$this->assertCacheContains( $cache, 1, $actor1 );
$this->assertCacheNotContains( $cache, 2, 'Goodbye', 12 );
}
public function testDoesNotCacheAnonUserId() {
$cache = new ActorCache( 5 );
$actor = new UserIdentityValue( 0, '127.0.0.1' );
$cache->add( 1, $actor );
$this->assertNull( $cache->getActor( ActorCache::KEY_USER_ID, 0 ) );
$this->assertNull( $cache->getActorId( ActorCache::KEY_USER_ID, 0 ) );
$this->assertSame( $actor, $cache->getActor( ActorCache::KEY_USER_NAME, '127.0.0.1' ) );
$this->assertSame( 1, $cache->getActorId( ActorCache::KEY_USER_NAME, '127.0.0.1' ) );
$this->assertSame( $actor, $cache->getActor( ActorCache::KEY_ACTOR_ID, 1 ) );
$this->assertSame( 1, $cache->getActorId( ActorCache::KEY_ACTOR_ID, 1 ) );
}
}