Jeroen De Dauw has submitted this change and it was merged. Change subject: Much more clean up in SqlUsageTracker ......................................................................
Much more clean up in SqlUsageTracker Oh wow, this was confusing. Sorry to say that. Some $entityIds contained actual EntityId objects, others strings. An $entities array should always contain Entity objects, not something else. Change-Id: I360124c8df1896896c1183fe8567e6d5d979eb08 --- M client/includes/Usage/Sql/SqlUsageTracker.php M client/includes/Usage/Sql/UsageTableUpdater.php M client/includes/Usage/UsageLookup.php M client/tests/phpunit/includes/Usage/UsageLookupContractTester.php M repo/tests/phpunit/includes/store/sql/EntityPerPageIdPagerTest.php 5 files changed, 73 insertions(+), 85 deletions(-) Approvals: Jeroen De Dauw: Looks good to me, approved diff --git a/client/includes/Usage/Sql/SqlUsageTracker.php b/client/includes/Usage/Sql/SqlUsageTracker.php index 701ccad..35eec2a 100644 --- a/client/includes/Usage/Sql/SqlUsageTracker.php +++ b/client/includes/Usage/Sql/SqlUsageTracker.php @@ -8,7 +8,6 @@ use Exception; use InvalidArgumentException; use Iterator; -use LoadBalancer; use Wikibase\Client\Store\Sql\ConnectionManager; use Wikibase\Client\Usage\EntityUsage; use Wikibase\Client\Usage\UsageLookup; @@ -24,11 +23,6 @@ * @author Daniel Kinzler */ class SqlUsageTracker implements UsageTracker, UsageLookup { - - /** - * @var string - */ - private $tableName; /** * @var EntityIdParser @@ -47,10 +41,9 @@ /** * @param EntityIdParser $idParser - * @param LoadBalancer $loadBalancer + * @param ConnectionManager $connectionManager */ public function __construct( EntityIdParser $idParser, ConnectionManager $connectionManager ) { - $this->tableName = 'wbc_entity_usage'; $this->idParser = $idParser; $this->connectionManager = $connectionManager; } @@ -61,7 +54,7 @@ * @return UsageTableUpdater */ private function newTableUpdater( DatabaseBase $db ) { - return new UsageTableUpdater( $db, $this->tableName, $this->batchSize ); + return new UsageTableUpdater( $db, 'wbc_entity_usage', $this->batchSize ); } /** @@ -83,13 +76,14 @@ } /** - * Returns the string serialization of an EntityId. + * @param EntityId[] $entityIds * - * @param EntityId $entityId - * @return string + * @return string[] */ - private function getEntityIdSerialization( EntityId $entityId ) { - return $entityId->getSerialization(); + private function getEntityIdStrings( array $entityIds ) { + return array_map( function( EntityId $entityId ) { + return $entityId->getSerialization(); + }, $entityIds ); } /** @@ -100,6 +94,7 @@ * * @throws InvalidArgumentException * @throws UsageTrackerException + * @throws Exception * @return EntityUsage[] Usages before the update, in the same form as $usages */ public function trackUsedEntities( $pageId, array $usages ) { @@ -110,13 +105,13 @@ $db = $this->connectionManager->beginAtomicSection( __METHOD__ ); try { - $oldUsage = $this->queryUsageForPage( $db, $pageId ); + $oldUsages = $this->queryUsagesForPage( $db, $pageId ); $tableUpdater = $this->newTableUpdater( $db ); - $tableUpdater->updateUsage( $pageId, $oldUsage, $usages ); + $tableUpdater->updateUsage( $pageId, $oldUsages, $usages ); $this->connectionManager->commitAtomicSection( $db, __METHOD__ ); - return $oldUsage; + return $oldUsages; } catch ( Exception $ex ) { $this->connectionManager->rollbackAtomicSection( $db, __METHOD__ ); @@ -131,22 +126,23 @@ /** * @see UsageTracker::removeEntities * - * @param EntityId[] $entities + * @param EntityId[] $entityIds * * @throws UsageTrackerException + * @throws Exception */ - public function removeEntities( array $entities ) { - if ( empty( $entities ) ) { + public function removeEntities( array $entityIds ) { + if ( empty( $entityIds ) ) { return; } - $entityIds = array_map( array( $this, 'getEntityIdSerialization' ), $entities ); + $idStrings = $this->getEntityIdStrings( $entityIds ); $db = $this->connectionManager->beginAtomicSection( __METHOD__ ); try { $tableUpdater = $this->newTableUpdater( $db ); - $tableUpdater->removeEntities( $entityIds ); + $tableUpdater->removeEntities( $idStrings ); $this->connectionManager->commitAtomicSection( $db, __METHOD__ ); } catch ( Exception $ex ) { @@ -168,12 +164,13 @@ * @return EntityUsage[] * @throws UsageTrackerException */ - public function getUsageForPage( $pageId ) { + public function getUsagesForPage( $pageId ) { $db = $this->connectionManager->getReadConnection(); - $usages = $this->queryUsageForPage( $db, $pageId ); + $usages = $this->queryUsagesForPage( $db, $pageId ); $this->connectionManager->releaseConnection( $db ); + return $usages; } @@ -184,13 +181,13 @@ * @throws InvalidArgumentException * @return EntityUsage[] */ - private function queryUsageForPage( DatabaseBase $db, $pageId ) { + private function queryUsagesForPage( DatabaseBase $db, $pageId ) { if ( !is_int( $pageId ) ) { throw new InvalidArgumentException( '$pageId must be an int.' ); } $res = $db->select( - $this->tableName, + 'wbc_entity_usage', array( 'eu_aspect', 'eu_entity_id' ), array( 'eu_page_id' => (int)$pageId ), __METHOD__ @@ -207,12 +204,11 @@ */ private function convertRowsToUsages( $rows ) { $usages = array(); - foreach ( $rows as $row ) { - $id = $this->idParser->parse( $row->eu_entity_id ); - $usage = new EntityUsage( $id, $row->eu_aspect ); + foreach ( $rows as $object ) { + $entityId = $this->idParser->parse( $object->eu_entity_id ); + $usage = new EntityUsage( $entityId, $object->eu_aspect ); $key = $usage->getIdentityString(); - $usages[$key] = $usage; } @@ -222,20 +218,19 @@ /** * @see UsageTracker::getPagesUsing * - * @param EntityId[] $entities + * @param EntityId[] $entityIds * @param string[] $aspects * * @return Iterator An iterator over page IDs. * @throws UsageTrackerException */ - public function getPagesUsing( array $entities, array $aspects = array() ) { - if ( empty( $entities ) ) { + public function getPagesUsing( array $entityIds, array $aspects = array() ) { + if ( empty( $entityIds ) ) { return array(); } - $entityIds = array_map( array( $this, 'getEntityIdSerialization' ), $entities ); - - $where = array( 'eu_entity_id' => $entityIds ); + $idStrings = $this->getEntityIdStrings( $entityIds ); + $where = array( 'eu_entity_id' => $idStrings ); if ( !empty( $aspects ) ) { $where['eu_aspect'] = $aspects; @@ -244,13 +239,13 @@ $db = $this->connectionManager->getReadConnection(); $res = $db->select( - $this->tableName, + 'wbc_entity_usage', array( 'DISTINCT eu_page_id' ), $where, __METHOD__ ); - $pages = $this->extractProperty( 'eu_page_id', $res ); + $pages = $this->extractProperty( $res, 'eu_page_id' ); $this->connectionManager->releaseConnection( $db ); @@ -271,40 +266,32 @@ return array(); } - $entityIdsBySerialization = array(); - $entityIdStrings = array(); + $entityIdMap = array(); - foreach ( $entityIds as $id ) { - $serialization = $this->getEntityIdSerialization( $id ); - $entityIdStrings[] = $serialization; - $entityIdsBySerialization[ $serialization ] = $id; + foreach ( $entityIds as $entityId ) { + $idString = $entityId->getSerialization(); + $entityIdMap[$idString] = $entityId; } - $usedEntityIdStrings = $this->getUsedEntities( $entityIdStrings ); + $usedIdStrings = $this->getUsedEntityIdStrings( array_keys( $entityIdMap ) ); - $unusedEntityIdStrings = array_diff( $entityIdStrings, $usedEntityIdStrings ); - - $unusedEntityIds = array_intersect_key( - $entityIdsBySerialization, - array_flip( $unusedEntityIdStrings ) - ); - - return $unusedEntityIds; + return array_diff_key( $entityIdMap, array_flip( $usedIdStrings ) ); } /** * Returns those entity ids which are used from a given set of entity ids. * - * @param string[] $entityIds + * @param string[] $idStrings + * * @return string[] */ - private function getUsedEntities( array $entityIds ) { - $where = array( 'eu_entity_id' => $entityIds ); + private function getUsedEntityIdStrings( array $idStrings ) { + $where = array( 'eu_entity_id' => $idStrings ); $db = $this->connectionManager->getReadConnection(); $res = $db->select( - $this->tableName, + 'wbc_entity_usage', array( 'eu_entity_id' ), $where, __METHOD__ @@ -312,25 +299,25 @@ $this->connectionManager->releaseConnection( $db ); - return $this->extractProperty( 'eu_entity_id', $res ); + return $this->extractProperty( $res, 'eu_entity_id' ); } /** - * Returns an array of values for $key property from the array $arr. + * Returns an array of values extracted from the $key property from each object. * + * @param array|Iterator $objects * @param string $key - * @param array|Iterator $arr * * @return array */ - private function extractProperty( $key, $arr ) { - $newArr = array(); + private function extractProperty( $objects, $key ) { + $array = array(); - foreach( $arr as $arrItem ) { - $newArr[] = $arrItem->$key; + foreach ( $objects as $object ) { + $array[] = $object->$key; } - return $newArr; + return $array; } } diff --git a/client/includes/Usage/Sql/UsageTableUpdater.php b/client/includes/Usage/Sql/UsageTableUpdater.php index 77070b9..ccdf941 100644 --- a/client/includes/Usage/Sql/UsageTableUpdater.php +++ b/client/includes/Usage/Sql/UsageTableUpdater.php @@ -174,16 +174,16 @@ /** * @param int $pageId * @param string $aspect - * @param string[] $entityIdStrings String IDs of the entities to be removed. + * @param string[] $idStrings Id strings of the entities to be removed. * * @return int The number of entries removed */ - private function removeAspectForPage( $pageId, $aspect, array $entityIdStrings ) { - if ( empty( $entityIdStrings ) ) { + private function removeAspectForPage( $pageId, $aspect, array $idStrings ) { + if ( empty( $idStrings ) ) { return 0; } - $batches = array_chunk( $entityIdStrings, $this->batchSize, true ); + $batches = array_chunk( $idStrings, $this->batchSize, true ); $c = 0; foreach ( $batches as $batch ) { @@ -238,14 +238,14 @@ * Removes usage tracking for the given set of entities. * This is used typically when entities were deleted. * - * @param string[] $entityIdStrings + * @param string[] $idStrings */ - public function removeEntities( array $entityIdStrings ) { - if ( empty( $entityIdStrings ) ) { + public function removeEntities( array $idStrings ) { + if ( empty( $idStrings ) ) { return; } - $batches = array_chunk( $entityIdStrings, $this->batchSize ); + $batches = array_chunk( $idStrings, $this->batchSize ); foreach ( $batches as $batch ) { $this->connection->delete( diff --git a/client/includes/Usage/UsageLookup.php b/client/includes/Usage/UsageLookup.php index d86b71d..6c93d3d 100644 --- a/client/includes/Usage/UsageLookup.php +++ b/client/includes/Usage/UsageLookup.php @@ -23,12 +23,12 @@ * @return EntityUsage[] * @throws UsageTrackerException */ - public function getUsageForPage( $pageId ); + public function getUsagesForPage( $pageId ); /** * Returns the pages that use any of the given entities. * - * @param EntityId[] $entities + * @param EntityId[] $entityIds * @param string[] $aspects Which aspects to consider (if omitted, all aspects are considered). * Use the EntityUsage::XXX_USAGE constants to represent aspects. * @@ -36,7 +36,7 @@ * If $aspects is given, only usages of these aspects are included in the result. * @throws UsageTrackerException */ - public function getPagesUsing( array $entities, array $aspects = array() ); + public function getPagesUsing( array $entityIds, array $aspects = array() ); /** * Returns the elements of $entities that are currently not used as @@ -44,11 +44,11 @@ * question which of a given list of entities are currently being used on * wiki pages. * - * @param EntityId[] $entities + * @param EntityId[] $entityIds * * @return EntityId[] A list of elements of $entities that are unused. * @throws UsageTrackerException */ - public function getUnusedEntities( array $entities ); + public function getUnusedEntities( array $entityIds ); } diff --git a/client/tests/phpunit/includes/Usage/UsageLookupContractTester.php b/client/tests/phpunit/includes/Usage/UsageLookupContractTester.php index 0798685..0a2eb96 100644 --- a/client/tests/phpunit/includes/Usage/UsageLookupContractTester.php +++ b/client/tests/phpunit/includes/Usage/UsageLookupContractTester.php @@ -48,9 +48,9 @@ $this->tracker->trackUsedEntities( 23, $usages ); - Assert::assertEmpty( $this->lookup->getUsageForPage( 24 ) ); + Assert::assertEmpty( $this->lookup->getUsagesForPage( 24 ) ); - $actualUsage = $this->lookup->getUsageForPage( 23 ); + $actualUsage = $this->lookup->getUsagesForPage( 23 ); Assert::assertCount( 3, $actualUsage ); $actualUsageStrings = $this->getUsageStrings( $actualUsage ); diff --git a/repo/tests/phpunit/includes/store/sql/EntityPerPageIdPagerTest.php b/repo/tests/phpunit/includes/store/sql/EntityPerPageIdPagerTest.php index dfbbea1..27cbc53 100644 --- a/repo/tests/phpunit/includes/store/sql/EntityPerPageIdPagerTest.php +++ b/repo/tests/phpunit/includes/store/sql/EntityPerPageIdPagerTest.php @@ -24,16 +24,16 @@ class EntityPerPageIdPagerTest extends \MediaWikiTestCase { /** - * @param EntityId[] $entities + * @param EntityId[] $entityIds * @param string|null $type * * @return EntityPerPageIdPager */ - protected function newPager( array $entities, $type = null ) { + protected function newPager( array $entityIds, $type = null ) { $keydIds = array(); - foreach ( $entities as $id ) { - $key = $id->getSerialization(); - $keydIds[$key] = $id; + foreach ( $entityIds as $entityId ) { + $key = $entityId->getSerialization(); + $keydIds[$key] = $entityId; } $listEntities = function( $entityType, $limit, EntityId $after = null ) use ( $keydIds ) { @@ -126,4 +126,5 @@ ) ); } + } -- To view, visit https://gerrit.wikimedia.org/r/166006 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I360124c8df1896896c1183fe8567e6d5d979eb08 Gerrit-PatchSet: 4 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: Thiemo Mättig (WMDE) <thiemo.maet...@wikimedia.de> Gerrit-Reviewer: Adrian Lang <adrian.l...@wikimedia.de> Gerrit-Reviewer: Daniel Kinzler <daniel.kinz...@wikimedia.de> Gerrit-Reviewer: Jeroen De Dauw <jeroended...@gmail.com> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits