Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error handling for some more edge cases #3080

Merged
merged 8 commits into from
Oct 23, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -86,6 +87,13 @@ public static QueryAction create(Client client, QueryActionRequest request)

switch (getFirstWord(sql)) {
case "SELECT":
SQLExpr rawExpr = toSqlExpr(sql);
Swiddis marked this conversation as resolved.
Show resolved Hide resolved
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) toSqlExpr(sql);

RewriteRuleExecutor<SQLQueryExpr> ruleExecutor =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -122,7 +123,23 @@ public boolean visit(SQLIdentifierExpr expr) {
String fullFieldName = arr[1];

String index = curScope().getAliases().get(alias);
if (index == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see issue related. Could you elaborate what's the current behavior without the changes? From what I see here, we try to find the index corresponding to an alias. If not found, does it mean the alias may be wrong?

Copy link
Collaborator Author

@Swiddis Swiddis Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that it doesn't get caught as a specific error so the response is unclear/unactionable -- when you pass invalid indices, this index not being found propagates up as a generic internal server error

OpenSearchException[Internal error occurred while processing request]
For more details, please send request for Json format to see the raw response from OpenSearch engine.

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 {
Expand Down
Loading