Improve error handling for some more edge cases (#3080)

* Add failing tests Signed-off-by: Simeon Widdis <[email protected]> * Fix the first test Signed-off-by: Simeon Widdis <[email protected]> * Revise the tests Signed-off-by: Simeon Widdis <[email protected]> * Fix wildcard tests Signed-off-by: Simeon Widdis <[email protected]> * Add license header Signed-off-by: Simeon Widdis <[email protected]> * Fix rerunning SQL parsing Signed-off-by: Simeon Widdis <[email protected]> --------- Signed-off-by: Simeon Widdis <[email protected]>
opensearch-project · Oct 23, 2024 · 5852ce3 · 5852ce3
1 parent 366d165
commit 5852ce3
Show file tree

Hide file tree

Showing 3 changed files with 108 additions and 1 deletion.
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
@@ -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
@@ -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<SQLQueryExpr> 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
@@ -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 {