Skip to content

Commit

Permalink
REST: Use specific PATCH error for missing property
Browse files Browse the repository at this point in the history
This replaces the 'patch-result-invalid-value' error code with
'patch-result-referenced-resource-not-found' when a PATCH request
refers to a non-existent property in any of its statements,
qualifiers or references.

Bug: T373884
Change-Id: If0fd40027aabed9858b2a2d39ab40ffb44db61ce
  • Loading branch information
Silvan-WMDE authored and jakobw committed Sep 16, 2024
1 parent a52806b commit 66531f7
Show file tree
Hide file tree
Showing 25 changed files with 420 additions and 47 deletions.
4 changes: 3 additions & 1 deletion repo/rest-api/specs/global/responses.json
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,7 @@
},
"patch-result-invalid-key": { "$ref": "./examples.json#/PatchResultInvalidKeyExample" },
"patch-result-invalid-value": { "$ref": "./examples.json#/PatchResultInvalidValueExample" },
"patch-result-referenced-resource-not-found": { "$ref": "./examples.json#/PatchResultResourceNotFoundExample" },
"patch-result-value-too-long": { "$ref": "./examples.json#/PatchResultValueTooLongExample" },
"patch-result-modified-read-only-value": {
"$ref": "./examples.json#/PatchResultModifiedReadOnlyValue"
Expand Down Expand Up @@ -493,7 +494,8 @@
"patch-result-invalid-value": { "$ref": "./examples.json#/PatchResultInvalidValueExample" },
"patch-result-modified-read-only-value": {
"$ref": "./examples.json#/PatchResultModifiedReadOnlyValue"
}
},
"patch-result-referenced-resource-not-found": { "$ref": "./examples.json#/PatchResultResourceNotFoundExample" }
}
}
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?php declare( strict_types=1 );

namespace Wikibase\Repo\RestApi\Application\Serialization\Exceptions;

use Throwable;

