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) <[email protected]>
Gerrit-Reviewer: Adrian Lang <[email protected]>
Gerrit-Reviewer: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Jeroen De Dauw <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
