Merge "REST: Create resource-not-found error"

wikimedia · Sep 5, 2024 · 70288f7 · 70288f7
2 parents 1240b45 + b2ebede
commit 70288f7
Show file tree

Hide file tree

Showing 11 changed files with 41 additions and 34 deletions.
diff --git a/repo/rest-api/specs/global/examples.json b/repo/rest-api/specs/global/examples.json
@@ -146,10 +146,13 @@
 			"message": "Could not find a statement with the ID: {statement_id}"
 		}
 	},
-	"SitelinkNotDefinedExample": {
+	"ResourceNotFoundExample": {
 		"value": {
-			"code": "sitelink-not-defined",
-			"message": "No sitelink found for the ID: {item_id} for the site {site_id}"
+			"code": "resource-not-found",
+			"message": "The requested resource does not exist",
+			"context": {
+				"resource_type": "{resource_type}"
+			}
 		}
 	},
 	"ItemLabelNotDefinedExample": {

diff --git a/repo/rest-api/specs/global/responses.json b/repo/rest-api/specs/global/responses.json
@@ -229,8 +229,8 @@
 					"item-not-found": {
 						"$ref": "./examples.json#/ItemNotFoundExample"
 					},
-					"sitelink-not-defined": {
-						"$ref": "./examples.json#/SitelinkNotDefinedExample"
+					"resource-not-found": {
+						"$ref": "./examples.json#/ResourceNotFoundExample"
 					}
 				}
 			}

diff --git a/repo/rest-api/src/Application/UseCases/GetSitelink/GetSitelink.php b/repo/rest-api/src/Application/UseCases/GetSitelink/GetSitelink.php
@@ -39,10 +39,7 @@ public function execute( GetSitelinkRequest $request ): GetSitelinkResponse {
 
 		$sitelink = $this->sitelinkRetriever->getSitelink( $itemId, $siteId );
 		if ( !$sitelink ) {
-			throw new UseCaseError(
-				UseCaseError::SITELINK_NOT_DEFINED,
-				"No sitelink found for the ID: {$itemId->getSerialization()} for the site $siteId"
-			);
+			throw UseCaseError::newResourceNotFound( 'sitelink' );
 		}
 
 		return new GetSitelinkResponse( $sitelink, $lastModified, $revisionId );

diff --git a/repo/rest-api/src/Application/UseCases/RemoveSitelink/RemoveSitelink.php b/repo/rest-api/src/Application/UseCases/RemoveSitelink/RemoveSitelink.php
@@ -52,10 +52,7 @@ public function execute( RemoveSitelinkRequest $request ): void {
 		$item = $this->itemRetriever->getItemWriteModel( $itemId );
 
 		if ( !$item->hasLinkToSite( $siteId ) ) {
-			throw new UseCaseError(
-				UseCaseError::SITELINK_NOT_DEFINED,
-				"No sitelink found for the ID: $itemId for the site $siteId"
-			);
+			throw UseCaseError::newResourceNotFound( 'sitelink' );
 		}
 
 		$removedSitelink = $item->getSiteLink( $siteId );

diff --git a/repo/rest-api/src/Application/UseCases/UseCaseError.php b/repo/rest-api/src/Application/UseCases/UseCaseError.php
@@ -44,7 +44,7 @@ class UseCaseError extends UseCaseException {
 	public const POLICY_VIOLATION_SITELINK_CONFLICT = 'sitelink-conflict';
 	public const PROPERTY_NOT_FOUND = 'property-not-found';
 	public const PROPERTY_STATEMENT_ID_MISMATCH = 'property-statement-id-mismatch';
-	public const SITELINK_NOT_DEFINED = 'sitelink-not-defined';
+	public const RESOURCE_NOT_FOUND = 'resource-not-found';
 	public const SITELINK_TITLE_NOT_FOUND = 'title-does-not-exist';
 	public const STATEMENT_GROUP_PROPERTY_ID_MISMATCH = 'statement-group-property-id-mismatch';
 	public const STATEMENT_NOT_FOUND = 'statement-not-found';
@@ -67,6 +67,7 @@ class UseCaseError extends UseCaseException {
 	public const CONTEXT_PROPERTY_ID = 'property_id';
 	public const CONTEXT_REASON = 'reason';
 	public const CONTEXT_REDIRECT_TARGET = 'redirect_target';
+	public const CONTEXT_RESOURCE_TYPE = 'resource_type';
 	public const CONTEXT_SITE_ID = 'site_id';
 	public const CONTEXT_STATEMENT_GROUP_PROPERTY_ID = 'statement_group_property_id';
 	public const CONTEXT_STATEMENT_ID = 'statement_id';
@@ -109,7 +110,7 @@ class UseCaseError extends UseCaseException {
 		self::PERMISSION_DENIED_UNKNOWN_REASON => [],
 		self::PROPERTY_NOT_FOUND => [],
 		self::PROPERTY_STATEMENT_ID_MISMATCH => [ self::CONTEXT_PROPERTY_ID, self::CONTEXT_STATEMENT_ID ],
-		self::SITELINK_NOT_DEFINED => [],
+		self::RESOURCE_NOT_FOUND => [ self::CONTEXT_RESOURCE_TYPE ],
 		self::SITELINK_TITLE_NOT_FOUND => [],
 		self::STATEMENT_GROUP_PROPERTY_ID_MISMATCH => [
 			self::CONTEXT_PATH,
@@ -234,6 +235,14 @@ public static function newPermissionDenied( string $reason ): self {
 		return new self( self::PERMISSION_DENIED, 'Access to resource is denied', [ self::CONTEXT_REASON => $reason ] );
 	}
 
+	public static function newResourceNotFound( string $resourceType ): self {
+		return new self(
+			self::RESOURCE_NOT_FOUND,
+			'The requested resource does not exist',
+			[ self::CONTEXT_RESOURCE_TYPE => $resourceType ]
+		);
+	}
+
 	public function getErrorCode(): string {
 		return $this->errorCode;
 	}

diff --git a/repo/rest-api/src/RouteHandlers/ErrorResponseToHttpStatus.php b/repo/rest-api/src/RouteHandlers/ErrorResponseToHttpStatus.php
@@ -33,7 +33,7 @@ class ErrorResponseToHttpStatus {
 		UseCaseError::ITEM_NOT_FOUND => 404,
 		UseCaseError::LABEL_NOT_DEFINED => 404,
 		UseCaseError::PROPERTY_NOT_FOUND => 404,
-		UseCaseError::SITELINK_NOT_DEFINED => 404,
+		UseCaseError::RESOURCE_NOT_FOUND => 404,
 		UseCaseError::STATEMENT_NOT_FOUND => 404,
 
 		// 409 errors:

diff --git a/repo/rest-api/src/RouteHandlers/openapi.json b/repo/rest-api/src/RouteHandlers/openapi.json
@@ -3950,8 +3950,8 @@
               "item-not-found": {
                 "$ref": "#/components/examples/ItemNotFoundExample"
               },
-              "sitelink-not-defined": {
-                "$ref": "#/components/examples/SitelinkNotDefinedExample"
+              "resource-not-found": {
+                "$ref": "#/components/examples/ResourceNotFoundExample"
               }
             }
           }
@@ -7122,10 +7122,13 @@
           "message": "Could not find a statement with the ID: {statement_id}"
         }
       },
-      "SitelinkNotDefinedExample": {
+      "ResourceNotFoundExample": {
         "value": {
-          "code": "sitelink-not-defined",
-          "message": "No sitelink found for the ID: {item_id} for the site {site_id}"
+          "code": "resource-not-found",
+          "message": "The requested resource does not exist",
+          "context": {
+            "resource_type": "{resource_type}"
+          }
         }
       },
       "ItemLabelNotDefinedExample": {

diff --git a/repo/rest-api/tests/mocha/api-testing/GetSitelinkTest.js b/repo/rest-api/tests/mocha/api-testing/GetSitelinkTest.js
@@ -85,8 +85,8 @@ describe( newGetSitelinkRequestBuilder().getRouteDescription(), () => {
 				.assertValidRequest()
 				.makeRequest();
 
-			assertValidError( response, 404, 'sitelink-not-defined' );
-			assert.include( response.body.message, siteId );
+			assertValidError( response, 404, 'resource-not-found', { resource_type: 'sitelink' } );
+			assert.strictEqual( response.body.message, 'The requested resource does not exist' );
 		} );
 	} );
 

diff --git a/repo/rest-api/tests/mocha/api-testing/RemoveSitelinkTest.js b/repo/rest-api/tests/mocha/api-testing/RemoveSitelinkTest.js
@@ -75,10 +75,10 @@ describe( newRemoveSitelinkRequestBuilder().getRouteDescription(), () => {
 				.assertValidRequest()
 				.makeRequest();
 
-			assertValidError( response, 404, 'sitelink-not-defined' );
-			assert.include( response.body.message, itemWithNoSitelink );
-			assert.include( response.body.message, siteId );
+			assertValidError( response, 404, 'resource-not-found', { resource_type: 'sitelink' } );
+			assert.strictEqual( response.body.message, 'The requested resource does not exist' );
 		} );
+
 		it( 'responds with 404 if the item does not exist', async () => {
 			const itemDoesNotExist = 'Q999999';
 			const response = await newRemoveSitelinkRequestBuilder( itemDoesNotExist, siteId )

diff --git a/repo/rest-api/tests/phpunit/Application/UseCases/GetSitelink/GetSitelinkTest.php b/repo/rest-api/tests/phpunit/Application/UseCases/GetSitelink/GetSitelinkTest.php
@@ -109,12 +109,9 @@ public function testGivenRequestedSitelinkDoesNotExist_throwsUseCaseError(): voi
 
 			$this->fail( 'this should not be reached' );
 		} catch ( UseCaseError $e ) {
-			$this->assertSame( UseCaseError::SITELINK_NOT_DEFINED, $e->getErrorCode() );
-			$this->assertSame(
-				'No sitelink found for the ID: Q11 for the site ' . TestValidatingRequestDeserializer::ALLOWED_SITE_IDS[0],
-				$e->getErrorMessage()
-			);
-			$this->assertSame( [], $e->getErrorContext() );
+			$this->assertSame( UseCaseError::RESOURCE_NOT_FOUND, $e->getErrorCode() );
+			$this->assertSame( 'The requested resource does not exist', $e->getErrorMessage() );
+			$this->assertSame( [ 'resource_type' => 'sitelink' ], $e->getErrorContext() );
 		}
 	}
 

diff --git a/repo/rest-api/tests/phpunit/Application/UseCases/RemoveSitelink/RemoveSitelinkTest.php b/repo/rest-api/tests/phpunit/Application/UseCases/RemoveSitelink/RemoveSitelinkTest.php
@@ -87,8 +87,9 @@ public function testGivenSitelinkNotFound_throws(): void {
 				->execute( new RemoveSitelinkRequest( "$itemId", $siteId, [], false, null, null ) );
 			$this->fail( 'expected exception was not thrown' );
 		} catch ( UseCaseError $e ) {
-			$this->assertSame( UseCaseError::SITELINK_NOT_DEFINED, $e->getErrorCode() );
-			$this->assertSame( "No sitelink found for the ID: $itemId for the site $siteId", $e->getErrorMessage() );
+			$this->assertSame( UseCaseError::RESOURCE_NOT_FOUND, $e->getErrorCode() );
+			$this->assertSame( 'The requested resource does not exist', $e->getErrorMessage() );
+			$this->assertSame( [ 'resource_type' => 'sitelink' ], $e->getErrorContext() );
 		}
 	}