From 4ca6f9a07c055cf8fd8fea571c99c02852c3be6f Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Fri, 30 Aug 2024 13:26:58 -0700 Subject: [PATCH 1/3] Fix #1383: map most recognized SERVER_ errors to 504 or 502, leave 500 for unrecognized --- .../mappers/ThrowableToErrorMapper.java | 51 +++++++++++++------ 1 file changed, 36 insertions(+), 15 deletions(-) diff --git a/src/main/java/io/stargate/sgv2/jsonapi/exception/mappers/ThrowableToErrorMapper.java b/src/main/java/io/stargate/sgv2/jsonapi/exception/mappers/ThrowableToErrorMapper.java index eb878b23ef..2bf4e089d5 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/exception/mappers/ThrowableToErrorMapper.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/exception/mappers/ThrowableToErrorMapper.java @@ -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()); @@ -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.GATEWAY_TIMEOUT); } 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); } } @@ -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); } } @@ -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); } } @@ -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(); } } From b4d5f07f762c7de3bc0fd5f96ffe9ae1a1daebb5 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Fri, 30 Aug 2024 14:29:16 -0700 Subject: [PATCH 2/3] Fix unit test --- .../collections/FindCollectionOperationTest.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/collections/FindCollectionOperationTest.java b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/collections/FindCollectionOperationTest.java index 75b078d4b1..4c5c0da691 100644 --- a/src/test/java/io/stargate/sgv2/jsonapi/service/operation/collections/FindCollectionOperationTest.java +++ b/src/test/java/io/stargate/sgv2/jsonapi/service/operation/collections/FindCollectionOperationTest.java @@ -3018,15 +3018,15 @@ public void readFailureException() throws UnknownHostException { ThrowableToErrorMapper.getMapperWithMessageFunction() .apply(failure, failure.getMessage()); AssertionsForClassTypes.assertThat(error).isNotNull(); - AssertionsForClassTypes.assertThat(error.message()) - .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)"); } } From 8271220ffc917ce99a8814fdef6a46cf1c80705a Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Tue, 3 Sep 2024 11:59:58 -0700 Subject: [PATCH 3/3] Fix one http response code based on code review --- .../sgv2/jsonapi/exception/mappers/ThrowableToErrorMapper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/io/stargate/sgv2/jsonapi/exception/mappers/ThrowableToErrorMapper.java b/src/main/java/io/stargate/sgv2/jsonapi/exception/mappers/ThrowableToErrorMapper.java index 2bf4e089d5..440a1cb35d 100644 --- a/src/main/java/io/stargate/sgv2/jsonapi/exception/mappers/ThrowableToErrorMapper.java +++ b/src/main/java/io/stargate/sgv2/jsonapi/exception/mappers/ThrowableToErrorMapper.java @@ -90,7 +90,7 @@ private static CommandResult.Error handleDriverException( } else if (throwable instanceof ClosedConnectionException) { return ErrorCode.SERVER_CLOSED_CONNECTION .toApiException("(DriverException/ClosedConnectionException) %s", message) - .getCommandResultError(Response.Status.GATEWAY_TIMEOUT); + .getCommandResultError(Response.Status.BAD_GATEWAY); } else if (throwable instanceof CoordinatorException) { return handleCoordinatorException((CoordinatorException) throwable, message); } else if (throwable instanceof DriverTimeoutException) {