WMDE-leszek has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/363149 )
Change subject: Fix foreign IDs handling in SqlEntityInfoBuilder
......................................................................
Fix foreign IDs handling in SqlEntityInfoBuilder
Database queries referring to page titles of entity pages,
and to full entity IDs should consider that IDs of entities
consider "foreign" by local repo are not prefixed in the
foreign repo's database.
This also makes SqlEntityInfoBuilder locally rely on numeric entity
IDs only when it is necessary. Otherwise full IDs (without
a prefix when applicable) are used. This makes the class
slightly less bound to the numeric entity IDs only.
SqlEntityInfoBuilderTest::testEntityIdsArePrefixedWithRepositoryNameFullEntityId
should have caught at least part of the issue but the
test setup was done wrong, so test has been passing
misleadingly.
This fixes the test, and adds tests covering the other
part of behaviour related to the issue.
Bug: T169199
Change-Id: I61b8c1a576b600c803284d528f40ae761a117ac6
---
M lib/includes/Store/Sql/SqlEntityInfoBuilder.php
M lib/tests/phpunit/Store/Sql/SqlEntityInfoBuilderTest.php
2 files changed, 113 insertions(+), 36 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/49/363149/1
diff --git a/lib/includes/Store/Sql/SqlEntityInfoBuilder.php
b/lib/includes/Store/Sql/SqlEntityInfoBuilder.php
index 7437598..50c366d 100644
--- a/lib/includes/Store/Sql/SqlEntityInfoBuilder.php
+++ b/lib/includes/Store/Sql/SqlEntityInfoBuilder.php
@@ -80,6 +80,18 @@
private $numericIdsByType = array();
/**
+ * Maps of ID strings to local ID parts (i.e. excluding the repository
prefix, if the
+ * instance is handling entities "foreign" to the local repo (i.e.
input entities are prefixed),
+ * group by entity type.
+ * Used to build database queries on tables that use entity ID (as a
string). Database used by
+ * the "foreign" repo does not contain prefix in the ID columns that
the local repo might be
+ * using for the other repo's entity IDs.
+ *
+ * @var array[]
+ */
+ private $localIdsByType = [];
+
+ /**
* Maps of id strings to page info records, grouped by entity type.
* This uses the same basic structure as $this->numericIdsByType.
* Each page info record is an associative array with keys page_id
@@ -195,6 +207,7 @@
$this->entityIds = array();
$this->entityInfo = array();
$this->numericIdsByType = array();
+ $this->localIdsByType = [];
foreach ( $ids as $id ) {
$this->updateEntityInfo( $id );
@@ -283,6 +296,7 @@
unset( $this->entityInfo[$idString] );
unset( $this->numericIdsByType[$type][$idString] );
+ unset( $this->localIdsByType[$type][$idString] );
}
/**
@@ -315,6 +329,7 @@
$this->entityInfo[$key]['id'] = $key;
// FIXME: this will fail for IDs that do not have a numeric form
$this->numericIdsByType[$type][$key] = $id->getNumericId();
+ $this->localIdsByType[$type][$key] = $id->getLocalPart();
}
/**
@@ -356,7 +371,7 @@
//NOTE: we make one DB query per entity type, so we can take
advantage of the
// database index on the term_entity_type field.
- foreach ( array_keys( $this->numericIdsByType ) as $type ) {
+ foreach ( array_keys( $this->localIdsByType ) as $type ) {
$this->collectTermsForEntities( $type, $termTypes,
$languages );
}
@@ -377,14 +392,12 @@
* @param string[]|null $languages
*/
private function collectTermsForEntities( $entityType, array $termTypes
= null, array $languages = null ) {
- $entityIds = $this->numericIdsByType[$entityType];
-
$where = [];
if ( $this->readFullEntityIdColumn === true ) {
- $where['term_full_entity_id'] = array_keys( $entityIds
);
+ $where['term_full_entity_id'] =
$this->localIdsByType[$entityType];
} else {
- $where['term_entity_id'] = $entityIds;
+ $where['term_entity_id'] =
$this->numericIdsByType[$entityType];
$where['term_entity_type'] = $entityType;
}
@@ -582,12 +595,16 @@
$this->entityInfo = array_diff_key( $this->entityInfo,
array_flip( $ids ) );
$this->entityIds = array_diff_key( $this->entityIds,
array_flip( $ids ) );
- foreach ( $this->numericIdsByType as &$numeridIds ) {
- $numeridIds = array_diff_key( $numeridIds, array_flip(
$ids ) );
+ foreach ( $this->numericIdsByType as &$numericIds ) {
+ $numericIds = array_diff_key( $numericIds, array_flip(
$ids ) );
+ }
+ foreach ( $this->localIdsByType as &$idsByType ) {
+ $idsByType = array_diff_key( $idsByType, array_flip(
$ids ) );
}
// remove empty entries
$this->numericIdsByType = array_filter( $this->numericIdsByType
);
+ $this->localIdsByType = array_filter( $this->localIdsByType );
}
/**
@@ -603,7 +620,6 @@
if ( isset( $this->pageInfoByType[$entityType] ) ) {
return $this->pageInfoByType[$entityType];
}
- $entityIds = $this->numericIdsByType[$entityType];
$dbr = $this->getConnection( DB_REPLICA );
@@ -619,7 +635,7 @@
$fields,
[
'page_namespace' =>
$this->entityNamespaceLookup->getEntityNamespace( $entityType ),
- 'page_title' => array_keys( $entityIds ),
+ 'page_title' =>
$this->localIdsByType[$entityType],
],
__METHOD__,
[],
@@ -629,7 +645,9 @@
$this->pageInfoByType[$entityType] = [];
foreach ( $res as $row ) {
- $this->pageInfoByType[$entityType][$row->page_title] = [
+ $id = $this->idParser->parse( $row->page_title );
+ $idKey = $id->getSerialization();
+ $this->pageInfoByType[$entityType][$idKey] = [
'page_id' => $row->page_id,
'redirect_target' => $row->rd_title,
];
@@ -648,7 +666,7 @@
private function getPageInfo() {
$info = array();
- foreach ( $this->numericIdsByType as $type => $ids ) {
+ foreach ( $this->localIdsByType as $type => $ids ) {
$info[$type] = $this->getPageInfoForType( $type );
}
diff --git a/lib/tests/phpunit/Store/Sql/SqlEntityInfoBuilderTest.php
b/lib/tests/phpunit/Store/Sql/SqlEntityInfoBuilderTest.php
index dce2df8..3aaea6c 100644
--- a/lib/tests/phpunit/Store/Sql/SqlEntityInfoBuilderTest.php
+++ b/lib/tests/phpunit/Store/Sql/SqlEntityInfoBuilderTest.php
@@ -11,6 +11,7 @@
use Wikibase\DataModel\Entity\ItemId;
use Wikibase\DataModel\Entity\Property;
use Wikibase\DataModel\Entity\PropertyId;
+use Wikibase\DataModel\Services\EntityId\PrefixMappingEntityIdParser;
use Wikibase\ItemContent;
use Wikibase\Lib\EntityIdComposer;
use Wikibase\Lib\Store\EntityNamespaceLookup;
@@ -253,18 +254,12 @@
$this->assertFalse( $builder->getEntityInfo()->hasEntityInfo(
$propertyId ) );
}
- public function testEntityIdsArePrefixedWithRepositoryName() {
- $itemId = new ItemId( 'foo:Q1' );
-
+ private function saveFakeForeignItemTerm( ItemId $itemId, $termType,
$termLanguage, $termText ) {
// Inserting a dummy label for item with numeric ID part equal
to 1.
// In this test local database is used to pretend to be a
databse of
// repository "foo". Terms fetched from the database should be
// matched to entity IDs using correct repository prefixes (this
// builder's responsibility as this information is not stored
in wb_terms table).
-
- $label = 'dummy label';
- $languageCode = 'en';
-
$this->insertRows(
'wb_terms',
[
@@ -279,16 +274,25 @@
[
$itemId->getEntityType(),
$itemId->getNumericId(),
- 'label',
- $languageCode,
- $label,
- $label
+ $termType,
+ $termLanguage,
+ $termText,
+ $termText
]
]
);
+ }
+
+ public function testEntityIdsArePrefixedWithRepositoryName() {
+ $itemId = new ItemId( 'foo:Q1' );
+
+ $label = 'dummy label';
+ $languageCode = 'en';
+
+ $this->saveFakeForeignItemTerm( $itemId, 'label',
$languageCode, $label );
$builder = new SqlEntityInfoBuilder(
- new BasicEntityIdParser(),
+ new PrefixMappingEntityIdParser( [ '' => 'foo' ], new
BasicEntityIdParser() ),
new EntityIdComposer( [
'item' => function( $repositoryName,
$uniquePart ) {
return
ItemId::newFromRepositoryAndNumber( $repositoryName, $uniquePart );
@@ -305,6 +309,31 @@
$entityInfo = $builder->getEntityInfo()->getEntityInfo( $itemId
);
$this->assertSame( $label,
$entityInfo['labels'][$languageCode]['value'] );
+ }
+
+ public function testRemoveMissingConsidersForeignEntities() {
+ $itemId = new ItemId( 'foo:Q1' );
+
+ $this->saveFakeForeignItemTerm( $itemId, 'label', 'en', 'dummy
label' );
+
+ $builder = new SqlEntityInfoBuilder(
+ new PrefixMappingEntityIdParser( [ '' => 'foo' ], new
BasicEntityIdParser() ),
+ new EntityIdComposer( [
+ 'item' => function( $repositoryName,
$uniquePart ) {
+ return
ItemId::newFromRepositoryAndNumber( $repositoryName, $uniquePart );
+ },
+ ] ),
+ $this->getEntityNamespaceLookup(),
+ [ $itemId ],
+ false,
+ 'foo'
+ );
+
+ $builder->removeMissing();
+
+ $entityInfo = $builder->getEntityInfo();
+
+ $this->assertTrue( $entityInfo->hasEntityInfo( $itemId ) );
}
public function
testConstructorIgnoresEntityIdsFromOtherRepositoriesFullEntityId() {
@@ -325,18 +354,12 @@
$this->assertFalse( $builder->getEntityInfo()->hasEntityInfo(
$propertyId ) );
}
- public function
testEntityIdsArePrefixedWithRepositoryNameFullEntityId() {
- $itemId = new ItemId( 'foo:Q1' );
-
+ private function saveFakeForeignItemTermUsingFullItemId( ItemId
$itemId, $termType, $termLanguage, $termText ) {
// Inserting a dummy label for item with numeric ID part equal
to 1.
// In this test local database is used to pretend to be a
databse of
// repository "foo". Terms fetched from the database should be
// matched to entity IDs using correct repository prefixes (this
// builder's responsibility as this information is not stored
in wb_terms table).
-
- $label = 'dummy label';
- $languageCode = 'en';
-
$this->insertRows(
'wb_terms',
[
@@ -352,17 +375,26 @@
[
$itemId->getEntityType(),
$itemId->getNumericId(),
- $itemId->getSerialization(),
- 'label',
- $languageCode,
- $label,
- $label
+ $itemId->getLocalPart(),
+ $termType,
+ $termLanguage,
+ $termText,
+ $termText
]
]
);
+ }
+
+ public function
testEntityIdsArePrefixedWithRepositoryNameFullEntityId() {
+ $itemId = new ItemId( 'foo:Q1' );
+
+ $label = 'dummy label';
+ $languageCode = 'en';
+
+ $this->saveFakeForeignItemTermUsingFullItemId( $itemId,
'label', $languageCode, $label );
$builder = new SqlEntityInfoBuilder(
- new BasicEntityIdParser(),
+ new PrefixMappingEntityIdParser( [ '' => 'foo' ], new
BasicEntityIdParser() ),
new EntityIdComposer( [
'item' => function( $repositoryName,
$uniquePart ) {
return
ItemId::newFromRepositoryAndNumber( $repositoryName, $uniquePart );
@@ -383,6 +415,33 @@
$this->assertSame( $label,
$entityInfo['labels'][$languageCode]['value'] );
}
+ public function
testRemoveMissingConsidersForeignEntities_fullEntityId() {
+ $itemId = new ItemId( 'foo:Q1' );
+
+ $this->saveFakeForeignItemTermUsingFullItemId( $itemId,
'label', 'en', 'dummy label' );
+
+ $builder = new SqlEntityInfoBuilder(
+ new PrefixMappingEntityIdParser( [ '' => 'foo' ], new
BasicEntityIdParser() ),
+ new EntityIdComposer( [
+ 'item' => function( $repositoryName,
$uniquePart ) {
+ return
ItemId::newFromRepositoryAndNumber( $repositoryName, $uniquePart );
+ },
+ ] ),
+ $this->getEntityNamespaceLookup(),
+ [ $itemId ],
+ false,
+ 'foo'
+ );
+
+ $builder->setReadFullEntityIdColumn( true );
+
+ $builder->removeMissing();
+
+ $entityInfo = $builder->getEntityInfo();
+
+ $this->assertTrue( $entityInfo->hasEntityInfo( $itemId ) );
+ }
+
/**
* @dataProvider retainEntityInfoProvider
*/
--
To view, visit https://gerrit.wikimedia.org/r/363149
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I61b8c1a576b600c803284d528f40ae761a117ac6
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: WMDE-leszek <leszek.mani...@wikimedia.de>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
