Handle the elasticsearch exceptions in JDBC formatted outputs (#362)

* Caught ES exception

* Added details in errMsgs to enrich the behavior; added IT

* Handled cases where ES exceptions are wrapped up; added default fetching details method

* Added factory method to construct ErrorMessage; extended exception type for ErrorMessage

* Added UT for ErrorMessageFactory

* addressed comments

src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/AsyncRestExecutor.java

@@ -24,6 +24,7 @@
 import com.amazon.opendistroforelasticsearch.sql.utils.LogUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.client.Client;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.rest.BytesRestResponse;
@@ -118,7 +119,7 @@ private void async(Client client, Map<String, String> params, QueryAction queryA
         Runnable runnable = () -> {
             try {
                 doExecuteWithTimeMeasured(client, params, queryAction, channel);
-            } catch (IOException | SqlParseException e) {
+            } catch (IOException | SqlParseException | ElasticsearchException e) {
                 Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_SYS).increment();
                 LOG.warn("[{}] [MCB] async task got an IO/SQL exception: {}", LogUtils.getRequestId(),
                         e.getMessage());

src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/ElasticsearchErrorMessage.java

@@ -0,0 +1,67 @@
+/*
+ *   Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ *   Licensed under the Apache License, Version 2.0 (the "License").
+ *   You may not use this file except in compliance with the License.
+ *   A copy of the License is located at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *   or in the "license" file accompanying this file. This file is distributed
+ *   on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ *   express or implied. See the License for the specific language governing
+ *   permissions and limitations under the License.
+ */
+
+package com.amazon.opendistroforelasticsearch.sql.executor.format;
+
+import com.amazon.opendistroforelasticsearch.sql.utils.StringUtils;
+import org.elasticsearch.ElasticsearchException;
+import org.elasticsearch.action.search.SearchPhaseExecutionException;
+import org.elasticsearch.action.search.ShardSearchFailure;
+
+public class ElasticsearchErrorMessage extends ErrorMessage<ElasticsearchException> {
+
+    ElasticsearchErrorMessage(ElasticsearchException exception, int status) {
+        super(exception, status);
+    }
+
+    @Override
+    protected String fetchReason() {
+        return "Error occurred in Elasticsearch engine: " + exception.getMessage();
+    }
+
+    /** Currently Sql-Jdbc plugin only supports string type as reason and details in the error messages */
+    @Override
+    protected String fetchDetails() {
+        StringBuilder details = new StringBuilder();
+        if (exception instanceof SearchPhaseExecutionException) {
+            details.append(fetchSearchPhaseExecutionExceptionDetails((SearchPhaseExecutionException) exception));
+        } else {
+            details.append(defaultDetails(exception));
+        }
+        details.append("\nFor more details, please send request for Json format to see the raw response from "
+                + "elasticsearch engine.");
+        return details.toString();
+    }
+
+    private String defaultDetails(ElasticsearchException exception) {
+        return exception.getDetailedMessage();
+    }
+
+    /**
+     * Could not deliver the exactly same error messages due to the limit of JDBC types.
+     * Currently our cases occurred only SearchPhaseExecutionException instances among all types of ES exceptions
+     * according to the survey, see all types: ElasticsearchException.ElasticsearchExceptionHandle.
+     * Either add methods of fetching details for different types, or re-make a consistent message by not giving
+     * detailed messages/root causes but only a suggestion message.
+     */
+    private String fetchSearchPhaseExecutionExceptionDetails(SearchPhaseExecutionException exception) {
+        StringBuilder details = new StringBuilder();
+        ShardSearchFailure[] shardFailures = exception.shardFailures();
+        for (ShardSearchFailure failure : shardFailures) {
+            details.append(StringUtils.format("Shard[%d]: %s\n", failure.shardId(), failure.getCause().toString()));
+        }
+        return details.toString();
+    }
+}

src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/ErrorMessage.java

@@ -18,16 +18,16 @@
 import org.elasticsearch.rest.RestStatus;
 import org.json.JSONObject;
 
-public class ErrorMessage {
+public class ErrorMessage<E extends Exception> {
 
-    private Exception exception;
+    protected E exception;
 
     private int status;
     private String type;
     private String reason;
     private String details;
 
-    public ErrorMessage(Exception exception, int status) {
+    public ErrorMessage(E exception, int status) {
         this.exception = exception;
         this.status = status;
 
@@ -40,13 +40,13 @@ private String fetchType() {
         return exception.getClass().getSimpleName();
     }
 
-    private String fetchReason() {
+    protected String fetchReason() {
         return status == RestStatus.BAD_REQUEST.getStatus()
                 ? "Invalid SQL query"
                 : "There was internal problem at backend";
     }
 
-    private String fetchDetails() {
+    protected String fetchDetails() {
         // Some exception prints internal information (full class name) which is security concern
         //return exception.toString();
         return emptyStringIfNull(exception.getLocalizedMessage());

src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/ErrorMessageFactory.java

@@ -0,0 +1,53 @@
+/*
+ *   Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ *   Licensed under the Apache License, Version 2.0 (the "License").
+ *   You may not use this file except in compliance with the License.
+ *   A copy of the License is located at
+ *
+ *       http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *   or in the "license" file accompanying this file. This file is distributed
+ *   on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+ *   express or implied. See the License for the specific language governing
+ *   permissions and limitations under the License.
+ */
+
+package com.amazon.opendistroforelasticsearch.sql.executor.format;
+
+import org.elasticsearch.ElasticsearchException;
+
+public class ErrorMessageFactory {
+    /**
+     * Create error message based on the exception type
+     * Exceptions of ES exception type and exceptions with wrapped ES exception causes
+     * should create {@link ElasticsearchErrorMessage}
+     *
+     * @param e         exception to create error message
+     * @param status    exception status code
+     * @return          error message
+     */
+
+    public static ErrorMessage createErrorMessage(Exception e, int status) {
+        if (e instanceof ElasticsearchException) {
+            return new ElasticsearchErrorMessage((ElasticsearchException) e,
+                    ((ElasticsearchException) e).status().getStatus());
+        } else if (unwrapCause(e) instanceof ElasticsearchException) {
+            ElasticsearchException exception = (ElasticsearchException) unwrapCause(e);
+            return new ElasticsearchErrorMessage(exception, exception.status().getStatus());
+        }
+        return new ErrorMessage(e, status);
+    }
+
+    public static Throwable unwrapCause(Throwable t) {
+        Throwable result = t;
+        if (result instanceof ElasticsearchException) {
+            return result;
+        }
+        if (result.getCause() == null) {
+            return result;
+        }
+        result = unwrapCause(result.getCause());
+        return result;
+    }
+}

src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/PrettyFormatRestExecutor.java

@@ -21,6 +21,7 @@
 import com.amazon.opendistroforelasticsearch.sql.query.join.BackOffRetryStrategy;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.client.Client;
 import org.elasticsearch.rest.BytesRestResponse;
 import org.elasticsearch.rest.RestChannel;
@@ -69,7 +70,12 @@ public String execute(Client client, Map<String, String> params, QueryAction que
             Object queryResult = QueryActionElasticExecutor.executeAnyAction(client, queryAction);
             protocol = new Protocol(client, queryAction, queryResult, format);
         } catch (Exception e) {
-            LOG.error("Error happened in pretty formatter", e);
+            if (e instanceof ElasticsearchException) {
+                LOG.warn("An error occurred in Elasticsearch engine: "
+                        + ((ElasticsearchException) e).getDetailedMessage(), e);
+            } else {
+                LOG.warn("Error happened in pretty formatter", e);
+            }
             protocol = new Protocol(e);
         }
 

src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/Protocol.java

@@ -70,7 +70,7 @@ public Protocol(Client client, QueryAction queryAction, Object queryResult, Stri
     public Protocol(Exception e) {
         this.formatType = null;
         this.status = ERROR_STATUS;
-        this.error = new ErrorMessage(e, ERROR_STATUS);
+        this.error = ErrorMessageFactory.createErrorMessage(e, status);
     }
 
     private ResultSet loadResultSet(Client client, QueryStatement queryStatement, Object queryResult) {

src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java

@@ -28,7 +28,7 @@
 import com.amazon.opendistroforelasticsearch.sql.executor.ActionRequestRestExecutorFactory;
 import com.amazon.opendistroforelasticsearch.sql.executor.Format;
 import com.amazon.opendistroforelasticsearch.sql.executor.RestExecutor;
-import com.amazon.opendistroforelasticsearch.sql.executor.format.ErrorMessage;
+import com.amazon.opendistroforelasticsearch.sql.executor.format.ErrorMessageFactory;
 import com.amazon.opendistroforelasticsearch.sql.metrics.MetricName;
 import com.amazon.opendistroforelasticsearch.sql.metrics.Metrics;
 import com.amazon.opendistroforelasticsearch.sql.query.QueryAction;
@@ -202,7 +202,7 @@ private void sendResponse(final RestChannel channel, final String message, final
     }
 
     private void reportError(final RestChannel channel, final Exception e, final RestStatus status) {
-        sendResponse(channel, new ErrorMessage(e, status.getStatus()).toString(), status);
+        sendResponse(channel, ErrorMessageFactory.createErrorMessage(e, status.getStatus()).toString(), status);
     }
 
     private boolean isSQLFeatureEnabled() {

src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlStatsAction.java

@@ -15,7 +15,7 @@
 
 package com.amazon.opendistroforelasticsearch.sql.plugin;
 
-import com.amazon.opendistroforelasticsearch.sql.executor.format.ErrorMessage;
+import com.amazon.opendistroforelasticsearch.sql.executor.format.ErrorMessageFactory;
 import com.amazon.opendistroforelasticsearch.sql.metrics.Metrics;
 import com.amazon.opendistroforelasticsearch.sql.utils.LogUtils;
 import org.apache.logging.log4j.LogManager;
@@ -69,7 +69,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
             LOG.error("Failed during Query SQL STATS Action.", e);
 
             return channel -> channel.sendResponse(new BytesRestResponse(SERVICE_UNAVAILABLE,
-                    new ErrorMessage(e, SERVICE_UNAVAILABLE.getStatus()).toString()));
+                    ErrorMessageFactory.createErrorMessage(e, SERVICE_UNAVAILABLE.getStatus()).toString()));
         }
     }
 

src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/QueryIT.java

@@ -36,6 +36,8 @@
 import java.util.Set;
 
 import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.TEST_INDEX_ACCOUNT;
+import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.TEST_INDEX_BANK;
+import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.TEST_INDEX_BANK_WITH_NULL_VALUES;
 import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.TEST_INDEX_GAME_OF_THRONES;
 import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.TEST_INDEX_NESTED_TYPE;
 import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.TEST_INDEX_ONLINE;
@@ -85,6 +87,7 @@ protected void init() throws Exception {
         // loadIndex(Index.JOIN);
         loadIndex(Index.BANK);
         loadIndex(Index.BANK_TWO);
+        loadIndex(Index.BANK_WITH_NULL_VALUES);
     }
 
     @Test
@@ -1768,6 +1771,40 @@ public void caseWhenJdbcResponseTest() {
         );
     }
 
+    @Test
+    public void functionInCaseFieldShouldThrowESExceptionDueToIllegalScriptInJdbc() {
+        String response = executeQuery(
+                "select case lower(firstname) when 'amber' then '1' else '2' end as cases from " + TEST_INDEX_ACCOUNT,
+                "jdbc");
+        queryInJdbcResponseShouldIndicateESException(response, "SearchPhaseExecutionException",
+                "For more details, please send request for Json format");
+    }
+
+    @Test
+    public void functionCallWithIllegalScriptShouldThrowESExceptionInJdbc() {
+        String response = executeQuery("select log(balance + 2) from " + TEST_INDEX_BANK,
+                "jdbc");
+        queryInJdbcResponseShouldIndicateESException(response, "SearchPhaseExecutionException",
+                "please send request for Json format to see the raw response from elasticsearch engine.");
+    }
+
+    @Ignore("Goes in different route, does not call PrettyFormatRestExecutor.execute methods." +
+            "The performRequest method in RestClient doesn't throw any exceptions for null value fields in script")
+    @Test
+    public void functionArgWithNullValueFieldShouldThrowESExceptionInJdbc() {
+        String response = executeQuery(
+                "select log(balance) from " + TEST_INDEX_BANK_WITH_NULL_VALUES, "jdbc");
+        queryInJdbcResponseShouldIndicateESException(response, "SearchPhaseExecutionException",
+                "For more details, please send request for Json format");
+    }
+
+    private void queryInJdbcResponseShouldIndicateESException(String response, String exceptionType, String... errMsgs) {
+        Assert.assertThat(response, containsString(exceptionType));
+        for (String errMsg: errMsgs) {
+            Assert.assertThat(response, containsString(errMsg));
+        }
+    }
+
     private String getScrollId(JSONObject response) {
         return response.getString("_scroll_id");
     }

src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/SQLIntegTestCase.java

@@ -34,6 +34,7 @@
 import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestUtils.createIndexByRestClient;
 import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestUtils.getAccountIndexMapping;
 import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestUtils.getBankIndexMapping;
+import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestUtils.getBankWithNullValuesIndexMapping;
 import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestUtils.getDateIndexMapping;
 import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestUtils.getDogIndexMapping;
 import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestUtils.getDogs2IndexMapping;
@@ -363,6 +364,10 @@ public enum Index {
                 "account_two",
                 getBankIndexMapping(),
                 "src/test/resources/bank_two.json"),
+        BANK_WITH_NULL_VALUES(TestsConstants.TEST_INDEX_BANK_WITH_NULL_VALUES,
+                "account_null",
+                getBankWithNullValuesIndexMapping(),
+                "src/test/resources/bank_with_null_values.json"),
         ORDER(TestsConstants.TEST_INDEX_ORDER,
                 "_doc",
                  getOrderIndexMapping(),

src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TestUtils.java

@@ -521,6 +521,36 @@ public static String getBankIndexMapping() {
                 "}";
     }
 
+    public static String getBankWithNullValuesIndexMapping() {
+        return "{\n" +
+                "  \"mappings\": {\n" +
+                "    \"properties\": {\n" +
+                "      \"account_number\": {\n" +
+                "        \"type\": \"long\"\n" +
+                "      },\n" +
+                "      \"address\": {\n" +
+                "        \"type\": \"text\"\n" +
+                "      },\n" +
+                "      \"age\": {\n" +
+                "        \"type\": \"integer\"\n" +
+                "      },\n" +
+                "      \"balance\": {\n" +
+                "        \"type\": \"long\"\n" +
+                "      },\n" +
+                "      \"gender\": {\n" +
+                "        \"type\": \"text\"\n" +
+                "      },\n" +
+                "      \"firstname\": {\n" +
+                "        \"type\": \"text\"\n" +
+                "      },\n" +
+                "      \"lastname\": {\n" +
+                "        \"type\": \"keyword\"\n" +
+                "      }\n" +
+                "    }\n" +
+                "  }\n" +
+                "}";
+    }
+
     public static String getOrderIndexMapping() {
         return "{\n" +
             "  \"mappings\": {\n" +

src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TestsConstants.java

@@ -42,6 +42,7 @@ public class TestsConstants {
     public final static String TEST_INDEX_JOIN_TYPE = TEST_INDEX + "_join_type";
     public final static String TEST_INDEX_BANK = TEST_INDEX + "_bank";
     public final static String TEST_INDEX_BANK_TWO = TEST_INDEX_BANK + "_two";
+    public final static String TEST_INDEX_BANK_WITH_NULL_VALUES = TEST_INDEX_BANK + "_with_null_values";
     public final static String TEST_INDEX_ORDER = TEST_INDEX + "_order";
     public final static String TEST_INDEX_WEBLOG = TEST_INDEX + "_weblog";
     public final static String TEST_INDEX_DATE = TEST_INDEX + "_date";