From 56685d61d58d4c0cec4295cc1a42b25e0d544d3f Mon Sep 17 00:00:00 2001
From: Lucas Werkmeister <lucas.werkmeister@wikimedia.de>
Date: Mon, 26 Oct 2020 14:08:44 +0100
Subject: [PATCH] Use LinkBatch in AffectedPagesFinder
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The AffectedPagesFinder needs the page ID for each affected title of a
sitelink/title change, so use the LinkBatch class to load them in bulk,
instead of potentially doing one database query per title later when we
call getArticleID().

In theory, it’s possible that this actually increases the number of
database queries. If all of the titles were already in the link cache,
then previously no database query would have been made; however,
LinkBatch always queries for titles even if they are already in the link
cache (T266494), so at least one database query is always made (plus
potentially one more for the gender cache, if applicable – though that
should be rare, since only user page titles need gender information and
those pages generally aren’t linked to items). However, I see no reason
to assume the pages would already be in the link cache at this point, so
I don’t expect this to actually increase the number of queries.

While we’re already touching the class and adding dependency injection
(for the link batch factory), also inject the logger and declare strict
types.

Bug: T266499
Change-Id: I73d6b87dbde36b9cc076ee45c34ed84223336761
---
 .../includes/Changes/AffectedPagesFinder.php  | 39 +++++++++++--------
 client/includes/WikibaseClient.php            |  4 +-
 .../Changes/AffectedPagesFinderTest.php       | 20 ++++++++++
 .../includes/Changes/ChangeHandlerTest.php    |  3 ++
 4 files changed, 48 insertions(+), 18 deletions(-)

diff --git a/client/includes/Changes/AffectedPagesFinder.php b/client/includes/Changes/AffectedPagesFinder.php
index b4bb68444f4..dc2dd8885ba 100644
--- a/client/includes/Changes/AffectedPagesFinder.php
+++ b/client/includes/Changes/AffectedPagesFinder.php
@@ -1,10 +1,14 @@
 <?php
 
+declare( strict_types = 1 );
+
 namespace Wikibase\Client\Changes;
 
 use ArrayIterator;
 use InvalidArgumentException;
+use MediaWiki\Cache\LinkBatchFactory;
 use Psr\Log\LoggerInterface;
+use Psr\Log\NullLogger;
 use Title;
 use TitleFactory;
 use Traversable;
@@ -13,7 +17,6 @@
 use Wikibase\Client\Usage\PageEntityUsages;
 use Wikibase\Client\Usage\UsageAspectTransformer;
 use Wikibase\Client\Usage\UsageLookup;
-use Wikibase\Client\WikibaseClient;
 use Wikibase\DataModel\Entity\EntityId;
 use Wikibase\Lib\Changes\Change;
 use Wikibase\Lib\Changes\EntityChange;
