jenkins-bot has submitted this change and it was merged. Change subject: Restrict ItemByTitleHelper site/title combinations ......................................................................
Restrict ItemByTitleHelper site/title combinations This restricts the ItemByTitleHelper to only allowing 1 Site and multiple titles or and euqal number of sites and titles. This will stop the div0 error and also will limit the crazy combinations that can be used such as 4 sites and 9 titles.. Also change the for loop into 2 foreach loops which are much easier to read and follow Removes GPL headers Bug: 53037 Change-Id: Ib04b5dd57f2e96d21e7a0af47a18be0a365d097f --- M repo/includes/api/ItemByTitleHelper.php M repo/tests/phpunit/includes/api/ItemByTitleHelperTest.php 2 files changed, 71 insertions(+), 32 deletions(-) Approvals: Daniel Kinzler: Looks good to me, approved jenkins-bot: Verified diff --git a/repo/includes/api/ItemByTitleHelper.php b/repo/includes/api/ItemByTitleHelper.php index 1c6c769..a8e27db 100644 --- a/repo/includes/api/ItemByTitleHelper.php +++ b/repo/includes/api/ItemByTitleHelper.php @@ -2,7 +2,6 @@ namespace Wikibase\Api; -use Wikibase\DataModel\Entity\EntityId; use Wikibase\DataModel\Entity\ItemId; use Wikibase\Item; use Wikibase\Repo\WikibaseRepo; @@ -11,12 +10,9 @@ * Helper class for api modules to resolve page+title pairs into items. * * @since 0.4 - * - * @file - * @ingroup WikibaseRepo - * * @licence GNU GPL v2+ * @author Marius Hoch < [email protected] > + * @author Adam Shorland */ class ItemByTitleHelper { /** @@ -62,13 +58,12 @@ * @return array */ public function getEntityIds( array $sites, array $titles, $normalize ) { + $counter = 0; $ids = array(); - $missing = 0; $numSites = count( $sites ); $numTitles = count( $titles ); - $max = max( $numSites, $numTitles ); - if ( $normalize === true && $max > 1 ) { + if ( $normalize && max( $numSites, $numTitles ) > 1 ) { // For performance reasons we only do this if the user asked for it and only for one title! $this->apiBase->dieUsage( 'Normalize is only allowed if exactly one site and one page have been given', @@ -76,30 +71,20 @@ ); } - $idxSites = 0; - $idxTitles = 0; + // Restrict the crazy combinations of sites and titles that can be used + if( $numSites !== 1 && $numSites !== $numTitles ) { + $this->apiBase->dieUsage( 'Must request one site or an equal number of sites and titles','params-illegal' ); + } - for ( $k = 0; $k < $max; $k++ ) { - $siteId = $sites[$idxSites++ % $numSites]; - $title = $this->stringNormalizer->trimToNFC( $titles[$idxTitles++ % $numTitles] ); - - $id = $this->siteLinkCache->getItemIdForLink( $siteId, $title ); - - // Try harder by requesting normalization on the external site. - if ( $id === false && $normalize === true ) { - $siteObj = $this->siteStore->getSite( $siteId ); - $id = $this->normalizeTitle( $title, $siteObj ); - } - - if ( $id === false ) { - $this->apiBase->getResult()->addValue( 'entities', (string)(--$missing), - array( 'site' => $siteId, 'title' => $title, 'missing' => "" ) - ); - } else { - $entityIdFormatter = WikibaseRepo::getDefaultInstance()->getEntityIdFormatter(); - - $id = ItemId::newFromNumber( $id ); - $ids[] = $entityIdFormatter->format( $id ); + foreach( $sites as $siteId ) { + foreach( $titles as $title ) { + $entityId = $this->getEntiyId( $siteId, $title, $normalize ); + if( !is_null( $entityId ) ) { + $ids[] = $entityId; + } else { + $counter--; + $this->addMissingEntityToResult( $siteId, $title, $counter ); + } } } @@ -107,6 +92,43 @@ } /** + * Tries to find entity id for given siteId and title combination + * + * @param string $siteId + * @param string $title + * @param bool $normalize + * + * @return string|null + */ + private function getEntiyId( $siteId, $title, $normalize ) { + $title = $this->stringNormalizer->trimToNFC( $title ); + $id = $this->siteLinkCache->getItemIdForLink( $siteId, $title ); + + // Try harder by requesting normalization on the external site. + if ( $id === false && $normalize === true ) { + $siteObj = $this->siteStore->getSite( $siteId ); + $id = $this->normalizeTitle( $title, $siteObj ); + } + + if ( $id === false ) { + return null; + } else { + return ItemId::newFromNumber( $id )->getPrefixedId(); + } + } + + /** + * @todo factor this out of ItemByTitleHelper, this has nothing to do with looking for item by titles + */ + protected function addMissingEntityToResult( $siteId, $title, $counter ){ + $this->apiBase->getResult()->addValue( + 'entities', + (string)($counter), + array( 'site' => $siteId, 'title' => $title, 'missing' => "" ) + ); + } + + /** * Tries to normalize the given page title against the given client site. * Updates $title accordingly and adds the normalization to the API output. * diff --git a/repo/tests/phpunit/includes/api/ItemByTitleHelperTest.php b/repo/tests/phpunit/includes/api/ItemByTitleHelperTest.php index b82857d..c3b3b00 100644 --- a/repo/tests/phpunit/includes/api/ItemByTitleHelperTest.php +++ b/repo/tests/phpunit/includes/api/ItemByTitleHelperTest.php @@ -66,7 +66,8 @@ $apiBaseMock ->expects( $expectDieUsage ? $this->once() : $this->never() ) - ->method( 'dieUsage' ); + ->method( 'dieUsage' ) + ->will( $this->throwException( new \UsageException( 'MockUsageExceptionMessage', 'MockUsageExceptionCode' ) ) ); $apiResultMock = $this->getMockBuilder( '\ApiResult' ) ->disableOriginalConstructor() @@ -168,6 +169,8 @@ * Makes sure the request will fail if we want normalization for two titles */ public function testGetEntityIdsNormalizationNotAllowed() { + $this->setExpectedException( 'UsageException' ); + $itemByTitleHelper = new ItemByTitleHelper( $this->getApiBaseMock( 0, true ), $this->getSiteLinkCacheMock( 1 ), @@ -217,4 +220,18 @@ // Normalization in unit tests is actually using Title::getPrefixedText instead of a real API call $this->assertEquals( \Title::newFromText( $title )->getPrefixedText(), $title ); } + + public function testNoSites(){ + $this->setExpectedException( 'UsageException' ); + + $itemByTitleHelper = new ItemByTitleHelper( + $this->getApiBaseMock( null, true ), + $this->getSiteLinkCacheMock( 123 ), + $this->getSiteStoreMock(), + new StringNormalizer() + ); + + $entityId = $itemByTitleHelper->getEntityIds( array( ), array( 'barfoo' ), false ); + } + } -- To view, visit https://gerrit.wikimedia.org/r/80797 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ib04b5dd57f2e96d21e7a0af47a18be0a365d097f Gerrit-PatchSet: 11 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: Addshore <[email protected]> Gerrit-Reviewer: Addshore <[email protected]> Gerrit-Reviewer: Aude <[email protected]> Gerrit-Reviewer: Daniel Kinzler <[email protected]> Gerrit-Reviewer: Denny Vrandecic <[email protected]> Gerrit-Reviewer: Jeroen De Dauw <[email protected]> Gerrit-Reviewer: Legoktm <[email protected]> Gerrit-Reviewer: Tobias Gritschacher <[email protected]> Gerrit-Reviewer: jenkins-bot _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
