From 94f156affffba259a61735eec0bb5324fa8e37fc Mon Sep 17 00:00:00 2001 From: Gursharan Singh <3442979+G8XSU@users.noreply.github.com> Date: Fri, 1 Dec 2023 12:11:57 -0800 Subject: [PATCH] Use Valid HTTP status codes in error responses VSS client-server protocol doesn't really need valid http status codes. Any non-200 status code is considered as error, but it is still good to response with valid status codes corresponding to error. Some http-clients might also have http-status code validation. No changes on client-side required, since any non-200 status code works. --- app/src/main/java/org/vss/api/AbstractVssApi.java | 7 ++++++- .../test/java/org/vss/api/DeleteObjectApiTest.java | 10 +++++----- app/src/test/java/org/vss/api/GetObjectApiTest.java | 12 ++++++------ .../java/org/vss/api/ListKeyVersionsApiTest.java | 10 +++++----- app/src/test/java/org/vss/api/PutObjectsApiTest.java | 10 +++++----- 5 files changed, 27 insertions(+), 22 deletions(-) diff --git a/app/src/main/java/org/vss/api/AbstractVssApi.java b/app/src/main/java/org/vss/api/AbstractVssApi.java index 53cb2e8..a3d583e 100644 --- a/app/src/main/java/org/vss/api/AbstractVssApi.java +++ b/app/src/main/java/org/vss/api/AbstractVssApi.java @@ -28,15 +28,20 @@ Response toResponse(GeneratedMessageV3 protoResponse) { Response toErrorResponse(Exception e) { ErrorCode errorCode; + int statusCode; if (e instanceof ConflictException) { errorCode = ErrorCode.CONFLICT_EXCEPTION; + statusCode = 409; } else if (e instanceof IllegalArgumentException || e instanceof InvalidProtocolBufferException) { errorCode = ErrorCode.INVALID_REQUEST_EXCEPTION; + statusCode = 400; } else if (e instanceof NoSuchKeyException) { errorCode = ErrorCode.NO_SUCH_KEY_EXCEPTION; + statusCode = 404; } else { errorCode = ErrorCode.INTERNAL_SERVER_EXCEPTION; + statusCode = 500; } ErrorResponse errorResponse = ErrorResponse.newBuilder() @@ -44,7 +49,7 @@ Response toErrorResponse(Exception e) { .setMessage(e.getMessage()) .build(); - return Response.status(errorCode.getNumber()) + return Response.status(statusCode) .entity(errorResponse.toByteArray()) .build(); } diff --git a/app/src/test/java/org/vss/api/DeleteObjectApiTest.java b/app/src/test/java/org/vss/api/DeleteObjectApiTest.java index ec3ba4e..9d85206 100644 --- a/app/src/test/java/org/vss/api/DeleteObjectApiTest.java +++ b/app/src/test/java/org/vss/api/DeleteObjectApiTest.java @@ -59,7 +59,7 @@ void execute_ValidPayload_ReturnsResponse() { @ParameterizedTest @MethodSource("provideErrorTestCases") void execute_InvalidPayload_ReturnsErrorResponse(Exception exception, - ErrorCode errorCode) { + ErrorCode errorCode, int statusCode) { DeleteObjectRequest expectedRequest = DeleteObjectRequest.newBuilder().setStoreId(TEST_STORE_ID).setKeyValue( KeyValue.newBuilder().setKey(TEST_KEY).setVersion(0) @@ -74,15 +74,15 @@ void execute_InvalidPayload_ReturnsErrorResponse(Exception exception, .setMessage("") .build(); assertThat(response.getEntity(), is(expectedErrorResponse.toByteArray())); - assertThat(response.getStatus(), is(expectedErrorResponse.getErrorCode().getNumber())); + assertThat(response.getStatus(), is(statusCode)); verify(mockKVStore).delete(expectedRequest); } private static Stream provideErrorTestCases() { return Stream.of( - Arguments.of(new ConflictException(""), ErrorCode.CONFLICT_EXCEPTION), - Arguments.of(new IllegalArgumentException(""), ErrorCode.INVALID_REQUEST_EXCEPTION), - Arguments.of(new RuntimeException(""), ErrorCode.INTERNAL_SERVER_EXCEPTION) + Arguments.of(new ConflictException(""), ErrorCode.CONFLICT_EXCEPTION, 409), + Arguments.of(new IllegalArgumentException(""), ErrorCode.INVALID_REQUEST_EXCEPTION, 400), + Arguments.of(new RuntimeException(""), ErrorCode.INTERNAL_SERVER_EXCEPTION, 500) ); } } diff --git a/app/src/test/java/org/vss/api/GetObjectApiTest.java b/app/src/test/java/org/vss/api/GetObjectApiTest.java index b617563..6324ddf 100644 --- a/app/src/test/java/org/vss/api/GetObjectApiTest.java +++ b/app/src/test/java/org/vss/api/GetObjectApiTest.java @@ -58,7 +58,7 @@ void execute_ValidPayload_ReturnsResponse() { @ParameterizedTest @MethodSource("provideErrorTestCases") void execute_InvalidPayload_ReturnsErrorResponse(Exception exception, - ErrorCode errorCode) { + ErrorCode errorCode, int statusCode) { GetObjectRequest expectedRequest = GetObjectRequest.newBuilder() .setStoreId(TEST_STORE_ID) .setKey(TEST_KEY) @@ -73,16 +73,16 @@ void execute_InvalidPayload_ReturnsErrorResponse(Exception exception, .setMessage("") .build(); assertThat(response.getEntity(), is(expectedErrorResponse.toByteArray())); - assertThat(response.getStatus(), is(expectedErrorResponse.getErrorCode().getNumber())); + assertThat(response.getStatus(), is(statusCode)); verify(mockKVStore).get(expectedRequest); } private static Stream provideErrorTestCases() { return Stream.of( - Arguments.of(new ConflictException(""), ErrorCode.CONFLICT_EXCEPTION), - Arguments.of(new IllegalArgumentException(""), ErrorCode.INVALID_REQUEST_EXCEPTION), - Arguments.of(new NoSuchKeyException(""), ErrorCode.NO_SUCH_KEY_EXCEPTION), - Arguments.of(new RuntimeException(""), ErrorCode.INTERNAL_SERVER_EXCEPTION) + Arguments.of(new ConflictException(""), ErrorCode.CONFLICT_EXCEPTION, 409), + Arguments.of(new IllegalArgumentException(""), ErrorCode.INVALID_REQUEST_EXCEPTION, 400), + Arguments.of(new NoSuchKeyException(""), ErrorCode.NO_SUCH_KEY_EXCEPTION, 404), + Arguments.of(new RuntimeException(""), ErrorCode.INTERNAL_SERVER_EXCEPTION, 500) ); } } diff --git a/app/src/test/java/org/vss/api/ListKeyVersionsApiTest.java b/app/src/test/java/org/vss/api/ListKeyVersionsApiTest.java index 6359ed0..2f27c6a 100644 --- a/app/src/test/java/org/vss/api/ListKeyVersionsApiTest.java +++ b/app/src/test/java/org/vss/api/ListKeyVersionsApiTest.java @@ -62,7 +62,7 @@ void execute_ValidPayload_ReturnsResponse() { @ParameterizedTest @MethodSource("provideErrorTestCases") void execute_InvalidPayload_ReturnsErrorResponse(Exception exception, - ErrorCode errorCode) { + ErrorCode errorCode, int statusCode) { ListKeyVersionsRequest expectedRequest = ListKeyVersionsRequest.newBuilder() .setStoreId(TEST_STORE_ID) @@ -78,15 +78,15 @@ void execute_InvalidPayload_ReturnsErrorResponse(Exception exception, .setMessage("") .build(); assertThat(response.getEntity(), is(expectedErrorResponse.toByteArray())); - assertThat(response.getStatus(), is(expectedErrorResponse.getErrorCode().getNumber())); + assertThat(response.getStatus(), is(statusCode)); verify(mockKVStore).listKeyVersions(expectedRequest); } private static Stream provideErrorTestCases() { return Stream.of( - Arguments.of(new ConflictException(""), ErrorCode.CONFLICT_EXCEPTION), - Arguments.of(new IllegalArgumentException(""), ErrorCode.INVALID_REQUEST_EXCEPTION), - Arguments.of(new RuntimeException(""), ErrorCode.INTERNAL_SERVER_EXCEPTION) + Arguments.of(new ConflictException(""), ErrorCode.CONFLICT_EXCEPTION, 409), + Arguments.of(new IllegalArgumentException(""), ErrorCode.INVALID_REQUEST_EXCEPTION, 400), + Arguments.of(new RuntimeException(""), ErrorCode.INTERNAL_SERVER_EXCEPTION, 500) ); } } diff --git a/app/src/test/java/org/vss/api/PutObjectsApiTest.java b/app/src/test/java/org/vss/api/PutObjectsApiTest.java index d816bb2..9a7316b 100644 --- a/app/src/test/java/org/vss/api/PutObjectsApiTest.java +++ b/app/src/test/java/org/vss/api/PutObjectsApiTest.java @@ -61,7 +61,7 @@ void execute_ValidPayload_ReturnsResponse() { @ParameterizedTest @MethodSource("provideErrorTestCases") void execute_InvalidPayload_ReturnsErrorResponse(Exception exception, - ErrorCode errorCode) { + ErrorCode errorCode, int statusCode) { PutObjectRequest expectedRequest = PutObjectRequest.newBuilder() .setStoreId(TEST_STORE_ID) @@ -77,15 +77,15 @@ void execute_InvalidPayload_ReturnsErrorResponse(Exception exception, .setMessage("") .build(); assertThat(response.getEntity(), is(expectedErrorResponse.toByteArray())); - assertThat(response.getStatus(), is(expectedErrorResponse.getErrorCode().getNumber())); + assertThat(response.getStatus(), is(statusCode)); verify(mockKVStore).put(expectedRequest); } private static Stream provideErrorTestCases() { return Stream.of( - Arguments.of(new ConflictException(""), ErrorCode.CONFLICT_EXCEPTION), - Arguments.of(new IllegalArgumentException(""), ErrorCode.INVALID_REQUEST_EXCEPTION), - Arguments.of(new RuntimeException(""), ErrorCode.INTERNAL_SERVER_EXCEPTION) + Arguments.of(new ConflictException(""), ErrorCode.CONFLICT_EXCEPTION, 409), + Arguments.of(new IllegalArgumentException(""), ErrorCode.INVALID_REQUEST_EXCEPTION, 400), + Arguments.of(new RuntimeException(""), ErrorCode.INTERNAL_SERVER_EXCEPTION, 500) ); } }