diff --git a/api/src/test/java/org/opensearch/sql/api/parser/UnifiedQueryParserTest.java b/api/src/test/java/org/opensearch/sql/api/parser/UnifiedQueryParserTest.java index bb06e7c7b8b..9304489a520 100644 --- a/api/src/test/java/org/opensearch/sql/api/parser/UnifiedQueryParserTest.java +++ b/api/src/test/java/org/opensearch/sql/api/parser/UnifiedQueryParserTest.java @@ -83,6 +83,21 @@ public void testSyntaxErrorThrows() { assertThrows(SyntaxCheckException.class, () -> context.getParser().parse("not a valid query")); } + @Test + public void sqlQueryWithoutStatementThrowsEmptyQueryError() throws Exception { + try (UnifiedQueryContext sql = + UnifiedQueryContext.builder() + .language(QueryType.SQL) + .catalog(DEFAULT_CATALOG, testSchema) + .build()) { + for (String query : new String[] {"", " ", "/* only a comment */", "-- line", ";"}) { + IllegalArgumentException e = + assertThrows(IllegalArgumentException.class, () -> sql.getParser().parse(query)); + assertEquals("Query is empty", e.getMessage()); + } + } + } + @Test public void deeplyNestedExpressionShouldBeRejected() throws Exception { String key = Settings.Key.MAX_EXPRESSION_DEPTH.getKeyValue(); diff --git a/integ-test/src/test/java/org/opensearch/sql/sql/QueryValidationIT.java b/integ-test/src/test/java/org/opensearch/sql/sql/QueryValidationIT.java index e814252cfe7..6d2c58c92ef 100644 --- a/integ-test/src/test/java/org/opensearch/sql/sql/QueryValidationIT.java +++ b/integ-test/src/test/java/org/opensearch/sql/sql/QueryValidationIT.java @@ -117,6 +117,26 @@ public void testSqlQueryWithArrayPayloadShouldFailWithBadRequest() throws IOExce whenExecuteMalformedPayload(); } + // A query with no statement used to parse successfully and fail later with an internal + // NullPointerException. It must be rejected as an empty query with a 400, without falling back + // to the legacy engine. + @Test + public void testQueryWithoutStatementShouldFailWithBadRequest() throws IOException { + for (String query : new String[] {"", " ", "/* only a comment */", "-- line", ";"}) { + for (boolean explain : new boolean[] {false, true}) { + Request request = + getSqlRequest(String.format(Locale.ROOT, "{\"query\": \"%s\"}", query), explain); + ResponseException e = + assertThrows(ResponseException.class, () -> client().performRequest(request)); + String context = "explain=" + explain + ", query=[" + query + "]"; + assertEquals(context, 400, e.getResponse().getStatusLine().getStatusCode()); + assertTrue(context, e.getMessage().contains("IllegalArgumentException")); + assertTrue(context, e.getMessage().contains("Query is empty")); + assertFalse(context, e.getMessage().contains("NullPointerException")); + } + } + } + private static void whenExecuteMalformedPayload() throws IOException { Request request = new Request("POST", QUERY_API_ENDPOINT); request.setJsonEntity("{ \"query\": [\"SELECT * FROM opensearch-sql_test_index_account\"] }"); diff --git a/integ-test/src/yamlRestTest/resources/rest-api-spec/api/sql.explain.json b/integ-test/src/yamlRestTest/resources/rest-api-spec/api/sql.explain.json new file mode 100644 index 00000000000..3fa3ab5788b --- /dev/null +++ b/integ-test/src/yamlRestTest/resources/rest-api-spec/api/sql.explain.json @@ -0,0 +1,22 @@ +{ + "sql.explain": { + "documentation": { + "url": "https://github.com/opensearch-project/sql/blob/main/docs/user/index.rst", + "description": "OpenSearch SQL Reference Manual" + }, + "stability" : "stable", + "url": { + "paths": [ + { + "path" : "/_plugins/_sql/_explain", + "methods" : ["POST"] + } + ] + }, + "params": {}, + "body": { + "description": "SQL Explain Query", + "required":true + } + } +} diff --git a/integ-test/src/yamlRestTest/resources/rest-api-spec/api/sql.json b/integ-test/src/yamlRestTest/resources/rest-api-spec/api/sql.json new file mode 100644 index 00000000000..3a52e991448 --- /dev/null +++ b/integ-test/src/yamlRestTest/resources/rest-api-spec/api/sql.json @@ -0,0 +1,27 @@ +{ + "sql": { + "documentation": { + "url": "https://github.com/opensearch-project/sql/blob/main/docs/user/index.rst", + "description": "OpenSearch SQL Reference Manual" + }, + "stability" : "stable", + "url": { + "paths": [ + { + "path" : "/_plugins/_sql", + "methods" : ["POST"] + } + ] + }, + "params": { + "format":{ + "type":"string", + "description":"response format: jdbc, csv, raw" + } + }, + "body": { + "description": "SQL Query", + "required":true + } + } +} diff --git a/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5879.yml b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5879.yml new file mode 100644 index 00000000000..e40865faceb --- /dev/null +++ b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5879.yml @@ -0,0 +1,49 @@ +"Empty SQL query returns 400 empty query error": + - skip: + features: + - headers + # Before the fix the empty statement built a null AST and failed with an internal + # NullPointerException. catch: bad_request asserts the HTTP status code is exactly 400. + # The query is now rejected as empty, and it does not fall back to the legacy engine. + - do: + catch: bad_request + headers: + Content-Type: 'application/json' + sql: + body: + query: "" + - match: { "$body": "/IllegalArgumentException/" } + - match: { "$body": "/Query\\s+is\\s+empty/" } + - match: { "$body": "/^(?s)(?!.*NullPointerException)/" } + +--- +"Comment and semicolon only SQL query returns 400 empty query error": + - skip: + features: + - headers + - do: + catch: bad_request + headers: + Content-Type: 'application/json' + sql: + body: + query: " /* nothing here */ ; " + - match: { "$body": "/IllegalArgumentException/" } + - match: { "$body": "/Query\\s+is\\s+empty/" } + - match: { "$body": "/^(?s)(?!.*NullPointerException)/" } + +--- +"Empty SQL explain returns 400 empty query error": + - skip: + features: + - headers + - do: + catch: bad_request + headers: + Content-Type: 'application/json' + sql.explain: + body: + query: "" + - match: { "$body": "/IllegalArgumentException/" } + - match: { "$body": "/Query\\s+is\\s+empty/" } + - match: { "$body": "/^(?s)(?!.*NullPointerException)/" } diff --git a/sql/src/main/java/org/opensearch/sql/sql/parser/AstStatementBuilder.java b/sql/src/main/java/org/opensearch/sql/sql/parser/AstStatementBuilder.java index dfb045f345a..0b9391c04fb 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/parser/AstStatementBuilder.java +++ b/sql/src/main/java/org/opensearch/sql/sql/parser/AstStatementBuilder.java @@ -23,6 +23,14 @@ public class AstStatementBuilder extends OpenSearchSQLParserBaseVisitor buildStatement(query)); + assertEquals("Query is empty", e.getMessage(), "[" + query + "]"); + } + } + + @Test + public void canBuildStatementWithSurroundingCommentsAndSemiColon() { + assertEquals( + new Query(project(values(emptyList()), alias("123", intLiteral(123))), 0, SQL), + buildStatement("/* leading */ SELECT 123 -- trailing\n;")); + } + + private Statement buildStatement(String query) { + return parser + .parse(query) + .accept( + new AstStatementBuilder( + new AstBuilder(query), + AstStatementBuilder.StatementBuilderContext.builder().build())); + } +}