Skip to content

Commit

Permalink
Use Valid HTTP status codes in error responses
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@G8XSU
G8XSU committed Dec 1, 2023
1 parent 0a185d8 commit 94f156a
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 22 deletions.
7 changes: 6 additions & 1 deletion app/src/main/java/org/vss/api/AbstractVssApi.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,23 +28,28 @@ 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()
.setErrorCode(errorCode)
.setMessage(e.getMessage())
.build();

return Response.status(errorCode.getNumber())
return Response.status(statusCode)
.entity(errorResponse.toByteArray())
.build();
}
Expand Down
10 changes: 5 additions & 5 deletions app/src/test/java/org/vss/api/DeleteObjectApiTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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<Arguments> 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)
);
}
}
12 changes: 6 additions & 6 deletions app/src/test/java/org/vss/api/GetObjectApiTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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<Arguments> 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)
);
}
}
10 changes: 5 additions & 5 deletions app/src/test/java/org/vss/api/ListKeyVersionsApiTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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<Arguments> 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)
);
}
}
10 changes: 5 additions & 5 deletions app/src/test/java/org/vss/api/PutObjectsApiTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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<Arguments> 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)
);
}
}

0 comments on commit 94f156a

Please sign in to comment.