Skip to content

Commit

Permalink
Merge "REST: Fix "Patch Item/Property Aliases" errors"
Browse files Browse the repository at this point in the history
  • Loading branch information
jenkins-bot authored and Gerrit Code Review committed Oct 26, 2023
2 parents c6e15b4 + 793f449 commit 49e0061
Show file tree
Hide file tree
Showing 13 changed files with 191 additions and 73 deletions.
14 changes: 7 additions & 7 deletions repo/rest-api/specs/global/examples.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -382,13 +382,13 @@
}
}
},
"PatchedAliasInvalidExample": {
"PatchedAliasesInvalidFieldExample": {
"value": {
"code": "patched-alias-invalid",
"message": "Changed alias for '{language_code}' is invalid: '{alias}'",
"code": "patched-aliases-invalid-field",
"message": "Patched value for '{field}' is invalid",
"context": {
"language": "{language_code}",
"value": "{alias}"
"path": "{field}",
"value": "{value}"
}
}
},
Expand All @@ -397,8 +397,8 @@
"code": "patched-duplicate-alias",
"message": "Aliases in language '{language_code}' contain duplicate alias: '{alias}'",
"context": {
"language": "{language_code}",
"value": "{alias}"
"alias": "{alias}",
"language": "{language_code}"
}
}
},
Expand Down
2 changes: 1 addition & 1 deletion repo/rest-api/specs/global/responses.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -982,7 +982,7 @@
},
"patched-alias-empty": { "$ref": "./examples.json#/PatchedAliasEmptyExample" },
"patched-alias-too-long": { "$ref": "./examples.json#/PatchedAliasTooLongExample" },
"patched-alias-invalid": { "$ref": "./examples.json#/PatchedAliasInvalidExample" },
"patched-aliases-invalid-field": { "$ref": "./examples.json#/PatchedAliasesInvalidFieldExample" },
"patched-duplicate-alias": { "$ref": "./examples.json#/PatchedDuplicateAliasExample" }
}
}
Expand Down
2 changes: 1 addition & 1 deletion repo/rest-api/src/Application/Serialization/AliasesDeserializer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public function deserialize( array $serialization ): AliasGroupList {
$aliasGroups = [];

foreach ( $serialization as $language => $aliasesInLanguage ) {
if ( !is_array( $aliasesInLanguage ) ) {
if ( !is_array( $aliasesInLanguage ) || !array_is_list( $aliasesInLanguage ) ) {
throw new InvalidFieldException( $language, $aliasesInLanguage );
}

Expand Down
34 changes: 26 additions & 8 deletions repo/rest-api/src/Application/UseCases/PatchItemAliases/PatchedAliasesValidator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use Wikibase\Repo\RestApi\Application\Serialization\AliasesDeserializer;
use Wikibase\Repo\RestApi\Application\Serialization\DuplicateAliasException;
use Wikibase\Repo\RestApi\Application\Serialization\EmptyAliasException;
use Wikibase\Repo\RestApi\Application\Serialization\InvalidFieldException;
use Wikibase\Repo\RestApi\Application\UseCases\UseCaseError;
use Wikibase\Repo\RestApi\Application\Validation\AliasesInLanguageValidator;
use Wikibase\Repo\RestApi\Application\Validation\LanguageCodeValidator;
Expand All @@ -30,9 +31,19 @@ public function __construct(
}

/**
* @param mixed $serialization
*
* @throws UseCaseError
*/
public function validateAndDeserialize( array $serialization ): AliasGroupList {
public function validateAndDeserialize( $serialization ): AliasGroupList {
if ( !is_array( $serialization ) || ( array_is_list( $serialization ) && count( $serialization ) > 0 ) ) {
throw new UseCaseError(
UseCaseError::PATCHED_ALIASES_INVALID_FIELD,
"Patched value for '' is invalid",
[ UseCaseError::CONTEXT_PATH => '', UseCaseError::CONTEXT_VALUE => $serialization ]
);
}

$this->validateLanguageCodes( array_keys( $serialization ) );

$aliases = $this->deserialize( $serialization );
Expand All @@ -57,6 +68,12 @@ private function deserialize( array $serialization ): AliasGroupList {
"Aliases in language '{$e->getField()}' contain duplicate alias: '{$e->getValue()}'",
[ UseCaseError::CONTEXT_LANGUAGE => $e->getField(), UseCaseError::CONTEXT_VALUE => $e->getValue() ]
);
} catch ( InvalidFieldException $e ) {
throw new UseCaseError(
UseCaseError::PATCHED_ALIASES_INVALID_FIELD,
"Patched value for '{$e->getField()}' is invalid",
[ UseCaseError::CONTEXT_PATH => $e->getField(), UseCaseError::CONTEXT_VALUE => $e->getValue() ]
);
}
}

Expand All @@ -70,21 +87,21 @@ private function validateAliases( AliasGroupList $aliases ): void {
$limit = $context[AliasesInLanguageValidator::CONTEXT_LIMIT];
throw new UseCaseError(
UseCaseError::PATCHED_ALIAS_TOO_LONG,
"Changed alias for '{$aliasGroup->getLanguageCode()}' must not be more than '$limit'",
"Changed alias for '{$aliasGroup->getLanguageCode()}' must not be more than $limit characters long",
[
UseCaseError::CONTEXT_LANGUAGE => $aliasGroup->getLanguageCode(),
UseCaseError::CONTEXT_VALUE => $context[AliasesInLanguageValidator::CONTEXT_VALUE],
UseCaseError::CONTEXT_CHARACTER_LIMIT => $limit,
]
);
default:
$alias = $context[AliasesInLanguageValidator::CONTEXT_VALUE];
$value = $context[AliasesInLanguageValidator::CONTEXT_VALUE];
throw new UseCaseError(
UseCaseError::PATCHED_ALIAS_INVALID,
"Changed alias for '{$aliasGroup->getLanguageCode()}' is invalid: '$alias'",
UseCaseError::PATCHED_ALIASES_INVALID_FIELD,
"Patched value for '{$aliasGroup->getLanguageCode()}' is invalid",
[
UseCaseError::CONTEXT_LANGUAGE => $aliasGroup->getLanguageCode(),
UseCaseError::CONTEXT_VALUE => $alias,
UseCaseError::CONTEXT_PATH => $aliasGroup->getLanguageCode(),
UseCaseError::CONTEXT_VALUE => $value,
]
);
}
Expand All @@ -96,11 +113,12 @@ private function validateLanguageCodes( array $languageCodes ): void {
foreach ( $languageCodes as $languageCode ) {
if ( $this->languageCodeValidator->validate( $languageCode ) ) {
throw new UseCaseError(
UseCaseError::PATCHED_ALIAS_INVALID_LANGUAGE_CODE,
UseCaseError::PATCHED_ALIASES_INVALID_LANGUAGE_CODE,
"Not a valid language code '$languageCode' in changed aliases",
[ UseCaseError::CONTEXT_LANGUAGE => $languageCode ]
);
}
}
}

}
5 changes: 4 additions & 1 deletion repo/rest-api/src/Application/UseCases/PatchJson.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ public function __construct( JsonPatcher $patcher ) {
$this->patcher = $patcher;
}

public function execute( array $serialization, array $patch ): array {
/**
* @return mixed
*/
public function execute( array $serialization, array $patch ) {
try {
return $this->patcher->patch( $serialization, $patch );
} catch ( PatchPathException $e ) {
Expand Down
36 changes: 27 additions & 9 deletions repo/rest-api/src/Application/UseCases/PatchPropertyAliases/PatchedAliasesValidator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use Wikibase\Repo\RestApi\Application\Serialization\AliasesDeserializer;
use Wikibase\Repo\RestApi\Application\Serialization\DuplicateAliasException;
use Wikibase\Repo\RestApi\Application\Serialization\EmptyAliasException;
use Wikibase\Repo\RestApi\Application\Serialization\InvalidFieldException;
use Wikibase\Repo\RestApi\Application\UseCases\UseCaseError;
use Wikibase\Repo\RestApi\Application\Validation\AliasesInLanguageValidator;
use Wikibase\Repo\RestApi\Application\Validation\LanguageCodeValidator;
Expand All @@ -30,9 +31,19 @@ public function __construct(
}

/**
* @param mixed $serialization
*
* @throws UseCaseError
*/
public function validateAndDeserialize( array $serialization ): AliasGroupList {
public function validateAndDeserialize( $serialization ): AliasGroupList {
if ( !is_array( $serialization ) ) {
throw new UseCaseError(
UseCaseError::PATCHED_ALIASES_INVALID_FIELD,
"Patched value for '' is invalid",
[ UseCaseError::CONTEXT_PATH => '', UseCaseError::CONTEXT_VALUE => $serialization ]
);
}

$this->validateLanguageCodes( array_keys( $serialization ) );

$aliases = $this->deserialize( $serialization );
Expand All @@ -57,6 +68,12 @@ private function deserialize( array $serialization ): AliasGroupList {
"Aliases in language '{$e->getField()}' contain duplicate alias: '{$e->getValue()}'",
[ UseCaseError::CONTEXT_LANGUAGE => $e->getField(), UseCaseError::CONTEXT_VALUE => $e->getValue() ]
);
} catch ( InvalidFieldException $e ) {
throw new UseCaseError(
UseCaseError::PATCHED_ALIASES_INVALID_FIELD,
"Patched value for '{$e->getField()}' is invalid",
[ UseCaseError::CONTEXT_PATH => $e->getField(), UseCaseError::CONTEXT_VALUE => $e->getValue() ]
);
}
}

Expand All @@ -70,21 +87,21 @@ private function validateAliases( AliasGroupList $aliases ): void {
$limit = $context[AliasesInLanguageValidator::CONTEXT_LIMIT];
throw new UseCaseError(
UseCaseError::PATCHED_ALIAS_TOO_LONG,
"Changed alias for '{$aliasGroup->getLanguageCode()}' must not be more than '$limit'",
"Changed alias for '{$aliasGroup->getLanguageCode()}' must not be more than $limit characters long",
[
UseCaseError::CONTEXT_LANGUAGE => $aliasGroup->getLanguageCode(),
UseCaseError::CONTEXT_VALUE => $context[AliasesInLanguageValidator::CONTEXT_VALUE],
UseCaseError::CONTEXT_CHARACTER_LIMIT => $limit,
]
);
default:
$alias = $context[AliasesInLanguageValidator::CONTEXT_VALUE];
$value = $context[AliasesInLanguageValidator::CONTEXT_VALUE];
throw new UseCaseError(
UseCaseError::PATCHED_ALIAS_INVALID,
"Changed alias for '{$aliasGroup->getLanguageCode()}' is invalid: '$alias'",
UseCaseError::PATCHED_ALIASES_INVALID_FIELD,
"Patched value for '{$aliasGroup->getLanguageCode()}' is invalid",
[
UseCaseError::CONTEXT_LANGUAGE => $aliasGroup->getLanguageCode(),
UseCaseError::CONTEXT_VALUE => $alias,
UseCaseError::CONTEXT_PATH => $aliasGroup->getLanguageCode(),
UseCaseError::CONTEXT_VALUE => $value,
]
);
}
Expand All @@ -96,11 +113,12 @@ private function validateLanguageCodes( array $languageCodes ): void {
foreach ( $languageCodes as $languageCode ) {
if ( $this->languageCodeValidator->validate( $languageCode ) ) {
throw new UseCaseError(
UseCaseError::PATCHED_ALIAS_INVALID_LANGUAGE_CODE,
"Not a valid language code '{$languageCode}' in changed aliases",
UseCaseError::PATCHED_ALIASES_INVALID_LANGUAGE_CODE,
"Not a valid language code '$languageCode' in changed aliases",
[ UseCaseError::CONTEXT_LANGUAGE => $languageCode ]
);
}
}
}

}
8 changes: 4 additions & 4 deletions repo/rest-api/src/Application/UseCases/UseCaseError.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ class UseCaseError extends UseCaseException {
public const PATCHED_DESCRIPTION_INVALID_LANGUAGE_CODE = 'patched-descriptions-invalid-language-code';
public const PATCHED_DESCRIPTION_TOO_LONG = 'patched-description-too-long';
public const PATCHED_ALIAS_EMPTY = 'patched-alias-empty';
public const PATCHED_ALIAS_INVALID = 'patched-alias-invalid';
public const PATCHED_ALIAS_INVALID_LANGUAGE_CODE = 'patched-alias-invalid-language-code';
public const PATCHED_ALIASES_INVALID_FIELD = 'patched-aliases-invalid-field';
public const PATCHED_ALIASES_INVALID_LANGUAGE_CODE = 'patched-aliases-invalid-language-code';
public const PATCHED_ALIAS_TOO_LONG = 'patched-alias-too-long';
public const PATCHED_ALIAS_DUPLICATE = 'patched-duplicate-alias';
public const PATCHED_STATEMENT_INVALID_FIELD = 'patched-statement-invalid-field';
Expand Down Expand Up @@ -126,8 +126,8 @@ class UseCaseError extends UseCaseException {
self::PATCHED_DESCRIPTION_INVALID_LANGUAGE_CODE => [ self::CONTEXT_LANGUAGE ],
self::PATCHED_DESCRIPTION_TOO_LONG => [ self::CONTEXT_LANGUAGE, self::CONTEXT_VALUE, self::CONTEXT_CHARACTER_LIMIT ],
self::PATCHED_ALIAS_EMPTY => [ self::CONTEXT_LANGUAGE ],
self::PATCHED_ALIAS_INVALID => [ self::CONTEXT_LANGUAGE, self::CONTEXT_VALUE ],
self::PATCHED_ALIAS_INVALID_LANGUAGE_CODE => [ self::CONTEXT_LANGUAGE ],
self::PATCHED_ALIASES_INVALID_FIELD => [ self::CONTEXT_PATH, self::CONTEXT_VALUE ],
self::PATCHED_ALIASES_INVALID_LANGUAGE_CODE => [ self::CONTEXT_LANGUAGE ],
self::PATCHED_ALIAS_TOO_LONG => [ self::CONTEXT_LANGUAGE, self::CONTEXT_VALUE, self::CONTEXT_CHARACTER_LIMIT ],
self::PATCHED_ALIAS_DUPLICATE => [ self::CONTEXT_LANGUAGE, self::CONTEXT_VALUE ],
self::PATCHED_STATEMENT_INVALID_FIELD => [ self::CONTEXT_PATH, self::CONTEXT_VALUE ],
Expand Down
4 changes: 2 additions & 2 deletions repo/rest-api/src/RouteHandlers/ErrorResponseToHttpStatus.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ class ErrorResponseToHttpStatus {
UseCaseError::PATCHED_LABEL_EMPTY => 422,
UseCaseError::PATCHED_LABEL_TOO_LONG => 422,
UseCaseError::PATCHED_ALIAS_EMPTY => 422,
UseCaseError::PATCHED_ALIAS_INVALID => 422,
UseCaseError::PATCHED_ALIAS_INVALID_LANGUAGE_CODE => 422,
UseCaseError::PATCHED_ALIASES_INVALID_FIELD => 422,
UseCaseError::PATCHED_ALIASES_INVALID_LANGUAGE_CODE => 422,
UseCaseError::PATCHED_ALIAS_TOO_LONG => 422,
UseCaseError::PATCHED_ALIAS_DUPLICATE => 422,
UseCaseError::PERMISSION_DENIED => 403,
Expand Down
34 changes: 27 additions & 7 deletions repo/rest-api/tests/mocha/api-testing/PatchItemAliasesTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,17 +263,37 @@ describe( newPatchItemAliasesRequestBuilder().getRouteDescription(), () => {
} );

it( 'alias contains invalid characters', async () => {
const language = 'en';
const invalidAlias = 'tab\t tab\t tab';
const response = await newPatchItemAliasesRequestBuilder( testItemId, [
{ op: 'add', path: `/${language}`, value: [ invalidAlias ] }
{ op: 'add', path: `/${testLanguage}`, value: [ invalidAlias ] }
] ).assertValidRequest().makeRequest();

expect( response ).to.have.status( 422 );
assert.strictEqual( response.body.code, 'patched-alias-invalid' );
assert.include( response.body.message, language );
assert.include( response.body.message, invalidAlias );
assert.deepEqual( response.body.context, { language, value: invalidAlias } );
assert.strictEqual( response.body.code, 'patched-aliases-invalid-field' );
assert.include( response.body.message, testLanguage );
assert.deepEqual( response.body.context, { path: testLanguage, value: invalidAlias } );
} );

it( 'aliases in language is not a list', async () => {
const invalidAliasesInLanguage = { object: 'not a list' };
const response = await newPatchItemAliasesRequestBuilder( testItemId, [
{ op: 'add', path: `/${testLanguage}`, value: invalidAliasesInLanguage }
] ).assertValidRequest().makeRequest();

const context = { path: testLanguage, value: invalidAliasesInLanguage };
assertValidErrorResponse( response, 422, 'patched-aliases-invalid-field', context );
assert.include( response.body.message, testLanguage );
} );

it( 'aliases is not an object', async () => {
const invalidAliases = [ 'list, not an object' ];
const response = await newPatchItemAliasesRequestBuilder( testItemId, [
{ op: 'add', path: '', value: invalidAliases }
] ).assertValidRequest().makeRequest();

const context = { path: '', value: invalidAliases };
assertValidErrorResponse( response, 422, 'patched-aliases-invalid-field', context );
assert.strictEqual( response.body.message, "Patched value for '' is invalid" );
} );

it( 'invalid language code', async () => {
Expand All @@ -283,7 +303,7 @@ describe( newPatchItemAliasesRequestBuilder().getRouteDescription(), () => {
] ).assertValidRequest().makeRequest();

expect( response ).to.have.status( 422 );
assert.strictEqual( response.body.code, 'patched-alias-invalid-language-code' );
assert.strictEqual( response.body.code, 'patched-aliases-invalid-language-code' );
assert.include( response.body.message, language );
assert.deepEqual( response.body.context, { language } );
} );
Expand Down
7 changes: 3 additions & 4 deletions repo/rest-api/tests/mocha/api-testing/PatchPropertyAliasesTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,9 @@ describe( newPatchPropertyAliasesRequestBuilder().getRouteDescription(), () => {
] ).assertValidRequest().makeRequest();

expect( response ).to.have.status( 422 );
assert.strictEqual( response.body.code, 'patched-alias-invalid' );
assert.strictEqual( response.body.code, 'patched-aliases-invalid-field' );
assert.include( response.body.message, language );
assert.include( response.body.message, invalidAlias );
assert.deepEqual( response.body.context, { language, value: invalidAlias } );
assert.deepEqual( response.body.context, { path: language, value: invalidAlias } );
} );

it( 'invalid language code', async () => {
Expand All @@ -220,7 +219,7 @@ describe( newPatchPropertyAliasesRequestBuilder().getRouteDescription(), () => {
] ).assertValidRequest().makeRequest();

expect( response ).to.have.status( 422 );
assert.strictEqual( response.body.code, 'patched-alias-invalid-language-code' );
assert.strictEqual( response.body.code, 'patched-aliases-invalid-language-code' );
assert.include( response.body.message, language );
assert.deepEqual( response.body.context, { language } );
} );
Expand Down
11 changes: 11 additions & 0 deletions repo/rest-api/tests/phpunit/Application/Serialization/AliasesDeserializerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,15 @@ public function testGivenInvalidAliasType_throwsException(): void {
}
}

public function testGivenInvalidAliasesType_throwsException(): void {
$invalidAliasesInLanguage = [ 'associative' => 'not a list' ];
try {
( new AliasesDeserializer() )->deserialize( [ 'en' => $invalidAliasesInLanguage ] );
$this->fail( 'this should not be reached' );
} catch ( InvalidFieldException $e ) {
$this->assertSame( 'en', $e->getField() );
$this->assertSame( $invalidAliasesInLanguage, $e->getValue() );
}
}

}
Loading

0 comments on commit 49e0061

Please sign in to comment.