@@ -36,25 +39,30 @@ class AffectedPagesFinder {
 	 */
 	private $titleFactory;
 
+	/** @var LinkBatchFactory */
+	private $linkBatchFactory;
+
 	/**
 	 * @var string
 	 */
 	private $siteId;
 
 	/**
-	 * @var bool
+	 * @var LoggerInterface
 	 */
-	private $checkPageExistence;
+	private $logger;
 
 	/**
-	 * @var LoggerInterface
+	 * @var bool
 	 */
-	private $logger;
+	private $checkPageExistence;
 
 	/**
 	 * @param UsageLookup $usageLookup
 	 * @param TitleFactory $titleFactory
+	 * @param LinkBatchFactory $linkBatchFactory
 	 * @param string $siteId
+	 * @param LoggerInterface|null $logger
 	 * @param bool $checkPageExistence To disable slow filtering that is not relevant in test
 	 *  scenarios. Not meant to be used in production!
 	 *
@@ -63,23 +71,17 @@ class AffectedPagesFinder {
 	public function __construct(
 		UsageLookup $usageLookup,
 		TitleFactory $titleFactory,
-		$siteId,
-		$checkPageExistence = true
+		LinkBatchFactory $linkBatchFactory,
+		string $siteId,
+		?LoggerInterface $logger = null,
+		bool $checkPageExistence = true
 	) {
-		if ( !is_string( $siteId ) ) {
-			throw new InvalidArgumentException( '$siteId must be a string' );
-		}
-
-		if ( !is_bool( $checkPageExistence ) ) {
-			throw new InvalidArgumentException( '$checkPageExistence must be a boolean' );
-		}
-
 		$this->usageLookup = $usageLookup;
 		$this->titleFactory = $titleFactory;
+		$this->linkBatchFactory = $linkBatchFactory;
 		$this->siteId = $siteId;
+		$this->logger = $logger ?: new NullLogger();
 		$this->checkPageExistence = $checkPageExistence;
-		// TODO inject me
-		$this->logger = WikibaseClient::getDefaultInstance()->getLogger();
 	}
 
 	/**
@@ -334,6 +336,9 @@ private function makeVirtualUsages( array $titles, EntityId $entityId, array $as
 			$usagesForItem[] = new EntityUsage( $entityId, $aspect, $modifier );
 		}
 
+		// bulk-load the page IDs into the LinkCache
+		$this->linkBatchFactory->newLinkBatch( $titles )->execute();
+
 		$usagesPerPage = [];
 		foreach ( $titles as $title ) {
 			$pageId = $title->getArticleID();
diff --git a/client/includes/WikibaseClient.php b/client/includes/WikibaseClient.php
index 0fbe076e5da..57a6af1c092 100644
--- a/client/includes/WikibaseClient.php
+++ b/client/includes/WikibaseClient.php
@@ -1123,7 +1123,9 @@ private function getAffectedPagesFinder(): AffectedPagesFinder {
 		return new AffectedPagesFinder(
 			$this->getStore()->getUsageLookup(),
 			new TitleFactory(),
-			$this->settings->getSetting( 'siteGlobalID' )
+			MediaWikiServices::getInstance()->getLinkBatchFactory(),
+			$this->settings->getSetting( 'siteGlobalID' ),
+			$this->getLogger()
 		);
 	}
 
diff --git a/client/tests/phpunit/integration/includes/Changes/AffectedPagesFinderTest.php b/client/tests/phpunit/integration/includes/Changes/AffectedPagesFinderTest.php
index 103e8776727..c18a83d154e 100644
--- a/client/tests/phpunit/integration/includes/Changes/AffectedPagesFinderTest.php
+++ b/client/tests/phpunit/integration/includes/Changes/AffectedPagesFinderTest.php
@@ -1,10 +1,14 @@
 <?php
 
+declare( strict_types = 1 );
+
 namespace Wikibase\Client\Tests\Integration\Changes;
 
 use ArrayIterator;
 use DataValues\DataValue;
 use DataValues\StringValue;
+use LinkBatch;
+use MediaWiki\Cache\LinkBatchFactory;
 use Title;
 use TitleFactory;
 use Traversable;
@@ -65,6 +69,18 @@ private function getTitleFactory() {
 		return $titleFactory;
 	}
 
+	private function getLinkBatchFactory(): LinkBatchFactory {
+		$linkBatch = $this->createMock( LinkBatch::class );
+		$linkBatch->method( 'execute' )
+			->willReturn( null );
+
+		$linkBatchFactory = $this->createMock( LinkBatchFactory::class );
+		$linkBatchFactory->method( 'newLinkBatch' )
+			->willReturn( $linkBatch );
+
+		return $linkBatchFactory;
+	}
+
 	private function getAffectedPagesFinder( array $usage, array $expectedAspects ) {
 		$usageLookup = $this->createMock( UsageLookup::class );
 
@@ -76,7 +92,9 @@ private function getAffectedPagesFinder( array $usage, array $expectedAspects )
 		$affectedPagesFinder = new AffectedPagesFinder(
 			$usageLookup,
 			$this->getTitleFactory(),
+			$this->getLinkBatchFactory(),
 			'enwiki',
+			null,
 			false
 		);
 
@@ -513,7 +531,9 @@ public function testGetAffectedUsagesByPage_withDeletedPage() {
 		$affectedPagesFinder = new AffectedPagesFinder(
 			$this->getSiteLinkUsageLookup( $pageTitle ),
 			new TitleFactory(),
+			$this->getLinkBatchFactory(),
 			'enwiki',
+			null,
 			false
 		);
 
diff --git a/client/tests/phpunit/integration/includes/Changes/ChangeHandlerTest.php b/client/tests/phpunit/integration/includes/Changes/ChangeHandlerTest.php
index d49e9c4c0f3..452a2939ac1 100644
--- a/client/tests/phpunit/integration/includes/Changes/ChangeHandlerTest.php
+++ b/client/tests/phpunit/integration/includes/Changes/ChangeHandlerTest.php
@@ -4,6 +4,7 @@
 
 use ArrayIterator;
 use InvalidArgumentException;
+use MediaWiki\MediaWikiServices;
 use MediaWikiIntegrationTestCase;
 use Psr\Log\NullLogger;
 use SiteLookup;
@@ -44,7 +45,9 @@ private function getAffectedPagesFinder( UsageLookup $usageLookup, TitleFactory
 		return new AffectedPagesFinder(
 			$usageLookup,
 			$titleFactory,
+			MediaWikiServices::getInstance()->getLinkBatchFactory(),
 			'enwiki',
+			null,
 			false
 		);
 	}