Aude has submitted this change and it was merged. Change subject: Fix caching issue in PropertySQLLookup ......................................................................
Fix caching issue in PropertySQLLookup Change-Id: If26f0d5a80a3af1f61c7a6c9951e15a8c38e37f4 --- M lib/includes/store/sql/PropertySQLLookup.php M lib/tests/phpunit/store/sql/PropertySQLLookupTest.php 2 files changed, 81 insertions(+), 20 deletions(-) Approvals: Aude: Verified; Looks good to me, approved jenkins-bot: Checked diff --git a/lib/includes/store/sql/PropertySQLLookup.php b/lib/includes/store/sql/PropertySQLLookup.php index 55a6ccb..93f5dd9 100644 --- a/lib/includes/store/sql/PropertySQLLookup.php +++ b/lib/includes/store/sql/PropertySQLLookup.php @@ -29,6 +29,7 @@ * * @licence GNU GPL v2+ * @author Katie Filbert < [email protected] > + * @author Daniel Kinzler */ class PropertySQLLookup implements PropertyLookup { @@ -72,15 +73,11 @@ $statementsByProperty[$propertyId->getNumericId()][] = $statement; - $property = $this->entityLookup->getEntity( $propertyId ); + $propertyLabel = $this->getPropertyLabel( $propertyId, $langCode ); - if ( $property !== null ) { - $propertyLabel = $property->getLabel( $langCode ); - - if ( $propertyLabel !== false ) { - $id = $property->getPrefixedId(); - $propertyList[$id] = $propertyLabel; - } + if ( $propertyLabel !== false ) { + $id = $propertyId->getPrefixedId(); + $propertyList[$id] = $propertyLabel; } } @@ -184,6 +181,10 @@ * @return EntityId|null */ protected function getPropertyIdByLabel( $propertyLabel, $langCode ) { + if ( $this->propertiesByLabel === null ) { + return null; + } + wfProfileIn( __METHOD__ ); $propertyId = array_search( $propertyLabel, $this->propertiesByLabel[$langCode] ); if ( $propertyId !== false ) { @@ -212,11 +213,17 @@ $snakList = new SnakList(); if ( defined( 'WB_EXPERIMENTAL_FEATURES' ) && WB_EXPERIMENTAL_FEATURES ) { - if ( $this->propertiesByLabel === null ) { + $propertyId = $this->getPropertyIdByLabel( $propertyLabel, $langCode ); + + if ( $propertyId === null ) { + //NOTE: No negative caching. If we are looking up a label that can't be found + // in the item, we'll always try to re-index the item's properties. + // We just hope that this is rare, because people notice when a label + // doesn't work. $this->indexPropertiesByLabel( $entityId, $langCode ); + $propertyId = $this->getPropertyIdByLabel( $propertyLabel, $langCode ); } - $propertyId = $this->getPropertyIdByLabel( $propertyLabel, $langCode ); $snakList = $propertyId !== null ? $this->getSnakListForProperty( $propertyId ) : $snakList; } diff --git a/lib/tests/phpunit/store/sql/PropertySQLLookupTest.php b/lib/tests/phpunit/store/sql/PropertySQLLookupTest.php index b1ae5b5..1c8596e 100644 --- a/lib/tests/phpunit/store/sql/PropertySQLLookupTest.php +++ b/lib/tests/phpunit/store/sql/PropertySQLLookupTest.php @@ -44,8 +44,14 @@ */ class PropertySQLLookupTest extends \MediaWikiTestCase { + /** + * @var \Wikibase\EntityLookup + */ protected $entityLookup; + /** + * @var \Wikibase\PropertySQLLookup + */ protected $propertyLookup; public function getPropertyData() { @@ -97,8 +103,8 @@ return $properties; } - public function getItem() { - $snakList = new SnakList(); + public function getItems() { + $items = array(); $properties = $this->getPropertyData(); @@ -107,8 +113,7 @@ array( 'property' => clone $properties[0], 'value' => new EntityId( Item::ENTITY_TYPE, 44 ) ), array( 'property' => clone $properties[1], 'value' => new EntityId( Item::ENTITY_TYPE, 45 ) ), array( 'property' => clone $properties[2], 'value' => new StringValue( 'Flag of Canada.svg' ) ), - array( 'property' => clone $properties[3], 'value' => new StringValue( 'CA' ) ), - array( 'property' => clone $properties[4], 'value' => new EntityId( Item::ENTITY_TYPE, 46 ) ) + array( 'property' => clone $properties[4], 'value' => new EntityId( Item::ENTITY_TYPE, 46 ) ), ); $statements = array(); @@ -133,7 +138,26 @@ $item->setClaims( $claims ); - return $item; + $items[] = $item; + + // ------------- + $item = $item->copy(); + + $itemId = new EntityId( Item::ENTITY_TYPE, 128 ); + $item->setId( $itemId ); + $item->setLabel( 'en', 'Nanada' ); + + $statement = new Statement( + new \Wikibase\PropertyNoValueSnak( $properties[3]->getId() ) + ); + + $claims = new Claims(); + $claims->addClaim( $statement ); + $item->setClaims( $claims ); + + $items[] = $item; + + return $items; } public function setUp() { @@ -147,7 +171,11 @@ $this->entityLookup->putEntity( $property ); } - $this->entityLookup->putEntity( $this->getItem() ); + $items = $this->getItems(); + + foreach ( $items as $item ) { + $this->entityLookup->putEntity( $item ); + } $this->propertyLookup = new PropertySQLLookup( $this->entityLookup ); } @@ -158,12 +186,14 @@ } public function getMainSnaksByPropertyLabelProvider() { - $entityId = new EntityId( Item::ENTITY_TYPE, 126 ); + $entity126 = new EntityId( Item::ENTITY_TYPE, 126 ); + $entity128 = new EntityId( Item::ENTITY_TYPE, 128 ); return array( - array( $entityId, 'capital', 'en', 2 ), - array( $entityId, 'currency', 'en', 1 ), - array( $entityId, 'president', 'en', 0 ) + array( $entity126, 'capital', 'en', 2 ), + array( $entity126, 'currency', 'en', 1 ), + array( $entity126, 'president', 'en', 0 ), + array( $entity128, 'country code', 'en', 1 ) ); } @@ -171,12 +201,36 @@ * @dataProvider getMainSnaksByPropertyLabelProvider */ public function testGetMainSnaksByPropertyLabel( $entityId, $propertyLabel, $langCode, $expected ) { + if ( !defined( 'WB_EXPERIMENTAL_FEATURES' ) || !WB_EXPERIMENTAL_FEATURES ) { + $this->markTestSkipped( "getMainSnaksByPropertyLabel is experimental" ); + } + $snakList = $this->propertyLookup->getMainSnaksByPropertyLabel( $entityId, $propertyLabel, $langCode ); $this->assertInstanceOf( '\Wikibase\SnakList', $snakList ); $this->assertEquals( $expected, $snakList->count() ); } + public function testGetMainSnaksByPropertyLabel2( ) { + if ( !defined( 'WB_EXPERIMENTAL_FEATURES' ) || !WB_EXPERIMENTAL_FEATURES ) { + $this->markTestSkipped( "getMainSnaksByPropertyLabel is experimental" ); + } + + $entity126 = new EntityId( Item::ENTITY_TYPE, 126 ); + + $snakList = $this->propertyLookup->getMainSnaksByPropertyLabel( $entity126, 'capital', 'en' ); + $this->assertEquals( 2, $snakList->count() ); + + $snakList = $this->propertyLookup->getMainSnaksByPropertyLabel( $entity126, 'country code', 'en' ); + $this->assertEquals( 0, $snakList->count() ); + + // try to find a property in another entity, if that property wasn't used by the previous entity. + $entity128 = new EntityId( Item::ENTITY_TYPE, 128 ); + + $snakList = $this->propertyLookup->getMainSnaksByPropertyLabel( $entity128, 'country code', 'en' ); + $this->assertEquals( 1, $snakList->count(), "property unknown to the first item" ); + } + public function getPropertyLabelProvider() { return array( array( 'capital', 'en', new EntityId( Property::ENTITY_TYPE, 4 ) ), -- To view, visit https://gerrit.wikimedia.org/r/56271 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: If26f0d5a80a3af1f61c7a6c9951e15a8c38e37f4 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: Daniel Kinzler <[email protected]> Gerrit-Reviewer: Aude <[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
