Fix #1383: map most recognized SERVER_ errors to 504 or 502, leave 50…

…0 for unrecognized (#1384)
stargate · Sep 3, 2024 · ee4c929 · ee4c929
1 parent 869d6af
commit ee4c929
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 20 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
@@ -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.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);
     }
   }
@@ -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();
         }
       }

diff --git a/...a/io/stargate/sgv2/jsonapi/service/operation/collections/FindCollectionOperationTest.java b/...a/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)");
     }
   }