Move category membership RC updates to CategoryMembershipChangeJob
authorAaron Schulz <aschulz@wikimedia.org>
Sun, 15 Nov 2015 02:29:37 +0000 (18:29 -0800)
committerAddshore <addshorewiki@gmail.com>
Thu, 3 Dec 2015 11:28:05 +0000 (11:28 +0000)
* 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
includes/DefaultSettings.php
includes/changes/CategoryMembershipChange.php
includes/deferred/LinksUpdate.php
includes/jobqueue/jobs/CategoryMembershipChangeJob.php [new file with mode: 0644]
includes/page/WikiPage.php
includes/specials/SpecialUndelete.php
tests/phpunit/includes/deferred/LinksUpdateTest.php

index c37cbaa..3e5c1b5 100644 (file)
@@ -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',
index 3d859a9..edc54f2 100644 (file)
@@ -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'
 );
index b4086f9..af6f9d9 100644 (file)
@@ -178,6 +178,7 @@ class CategoryMembershipChange {
                        }
                }
 
+               /** @var RecentChange $rc */
                $rc = call_user_func_array(
                        $this->newForCategorizationCallback,
                        array(
index c253e74..f9d7e9c 100644 (file)
@@ -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 (file)
index 0000000..4487970
--- /dev/null
@@ -0,0 +1,225 @@
+<?php
+/**
+ * Updater for link tracking tables after a page edit.
+ *
+ * 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
+ */
+
+/**
+ * Job to add recent change entries mentioning category membership changes
+ *
+ * Parameters include:
+ *   - pageId : page ID
+ *   - revTimestamp : timestamp of the triggering revision
+ *
+ * Category changes will be mentioned for revisions at/after the timestamp for this page
+ *
+ * @since 1.27
+ */
+class CategoryMembershipChangeJob extends Job {
+       const ENQUEUE_FUDGE_SEC = 60;
+
+       public function __construct( Title $title, array $params ) {
+               parent::__construct( 'categoryMembershipChange', $title, $params );
+               // Only need one job per page. Note that ENQUEUE_FUDGE_SEC handles races where an
+               // older revision job gets inserted while the newer revision job is de-duplicated.
+               $this->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;
+       }
+}
index 98bca05..5a6511c 100644 (file)
@@ -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'] ) );
index 664205a..492db26 100644 (file)
@@ -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
+                               )
                        );
                }
 
index 25ee5ec..c8b4bda 100644 (file)
@@ -1,6 +1,7 @@
 <?php
 
 /**
+ * @group LinksUpdate
  * @group Database
  * ^--- make sure temporary tables are used.
  */
@@ -147,6 +148,7 @@ class LinksUpdateTest extends MediaWikiTestCase {
                $title = Title::newFromText( 'Testing' );
                $wikiPage = new WikiPage( $title );
                $wikiPage->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 );
+               }
+       }
 }