Jeroen De Dauw has submitted this change and it was merged. Change subject: (bug #55569) Make Claims list not use hashes. ......................................................................
(bug #55569) Make Claims list not use hashes. Claim hashes are not unique and should only be used as a way to check for equivalence. Thus, Claims should nto be a HashArray. Change-Id: Idd5ffb72774a948852a6dbe2e05700b9aa94a211 --- M DataModel/Claim/ClaimListAccess.php M DataModel/Claim/Claims.php M DataModel/Entity/Entity.php M tests/phpunit/Claim/ClaimAggregateTest.php M tests/phpunit/Claim/ClaimListAccessTest.php M tests/phpunit/Claim/ClaimsTest.php M tests/phpunit/Entity/EntityTest.php M tests/phpunit/Entity/ItemTest.php M tests/phpunit/EntityDiffTest.php 9 files changed, 714 insertions(+), 214 deletions(-) Approvals: Jeroen De Dauw: Looks good to me, approved jenkins-bot: Verified diff --git a/DataModel/Claim/ClaimListAccess.php b/DataModel/Claim/ClaimListAccess.php index 9c0a20b..c4d7350 100644 --- a/DataModel/Claim/ClaimListAccess.php +++ b/DataModel/Claim/ClaimListAccess.php @@ -16,7 +16,8 @@ interface ClaimListAccess { /** - * Adds the provided claims to the list. + * Adds the provided claims to the list. If a claim with the same GUID is + * already in the list, it is replaced. * * @since 0.2 * @@ -25,7 +26,7 @@ public function addClaim( Claim $claim ); /** - * Returns if the list contains a claim with the same hash as the provided claim. + * Returns if the list contains a claim with the same GUID as the provided claim. * * @since 0.2 * @@ -36,7 +37,8 @@ public function hasClaim( Claim $claim ); /** - * Removes the claim with the same hash as the provided claim if such a claim exists in the list. + * Removes the claim with the same GUID as the provided claim if such a claim exists in the list. + * If the claim is not in the list, the call has no effect. * * @since 0.2 * diff --git a/DataModel/Claim/Claims.php b/DataModel/Claim/Claims.php index b0eb29d..a48e86a 100644 --- a/DataModel/Claim/Claims.php +++ b/DataModel/Claim/Claims.php @@ -2,11 +2,14 @@ namespace Wikibase; +use ArrayObject; +use Diff\Diff; use Diff\Differ; use Diff\DiffOpAdd; use Diff\DiffOpChange; use Diff\DiffOpRemove; use Diff\MapDiffer; +use Hashable; use InvalidArgumentException; /** @@ -14,9 +17,6 @@ * @see Claims * * A claim (identified using it's GUID) can only be added once. - * - * TODO: if it turns out we do not need efficient lookups by guid after all - * we can switch back to extending the simpler HashableObjectStorage. * * @since 0.1 * @@ -27,28 +27,31 @@ * @author Jeroen De Dauw < jeroended...@gmail.com > * @author Daniel Kinzler */ -class Claims extends HashArray implements ClaimListAccess { - - /** - * Maps claim GUIDs to their offsets. - * - * @since 0.1 - * - * @var array [ element hash (string) => element offset (string|int) ] - */ - protected $guidIndex = array(); +class Claims extends ArrayObject implements ClaimListAccess, Hashable { /** * Constructor. + * * @see GenericArrayObject::__construct * * @since 0.3 * * @param null|array $input + * + * @throws \InvalidArgumentException */ public function __construct( $input = null ) { - $this->acceptDuplicates = true; - parent::__construct( $input ); + parent::__construct( array() ); + + if ( $input !== null ) { + if ( !is_array( $input) && !( $input instanceof \Traversable ) ) { + throw new InvalidArgumentException( '$input must be traversable' ); + } + + foreach ( $input as $claim ) { + $this->append( $claim ); + } + } } /** @@ -63,6 +66,45 @@ } /** + * @since 0.5 + * + * @param string $guid + * + * @throws \InvalidArgumentException + * @return string + */ + protected function getGuidKey( $guid ) { + if ( !is_string( $guid ) ) { + throw new InvalidArgumentException( 'Expected a GUID string' ); + } + + $key = strtoupper( $guid ); + return $key; + } + + /** + * @param Claim $claim + * + * @since 0.5 + * + * + * @param Claim $claim + * + * @throws \InvalidArgumentException + * @return string + */ + protected function getClaimKey( Claim $claim ) { + $guid = $claim->getGuid(); + + if ( $guid === null ) { + throw new InvalidArgumentException( 'Can\'t handle claims with no GUID set!' ); + } + + $key = $this->getGuidKey( $guid ); + return $key; + } + + /** * @see ClaimListAccess::addClaim * * @since 0.1 @@ -70,7 +112,25 @@ * @param Claim $claim */ public function addClaim( Claim $claim ) { - $this->append( $claim ); + $key = $this->getClaimKey( $claim ); + $this->offsetSet( $key, $claim ); + } + + /** + * @see ArrayAccess::append + * + * @since 0.5 + * + * @param Claim $claim + * + * @throws \InvalidArgumentException + */ + public function append( $claim ) { + if ( !( $claim instanceof Claim ) ) { + throw new InvalidArgumentException( '$claim must be a Claim instances' ); + } + + $this->addClaim( $claim ); } /** @@ -83,7 +143,14 @@ * @return boolean */ public function hasClaim( Claim $claim ) { - return $this->hasElement( $claim ); + $guid = $claim->getGuid(); + + if ( $guid === null ) { + return false; + } + + $key = $this->getGuidKey( $guid ); + return $this->offsetExists( $key ); } /** @@ -94,7 +161,17 @@ * @param Claim $claim */ public function removeClaim( Claim $claim ) { - $this->removeElement( $claim ); + $guid = $claim->getGuid(); + + if ( $guid === null ) { + return; + } + + $key = $this->getGuidKey( $guid ); + + if ( $this->offsetExists( $key ) ) { + $this->offsetUnset( $key ); + } } /** @@ -107,7 +184,7 @@ * @return boolean */ public function hasClaimWithGuid( $claimGuid ) { - return array_key_exists( $claimGuid, $this->guidIndex ); + return $this->offsetExists( $claimGuid ); } /** @@ -118,7 +195,9 @@ * @param string $claimGuid */ public function removeClaimWithGuid( $claimGuid ) { - $this->offsetUnset( $this->guidIndex[$claimGuid] ); + if ( $this->offsetExists( $claimGuid ) ) { + $this->offsetUnset( $claimGuid ); + } } /** @@ -131,12 +210,75 @@ * @return Claim|null */ public function getClaimWithGuid( $claimGuid ) { - if ( $this->hasClaimWithGuid( $claimGuid ) ) { - return $this->offsetGet( $this->guidIndex[$claimGuid] ); - } - else { + if ( $this->offsetExists( $claimGuid ) ) { + return $this->offsetGet( $claimGuid ); + } else { return null; } + } + + /** + * @see ArrayAccess::offsetExists + * + * @param string $guid + * + * @return bool + * + * @throws \InvalidArgumentException + */ + public function offsetExists( $guid ) { + $key = $this->getGuidKey( $guid ); + return parent::offsetExists( $key ); + } + + /** + * @see ArrayAccess::offsetGet + * + * @param string $guid + * + * @return Claim + * + * @throws \InvalidArgumentException + */ + public function offsetGet( $guid ) { + $key = $this->getGuidKey( $guid ); + return parent::offsetGet( $key ); + } + + /** + * @see ArrayAccess::offsetSet + * + * @param string $guid + * @param Claim $claim + * + * @throws \InvalidArgumentException + */ + public function offsetSet( $guid, $claim ) { + if ( !is_object( $claim ) || !( $claim instanceof Claim ) ) { + throw new InvalidArgumentException( 'Expected a Claim instance' ); + } + + $claimKey = $this->getClaimKey( $claim ); + + if ( $guid !== null ) { + $guidKey = $this->getGuidKey( $guid ); + + if ( $guidKey !== $claimKey ) { + throw new InvalidArgumentException( 'The key must be the claim\'s GUID.' ); + } + } + + parent::offsetSet( $claimKey, $claim ); + } + + /** + * @see ArrayAccess::offsetUnset + * + * @param string $guid + */ + public function offsetUnset( $guid ) { + $key = $this->getGuidKey( $guid ); + parent::offsetUnset( $key ); } /** @@ -147,7 +289,9 @@ * @return string[] */ public function getGuids() { - return array_keys( $this->guidIndex ); + return array_map( function ( Claim $claim ) { + return $claim->getGuid(); + }, iterator_to_array( $this ) ); } /** @@ -188,104 +332,71 @@ /* @var Claim $claim */ foreach ( $this as $claim ) { - $snaks[] = $claim->getMainSnak(); + $guid = $claim->getGuid(); + $snaks[$guid] = $claim->getMainSnak(); } return $snaks; } /** - * @see GenericArrayObject::preSetElement + * Returns a map from GUIDs to claim hashes. * - * @since 0.3 + * @since 0.4 * - * @param int|string $index - * @param Claim $claim - * - * @return boolean + * @return string[] */ - protected function preSetElement( $index, $claim ) { - $shouldSet = parent::preSetElement( $index, $claim ); + public function getHashes() { + $snaks = array(); - if ( $shouldSet ) { - $this->guidIndex[$claim->getGuid()] = $index; + /* @var Claim $claim */ + foreach ( $this as $claim ) { + $guid = $claim->getGuid(); + $snaks[$guid] = $claim->getHash(); } - return $shouldSet; - } - - /** - * @see ArrayObject::offsetUnset() - * - * @since 0.3 - * - * @param mixed $index - */ - public function offsetUnset( $index ) { - if ( $this->offsetExists( $index ) ) { - /** - * @var Claim $claim - */ - $claim = $this->offsetGet( $index ); - - unset( $this->guidIndex[$claim->getGuid()] ); - parent::offsetUnset( $index ); - } + return $snaks; } /** * @since 0.4 * * @param Claims $claims - * @param Differ|null $differ + * @param Differ|null $differ for building a diff of two GUID-to-hash maps. * - * @return \Diff\Diff + * @return Diff * @throws InvalidArgumentException */ public function getDiff( Claims $claims, Differ $differ = null ) { - assert( $this->indicesAreUpToDate() ); - assert( $claims->indicesAreUpToDate() ); - if ( $differ === null ) { $differ = new MapDiffer(); } - $sourceHashes = array(); - $targetHashes = array(); + $sourceHashes = $this->getHashes(); + $targetHashes = $claims->getHashes(); - /** - * @var Claim $claim - */ - foreach ( $this as $claim ) { - $sourceHashes[$claim->getGuid()] = $claim->getHash(); - } + $diff = new Diff( array(), true ); - foreach ( $claims as $claim ) { - $targetHashes[$claim->getGuid()] = $claim->getHash(); - } - - $diff = new \Diff\Diff( array(), true ); - - foreach ( $differ->doDiff( $sourceHashes, $targetHashes ) as $diffOp ) { + foreach ( $differ->doDiff( $sourceHashes, $targetHashes ) as $guid => $diffOp ) { if ( $diffOp instanceof DiffOpChange ) { - $oldClaim = $this->getByElementHash( $diffOp->getOldValue() ); - $newClaim = $claims->getByElementHash( $diffOp->getNewValue() ); + $oldClaim = $this->getClaimWithGuid( $guid ); + $newClaim = $claims->getClaimWithGuid( $guid ); assert( $oldClaim instanceof Claim ); assert( $newClaim instanceof Claim ); assert( $oldClaim->getGuid() === $newClaim->getGuid() ); - $diff[$oldClaim->getGuid()] = new DiffOpChange( $oldClaim, $newClaim ); + $diff[$guid] = new DiffOpChange( $oldClaim, $newClaim ); } elseif ( $diffOp instanceof DiffOpAdd ) { - $claim = $claims->getByElementHash( $diffOp->getNewValue() ); + $claim = $claims->getClaimWithGuid( $guid ); assert( $claim instanceof Claim ); - $diff[$claim->getGuid()] = new DiffOpAdd( $claim ); + $diff[$guid] = new DiffOpAdd( $claim ); } elseif ( $diffOp instanceof DiffOpRemove ) { - $claim = $this->getByElementHash( $diffOp->getOldValue() ); + $claim = $this->getClaimWithGuid( $guid ); assert( $claim instanceof Claim ); - $diff[$claim->getGuid()] = new DiffOpRemove( $claim ); + $diff[$guid] = new DiffOpRemove( $claim ); } else { throw new InvalidArgumentException( 'Invalid DiffOp type cannot be handled' ); @@ -295,4 +406,31 @@ return $diff; } + /** + * Returns a hash based on the value of the object. + * The hash is based on the hashes of the claims contained, + * with the order of claims considered significant. + * + * @since 0.5 + * + * @return string + */ + public function getHash() { + $hash = sha1( '' ); + + /* @var Claim $claim */ + foreach ( $this as $claim ) { + $hash = sha1( $hash . $claim->getHash() ); + } + + return $hash; + } + + /** + * Returns true if this list contains no claims + */ + public function isEmpty() { + $iter = $this->getIterator(); + return !$iter->valid(); + } } diff --git a/DataModel/Entity/Entity.php b/DataModel/Entity/Entity.php index 02d0c94..9fc87f3 100644 --- a/DataModel/Entity/Entity.php +++ b/DataModel/Entity/Entity.php @@ -51,7 +51,7 @@ /** * @since 0.3 * - * @var Claims|null + * @var Claim[]|null */ protected $claims; @@ -715,9 +715,14 @@ * @param Claim $claim */ public function addClaim( Claim $claim ) { + if ( $claim->getGuid() === null ) { + throw new InvalidArgumentException( 'Can\'t add a Claim without a GUID.' ); + } + + // TODO: ensure guid is valid for entity + $this->unstubClaims(); $this->claims[] = $claim; - // TODO: ensure guid is valid for entity } /** diff --git a/tests/phpunit/Claim/ClaimAggregateTest.php b/tests/phpunit/Claim/ClaimAggregateTest.php index 22d8540..90583f6 100644 --- a/tests/phpunit/Claim/ClaimAggregateTest.php +++ b/tests/phpunit/Claim/ClaimAggregateTest.php @@ -43,6 +43,10 @@ $argLists = array(); + foreach ( $claims as $i => $claim ) { + $claim->setGuid( "ClaimListAccessTest\$claim-$i" ); + } + /** * @var ClaimAggregate $aggregate */ diff --git a/tests/phpunit/Claim/ClaimListAccessTest.php b/tests/phpunit/Claim/ClaimListAccessTest.php index ad746f4..a3b9a04 100644 --- a/tests/phpunit/Claim/ClaimListAccessTest.php +++ b/tests/phpunit/Claim/ClaimListAccessTest.php @@ -39,6 +39,10 @@ $argLists = array(); + foreach ( $claims as $i => $claim ) { + $claim->setGuid( "ClaimListAccessTest\$claim-$i" ); + } + /** * @var ClaimListAccess $list */ diff --git a/tests/phpunit/Claim/ClaimsTest.php b/tests/phpunit/Claim/ClaimsTest.php index 016d08b..bf2070d 100644 --- a/tests/phpunit/Claim/ClaimsTest.php +++ b/tests/phpunit/Claim/ClaimsTest.php @@ -7,214 +7,488 @@ use Diff\DiffOpAdd; use Diff\DiffOpChange; use Diff\DiffOpRemove; +use ReflectionClass; use Wikibase\Claim; use Wikibase\Claims; -use Wikibase\EntityId; +use Wikibase\DataModel\Entity\PropertyId; use Wikibase\Property; use Wikibase\PropertyNoValueSnak; use Wikibase\PropertySomeValueSnak; use Wikibase\PropertyValueSnak; use Wikibase\Reference; use Wikibase\ReferenceList; +use Wikibase\Snak; use Wikibase\SnakList; use Wikibase\Statement; /** * @covers Wikibase\Claims * - * @file * @since 0.1 - * - * @ingroup WikibaseLib - * @ingroup Test * * @group Wikibase * @group WikibaseDataModel * @group WikibaseClaim * * @licence GNU GPL v2+ - * @author Jeroen De Dauw < jeroended...@gmail.com > + * @author Daniel Kinzler */ class ClaimsTest extends \PHPUnit_Framework_TestCase { - public function getInstanceClass() { - return '\Wikibase\Claims'; - } + protected $guidCounter = 0; - public function instanceProvider() { - $class = $this->getInstanceClass(); - - $instances = array(); - - foreach ( $this->getConstructorArg() as $arg ) { - $instances[] = array( new $class( $arg ) ); + protected function makeClaim( Snak $mainSnak, $guid = null ) { + if ( $guid === null ) { + $this->guidCounter++; + $guid = 'TEST$claim-' . $this->guidCounter; } - return $instances; + $claim = new Claim( $mainSnak ); + $claim->setGuid( $guid ); + + return $claim; } - public function getElementInstances() { - $instances = array(); + protected function makeStatement( Snak $mainSnak, $guid = null ) { + if ( $guid === null ) { + $this->guidCounter++; + $guid = 'TEST$statement-' . $this->guidCounter; + } - $instances[] = new Claim( - new PropertyNoValueSnak( - new EntityId( Property::ENTITY_TYPE, 23 ) ) ); + $claim = new Statement( $mainSnak ); + $claim->setGuid( $guid ); - $instances[] = new Claim( - new PropertySomeValueSnak( - new EntityId( Property::ENTITY_TYPE, 42 ) ) ); - - $instances[] = new Claim( - new PropertyValueSnak( - new EntityId( Property::ENTITY_TYPE, 42 ), - new StringValue( "foo" ) ) ); - - return $instances; + return $claim; } - public function getConstructorArg() { + /** + * @dataProvider constructorProvider + */ + public function testConstructor() { + $class = new ReflectionClass( 'Wikibase\Claims' ); + $class->newInstanceArgs( func_get_args() ); + } + + public function constructorProvider() { return array( - null, array(), - $this->getElementInstances(), + array( null ), + array( array() ), + array( array( $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ) ) ), ); } /** - * @dataProvider instanceProvider - * - * @param \Wikibase\Claims $array + * @dataProvider constructorErrorProvider */ - public function testHasClaim( Claims $array ) { - /** - * @var Claim $hashable - */ - foreach ( iterator_to_array( $array ) as $hashable ) { - $this->assertTrue( $array->hasClaim( $hashable ) ); - $array->removeClaim( $hashable ); - $this->assertFalse( $array->hasClaim( $hashable ) ); - } + public function testConstructorError() { + $this->setExpectedException( 'InvalidArgumentException' ); - $this->assertTrue( true ); + $class = new ReflectionClass( 'Wikibase\Claims' ); + $class->newInstanceArgs( func_get_args() ); } - /** - * @dataProvider instanceProvider - * - * @param \Wikibase\Claims $array - */ - public function testRemoveClaim( Claims $array ) { - $elementCount = count( $array ); - - /** - * @var Claim $element - */ - foreach ( iterator_to_array( $array ) as $element ) { - $this->assertTrue( $array->hasClaim( $element ) ); - - $array->removeClaim( $element ); - - $this->assertFalse( $array->hasClaim( $element ) ); - $this->assertEquals( --$elementCount, count( $array ) ); - } - - $elements = $this->getElementInstances(); - $element = array_shift( $elements ); - - $array->removeClaim( $element ); - $array->removeClaim( $element ); - - $this->assertTrue( true ); + public function constructorErrorProvider() { + return array( + array( 17 ), + array( array( "foo" ) ), + ); } - /** - * @dataProvider instanceProvider - * - * @param \Wikibase\Claims $array - */ - public function testAddClaim( Claims $array ) { - $elementCount = count( $array ); + public function testHasClaim() { + $claims = new Claims(); + $claim1 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $claim2 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P16" ) ) ); - $elements = $this->getElementInstances(); - $element = array_shift( $elements ); + $this->assertFalse( $claims->hasClaim( $claim1 ) ); + $this->assertFalse( $claims->hasClaim( $claim2 ) ); - ++$elementCount; + $claims->addClaim( $claim1 ); + $this->assertTrue( $claims->hasClaim( $claim1 ) ); + $this->assertFalse( $claims->hasClaim( $claim2 ) ); - $array->addClaim( $element ); + $claims->addClaim( $claim2 ); + $this->assertTrue( $claims->hasClaim( $claim1 ) ); + $this->assertTrue( $claims->hasClaim( $claim2 ) ); - $this->assertEquals( $elementCount, count( $array ) ); + // no guid + $claim0 = new Claim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $this->assertFalse( $claims->hasClaim( $claim0 ) ); } - /** - * @dataProvider instanceProvider - * - * @param \Wikibase\Claims $array - */ - public function testGetMainSnaks( Claims $array ) { - $snaks = $array->getMainSnaks(); - $this->assertInternalType( 'array', $snaks ); - $this->assertSameSize( $array, $snaks ); + public function testHasClaimWithGuid() { + $claims = new Claims(); + $claim1 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $claim2 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P16" ) ) ); + + $this->assertFalse( $claims->hasClaimWithGuid( $claim1->getGuid() ) ); + $this->assertFalse( $claims->hasClaimWithGuid( $claim2->getGuid() ) ); + + $claims->addClaim( $claim1 ); + $this->assertTrue( $claims->hasClaimWithGuid( $claim1->getGuid() ) ); + $this->assertFalse( $claims->hasClaimWithGuid( $claim2->getGuid() ) ); + + $claims->addClaim( $claim2 ); + $this->assertTrue( $claims->hasClaimWithGuid( $claim1->getGuid() ) ); + $this->assertTrue( $claims->hasClaimWithGuid( $claim2->getGuid() ) ); } - public function testGetClaimsForProperty() { - $array = new Claims( $this->getElementInstances() ); + public function testRemoveClaim() { + $claims = new Claims(); + $claim1 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $claim2 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P16" ) ) ); - $claims = $array->getClaimsForProperty( 42 ); - $this->assertInstanceOf( 'Wikibase\Claims', $claims ); + $claims->addClaim( $claim1 ); + $claims->addClaim( $claim2 ); $this->assertCount( 2, $claims ); - $claims = $array->getClaimsForProperty( 23 ); - $this->assertInstanceOf( 'Wikibase\Claims', $claims ); + $claims->removeClaim( $claim1 ); + $this->assertFalse( $claims->hasClaim( $claim1 ) ); + $this->assertNull( $claims->getClaimWithGuid( $claim1->getGuid() ) ); $this->assertCount( 1, $claims ); - $claims = $array->getClaimsForProperty( 9000 ); - $this->assertInstanceOf( 'Wikibase\Claims', $claims ); + $claims->removeClaim( $claim2 ); + $this->assertFalse( $claims->hasClaim( $claim2 ) ); + $this->assertNull( $claims->getClaimWithGuid( $claim2->getGuid() ) ); + $this->assertCount( 0, $claims ); + + // no guid + $claim0 = new Claim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $claims->removeClaim( $claim0 ); + } + + public function testRemoveClaimWithGuid() { + $claims = new Claims(); + $claim1 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $claim2 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P16" ) ) ); + + $claims->addClaim( $claim1 ); + $claims->addClaim( $claim2 ); + $this->assertCount( 2, $claims ); + + $claims->removeClaimWithGuid( $claim1->getGuid() ); + $this->assertFalse( $claims->hasClaim( $claim1 ) ); + $this->assertNull( $claims->getClaimWithGuid( $claim1->getGuid() ) ); + $this->assertCount( 1, $claims ); + + $claims->removeClaimWithGuid( $claim2->getGuid() ); + $this->assertFalse( $claims->hasClaim( $claim2 ) ); + $this->assertNull( $claims->getClaimWithGuid( $claim2->getGuid() ) ); $this->assertCount( 0, $claims ); } + public function testOffsetUnset() { + $claims = new Claims(); + $claim1 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $claim2 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P16" ) ) ); + + $claims->addClaim( $claim1 ); + $claims->addClaim( $claim2 ); + $this->assertCount( 2, $claims ); + + $claims->offsetUnset( $claim1->getGuid() ); + $this->assertFalse( $claims->hasClaim( $claim1 ) ); + $this->assertNull( $claims->getClaimWithGuid( $claim1->getGuid() ) ); + $this->assertCount( 1, $claims ); + + $claims->offsetUnset( $claim2->getGuid() ); + $this->assertFalse( $claims->hasClaim( $claim2 ) ); + $this->assertNull( $claims->getClaimWithGuid( $claim2->getGuid() ) ); + $this->assertCount( 0, $claims ); + } + + public function testGetClaimWithGuid() { + $claims = new Claims(); + $claim1 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $claim2 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P16" ) ) ); + + $claims->addClaim( $claim1 ); + $this->assertSame( $claim1, $claims->getClaimWithGuid( $claim1->getGuid() ) ); + $this->assertNull( $claims->getClaimWithGuid( $claim2->getGuid() ) ); + + $claims->addClaim( $claim2 ); + $this->assertSame( $claim1, $claims->getClaimWithGuid( $claim1->getGuid() ) ); + $this->assertSame( $claim2, $claims->getClaimWithGuid( $claim2->getGuid() ) ); + } + + public function testOffsetGet() { + $claims = new Claims(); + $claim1 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $claim2 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P16" ) ) ); + + $claims->addClaim( $claim1 ); + $this->assertSame( $claim1, $claims->offsetGet( $claim1->getGuid() ) ); + + $claims->addClaim( $claim2 ); + $this->assertSame( $claim1, $claims->offsetGet( $claim1->getGuid() ) ); + $this->assertSame( $claim2, $claims->offsetGet( $claim2->getGuid() ) ); + } + + public function testAddClaim() { + $claims = new Claims(); + $claim1 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $claim2 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P16" ) ) ); + + $claims->addClaim( $claim1 ); + $claims->addClaim( $claim2 ); + + $this->assertCount( 2, $claims ); + $this->assertEquals( $claim1, $claims[$claim1->getGuid()] ); + $this->assertEquals( $claim2, $claims[$claim2->getGuid()] ); + + $claims->addClaim( $claim1 ); + $claims->addClaim( $claim2 ); + + $this->assertCount( 2, $claims ); + + $this->assertNotNull( $claims->getClaimWithGuid( $claim1->getGuid() ) ); + $this->assertNotNull( $claims->getClaimWithGuid( $claim2->getGuid() ) ); + } + + public function testAppend() { + $claims = new Claims(); + $claim1 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $claim2 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P16" ) ) ); + + $claims->append( $claim1 ); + $claims->append( $claim2 ); + + $this->assertCount( 2, $claims ); + $this->assertEquals( $claim1, $claims[$claim1->getGuid()] ); + $this->assertEquals( $claim2, $claims[$claim2->getGuid()] ); + + $claims->append( $claim1 ); + $claims->append( $claim2 ); + + $this->assertCount( 2, $claims ); + } + + public function testAppendOperator() { + $claims = new Claims(); + $claim1 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $claim2 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P16" ) ) ); + + $claims[] = $claim1; + $claims[] = $claim2; + + $this->assertCount( 2, $claims ); + $this->assertEquals( $claim1, $claims[$claim1->getGuid()] ); + $this->assertEquals( $claim2, $claims[$claim2->getGuid()] ); + + $claims[] = $claim1; + $claims[] = $claim2; + + $this->assertCount( 2, $claims ); + } + + public function testOffsetSet() { + $claims = new Claims(); + $claim1 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $claim2 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P16" ) ) ); + + $claims->offsetSet( $claim1->getGuid(), $claim1 ); + $claims->offsetSet( $claim2->getGuid(), $claim2 ); + + $this->assertCount( 2, $claims ); + $this->assertEquals( $claim1, $claims[$claim1->getGuid()] ); + $this->assertEquals( $claim2, $claims[$claim2->getGuid()] ); + + $claims->offsetSet( $claim1->getGuid(), $claim1 ); + $claims->offsetSet( $claim2->getGuid(), $claim2 ); + + $this->assertCount( 2, $claims ); + + $this->setExpectedException( 'InvalidArgumentException' ); + $claims->offsetSet( 'spam', $claim1 ); + } + + public function testOffsetSetOperator() { + $claims = new Claims(); + $claim1 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $claim2 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P16" ) ) ); + + $claims[$claim1->getGuid()] = $claim1; + $claims[$claim2->getGuid()] = $claim2; + + $this->assertCount( 2, $claims ); + $this->assertEquals( $claim1, $claims[$claim1->getGuid()] ); + $this->assertEquals( $claim2, $claims[$claim2->getGuid()] ); + + $claims[$claim1->getGuid()] = $claim1; + $claims[$claim2->getGuid()] = $claim2; + + $this->assertCount( 2, $claims ); + } + + public function testGuidNormalization() { + $claims = new Claims(); + $claim1 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $claim2 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P16" ) ) ); + + $claim1LowerGuid = strtolower( $claim1->getGuid() ); + $claim2UpperGuid = strtoupper( $claim2->getGuid() ); + + $claims->addClaim( $claim1 ); + $claims->addClaim( $claim2 ); + $this->assertCount( 2, $claims ); + + $this->assertEquals( $claim1, $claims->getClaimWithGuid( $claim1LowerGuid ) ); + $this->assertEquals( $claim2, $claims->getClaimWithGuid( $claim2UpperGuid ) ); + + $this->assertEquals( $claim1, $claims->offsetGet( $claim1LowerGuid ) ); + $this->assertEquals( $claim2, $claims->offsetGet( $claim2UpperGuid ) ); + + $this->assertEquals( $claim1, $claims[$claim1LowerGuid] ); + $this->assertEquals( $claim2, $claims[$claim2UpperGuid] ); + + $claims = new Claims(); + $claims->offsetSet( strtoupper( $claim1LowerGuid ), $claim1 ); + $claims->offsetSet( strtolower( $claim2UpperGuid ), $claim2 ); + $this->assertCount( 2, $claims ); + + $this->assertEquals( $claim1, $claims->getClaimWithGuid( $claim1LowerGuid ) ); + $this->assertEquals( $claim2, $claims->getClaimWithGuid( $claim2UpperGuid ) ); + } + + public function testGetMainSnaks() { + $claims = new Claims( array( + $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P42" ) ) ), + $this->makeClaim( new PropertySomeValueSnak( new PropertyId( "P42" ) ) ), + $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P23" ) ) ), + $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P9000" ) ) ), + ) ); + + $snaks = $claims->getMainSnaks(); + $this->assertInternalType( 'array', $snaks ); + $this->assertSameSize( $claims, $snaks ); + + foreach ( $snaks as $guid => $snak ) { + $this->assertInstanceOf( 'Wikibase\Snak', $snak ); + $this->assertTrue( $claims->hasClaimWithGuid( $guid ) ); + } + } + + public function testGetGuids() { + $claims = new Claims( array( + $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P42" ) ) ), + $this->makeClaim( new PropertySomeValueSnak( new PropertyId( "P42" ) ) ), + $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P23" ) ) ), + $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P9000" ) ) ), + ) ); + + $guids = $claims->getGuids(); + $this->assertInternalType( 'array', $guids ); + $this->assertSameSize( $claims, $guids ); + + foreach ( $guids as $guid ) { + $this->assertInternalType( 'string', $guid ); + $this->assertTrue( $claims->hasClaimWithGuid( $guid ) ); + } + } + + public function testGetHashes() { + $claims = new Claims( array( + $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P42" ) ) ), + $this->makeClaim( new PropertySomeValueSnak( new PropertyId( "P42" ) ) ), + $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P23" ) ) ), + $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P9000" ) ) ), + ) ); + + $hashes = $claims->getHashes(); + $this->assertInternalType( 'array', $hashes ); + $this->assertSameSize( $claims, $hashes ); + + foreach ( $hashes as $hash ) { + $this->assertInternalType( 'string', $hash ); + } + + $hashSet = array_flip( $hashes ); + + foreach ( $claims as $claim ) { + $hash = $claim->getHash(); + $this->assertArrayHasKey( $hash, $hashSet ); + } + } + + public function testGetClaimsForProperty() { + $claims = new Claims( array( + $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P42" ) ) ), + $this->makeClaim( new PropertySomeValueSnak( new PropertyId( "P42" ) ) ), + $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P23" ) ) ), + ) ); + + $matches = $claims->getClaimsForProperty( 42 ); + $this->assertInstanceOf( 'Wikibase\Claims', $claims ); + $this->assertCount( 2, $matches ); + + $matches = $claims->getClaimsForProperty( 23 ); + $this->assertInstanceOf( 'Wikibase\Claims', $claims ); + $this->assertCount( 1, $matches ); + + $matches = $claims->getClaimsForProperty( 9000 ); + $this->assertInstanceOf( 'Wikibase\Claims', $claims ); + $this->assertCount( 0, $matches ); + } + + /** + * Attempts to add Claims with no GUID set will fail. + */ + public function testNoGuidFailure() { + $claim = new Claim( new PropertyNoValueSnak( 42 ) ); + $list = new Claims(); + + $this->setExpectedException( 'InvalidArgumentException' ); + $list->addClaim( $claim ); + } + public function testDuplicateClaims() { - $firstClaim = new Claim( new PropertyNoValueSnak( 42 ) ); - $secondClaim = new Claim( new PropertyNoValueSnak( 42 ) ); + $firstClaim = $this->makeClaim( new PropertyNoValueSnak( 42 ) ); + $secondClaim = $this->makeClaim( new PropertyNoValueSnak( 42 ) ); $list = new Claims(); - $this->assertTrue( $list->addElement( $firstClaim ), 'Adding the first element should work' ); - $this->assertTrue( $list->addElement( $secondClaim ), 'Adding a duplicate element should work' ); + $list->addClaim( $firstClaim ); + $list->addClaim( $secondClaim ); - $this->assertEquals( 2, count( $list->getArrayCopy() ), 'Adding two duplicates to an empty list should result in a count of two' ); + $this->assertEquals( 2, count( $list ), 'Adding two duplicates to an empty list should result in a count of two' ); - $this->assertTrue( $list->addElement( new Claim( new PropertySomeValueSnak( 1 ) ) ) ); + $this->assertEquals( $firstClaim, $list->getClaimWithGuid( $firstClaim->getGuid() ) ); + $this->assertEquals( $secondClaim, $list->getClaimWithGuid( $secondClaim->getGuid() ) ); - $list->removeDuplicates(); + $list->removeClaimWithGuid( $secondClaim->getGuid() ); - $this->assertEquals( 2, count( $list->getArrayCopy() ), 'Removing duplicates from a list should work' ); + $this->assertNotNull( $list->getClaimWithGuid( $firstClaim->getGuid() ) ); + $this->assertNull( $list->getClaimWithGuid( $secondClaim->getGuid() ) ); } public function getDiffProvider() { $argLists = array(); - $claim0 = new Claim( new PropertyNoValueSnak( 42 ) ); - $claim1 = new Claim( new PropertySomeValueSnak( 42 ) ); - $claim2 = new Claim( new PropertyValueSnak( 42, new StringValue( 'ohi' ) ) ); - $claim3 = new Claim( new PropertyNoValueSnak( 1 ) ); - $claim4 = new Claim( new PropertyNoValueSnak( 2 ) ); + $claim0 = $this->makeClaim( new PropertyNoValueSnak( 42 ) ); + $claim1 = $this->makeClaim( new PropertySomeValueSnak( 42 ) ); + $claim2 = $this->makeClaim( new PropertyValueSnak( 42, new StringValue( 'ohi' ) ) ); + $claim3 = $this->makeClaim( new PropertyNoValueSnak( 1 ) ); + $claim4 = $this->makeClaim( new PropertyNoValueSnak( 2 ) ); - $statement0 = new Statement( new PropertyNoValueSnak( 5 ) ); + $statement0 = $this->makeStatement( new PropertyNoValueSnak( 5 ) ); $statement0->setRank( Statement::RANK_PREFERRED ); - $statement1 = new Statement( new PropertyNoValueSnak( 5 ) ); + $statement1 = $this->makeStatement( new PropertyNoValueSnak( 5 ) ); $statement1->setReferences( new ReferenceList( array( new Reference( new SnakList( array( new PropertyValueSnak( 10, new StringValue( 'spam' ) ) ) ) ) ) ) ); - $claim0->setGuid( 'claim0' ); - $claim1->setGuid( 'claim1' ); - $claim2->setGuid( 'claim2' ); - $statement1->setGuid( 'statement1' ); - $statement0->setGuid( 'statement0' ); - + // same GUID, changed main snak $claim2v2 = unserialize( serialize( $claim2 ) ); $claim2v2->setMainSnak( new PropertyValueSnak( 42, new StringValue( 'omnomnom' ) ) ); + // different GUID, same contents, same hash + $claim0a = unserialize( serialize( $claim0 ) ); + $claim0a->setGuid( 'TEST$claim0x' ); + $claim0a->setMainSnak( new PropertyValueSnak( 99, new StringValue( 'frob' ) ) ); + + // same GUID as $claim0a, same content/hash as $claim0 + $claim0b = unserialize( serialize( $claim0 ) ); + $claim0b->setGuid( 'TEST$claim0x' ); $source = new Claims(); $target = new Claims(); @@ -277,6 +551,11 @@ $expected = new Diff( array( $claim2->getGuid() => new DiffOpChange( $claim2, $claim2v2 ) ), true ); $argLists[] = array( $source, $target, $expected, 'Changing the value of a claim should result in a change op' ); + $source = new Claims( array( $claim0, $claim0a ) ); + $target = new Claims( array( $claim0, $claim0b ) ); + $expected = new Diff( array( $claim0a->getGuid() => new DiffOpChange( $claim0a, $claim0b ) ), true ); + $argLists[] = array( $source, $target, $expected, 'It should be possible for a claim to become the same as another claim' ); + return $argLists; } @@ -302,4 +581,62 @@ $claims->getClaimsForProperty( 'foo bar' ); } + public function testGetHash() { + $claimsA = new Claims(); + $claimsB = new Claims(); + $claim1 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + $claim2 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P16" ) ) ); + + $this->assertEquals( $claimsA->getHash(), $claimsB->getHash(), 'empty list' ); + + $claimsA->addClaim( $claim1 ); + $claimsB->addClaim( $claim2 ); + $this->assertNotEquals( $claimsA->getHash(), $claimsB->getHash(), 'different content' ); + + $claimsA->addClaim( $claim2 ); + $claimsB->addClaim( $claim1 ); + $this->assertNotEquals( $claimsA->getHash(), $claimsB->getHash(), 'different order' ); + + $claimsA->removeClaim( $claim1 ); + $claimsB->removeClaim( $claim1 ); + $this->assertEquals( $claimsA->getHash(), $claimsB->getHash(), 'same content' ); + } + + public function testItrerator() { + $claims = new Claims( array( + $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P42" ) ) ), + $this->makeClaim( new PropertySomeValueSnak( new PropertyId( "P42" ) ) ), + $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P23" ) ) ), + $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P9000" ) ) ), + ) ); + + $array = iterator_to_array( $claims->getIterator() ); + + $this->assertSameSize( $claims, $array ); + + reset( $array ); + reset( $claims ); + + while ( $actual = current( $array ) ) { + $expected = current( $claims ); + + $this->assertEquals( $actual, $expected ); + + next( $claims ); + next( $array ); + } + } + + public function testIsEmpty() { + $claims = new Claims(); + $claim1 = $this->makeClaim( new PropertyNoValueSnak( new PropertyId( "P15" ) ) ); + + $this->assertTrue( $claims->isEmpty() ); + + $claims->addClaim( $claim1 ); + $this->assertFalse( $claims->isEmpty() ); + + $claims->removeClaim( $claim1 ); + $this->assertTrue( $claims->isEmpty() ); + } } diff --git a/tests/phpunit/Entity/EntityTest.php b/tests/phpunit/Entity/EntityTest.php index 71ab54a..b30387b 100644 --- a/tests/phpunit/Entity/EntityTest.php +++ b/tests/phpunit/Entity/EntityTest.php @@ -886,6 +886,9 @@ $claim1 = new Claim( new PropertySomeValueSnak( 42 ) ), ); + $claims[0]->setGuid( 'TEST$NVS42' ); + $claims[1]->setGuid( 'TEST$SVS42' ); + $entity->setClaims( new Claims( $claims ) ); $this->assertSameSize( $claims, $entity->getClaims(), "added some claims" ); diff --git a/tests/phpunit/Entity/ItemTest.php b/tests/phpunit/Entity/ItemTest.php index df882b5..0dba54d 100644 --- a/tests/phpunit/Entity/ItemTest.php +++ b/tests/phpunit/Entity/ItemTest.php @@ -97,6 +97,10 @@ ) ) ); + foreach ( $claims as $i => $claim ) { + $claim->setGuid( "ItemTest\$claim-$i" ); + } + return $claims; } diff --git a/tests/phpunit/EntityDiffTest.php b/tests/phpunit/EntityDiffTest.php index 6280237..508273b 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( 'EntityDiffTest$foo' ); + + $claims = new \Wikibase\Claims( array( $claim ) ); $diffOps['claim'] = $claims->getDiff( new \Wikibase\Claims() ); -- To view, visit https://gerrit.wikimedia.org/r/89015 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Idd5ffb72774a948852a6dbe2e05700b9aa94a211 Gerrit-PatchSet: 4 Gerrit-Project: mediawiki/extensions/WikibaseDataModel Gerrit-Branch: master Gerrit-Owner: Daniel Kinzler <daniel.kinz...@wikimedia.de> Gerrit-Reviewer: Addshore <addshorew...@gmail.com> Gerrit-Reviewer: Daniel Kinzler <daniel.kinz...@wikimedia.de> Gerrit-Reviewer: Jeroen De Dauw <jeroended...@gmail.com> Gerrit-Reviewer: Tobias Gritschacher <tobias.gritschac...@wikimedia.de> Gerrit-Reviewer: jenkins-bot _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits