From c7f006cee333973092a780af83da4fe45327af0f Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Wed, 23 Oct 2024 12:35:56 -0700 Subject: [PATCH 1/2] Improve error handling for some more edge cases (#3080) * Add failing tests Signed-off-by: Simeon Widdis * Fix the first test Signed-off-by: Simeon Widdis * Revise the tests Signed-off-by: Simeon Widdis * Fix wildcard tests Signed-off-by: Simeon Widdis * Add license header Signed-off-by: Simeon Widdis * Fix rerunning SQL parsing Signed-off-by: Simeon Widdis --------- Signed-off-by: Simeon Widdis --- .../sql/legacy/MalformedQueryIT.java | 82 +++++++++++++++++++ .../legacy/query/OpenSearchActionFactory.java | 10 ++- .../matchtoterm/TermFieldRewriter.java | 17 ++++ 3 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java new file mode 100644 index 0000000000..84b60fdabd --- /dev/null +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java @@ -0,0 +1,82 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.legacy; + +import java.io.IOException; +import java.util.Locale; +import org.apache.hc.core5.http.ParseException; +import org.apache.hc.core5.http.io.entity.EntityUtils; +import org.json.JSONObject; +import org.junit.Assert; +import org.opensearch.client.ResponseException; + +/** Tests for clean handling of various types of invalid queries */ +public class MalformedQueryIT extends SQLIntegTestCase { + @Override + protected void init() throws Exception { + loadIndex(Index.BANK); + loadIndex(Index.BANK_TWO); + } + + public void testJoinWithInvalidCondition() throws IOException, ParseException { + ResponseException result = + assertThrows( + "Expected Join query with malformed 'ON' to raise error, but didn't", + ResponseException.class, + () -> + executeQuery( + String.format( + Locale.ROOT, + "SELECT a.firstname, b.age FROM %s AS a INNER JOIN %s AS b %%" + + " a.account_number=b.account_number", + TestsConstants.TEST_INDEX_BANK, + TestsConstants.TEST_INDEX_BANK_TWO))); + var errMsg = new JSONObject(EntityUtils.toString(result.getResponse().getEntity())); + + Assert.assertEquals("SqlParseException", errMsg.getJSONObject("error").getString("type")); + Assert.assertEquals(400, errMsg.getInt("status")); + } + + public void testWrappedWildcardInSubquery() throws IOException, ParseException { + ResponseException result = + assertThrows( + "Expected wildcard subquery to raise error, but didn't", + ResponseException.class, + () -> + executeQuery( + String.format( + Locale.ROOT, + "SELECT a.first_name FROM %s AS a WHERE a.age IN (SELECT age FROM" + + " `opensearch-sql_test_index_*` WHERE age > 30)", + TestsConstants.TEST_INDEX_BANK, + TestsConstants.TEST_INDEX_BANK_TWO))); + var errMsg = new JSONObject(EntityUtils.toString(result.getResponse().getEntity())); + System.err.println("Full response: " + errMsg); + + Assert.assertEquals("IndexNotFoundException", errMsg.getJSONObject("error").getString("type")); + Assert.assertEquals(404, errMsg.getInt("status")); + } + + public void testUnwrappedWildcardInSubquery() throws IOException, ParseException { + ResponseException result = + assertThrows( + "Expected wildcard subquery to raise error, but didn't", + ResponseException.class, + () -> + executeQuery( + String.format( + Locale.ROOT, + "SELECT a.first_name FROM %s AS a WHERE a.age IN (SELECT age FROM * WHERE" + + " age > 30)", + TestsConstants.TEST_INDEX_BANK, + TestsConstants.TEST_INDEX_BANK_TWO))); + var errMsg = new JSONObject(EntityUtils.toString(result.getResponse().getEntity())); + System.err.println("Full response: " + errMsg); + + Assert.assertEquals("IndexNotFoundException", errMsg.getJSONObject("error").getString("type")); + Assert.assertEquals(404, errMsg.getInt("status")); + } +} diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java index b9a7c9f218..a5bfeebfbd 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/query/OpenSearchActionFactory.java @@ -8,6 +8,7 @@ import static org.opensearch.sql.legacy.domain.IndexStatement.StatementType; import static org.opensearch.sql.legacy.utils.Util.toSqlExpr; +import com.alibaba.druid.sql.ast.SQLExpr; import com.alibaba.druid.sql.ast.expr.SQLAggregateExpr; import com.alibaba.druid.sql.ast.expr.SQLAllColumnExpr; import com.alibaba.druid.sql.ast.expr.SQLMethodInvokeExpr; @@ -86,7 +87,14 @@ public static QueryAction create(Client client, QueryActionRequest request) switch (getFirstWord(sql)) { case "SELECT": - SQLQueryExpr sqlExpr = (SQLQueryExpr) toSqlExpr(sql); + SQLExpr rawExpr = toSqlExpr(sql); + if (!(rawExpr instanceof SQLQueryExpr)) { + throw new SqlParseException( + "Expected a query expression, but found a " + + rawExpr.getClass().getSimpleName() + + ". The query is not runnable."); + } + SQLQueryExpr sqlExpr = (SQLQueryExpr) rawExpr; RewriteRuleExecutor ruleExecutor = RewriteRuleExecutor.builder() diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/rewriter/matchtoterm/TermFieldRewriter.java b/legacy/src/main/java/org/opensearch/sql/legacy/rewriter/matchtoterm/TermFieldRewriter.java index 1200c8befb..344d1a268b 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/rewriter/matchtoterm/TermFieldRewriter.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/rewriter/matchtoterm/TermFieldRewriter.java @@ -28,6 +28,7 @@ import java.util.Map; import java.util.Optional; import java.util.stream.Stream; +import org.opensearch.index.IndexNotFoundException; import org.opensearch.sql.legacy.esdomain.LocalClusterState; import org.opensearch.sql.legacy.esdomain.mapping.FieldMappings; import org.opensearch.sql.legacy.esdomain.mapping.IndexMappings; @@ -122,7 +123,23 @@ public boolean visit(SQLIdentifierExpr expr) { String fullFieldName = arr[1]; String index = curScope().getAliases().get(alias); + if (index == null) { + throw new IndexNotFoundException( + String.format( + "The requested table '%s' does not correspond to any known index. Only indices or" + + " table aliases are allowed.", + alias.replaceFirst("_\\d+$", ""))); + } + FieldMappings fieldMappings = curScope().getMapper().mapping(index); + if (fieldMappings == null) { + throw new IndexNotFoundException( + String.format( + "The index '%s' could not be found. Note that wildcard indices are not permitted" + + " in SQL.", + index)); + } + if (fieldMappings.has(fullFieldName)) { source = fieldMappings.mapping(fullFieldName); } else { From ec7b1a9d567579602f562b7c59fc2c478e9fce07 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Wed, 23 Oct 2024 15:26:59 -0700 Subject: [PATCH 2/2] Fix test imports Signed-off-by: Simeon Widdis --- .../test/java/org/opensearch/sql/legacy/MalformedQueryIT.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java b/integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java index 84b60fdabd..a0aa1b08a9 100644 --- a/integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/legacy/MalformedQueryIT.java @@ -7,8 +7,8 @@ import java.io.IOException; import java.util.Locale; -import org.apache.hc.core5.http.ParseException; -import org.apache.hc.core5.http.io.entity.EntityUtils; +import org.apache.http.ParseException; +import org.apache.http.util.EntityUtils; import org.json.JSONObject; import org.junit.Assert; import org.opensearch.client.ResponseException;