Daniel Kinzler has uploaded a new change for review.
https://gerrit.wikimedia.org/r/123860
Change subject: (bug 62644) snak validation in ChangeOpMainSnak.
......................................................................
(bug 62644) snak validation in ChangeOpMainSnak.
Change-Id: I99ed03d15643a3b42a60072d2fb3824ca80622d6
---
M repo/includes/ChangeOp/ChangeOpClaim.php
M repo/includes/ChangeOp/ChangeOpFactory.php
M repo/includes/ChangeOp/ChangeOpMainSnak.php
M repo/includes/api/ClaimModificationHelper.php
M repo/includes/api/CreateClaim.php
M repo/includes/api/SetClaimValue.php
M repo/tests/phpunit/includes/ChangeOp/ChangeOpMainSnakTest.php
M repo/tests/phpunit/includes/api/CreateClaimTest.php
M repo/tests/phpunit/includes/api/SetClaimValueTest.php
9 files changed, 176 insertions(+), 55 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/60/123860/1
diff --git a/repo/includes/ChangeOp/ChangeOpClaim.php
b/repo/includes/ChangeOp/ChangeOpClaim.php
index c04b0c8..1f434cb 100644
--- a/repo/includes/ChangeOp/ChangeOpClaim.php
+++ b/repo/includes/ChangeOp/ChangeOpClaim.php
@@ -200,6 +200,8 @@
}
/**
+ * @since 0.5
+ *
* @throws ChangeOpException
*/
protected function validate() {
diff --git a/repo/includes/ChangeOp/ChangeOpFactory.php
b/repo/includes/ChangeOp/ChangeOpFactory.php
index c7793ee..6fa44e2 100644
--- a/repo/includes/ChangeOp/ChangeOpFactory.php
+++ b/repo/includes/ChangeOp/ChangeOpFactory.php
@@ -229,7 +229,7 @@
* @return ChangeOp
*/
public function newSetMainSnakOp( $claimGuid, Snak $snak ) {
- return new ChangeOpMainSnak( $claimGuid, $snak,
$this->guidGenerator );
+ return new ChangeOpMainSnak( $claimGuid, $snak,
$this->guidGenerator, $this->snakValidator );
}
/**
diff --git a/repo/includes/ChangeOp/ChangeOpMainSnak.php
b/repo/includes/ChangeOp/ChangeOpMainSnak.php
index ef60baf..83a0982 100644
--- a/repo/includes/ChangeOp/ChangeOpMainSnak.php
+++ b/repo/includes/ChangeOp/ChangeOpMainSnak.php
@@ -8,6 +8,7 @@
use Wikibase\DataModel\Snak\Snak;
use Wikibase\Lib\ClaimGuidGenerator;
use Wikibase\Summary;
+use Wikibase\Validators\SnakValidator;
/**
* Class for mainsnak change operation
@@ -15,6 +16,7 @@
* @since 0.4
* @licence GNU GPL v2+
* @author Tobias Gritschacher < tobias.gritschac...@wikimedia.de >
+ * @author Daniel Kinzler
*/
class ChangeOpMainSnak extends ChangeOpBase {
@@ -33,6 +35,11 @@
protected $snak;
/**
+ * @var SnakValidator
+ */
+ private $snakValidator;
+
+ /**
* Constructs a new mainsnak change operation
*
* @since 0.4
@@ -40,10 +47,16 @@
* @param string $claimGuid
* @param Snak|null $snak
* @param ClaimGuidGenerator $guidGenerator
+ * @param SnakValidator $snakValidator
*
* @throws InvalidArgumentException
*/
- public function __construct( $claimGuid, $snak, ClaimGuidGenerator
$guidGenerator ) {
+ public function __construct(
+ $claimGuid,
+ $snak,
+ ClaimGuidGenerator $guidGenerator,
+ SnakValidator $snakValidator
+ ) {
if ( !is_string( $claimGuid ) ) {
throw new InvalidArgumentException( '$claimGuid needs
to be a string' );
}
@@ -55,6 +68,7 @@
$this->claimGuid = $claimGuid;
$this->snak = $snak;
$this->guidGenerator = $guidGenerator;
+ $this->snakValidator = $snakValidator;
}
/**
@@ -70,6 +84,8 @@
* - the claim's mainsnak gets set to $snak when $claimGuid and $snak
are set
*/
public function apply( Entity $entity, Summary $summary = null ) {
+ $this->validate();
+
$claims = new Claims( $entity->getClaims() );
if ( is_null( $this->claimGuid ) || empty( $this->claimGuid ) )
{
@@ -129,23 +145,6 @@
/**
* @since 0.4
*
- * @param Claims $claims
- * @param Summary $summary
- *
- * @throws ChangeOpException
- */
- protected function removeClaim( Claims $claims, Summary $summary = null
) {
- if( !$claims->hasClaimWithGuid( $this->claimGuid ) ) {
- throw new ChangeOpException( "Entity does not have
claim with GUID $this->claimGuid" );
- }
- $removedSnak = $claims->getClaimWithGuid( $this->claimGuid
)->getMainSnak();
- $claims->removeClaimWithGuid( $this->claimGuid );
- $this->updateSummary( $summary, 'remove', '',
$this->getClaimSummaryArgs( $removedSnak ) );
- }
-
- /**
- * @since 0.4
- *
* @param Snak $mainSnak
*
* @return array
@@ -154,4 +153,17 @@
$propertyId = $mainSnak->getPropertyId();
return array( array( $propertyId->getPrefixedId() => $mainSnak
) );
}
+
+ /**
+ * @since 0.5
+ *
+ * @throws ChangeOpException
+ */
+ protected function validate() {
+ $result = $this->snakValidator->validate( $this->snak );
+
+ if ( !$result->isValid() ) {
+ throw new ChangeOpValidationException( $result );
+ }
+ }
}
diff --git a/repo/includes/api/ClaimModificationHelper.php
b/repo/includes/api/ClaimModificationHelper.php
index 5602471..bc5dab9 100644
--- a/repo/includes/api/ClaimModificationHelper.php
+++ b/repo/includes/api/ClaimModificationHelper.php
@@ -149,6 +149,7 @@
}
/** @var Snak $snak */
+ //FIXME: remove snak validation logic here, once it is applied
consistently by ChangeOps.
$this->snakValidation->validateSnak( $snak );
return $snak;
}
diff --git a/repo/includes/api/CreateClaim.php
b/repo/includes/api/CreateClaim.php
index 66434a2..696f80c 100644
--- a/repo/includes/api/CreateClaim.php
+++ b/repo/includes/api/CreateClaim.php
@@ -54,6 +54,8 @@
try {
$changeOp->apply( $entity, $summary );
} catch ( ChangeOpException $e ) {
+ //FIXME: implement generic localization of exceptions,
+ // especially ChangeOpValidationException.
$this->dieUsage( $e->getMessage(), 'failed-save' );
}
diff --git a/repo/includes/api/SetClaimValue.php
b/repo/includes/api/SetClaimValue.php
index d87ecfc..3684ec0 100644
--- a/repo/includes/api/SetClaimValue.php
+++ b/repo/includes/api/SetClaimValue.php
@@ -47,6 +47,8 @@
try {
$changeOp->apply( $entity, $summary );
} catch ( ChangeOpException $e ) {
+ //FIXME: implement generic localization of exceptions,
+ // especially ChangeOpValidationException.
$this->dieUsage( $e->getMessage(), 'failed-save' );
}
diff --git a/repo/tests/phpunit/includes/ChangeOp/ChangeOpMainSnakTest.php
b/repo/tests/phpunit/includes/ChangeOp/ChangeOpMainSnakTest.php
index 7414cf5..c558cc0 100644
--- a/repo/tests/phpunit/includes/ChangeOp/ChangeOpMainSnakTest.php
+++ b/repo/tests/phpunit/includes/ChangeOp/ChangeOpMainSnakTest.php
@@ -3,6 +3,7 @@
namespace Wikibase\Test;
use DataValues\DataValue;
+use DataValues\NumberValue;
use DataValues\StringValue;
use Wikibase\ChangeOp\ChangeOp;
use Wikibase\ChangeOp\ChangeOpMainSnak;
@@ -10,7 +11,10 @@
use Wikibase\DataModel\Entity\Entity;
use Wikibase\DataModel\Entity\Item;
use Wikibase\DataModel\Entity\ItemId;
+use Wikibase\DataModel\Entity\PropertyId;
+use Wikibase\DataModel\Snak\PropertyNoValueSnak;
use Wikibase\DataModel\Snak\PropertyValueSnak;
+use Wikibase\DataModel\Snak\Snak;
use Wikibase\ItemContent;
use Wikibase\Lib\ClaimGuidGenerator;
use InvalidArgumentException;
@@ -25,22 +29,37 @@
*
* @licence GNU GPL v2+
* @author Tobias Gritschacher < tobias.gritschac...@wikimedia.de >
+ * @author Daniel Kinzler
*/
class ChangeOpMainSnakTest extends \PHPUnit_Framework_TestCase {
+ /**
+ * @var ClaimTestHelper
+ */
+ protected $helper;
+
+ /**
+ * @param null $name
+ * @param array $data
+ * @param string $dataName
+ */
+ public function __construct( $name = null, array $data = array(),
$dataName = '' ) {
+ parent::__construct( $name, $data, $dataName );
+
+ $this->helper = new ClaimTestHelper();
+ }
+
public function invalidArgumentProvider() {
$item = ItemContent::newFromArray( array( 'entity' => 'q42' )
)->getEntity();
- $validGuidGenerator = new ClaimGuidGenerator();
- $guidGenerator = new ClaimGuidGenerator();
- $validClaimGuid = $guidGenerator->newGuid( $item->getId(
$item->getId() ) );
+ $validClaimGuid = $this->helper->getGuidGenerator()->newGuid(
$item->getId() );
$validSnak = new PropertyValueSnak( 7201010, new StringValue(
'o_O' ) );
$args = array();
- $args[] = array( 123, $validSnak, $validGuidGenerator );
- $args[] = array( 123, null, $validGuidGenerator );
- $args[] = array( $validClaimGuid, 'notASnak',
$validGuidGenerator );
- $args[] = array( '', 'notASnak', $validGuidGenerator );
- $args[] = array( '', null, $validGuidGenerator );
+ $args[] = array( 123, $validSnak );
+ $args[] = array( 123, null );
+ $args[] = array( $validClaimGuid, 'notASnak' );
+ $args[] = array( '', 'notASnak' );
+ $args[] = array( '', null );
return $args;
}
@@ -50,30 +69,50 @@
*
* @expectedException InvalidArgumentException
*/
- public function testInvalidConstruct( $claimGuid, $snak, $guidGenerator
) {
- new ChangeOpMainSnak( $claimGuid, $snak, $guidGenerator );
+ public function testInvalidConstruct( $claimGuid, $snak ) {
+ new ChangeOpMainSnak(
+ $claimGuid,
+ $snak,
+ $this->helper->getGuidGenerator(),
+ $this->helper->getMockSnakValidator()
+ );
+ }
+
+ /**
+ * @param string $claimGuid
+ * @param Snak $snak
+ *
+ * @return ChangeOpMainSnak
+ */
+ protected function newChangeOpMainSnak( $claimGuid, Snak $snak ) {
+ return new ChangeOpMainSnak(
+ $claimGuid,
+ $snak,
+ $this->helper->getGuidGenerator(),
+ $this->helper->getMockSnakValidator()
+ );
}
public function provideChangeOps() {
- $snak = new PropertyValueSnak( 2754236, new StringValue( 'test'
) );
+ $snak = $this->makeSnak( 'P5', 'test' );
$args = array();
// add a new claim
- $item = $this->makeNewItemWithClaim( 'q123', $snak );
- $newSnak = new PropertyValueSnak( 78462378, new StringValue(
'newSnak' ) );
+ $item = $this->makeNewItemWithClaim( 'Q123', $snak );
+ $newSnak = $this->makeSnak( 'P8', 'newSnak' );
$claimGuid = '';
- $changeOp = new ChangeOpMainSnak( $claimGuid, $newSnak, new
ClaimGuidGenerator() );
+ $changeOp = $this->newChangeOpMainSnak( $claimGuid, $newSnak );
$expected = $newSnak->getDataValue();
$args['add new claim'] = array ( $item, $changeOp, $expected );
// update an existing claim with a new main snak value
- $item = $this->makeNewItemWithClaim( 'q234', $snak );
- $newSnak = new PropertyValueSnak( 2754236, new StringValue(
'changedSnak' ) );
+ $item = $this->makeNewItemWithClaim( 'Q234', $snak );
+ $newSnak = $this->makeSnak( 'P5', 'changedSnak' );
$claims = $item->getClaims();
$claim = reset( $claims );
$claimGuid = $claim->getGuid();
- $changeOp = new ChangeOpMainSnak( $claimGuid, $newSnak, new
ClaimGuidGenerator() );
+ $changeOp = $this->newChangeOpMainSnak( $claimGuid, $newSnak );
$expected = $newSnak->getDataValue();
$args['update claim by guid'] = array ( $item, $changeOp,
$expected );
@@ -99,31 +138,39 @@
}
public function provideInvalidApply() {
- $snak = new PropertyValueSnak( 67573284, new StringValue(
'test' ) );
- $newSnak = new PropertyValueSnak( 12651236, new StringValue(
'newww' ) );
- $item = $this->makeNewItemWithClaim( 'q777', $snak );
+ $snak = $this->makeSnak( 'P11', 'test' );
+ $item = $this->makeNewItemWithClaim( 'Q777', $snak );
$claims = $item->getClaims();
$claim = reset( $claims );
$claimGuid = $claim->getGuid();
- $guidGenerator = new ClaimGuidGenerator();
// apply change to the wrong item
$wrongItem = Item::newEmpty();
$wrongItem->setId( new ItemId( "Q888" ) );
- $args['wrong entity'] = array ( $wrongItem, new
ChangeOpMainSnak( $claimGuid, $newSnak, $guidGenerator ) );
+ $newSnak = $this->makeSnak( 'P12', 'newww' );
+ $args['wrong entity'] = array ( $wrongItem,
$this->newChangeOpMainSnak( $claimGuid, $newSnak ) );
// apply change to an unknown claim
$wrongClaimId = $item->getId()->getPrefixedId() .
'$DEADBEEF-DEAD-BEEF-DEAD-BEEFDEADBEEF';
- $args['unknown claim'] = array ( $item, new ChangeOpMainSnak(
$wrongClaimId, $newSnak, $guidGenerator ) );
+ $args['unknown claim'] = array ( $item,
$this->newChangeOpMainSnak( $wrongClaimId, $newSnak ) );
// update an existing claim with wrong main snak property
- $newSnak = new PropertyValueSnak( 78462378, new StringValue(
'changedSnak' ) );
+ $newSnak = $this->makeSnak( 'P13', 'changedSnak' );
$claims = $item->getClaims();
$claim = reset( $claims );
$claimGuid = $claim->getGuid();
- $changeOp = new ChangeOpMainSnak( $claimGuid, $newSnak,
$guidGenerator );
+ $changeOp = $this->newChangeOpMainSnak( $claimGuid, $newSnak );
$args['wrong main snak property'] = array ( $item, $changeOp );
+
+ // apply invalid main snak
+ $badSnak = $this->makeSnak( 'P12', new NumberValue( 5 ) );
+ $args['bad value type'] = array ( $wrongItem,
$this->newChangeOpMainSnak( $claimGuid, $badSnak ) );
+
+ // apply invalid main snak
+ // NOTE: the mock validator considers "INVALID" to be invalid.
+ $badSnak = $this->makeSnak( 'P12', 'INVALID' );
+ $args['invalid value'] = array ( $wrongItem,
$this->newChangeOpMainSnak( $claimGuid, $badSnak ) );
return $args;
}
@@ -140,11 +187,27 @@
protected function makeNewItemWithClaim( $itemId, $snak ) {
$entity = Item::newFromArray( array( 'entity' => $itemId ) );
$claim = $entity->newClaim( $snak );
- $claim->setGuid( $entity->getId()->getPrefixedId() .
'$D8404CDA-25E4-4334-AG93-A3290BCD9C0P' );
+ $claim->setGuid( $this->helper->getGuidGenerator()->newGuid(
$entity->getId() ) );
$claims = new Claims();
$claims->addClaim( $claim );
$entity->setClaims( $claims );
return $entity;
}
+
+ protected function makeSnak( $propertyId, $value ) {
+ if ( is_string( $propertyId ) ) {
+ $propertyId = new PropertyId( $propertyId );
+ }
+
+ if ( is_string( $value ) ) {
+ $value = new StringValue( $value );
+ }
+
+ if ( $value === null ) {
+ return new PropertyNoValueSnak( $propertyId );
+ } else {
+ return new PropertyValueSnak( $propertyId, $value );
+ }
+ }
}
diff --git a/repo/tests/phpunit/includes/api/CreateClaimTest.php
b/repo/tests/phpunit/includes/api/CreateClaimTest.php
index 96f7d11..ed17af0 100644
--- a/repo/tests/phpunit/includes/api/CreateClaimTest.php
+++ b/repo/tests/phpunit/includes/api/CreateClaimTest.php
@@ -180,6 +180,16 @@
);
$argLists[] = array( 'invalid-snak', $params );
+ //10
+ $params = array(
+ 'action' => 'wbcreateclaim',
+ 'entity' => '-',
+ 'snaktype' => 'value',
+ 'property' => '-',
+ 'value' => '" "', //blank is invalid
+ );
+ $argLists[] = array( 'invalid-snak-value', $params );
+
return $argLists;
}
diff --git a/repo/tests/phpunit/includes/api/SetClaimValueTest.php
b/repo/tests/phpunit/includes/api/SetClaimValueTest.php
index 0a6f3b2..23dae2c 100644
--- a/repo/tests/phpunit/includes/api/SetClaimValueTest.php
+++ b/repo/tests/phpunit/includes/api/SetClaimValueTest.php
@@ -11,6 +11,7 @@
use Wikibase\DataModel\Claim\Claim;
use Wikibase\DataModel\Entity\EntityId;
use Wikibase\DataModel\Entity\Item;
+use Wikibase\DataModel\Entity\ItemId;
use Wikibase\Lib\EntityIdLinkFormatter;
use Wikibase\Lib\SnakFormatter;
use Wikibase\DataModel\Entity\Property;
@@ -45,6 +46,17 @@
* @var ValueFormatter
*/
private $propertyValueFormatter = null;
+
+ public function setUp() {
+ parent::setUp();
+
+ static $hasEntities = false;
+
+ if ( !$hasEntities ) {
+ $this->initTestEntities( array( 'StringProp', 'Berlin'
) );
+ $hasEntities = true;
+ }
+ }
/**
* @param Entity $entity
@@ -144,29 +156,46 @@
}
/**
- * @dataProvider invalidClaimProvider
+ * @dataProvider invalidRequestProvider
*/
- public function testInvalidClaimGuid( $claimGuid ) {
+ public function testInvalidRequest( $itemHandle, $claimGuid, $snakType,
$value, $error ) {
+ $itemId = new ItemId( EntityTestHelper::getId( $itemHandle ) );
+ $item =
WikibaseRepo::getDefaultInstance()->getEntityLookup()->getEntity( $itemId );
+
+ if ( $claimGuid === null ) {
+ $claims = $item->getClaims();
+
+ /* @var Claim $claim */
+ $claim = reset( $claims );
+ $claimGuid = $claim->getGuid();
+ }
+
+ if ( !is_string( $value ) ) {
+ $value = json_encode( $value );
+ }
+
$params = array(
'action' => 'wbsetclaimvalue',
'claim' => $claimGuid,
- 'snaktype' => 'value',
- 'value' => 'abc',
+ 'snaktype' => $snakType,
+ 'value' => $value,
);
try {
$this->doApiRequestWithToken( $params );
- $this->fail( 'Invalid claim guid did not raise an
error' );
+ $this->fail( 'Invalid request did not raise an error' );
} catch ( \UsageException $e ) {
- $this->assertEquals( 'invalid-guid',
$e->getCodeString(), 'Invalid claim guid raised correct error' );
+ $this->assertEquals( $error, $e->getCodeString(),
'Invalid claim guid raised correct error' );
}
}
- public function invalidClaimProvider() {
+ public function invalidRequestProvider() {
return array(
- array( 'xyz' ),
- array( 'x$y$z' ),
- array( 'i1813$358fa2a0-4345-82b6-12a4-7b0fee494a5f' )
+ 'bad guid 1' => array( 'Berlin', 'xyz', 'value', 'abc',
'invalid-guid' ),
+ 'bad guid 2' => array( 'Berlin', 'x$y$z', 'value',
'abc', 'invalid-guid' ),
+ 'bad guid 3' => array( 'Berlin',
'i1813$358fa2a0-4345-82b6-12a4-7b0fee494a5f', 'value', 'abc', 'invalid-guid' ),
+ 'bad snak type' => array( 'Berlin', null, 'alksdjf',
'abc', 'unknown_snaktype' ),
+ 'bad snak value' => array( 'Berlin', null, 'value', '
', 'invalid-snak' ),
);
}
--
To view, visit https://gerrit.wikimedia.org/r/123860
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I99ed03d15643a3b42a60072d2fb3824ca80622d6
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
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
