jenkins-bot has submitted this change and it was merged. (
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/ChangeHandlerTest.php
M client/tests/phpunit/includes/Changes/MockPageUpdater.php
M repo/includes/Notifications/JobQueueChangeNotificationSender.php
5 files changed, 60 insertions(+), 47 deletions(-)
Approvals:
Aaron Schulz: Looks good to me, approved
jenkins-bot: Verified
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 8f6de54..f637c25 100644
--- a/client/includes/Changes/WikiPageUpdater.php
+++ b/client/includes/Changes/WikiPageUpdater.php
@@ -104,27 +104,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,
+ ];
}
/**
@@ -140,19 +150,16 @@
$jobs = [];
$titleBatches = array_chunk( $titles,
$this->purgeCacheBatchSize );
+ $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 )
);
}
@@ -178,12 +185,7 @@
$jobCount = count( $titles );
foreach ( $titles as $title ) {
- $job = new RefreshLinksJob(
- $title,
- $this->addRootJobParameters( [], $rootJobParams
)
- );
-
- $this->jobQueueGroup->lazyPush( $job );
+ $this->jobQueueGroup->lazyPush( new RefreshLinksJob(
$title, $rootJobParams ) );
}
$this->incrementStats( 'RefreshLinks.jobs', $jobCount );
@@ -198,12 +200,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/ChangeHandlerTest.php
b/client/tests/phpunit/includes/Changes/ChangeHandlerTest.php
index ababad6..e81bb88 100644
--- a/client/tests/phpunit/includes/Changes/ChangeHandlerTest.php
+++ b/client/tests/phpunit/includes/Changes/ChangeHandlerTest.php
@@ -522,7 +522,7 @@
101 => new PageEntityUsages( 101, [ new EntityUsage(
$q100, 'X' ) ] ),
];
- $titleBatchHash = '0f89093daf80eabc67b8369a6ca88d58f2abcc80';
+ $titleBatchHash = 'f0b873699a63c858667e54cd071f7d9209faeda1';
$strangeHash = '97c72edc416a2b659492401306e31c2dd8ffcc49';
$regularRootJobParams = [
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: merged
Gerrit-Change-Id: I7334a0ac204f25a0272feecb0c6d0581ed8035d0
Gerrit-PatchSet: 4
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Thiemo Mättig (WMDE) <thiemo.maet...@wikimedia.de>
Gerrit-Reviewer: Aaron Schulz <asch...@wikimedia.org>
Gerrit-Reviewer: Daniel Kinzler <daniel.kinz...@wikimedia.de>
Gerrit-Reviewer: Ladsgroup <ladsgr...@gmail.com>
Gerrit-Reviewer: Thiemo Mättig (WMDE) <thiemo.maet...@wikimedia.de>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
