From 05d729babee8dd686b05583440742c98a77c1627 Mon Sep 17 00:00:00 2001 From: Chen Dai <46505291+dai-chen@users.noreply.github.com> Date: Mon, 25 Mar 2019 17:18:25 -0700 Subject: [PATCH] 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 --- .../sql/executor/format/ErrorMessage.java | 11 +-- .../sql/plugin/RestSqlAction.java | 67 +++++++++++-------- .../matchtoterm/TermFieldRewriter.java | 4 ++ .../nestedfield/NestedFieldRewriter.java | 3 + .../sql/unittest/NestedFieldRewriterTest.java | 6 ++ 5 files changed, 55 insertions(+), 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 index 556e14c821..95ac3d61ec 100644 --- 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 index aa7c35d51e..c84dca41b7 100644 --- 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,30 +67,17 @@ 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 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 additionalParams = new HashMap<>(); for (String paramName : responseParams()) { @@ -87,12 +85,11 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli 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 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)); + } } \ No newline at end of file diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldRewriter.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldRewriter.java index eda63be9d4..5bd138392f 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/matchtoterm/TermFieldRewriter.java +++ b/src/main/java/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 indexToType = new HashMap<>(); collect(query.getFrom(), indexToType, curScope().getAliases()); curScope().setMapper(getMappings(indexToType)); diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/nestedfield/NestedFieldRewriter.java b/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/nestedfield/NestedFieldRewriter.java index e160e389b8..2967c9fdd7 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/rewriter/nestedfield/NestedFieldRewriter.java +++ b/src/main/java/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/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/NestedFieldRewriterTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/NestedFieldRewriterTest.java index d46aaa264d..585b71680d 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/NestedFieldRewriterTest.java +++ b/src/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(