Daniel Kinzler has uploaded a new change for review.
https://gerrit.wikimedia.org/r/88770
Change subject: Make the EntityId in Claim GUIDs case-insensitive.
......................................................................
Make the EntityId in Claim GUIDs case-insensitive.
Change-Id: Ifd8f90c7a8dc8691cade2b3826d41a8200b17063
---
M DataModel/Claim/Claim.php
M DataModel/Claim/Claims.php
M tests/phpunit/Claim/ClaimTest.php
M tests/phpunit/Claim/ClaimsTest.php
M tests/phpunit/Claim/StatementTest.php
M tests/phpunit/Entity/EntityTest.php
M tests/phpunit/EntityDiffTest.php
7 files changed, 224 insertions(+), 33 deletions(-)
git pull
ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/WikibaseDataModel
refs/changes/70/88770/1
diff --git a/DataModel/Claim/Claim.php b/DataModel/Claim/Claim.php
index d6e3daa..32e10b6 100644
--- a/DataModel/Claim/Claim.php
+++ b/DataModel/Claim/Claim.php
@@ -3,6 +3,7 @@
namespace Wikibase;
use InvalidArgumentException;
+use Wikibase\DataModel\Claim\ClaimGuid;
/**
* Class that represents a single Wikibase claim.
@@ -153,12 +154,17 @@
* @param string|null $guid
*
* @throws InvalidArgumentException
+ * @todo: use/allow ClaimGuid object
*/
public function setGuid( $guid ) {
if ( !is_string( $guid ) && $guid !== null ) {
throw new InvalidArgumentException( 'Can only set the
GUID to string values or null' );
}
+ if ( $guid !== null && strpos( $guid, ClaimGuid::SEPARATOR )
=== false ) {
+ throw new InvalidArgumentException( 'Malformed GUID: '
. $guid );
+ }
+
$this->guid = $guid;
}
diff --git a/DataModel/Claim/Claims.php b/DataModel/Claim/Claims.php
index b0eb29d..e90ff69 100644
--- a/DataModel/Claim/Claims.php
+++ b/DataModel/Claim/Claims.php
@@ -8,6 +8,7 @@
use Diff\DiffOpRemove;
use Diff\MapDiffer;
use InvalidArgumentException;
+use Wikibase\DataModel\Claim\ClaimGuid;
/**
* Implementation of the Claims interface.
@@ -107,7 +108,8 @@
* @return boolean
*/
public function hasClaimWithGuid( $claimGuid ) {
- return array_key_exists( $claimGuid, $this->guidIndex );
+ $key = $this->normalizeGuidKey( $claimGuid );
+ return array_key_exists( $key, $this->guidIndex );
}
/**
@@ -118,7 +120,8 @@
* @param string $claimGuid
*/
public function removeClaimWithGuid( $claimGuid ) {
- $this->offsetUnset( $this->guidIndex[$claimGuid] );
+ $key = $this->normalizeGuidKey( $claimGuid );
+ $this->offsetUnset( $this->guidIndex[$key] );
}
/**
@@ -132,7 +135,8 @@
*/
public function getClaimWithGuid( $claimGuid ) {
if ( $this->hasClaimWithGuid( $claimGuid ) ) {
- return $this->offsetGet( $this->guidIndex[$claimGuid] );
+ $key = $this->normalizeGuidKey( $claimGuid );
+ return $this->offsetGet( $this->guidIndex[$key] );
}
else {
return null;
@@ -195,6 +199,45 @@
}
/**
+ * @param string|null $key
+ *
+ * @return null|string
+ *
+ * @throws \InvalidArgumentException
+ */
+ private function normalizeGuidKey( $key ) {
+ if ( $key === null ) {
+ return null;
+ }
+
+ //TODO: accept ClaimGuid objects here and throughout the
ClaimListAccess interface
+
+ if ( !is_string( $key ) ) {
+ throw new InvalidArgumentException( 'GUID must be a
string' );
+ }
+
+ $keyParts = explode( ClaimGuid::SEPARATOR, $key );
+
+ if ( count( $keyParts ) !== 2 ) {
+ throw new InvalidArgumentException( 'Malformed Claim
GUID: ' . $key );
+ }
+
+ //NOTE: We could use an EntityIdParser to construct and
EntityId object,
+ // and then use $id->getPrefixedId() to get the prefix.
+ // But that seems overkill, strtoupper should do.
+ //TODO: normalize $keyParts[1] as well, but consider the
implications.
+ $key = strtoupper( $keyParts[0] ) . ClaimGuid::SEPARATOR .
$keyParts[1];
+
+ return $key;
+ }
+
+ private function getGuidKey( Claim $claim ) {
+ $key = $claim->getGuid();
+ $key = $this->normalizeGuidKey( $key );
+ return $key;
+ }
+
+ /**
* @see GenericArrayObject::preSetElement
*
* @since 0.3
@@ -208,7 +251,11 @@
$shouldSet = parent::preSetElement( $index, $claim );
if ( $shouldSet ) {
- $this->guidIndex[$claim->getGuid()] = $index;
+ $key = $this->getGuidKey( $claim );
+
+ if ( $key !== null ) {
+ $this->guidIndex[$key] = $index;
+ }
}
return $shouldSet;
@@ -228,7 +275,12 @@
*/
$claim = $this->offsetGet( $index );
- unset( $this->guidIndex[$claim->getGuid()] );
+ $key = $this->getGuidKey( $claim );
+
+ if ( $key !== null ) {
+ unset( $this->guidIndex[$key] );
+ }
+
parent::offsetUnset( $index );
}
}
@@ -257,11 +309,17 @@
* @var Claim $claim
*/
foreach ( $this as $claim ) {
- $sourceHashes[$claim->getGuid()] = $claim->getHash();
+ $key = $this->getGuidKey( $claim );
+ assert( $key !== null );
+
+ $sourceHashes[$key] = $claim->getHash();
}
foreach ( $claims as $claim ) {
- $targetHashes[$claim->getGuid()] = $claim->getHash();
+ $key = $this->getGuidKey( $claim );
+ assert( $key !== null );
+
+ $targetHashes[$key] = $claim->getHash();
}
$diff = new \Diff\Diff( array(), true );
@@ -273,19 +331,24 @@
assert( $oldClaim instanceof Claim );
assert( $newClaim instanceof Claim );
- assert( $oldClaim->getGuid() ===
$newClaim->getGuid() );
+ assert( $this->getGuidKey( $oldClaim ) ===
$this->getGuidKey( $newClaim ) );
- $diff[$oldClaim->getGuid()] = new DiffOpChange(
$oldClaim, $newClaim );
+ $key = $this->getGuidKey( $oldClaim );
+ $diff[$key] = new DiffOpChange( $oldClaim,
$newClaim );
}
elseif ( $diffOp instanceof DiffOpAdd ) {
$claim = $claims->getByElementHash(
$diffOp->getNewValue() );
assert( $claim instanceof Claim );
- $diff[$claim->getGuid()] = new DiffOpAdd(
$claim );
+
+ $key = $this->getGuidKey( $claim );
+ $diff[$key] = new DiffOpAdd( $claim );
}
elseif ( $diffOp instanceof DiffOpRemove ) {
$claim = $this->getByElementHash(
$diffOp->getOldValue() );
assert( $claim instanceof Claim );
- $diff[$claim->getGuid()] = new DiffOpRemove(
$claim );
+
+ $key = $this->getGuidKey( $claim );
+ $diff[$key] = new DiffOpRemove( $claim );
}
else {
throw new InvalidArgumentException( 'Invalid
DiffOp type cannot be handled' );
diff --git a/tests/phpunit/Claim/ClaimTest.php
b/tests/phpunit/Claim/ClaimTest.php
index ced36a2..2593a55 100644
--- a/tests/phpunit/Claim/ClaimTest.php
+++ b/tests/phpunit/Claim/ClaimTest.php
@@ -142,8 +142,8 @@
* @dataProvider instanceProvider
*/
public function testSetGuid( Claim $claim ) {
- $claim->setGuid( 'foo-bar-baz' );
- $this->assertEquals( 'foo-bar-baz', $claim->getGuid() );
+ $claim->setGuid( 'TEST$foo-bar-baz' );
+ $this->assertEquals( 'TEST$foo-bar-baz', $claim->getGuid() );
}
/**
@@ -154,8 +154,8 @@
$this->assertTrue( $guid === null || is_string( $guid ) );
$this->assertEquals( $guid, $claim->getGuid() );
- $claim->setGuid( 'foobar' );
- $this->assertEquals( 'foobar', $claim->getGuid() );
+ $claim->setGuid( 'TEST$foobar' );
+ $this->assertEquals( 'TEST$foobar', $claim->getGuid() );
}
/**
@@ -182,10 +182,10 @@
public function testGetHashStability() {
$claim0 = new Claim( new PropertyNoValueSnak( 42 ) );
- $claim0->setGuid( 'claim0' );
+ $claim0->setGuid( 'TEST$claim0' );
$claim1 = new Claim( new PropertyNoValueSnak( 42 ) );
- $claim1->setGuid( 'claim1' );
+ $claim1->setGuid( 'TEST$claim1' );
$this->assertEquals( $claim0->getHash(), $claim1->getHash() );
}
diff --git a/tests/phpunit/Claim/ClaimsTest.php
b/tests/phpunit/Claim/ClaimsTest.php
index 016d08b..789b385 100644
--- a/tests/phpunit/Claim/ClaimsTest.php
+++ b/tests/phpunit/Claim/ClaimsTest.php
@@ -9,7 +9,8 @@
use Diff\DiffOpRemove;
use Wikibase\Claim;
use Wikibase\Claims;
-use Wikibase\EntityId;
+use Wikibase\DataModel\Claim\ClaimGuid;
+use Wikibase\DataModel\Entity\PropertyId;
use Wikibase\Property;
use Wikibase\PropertyNoValueSnak;
use Wikibase\PropertySomeValueSnak;
@@ -34,6 +35,7 @@
*
* @licence GNU GPL v2+
* @author Jeroen De Dauw < jeroended...@gmail.com >
+ * @author Daniel Kinzler
*/
class ClaimsTest extends \PHPUnit_Framework_TestCase {
@@ -58,16 +60,23 @@
$instances[] = new Claim(
new PropertyNoValueSnak(
- new EntityId( Property::ENTITY_TYPE, 23 ) ) );
+ new PropertyId( 'P23' ) ) );
$instances[] = new Claim(
new PropertySomeValueSnak(
- new EntityId( Property::ENTITY_TYPE, 42 ) ) );
+ new PropertyId( 'P42' ) ) );
$instances[] = new Claim(
new PropertyValueSnak(
- new EntityId( Property::ENTITY_TYPE, 42 ),
+ new PropertyId( 'P42' ),
new StringValue( "foo" ) ) );
+
+ $i = 0;
+ foreach ( $instances as $instance ) {
+ $i++;
+ $guid = 'x' . $i . '$' . '7' . $i . 'a';
+ $instance->setGuid( $guid );
+ }
return $instances;
}
@@ -93,6 +102,114 @@
$this->assertTrue( $array->hasClaim( $hashable ) );
$array->removeClaim( $hashable );
$this->assertFalse( $array->hasClaim( $hashable ) );
+ }
+
+ $this->assertTrue( true );
+ }
+
+ /**
+ * @dataProvider instanceProvider
+ *
+ * @param \Wikibase\Claims $array
+ */
+ public function testGetGuids( Claims $array ) {
+ $guids = $array->getGuids();
+
+ $this->assertEquals( count( $array ), count( $guids ), 'Number
of GUIDs' );
+
+ /**
+ * @var Claim $hashable
+ */
+ foreach ( $guids as $guid ) {
+ $claim = $array->getClaimWithGuid( $guid );
+ $this->assertEquals( strtolower( $guid ), strtolower(
$claim->getGuid() ) );
+ }
+
+ $this->assertTrue( true );
+ }
+
+ /**
+ * @dataProvider instanceProvider
+ *
+ * @param \Wikibase\Claims $array
+ */
+ public function testHasClaimWithGuid( Claims $array ) {
+ /**
+ * @var Claim $hashable
+ */
+ foreach ( iterator_to_array( $array ) as $hashable ) {
+ $guidParts = explode( ClaimGuid::SEPARATOR,
$hashable->getGuid() );
+ $upperGuid = strtoupper( $guidParts[0] ) .
ClaimGuid::SEPARATOR . $guidParts[1];
+ $lowerGuid = strtolower( $guidParts[0] ) .
ClaimGuid::SEPARATOR . $guidParts[1];
+
+ $this->assertTrue( $array->hasClaimWithGuid( $lowerGuid
) );
+ $this->assertTrue( $array->hasClaimWithGuid( $upperGuid
) );
+ $array->removeClaim( $hashable );
+ $this->assertFalse( $array->hasClaimWithGuid(
$lowerGuid ) );
+ $this->assertFalse( $array->hasClaimWithGuid(
$upperGuid ) );
+ }
+
+ $this->assertTrue( true );
+ }
+
+ /**
+ * @dataProvider instanceProvider
+ *
+ * @param \Wikibase\Claims $array
+ */
+ public function testRemoveClaimWithGuid( Claims $array ) {
+ /**
+ * @var Claim $hashable
+ */
+ foreach ( iterator_to_array( $array ) as $hashable ) {
+ $guidParts = explode( ClaimGuid::SEPARATOR,
$hashable->getGuid() );
+ $upperGuid = strtoupper( $guidParts[0] ) .
ClaimGuid::SEPARATOR . $guidParts[1];
+ $lowerGuid = strtolower( $guidParts[0] ) .
ClaimGuid::SEPARATOR . $guidParts[1];
+
+ // check with upper case
+ $array->removeClaimWithGuid( $upperGuid );
+ $this->assertFalse( $array->hasClaimWithGuid(
$lowerGuid ) );
+ $this->assertFalse( $array->hasClaimWithGuid(
$upperGuid ) );
+ $this->assertFalse( $array->hasClaim( $hashable ) );
+
+ // check with lower case
+ $array->addClaim( $hashable );
+ $this->assertTrue( $array->hasClaimWithGuid( $lowerGuid
) );
+
+ $array->removeClaimWithGuid( $lowerGuid );
+ $this->assertFalse( $array->hasClaimWithGuid(
$lowerGuid ) );
+ $this->assertFalse( $array->hasClaimWithGuid(
$upperGuid ) );
+ $this->assertFalse( $array->hasClaim( $hashable ) );
+ }
+
+ $this->assertTrue( true );
+ }
+
+ /**
+ * @dataProvider instanceProvider
+ *
+ * @param \Wikibase\Claims $array
+ */
+ public function testGetClaimWithGuid( Claims $array ) {
+ /**
+ * @var Claim $hashable
+ */
+ foreach ( iterator_to_array( $array ) as $hashable ) {
+ $guidParts = explode( ClaimGuid::SEPARATOR,
$hashable->getGuid() );
+ $upperGuid = strtoupper( $guidParts[0] ) .
ClaimGuid::SEPARATOR . $guidParts[1];
+ $lowerGuid = strtolower( $guidParts[0] ) .
ClaimGuid::SEPARATOR . $guidParts[1];
+
+ $lowerClaim = $array->getClaimWithGuid( $lowerGuid );
+ $upperClaim = $array->getClaimWithGuid( $upperGuid );
+
+ $this->assertNotNull( $lowerClaim );
+ $this->assertSame( $lowerClaim, $upperClaim );
+ $this->assertEquals( strtolower( $lowerClaim->getGuid()
), $lowerGuid );
+
+ $array->removeClaim( $hashable );
+
+ $this->assertNull( $array->getClaimWithGuid( $lowerGuid
) );
+ $this->assertNull( $array->getClaimWithGuid( $upperGuid
) );
}
$this->assertTrue( true );
@@ -206,11 +323,13 @@
new SnakList( array( new PropertyValueSnak( 10, new
StringValue( 'spam' ) ) ) )
) ) ) );
- $claim0->setGuid( 'claim0' );
- $claim1->setGuid( 'claim1' );
- $claim2->setGuid( 'claim2' );
- $statement1->setGuid( 'statement1' );
- $statement0->setGuid( 'statement0' );
+ $claim0->setGuid( 'X$claim0' );
+ $claim1->setGuid( 'X$claim1' );
+ $claim2->setGuid( 'X$claim2' );
+ $claim3->setGuid( 'X$claim3' );
+ $claim4->setGuid( 'X$claim4' );
+ $statement1->setGuid( 'X$statement1' );
+ $statement0->setGuid( 'X$statement0' );
$claim2v2 = unserialize( serialize( $claim2 ) );
$claim2v2->setMainSnak( new PropertyValueSnak( 42, new
StringValue( 'omnomnom' ) ) );
diff --git a/tests/phpunit/Claim/StatementTest.php
b/tests/phpunit/Claim/StatementTest.php
index 127e6f1..b7a25b5 100644
--- a/tests/phpunit/Claim/StatementTest.php
+++ b/tests/phpunit/Claim/StatementTest.php
@@ -166,11 +166,11 @@
public function testGetHash() {
$claim0 = new Statement( new PropertyNoValueSnak( 42 ) );
- $claim0->setGuid( 'claim0' );
+ $claim0->setGuid( 'TEST$claim0' );
$claim0->setRank( Claim::RANK_DEPRECATED );
$claim1 = new Statement( new PropertyNoValueSnak( 42 ) );
- $claim1->setGuid( 'claim1' );
+ $claim1->setGuid( 'TEST$claim1' );
$claim1->setRank( Claim::RANK_DEPRECATED );
$this->assertEquals( $claim0->getHash(), $claim1->getHash() );
diff --git a/tests/phpunit/Entity/EntityTest.php
b/tests/phpunit/Entity/EntityTest.php
index 71ab54a..50819b0 100644
--- a/tests/phpunit/Entity/EntityTest.php
+++ b/tests/phpunit/Entity/EntityTest.php
@@ -793,10 +793,10 @@
$claim2 = new Claim( new PropertyValueSnak( 42, new
StringValue( 'ohi' ) ) );
$claim3 = new Claim( new PropertyNoValueSnak( 1 ) );
- $claim0->setGuid( 'claim0' );
- $claim1->setGuid( 'claim1' );
- $claim2->setGuid( 'claim2' );
- $claim3->setGuid( 'claim3' );
+ $claim0->setGuid( 'TEST$claim0' );
+ $claim1->setGuid( 'TEST$claim1' );
+ $claim2->setGuid( 'TEST$claim2' );
+ $claim3->setGuid( 'TEST$claim3' );
$argLists = array();
diff --git a/tests/phpunit/EntityDiffTest.php b/tests/phpunit/EntityDiffTest.php
index 6280237..aad9c02 100644
--- a/tests/phpunit/EntityDiffTest.php
+++ b/tests/phpunit/EntityDiffTest.php
@@ -85,7 +85,10 @@
$diffs[] = new EntityDiff( $diffOps );
- $claims = new \Wikibase\Claims( array( new \Wikibase\Claim( new
\Wikibase\PropertyNoValueSnak( 42 ) ) ) );
+ $claim = new \Wikibase\Claim( new
\Wikibase\PropertyNoValueSnak( 42 ) );
+ $claim->setGuid( 'q17$46abc' );
+
+ $claims = new \Wikibase\Claims( array( $claim ) );
$diffOps['claim'] = $claims->getDiff( new \Wikibase\Claims() );
--
To view, visit https://gerrit.wikimedia.org/r/88770
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifd8f90c7a8dc8691cade2b3826d41a8200b17063
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/WikibaseDataModel
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <daniel.kinz...@wikimedia.de>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
