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

Improvements to user preferences fetching/saving

Browse source 
== Status quo ==

When saving user preferences, we want to lock the rows to avoid
accidentally overriding a concurrent options update. So usually
what extensions do is:

$value = $mngr->getOption( 'option', ..., READ_LOCKING );
if ( $value !== 'new_value' ) {
  $mngr->setOption( 'option', 'new_value' );
  $mngr->saveOptions()
}

Previously for extra caution we've ignored all caches in options
manager if >= READ_LOCKING flags were passed. This resulted in
re-reading all the options multiple times. At worst, 3 times:
1. If READ_NORMAL read was made for update - that's once,
2. On setOption, one more read, forcefully from primary
3. On saveOptions, one more read from primary, non-locking,
to figure out which option keys need to be deleted.

Also, READ_LOCKING was not used where it clearly had to be used,
for example right before the update. This was trying to fix any
kind of error on part of the manager clients, unsuccessfully so.

== New approach ==

1. Cache modified user options separately from originals and merge
them on demand. This means when refetching originals with LOCKING
we don't wipe out all modifications made to the cache with setOption.
Extra bonus - we no longer need to load all options to set an option.
2. Split the originals cache into 2 layers - one for stuff that
comes from DB directly, and one with applied normalizations and
whatever hooks modify. This let's us avoid refetching DB options
after we save them, but still let's the hooks execute on newly set
options after they're saved.
3. Cache options with all query flags. This is a bit controversial, but
ideally LOCKING flags will be applied on options fetch right before
they are saved. We have to re-read options with LOCKING in saveOptions
to avoid races, but if the caller did 'getOption( ..., LOCKING),
setOption(), save()' we will not need to re-select with LOCKING again.

Bug: T280220
Change-Id: Ibed2789f5260b725fd806b4470631aa30d814ce6
This commit is contained in:
Petr Pchelko 2021-06-11 11:48:19 -07:00 committed by Legoktm
parent 4c85648f32
commit 73bcc40836
4 changed files with 187 additions and 116 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
RELEASE-NOTES-1.37
View file

@ -445,6 +445,8 @@ because of Phabricator reports.
and ALF_PRELOAD_EXISTENCE have been removed. They're unused since 1.25.
* CSS class 'mw-htmlform-field-autoinfuse' used by some forms has been
renamed to 'mw-htmlform-autoinfuse'. (T278036)
* User::newFromRow does not accept pre-loaded user preferences under
$data['user_properties'] anymore. This optimization was not used.
* …
== Compatibility ==

6
includes/user/User.php
View file

@ -1321,7 +1321,6 @@ class User implements Authority, IDBAccessObject, UserIdentity, UserEmailContact
* table. Previously you were supposed to pass an array of strings
* here, but we also need expiry info nowadays, so an array of
* strings is ignored.
* user_properties Array with properties out of the user_properties table
*/
protected function loadFromRow( $row, $data = null ) {
if ( !is_object( $row ) ) {
@ -1422,11 +1421,6 @@ class User implements Authority, IDBAccessObject, UserIdentity, UserEmailContact
$this->queryFlagsUsed
);
}
if ( isset( $data['user_properties'] ) && is_array( $data['user_properties'] ) ) {
MediaWikiServices::getInstance()
->getUserOptionsManager()
->loadUserOptions( $this, $this->queryFlagsUsed, $data['user_properties'] );
}
}
}

260
includes/user/UserOptionsManager.php
View file

