Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #1383: map most recognized SERVER_ errors to 504 or 502, leave 500 for unrecognized #1384

Merged
merged 4 commits into from
Sep 3, 2024
Merged

Fix #1383: map most recognized SERVER_ errors to 504 or 502, leave 500 for unrecognized #1384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ca6f9a
Fix #1383: map most recognized SERVER_ errors to 504 or 502, leave 50…
tatu-at-datastax Aug 30, 2024
b4d5f07
Fix unit test
tatu-at-datastax Aug 30, 2024
ce6a878
Merge branch 'main' into tatu/1383-http-500-to-504
tatu-at-datastax Sep 3, 2024
8271220
Fix one http response code based on code review
tatu-at-datastax Sep 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 36 additions & 15 deletions src/main/java/io/stargate/sgv2/jsonapi/exception/mappers/ThrowableToErrorMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public final class ThrowableToErrorMapper {
// if our own exception, shortcut
if (throwable instanceof JsonApiException jae) {
if (jae.getErrorCode().equals(ErrorCode.SERVER_EMBEDDING_GATEWAY_NOT_AVAILABLE)) {
// 30-Aug-2021: [data-api#1383] Why is this special case here?
return jae.getCommandResultError(message, Response.Status.INTERNAL_SERVER_ERROR);
}
return jae.getCommandResultError(message, jae.getHttpStatus());
Expand Down Expand Up @@ -88,17 +89,21 @@ private static CommandResult.Error handleDriverException(
return handleAllNodesFailedException((AllNodesFailedException) throwable, message);
} else if (throwable instanceof ClosedConnectionException) {
return ErrorCode.SERVER_CLOSED_CONNECTION
.toApiException(message)
.getCommandResultError(Response.Status.INTERNAL_SERVER_ERROR);
.toApiException("(DriverException/ClosedConnectionException) %s", message)
.getCommandResultError(Response.Status.BAD_GATEWAY);
} else if (throwable instanceof CoordinatorException) {
return handleCoordinatorException((CoordinatorException) throwable, message);
} else if (throwable instanceof DriverTimeoutException) {
return ErrorCode.SERVER_DRIVER_TIMEOUT
.toApiException(message)
.getCommandResultError(Response.Status.INTERNAL_SERVER_ERROR);
.toApiException("(DriverException/DriverTimeoutException) %s", message)
.getCommandResultError(Response.Status.GATEWAY_TIMEOUT);
} else {
// Leave this as 500 since we do not recognize the exception: should add new cases
// when we encounter new exceptions
return ErrorCode.SERVER_DRIVER_FAILURE
.toApiException("root cause: (%s) %s", throwable.getClass().getName(), message)
.toApiException(
"(DriverException/unrecognized) root cause: (%s) %s",
throwable.getClass().getName(), message)
.getCommandResultError(Response.Status.INTERNAL_SERVER_ERROR);
}
}
Expand All @@ -110,8 +115,12 @@ private static CommandResult.Error handleCoordinatorException(
} else if (throwable instanceof QueryExecutionException) {
return handleQueryExecutionException((QueryExecutionException) throwable, message);
} else {
// Leave this as 500 since we do not recognize the exception: should add new cases
// when we encounter new exceptions
return ErrorCode.SERVER_COORDINATOR_FAILURE
.toApiException("root cause: (%s) %s", throwable.getClass().getName(), message)
.toApiException(
"(CoordinatorException/unrecognized) root cause: (%s) %s",
throwable.getClass().getName(), message)
.getCommandResultError(Response.Status.INTERNAL_SERVER_ERROR);
}
}
Expand All @@ -121,20 +130,29 @@ private static CommandResult.Error handleQueryExecutionException(
if (throwable instanceof QueryConsistencyException e) {
if (e instanceof WriteTimeoutException || e instanceof ReadTimeoutException) {
return ErrorCode.SERVER_DRIVER_TIMEOUT
.toApiException(message)
.getCommandResultError(Response.Status.INTERNAL_SERVER_ERROR);
.toApiException(
"(QueryConsistencyException/%s) %s", e.getClass().getSimpleName(), message)
.getCommandResultError(Response.Status.GATEWAY_TIMEOUT);
} else if (e instanceof ReadFailureException) {
return ErrorCode.SERVER_READ_FAILED
.toApiException("root cause: (%s) %s", e.getClass().getName(), message)
.getCommandResultError(Response.Status.INTERNAL_SERVER_ERROR);
.toApiException("(QueryConsistencyException/ReadFailureException) %s", message)
.getCommandResultError(Response.Status.BAD_GATEWAY);
} else {
// Leave this as 500 since we do not recognize the exception: should add new cases
// when we encounter new exceptions
return ErrorCode.SERVER_QUERY_CONSISTENCY_FAILURE
.toApiException("root cause: (%s) %s", e.getClass().getName(), message)
.toApiException(
"(QueryConsistencyException/unrecognized) root cause: (%s) %s",
throwable.getClass().getName(), message)
.getCommandResultError(Response.Status.INTERNAL_SERVER_ERROR);
}
} else {
// Leave this as 500 since we do not recognize the exception: should add new cases
// when we encounter new exceptions
return ErrorCode.SERVER_QUERY_EXECUTION_FAILURE
.toApiException("root cause: (%s) %s", throwable.getClass().getName(), message)
.toApiException(
"(QueryExecutionException/unrecognized) root cause: (%s) %s",
throwable.getClass().getName(), message)
.getCommandResultError(Response.Status.INTERNAL_SERVER_ERROR);
}
}
Expand Down Expand Up @@ -196,12 +214,15 @@ private static CommandResult.Error handleAllNodesFailedException(
// Driver NoNodeAvailableException -> ErrorCode.NO_NODE_AVAILABLE
} else if (error instanceof NoNodeAvailableException) {
return ErrorCode.SERVER_NO_NODE_AVAILABLE
.toApiException()
.getCommandResultError(message, Response.Status.INTERNAL_SERVER_ERROR);
.toApiException("(AllNodesFailedException/NoNodeAvailableException) %s", message)
.getCommandResultError(message, Response.Status.BAD_GATEWAY);
} else if (error instanceof DriverTimeoutException) {
// [data-api#1205] Need to map DriverTimeoutException as well
return ErrorCode.SERVER_DRIVER_TIMEOUT
.toApiException(Response.Status.INTERNAL_SERVER_ERROR, message)
.toApiException(
Response.Status.GATEWAY_TIMEOUT,
"(AllNodesFailedException/DriverTimeoutException) %s",
message)
.getCommandResultError();
}
}
Expand Down
10 changes: 5 additions & 5 deletions ...a/io/stargate/sgv2/jsonapi/service/operation/collections/FindCollectionOperationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3018,15 +3018,15 @@ public void readFailureException() throws UnknownHostException {
ThrowableToErrorMapper.getMapperWithMessageFunction()
.apply(failure, failure.getMessage());
AssertionsForClassTypes.assertThat(error).isNotNull();
AssertionsForClassTypes.assertThat(error.message())
tatu-at-datastax marked this conversation as resolved.
Show resolved Hide resolved
.isEqualTo(
"Database read failed: root cause: (com.datastax.oss.driver.api.core.servererrors.ReadFailureException) Cassandra failure during read query at consistency ONE (0 responses were required but only 1 replica responded, 1 failed)");
AssertionsForClassTypes.assertThat(error.fields().get("errorCode"))
.isEqualTo("SERVER_READ_FAILED");
AssertionsForClassTypes.assertThat(error.fields().get("exceptionClass"))
.isEqualTo("JsonApiException");
AssertionsForClassTypes.assertThat(error.status())
.isEqualTo(Response.Status.INTERNAL_SERVER_ERROR);
AssertionsForClassTypes.assertThat(error.status()).isEqualTo(Response.Status.BAD_GATEWAY);
AssertionsForClassTypes.assertThat(error.message())
.startsWith("Database read failed")
.endsWith(
"Cassandra failure during read query at consistency ONE (0 responses were required but only 1 replica responded, 1 failed)");
}
}

Expand Down