From 48e13283151f6b90360d6c41d2cc616adc98c0fa Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 22 Jan 2021 08:57:30 -0800 Subject: [PATCH 1/3] Fix sql error message --- .../elasticsearch/response/error/ErrorMessage.java | 12 ++++++++++-- .../response/error/ErrorMessageFactory.java | 2 +- .../response/format/JdbcResponseFormatter.java | 12 ++++++++---- .../response/format/JdbcResponseFormatterTest.java | 6 +++--- 4 files changed, 22 insertions(+), 10 deletions(-) diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/error/ErrorMessage.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/error/ErrorMessage.java index d09b043dbd..41a047bea0 100644 --- a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/error/ErrorMessage.java +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/error/ErrorMessage.java @@ -17,6 +17,7 @@ package com.amazon.opendistroforelasticsearch.sql.elasticsearch.response.error; +import lombok.Getter; import org.elasticsearch.rest.RestStatus; import org.json.JSONObject; @@ -25,17 +26,24 @@ */ public class ErrorMessage { - protected Exception exception; + protected Throwable exception; + @Getter private int status; + + @Getter private String type; + + @Getter private String reason; + + @Getter private String details; /** * Error Message Constructor. */ - public ErrorMessage(Exception exception, int status) { + public ErrorMessage(Throwable exception, int status) { this.exception = exception; this.status = status; diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/error/ErrorMessageFactory.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/error/ErrorMessageFactory.java index e0538b9cf1..73d0f924dc 100644 --- a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/error/ErrorMessageFactory.java +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/error/ErrorMessageFactory.java @@ -31,7 +31,7 @@ public class ErrorMessageFactory { * @param status exception status code * @return error message */ - public static ErrorMessage createErrorMessage(Exception e, int status) { + public static ErrorMessage createErrorMessage(Throwable e, int status) { Throwable cause = unwrapCause(e); if (cause instanceof ElasticsearchException) { ElasticsearchException exception = (ElasticsearchException) cause; diff --git a/protocol/src/main/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatter.java b/protocol/src/main/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatter.java index b293e2db77..a6adb4aa85 100644 --- a/protocol/src/main/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatter.java +++ b/protocol/src/main/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatter.java @@ -18,6 +18,8 @@ import com.amazon.opendistroforelasticsearch.sql.common.antlr.SyntaxCheckException; import com.amazon.opendistroforelasticsearch.sql.data.type.ExprType; +import com.amazon.opendistroforelasticsearch.sql.elasticsearch.response.error.ErrorMessage; +import com.amazon.opendistroforelasticsearch.sql.elasticsearch.response.error.ErrorMessageFactory; import com.amazon.opendistroforelasticsearch.sql.exception.QueryEngineException; import com.amazon.opendistroforelasticsearch.sql.executor.ExecutionEngine.Schema; import com.amazon.opendistroforelasticsearch.sql.protocol.response.QueryResult; @@ -56,11 +58,13 @@ protected Object buildJsonObject(QueryResult response) { @Override public String format(Throwable t) { + ErrorMessage message = ErrorMessageFactory.createErrorMessage(t, getStatus(t)); + Error error = new Error( - t.getClass().getSimpleName(), - t.getMessage(), - t.getMessage()); - return jsonify(new JdbcErrorResponse(error, getStatus(t))); + message.getType(), + message.getReason(), + message.getDetails()); + return jsonify(new JdbcErrorResponse(error, message.getStatus())); } private Column fetchColumn(Schema.Column col) { diff --git a/protocol/src/test/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatterTest.java b/protocol/src/test/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatterTest.java index 2705b31d50..208f4cc902 100644 --- a/protocol/src/test/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatterTest.java +++ b/protocol/src/test/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatterTest.java @@ -118,7 +118,7 @@ void format_client_error_response_due_to_syntax_exception() { "{\"error\":" + "{\"" + "type\":\"SyntaxCheckException\"," - + "\"reason\":\"Invalid query syntax\"," + + "\"reason\":\"Invalid Query\"," + "\"details\":\"Invalid query syntax\"" + "}," + "\"status\":400}", @@ -132,7 +132,7 @@ void format_client_error_response_due_to_semantic_exception() { "{\"error\":" + "{\"" + "type\":\"SemanticCheckException\"," - + "\"reason\":\"Invalid query semantics\"," + + "\"reason\":\"Invalid Query\"," + "\"details\":\"Invalid query semantics\"" + "}," + "\"status\":400}", @@ -146,7 +146,7 @@ void format_server_error_response() { "{\"error\":" + "{\"" + "type\":\"IllegalStateException\"," - + "\"reason\":\"Execution error\"," + + "\"reason\":\"There was internal problem at backend\"," + "\"details\":\"Execution error\"" + "}," + "\"status\":503}", From 5b1b680f8c1be09a1c79d7619b6df96d82445bb2 Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 22 Jan 2021 10:28:46 -0800 Subject: [PATCH 2/3] Refactor format method --- .../sql/elasticsearch/response/error/ErrorMessage.java | 9 ++++----- .../protocol/response/format/JdbcResponseFormatter.java | 6 +++--- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/error/ErrorMessage.java b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/error/ErrorMessage.java index 41a047bea0..ece094d8f6 100644 --- a/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/error/ErrorMessage.java +++ b/elasticsearch/src/main/java/com/amazon/opendistroforelasticsearch/sql/elasticsearch/response/error/ErrorMessage.java @@ -28,17 +28,16 @@ public class ErrorMessage { protected Throwable exception; - @Getter - private int status; + private final int status; @Getter - private String type; + private final String type; @Getter - private String reason; + private final String reason; @Getter - private String details; + private final String details; /** * Error Message Constructor. diff --git a/protocol/src/main/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatter.java b/protocol/src/main/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatter.java index a6adb4aa85..52f8c1d703 100644 --- a/protocol/src/main/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatter.java +++ b/protocol/src/main/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatter.java @@ -58,13 +58,13 @@ protected Object buildJsonObject(QueryResult response) { @Override public String format(Throwable t) { - ErrorMessage message = ErrorMessageFactory.createErrorMessage(t, getStatus(t)); - + int status = getStatus(t); + ErrorMessage message = ErrorMessageFactory.createErrorMessage(t, status); Error error = new Error( message.getType(), message.getReason(), message.getDetails()); - return jsonify(new JdbcErrorResponse(error, message.getStatus())); + return jsonify(new JdbcErrorResponse(error, status)); } private Column fetchColumn(Schema.Column col) { From f0297e347edc4f1bda99c5a2ed2c80e3d6a39d10 Mon Sep 17 00:00:00 2001 From: Dai Date: Fri, 22 Jan 2021 10:47:51 -0800 Subject: [PATCH 3/3] Add more UT --- .../format/JdbcResponseFormatterTest.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/protocol/src/test/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatterTest.java b/protocol/src/test/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatterTest.java index 208f4cc902..e421016c00 100644 --- a/protocol/src/test/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatterTest.java +++ b/protocol/src/test/java/com/amazon/opendistroforelasticsearch/sql/protocol/response/format/JdbcResponseFormatterTest.java @@ -39,6 +39,7 @@ import com.google.common.collect.ImmutableMap; import com.google.gson.JsonParser; import java.util.Arrays; +import org.elasticsearch.ElasticsearchException; import org.junit.jupiter.api.DisplayNameGeneration; import org.junit.jupiter.api.DisplayNameGenerator; import org.junit.jupiter.api.Test; @@ -154,6 +155,25 @@ void format_server_error_response() { ); } + @Test + void format_server_error_response_due_to_elasticsearch() { + assertJsonEquals( + "{\"error\":" + + "{\"" + + "type\":\"ElasticsearchException\"," + + "\"reason\":\"Error occurred in Elasticsearch engine: all shards failed\"," + + "\"details\":\"ElasticsearchException[all shards failed]; " + + "nested: IllegalStateException[Execution error];; " + + "java.lang.IllegalStateException: Execution error\\n" + + "For more details, please send request for Json format to see the raw response " + + "from elasticsearch engine.\"" + + "}," + + "\"status\":503}", + formatter.format(new ElasticsearchException("all shards failed", + new IllegalStateException("Execution error"))) + ); + } + private static void assertJsonEquals(String expected, String actual) { assertEquals( JsonParser.parseString(expected),