@ -34,7 +34,6 @@ use MediaWiki\Languages\LanguageConverterFactory;
use MediaWiki\MediaWikiServices;
use Psr\Log\LoggerInterface;
use User;
use Wikimedia\Rdbms\IDatabase;
use Wikimedia\Rdbms\ILoadBalancer;
/**
@ -66,12 +65,22 @@ class UserOptionsManager extends UserOptionsLookup {
/** @var LoggerInterface */
private $logger;
/** @var array Cached options by user */
private $optionsCache = [];
/** @var array options modified withing this request */
private $modifiedOptions = [];
/** @var array Cached original user options fetched from database */
/**
* @var array Cached original user options with all the adjustments
* like time correction and hook changes applied.
* Ready to be returned.
*/
private $originalOptionsCache = [];
/**
* @var array Cached original user options as fetched from database,
* no adjustments applied.
*/
private $optionsFromDb = [];
/** @var HookRunner */
private $hookRunner;
@ -175,18 +184,11 @@ class UserOptionsManager extends UserOptionsLookup {
* @param mixed $val New value to set
*/
public function setOption( UserIdentity $user, string $oname, $val ) {
// In case the options are modified, we need to refetch
// latest options in case they were not fetched from master
// so that we don't get a race condition trying to save modified options.
$this->loadUserOptions( $user, self::READ_LATEST );
// Explicitly NULL values should refer to defaults
if ( $val === null ) {
$val = $this->defaultOptionsLookup->getDefaultOption( $oname );
}
$userKey = $this->getCacheKey( $user );
$this->optionsCache[$userKey][$oname] = $val;
$this->modifiedOptions[$this->getCacheKey( $user )][$oname] = $val;
}
/**
@ -196,6 +198,8 @@ class UserOptionsManager extends UserOptionsLookup {
* Supported values are everything that can be reported by getOptionKinds()
* and 'all', which forces a reset of *all* preferences and overrides everything else.
*
* @note You need to call saveOptions() to actually write to the database.
*
* @param UserIdentity $user
* @param IContextSource $context Context source used when $resetKinds does not contain 'all'.
* @param array|string $resetKinds Which kinds of preferences to reset.
@ -206,7 +210,7 @@ class UserOptionsManager extends UserOptionsLookup {
IContextSource $context,
$resetKinds = [ 'registered', 'registered-multiselect', 'registered-checkmatrix', 'unused' ]
) {
$oldOptions = $this->loadUserOptions( $user, self::READ_LATEST );
$oldOptions = $this->loadUserOptions( $user, self::READ_LOCKING );
$defaultOptions = $this->defaultOptionsLookup->getDefaultOptions();
if ( !is_array( $resetKinds ) ) {
@ -214,7 +218,7 @@ class UserOptionsManager extends UserOptionsLookup {
}
if ( in_array( 'all', $resetKinds ) ) {
$newOptions = $defaultOptions;
$newOptions = $defaultOptions + array_fill_keys( array_keys( $oldOptions ), null );
} else {
$optionKinds = $this->getOptionKinds( $user, $context );
$resetKinds = array_intersect( $resetKinds, $this->listOptionKinds() );
@ -238,7 +242,7 @@ class UserOptionsManager extends UserOptionsLookup {
User::newFromIdentity( $user ), $newOptions, $oldOptions, $resetKinds
);
$this->optionsCache[$this->getCacheKey( $user )] = $newOptions;
$this->modifiedOptions[$this->getCacheKey( $user )] = $newOptions;
}
/**
@ -381,29 +385,27 @@ class UserOptionsManager extends UserOptionsLookup {
$userKey = $this->getCacheKey( $user );
// Not using getOptions(), to keep hidden preferences in database
$saveOptions = $this->loadUserOptions( $user, self::READ_LATEST );
$optionsToSave = $this->loadUserOptions( $user, self::READ_LOCKING );
$originalOptions = $this->originalOptionsCache[$userKey] ?? [];
// Allow hooks to abort, for instance to save to a global profile.
// Reset options to default state before saving.
// TODO: Deprecate passing User to the hook.
if ( !$this->hookRunner->onUserSaveOptions(
User::newFromIdentity( $user ), $saveOptions, $originalOptions )
User::newFromIdentity( $user ), $optionsToSave, $originalOptions )
) {
return;
}
// In case options were modified by the hook
$this->optionsCache[$userKey] = $saveOptions;
$userId = $user->getId();
$insert_rows = []; // all the new preference rows
foreach ( $saveOptions as $key => $value ) {
$rowsToInsert = []; // all the new preference rows
foreach ( $optionsToSave as $key => $value ) {
// Don't bother storing default values
$defaultOption = $this->defaultOptionsLookup->getDefaultOption( $key );
if ( ( $defaultOption === null && $value !== false && $value !== null )
|| $value != $defaultOption
) {
$insert_rows[] = [
$rowsToInsert[] = [
'up_user' => $userId,
'up_property' => $key,
'up_value' => $value,
@ -411,32 +413,22 @@ class UserOptionsManager extends UserOptionsLookup {
}
}
$dbw = $this->loadBalancer->getConnectionRef( DB_PRIMARY );
$res = $dbw->select(
'user_properties',
[ 'up_property', 'up_value' ],
[ 'up_user' => $userId ],
__METHOD__
);
// Find prior rows that need to be removed or updated. These rows will
// all be deleted (the latter so that INSERT IGNORE applies the new values).
$keysDelete = [];
foreach ( $res as $row ) {
if ( !isset( $saveOptions[$row->up_property] ) ||
$saveOptions[$row->up_property] !== $row->up_value
$keysToDelete = [];
foreach ( $this->optionsFromDb[$userKey] as $dbOptionKey => $dbOptionValue ) {
if ( !isset( $optionsToSave[$dbOptionKey] ) ||
$optionsToSave[$dbOptionKey] !== $dbOptionValue
) {
$keysDelete[] = $row->up_property;
$keysToDelete[] = $dbOptionKey;
}
}
if ( !count( $keysDelete ) && !count( $insert_rows ) ) {
if ( !count( $keysToDelete ) && !count( $rowsToInsert ) ) {
return;
}
$this->originalOptionsCache[$userKey] = null;
if ( count( $keysDelete ) ) {
$dbw = $this->loadBalancer->getConnectionRef( DB_PRIMARY );
if ( count( $keysToDelete ) ) {
// Do the DELETE by PRIMARY KEY for prior rows.
// In the past a very large portion of calls to this function are for setting
// 'rememberpassword' for new accounts (a preference that has since been removed).
@ -448,7 +440,7 @@ class UserOptionsManager extends UserOptionsLookup {
'user_properties',
[
'up_user' => $userId,
'up_property' => $keysDelete
'up_property' => $keysToDelete
],
__METHOD__
);
@ -456,10 +448,21 @@ class UserOptionsManager extends UserOptionsLookup {
// Insert the new preference rows
$dbw->insert(
'user_properties',
$insert_rows,
$rowsToInsert,
__METHOD__,
[ 'IGNORE' ]
);
// We are storing these into the database, so that's what we will get if we fetch again
// So, set the just saved options to the cache, but don't prevent some other part of the
// code from locking the options again but saying READ_LATEST was used for caching.
$this->setOptionsFromDb( $user, self::READ_LATEST, $rowsToInsert );
// It's pretty cheap to recalculate new original later
// to apply whatever adjustments we apply when fetching from DB
// and re-merge with the defaults.
unset( $this->originalOptionsCache[$userKey] );
// And nothing is modified anymore
unset( $this->modifiedOptions[$userKey] );
}
/**
@ -472,7 +475,7 @@ class UserOptionsManager extends UserOptionsLookup {
*
* @param UserIdentity $user
* @param int $queryFlags
* @param array|null $data preloaded row from the user_properties table
* @param array|null $data associative array of non-default options.
* @return array
* @internal To be called by User loading code to provide the $data
*/
@ -482,24 +485,115 @@ class UserOptionsManager extends UserOptionsLookup {
array $data = null
): array {
$userKey = $this->getCacheKey( $user );
if ( $this->canUseCachedValues( $user, $queryFlags )
&& isset( $this->optionsCache[$userKey] )
) {
return $this->optionsCache[$userKey];
$originalOptions = $this->loadOriginalOptions( $user, $queryFlags, $data );
return array_merge( $originalOptions, $this->modifiedOptions[$userKey] ?? [] );
}
/**
* Clears cached user options.
* @internal To be used by User::clearInstanceCache
* @param UserIdentity $user
*/
public function clearUserOptionsCache( UserIdentity $user ) {
$cacheKey = $this->getCacheKey( $user );
unset( $this->modifiedOptions[$cacheKey] );
unset( $this->optionsFromDb[$cacheKey] );
unset( $this->originalOptionsCache[$cacheKey] );
unset( $this->queryFlagsUsedForCaching[$cacheKey] );
}
/**
* Fetches the options directly from the database with no caches.
*
* @param UserIdentity $user
* @param int $queryFlags
* @param array|null $prefetchedOptions
* @return array
*/
private function loadOptionsFromDb(
UserIdentity $user,
int $queryFlags,
array $prefetchedOptions = null
): array {
if ( $prefetchedOptions === null ) {
$this->logger->debug( 'Loading options from database', [ 'user_id' => $user->getId() ] );
[ $dbr, $options ] = $this->getDBAndOptionsForQueryFlags( $queryFlags );
$res = $dbr->select(
'user_properties',
[ 'up_property', 'up_value' ],
[ 'up_user' => $user->getId() ],
__METHOD__,
$options
);
} else {
$res = [];
foreach ( $prefetchedOptions as $name => $value ) {
$res[] = [
'up_property' => $name,
'up_value' => $value,
];
}
}
return $this->setOptionsFromDb( $user, $queryFlags, $res );
}
$options = $this->defaultOptionsLookup->getDefaultOptions();
/**
* Builds associative options array from rows fetched from DB.
*
* @param UserIdentity $user
* @param int $queryFlags
* @param iterable<object|array> $rows
* @return array
*/
private function setOptionsFromDb(
UserIdentity $user,
int $queryFlags,
iterable $rows
): array {
$userKey = $this->getCacheKey( $user );
$options = [];
foreach ( $rows as $row ) {
$row = (object)$row;
// Convert '0' to 0. PHP's boolean conversion considers them both
// false, but e.g. JavaScript considers the former as true.
// @todo: T54542 Somehow determine the desired type (string/int/bool)
// and convert all values here.
if ( $row->up_value === '0' ) {
$row->up_value = 0;
}
$options[$row->up_property] = $row->up_value;
}
$this->optionsFromDb[$userKey] = $options;
$this->queryFlagsUsedForCaching[$userKey] = $queryFlags;
return $options;
}
/**
* Loads the original user options from the database and applies various transforms,
* like timecorrection. Runs hooks.
*
* @param UserIdentity $user
* @param int $queryFlags
* @param array|null $data associative array of non-default options
* @return array
*/
private function loadOriginalOptions(
UserIdentity $user,
int $queryFlags = self::READ_NORMAL,
array $data = null
): array {
$userKey = $this->getCacheKey( $user );
$defaultOptions = $this->defaultOptionsLookup->getDefaultOptions();
if ( !$user->isRegistered() ) {
// For unlogged-in users, load language/variant options from request.
// There's no need to do it for logged-in users: they can set preferences,
// and handling of page content is done by $pageLang->getPreferredVariant() and such,
// so don't override user's choice (especially when the user chooses site default).
$variant = $this->languageConverterFactory->getLanguageConverter()->getDefaultVariant();
$options['variant'] = $variant;
$options['language'] = $variant;
$this->optionsCache[$userKey] = $options;
return $options;
$defaultOptions['variant'] = $variant;
$defaultOptions['language'] = $variant;
$this->originalOptionsCache[$userKey] = $defaultOptions;
return $defaultOptions;
}
// In case options were already loaded from the database before and no options
@ -507,42 +601,10 @@ class UserOptionsManager extends UserOptionsLookup {
if ( $this->canUseCachedValues( $user, $queryFlags )
&& isset( $this->originalOptionsCache[$userKey] )
) {
$this->logger->debug( 'Loading options from override cache', [
'user_id' => $user->getId()
] );
return $this->originalOptionsCache[$userKey];
} else {
if ( !is_array( $data ) ) {
$this->logger->debug( 'Loading options from database', [
'user_id' => $user->getId()
] );
$dbr = $this->getDBForQueryFlags( $queryFlags );
$res = $dbr->select(
'user_properties',
[ 'up_property', 'up_value' ],
[ 'up_user' => $user->getId() ],
__METHOD__
);
$data = [];
foreach ( $res as $row ) {
// Convert '0' to 0. PHP's boolean conversion considers them both
// false, but e.g. JavaScript considers the former as true.
// @todo: T54542 Somehow determine the desired type (string/int/bool)
// and convert all values here.
if ( $row->up_value === '0' ) {
$row->up_value = 0;
}
$data[$row->up_property] = $row->up_value;
}
}
foreach ( $data as $property => $value ) {
$options[$property] = $value;
}
}
$options = $this->loadOptionsFromDb( $user, $queryFlags, $data ) + $defaultOptions;
// Replace deprecated language codes
$options['language'] = LanguageCode::replaceDeprecatedCodes( $options['language'] );
// Fix up timezone offset (Due to DST it can change from what was stored in the DB)
@ -563,23 +625,8 @@ class UserOptionsManager extends UserOptionsLookup {
$this->hookRunner->onUserLoadOptions(
User::newFromIdentity( $user ), $options
);
$this->originalOptionsCache[$userKey] = $options;
$this->optionsCache[$userKey] = $options;
return $this->optionsCache[$userKey];
}
/**
* Clears cached user options.
* @internal To be used by User::clearInstanceCache
* @param UserIdentity $user
*/
public function clearUserOptionsCache( UserIdentity $user ) {
$cacheKey = $this->getCacheKey( $user );
$this->optionsCache[$cacheKey] = null;
$this->originalOptionsCache[$cacheKey] = null;
$this->queryFlagsUsedForCaching[$cacheKey] = null;
return $options;
}
/**
@ -593,11 +640,11 @@ class UserOptionsManager extends UserOptionsLookup {
/**
* @param int $queryFlags a bit field composed of READ_XXX flags
* @return IDatabase
* @return array [ IDatabase $db, array $options ]
*/
private function getDBForQueryFlags( $queryFlags ): IDatabase {
list( $mode, ) = DBAccessObjectUtils::getDBOptions( $queryFlags );
return $this->loadBalancer->getConnectionRef( $mode, [] );
private function getDBAndOptionsForQueryFlags( $queryFlags ): array {
list( $mode, $options ) = DBAccessObjectUtils::getDBOptions( $queryFlags );
return [ $this->loadBalancer->getConnectionRef( $mode, [] ), $options ];
}
/**
@ -612,9 +659,6 @@ class UserOptionsManager extends UserOptionsLookup {
// so $queryFlags are ignored.
return true;
}
if ( $queryFlags >= self::READ_LOCKING ) {
return false;
}
$userKey = $this->getCacheKey( $user );
$queryFlagsUsed = $this->queryFlagsUsedForCaching[$userKey] ?? self::READ_NONE;
return $queryFlagsUsed >= $queryFlags;

35
tests/phpunit/includes/user/UserOptionsManagerTest.php
View file

@ -5,6 +5,7 @@ use MediaWiki\MediaWikiServices;
use MediaWiki\User\UserOptionsLookup;
use MediaWiki\User\UserOptionsManager;
use Psr\Log\NullLogger;
use Wikimedia\Rdbms\ILoadBalancer;
/**
* @group Database
@ -14,7 +15,8 @@ class UserOptionsManagerTest extends UserOptionsLookupTest {
private function getManager(
string $langCode = 'qqq',
array $defaultOptionsOverrides = []
array $defaultOptionsOverrides = [],
ILoadBalancer $lbOverride = null
) {
$services = MediaWikiServices::getInstance();
return new UserOptionsManager(
@ -27,7 +29,7 @@ class UserOptionsManagerTest extends UserOptionsLookupTest {
),
$this->getDefaultManager( $langCode, $defaultOptionsOverrides ),
$services->getLanguageConverterFactory(),
$services->getDBLoadBalancer(),
$lbOverride ?? $services->getDBLoadBalancer(),
new NullLogger(),
$services->getHookContainer()
);
@ -248,4 +250,33 @@ class UserOptionsManagerTest extends UserOptionsLookupTest {
$manager->clearUserOptionsCache( $user );
$this->assertNull( $manager->getOption( $user, 'test_option' ) );
}
public function testOptionsForUpdateNotRefetchedBeforeInsert() {
$mockDb = $this->createMock( \Wikimedia\Rdbms\IDatabase::class );
$mockDb->expects( $this->once() ) // This is critical what we are testing
->method( 'select' )
->willReturn( new FakeResultWrapper( [
[
'up_value' => 'blabla',
'up_property' => 'test_option',
]
] ) );
$mockLoadBalancer = $this->createMock( ILoadBalancer::class );
$mockLoadBalancer
->method( 'getConnectionRef' )
->willReturn( $mockDb );
$user = $this->getTestUser()->getUser();
$manager = $this->getManager( 'qqq', [], $mockLoadBalancer );
$manager->getOption(
$user,
'test_option',
null,
false,
UserOptionsManager::READ_LOCKING
);
$manager->getOption( $user, 'test_option2' );
$manager->setOption( $user, 'test_option', 'test_value' );
$manager->setOption( $user, 'test_option2', 'test_value2' );
$manager->saveOptions( $user );
}
}