Thiemo Mättig (WMDE) has uploaded a new change for review. ( https://gerrit.wikimedia.org/r/378719 )
Change subject: Refactor possibly fragile ChangeHandler/WikiPageUpdater hash calculations ...................................................................... Refactor possibly fragile ChangeHandler/WikiPageUpdater hash calculations This is a series of refactoring I did while reviewing I566d225. Bug: T173710 Change-Id: I7334a0ac204f25a0272feecb0c6d0581ed8035d0 --- M client/includes/Changes/ChangeHandler.php M client/includes/Changes/WikiPageUpdater.php M client/tests/phpunit/includes/Changes/MockPageUpdater.php M repo/includes/Notifications/JobQueueChangeNotificationSender.php 4 files changed, 64 insertions(+), 45 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase refs/changes/19/378719/1 diff --git a/client/includes/Changes/ChangeHandler.php b/client/includes/Changes/ChangeHandler.php index 4ce3e10..d14fb8a 100644 --- a/client/includes/Changes/ChangeHandler.php +++ b/client/includes/Changes/ChangeHandler.php @@ -6,12 +6,11 @@ use Hooks; use InvalidArgumentException; use LinkBatch; -use MWException; use SiteLookup; use Title; -use Traversable; use Wikibase\Change; use Wikibase\Client\Store\TitleFactory; +use Wikibase\Client\Usage\PageEntityUsages; use Wikibase\EntityChange; /** @@ -139,21 +138,16 @@ * @return string a signature based on the hash of the given titles */ private function getTitleBatchSignature( array $titles ) { - usort( $titles, function ( Title $a, Title $b ) { - return $a->getArticleID() - $b->getArticleID(); - } ); + $pages = []; - return 'title-batch:' . sha1( - join( - "\n", - array_map( - function( Title $title ) { - return $title->getPrefixedDBkey(); - }, - $titles - ) - ) - ); + /** @see WikiPageUpdater::getPageParamForRefreshLinksJob */ + foreach ( $titles as $title ) { + $id = $title->getArticleID(); + $pages[$id] = [ $title->getNamespace(), $title->getDBkey() ]; + } + + ksort( $pages ); + return 'title-batch:' . sha1( json_encode( $pages ) ); } /** @@ -180,7 +174,7 @@ } /** - * @param Traversable $usagesPerPage A sequence of PageEntityUsages objects + * @param PageEntityUsages[] $usagesPerPage * * @return Title[] */ diff --git a/client/includes/Changes/WikiPageUpdater.php b/client/includes/Changes/WikiPageUpdater.php index c6b83b5..6227e29 100644 --- a/client/includes/Changes/WikiPageUpdater.php +++ b/client/includes/Changes/WikiPageUpdater.php @@ -91,6 +91,10 @@ $this->dbBatchSize = $dbBatchSize; } + /** + * @param string $updateType + * @param int $delta + */ private function incrementStats( $updateType, $delta ) { if ( $this->stats ) { $this->stats->updateCount( 'wikibase.client.pageupdates.' . $updateType, $delta ); @@ -98,27 +102,37 @@ } /** - * @param array $params + * @param Title[] $titles * @param array $rootJobParams + * * @return array */ - private function addRootJobParameters( array $params, array $rootJobParams ) { - // See JobQueueChangeNotificationSender::getJobSpecification for relevant root job parameters. + private function buildJobParams( array $titles, array $rootJobParams ) { + $pages = $this->getPageParamForRefreshLinksJob( $titles ); + /** + * @see JobQueueChangeNotificationSender::getJobSpecification for relevant root job + * parameters. + */ if ( isset( $rootJobParams['rootJobSignature'] ) ) { - $params['rootJobSignature'] = $rootJobParams['rootJobSignature']; + $signature = $rootJobParams['rootJobSignature']; } else { - ksort( $params ); // apply canonical ordering before hashing - $params['rootJobSignature'] = 'params:' . sha1( json_encode( $params ) ); + // Apply canonical ordering before hashing + ksort( $pages ); + $signature = 'params:' . sha1( json_encode( $pages ) ); } if ( isset( $rootJobParams['rootJobTimestamp'] ) ) { - $params['rootJobTimestamp'] = $rootJobParams['rootJobTimestamp']; + $timestamp = $rootJobParams['rootJobTimestamp']; } else { - $params['rootJobTimestamp'] = wfTimestampNow(); + $timestamp = wfTimestampNow(); } - return $params; + return [ + 'pages' => $pages, + 'rootJobSignature' => $signature, + 'rootJobTimestamp' => $timestamp, + ]; } /** @@ -134,19 +148,16 @@ $jobs = []; $titleBatches = array_chunk( $titles, $this->dbBatchSize ); + $dummyTitle = Title::makeTitle( NS_SPECIAL, 'Badtitle/' . __CLASS__ ); /* @var Title[] $batch */ foreach ( $titleBatches as $batch ) { wfDebugLog( __CLASS__, __FUNCTION__ . ": scheduling HTMLCacheUpdateJob for " . count( $batch ) . " titles" ); - $dummyTitle = Title::makeTitle( NS_SPECIAL, 'Badtitle/' . __CLASS__ ); - $jobs[] = new HTMLCacheUpdateJob( $dummyTitle, // the title will be ignored because the 'pages' parameter is set. - $this->addRootJobParameters( [ - 'pages' => $this->getPageParamForRefreshLinksJob( $batch ) - ], $rootJobParams ) + $this->buildJobParams( $batch, $rootJobParams ) ); } @@ -168,19 +179,16 @@ $jobs = []; $titleBatches = array_chunk( $titles, $this->dbBatchSize ); + $dummyTitle = Title::makeTitle( NS_SPECIAL, 'Badtitle/' . __CLASS__ ); /* @var Title[] $batch */ foreach ( $titleBatches as $batch ) { wfDebugLog( __CLASS__, __FUNCTION__ . ": scheduling refresh links for " . count( $batch ) . " titles" ); - $dummyTitle = Title::makeTitle( NS_SPECIAL, 'Badtitle/' . __CLASS__ ); - $jobs[] = new RefreshLinksJob( $dummyTitle, // the title will be ignored because the 'pages' parameter is set. - $this->addRootJobParameters( [ - 'pages' => $this->getPageParamForRefreshLinksJob( $batch ), - ], $rootJobParams ) + $this->buildJobParams( $batch, $rootJobParams ) ); } @@ -197,12 +205,10 @@ private function getPageParamForRefreshLinksJob( array $titles ) { $pages = []; - foreach ( $titles as $t ) { - $id = $t->getArticleID(); - $pages[$id] = [ - $t->getNamespace(), - $t->getDBkey() - ]; + /** @see ChangeHandler::getTitleBatchSignature */ + foreach ( $titles as $title ) { + $id = $title->getArticleID(); + $pages[$id] = [ $title->getNamespace(), $title->getDBkey() ]; } return $pages; diff --git a/client/tests/phpunit/includes/Changes/MockPageUpdater.php b/client/tests/phpunit/includes/Changes/MockPageUpdater.php index 051331a..e6e792a 100644 --- a/client/tests/phpunit/includes/Changes/MockPageUpdater.php +++ b/client/tests/phpunit/includes/Changes/MockPageUpdater.php @@ -17,12 +17,18 @@ */ class MockPageUpdater implements PageUpdater { + /** + * @var array[] Collections of affected objects as provided to the individual methods + */ private $updates = [ 'purgeWebCache' => [], 'scheduleRefreshLinks' => [], 'injectRCRecord' => [], ]; + /** + * @var array[] Collections of root job parameters as provided to the individual methods + */ private $rootJobParams = [ 'purgeWebCache' => [], 'scheduleRefreshLinks' => [], @@ -69,10 +75,16 @@ $this->rootJobParams['injectRCRecord'] += $rootJobParams; } + /** + * @return array[] + */ public function getUpdates() { return $this->updates; } + /** + * @return array[] + */ public function getRootJobParams() { return $this->rootJobParams; } diff --git a/repo/includes/Notifications/JobQueueChangeNotificationSender.php b/repo/includes/Notifications/JobQueueChangeNotificationSender.php index 38b7aeb..a875270 100644 --- a/repo/includes/Notifications/JobQueueChangeNotificationSender.php +++ b/repo/includes/Notifications/JobQueueChangeNotificationSender.php @@ -86,6 +86,11 @@ ); } + /** + * @param Change[] $changes + * + * @return JobSpecification + */ private function getJobSpecification( array $changes ) { $changeIds = array_map( function ( Change $change ) { @@ -98,9 +103,11 @@ 'repo' => $this->repoDB, 'changeIds' => $changeIds, - // Set root job parameters for deduplication. - // Compare WikiPageUpdater::addRootJobParameters - // and InjectRCRecordsJob::makeJobSpecification. + /** + * Set root job parameters for deduplication. Compare + * @see WikiPageUpdater::buildJobParams and + * @see InjectRCRecordsJob::makeJobSpecification. + */ 'rootJobTimestamp' => wfTimestampNow(), ]; -- To view, visit https://gerrit.wikimedia.org/r/378719 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I7334a0ac204f25a0272feecb0c6d0581ed8035d0 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: Thiemo Mättig (WMDE) <thiemo.maet...@wikimedia.de> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits