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

Avoid using stale data for revision visibility and actor data

Browse source 
Created class RevisionStoreCacheRecord to avoid returning stale
cached revision visibility and actor data when changes were
made after the cache was populated for that revision.

Bug: T216159
Change-Id: Iabed80e06a08ff0793dfe64db881cbcd535cb13f
This commit is contained in:
Bill Pirkle 2019-02-27 15:26:17 -06:00
parent da97c1faad
commit 276bf4a76b
6 changed files with 461 additions and 15 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

54
includes/Revision/RevisionStore.php
View file

@ -1760,10 +1760,16 @@ class RevisionStore
* @param object $row
* @param int $queryFlags
* @param Title|null $title
*
* @param bool $fromCache if true, the returned RevisionRecord will ensure that no stale
* data is returned from getters, by querying the database as needed
* @return RevisionRecord
*/
public function newRevisionFromRow( $row, $queryFlags = 0, Title $title = null ) {
public function newRevisionFromRow(
$row,
$queryFlags = 0,
Title $title = null,
$fromCache = false
) {
Assert::parameterType( 'object', $row, '$row' );
if ( !$title ) {
@ -1797,7 +1803,23 @@ class RevisionStore
$slots = $this->newRevisionSlots( $row->rev_id, $row, $queryFlags, $title );
return new RevisionStoreRecord( $title, $user, $comment, $row, $slots, $this->wikiId );
// If this is a cached row, instantiate a cache-aware revision class to avoid stale data.
if ( $fromCache ) {
$rev = new RevisionStoreCacheRecord(
function ( $revId ) use ( $queryFlags ) {
$db = $this->getDBConnectionRefForQueryFlags( $queryFlags );
return $this->fetchRevisionRowFromConds(
$db,
[ 'rev_id' => intval( $revId ) ]
);
},
$title, $user, $comment, $row, $slots, $this->wikiId
);
} else {
$rev = new RevisionStoreRecord(
$title, $user, $comment, $row, $slots, $this->wikiId );
}
return $rev;
}
/**
@ -2773,27 +2795,29 @@ class RevisionStore
return false;
}
// Load the row from cache if possible. If not possible, populate the cache.
// As a minor optimization, remember if this was a cache hit or miss.
// We can sometimes avoid a database query later if this is a cache miss.
$fromCache = true;
$row = $this->cache->getWithSetCallback(
// Page/rev IDs passed in from DB to reflect history merges
$this->getRevisionRowCacheKey( $db, $pageId, $revId ),
WANObjectCache::TTL_WEEK,
function ( $curValue, &$ttl, array &$setOpts ) use ( $db, $pageId, $revId ) {
function ( $curValue, &$ttl, array &$setOpts ) use (
$db, $pageId, $revId, &$fromCache
) {
$setOpts += Database::getCacheSetOptions( $db );
$conds = [
'rev_page' => intval( $pageId ),
'page_id' => intval( $pageId ),
'rev_id' => intval( $revId ),
];
$row = $this->fetchRevisionRowFromConds( $db, $conds );
return $row ?: false; // don't cache negatives
$row = $this->fetchRevisionRowFromConds( $db, [ 'rev_id' => intval( $revId ) ] );
if ( $row ) {
$fromCache = false;
}
return $row; // don't cache negatives
}
);
// Reflect revision deletion and user renames
// Reflect revision deletion and user renames.
if ( $row ) {
return $this->newRevisionFromRow( $row, 0, $title );
return $this->newRevisionFromRow( $row, 0, $title, $fromCache );
} else {
return false;
}

137
includes/Revision/RevisionStoreCacheRecord.php Normal file
View file

@ -0,0 +1,137 @@
<?php
/**
* A RevisionStoreRecord loaded from the cache.
*
* 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\Revision;
use MediaWiki\User\UserIdentity;
use MediaWiki\User\UserIdentityValue;
use CommentStoreComment;
use InvalidArgumentException;
use Title;
use User;
/**
* A cached RevisionStoreRecord. Ensures that changes performed "behind the back"
* of the cache do not cause the revision record to deliver stale data.
*
* @since 1.33
*/
class RevisionStoreCacheRecord extends RevisionStoreRecord {
/**
* @var callable
*/
private $mCallback;
/**
* @note Avoid calling this constructor directly. Use the appropriate methods
* in RevisionStore instead.
*
* @param callable $callback Callback for loading data. Signature: function ( $id ): object
* @param Title $title The title of the page this Revision is associated with.
* @param UserIdentity $user
* @param CommentStoreComment $comment
* @param object $row A row from the revision table. Use RevisionStore::getQueryInfo() to build
* a query that yields the required fields.
* @param RevisionSlots $slots The slots of this revision.
* @param bool|string $wikiId the wiki ID of the site this Revision belongs to,
* or false for the local site.
*/
function __construct(
$callback,
Title $title,
UserIdentity $user,
CommentStoreComment $comment,
$row,
RevisionSlots $slots,
$wikiId = false
) {
parent::__construct( $title, $user, $comment, $row, $slots, $wikiId );
$this->mCallback = $callback;
}
/**
* Overridden to ensure that we return a fresh value and not a cached one.
*
* @return int
*/
public function getVisibility() {
if ( $this->mCallback ) {
$this->loadFreshRow();
}
return parent::getVisibility();
}
/**
* Overridden to ensure that we return a fresh value and not a cached one.
*
* @param int $audience
* @param User|null $user
*
* @return UserIdentity The identity of the revision author, null if access is forbidden.
*/
public function getUser( $audience = self::FOR_PUBLIC, User $user = null ) {
if ( $this->mCallback ) {
$this->loadFreshRow();
}
return parent::getUser( $audience, $user );
}
/**
* Load a fresh row from the database to ensure we return updated information
* @throws RevisionAccessException if the row could not be loaded
*/
private function loadFreshRow() {
$freshRow = call_user_func( $this->mCallback, $this->mId );
// Set to null to ensure we do not make unnecessary queries for subsequent getter calls,
// and to allow the closure to be freed.
$this->mCallback = null;
if ( $freshRow ) {
$this->mDeleted = intval( $freshRow->rev_deleted );
try {
$this->mUser = User::newFromAnyId(
$freshRow->rev_user ?? null,
$freshRow->rev_user_text ?? null,
$freshRow->rev_actor ?? null
);
} catch ( InvalidArgumentException $ex ) {
wfWarn(
__METHOD__
. ': '
. $this->mTitle->getPrefixedDBkey()
. ': '
. $ex->getMessage()
);
$this->mUser = new UserIdentityValue( 0, 'Unknown user', 0 );
}
} else {
throw new RevisionAccessException(
'Unable to load fresh row for rev_id: ' . $this->mId
);
}
}
}

1
tests/common/TestsAutoLoader.php
View file

@ -170,6 +170,7 @@ $wgAutoloadClasses += [
'MediaWiki\Tests\Revision\RevisionRecordTests' => "$testDir/phpunit/includes/Revision/RevisionRecordTests.php",
'MediaWiki\Tests\Revision\RevisionStoreDbTestBase' => "$testDir/phpunit/includes/Revision/RevisionStoreDbTestBase.php",
'MediaWiki\Tests\Revision\PreMcrSchemaOverride' => "$testDir/phpunit/includes/Revision/PreMcrSchemaOverride.php",
'MediaWiki\Tests\Revision\RevisionStoreRecordTest' => "$testDir/phpunit/includes/Revision/RevisionStoreRecordTest.php",
# tests/phpunit/languages
'LanguageClassesTestCase' => "$testDir/phpunit/languages/LanguageClassesTestCase.php",

89
tests/phpunit/includes/Revision/RevisionStoreCacheRecordTest.php Normal file
View file

@ -0,0 +1,89 @@
<?php
namespace MediaWiki\Tests\Revision;
use Title;
use MediaWiki\User\UserIdentityValue;
use TextContent;
use CommentStoreComment;
use MediaWiki\Revision\RevisionStoreCacheRecord;
use MediaWiki\Revision\RevisionSlots;
use MediaWiki\Revision\SlotRecord;
use MediaWiki\Revision\RevisionRecord;
use MediaWiki\Revision\RevisionStoreRecord;
/**
* @covers \MediaWiki\Revision\RevisionStoreCacheRecord
* @covers \MediaWiki\Revision\RevisionStoreRecord
* @covers \MediaWiki\Revision\RevisionRecord
*/
class RevisionStoreCacheRecordTest extends RevisionStoreRecordTest {
/**
* @param array $rowOverrides
* @param bool|callable callback function to use, or false for a default no-op callback
*
* @return RevisionStoreRecord
*/
protected function newRevision( array $rowOverrides = [], $callback = false ) {
$title = Title::newFromText( 'Dummy' );
$title->resetArticleID( 17 );
$user = new UserIdentityValue( 11, 'Tester', 0 );
$comment = CommentStoreComment::newUnsavedComment( 'Hello World' );
$main = SlotRecord::newUnsaved( SlotRecord::MAIN, new TextContent( 'Lorem Ipsum' ) );
$aux = SlotRecord::newUnsaved( 'aux', new TextContent( 'Frumious Bandersnatch' ) );
$slots = new RevisionSlots( [ $main, $aux ] );
$row = [
'rev_id' => '7',
'rev_page' => strval( $title->getArticleID() ),
'rev_timestamp' => '20200101000000',
'rev_deleted' => 0,
'rev_minor_edit' => 0,
'rev_parent_id' => '5',
'rev_len' => $slots->computeSize(),
'rev_sha1' => $slots->computeSha1(),
'rev_user' => '11',
'page_latest' => '18',
];
$row = array_merge( $row, $rowOverrides );
if ( !$callback ) {
$callback = function ( $revId ) use ( $row ) {
return (object)$row;
};
}
return new RevisionStoreCacheRecord(
$callback,
$title,
$user,
$comment,
(object)$row,
$slots
);
}
public function testCallback() {
// Provide a callback that returns non-default values. Asserting the revision returns
// these values confirms callback execution and behavior. Also confirm the callback
// is only invoked once, even for multiple getter calls.
$rowOverrides = [
'rev_deleted' => RevisionRecord::DELETED_TEXT,
'rev_user' => 12,
];
$callbackInvoked = 0;
$callback = function ( $revId ) use ( &$callbackInvoked, $rowOverrides ) {
$callbackInvoked++;
return (object)$rowOverrides;
};
$rev = $this->newRevision( [], $callback );
$this->assertSame( RevisionRecord::DELETED_TEXT, $rev->getVisibility() );
$this->assertSame( 12, $rev->getUser()->getId() );
$this->assertSame( 1, $callbackInvoked );
}
}

192
tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php
View file

@ -14,6 +14,7 @@ use MediaWiki\Revision\IncompleteRevisionException;
use MediaWiki\Revision\MutableRevisionRecord;
use MediaWiki\Revision\RevisionArchiveRecord;
use MediaWiki\Revision\RevisionRecord;
use MediaWiki\Revision\RevisionStoreRecord;
use MediaWiki\Revision\RevisionSlots;
use MediaWiki\Revision\RevisionStore;
use MediaWiki\Revision\SlotRecord;
@ -1705,4 +1706,195 @@ abstract class RevisionStoreDbTestBase extends MediaWikiTestCase {
$this->testNewMutableRevisionFromArray( $array );
}
/**
* Creates a new revision for testing caching behavior
*
* @param WikiPage $page the page for the new revision
* @param RevisionStore $store store object to use for creating the revision
* @return bool|RevisionStoreRecord the revision created, or false if missing
*/
private function createRevisionStoreCacheRecord( $page, $store ) {
$user = MediaWikiTestCase::getMutableTestUser()->getUser();
$updater = $page->newPageUpdater( $user );
$updater->setContent( SlotRecord::MAIN, new WikitextContent( __METHOD__ ) );
$summary = CommentStoreComment::newUnsavedComment( __METHOD__ );
$rev = $updater->saveRevision( $summary, EDIT_NEW );
return $store->getKnownCurrentRevision( $page->getTitle(), $rev->getId() );
}
/**
* @covers \MediaWiki\Revision\RevisionStore::getKnownCurrentRevision
*/
public function testGetKnownCurrentRevision_userNameChange() {
global $wgActorTableSchemaMigrationStage;
$this->overrideMwServices();
$cache = new WANObjectCache( [ 'cache' => new HashBagOStuff() ] );
$this->setService( 'MainWANObjectCache', $cache );
$store = MediaWikiServices::getInstance()->getRevisionStore();
$page = $this->getNonexistingTestPage();
$rev = $this->createRevisionStoreCacheRecord( $page, $store );
// Grab the user name
$userNameBefore = $rev->getUser()->getName();
// Change the user name in the database, "behind the back" of the cache
$newUserName = "Renamed $userNameBefore";
$this->db->update( 'revision',
[ 'rev_user_text' => $newUserName ],
[ 'rev_id' => $rev->getId() ] );
$this->db->update( 'user',
[ 'user_name' => $newUserName ],
[ 'user_id' => $rev->getUser()->getId() ] );
if ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_WRITE_NEW ) {
$this->db->update( 'actor',
[ 'actor_name' => $newUserName ],
[ 'actor_user' => $rev->getUser()->getId() ] );
}
// Reload the revision and regrab the user name.
$revAfter = $store->getKnownCurrentRevision( $page->getTitle(), $rev->getId() );
$userNameAfter = $revAfter->getUser()->getName();
// The two user names should be different.
// If they are the same, we are seeing a cached value, which is bad.
$this->assertNotSame( $userNameBefore, $userNameAfter );
// This is implied by the above assertion, but explicitly check it, for completeness
$this->assertSame( $newUserName, $userNameAfter );
}
/**
* @covers \MediaWiki\Revision\RevisionStore::getKnownCurrentRevision
*/
public function testGetKnownCurrentRevision_revDelete() {
$this->overrideMwServices();
$cache = new WANObjectCache( [ 'cache' => new HashBagOStuff() ] );
$this->setService( 'MainWANObjectCache', $cache );
$store = MediaWikiServices::getInstance()->getRevisionStore();
$page = $this->getNonexistingTestPage();
$rev = $this->createRevisionStoreCacheRecord( $page, $store );
// Grab the deleted bitmask
$deletedBefore = $rev->getVisibility();
// Change the deleted bitmask in the database, "behind the back" of the cache
$this->db->update( 'revision',
[ 'rev_deleted' => RevisionRecord::DELETED_TEXT ],
[ 'rev_id' => $rev->getId() ] );
// Reload the revision and regrab the visibility flag.
$revAfter = $store->getKnownCurrentRevision( $page->getTitle(), $rev->getId() );
$deletedAfter = $revAfter->getVisibility();
// The two deleted flags should be different.
// If they are the same, we are seeing a cached value, which is bad.
$this->assertNotSame( $deletedBefore, $deletedAfter );
// This is implied by the above assertion, but explicitly check it, for completeness
$this->assertSame( RevisionRecord::DELETED_TEXT, $deletedAfter );
}
/**
* @covers \MediaWiki\Revision\RevisionStore::newRevisionFromRow
*/
public function testNewRevisionFromRow_userNameChange() {
global $wgActorTableSchemaMigrationStage;
$page = $this->getTestPage();
$text = __METHOD__;
/** @var Revision $rev */
$rev = $page->doEditContent(
new WikitextContent( $text ),
__METHOD__
)->value['revision'];
$store = MediaWikiServices::getInstance()->getRevisionStore();
$record = $store->newRevisionFromRow(
$this->revisionToRow( $rev ),
[],
$page->getTitle()
);
// Grab the user name
$userNameBefore = $record->getUser()->getName();
// Change the user name in the database
$newUserName = "Renamed $userNameBefore";
$this->db->update( 'revision',
[ 'rev_user_text' => $newUserName ],
[ 'rev_id' => $record->getId() ] );
$this->db->update( 'user',
[ 'user_name' => $newUserName ],
[ 'user_id' => $record->getUser()->getId() ] );
if ( $wgActorTableSchemaMigrationStage & SCHEMA_COMPAT_WRITE_NEW ) {
$this->db->update( 'actor',
[ 'actor_name' => $newUserName ],
[ 'actor_user' => $record->getUser()->getId() ] );
}
// Reload the record, passing $fromCache as true to force fresh info from the db,
// and regrab the user name
$recordAfter = $store->newRevisionFromRow(
$this->revisionToRow( $rev ),
[],
$page->getTitle(),
true
);
$userNameAfter = $recordAfter->getUser()->getName();
// The two user names should be different.
// If they are the same, we are seeing a cached value, which is bad.
$this->assertNotSame( $userNameBefore, $userNameAfter );
// This is implied by the above assertion, but explicitly check it, for completeness
$this->assertSame( $newUserName, $userNameAfter );
}
/**
* @covers \MediaWiki\Revision\RevisionStore::newRevisionFromRow
*/
public function testNewRevisionFromRow_revDelete() {
$page = $this->getTestPage();
$text = __METHOD__;
/** @var Revision $rev */
$rev = $page->doEditContent(
new WikitextContent( $text ),
__METHOD__
)->value['revision'];
$store = MediaWikiServices::getInstance()->getRevisionStore();
$record = $store->newRevisionFromRow(
$this->revisionToRow( $rev ),
[],
$page->getTitle()
);
// Grab the deleted bitmask
$deletedBefore = $record->getVisibility();
// Change the deleted bitmask in the database
$this->db->update( 'revision',
[ 'rev_deleted' => RevisionRecord::DELETED_TEXT ],
[ 'rev_id' => $record->getId() ] );
// Reload the record, passing $fromCache as true to force fresh info from the db,
// and regrab the deleted bitmask
$recordAfter = $store->newRevisionFromRow(
$this->revisionToRow( $rev ),
[],
$page->getTitle(),
true
);
$deletedAfter = $recordAfter->getVisibility();
// The two deleted flags should be different, because we modified the database.
$this->assertNotSame( $deletedBefore, $deletedAfter );
// This is implied by the above assertion, but explicitly check it, for completeness
$this->assertSame( RevisionRecord::DELETED_TEXT, $deletedAfter );
}
}

3
tests/phpunit/includes/Revision/RevisionStoreTest.php
View file

@ -21,6 +21,9 @@ use Wikimedia\Rdbms\LoadBalancer;
use Wikimedia\TestingAccessWrapper;
use WikitextContent;
/**
* Tests RevisionStore
*/
class RevisionStoreTest extends MediaWikiTestCase {
private function useTextId() {