/**
* @license GPL-2.0-or-later
*/
class PropertyNotFoundException extends SerializationException {
private string $path;
private string $value;

public function __construct( string $value, string $path = '', string $message = '', Throwable $previous = null ) {
$this->value = $value;
$this->path = $path;

parent::__construct( $message, 0, $previous );
}

public function getValue(): string {
return $this->value;
}

public function getPath(): string {
return $this->path;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use Wikibase\DataModel\Snak\Snak;
use Wikibase\Repo\RestApi\Application\Serialization\Exceptions\InvalidFieldException;
use Wikibase\Repo\RestApi\Application\Serialization\Exceptions\MissingFieldException;
use Wikibase\Repo\RestApi\Application\Serialization\Exceptions\PropertyNotFoundException;
use Wikibase\Repo\RestApi\Domain\ReadModel\Value;

/**
Expand All @@ -37,6 +38,7 @@ public function __construct(
/**
* @throws MissingFieldException
* @throws InvalidFieldException
* @throws PropertyNotFoundException
*/
public function deserialize( array $serialization, string $basePath = '' ): Snak {
$this->validateSerialization( $serialization, $basePath );
Expand All @@ -46,7 +48,7 @@ public function deserialize( array $serialization, string $basePath = '' ): Snak
try {
$dataTypeId = $this->dataTypeLookup->getDataTypeIdForProperty( $propertyId );
} catch ( Exception $e ) {
throw new InvalidFieldException( 'id', $serialization['property']['id'], "$basePath/property/id" );
throw new PropertyNotFoundException( $serialization['property']['id'], "$basePath/property/id" );
}

switch ( $serialization['value']['type'] ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use Wikibase\DataModel\Reference;
use Wikibase\Repo\RestApi\Application\Serialization\Exceptions\InvalidFieldException;
use Wikibase\Repo\RestApi\Application\Serialization\Exceptions\MissingFieldException;
use Wikibase\Repo\RestApi\Application\Serialization\Exceptions\PropertyNotFoundException;

/**
* @license GPL-2.0-or-later
Expand All @@ -20,6 +21,7 @@ public function __construct( PropertyValuePairDeserializer $propertyValuePairDes
/**
* @throws MissingFieldException
* @throws InvalidFieldException
* @throws PropertyNotFoundException
*/
public function deserialize( array $serialization, string $basePath = '' ): Reference {
if ( count( $serialization ) && array_is_list( $serialization ) ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use Wikibase\Repo\RestApi\Application\Serialization\Exceptions\InvalidFieldException;
use Wikibase\Repo\RestApi\Application\Serialization\Exceptions\InvalidFieldTypeException;
use Wikibase\Repo\RestApi\Application\Serialization\Exceptions\MissingFieldException;
use Wikibase\Repo\RestApi\Application\Serialization\Exceptions\PropertyNotFoundException;

/**
* @license GPL-2.0-or-later
Expand All @@ -29,6 +30,7 @@ public function __construct(
* @throws InvalidFieldTypeException
* @throws InvalidFieldException
* @throws MissingFieldException
* @throws PropertyNotFoundException
*/
public function deserialize( array $serialization, string $basePath = '' ): Statement {
if ( count( $serialization ) && array_is_list( $serialization ) ) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ private function handleStatementsValidationErrors( ValidationError $validationEr
throw UseCaseError::newInvalidValue( $context[StatementsValidator::CONTEXT_PATH] );
case StatementValidator::CODE_INVALID_FIELD:
throw UseCaseError::newInvalidValue( $context[StatementValidator::CONTEXT_PATH] );
case StatementValidator::CODE_PROPERTY_NOT_FOUND:
case StatementValidator::CODE_INVALID_FIELD_TYPE:
throw UseCaseError::newInvalidValue( $context[StatementValidator::CONTEXT_PATH] );
case StatementValidator::CODE_MISSING_FIELD:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public function validateAndDeserialize( StatementSerializationRequest $request )
if ( $validationError ) {
$context = $validationError->getContext();
switch ( $validationError->getCode() ) {
case StatementValidator::CODE_PROPERTY_NOT_FOUND:
case StatementValidator::CODE_INVALID_FIELD:
throw UseCaseError::newInvalidValue( $context[StatementValidator::CONTEXT_PATH] );
case StatementValidator::CODE_MISSING_FIELD:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -395,45 +395,54 @@ private function assertValidStatements( array $serialization, Item $originalItem
$context[StatementValidator::CONTEXT_PATH],
$context[StatementValidator::CONTEXT_FIELD]
);
}
}

// get StatementIds for all Statements in a StatementList, removing any that are null
$getStatementIds = fn( StatementList $statementList ) => array_filter( array_map(
fn( Statement $statement ) => $statement->getGuid(),
iterator_to_array( $statementList )
) );
case StatementValidator::CODE_PROPERTY_NOT_FOUND:
throw UseCaseError::newPatchResultReferencedResourceNotFound(
$context[StatementValidator::CONTEXT_PATH],
$context[StatementValidator::CONTEXT_VALUE]
);

$getStatementIdPath = function( array $serialization, string $id ): string {
foreach ( $serialization as $propertyId => $statementGroup ) {
foreach ( $statementGroup as $groupIndex => $statement ) {
if ( isset( $statement['id'] ) && $statement['id'] === $id ) {
return "/statements/$propertyId/$groupIndex";
}
}
default:
throw new LogicException( "Unknown validation error code: {$validationError->getCode()}" );
}

throw new LogicException( "Statement ID '$id' not found in patch result" );
};
}

$originalStatements = $originalItem->getStatements();
$originalStatementsIds = $getStatementIds( $originalStatements );
$originalStatementsIds = $this->getStatementIds( $originalStatements );
$patchedStatements = $this->statementsValidator->getValidatedStatements();
$patchedStatementsIds = $getStatementIds( $patchedStatements );
$patchedStatementsIds = $this->getStatementIds( $patchedStatements );
foreach ( array_count_values( $patchedStatementsIds ) as $id => $occurrence ) {
if ( $occurrence > 1 || !in_array( $id, $originalStatementsIds ) ) {
$path = "{$getStatementIdPath( $serialization['statements'], $id )}/id";
$path = "{$this->getStatementIdPath( $serialization['statements'], $id )}/id";
throw UseCaseError::newPatchResultModifiedReadOnlyValue( $path );
}

$originalPropertyId = $originalStatements->getFirstStatementWithGuid( $id )->getPropertyId();
if ( !$patchedStatements->getFirstStatementWithGuid( $id )->getPropertyId()->equals(
$originalPropertyId
) ) {
$path = "{$getStatementIdPath( $serialization['statements'], $id )}/property/id";
$path = "{$this->getStatementIdPath( $serialization['statements'], $id )}/property/id";
throw UseCaseError::newPatchResultModifiedReadOnlyValue( $path );
}
}
}

private function getStatementIds( StatementList $statementList ): array {
return array_filter( array_map(
fn( Statement $statement ) => $statement->getGuid(),
iterator_to_array( $statementList )
) );
}

private function getStatementIdPath( array $serialization, string $id ): string {
foreach ( $serialization as $propertyId => $statementGroup ) {
foreach ( $statementGroup as $groupIndex => $statement ) {
if ( isset( $statement['id'] ) && $statement['id'] === $id ) {
return "/statements/$propertyId/$groupIndex";
}
}
}

throw new LogicException( "Statement ID '$id' not found in patch result" );
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,14 @@ private function assertValidStatements(
UseCaseError::CONTEXT_STATEMENT_PROPERTY_ID => $context[ StatementsValidator::CONTEXT_PROPERTY_ID_VALUE ],
]
);
case StatementValidator::CODE_PROPERTY_NOT_FOUND:
throw UseCaseError::newPatchResultReferencedResourceNotFound(
$context[StatementValidator::CONTEXT_PATH],
$context[StatementValidator::CONTEXT_VALUE]
);

default:
throw new LogicException( "Unknown validation error code: {$validationError->getCode()}" );
}
}

Expand All @@ -316,31 +324,31 @@ private function assertValidStatements(
$originalStatementsIds = $getStatementIds( $originalStatements );
$patchedStatementsIds = $getStatementIds( $patchedStatements );

$getStatementIdPath = function( array $serialization, string $id ): string {
foreach ( $serialization as $propertyId => $statementGroup ) {
foreach ( $statementGroup as $groupIndex => $statement ) {
if ( isset( $statement['id'] ) && $statement['id'] === $id ) {
return "/statements/$propertyId/$groupIndex";
}
}
}

throw new LogicException( "Statement ID '$id' not found in patch result" );
};

foreach ( array_count_values( $patchedStatementsIds ) as $id => $occurrence ) {
if ( $occurrence > 1 || !in_array( $id, $originalStatementsIds ) ) {
$path = "{$getStatementIdPath( $statementsSerialization, $id )}/id";
$path = "{$this->getStatementIdPath( $statementsSerialization, $id )}/id";
throw UseCaseError::newPatchResultModifiedReadOnlyValue( $path );

}

$originalPropertyId = $originalStatements->getFirstStatementWithGuid( $id )->getPropertyId();
if ( !$patchedStatements->getFirstStatementWithGuid( $id )->getPropertyId()->equals( $originalPropertyId ) ) {
$path = "{$getStatementIdPath( $statementsSerialization, $id )}/property/id";
$path = "{$this->getStatementIdPath( $statementsSerialization, $id )}/property/id";
throw UseCaseError::newPatchResultModifiedReadOnlyValue( $path );
}
}
}

private function getStatementIdPath( array $serialization, string $id ): string {
foreach ( $serialization as $propertyId => $statementGroup ) {
foreach ( $statementGroup as $groupIndex => $statement ) {
if ( isset( $statement['id'] ) && $statement['id'] === $id ) {
return "/statements/$propertyId/$groupIndex";
}
}
}

throw new LogicException( "Statement ID '$id' not found in patch result" );
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ public function validateAndDeserializeStatement( $patchedStatement ): Statement
$context[StatementValidator::CONTEXT_PATH],
$context[StatementValidator::CONTEXT_VALUE]
);
case StatementValidator::CODE_PROPERTY_NOT_FOUND:
throw UseCaseError::newPatchResultReferencedResourceNotFound(
$context[StatementValidator::CONTEXT_PATH],
$context[StatementValidator::CONTEXT_VALUE]
);

default:
throw new LogicException( "Unexpected validation error code: {$validationError->getCode()}" );
}
Expand Down
10 changes: 10 additions & 0 deletions repo/rest-api/src/Application/Validation/StatementValidator.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Wikibase\Repo\RestApi\Application\Serialization\Exceptions\InvalidFieldException;
use Wikibase\Repo\RestApi\Application\Serialization\Exceptions\InvalidFieldTypeException;
use Wikibase\Repo\RestApi\Application\Serialization\Exceptions\MissingFieldException;
use Wikibase\Repo\RestApi\Application\Serialization\Exceptions\PropertyNotFoundException;
use Wikibase\Repo\RestApi\Application\Serialization\StatementDeserializer;

/**
Expand All @@ -17,6 +18,7 @@ class StatementValidator {
public const CODE_INVALID_FIELD = 'statement-validator-code-invalid-statement-field';
public const CODE_MISSING_FIELD = 'statement-validator-code-missing-statement-field';
public const CODE_INVALID_FIELD_TYPE = 'statement-validator-code-invalid-statement-type';
public const CODE_PROPERTY_NOT_FOUND = 'statement-validator-code-property-not-found';

public const CONTEXT_FIELD = 'statement-validator-context-field';
public const CONTEXT_PATH = 'statement-validator-context-path';
Expand Down Expand Up @@ -55,6 +57,14 @@ public function validate( array $statementSerialization, string $basePath = '' )
self::CONTEXT_VALUE => $e->getValue(),
]
);
} catch ( PropertyNotFoundException $e ) {
return new ValidationError(
self::CODE_PROPERTY_NOT_FOUND,
[
self::CONTEXT_PATH => $e->getPath(),
self::CONTEXT_VALUE => $e->getValue(),
]
);
}

return null;
Expand Down
6 changes: 6 additions & 0 deletions repo/rest-api/src/RouteHandlers/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -5002,6 +5002,9 @@
"patch-result-invalid-value": {
"$ref": "#/components/examples/PatchResultInvalidValueExample"
},
"patch-result-referenced-resource-not-found": {
"$ref": "#/components/examples/PatchResultResourceNotFoundExample"
},
"patch-result-value-too-long": {
"$ref": "#/components/examples/PatchResultValueTooLongExample"
},
Expand Down Expand Up @@ -5158,6 +5161,9 @@
},
"patch-result-modified-read-only-value": {
"$ref": "#/components/examples/PatchResultModifiedReadOnlyValue"
},
"patch-result-referenced-resource-not-found": {
"$ref": "#/components/examples/PatchResultResourceNotFoundExample"
}
}
}
Expand Down
38 changes: 38 additions & 0 deletions repo/rest-api/tests/mocha/api-testing/PatchItemStatementTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,44 @@ describe( 'PATCH statement tests', () => {
} );
} );

it( 'rejects qualifier with non-existent property', async () => {
const nonExistentProperty = 'P9999999';
const patch = [ {
op: 'add',
path: '/qualifiers',
value: [ { property: { id: nonExistentProperty }, value: { type: 'novalue' } } ]
} ];
const response = await newPatchRequestBuilder( testStatementId, patch )
.assertValidRequest().makeRequest();

assertValidError(
response,
422,
'patch-result-referenced-resource-not-found',
{ path: '/qualifiers/0/property/id', value: nonExistentProperty }
);
assert.strictEqual( response.body.message, 'The referenced resource does not exist' );
} );

it( 'rejects reference with non-existent property', async () => {
const nonExistentProperty = 'P9999999';
const patch = [ {
op: 'add',
path: '/references/0',
value: { parts: [ { property: { id: nonExistentProperty }, value: { type: 'novalue' } } ] }
} ];
const response = await newPatchRequestBuilder( testStatementId, patch )
.assertValidRequest().makeRequest();

assertValidError(
response,
422,
'patch-result-referenced-resource-not-found',
{ path: '/references/0/parts/0/property/id', value: nonExistentProperty }
);
assert.strictEqual( response.body.message, 'The referenced resource does not exist' );
} );

} );

} );
Expand Down
Loading

0 comments on commit 66531f7

Please sign in to comment.