From: Bill Pirkle Date: Wed, 27 Feb 2019 21:26:17 +0000 (-0600) Subject: Avoid using stale data for revision visibility and actor data X-Git-Tag: 1.34.0-rc.0~2563^2 X-Git-Url: http://git.cyclocoop.org/%22%20.%20generer_url_aide%28?a=commitdiff_plain;h=276bf4a76b7ca968012d9b14e9645468c9566c16;p=lhc%2Fweb%2Fwiklou.git Avoid using stale data for revision visibility and actor data 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 --- diff --git a/includes/Revision/RevisionStore.php b/includes/Revision/RevisionStore.php index cf19ffbc47..23189cc713 100644 --- a/includes/Revision/RevisionStore.php +++ b/includes/Revision/RevisionStore.php @@ -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; } diff --git a/includes/Revision/RevisionStoreCacheRecord.php b/includes/Revision/RevisionStoreCacheRecord.php new file mode 100644 index 0000000000..ef5f10e312 --- /dev/null +++ b/includes/Revision/RevisionStoreCacheRecord.php @@ -0,0 +1,137 @@ +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 + ); + } + } + +} diff --git a/tests/common/TestsAutoLoader.php b/tests/common/TestsAutoLoader.php index f742a1bae8..96bd6ae221 100644 --- a/tests/common/TestsAutoLoader.php +++ b/tests/common/TestsAutoLoader.php @@ -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", diff --git a/tests/phpunit/includes/Revision/RevisionStoreCacheRecordTest.php b/tests/phpunit/includes/Revision/RevisionStoreCacheRecordTest.php new file mode 100644 index 0000000000..8684cd392b --- /dev/null +++ b/tests/phpunit/includes/Revision/RevisionStoreCacheRecordTest.php @@ -0,0 +1,89 @@ +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 ); + } + +} diff --git a/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php b/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php index 018df48115..51c483d55d 100644 --- a/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php +++ b/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php @@ -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 ); + } + } diff --git a/tests/phpunit/includes/Revision/RevisionStoreTest.php b/tests/phpunit/includes/Revision/RevisionStoreTest.php index c4b274dd6a..1ddc2545cc 100644 --- a/tests/phpunit/includes/Revision/RevisionStoreTest.php +++ b/tests/phpunit/includes/Revision/RevisionStoreTest.php @@ -21,6 +21,9 @@ use Wikimedia\Rdbms\LoadBalancer; use Wikimedia\TestingAccessWrapper; use WikitextContent; +/** + * Tests RevisionStore + */ class RevisionStoreTest extends MediaWikiTestCase { private function useTextId() {