Daniel Kinzler has uploaded a new change for review.
https://gerrit.wikimedia.org/r/68369
Change subject: (bug 49498) Handle errors from invalid snak values.
......................................................................
(bug 49498) Handle errors from invalid snak values.
When providing invalid snak values to the API, we should respond
with a meaningful error message, not a fatal error.
Note that this needs I4d6b27a to be applied to the DataValues
project in order to work correctly.
Change-Id: I09e54dca33a3d143b1fa0d2f6ccf331ba8241a36
---
M DataModel/DataModel/Entity/EntityId.php
M DataModel/DataModel/Snak/SnakFactory.php
M repo/includes/api/CreateClaim.php
M repo/includes/api/SetClaim.php
M repo/includes/api/SetClaimValue.php
M repo/includes/api/SetQualifier.php
M repo/includes/api/SetReference.php
M repo/tests/phpunit/includes/api/CreateClaimTest.php
8 files changed, 91 insertions(+), 39 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/69/68369/1
diff --git a/DataModel/DataModel/Entity/EntityId.php
b/DataModel/DataModel/Entity/EntityId.php
index d093be7..dc238f7 100644
--- a/DataModel/DataModel/Entity/EntityId.php
+++ b/DataModel/DataModel/Entity/EntityId.php
@@ -2,6 +2,7 @@
namespace Wikibase;
+use DataValues\IllegalValueException;
use InvalidArgumentException;
use ValueParsers\ParseException;
@@ -273,15 +274,18 @@
/**
* Constructs a new instance of the DataValue from the provided data.
- * This can round-trip with @see getArrayValue
+ * This can round-trip with
+ * @see getArrayValue
*
* @since 0.4
*
* @param mixed $data
*
+ * @throws \DataValues\IllegalValueException
* @return \DataValues\DataValue
*/
public static function newFromArray( $data ) {
+ self::requireArrayFields( $data, array( 'entity-type',
'numeric-id' ) );
return new static( $data['entity-type'], $data['numeric-id'] );
}
diff --git a/DataModel/DataModel/Snak/SnakFactory.php
b/DataModel/DataModel/Snak/SnakFactory.php
index ebdc0ef..c938232 100644
--- a/DataModel/DataModel/Snak/SnakFactory.php
+++ b/DataModel/DataModel/Snak/SnakFactory.php
@@ -2,6 +2,8 @@
namespace Wikibase;
+use DataValues\IllegalValueException;
+use InvalidArgumentException;
use MWException;
/**
@@ -36,16 +38,17 @@
*
* @since 0.3
*
- * @param EntityId $propertyId
- * @param string $snakType
+ * @param EntityId $propertyId
+ * @param string $snakType
* @param string|null $snakValue
*
+ * @throws IllegalValueException
+ * @throws InvalidArgumentException
* @return Snak
- * @throws MWException
*/
public function newSnak( EntityId $propertyId, $snakType, $snakValue =
null ) {
if ( $propertyId->getEntityType() !== Property::ENTITY_TYPE ) {
- throw new MWException( 'Expected an EntityId of a
property' );
+ throw new InvalidArgumentException( 'Expected an
EntityId of a property' );
}
switch ( $snakType ) {
@@ -60,9 +63,7 @@
break;
}
- if ( !isset( $snak ) ) {
- throw new MWException( '$snak was not set to an
instance of Snak' );
- }
+ assert( isset( $snak ) );
return $snak;
}
diff --git a/repo/includes/api/CreateClaim.php
b/repo/includes/api/CreateClaim.php
index 0d79e5a..53469b1 100644
--- a/repo/includes/api/CreateClaim.php
+++ b/repo/includes/api/CreateClaim.php
@@ -4,6 +4,9 @@
use ApiBase, MWException;
+use DataValues\IllegalValueException;
+use InvalidArgumentException;
+use UsageException;
use ValueParsers\ParseException;
use Wikibase\EntityId;
use Wikibase\Entity;
@@ -66,9 +69,18 @@
try {
$snak = $this->getSnakInstance();
}
- catch ( \Exception $ex ) {
+ catch ( IllegalValueException $ex ) {
wfProfileOut( __METHOD__ );
- $this->dieUsageMsg( $ex->getMessage() );
+ $this->dieUsage( $ex->getMessage(),
'claim-invalid-snak' );
+ }
+ catch ( InvalidArgumentException $ex ) {
+ // shouldn't happen, but might.
+ wfProfileOut( __METHOD__ );
+ $this->dieUsage( $ex->getMessage(),
'claim-invalid-snak' );
+ }
+ catch ( ParseException $parseException ) {
+ wfProfileOut( __METHOD__ );
+ $this->dieUsage( $parseException->getMessage(),
'claim-invalid-guid' );
}
$claim = $this->addClaim( $entityContent->getEntity(), $snak );
@@ -168,7 +180,8 @@
* @since 0.2
*
* @return Snak
- * @throws MWException
+ * @throws ParseException
+ * @throws IllegalValueException
*/
protected function getSnakInstance() {
$params = $this->extractRequestParams();
@@ -176,12 +189,7 @@
$factory = new SnakFactory();
$entityIdParser =
WikibaseRepo::getDefaultInstance()->getEntityIdParser();
- try {
- $entityId = $entityIdParser->parse( $params['property']
);
- }
- catch ( ParseException $parseException ) {
- throw new MWException( $parseException->getMessage(),
'setclaim-invalid-guid' );
- }
+ $entityId = $entityIdParser->parse( $params['property'] );
return $factory->newSnak(
$entityId,
diff --git a/repo/includes/api/SetClaim.php b/repo/includes/api/SetClaim.php
index 70a94db..23b34be 100644
--- a/repo/includes/api/SetClaim.php
+++ b/repo/includes/api/SetClaim.php
@@ -2,6 +2,7 @@
namespace Wikibase\Api;
+use DataValues\IllegalValueException;
use Diff\CallbackListDiffer;
use MWException;
use ApiBase;
@@ -105,11 +106,15 @@
$unserializer = $serializerFactory->newUnserializerForClass(
'Wikibase\Claim' );
$params = $this->extractRequestParams();
- $claim = $unserializer->newFromSerialization(
\FormatJson::decode( $params['claim'], true ) );
- assert( $claim instanceof Claim );
+ try {
+ $claim = $unserializer->newFromSerialization(
\FormatJson::decode( $params['claim'], true ) );
- return $claim;
+ assert( $claim instanceof Claim );
+ return $claim;
+ } catch ( IllegalValueException $ex ) {
+ $this->dieUsage( $ex->getMessage(),
'setclaim-invalid-claim' );
+ }
}
/**
diff --git a/repo/includes/api/SetClaimValue.php
b/repo/includes/api/SetClaimValue.php
index d8aba33..2774f42 100644
--- a/repo/includes/api/SetClaimValue.php
+++ b/repo/includes/api/SetClaimValue.php
@@ -4,6 +4,7 @@
use ApiBase, MWException;
+use DataValues\IllegalValueException;
use Wikibase\Autocomment;
use Wikibase\EntityId;
use Wikibase\Entity;
@@ -143,11 +144,16 @@
$constructorArguments[] =
$content->getProperty()->newDataValue( $value );
}
- $claim->setMainSnak( SnakObject::newFromType( $snakType,
$constructorArguments ) );
+ try {
+ $snak = SnakObject::newFromType( $snakType,
$constructorArguments );
+ $claim->setMainSnak( $snak );
- $entity->setClaims( $claims );
+ $entity->setClaims( $claims );
- return $claim;
+ return $claim;
+ } catch ( IllegalValueException $ex ) {
+ $this->dieUsage( $ex->getMessage(),
'setclaim-invalid-snak' );
+ }
}
/**
diff --git a/repo/includes/api/SetQualifier.php
b/repo/includes/api/SetQualifier.php
index 301eecd..31891bd 100644
--- a/repo/includes/api/SetQualifier.php
+++ b/repo/includes/api/SetQualifier.php
@@ -3,6 +3,7 @@
namespace Wikibase\Api;
use ApiBase;
+use DataValues\IllegalValueException;
use MWException;
use ValueParsers\ParseException;
@@ -220,10 +221,16 @@
$snakValue = null;
}
- $factory = new SnakFactory();
- $newQualifier = $factory->newSnak( $propertyId, $snakType,
$snakValue );
+ try {
+ $factory = new SnakFactory();
+ $newQualifier = $factory->newSnak( $propertyId,
$snakType, $snakValue );
- return $qualifiers->addSnak( $newQualifier );
+ return $qualifiers->addSnak( $newQualifier );
+ } catch ( IllegalValueException $ex ) {
+ $this->dieUsage( $ex->getMessage(),
'setclaim-invalid-snak' );
+ }
+
+ return false; // we should never get here.
}
protected function getParsedEntityId( $prefixedId, $errorCode ) {
@@ -255,13 +262,19 @@
$propertyId = $this->getParsedEntityId( $params['property'],
'invalid-property-id' );
- $newQualifier = $factory->newSnak(
- $propertyId,
- $params['snaktype'],
- isset( $params['value'] ) ? \FormatJson::decode(
$params['value'], true ) : null
- );
+ try {
+ $newQualifier = $factory->newSnak(
+ $propertyId,
+ $params['snaktype'],
+ isset( $params['value'] ) ?
\FormatJson::decode( $params['value'], true ) : null
+ );
- return $qualifiers->addSnak( $newQualifier );
+ return $qualifiers->addSnak( $newQualifier );
+ } catch ( IllegalValueException $ex ) {
+ $this->dieUsage( $ex->getMessage(),
'setclaim-invalid-snak' );
+ }
+
+ return false; // we should never get here.
}
/**
diff --git a/repo/includes/api/SetReference.php
b/repo/includes/api/SetReference.php
index f6b0bef..da5abff 100644
--- a/repo/includes/api/SetReference.php
+++ b/repo/includes/api/SetReference.php
@@ -4,6 +4,7 @@
use ApiBase, MWException;
+use DataValues\IllegalValueException;
use Wikibase\EntityId;
use Wikibase\Entity;
use Wikibase\EntityContent;
@@ -128,13 +129,17 @@
$serializerFactory = new
\Wikibase\Lib\Serializers\SerializerFactory();
$snakUnserializer =
$serializerFactory->newUnserializerForClass( 'Wikibase\Snak' );
- foreach ( $rawSnaks as $byPropertySnaks ) {
- if ( !is_array( $byPropertySnaks ) ) {
- $this->dieUsage( 'Invalid snak JSON given',
'setreference-invalid-snaks' );
+ try {
+ foreach ( $rawSnaks as $byPropertySnaks ) {
+ if ( !is_array( $byPropertySnaks ) ) {
+ $this->dieUsage( 'Invalid snak JSON
given', 'setreference-invalid-snaks' );
+ }
+ foreach ( $byPropertySnaks as $rawSnak ) {
+ $snaks[] =
$snakUnserializer->newFromSerialization( $rawSnak );
+ }
}
- foreach ( $byPropertySnaks as $rawSnak ) {
- $snaks[] =
$snakUnserializer->newFromSerialization( $rawSnak );
- }
+ } catch ( IllegalValueException $ex ) {
+ $this->dieUsage( $ex->getMessage(),
'setreference-invalid-snaks' );
}
return $snaks;
diff --git a/repo/tests/phpunit/includes/api/CreateClaimTest.php
b/repo/tests/phpunit/includes/api/CreateClaimTest.php
index b2d7ac0..1cf4e17 100644
--- a/repo/tests/phpunit/includes/api/CreateClaimTest.php
+++ b/repo/tests/phpunit/includes/api/CreateClaimTest.php
@@ -123,7 +123,7 @@
'value' => '"foo"',
);
- $argLists[] = array( 'unknownerror', $params );
+ $argLists[] = array( 'claim-invalid-snak', $params );
$params = array(
'action' => 'wbcreateclaim',
@@ -167,6 +167,16 @@
$argLists[] = array( 'claim-value-missing', $params );
+ $params = array(
+ 'action' => 'wbcreateclaim',
+ 'entity' => '-',
+ 'snaktype' => 'value',
+ 'property' => '-',
+ 'value' => '{"x":"foo", "y":"bar"}',
+ );
+
+ $argLists[] = array( 'claim-invalid-snak', $params );
+
return $argLists;
}
--
To view, visit https://gerrit.wikimedia.org/r/68369
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I09e54dca33a3d143b1fa0d2f6ccf331ba8241a36
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
