Skip to content

Commit

Permalink
Use LinkBatch in AffectedPagesFinder
Browse files Browse the repository at this point in the history
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
  • Loading branch information
lucaswerkmeister committed Nov 2, 2020
1 parent acf74d9 commit 56685d6
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 18 deletions.
39 changes: 22 additions & 17 deletions 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;
Expand All @@ -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;
Expand All @@ -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!
*
Expand All @@ -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();
}

/**
Expand Down Expand Up @@ -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();
Expand Down
4 changes: 3 additions & 1 deletion client/includes/WikibaseClient.php
Expand Up @@ -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()
);
}

Expand Down
@@ -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;
Expand Down Expand Up @@ -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 );

Expand All @@ -76,7 +92,9 @@ private function getAffectedPagesFinder( array $usage, array $expectedAspects )
$affectedPagesFinder = new AffectedPagesFinder(
$usageLookup,
$this->getTitleFactory(),
$this->getLinkBatchFactory(),
'enwiki',
null,
false
);

Expand Down Expand Up @@ -513,7 +531,9 @@ public function testGetAffectedUsagesByPage_withDeletedPage() {
$affectedPagesFinder = new AffectedPagesFinder(
$this->getSiteLinkUsageLookup( $pageTitle ),
new TitleFactory(),
$this->getLinkBatchFactory(),
'enwiki',
null,
false
);

Expand Down
Expand Up @@ -4,6 +4,7 @@

use ArrayIterator;
use InvalidArgumentException;
use MediaWiki\MediaWikiServices;
use MediaWikiIntegrationTestCase;
use Psr\Log\NullLogger;
use SiteLookup;
Expand Down Expand Up @@ -44,7 +45,9 @@ private function getAffectedPagesFinder( UsageLookup $usageLookup, TitleFactory
return new AffectedPagesFinder(
$usageLookup,
$titleFactory,
MediaWikiServices::getInstance()->getLinkBatchFactory(),
'enwiki',
null,
false
);
}
Expand Down

0 comments on commit 56685d6

Please sign in to comment.