jenkins-bot has submitted this change and it was merged.
Change subject: (bug 49264) Make SnakValidator fail on bad values.
......................................................................
(bug 49264) Make SnakValidator fail on bad values.
Snaks that contain a UnDeserializableValue, or whose
value type doesn't match the respective property's DataType,
should be considered invalid.
IMPORTANT: requires Id27f19020 in WikibaseDataModel
Change-Id: I1e2543c75f0407a22e531aac67ace9cf3f5670ae
---
M lib/WikibaseLib.i18n.php
M lib/includes/EntityRetrievingDataTypeLookup.php
M lib/includes/InMemoryDataTypeLookup.php
M repo/includes/Validators/SnakValidator.php
M repo/includes/api/CreateClaim.php
M repo/includes/api/SetClaimValue.php
M repo/includes/api/SetQualifier.php
M repo/tests/phpunit/includes/Validators/SnakValidatorTest.php
8 files changed, 109 insertions(+), 67 deletions(-)
Approvals:
Addshore: Looks good to me, approved
jenkins-bot: Verified
diff --git a/lib/WikibaseLib.i18n.php b/lib/WikibaseLib.i18n.php
index 6f475df..464c13d 100644
--- a/lib/WikibaseLib.i18n.php
+++ b/lib/WikibaseLib.i18n.php
@@ -60,6 +60,9 @@
'wikibase-validator-bad-entity-id' => 'Malformed ID: $1',
'wikibase-validator-bad-entity-type' => 'Unexpected entity type $1',
'wikibase-validator-no-such-entity' => '$1 not found',
+ 'wikibase-validator-no-such-property' => 'Property $1 not found',
+ 'wikibase-validator-bad-value' => 'Illegal value: $1',
+ 'wikibase-validator-bad-value-type' => 'Bad value type $1, expected $2',
'datatypes-type-wikibase-item' => 'Item',
'datatypes-type-commonsMedia' => 'Commons media file',
'version-wikibase' => 'Wikibase',
@@ -178,6 +181,23 @@
Parameters:
* $1 is the entity id',
+ 'wikibase-validator-no-such-property' => 'Input validation error shown
when the property used for a statement could not be found.
+This kind of error is unlekely to occur during normal operation, since the
user interface should prevent illegal values from being entered.
+
+Parameters:
+* $1 is the property id',
+ 'wikibase-validator-bad-value' => 'Input validation error shown when
the value is syntactically or structurally invalid.
+This kind of error is unlekely to occur during normal operation, since the
user interface should prevent illegal values from being entered.
+
+Parameters:
+* $1 is the technical error message describing the problem',
+ 'wikibase-validator-bad-value-type' => 'Input validation error shown
when the value has the wrong type for the property it is applied to.
+This kind of error is unlekely to occur during normal operation, since the
user interface should prevent illegal values from being entered.
+
+Parameters:
+* $1 is the actual value type
+* $2 is the expected value type',
+
'datatypes-type-wikibase-item' => 'The name of a data type for items in
Wikibase.
{{Identical|Item}}',
'datatypes-type-commonsMedia' => 'The name of a data type for media
files on Wikimedia Commons (proper name, capitalised in English; first letter
capitalised anyway in this message and relatives).',
diff --git a/lib/includes/EntityRetrievingDataTypeLookup.php
b/lib/includes/EntityRetrievingDataTypeLookup.php
index add731d..36c7843 100644
--- a/lib/includes/EntityRetrievingDataTypeLookup.php
+++ b/lib/includes/EntityRetrievingDataTypeLookup.php
@@ -51,6 +51,7 @@
* @param EntityId $propertyId
*
* @return string
+ * @throws PropertyNotFoundException
*/
public function getDataTypeIdForProperty( EntityId $propertyId ) {
$this->verifyIdIsOfAProperty( $propertyId );
diff --git a/lib/includes/InMemoryDataTypeLookup.php
b/lib/includes/InMemoryDataTypeLookup.php
index caf9418..1ca08d8 100644
--- a/lib/includes/InMemoryDataTypeLookup.php
+++ b/lib/includes/InMemoryDataTypeLookup.php
@@ -3,7 +3,6 @@
namespace Wikibase\Lib;
use InvalidArgumentException;
-use OutOfBoundsException;
use Wikibase\EntityId;
use Wikibase\Property;
@@ -70,7 +69,7 @@
$numericId = $propertyId->getNumericId();
if ( !array_key_exists( $numericId, $this->dataTypeIds ) ) {
- throw new PropertyNotFoundException( $propertyId );
+ throw new PropertyNotFoundException( $propertyId, "The
DataType for property '$numericId' is not set" );
}
}
diff --git a/repo/includes/Validators/SnakValidator.php
b/repo/includes/Validators/SnakValidator.php
index 243079e..3c426e3 100644
--- a/repo/includes/Validators/SnakValidator.php
+++ b/repo/includes/Validators/SnakValidator.php
@@ -28,12 +28,14 @@
use DataTypes\DataTypeFactory;
+use DataValues\UnDeserializableValue;
use DataValues\DataValue;
+use ValueValidators\Error;
use ValueValidators\Result;
use ValueValidators\ValueValidator;
use Wikibase\Claim;
use Wikibase\Lib\PropertyDataTypeLookup;
-use Wikibase\PropertyNoValueSnak;
+use Wikibase\Lib\PropertyNotFoundException;
use Wikibase\PropertyValueSnak;
use Wikibase\Reference;
use Wikibase\References;
@@ -162,27 +164,21 @@
// with a canValidate() method, to determine which
validator to use
// for a given snak.
- if ( $snak instanceof SnakObject ) {
+ $propertyId = $snak->getPropertyId();
- //NOTE: We only really need the DataType in case of
PropertyValueSnak,
- // but getDataTypeIdForProperty() is a quick way
to check if the property exists.
-
- //XXX: getDataTypeIdForProperty may throw a
PropertyNotFoundException.
- // Shall we catch that and report it using the
Result object?
-
- $propertyId = $snak->getPropertyId();
+ try {
$typeId =
$this->propertyDataTypeLookup->getDataTypeIdForProperty( $propertyId );
if ( $snak instanceof PropertyValueSnak ) {
$dataValue = $snak->getDataValue();
$result = $this->validateDataValue( $dataValue,
$typeId );
-
- //TODO: include property ID in any error report
} else {
$result = Result::newSuccess();
}
- } else {
- $result = Result::newSuccess();
+ } catch ( PropertyNotFoundException $ex ) {
+ $result = Result::newError( array(
+ Error::newError( "Property $propertyId not
found!", null, 'no-such-property', array( $propertyId ) )
+ ) );
}
return $result;
@@ -199,7 +195,18 @@
public function validateDataValue( DataValue $dataValue, $dataTypeId ) {
$dataType = $this->dataTypeFactory->getType( $dataTypeId );
- $result = Result::newSuccess();
+ if ( $dataValue instanceof UnDeserializableValue ) {
+ $result = Result::newError( array(
+ Error::newError( "Bad snak value: " .
$dataValue->getReason(), null, 'bad-value', array( $dataValue->getReason() ) )
+ ) );
+ } elseif ( $dataType->getDataValueType() !=
$dataValue->getType() ) {
+ $result = Result::newError( array(
+ Error::newError( "Bad value type: " .
$dataValue->getType() . ", expected " . $dataType->getDataValueType(),
+ null, 'bad-value-type', array(
$dataValue->getType(), $dataType->getDataValueType() ) )
+ ) );
+ } else {
+ $result = Result::newSuccess();
+ }
//XXX: Perhaps DataType should have a validate() method (even
implement ValueValidator)
// At least, DataType should expose only one validator,
which would be a CompositeValidator
diff --git a/repo/includes/api/CreateClaim.php
b/repo/includes/api/CreateClaim.php
index e5027e1..884ddb9 100644
--- a/repo/includes/api/CreateClaim.php
+++ b/repo/includes/api/CreateClaim.php
@@ -12,6 +12,7 @@
use Wikibase\Entity;
use Wikibase\EntityContent;
use Wikibase\EntityContentFactory;
+use Wikibase\Lib\PropertyNotFoundException;
use Wikibase\Property;
use Wikibase\Repo\WikibaseRepo;
use Wikibase\LibRegistry;
@@ -92,29 +93,26 @@
// entity type or providing an invalid DataValue.
try {
$snak = $this->getSnakInstance();
+
+ $this->snakValidation->validateSnak( $snak );
+
+ $claim = $this->addClaim( $entityContent->getEntity(),
$snak );
+ $summary = $this->createSummary( $snak, 'create' );
+
+ $this->saveChanges( $entityContent, $summary );
+
+ $this->outputClaim( $claim );
+
}
catch ( IllegalValueException $ex ) {
- wfProfileOut( __METHOD__ );
$this->dieUsage( 'Invalid snak: IllegalValueException',
'invalid-snak' );
}
- catch ( InvalidArgumentException $ex ) {
- // shouldn't happen, but might.
- wfProfileOut( __METHOD__ );
- $this->dieUsage( 'Invalid snak:
InvalidArgumentException', 'invalid-snak' );
+ catch ( PropertyNotFoundException $ex ) {
+ $this->dieUsage( 'Invalid snak:
PropertyNotFoundException', 'invalid-snak' );
}
catch ( ParseException $parseException ) {
- wfProfileOut( __METHOD__ );
$this->dieUsage( 'Invalid guid: ParseException',
'invalid-guid' );
}
-
- $this->snakValidation->validateSnak( $snak );
-
- $claim = $this->addClaim( $entityContent->getEntity(), $snak );
- $summary = $this->createSummary( $snak, 'create' );
-
- $this->saveChanges( $entityContent, $summary );
-
- $this->outputClaim( $claim );
wfProfileOut( __METHOD__ );
}
diff --git a/repo/includes/api/SetClaimValue.php
b/repo/includes/api/SetClaimValue.php
index 9f79964..495d8cd 100644
--- a/repo/includes/api/SetClaimValue.php
+++ b/repo/includes/api/SetClaimValue.php
@@ -6,11 +6,14 @@
use DataValues\IllegalValueException;
use ApiMain;
+use ValueParsers\ParseException;
use Wikibase\Autocomment;
use Wikibase\EntityId;
use Wikibase\Entity;
use Wikibase\EntityContent;
use Wikibase\EntityContentFactory;
+use Wikibase\Lib\PropertyNotFoundException;
+use Wikibase\Lib\SnakConstructionService;
use Wikibase\SnakObject;
use Wikibase\Claim;
use Wikibase\Claims;
@@ -53,6 +56,11 @@
protected $snakValidation;
/**
+ * @var SnakConstructionService
+ */
+ protected $snakConstruction;
+
+ /**
* see ApiBase::__construct()
*
* @param ApiMain $mainModule
@@ -68,6 +76,8 @@
WikibaseRepo::getDefaultInstance()->getDataTypeFactory(),
new ValidatorErrorLocalizer()
);
+
+ $this->snakConstruction =
WikibaseRepo::getDefaultInstance()->getSnakConstructionService();
}
/**
@@ -82,16 +92,29 @@
$params = $this->extractRequestParams();
- $claim = $this->updateClaim(
- $content->getEntity(),
- $params['claim'],
- $params['snaktype'],
- isset( $params['value'] ) ? \FormatJson::decode(
$params['value'], true ) : null
- );
+ try {
+ $claim = $this->updateClaim(
+ $content->getEntity(),
+ $params['claim'],
+ $params['snaktype'],
+ isset( $params['value'] ) ?
\FormatJson::decode( $params['value'], true ) : null
+ );
- $this->saveChanges( $content );
-
- $this->outputClaim( $claim );
+ $this->saveChanges( $content );
+ $this->outputClaim( $claim );
+ }
+ catch ( IllegalValueException $ex ) {
+ wfProfileOut( __METHOD__ );
+ $this->dieUsage( 'Invalid snak: IllegalValueException',
'invalid-snak' );
+ }
+ catch ( PropertyNotFoundException $ex ) {
+ wfProfileOut( __METHOD__ );
+ $this->dieUsage( 'Invalid snak:
PropertyNotFoundException', 'invalid-snak' );
+ }
+ catch ( ParseException $parseException ) {
+ wfProfileOut( __METHOD__ );
+ $this->dieUsage( 'Invalid guid: ParseException',
'invalid-guid' );
+ }
wfProfileOut( __METHOD__ );
}
@@ -134,7 +157,7 @@
* @param \Wikibase\Entity $entity
* @param string $guid
* @param string $snakType
- * @param string|null $value
+ * @param mixed $value
*
* @return \Wikibase\Claim
*/
@@ -146,27 +169,10 @@
}
$claim = $claims->getClaimWithGuid( $guid );
-
- $constructorArguments = array(
$claim->getMainSnak()->getPropertyId() );
-
- if ( $value !== null ) {
- /**
- * @var \Wikibase\PropertyContent $content
- */
- $content =
EntityContentFactory::singleton()->getFromId(
$claim->getMainSnak()->getPropertyId() );
-
- if ( $content === null ) {
- $this->dieUsage( 'The value cannot be
interpreted since the property cannot be found, and thus the type of the value
not be determined', 'no-such-property' );
- }
-
- $constructorArguments[] =
\DataValues\DataValueFactory::singleton()->newDataValue(
-
$content->getProperty()->getDataType()->getDataValueType(),
- $value
- );
- }
+ $oldSnak = $claim->getMainSnak();
try {
- $snak = SnakObject::newFromType( $snakType,
$constructorArguments ); //TODO: use SnakFactory
+ $snak = $this->snakConstruction->newSnak(
$oldSnak->getPropertyId(), $snakType, $value );
$this->snakValidation->validateSnak( $snak );
$claim->setMainSnak( $snak );
diff --git a/repo/includes/api/SetQualifier.php
b/repo/includes/api/SetQualifier.php
index d92999a..ea99e2e 100644
--- a/repo/includes/api/SetQualifier.php
+++ b/repo/includes/api/SetQualifier.php
@@ -13,6 +13,7 @@
use Wikibase\Entity;
use Wikibase\EntityContentFactory;
use Wikibase\Claim;
+use Wikibase\Lib\PropertyNotFoundException;
use Wikibase\Property;
use Wikibase\Repo\WikibaseRepo;
use Wikibase\Snak;
@@ -249,6 +250,8 @@
//Note: This handles failures during snak
instantiation, not validation.
// Validation errors are handled by the validation
helper.
$this->dieUsage( $illegalValueException->getMessage(),
'invalid-snak' );
+ } catch ( PropertyNotFoundException $ex ) {
+ $this->dieUsage( $ex->getMessage(), 'invalid-snak' );
}
return false; // we should never get here.
diff --git a/repo/tests/phpunit/includes/Validators/SnakValidatorTest.php
b/repo/tests/phpunit/includes/Validators/SnakValidatorTest.php
index 27250fb..27f691a 100644
--- a/repo/tests/phpunit/includes/Validators/SnakValidatorTest.php
+++ b/repo/tests/phpunit/includes/Validators/SnakValidatorTest.php
@@ -4,12 +4,10 @@
use DataTypes\DataType;
use DataTypes\DataTypeFactory;
+use DataValues\UnDeserializableValue;
use DataValues\DataValue;
use DataValues\StringValue;
-use ValueValidators\Error;
-use ValueValidators\Result;
-use ValueValidators\StringValidator;
-use ValueValidators\ValueValidator;
+use DataValues\UnknownValue;
use Wikibase\Claim;
use Wikibase\EntityId;
use Wikibase\Lib\InMemoryDataTypeLookup;
@@ -228,6 +226,7 @@
public static function provideValidate() {
$p1 = new EntityId( Property::ENTITY_TYPE, 1 ); // numeric
$p2 = new EntityId( Property::ENTITY_TYPE, 2 ); // alphabetic
+ $p3 = new EntityId( Property::ENTITY_TYPE, 3 ); // bad
$cases = array();
@@ -249,6 +248,15 @@
$snak = new PropertyValueSnak( $p1, new StringValue( 'abc' ) );
$cases[] = array( $snak, 'invalid numeric value', false );
+ $snak = new PropertyValueSnak( $p1, new UnDeserializableValue(
'abc', 'string', 'error' ) );
+ $cases[] = array( $snak, 'bad value', false );
+
+ $snak = new PropertyValueSnak( $p1, new UnknownValue( 'abc' ) );
+ $cases[] = array( $snak, 'wrong value type', false );
+
+ $snak = new PropertyValueSnak( $p3, new StringValue( 'abc' ) );
+ $cases[] = array( $snak, 'bad property', false );
+
return $cases;
}
@@ -269,9 +277,9 @@
array( new StringValue( '123' ), 'alphabetic', 'p2',
'invalid alphabetic value', false ),
array( new StringValue( 'abc' ), 'alphabetic', 'p2',
'valid alphabetic value', true ),
array( new StringValue( 'abc' ), 'numeric', 'p1',
'invalid numeric value', false ),
-
- //XXX: string length check is currently broken
- //array( new StringValue( '01234567890123456789' ),
'numeric', 'p1', 'overly long numeric value', false ),
+ array( new StringValue( '01234567890123456789' ),
'numeric', 'p1', 'overly long numeric value', false ),
+ array( new UnknownValue( 'abc' ), 'alphabetic', 'p2',
'bad value type', false ),
+ array( new UnDeserializableValue( 'abc', 'string',
'error' ), 'numeric', 'p1', 'bad value', false ),
);
}
--
To view, visit https://gerrit.wikimedia.org/r/69659
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I1e2543c75f0407a22e531aac67ace9cf3f5670ae
Gerrit-PatchSet: 12
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Addshore <[email protected]>
Gerrit-Reviewer: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Daniel Werner <[email protected]>
Gerrit-Reviewer: Jeroen De Dauw <[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
