Skip to content

Commit c128c50

Browse files
outdooracornoutdooracorn
committed
REST: Improve JsonPatchValidator errors
Use the improved error handling we added to swaggest/json-diff to improve errors during patch validation Bug: T320705 Change-Id: I644e985bb1f98f9872b2f9c6435b2e3f30df813c
1 parent 6ac4b05 commit c128c50

File tree

10 files changed

+201
-9
lines changed

10 files changed

+201
-9
lines changed

‎repo/rest-api/src/Infrastructure/JsonDiffJsonPatchValidator.php‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,11 @@
44

55
use Swaggest\JsonDiff\Exception;
66
use Swaggest\JsonDiff\JsonPatch;
7+
use Swaggest\JsonDiff\MissingFieldException;
8+
use Swaggest\JsonDiff\UnknownOperationException;
79
use Wikibase\Repo\RestApi\Domain\Services\JsonPatchValidator;
10+
use Wikibase\Repo\RestApi\Validation\PatchInvalidOpValidationError;
11+
use Wikibase\Repo\RestApi\Validation\PatchMissingFieldValidationError;
812
use Wikibase\Repo\RestApi\Validation\ValidationError;
913

1014
/**
@@ -15,6 +19,18 @@ class JsonDiffJsonPatchValidator implements JsonPatchValidator {
1519
public function validate( array $patch, string $source ): ?ValidationError {
1620
try {
1721
JsonPatch::import( $patch );
22+
} catch ( MissingFieldException $e ) {
23+
return new PatchMissingFieldValidationError(
24+
$e->getMissingField(),
25+
$source,
26+
[ 'operation' => (array)$e->getOperation() ]
27+
);
28+
} catch ( UnknownOperationException $e ) {
29+
return new PatchInvalidOpValidationError(
30+
$e->getOperation()->op,
31+
$source,
32+
[ 'operation' => (array)$e->getOperation() ]
33+
);
1834
} catch ( Exception $e ) {
1935
return new ValidationError( '', $source );
2036
}

‎repo/rest-api/src/Presentation/ErrorResponseToHttpStatus.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@ class ErrorResponseToHttpStatus {
2121
ErrorResponse::INVALID_OPERATION_CHANGED_STATEMENT_ID => 400,
2222
ErrorResponse::INVALID_OPERATION_CHANGED_PROPERTY => 400,
2323
ErrorResponse::INVALID_PATCH => 400,
24+
ErrorResponse::INVALID_PATCH_OPERATION => 400,
25+
ErrorResponse::MISSING_JSON_PATCH_FIELD => 400,
2426
ErrorResponse::PERMISSION_DENIED => 403,
2527
ErrorResponse::ITEM_NOT_FOUND => 404,
2628
ErrorResponse::ITEM_REDIRECTED => 409,

‎repo/rest-api/src/UseCases/ErrorResponse.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ class ErrorResponse {
1919
public const CANNOT_APPLY_PATCH = 'cannot-apply-patch';
2020
public const PATCH_TEST_FAILED = 'patch-test-failed';
2121
public const INVALID_PATCH = 'invalid-patch';
22+
public const INVALID_PATCH_OPERATION = 'invalid-patch-operation';
23+
public const MISSING_JSON_PATCH_FIELD = 'missing-json-patch-field';
2224
public const ITEM_NOT_FOUND = 'item-not-found';
2325
public const ITEM_REDIRECTED = 'redirected-item';
2426
public const PERMISSION_DENIED = 'permission-denied';

‎repo/rest-api/src/UseCases/PatchItemStatement/PatchItemStatementErrorResponse.php‎

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
use LogicException;
66
use Wikibase\Repo\RestApi\UseCases\ErrorResponse;
7+
use Wikibase\Repo\RestApi\Validation\PatchInvalidOpValidationError;
8+
use Wikibase\Repo\RestApi\Validation\PatchMissingFieldValidationError;
79
use Wikibase\Repo\RestApi\Validation\ValidationError;
810

911
/**
@@ -27,11 +29,27 @@ public static function newFromValidationError( ValidationError $validationError
2729
);
2830

2931
case PatchItemStatementValidator::SOURCE_PATCH:
30-
return new self(
31-
ErrorResponse::INVALID_PATCH,
32-
"The provided patch is invalid"
33-
);
34-
32+
switch ( true ) {
33+
case $validationError instanceof PatchInvalidOpValidationError:
34+
$op = $validationError->getValue();
35+
return new self(
36+
ErrorResponse::INVALID_PATCH_OPERATION,
37+
"Incorrect JSON patch operation: '$op'",
38+
$validationError->getContext()
39+
);
40+
case $validationError instanceof PatchMissingFieldValidationError:
41+
$field = $validationError->getValue();
42+
return new self(
43+
ErrorResponse::MISSING_JSON_PATCH_FIELD,
44+
"Missing '$field' in JSON patch",
45+
$validationError->getContext()
46+
);
47+
default:
48+
return new self(
49+
ErrorResponse::INVALID_PATCH,
50+
"The provided patch is invalid"
51+
);
52+
}
3553
case PatchItemStatementValidator::SOURCE_EDIT_TAGS:
3654
return new self(
3755
ErrorResponse::INVALID_EDIT_TAG,

‎repo/rest-api/src/Validation/PatchInvalidOpValidationError.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<?php declare( strict_types=1 );
2+
3+
namespace Wikibase\Repo\RestApi\Validation;
4+
5+
/**
6+
* @license GPL-2.0-or-later
7+
*/
8+
class PatchInvalidOpValidationError extends ValidationError {
9+
10+
}

