Better error handling (#13)

* Add check in rewriters for query without FROM * Return useful information when error happens * Return useful information when error happens * remove integration test * Remove unused comment * Reuse new added sendResponse
opendistro-for-elasticsearch · Mar 26, 2019 · 05d729b · 05d729b
1 parent c327af3
commit 05d729b
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 36 deletions.
diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/ErrorMessage.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/executor/format/ErrorMessage.java
@@ -17,9 +17,6 @@
 
 import org.json.JSONObject;
 
-import java.io.PrintWriter;
-import java.io.StringWriter;
-
 public class ErrorMessage {
 
     private Exception exception;
@@ -38,16 +35,12 @@ public ErrorMessage(Exception exception, int status) {
         this.details = fetchDetails();
     }
 
-    private String fetchType() { return exception.getClass().getName(); }
+    private String fetchType() { return exception.getClass().getSimpleName(); }
 
     private String fetchReason() { return emptyStringIfNull(exception.getLocalizedMessage()); }
 
     private String fetchDetails() {
-        StringWriter sw = new StringWriter();
-        PrintWriter pw = new PrintWriter(sw);
-        exception.printStackTrace(pw);
-
-        return sw.toString();
+        return exception.toString();
     }
 
     private String emptyStringIfNull(String str) { return str != null ? str : ""; }

diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java
@@ -15,17 +15,24 @@
 
 package com.amazon.opendistroforelasticsearch.sql.plugin;
 
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
-import org.elasticsearch.client.node.NodeClient;
-import org.elasticsearch.common.settings.Settings;
+import com.alibaba.druid.sql.parser.ParserException;
+import com.amazon.opendistroforelasticsearch.sql.exception.SqlParseException;
 import com.amazon.opendistroforelasticsearch.sql.executor.ActionRequestRestExecutorFactory;
 import com.amazon.opendistroforelasticsearch.sql.executor.RestExecutor;
-import org.elasticsearch.rest.*;
+import com.amazon.opendistroforelasticsearch.sql.executor.format.ErrorMessage;
+import com.amazon.opendistroforelasticsearch.sql.query.QueryAction;
 import com.amazon.opendistroforelasticsearch.sql.request.SqlRequest;
 import com.amazon.opendistroforelasticsearch.sql.request.SqlRequestFactory;
-import com.amazon.opendistroforelasticsearch.sql.exception.SqlParseException;
-import com.amazon.opendistroforelasticsearch.sql.query.QueryAction;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.elasticsearch.client.node.NodeClient;
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.index.IndexNotFoundException;
+import org.elasticsearch.rest.BaseRestHandler;
+import org.elasticsearch.rest.BytesRestResponse;
+import org.elasticsearch.rest.RestController;
+import org.elasticsearch.rest.RestRequest;
+import org.elasticsearch.rest.RestStatus;
 
 import java.sql.SQLFeatureNotSupportedException;
 import java.util.Arrays;
@@ -34,6 +41,10 @@
 import java.util.Map;
 import java.util.Set;
 
+import static org.elasticsearch.rest.RestStatus.BAD_REQUEST;
+import static org.elasticsearch.rest.RestStatus.OK;
+import static org.elasticsearch.rest.RestStatus.SERVICE_UNAVAILABLE;
+
 public class RestSqlAction extends BaseRestHandler {
     private static final Logger LOG = LogManager.getLogger(RestSqlAction.class);
 
@@ -56,43 +67,29 @@ public String getName() {
 
     @Override
     protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) {
-        SqlRequest sqlRequest;
-        try {
-            sqlRequest = SqlRequestFactory.getSqlRequest(request);
-        } catch(IllegalArgumentException e) {
-            // FIXME: need to send proper error response to client.
-            LOG.error("Failed to parse SQL request.", e);
-            return null;
-        }
-
         try {
-            SearchDao searchDao = new SearchDao(client);
-            QueryAction queryAction= null;
-
-            queryAction = searchDao.explain(sqlRequest.getSql());
+            final SqlRequest sqlRequest = SqlRequestFactory.getSqlRequest(request);
+            final QueryAction queryAction = new SearchDao(client).explain(sqlRequest.getSql());
             queryAction.setSqlRequest(sqlRequest);
 
-            // TODO add unittests to explain. (rest level?)
             if (request.path().endsWith("/_explain")) {
                 final String jsonExplanation = queryAction.explain().explain();
-                return channel -> channel.sendResponse(new BytesRestResponse(RestStatus.OK, jsonExplanation));
+                return sendResponse(jsonExplanation, OK);
             } else {
                 Map<String, String> params = request.params();
                 RestExecutor restExecutor = ActionRequestRestExecutorFactory.createExecutor(params.get("format"), queryAction);
-                final QueryAction finalQueryAction = queryAction;
                 //doing this hack because elasticsearch throws exception for un-consumed props
                 Map<String,String> additionalParams = new HashMap<>();
                 for (String paramName : responseParams()) {
                     if (request.hasParam(paramName)) {
                         additionalParams.put(paramName, request.param(paramName));
                     }
                 }
-                return channel -> restExecutor.execute(client, additionalParams, finalQueryAction, channel);
+                return channel -> restExecutor.execute(client, additionalParams, queryAction, channel);
             }
-        } catch (SqlParseException | SQLFeatureNotSupportedException e) {
-            // FIXME: need to catch all exceptions to avoid ES process from crashing
+        } catch (Exception e) {
             LOG.error("Failed during Query Action.", e);
-            return null;
+            return reportError(e, isClientError(e) ? BAD_REQUEST : SERVICE_UNAVAILABLE);
         }
     }
 
@@ -103,4 +100,20 @@ protected Set<String> responseParams() {
         return responseParams;
     }
 
+    private boolean isClientError(Exception e) {
+        return e instanceof NullPointerException | // NPE is hard to differentiate but more likely caused by bad query
+               e instanceof SqlParseException |
+               e instanceof ParserException |
+               e instanceof SQLFeatureNotSupportedException |
+               e instanceof IllegalArgumentException |
+               e instanceof IndexNotFoundException;
+    }
+
+    private RestChannelConsumer reportError(Exception e, RestStatus status) {
+        return sendResponse(new ErrorMessage(e, status.getStatus()).toString(), status);
+    }
+
+    private RestChannelConsumer sendResponse(String message, RestStatus status) {
+        return channel -> channel.sendResponse(new BytesRestResponse(status, message));
+    }
 }
diff --git a/...ava/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldRewriter.java b/...ava/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldRewriter.java
@@ -65,6 +65,10 @@ public TermFieldRewriter(Client client, TermRewriterFilter filterType) {
     @Override
     public boolean visit(MySqlSelectQueryBlock query) {
         environment.push(new TermFieldScope());
+        if (query.getFrom() == null) {
+            return false;
+        }
+
         Map<String, String> indexToType = new HashMap<>();
         collect(query.getFrom(), indexToType, curScope().getAliases());
         curScope().setMapper(getMappings(indexToType));

diff --git a/...a/com/amazon/opendistroforelasticsearch/sql/rewriter/nestedfield/NestedFieldRewriter.java b/...a/com/amazon/opendistroforelasticsearch/sql/rewriter/nestedfield/NestedFieldRewriter.java
@@ -72,6 +72,9 @@ public class NestedFieldRewriter extends MySqlASTVisitorAdapter {
     @Override
     public boolean visit(MySqlSelectQueryBlock query) {
         environment.push(new Scope());
+        if (query.getFrom() == null) {
+            return false;
+        }
 
         query.getFrom().setParent(query);
         new From(query.getFrom()).rewrite(curScope());

diff --git a/...test/java/com/amazon/opendistroforelasticsearch/sql/unittest/NestedFieldRewriterTest.java b/...test/java/com/amazon/opendistroforelasticsearch/sql/unittest/NestedFieldRewriterTest.java
@@ -48,6 +48,12 @@ public void regression() {
         noImpact("SELECT COUNT(*) FROM team GROUP BY region");
     }
 
+    @Test
+    public void selectWithoutFrom() {
+        // Expect no exception thrown
+        query("SELECT now()");
+    }
+
     @Test
     public void selectAll() {
         same(