From 6dedffc2d7f8a43f563d85db4110984818be7444 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Sat, 14 Nov 2015 18:29:37 -0800 Subject: [PATCH] Move category membership RC updates to CategoryMembershipChangeJob * Recursive link updates no longer mention an category changes. It's hard to avoid either duplicate mentioning of changes or confusing explicit and automatic category changes. * LinksUpdate no longer handles this logic, but rather WikiPage decides to spawn this update when needed in doEditUpdates(). * Fix race conditions with calculating category deltas. Do not rely on the link tables for the read used to determine these writes, as they may be out-of-date due to slave lag. Using the master would still not be good enough since that would assume FIFO and serialized job execution, which is not garaunteed. Use the parser output of the relevant revisions to determine the RC rows. If 3 users quickly edit a page's categories, the old way could misattribute who actually changed what. * Make sure RC rows are inserted in an order that matches that of the corresponding revisions. * Better avoid mentioning time-based (parser functions) category changes so they don't get attributed to the next editor. * Also wait for slaves between RC row insertions if there where many category changes (it theory it could well over 10K rows). * Using a separate job better separates concerns as LinksUpdate should not have to care about recent changes updates. * Added more docs to $wgRCWatchCategoryMembership. Bug: T95501 Change-Id: I5863e7d7483a4fd1fa633597af66a0088ace4c68 --- autoload.php | 1 + includes/DefaultSettings.php | 10 +- includes/changes/CategoryMembershipChange.php | 1 + includes/deferred/LinksUpdate.php | 28 --- .../jobs/CategoryMembershipChangeJob.php | 225 ++++++++++++++++++ includes/page/WikiPage.php | 34 ++- includes/specials/SpecialUndelete.php | 9 +- .../includes/deferred/LinksUpdateTest.php | 43 +++- 8 files changed, 305 insertions(+), 46 deletions(-) create mode 100644 includes/jobqueue/jobs/CategoryMembershipChangeJob.php diff --git a/autoload.php b/autoload.php index c37cbaa5a4..3e5c1b50a5 100644 --- a/autoload.php +++ b/autoload.php @@ -192,6 +192,7 @@ $wgAutoloadLocalClasses = array( 'Category' => __DIR__ . '/includes/Category.php', 'CategoryFinder' => __DIR__ . '/includes/CategoryFinder.php', 'CategoryMembershipChange' => __DIR__ . '/includes/changes/CategoryMembershipChange.php', + 'CategoryMembershipChangeJob' => __DIR__ . '/includes/jobqueue/jobs/CategoryMembershipChangeJob.php', 'CategoryPage' => __DIR__ . '/includes/page/CategoryPage.php', 'CategoryPager' => __DIR__ . '/includes/specials/SpecialCategories.php', 'CategoryViewer' => __DIR__ . '/includes/CategoryViewer.php', diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index 3d859a9e50..edc54f2722 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -6194,7 +6194,14 @@ $wgRCEngines = array( ); /** - * Treat category membership changes as a RecentChange + * Treat category membership changes as a RecentChange. + * Changes are mentioned in RC for page actions as follows: + * - creation: pages created with categories are mentioned + * - edit: category additions/removals to existing pages are mentioned + * - move: nothing is mentioned (unless templates used depend on the title) + * - deletion: nothing is mentioned + * - undeletion: nothing is mentioned + * * @since 1.27 */ $wgRCWatchCategoryMembership = false; @@ -6750,6 +6757,7 @@ $wgJobClasses = array( 'refreshLinksPrioritized' => 'RefreshLinksJob', // for cascading protection 'refreshLinksDynamic' => 'RefreshLinksJob', // for pages with dynamic content 'activityUpdateJob' => 'ActivityUpdateJob', + 'categoryMembershipChange' => 'CategoryMembershipChangeJob', 'enqueue' => 'EnqueueJob', // local queue for multi-DC setups 'null' => 'NullJob' ); diff --git a/includes/changes/CategoryMembershipChange.php b/includes/changes/CategoryMembershipChange.php index b4086f9be6..af6f9d920f 100644 --- a/includes/changes/CategoryMembershipChange.php +++ b/includes/changes/CategoryMembershipChange.php @@ -178,6 +178,7 @@ class CategoryMembershipChange { } } + /** @var RecentChange $rc */ $rc = call_user_func_array( $this->newForCategorizationCallback, array( diff --git a/includes/deferred/LinksUpdate.php b/includes/deferred/LinksUpdate.php index c253e74403..f9d7e9c152 100644 --- a/includes/deferred/LinksUpdate.php +++ b/includes/deferred/LinksUpdate.php @@ -153,8 +153,6 @@ class LinksUpdate extends SqlDataUpdate implements EnqueueableDataUpdate { } protected function doIncrementalUpdate() { - global $wgRCWatchCategoryMembership; - # Page links $existing = $this->getExistingLinks(); $this->linkDeletions = $this->getLinkDeletions( $existing ); @@ -206,14 +204,6 @@ class LinksUpdate extends SqlDataUpdate implements EnqueueableDataUpdate { $this->invalidateCategories( $categoryUpdates ); $this->updateCategoryCounts( $categoryInserts, $categoryDeletes ); - # Category membership changes - if ( - $wgRCWatchCategoryMembership && - !$this->mTriggeredRecursive && ( $categoryInserts || $categoryDeletes ) - ) { - $this->triggerCategoryChanges( $categoryInserts, $categoryDeletes ); - } - # Page properties $existing = $this->getExistingProperties(); @@ -237,24 +227,6 @@ class LinksUpdate extends SqlDataUpdate implements EnqueueableDataUpdate { } - private function triggerCategoryChanges( $categoryInserts, $categoryDeletes ) { - $catMembChange = new CategoryMembershipChange( $this->mTitle, $this->mRevision ); - - if ( $this->mRecursive ) { - $catMembChange->checkTemplateLinks(); - } - - foreach ( $categoryInserts as $categoryName => $value ) { - $categoryTitle = Title::newFromText( $categoryName, NS_CATEGORY ); - $catMembChange->triggerCategoryAddedNotification( $categoryTitle ); - } - - foreach ( $categoryDeletes as $categoryName => $value ) { - $categoryTitle = Title::newFromText( $categoryName, NS_CATEGORY ); - $catMembChange->triggerCategoryRemovedNotification( $categoryTitle ); - } - } - /** * Queue recursive jobs for this page * diff --git a/includes/jobqueue/jobs/CategoryMembershipChangeJob.php b/includes/jobqueue/jobs/CategoryMembershipChangeJob.php new file mode 100644 index 0000000000..4487970049 --- /dev/null +++ b/includes/jobqueue/jobs/CategoryMembershipChangeJob.php @@ -0,0 +1,225 @@ +removeDuplicates = true; + } + + public function run() { + $page = WikiPage::newFromID( $this->params['pageId'], WikiPage::READ_LATEST ); + if ( !$page ) { + $this->setLastError( "Could not find page #{$this->params['pageId']}" ); + return false; // deleted? + } + + $dbw = wfGetDB( DB_MASTER ); + + // Use a named lock so that jobs for this page see each others' changes + $fname = __METHOD__; + $lockKey = "CategoryMembershipUpdates:{$page->getId()}"; + if ( !$dbw->lock( $lockKey, $fname, 10 ) ) { + $this->setLastError( "Could not acquire lock '$lockKey'" ); + return false; + } + + $unlocker = new ScopedCallback( function () use ( $dbw, $lockKey, $fname ) { + $dbw->unlock( $lockKey, $fname ); + } ); + + // Sanity: clear any DB transaction snapshot + $dbw->commit( __METHOD__, 'flush' ); + + $cutoffUnix = wfTimestamp( TS_UNIX, $this->params['revTimestamp'] ); + // Using ENQUEUE_FUDGE_SEC handles jobs inserted out of revision order due to the delay + // between COMMIT and actual enqueueing of the CategoryMembershipChangeJob job. + $cutoffUnix -= self::ENQUEUE_FUDGE_SEC; + + // Get the newest revision that has a SRC_CATEGORIZE row... + $row = $dbw->selectRow( + array( 'revision', 'recentchanges' ), + array( 'rev_timestamp', 'rev_id' ), + array( + 'rev_page' => $page->getId(), + 'rev_timestamp >= ' . $dbw->addQuotes( $dbw->timestamp( $cutoffUnix ) ) + ), + __METHOD__, + array( 'ORDER BY' => 'rev_timestamp DESC, rev_id DESC' ), + array( + 'recentchanges' => array( + 'INNER JOIN', + array( + 'rc_this_oldid = rev_id', + 'rc_source' => RecentChange::SRC_CATEGORIZE, + // Allow rc_cur_id or rc_timestamp index usage + 'rc_cur_id = rev_page', + 'rc_timestamp >= rev_timestamp' + ) + ) + ) + ); + // Only consider revisions newer than any such revision + if ( $row ) { + $cutoffUnix = wfTimestamp( TS_UNIX, $row->rev_timestamp ); + $lastRevId = (int)$row->rev_id; + } else { + $lastRevId = 0; + } + + // Find revisions to this page made around and after this revision which lack category + // notifications in recent changes. This lets jobs pick up were the last one left off. + $encCutoff = $dbw->addQuotes( $dbw->timestamp( $cutoffUnix ) ); + $res = $dbw->select( + 'revision', + Revision::selectFields(), + array( + 'rev_page' => $page->getId(), + "rev_timestamp > $encCutoff" . + " OR (rev_timestamp = $encCutoff AND rev_id > $lastRevId)" + ), + __METHOD__, + array( 'ORDER BY' => 'rev_timestamp ASC, rev_id ASC' ) + ); + + // Apply all category updates in revision timestamp order + foreach ( $res as $row ) { + $this->notifyUpdatesForRevision( $page, Revision::newFromRow( $row ) ); + } + + ScopedCallback::consume( $unlocker ); + + return true; + } + + /** + * @param WikiPage $page + * @param Revision $newRev + * @throws MWException + */ + protected function notifyUpdatesForRevision( WikiPage $page, Revision $newRev ) { + $config = RequestContext::getMain()->getConfig(); + $title = $page->getTitle(); + + // Get the new revision + if ( !$newRev->getContent() ) { + return; // deleted? + } + + // Get the prior revision (the same for null edits) + if ( $newRev->getParentId() ) { + $oldRev = Revision::newFromId( $newRev->getParentId(), Revision::READ_LATEST ); + if ( !$oldRev->getContent() ) { + return; // deleted? + } + } else { + $oldRev = null; + } + + // Parse the new revision and get the categories + $categoryChanges = $this->getExplicitCategoriesChanges( $title, $newRev, $oldRev ); + list( $categoryInserts, $categoryDeletes ) = $categoryChanges; + if ( !$categoryInserts && !$categoryDeletes ) { + return; // nothing to do + } + + $dbw = wfGetDB( DB_MASTER ); + $catMembChange = new CategoryMembershipChange( $title, $newRev ); + $catMembChange->checkTemplateLinks(); + + $batchSize = $config->get( 'UpdateRowsPerQuery' ); + $insertCount = 0; + + foreach ( $categoryInserts as $categoryName ) { + $categoryTitle = Title::newFromText( $categoryName, NS_CATEGORY ); + $catMembChange->triggerCategoryAddedNotification( $categoryTitle ); + if ( $insertCount++ && ( $insertCount % $batchSize ) == 0 ) { + $dbw->commit( __METHOD__, 'flush' ); + wfWaitForSlaves(); + } + } + + foreach ( $categoryDeletes as $categoryName ) { + $categoryTitle = Title::newFromText( $categoryName, NS_CATEGORY ); + $catMembChange->triggerCategoryRemovedNotification( $categoryTitle ); + if ( $insertCount++ && ( $insertCount++ % $batchSize ) == 0 ) { + $dbw->commit( __METHOD__, 'flush' ); + wfWaitForSlaves(); + } + } + } + + private function getExplicitCategoriesChanges( + Title $title, Revision $newRev, Revision $oldRev = null + ) { + // Inject the same timestamp for both revision parses to avoid seeing category changes + // due to time-based parser functions. Inject the same page title for the parses too. + // Note that REPEATABLE-READ makes template/file pages appear unchanged between parses. + $parseTimestamp = $newRev->getTimestamp(); + // Parse the old rev and get the categories. Do not use link tables as that + // assumes these updates are perfectly FIFO and that link tables are always + // up to date, neither of which are true. + $oldCategories = $oldRev + ? $this->getCategoriesAtRev( $title, $oldRev, $parseTimestamp ) + : array(); + // Parse the new revision and get the categories + $newCategories = $this->getCategoriesAtRev( $title, $newRev, $parseTimestamp ); + + $categoryInserts = array_values( array_diff( $newCategories, $oldCategories ) ); + $categoryDeletes = array_values( array_diff( $oldCategories, $newCategories ) ); + + return array( $categoryInserts, $categoryDeletes ); + } + + private function getCategoriesAtRev( Title $title, Revision $rev, $parseTimestamp ) { + $content = $rev->getContent(); + $options = $content->getContentHandler()->makeParserOptions( 'canonical' ); + $options->setTimestamp( $parseTimestamp ); + // This could possibly use the parser cache if it checked the revision ID, + // but that's more complicated than it's worth. + $output = $content->getParserOutput( $title, $rev->getId(), $options ); + + return array_keys( $output->getCategories() ); + } + + public function getDeduplicationInfo() { + $info = parent::getDeduplicationInfo(); + unset( $info['params']['revTimestamp'] ); // first job wins + + return $info; + } +} diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php index 98bca0525b..5a6511ca8a 100644 --- a/includes/page/WikiPage.php +++ b/includes/page/WikiPage.php @@ -1742,6 +1742,7 @@ class WikiPage implements Page, IDBAccessObject { $isminor = ( $flags & EDIT_MINOR ) && $user->isAllowed( 'minoredit' ); $bot = $flags & EDIT_FORCE_BOT; + $old_revision = $this->getRevision(); // current revision $old_content = $this->getContent( Revision::RAW ); // current revision's content $oldsize = $old_content ? $old_content->getSize() : 0; @@ -1869,7 +1870,8 @@ class WikiPage implements Page, IDBAccessObject { $user, array( 'changed' => $changed, - 'oldcountable' => $oldcountable + 'oldcountable' => $oldcountable, + 'oldrevision' => $old_revision ) ); @@ -1955,7 +1957,11 @@ class WikiPage implements Page, IDBAccessObject { $this->mTimestamp = $now; // Update links, etc. - $this->doEditUpdates( $revision, $user, array( 'created' => true ) ); + $this->doEditUpdates( + $revision, + $user, + array( 'created' => true, 'oldrevision' => $old_revision ) + ); $hook_args = array( &$this, &$user, $content, $summary, $flags & EDIT_MINOR, null, null, &$flags, $revision ); @@ -2156,6 +2162,8 @@ class WikiPage implements Page, IDBAccessObject { * - changed: boolean, whether the revision changed the content (default true) * - created: boolean, whether the revision created the page (default false) * - moved: boolean, whether the page was moved (default false) + * - restored: boolean, whether the page was undeleted (default false) + * - oldrevision: Revision object for the pre-update revision (default null) * - oldcountable: boolean, null, or string 'no-change' (default null): * - boolean: whether the page was counted as an article before that * revision, only used in changed is true and created is false @@ -2164,10 +2172,14 @@ class WikiPage implements Page, IDBAccessObject { * - 'no-change': don't update the article count, ever */ public function doEditUpdates( Revision $revision, User $user, array $options = array() ) { + global $wgRCWatchCategoryMembership; + $options += array( 'changed' => true, 'created' => false, 'moved' => false, + 'restored' => false, + 'oldrevision' => null, 'oldcountable' => null ); $content = $revision->getContent(); @@ -2194,7 +2206,8 @@ class WikiPage implements Page, IDBAccessObject { if ( $content ) { $recursive = $options['changed']; // bug 50785 $updates = $content->getSecondaryDataUpdates( - $this->getTitle(), null, $recursive, $editInfo->output ); + $this->getTitle(), null, $recursive, $editInfo->output + ); foreach ( $updates as $update ) { if ( $update instanceof LinksUpdate ) { $update->setRevision( $revision ); @@ -2202,6 +2215,21 @@ class WikiPage implements Page, IDBAccessObject { } DeferredUpdates::addUpdate( $update ); } + if ( $wgRCWatchCategoryMembership + && ( $options['changed'] || $options['created'] ) + && !$options['restored'] + ) { + // Note: jobs are pushed after deferred updates, so the job should be able to see + // the recent change entry (also done via deferred updates) and carry over any + // bot/deletion/IP flags, ect. + JobQueueGroup::singleton()->lazyPush( new CategoryMembershipChangeJob( + $this->getTitle(), + array( + 'pageId' => $this->getId(), + 'revTimestamp' => $revision->getTimestamp() + ) + ) ); + } } Hooks::run( 'ArticleEditUpdates', array( &$this, &$editInfo, $options['changed'] ) ); diff --git a/includes/specials/SpecialUndelete.php b/includes/specials/SpecialUndelete.php index 664205aa20..492db26df5 100644 --- a/includes/specials/SpecialUndelete.php +++ b/includes/specials/SpecialUndelete.php @@ -625,11 +625,14 @@ class PageArchive { $wasnew = $article->updateIfNewerOn( $dbw, $revision, $previousRevId ); if ( $created || $wasnew ) { // Update site stats, link tables, etc - $user = User::newFromName( $revision->getUserText( Revision::RAW ), false ); $article->doEditUpdates( $revision, - $user, - array( 'created' => $created, 'oldcountable' => $oldcountable ) + User::newFromName( $revision->getUserText( Revision::RAW ), false ), + array( + 'created' => $created, + 'oldcountable' => $oldcountable, + 'restored' => true + ) ); } diff --git a/tests/phpunit/includes/deferred/LinksUpdateTest.php b/tests/phpunit/includes/deferred/LinksUpdateTest.php index 25ee5ecaba..c8b4bdaac0 100644 --- a/tests/phpunit/includes/deferred/LinksUpdateTest.php +++ b/tests/phpunit/includes/deferred/LinksUpdateTest.php @@ -1,6 +1,7 @@ doEditContent( new WikitextContent( '[[Category:Foo]]' ), 'added category' ); + $this->runAllRelatedJobs(); $this->assertRecentChangeByCategorization( $title, @@ -155,7 +157,9 @@ class LinksUpdateTest extends MediaWikiTestCase { array( array( 'Foo', '[[:Testing]] added to category' ) ) ); - $wikiPage->doEditContent( new WikitextContent( '[[Category:Bar]]' ), 'added category' ); + $wikiPage->doEditContent( new WikitextContent( '[[Category:Bar]]' ), 'replaced category' ); + $this->runAllRelatedJobs(); + $this->assertRecentChangeByCategorization( $title, $wikiPage->getParserOutput( new ParserOptions() ), @@ -184,15 +188,27 @@ class LinksUpdateTest extends MediaWikiTestCase { $wikiPage = new WikiPage( Title::newFromText( 'Testing' ) ); $wikiPage->doEditContent( new WikitextContent( '{{TestingTemplate}}' ), 'added template' ); + $this->runAllRelatedJobs(); + $otherWikiPage = new WikiPage( Title::newFromText( 'Some_other_page' ) ); $otherWikiPage->doEditContent( new WikitextContent( '{{TestingTemplate}}' ), 'added template' ); - $templatePage->doEditContent( new WikitextContent( '[[Category:Foo]]' ), 'added category' ); + $this->runAllRelatedJobs(); + + $this->assertRecentChangeByCategorization( + $templateTitle, + $templatePage->getParserOutput( new ParserOptions() ), + Title::newFromText( 'Baz' ), + array() + ); + + $templatePage->doEditContent( new WikitextContent( '[[Category:Baz]]' ), 'added category' ); + $this->runAllRelatedJobs(); $this->assertRecentChangeByCategorization( $templateTitle, $templatePage->getParserOutput( new ParserOptions() ), - Title::newFromText( 'Foo' ), - array( array( 'Foo', '[[:Template:TestingTemplate]] and 2 pages added to category' ) ) + Title::newFromText( 'Baz' ), + array( array( 'Baz', '[[:Template:TestingTemplate]] and 2 pages added to category' ) ) ); } @@ -330,13 +346,6 @@ class LinksUpdateTest extends MediaWikiTestCase { protected function assertRecentChangeByCategorization( Title $pageTitle, ParserOutput $parserOutput, Title $categoryTitle, $expectedRows ) { - $update = new LinksUpdate( $pageTitle, $parserOutput ); - $revision = Revision::newFromTitle( $pageTitle ); - $update->setRevision( $revision ); - $update->beginTransaction(); - $update->doUpdate(); - $update->commitTransaction(); - $this->assertSelect( 'recentchanges', 'rc_title, rc_comment', @@ -348,4 +357,16 @@ class LinksUpdateTest extends MediaWikiTestCase { $expectedRows ); } + + private function runAllRelatedJobs() { + $queueGroup = JobQueueGroup::singleton(); + while ( $job = $queueGroup->pop( 'refreshLinksPrioritized' ) ) { + $job->run(); + $queueGroup->ack( $job ); + } + while ( $job = $queueGroup->pop( 'categoryMembershipChange' ) ) { + $job->run(); + $queueGroup->ack( $job ); + } + } } -- 2.20.1