‎repo/rest-api/src/Validation/PatchMissingFieldValidationError.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<?php declare( strict_types=1 );
2+
3+
namespace Wikibase\Repo\RestApi\Validation;
4+
5+
/**
6+
* @license GPL-2.0-or-later
7+
*/
8+
class PatchMissingFieldValidationError extends ValidationError {
9+
10+
}

‎repo/rest-api/src/Validation/ValidationError.php‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,12 @@
88
class ValidationError {
99
private string $value;
1010
private string $source;
11+
private ?array $context;
1112

12-
public function __construct( string $value, string $source ) {
13+
public function __construct( string $value, string $source, array $context = null ) {
1314
$this->value = $value;
1415
$this->source = $source;
16+
$this->context = $context;
1517
}
1618

1719
public function getValue(): string {
@@ -22,4 +24,7 @@ public function getSource(): string {
2224
return $this->source;
2325
}
2426

27+
public function getContext(): ?array {
28+
return $this->context;
29+
}
2530
}

‎repo/rest-api/tests/mocha/api-testing/PatchItemStatementTest.js‎

Lines changed: 52 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,10 +14,15 @@ function makeEtag( ...revisionIds ) {
1414
return revisionIds.map( ( revId ) => `"${revId}"` ).join( ',' );
1515
}
1616

17-
function assertValid400Response( response, responseBodyErrorCode ) {
17+
function assertValid400Response( response, responseBodyErrorCode, context = null ) {
1818
assert.strictEqual( response.status, 400 );
1919
assert.header( response, 'Content-Language', 'en' );
2020
assert.strictEqual( response.body.code, responseBodyErrorCode );
21+
if ( context === null ) {
22+
assert.notProperty( response.body, 'context' );
23+
} else {
24+
assert.deepStrictEqual( response.body.context, context );
25+
}
2126
}
2227

2328
function assertValid404Response( response, responseBodyErrorCode ) {
@@ -187,13 +192,58 @@ describe( 'PATCH statement tests', () => {
187192
} );
188193

189194
it( 'invalid patch', async () => {
190-
const invalidPatch = { patch: 'this is not a valid JSON Patch' };
195+
const invalidPatch = { foo: 'this is not a valid JSON Patch' };
191196
const response = await newPatchRequestBuilder( testStatementId, invalidPatch )
192197
.assertInvalidRequest().makeRequest();
193198

194199
assertValid400Response( response, 'invalid-patch' );
195200
} );
196201

202+
it( "invalid patch - missing 'op' field", async () => {
203+
const invalidOperation = { path: '/a/b/c', value: 'test' };
204+
const response = await newPatchRequestBuilder( testStatementId, [ invalidOperation ] )
205+
.assertInvalidRequest().makeRequest();
206+
207+
assertValid400Response( response, 'missing-json-patch-field', { operation: invalidOperation } );
208+
assert.include( response.body.message, "'op'" );
209+
} );
210+
211+
it( "invalid patch - missing 'path' field", async () => {
212+
const invalidOperation = { op: 'remove' };
213+
const response = await newPatchRequestBuilder( testStatementId, [ invalidOperation ] )
214+
.assertInvalidRequest().makeRequest();
215+
216+
assertValid400Response( response, 'missing-json-patch-field', { operation: invalidOperation } );
217+
assert.include( response.body.message, "'path'" );
218+
} );
219+
220+
it( "invalid patch - missing 'value' field", async () => {
221+
const invalidOperation = { op: 'add', path: '/a/b/c' };
222+
const response = await newPatchRequestBuilder( testStatementId, [ invalidOperation ] )
223+
.makeRequest();
224+
225+
assertValid400Response( response, 'missing-json-patch-field', { operation: invalidOperation } );
226+
assert.include( response.body.message, "'value'" );
227+
} );
228+
229+
it( "invalid patch - missing 'from' field", async () => {
230+
const invalidOperation = { op: 'move', path: '/a/b/c' };
231+
const response = await newPatchRequestBuilder( testStatementId, [ invalidOperation ] )
232+
.makeRequest();
233+
234+
assertValid400Response( response, 'missing-json-patch-field', { operation: invalidOperation } );
235+
assert.include( response.body.message, "'from'" );
236+
} );
237+
238+
it( "invalid patch - invalid 'op' field", async () => {
239+
const invalidOperation = { op: 'foobar', path: '/a/b/c', value: 'test' };
240+
const response = await newPatchRequestBuilder( testStatementId, [ invalidOperation ] )
241+
.assertInvalidRequest().makeRequest();
242+
243+
assertValid400Response( response, 'invalid-patch-operation', { operation: invalidOperation } );
244+
assert.include( response.body.message, "'foobar'" );
245+
} );
246+
197247
it( 'invalid edit tag', async () => {
198248
const invalidEditTag = 'invalid tag';
199249
const response = await newPatchRequestBuilder( testStatementId, [] )

‎repo/rest-api/tests/phpunit/Infrastructure/JsonDiffJsonPatchValidatorTest.php‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,12 @@
22

33
namespace Wikibase\Repo\Tests\RestApi\Infrastructure;
44

5+
use Generator;
56
use PHPUnit\Framework\TestCase;
67
use Swaggest\JsonDiff\JsonDiff;
78
use Wikibase\Repo\RestApi\Infrastructure\JsonDiffJsonPatchValidator;
9+
use Wikibase\Repo\RestApi\Validation\PatchInvalidOpValidationError;
10+
use Wikibase\Repo\RestApi\Validation\PatchMissingFieldValidationError;
811

912
/**
1013
* @covers \Wikibase\Repo\RestApi\Infrastructure\JsonDiffJsonPatchValidator
@@ -28,6 +31,62 @@ public function testInvalidJsonPatch(): void {
2831

2932
$this->assertSame( $source, $error->getSource() );
3033
$this->assertEmpty( $error->getValue() );
34+
$this->assertNull( $error->getContext() );
35+
}
36+
37+
/**
38+
* @dataProvider provideInvalidJsonPatch
39+
*/
40+
public function testInvalidJsonPatch_specificExceptions( string $errorInstance, array $patch, string $value, ?array $context ): void {
41+
$source = 'test source';
42+
$error = ( new JsonDiffJsonPatchValidator() )->validate( $patch, $source );
43+
44+
$this->assertInstanceOf( $errorInstance, $error );
45+
$this->assertSame( $source, $error->getSource() );
46+
$this->assertSame( $value, $error->getValue() );
47+
$this->assertSame( $context, $error->getContext() );
48+
}
49+
50+
public function provideInvalidJsonPatch(): Generator {
51+
$invalidOperationObject = [ 'path' => '/a/b/c', 'value' => 'foo' ];
52+
yield 'missing "op" field' => [
53+
PatchMissingFieldValidationError::class,
54+
[ $invalidOperationObject ],
55+
'op',
56+
[ 'operation' => $invalidOperationObject ]
57+
];
58+
59+
$invalidOperationObject = [ 'op' => 'add', 'value' => 'foo' ];
60+
yield 'missing "path" field' => [
61+
PatchMissingFieldValidationError::class,
62+
[ $invalidOperationObject ],
63+
'path',
64+
[ 'operation' => $invalidOperationObject ]
65+
];
66+
67+
$invalidOperationObject = [ 'op' => 'add', 'path' => '/a/b/c' ];
68+
yield 'missing "value" field' => [
69+
PatchMissingFieldValidationError::class,
70+
[ $invalidOperationObject ],
71+
'value',
72+
[ 'operation' => $invalidOperationObject ]
73+
];
74+
75+
$invalidOperationObject = [ 'op' => 'copy', 'path' => '/a/b/c' ];
76+
yield 'missing "from" field' => [
77+
PatchMissingFieldValidationError::class,
78+
[ $invalidOperationObject ],
79+
'from',
80+
[ 'operation' => $invalidOperationObject ]
81+
];
82+
83+
$invalidOperationObject = [ 'op' => 'invalid', 'path' => '/a/b/c', 'value' => 'foo' ];
84+
yield 'invalid "op" field' => [
85+
PatchInvalidOpValidationError::class,
86+
[ $invalidOperationObject ],
87+
'invalid',
88+
[ 'operation' => $invalidOperationObject ]
89+
];
3190
}
3291

3392
public function testValidJsonPatch(): void {

‎repo/rest-api/tests/phpunit/UseCases/PatchItemStatement/PatchItemStatementErrorResponseTest.php‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
use Wikibase\Repo\RestApi\UseCases\ErrorResponse;
77
use Wikibase\Repo\RestApi\UseCases\PatchItemStatement\PatchItemStatementErrorResponse;
88
use Wikibase\Repo\RestApi\UseCases\PatchItemStatement\PatchItemStatementValidator;
9+
use Wikibase\Repo\RestApi\Validation\PatchInvalidOpValidationError;
10+
use Wikibase\Repo\RestApi\Validation\PatchMissingFieldValidationError;
911
use Wikibase\Repo\RestApi\Validation\ValidationError;
1012

1113
/**
@@ -23,12 +25,14 @@ class PatchItemStatementErrorResponseTest extends TestCase {
2325
public function testNewFromValidationError(
2426
ValidationError $validationError,
2527
string $expectedCode,
26-
string $expectedMessage
28+
string $expectedMessage,
29+
array $expectedContext = null
2730
): void {
2831
$response = PatchItemStatementErrorResponse::newFromValidationError( $validationError );
2932

3033
$this->assertEquals( $expectedCode, $response->getCode() );
3134
$this->assertEquals( $expectedMessage, $response->getMessage() );
35+
$this->assertEquals( $expectedContext, $response->getContext() );
3236
}
3337

3438
public function provideValidationError(): \Generator {
@@ -50,6 +54,22 @@ public function provideValidationError(): \Generator {
5054
'The provided patch is invalid'
5155
];
5256

57+
$context = [ 'operation' => [ 'path' => '/a/b/c', 'value' => 'test' ] ];
58+
yield 'from missing patch field' => [
59+
new PatchMissingFieldValidationError( 'op', PatchItemStatementValidator::SOURCE_PATCH, $context ),
60+
ErrorResponse::MISSING_JSON_PATCH_FIELD,
61+
"Missing 'op' in JSON patch",
62+
$context
63+
];
64+
65+
$context = [ 'operation' => [ 'op' => 'bad', 'path' => '/a/b/c', 'value' => 'test' ] ];
66+
yield 'from invalid patch operation' => [
67+
new PatchInvalidOpValidationError( 'bad', PatchItemStatementValidator::SOURCE_PATCH, $context ),
68+
ErrorResponse::INVALID_PATCH_OPERATION,
69+
"Incorrect JSON patch operation: 'bad'",
70+
$context
71+
];
72+
5373
yield 'from comment too long' => [
5474
new ValidationError( '500', PatchItemStatementValidator::SOURCE_COMMENT ),
5575
ErrorResponse::COMMENT_TOO_LONG,

0 commit comments

Comments
 (0)