From f8561e651a7d2d6745824188c97e5f44911d7c26 Mon Sep 17 00:00:00 2001 From: Andy Galkin <1936087+galkk@users.noreply.github.com> Date: Thu, 20 Jun 2019 15:16:34 -0700 Subject: [PATCH] Fix improper handling of the table expression (#89) * Throws ParserException for case, when the query was incorrect, allowing to identify it as a client error * Improved log message to differentiate server and client side errors in log --- .../sql/plugin/RestSqlAction.java | 6 ++- .../matchtoterm/TermFieldRewriter.java | 40 +++++++++++------- .../unittest/regression/RegressionTest.java | 42 +++++++++++++++++++ 3 files changed, 71 insertions(+), 17 deletions(-) create mode 100644 src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/regression/RegressionTest.java 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 5c351d5e35..06d43f4ee9 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/sql/plugin/RestSqlAction.java @@ -120,12 +120,14 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli } } catch (Exception e) { if (isClientError(e)) { + LOG.error(String.format(Locale.ROOT, "[%s] Client side error during query execution", LogUtils.getRequestId()), e); Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_CUS).increment(); + return reportError(e, BAD_REQUEST); } else { + LOG.error(String.format(Locale.ROOT, "[%s] Server side error during query execution", LogUtils.getRequestId()), e); Metrics.getInstance().getNumericalMetric(MetricName.FAILED_REQ_COUNT_SYS).increment(); + return reportError(e, SERVICE_UNAVAILABLE); } - LOG.error(String.format(Locale.ROOT, "[%s] Failed during query execution", LogUtils.getRequestId()), e); - return reportError(e, isClientError(e) ? BAD_REQUEST : SERVICE_UNAVAILABLE); } } 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 b57d8be297..8594d0cf01 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 @@ -15,6 +15,7 @@ package com.amazon.opendistroforelasticsearch.sql.rewriter.matchtoterm; +import com.alibaba.druid.sql.ast.SQLExpr; import com.alibaba.druid.sql.ast.expr.SQLBinaryOpExpr; import com.alibaba.druid.sql.ast.expr.SQLBinaryOperator; import com.alibaba.druid.sql.ast.expr.SQLIdentifierExpr; @@ -28,6 +29,7 @@ import com.alibaba.druid.sql.dialect.mysql.ast.expr.MySqlSelectGroupByExpr; import com.alibaba.druid.sql.dialect.mysql.ast.statement.MySqlSelectQueryBlock; import com.alibaba.druid.sql.dialect.mysql.visitor.MySqlASTVisitorAdapter; +import com.alibaba.druid.sql.parser.ParserException; import com.amazon.opendistroforelasticsearch.sql.esdomain.LocalClusterState; import org.elasticsearch.client.Client; import org.json.JSONObject; @@ -147,24 +149,32 @@ public boolean visit(SQLIdentifierExpr expr) { public void collect(SQLTableSource tableSource, Map indexToType, Map aliases) { if (tableSource instanceof SQLExprTableSource) { - String table = null; - if (((SQLExprTableSource) tableSource).getExpr() instanceof SQLIdentifierExpr) { - table = ((SQLIdentifierExpr) ((SQLExprTableSource) tableSource).getExpr()).getName(); - indexToType.put( - table, - null - ); - } else if (((SQLExprTableSource) tableSource).getExpr() instanceof SQLBinaryOpExpr) { - table = ((SQLIdentifierExpr)((SQLBinaryOpExpr) ((SQLExprTableSource) tableSource).getExpr()).getLeft()).getName(); - indexToType.put( - table, - ((SQLIdentifierExpr)((SQLBinaryOpExpr) ((SQLExprTableSource) tableSource).getExpr()).getRight()).getName() - ); + + String tableName = null; + SQLExprTableSource sqlExprTableSource = (SQLExprTableSource) tableSource; + + if (sqlExprTableSource.getExpr() instanceof SQLIdentifierExpr) { + SQLIdentifierExpr sqlIdentifier = (SQLIdentifierExpr) sqlExprTableSource.getExpr(); + tableName = sqlIdentifier.getName(); + indexToType.put(tableName, null); + } else if (sqlExprTableSource.getExpr() instanceof SQLBinaryOpExpr) { + SQLBinaryOpExpr sqlBinaryOpExpr = (SQLBinaryOpExpr) sqlExprTableSource.getExpr(); + tableName = ((SQLIdentifierExpr) sqlBinaryOpExpr.getLeft()).getName(); + SQLExpr rightSideOfExpression = sqlBinaryOpExpr.getRight(); + + // This assumes that right side of the expression is different name in query + if (rightSideOfExpression instanceof SQLIdentifierExpr) { + SQLIdentifierExpr right = (SQLIdentifierExpr) rightSideOfExpression; + indexToType.put(tableName, right.getName()); + } else { + throw new ParserException("Right side of the expression [" + rightSideOfExpression.toString() + + "] is expected to be an identifier"); + } } if (tableSource.getAlias() != null) { - aliases.put(tableSource.getAlias(), table); + aliases.put(tableSource.getAlias(), tableName); } else { - aliases.put(table, table); + aliases.put(tableName, tableName); } } else if (tableSource instanceof SQLJoinTableSource) { diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/regression/RegressionTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/regression/RegressionTest.java new file mode 100644 index 0000000000..083c8cde0b --- /dev/null +++ b/src/test/java/com/amazon/opendistroforelasticsearch/sql/unittest/regression/RegressionTest.java @@ -0,0 +1,42 @@ +/* + * 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.unittest.regression; + +import com.alibaba.druid.sql.parser.ParserException; +import com.amazon.opendistroforelasticsearch.sql.exception.SqlParseException; +import com.amazon.opendistroforelasticsearch.sql.query.ESActionFactory; +import org.elasticsearch.client.Client; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; + +import java.sql.SQLFeatureNotSupportedException; + +/** + * Special cases for queries that had broken our parser in past. + */ +@RunWith(MockitoJUnitRunner.class) +public class RegressionTest { + + @Mock + Client client; + + @Test(expected = ParserException.class) + public void missingWhereAndFieldName() throws SQLFeatureNotSupportedException, SqlParseException { + ESActionFactory.create(client, "select * from products like 'SomeProduct*' limit 10"); + } +}