From 62586d35d52922ea4d4ddc73e46fdf72998d5d10 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Wed, 14 Oct 2026 13:51:29 -0700 Subject: [PATCH 1/4] [BugFix] Fix empty SQL query failing with internal NullPointerException (#5879) The SQL grammar's root rule (sqlStatement? SEMI? EOF) accepts input with no statement, so an empty, whitespace-only, comment-only or ";" query parsed successfully and AstStatementBuilder returned a null Statement. QueryPlanFactory then dereferenced it, and the response exposed the resulting NullPointerException. Make sqlStatement required in the root rule, as #5868 did for PPL. Such queries now fail in the v2 parser with a SyntaxCheckException, fall back to the legacy engine like any other v2 syntax error, and return its standard 400 syntax error. This also covers the unified-API SqlV2QueryParser, which threw a NullPointerException on the same input. Add sql and sql.explain REST API specs so the YAML REST test can call the SQL endpoints. Signed-off-by: Jialiang Liang --- .../api/parser/UnifiedQueryParserTest.java | 13 ++++ .../opensearch/sql/sql/QueryValidationIT.java | 31 ++++++++++ .../rest-api-spec/api/sql.explain.json | 22 +++++++ .../resources/rest-api-spec/api/sql.json | 27 ++++++++ .../rest-api-spec/test/issues/5879.yml | 62 +++++++++++++++++++ sql/src/main/antlr/OpenSearchSQLParser.g4 | 2 +- .../sql/sql/antlr/SQLSyntaxParserTest.java | 14 +++++ 7 files changed, 170 insertions(+), 1 deletion(-) create mode 100644 integ-test/src/yamlRestTest/resources/rest-api-spec/api/sql.explain.json create mode 100644 integ-test/src/yamlRestTest/resources/rest-api-spec/api/sql.json create mode 100644 integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5879.yml 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..d1386b50ece 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,19 @@ public void testSyntaxErrorThrows() { assertThrows(SyntaxCheckException.class, () -> context.getParser().parse("not a valid query")); } + @Test + public void sqlQueryWithoutStatementThrowsSyntaxError() throws Exception { + try (UnifiedQueryContext sql = + UnifiedQueryContext.builder() + .language(QueryType.SQL) + .catalog(DEFAULT_CATALOG, testSchema) + .build()) { + for (String query : new String[] {"", " ", "/* only a comment */", "-- line", ";"}) { + assertThrows(SyntaxCheckException.class, () -> sql.getParser().parse(query)); + } + } + } + @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..8418e7a9e54 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 @@ -8,6 +8,8 @@ import static org.hamcrest.Matchers.is; import static org.opensearch.core.rest.RestStatus.BAD_REQUEST; import static org.opensearch.sql.legacy.plugin.RestSqlAction.QUERY_API_ENDPOINT; +import static org.opensearch.sql.ppl.PPLIntegTestCase.disableCalcite; +import static org.opensearch.sql.ppl.PPLIntegTestCase.enableCalcite; import static org.opensearch.sql.util.Capability.QUERY_ERROR_MESSAGE; import static org.opensearch.sql.util.MatcherUtils.featureValueOf; @@ -117,6 +119,35 @@ 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 a syntax error with a 400. + @Test + public void testQueryWithoutStatementShouldFailWithBadRequest() throws IOException { + try { + for (boolean calcite : new boolean[] {false, true}) { + if (calcite) { + enableCalcite(); + } else { + disableCalcite(); + } + 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 = + "calcite=" + calcite + ", explain=" + explain + ", query=[" + query + "]"; + assertEquals(context, 400, e.getResponse().getStatusLine().getStatusCode()); + assertFalse(context, e.getMessage().contains("NullPointerException")); + } + } + } + } finally { + disableCalcite(); + } + } + 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..7202e427248 --- /dev/null +++ b/integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/5879.yml @@ -0,0 +1,62 @@ +setup: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled : true + +--- +teardown: + - do: + query.settings: + body: + transient: + plugins.calcite.enabled : null + +--- +"Empty SQL query returns 400 syntax 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 v2 parser now rejects the query, and it falls back to the legacy engine's syntax error. + - do: + catch: bad_request + headers: + Content-Type: 'application/json' + sql: + body: + query: "" + - match: { "$body": "/SQLFeatureNotSupportedException/" } + - match: { "$body": "/Query\\s+must\\s+start\\s+with\\s+SELECT/" } + +--- +"Comment and semicolon only SQL query returns 400 syntax error": + - skip: + features: + - headers + - do: + catch: bad_request + headers: + Content-Type: 'application/json' + sql: + body: + query: " /* nothing here */ ; " + - match: { "$body": "/SQLFeatureNotSupportedException/" } + - match: { "$body": "/Query\\s+must\\s+start\\s+with\\s+SELECT/" } + +--- +"Empty SQL explain returns 400 syntax error": + - skip: + features: + - headers + - do: + catch: bad_request + headers: + Content-Type: 'application/json' + sql.explain: + body: + query: "" + - match: { "$body": "/SQLFeatureNotSupportedException/" } + - match: { "$body": "/Query\\s+must\\s+start\\s+with\\s+SELECT/" } diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index f33a086738e..4f0b7eadafe 100644 --- a/sql/src/main/antlr/OpenSearchSQLParser.g4 +++ b/sql/src/main/antlr/OpenSearchSQLParser.g4 @@ -51,7 +51,7 @@ options { tokenVocab = OpenSearchSQLLexer; } // Root rule root - : sqlStatement? SEMI? EOF + : sqlStatement SEMI? EOF ; // Only SELECT diff --git a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java index c43044508b8..1efeb31a159 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java @@ -8,6 +8,7 @@ import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Streams; @@ -34,6 +35,19 @@ public void canParseQueryEndWithSemiColon() { assertNotNull(parser.parse("SELECT 123;")); } + @Test + public void queryWithoutStatementShouldFail() { + for (String query : new String[] {"", " \n\t ", "/* only a comment */", "-- line", ";"}) { + SyntaxCheckException e = assertThrows(SyntaxCheckException.class, () -> parser.parse(query)); + assertTrue(e.getMessage().contains("is not a valid term"), e.getMessage()); + } + } + + @Test + public void canParseQueryWithSurroundingCommentsAndSemiColon() { + assertNotNull(parser.parse("/* leading */ SELECT 123 -- trailing\n;")); + } + @Test public void canParseSelectLiterals() { assertNotNull(parser.parse("SELECT 123, 'hello'")); From 403a3adf59559c785f05c44056fe98d8930e7a61 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Wed, 14 Oct 2026 14:06:10 -0700 Subject: [PATCH 2/4] Remove Calcite toggling from SQL empty-query tests The Calcite engine only applies to PPL: QueryService.shouldUseCalcite returns isCalciteEnabled(settings) && queryType == QueryType.PPL. An empty SQL query is also rejected by the parser before QueryService is reached, so toggling plugins.calcite.enabled had no effect on these tests. Signed-off-by: Jialiang Liang --- .../opensearch/sql/sql/QueryValidationIT.java | 32 ++++++------------- .../rest-api-spec/test/issues/5879.yml | 16 ---------- 2 files changed, 9 insertions(+), 39 deletions(-) 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 8418e7a9e54..7cd9b6c19e1 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 @@ -8,8 +8,6 @@ import static org.hamcrest.Matchers.is; import static org.opensearch.core.rest.RestStatus.BAD_REQUEST; import static org.opensearch.sql.legacy.plugin.RestSqlAction.QUERY_API_ENDPOINT; -import static org.opensearch.sql.ppl.PPLIntegTestCase.disableCalcite; -import static org.opensearch.sql.ppl.PPLIntegTestCase.enableCalcite; import static org.opensearch.sql.util.Capability.QUERY_ERROR_MESSAGE; import static org.opensearch.sql.util.MatcherUtils.featureValueOf; @@ -123,28 +121,16 @@ public void testSqlQueryWithArrayPayloadShouldFailWithBadRequest() throws IOExce // NullPointerException. It must be rejected as a syntax error with a 400. @Test public void testQueryWithoutStatementShouldFailWithBadRequest() throws IOException { - try { - for (boolean calcite : new boolean[] {false, true}) { - if (calcite) { - enableCalcite(); - } else { - disableCalcite(); - } - 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 = - "calcite=" + calcite + ", explain=" + explain + ", query=[" + query + "]"; - assertEquals(context, 400, e.getResponse().getStatusLine().getStatusCode()); - assertFalse(context, e.getMessage().contains("NullPointerException")); - } - } + 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()); + assertFalse(context, e.getMessage().contains("NullPointerException")); } - } finally { - disableCalcite(); } } 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 index 7202e427248..42956a5f50e 100644 --- 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 @@ -1,19 +1,3 @@ -setup: - - do: - query.settings: - body: - transient: - plugins.calcite.enabled : true - ---- -teardown: - - do: - query.settings: - body: - transient: - plugins.calcite.enabled : null - ---- "Empty SQL query returns 400 syntax error": - skip: features: From bc88f54080ffc4d4ec430d65d70d798725b77255 Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Wed, 14 Oct 2026 14:20:34 -0700 Subject: [PATCH 3/4] Do not fall back to the legacy engine for an empty SQL query A query with no statement (empty, whitespace, comments or semicolons only) cannot run on the legacy engine either, so falling back only replaced the v2 parser's SyntaxCheckException with a misleading legacy error. Detect such input with the SQL lexer and return the v2 syntax error (400), as PPL does. Signed-off-by: Jialiang Liang --- .../opensearch/sql/sql/QueryValidationIT.java | 6 ++- .../rest-api-spec/test/issues/5879.yml | 15 +++--- .../sql/legacy/plugin/RestSQLQueryAction.java | 14 ++++- .../legacy/plugin/RestSQLQueryActionTest.java | 51 +++++++++++++++++++ .../sql/sql/antlr/SQLSyntaxParser.java | 23 ++++++++- .../sql/sql/antlr/SQLSyntaxParserTest.java | 16 ++++++ 6 files changed, 114 insertions(+), 11 deletions(-) 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 7cd9b6c19e1..de1ac67ddc4 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 @@ -118,7 +118,8 @@ public void testSqlQueryWithArrayPayloadShouldFailWithBadRequest() throws IOExce } // A query with no statement used to parse successfully and fail later with an internal - // NullPointerException. It must be rejected as a syntax error with a 400. + // NullPointerException. It must be rejected by the v2 parser with a 400 syntax error, without + // falling back to the legacy engine. @Test public void testQueryWithoutStatementShouldFailWithBadRequest() throws IOException { for (String query : new String[] {"", " ", "/* only a comment */", "-- line", ";"}) { @@ -129,6 +130,9 @@ public void testQueryWithoutStatementShouldFailWithBadRequest() throws IOExcepti assertThrows(ResponseException.class, () -> client().performRequest(request)); String context = "explain=" + explain + ", query=[" + query + "]"; assertEquals(context, 400, e.getResponse().getStatusLine().getStatusCode()); + assertTrue(context, e.getMessage().contains("SyntaxCheckException")); + String offendingToken = query.equals(";") ? "[;]" : "[]"; + assertTrue(context, e.getMessage().contains(offendingToken + " is not a valid term")); assertFalse(context, e.getMessage().contains("NullPointerException")); } } 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 index 42956a5f50e..c1d40839ac4 100644 --- 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 @@ -4,7 +4,8 @@ - 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 v2 parser now rejects the query, and it falls back to the legacy engine's syntax error. + # The v2 parser now rejects the query with a syntax error, and it does not fall back to the + # legacy engine. - do: catch: bad_request headers: @@ -12,8 +13,8 @@ sql: body: query: "" - - match: { "$body": "/SQLFeatureNotSupportedException/" } - - match: { "$body": "/Query\\s+must\\s+start\\s+with\\s+SELECT/" } + - match: { "$body": "/SyntaxCheckException/" } + - match: { "$body": "/\\[\\]\\s+is\\s+not\\s+a\\s+valid\\s+term/" } --- "Comment and semicolon only SQL query returns 400 syntax error": @@ -27,8 +28,8 @@ sql: body: query: " /* nothing here */ ; " - - match: { "$body": "/SQLFeatureNotSupportedException/" } - - match: { "$body": "/Query\\s+must\\s+start\\s+with\\s+SELECT/" } + - match: { "$body": "/SyntaxCheckException/" } + - match: { "$body": "/\\[;\\]\\s+is\\s+not\\s+a\\s+valid\\s+term/" } --- "Empty SQL explain returns 400 syntax error": @@ -42,5 +43,5 @@ sql.explain: body: query: "" - - match: { "$body": "/SQLFeatureNotSupportedException/" } - - match: { "$body": "/Query\\s+must\\s+start\\s+with\\s+SELECT/" } + - match: { "$body": "/SyntaxCheckException/" } + - match: { "$body": "/\\[\\]\\s+is\\s+not\\s+a\\s+valid\\s+term/" } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java index 21badf79412..e0d0b6305a6 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -35,6 +35,7 @@ import org.opensearch.sql.protocol.response.format.RawResponseFormatter; import org.opensearch.sql.protocol.response.format.ResponseFormatter; import org.opensearch.sql.sql.SQLService; +import org.opensearch.sql.sql.antlr.SQLSyntaxParser; import org.opensearch.sql.sql.domain.SQLQueryRequest; import org.opensearch.transport.client.node.NodeClient; @@ -99,6 +100,7 @@ public RestChannelConsumer prepareRequest( request, fallBackListener( channel, + request, createExplainResponseListener(channel, executionErrorHandler), fallbackHandler)); } @@ -109,10 +111,12 @@ public RestChannelConsumer prepareRequest( request, fallBackListener( channel, + request, createQueryResponseListener(channel, request, executionErrorHandler), fallbackHandler), fallBackListener( channel, + request, createExplainResponseListener(channel, executionErrorHandler), fallbackHandler)); } @@ -120,6 +124,7 @@ public RestChannelConsumer prepareRequest( private ResponseListener fallBackListener( RestChannel channel, + SQLQueryRequest request, ResponseListener next, BiConsumer fallBackHandler) { return new ResponseListener() { @@ -131,7 +136,10 @@ public void onResponse(T response) { @Override public void onFailure(Exception e) { - if (e instanceof SyntaxCheckException || e instanceof UnsupportedCursorRequestException) { + // A query without a statement cannot run on the legacy engine either, so falling back + // would only replace the syntax error with a misleading one. + if ((e instanceof SyntaxCheckException && !hasNoStatement(request)) + || e instanceof UnsupportedCursorRequestException) { fallBackHandler.accept(channel, e); } else { next.onFailure(e); @@ -140,6 +148,10 @@ public void onFailure(Exception e) { }; } + private static boolean hasNoStatement(SQLQueryRequest request) { + return request.getQuery() != null && SQLSyntaxParser.hasNoStatement(request.getQuery()); + } + private ResponseListener createExplainResponseListener( RestChannel channel, BiConsumer errorHandler) { return new ResponseListener<>() { diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSQLQueryActionTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSQLQueryActionTest.java index 9ce1ac8f3a4..2c963e1dd71 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSQLQueryActionTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSQLQueryActionTest.java @@ -137,6 +137,57 @@ public void queryThatNotSupportIsHandledByFallbackHandler() throws Exception { assertTrue(fallback.get()); } + @Test + public void queryWithoutStatementIsHandledByExecutionErrorHandler() throws Exception { + for (String path : new String[] {QUERY_API_ENDPOINT, EXPLAIN_API_ENDPOINT}) { + SQLQueryRequest request = + new SQLQueryRequest( + new JSONObject("{\"query\": \" /* c */ ; \"}"), + " /* c */ ; ", + path, + path.equals(QUERY_API_ENDPOINT) ? "jdbc" : "standard"); + + AtomicBoolean executionErrorHandler = new AtomicBoolean(false); + RestSQLQueryAction queryAction = new RestSQLQueryAction(injector); + queryAction + .prepareRequest( + request, + (channel, exception) -> { + fail(); + }, + (channel, exception) -> { + executionErrorHandler.set(true); + assertTrue(exception instanceof SyntaxCheckException); + }) + .accept(restChannel); + + assertTrue(path, executionErrorHandler.get()); + } + } + + @Test + public void invalidQueryWithStatementIsHandledByFallbackHandler() throws Exception { + SQLQueryRequest request = + new SQLQueryRequest( + new JSONObject("{\"query\": \"SELEC 1\"}"), "SELEC 1", QUERY_API_ENDPOINT, "jdbc"); + + AtomicBoolean fallback = new AtomicBoolean(false); + RestSQLQueryAction queryAction = new RestSQLQueryAction(injector); + queryAction + .prepareRequest( + request, + (channel, exception) -> { + fallback.set(true); + assertTrue(exception instanceof SyntaxCheckException); + }, + (channel, exception) -> { + fail(); + }) + .accept(restChannel); + + assertTrue(fallback.get()); + } + @Test public void queryExecutionFailedIsHandledByExecutionErrorHandler() throws Exception { SQLQueryRequest request = diff --git a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java index d1a6adc236a..dfb58ee3c59 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java +++ b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java @@ -6,6 +6,7 @@ package org.opensearch.sql.sql.antlr; import org.antlr.v4.runtime.CommonTokenStream; +import org.antlr.v4.runtime.Token; import org.antlr.v4.runtime.tree.ParseTree; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -29,8 +30,7 @@ public class SQLSyntaxParser implements Parser { public ParseTree parse(String query) { AnonymizerListener anonymizer = new AnonymizerListener(); - OpenSearchSQLLexer lexer = new OpenSearchSQLLexer(new CaseInsensitiveCharStream(query)); - OpenSearchSQLParser parser = new OpenSearchSQLParser(new CommonTokenStream(lexer)); + OpenSearchSQLParser parser = new OpenSearchSQLParser(new CommonTokenStream(createLexer(query))); parser.addErrorListener(new SyntaxAnalysisErrorListener()); parser.addParseListener(anonymizer); @@ -39,4 +39,23 @@ public ParseTree parse(String query) { return parseTree; } + + /** + * Check if a SQL query has no statement, i.e. the parser sees nothing in it but semicolons. + * Whitespace and comments are skipped by the lexer the same way as in {@link #parse(String)}. + * + * @param query a SQL query + * @return true if the query has no statement + */ + public static boolean hasNoStatement(String query) { + return createLexer(query).getAllTokens().stream() + .allMatch( + token -> + token.getChannel() != Token.DEFAULT_CHANNEL + || token.getType() == OpenSearchSQLLexer.SEMI); + } + + private static OpenSearchSQLLexer createLexer(String query) { + return new OpenSearchSQLLexer(new CaseInsensitiveCharStream(query)); + } } diff --git a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java index 1efeb31a159..73b81c3996c 100644 --- a/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java +++ b/sql/src/test/java/org/opensearch/sql/sql/antlr/SQLSyntaxParserTest.java @@ -6,6 +6,7 @@ package org.opensearch.sql.sql.antlr; import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -43,6 +44,21 @@ public void queryWithoutStatementShouldFail() { } } + @Test + public void queryWithOnlyWhitespaceCommentsOrSemiColonsHasNoStatement() { + for (String query : + new String[] {"", " \n\t ", "/* only a comment */", "-- line", ";", " ; "}) { + assertTrue(SQLSyntaxParser.hasNoStatement(query), "[" + query + "]"); + } + } + + @Test + public void queryWithAnyOtherTokenHasStatement() { + for (String query : new String[] {"SELECT 1", "/* c */ SELECT 1;", "SELEC 1"}) { + assertFalse(SQLSyntaxParser.hasNoStatement(query), "[" + query + "]"); + } + } + @Test public void canParseQueryWithSurroundingCommentsAndSemiColon() { assertNotNull(parser.parse("/* leading */ SELECT 123 -- trailing\n;")); From 4ff7fa81a1c58ccf3ca903c9bf37921358b272cb Mon Sep 17 00:00:00 2001 From: Jialiang Liang Date: Wed, 14 Oct 2026 15:15:13 -0700 Subject: [PATCH 4/4] Reject an empty SQL query in AstStatementBuilder instead of the grammar Follow review feedback: keep the optional statement in the SQL root rule and reject a root with no statement in AstStatementBuilder.visitRoot with IllegalArgumentException("Query is empty"). IllegalArgumentException is classified as a client error, so the request returns 400, and it is not a SyntaxCheckException, so it does not fall back to the legacy engine. This removes the grammar change, SQLSyntaxParser.hasNoStatement and the fallback listener change, along with their tests. The api module's SqlV2QueryParser uses the same AstStatementBuilder, so it now throws the same exception instead of a NullPointerException. Signed-off-by: Jialiang Liang --- .../api/parser/UnifiedQueryParserTest.java | 6 ++- .../opensearch/sql/sql/QueryValidationIT.java | 9 ++-- .../rest-api-spec/test/issues/5879.yml | 24 +++++---- .../sql/legacy/plugin/RestSQLQueryAction.java | 14 +---- .../legacy/plugin/RestSQLQueryActionTest.java | 51 ------------------- sql/src/main/antlr/OpenSearchSQLParser.g4 | 2 +- .../sql/sql/antlr/SQLSyntaxParser.java | 23 +-------- .../sql/sql/parser/AstStatementBuilder.java | 8 +++ .../sql/sql/antlr/SQLSyntaxParserTest.java | 30 ----------- .../sql/parser/AstStatementBuilderTest.java | 50 ++++++++++++++++++ 10 files changed, 83 insertions(+), 134 deletions(-) create mode 100644 sql/src/test/java/org/opensearch/sql/sql/parser/AstStatementBuilderTest.java 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 d1386b50ece..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 @@ -84,14 +84,16 @@ public void testSyntaxErrorThrows() { } @Test - public void sqlQueryWithoutStatementThrowsSyntaxError() throws Exception { + 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", ";"}) { - assertThrows(SyntaxCheckException.class, () -> sql.getParser().parse(query)); + IllegalArgumentException e = + assertThrows(IllegalArgumentException.class, () -> sql.getParser().parse(query)); + assertEquals("Query is empty", e.getMessage()); } } } 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 de1ac67ddc4..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 @@ -118,8 +118,8 @@ public void testSqlQueryWithArrayPayloadShouldFailWithBadRequest() throws IOExce } // A query with no statement used to parse successfully and fail later with an internal - // NullPointerException. It must be rejected by the v2 parser with a 400 syntax error, without - // falling back to the legacy engine. + // 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", ";"}) { @@ -130,9 +130,8 @@ public void testQueryWithoutStatementShouldFailWithBadRequest() throws IOExcepti assertThrows(ResponseException.class, () -> client().performRequest(request)); String context = "explain=" + explain + ", query=[" + query + "]"; assertEquals(context, 400, e.getResponse().getStatusLine().getStatusCode()); - assertTrue(context, e.getMessage().contains("SyntaxCheckException")); - String offendingToken = query.equals(";") ? "[;]" : "[]"; - assertTrue(context, e.getMessage().contains(offendingToken + " is not a valid term")); + assertTrue(context, e.getMessage().contains("IllegalArgumentException")); + assertTrue(context, e.getMessage().contains("Query is empty")); assertFalse(context, e.getMessage().contains("NullPointerException")); } } 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 index c1d40839ac4..e40865faceb 100644 --- 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 @@ -1,11 +1,10 @@ -"Empty SQL query returns 400 syntax error": +"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 v2 parser now rejects the query with a syntax error, and it does not fall back to the - # legacy engine. + # The query is now rejected as empty, and it does not fall back to the legacy engine. - do: catch: bad_request headers: @@ -13,11 +12,12 @@ sql: body: query: "" - - match: { "$body": "/SyntaxCheckException/" } - - match: { "$body": "/\\[\\]\\s+is\\s+not\\s+a\\s+valid\\s+term/" } + - match: { "$body": "/IllegalArgumentException/" } + - match: { "$body": "/Query\\s+is\\s+empty/" } + - match: { "$body": "/^(?s)(?!.*NullPointerException)/" } --- -"Comment and semicolon only SQL query returns 400 syntax error": +"Comment and semicolon only SQL query returns 400 empty query error": - skip: features: - headers @@ -28,11 +28,12 @@ sql: body: query: " /* nothing here */ ; " - - match: { "$body": "/SyntaxCheckException/" } - - match: { "$body": "/\\[;\\]\\s+is\\s+not\\s+a\\s+valid\\s+term/" } + - match: { "$body": "/IllegalArgumentException/" } + - match: { "$body": "/Query\\s+is\\s+empty/" } + - match: { "$body": "/^(?s)(?!.*NullPointerException)/" } --- -"Empty SQL explain returns 400 syntax error": +"Empty SQL explain returns 400 empty query error": - skip: features: - headers @@ -43,5 +44,6 @@ sql.explain: body: query: "" - - match: { "$body": "/SyntaxCheckException/" } - - match: { "$body": "/\\[\\]\\s+is\\s+not\\s+a\\s+valid\\s+term/" } + - match: { "$body": "/IllegalArgumentException/" } + - match: { "$body": "/Query\\s+is\\s+empty/" } + - match: { "$body": "/^(?s)(?!.*NullPointerException)/" } diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java index e0d0b6305a6..21badf79412 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -35,7 +35,6 @@ import org.opensearch.sql.protocol.response.format.RawResponseFormatter; import org.opensearch.sql.protocol.response.format.ResponseFormatter; import org.opensearch.sql.sql.SQLService; -import org.opensearch.sql.sql.antlr.SQLSyntaxParser; import org.opensearch.sql.sql.domain.SQLQueryRequest; import org.opensearch.transport.client.node.NodeClient; @@ -100,7 +99,6 @@ public RestChannelConsumer prepareRequest( request, fallBackListener( channel, - request, createExplainResponseListener(channel, executionErrorHandler), fallbackHandler)); } @@ -111,12 +109,10 @@ public RestChannelConsumer prepareRequest( request, fallBackListener( channel, - request, createQueryResponseListener(channel, request, executionErrorHandler), fallbackHandler), fallBackListener( channel, - request, createExplainResponseListener(channel, executionErrorHandler), fallbackHandler)); } @@ -124,7 +120,6 @@ public RestChannelConsumer prepareRequest( private ResponseListener fallBackListener( RestChannel channel, - SQLQueryRequest request, ResponseListener next, BiConsumer fallBackHandler) { return new ResponseListener() { @@ -136,10 +131,7 @@ public void onResponse(T response) { @Override public void onFailure(Exception e) { - // A query without a statement cannot run on the legacy engine either, so falling back - // would only replace the syntax error with a misleading one. - if ((e instanceof SyntaxCheckException && !hasNoStatement(request)) - || e instanceof UnsupportedCursorRequestException) { + if (e instanceof SyntaxCheckException || e instanceof UnsupportedCursorRequestException) { fallBackHandler.accept(channel, e); } else { next.onFailure(e); @@ -148,10 +140,6 @@ public void onFailure(Exception e) { }; } - private static boolean hasNoStatement(SQLQueryRequest request) { - return request.getQuery() != null && SQLSyntaxParser.hasNoStatement(request.getQuery()); - } - private ResponseListener createExplainResponseListener( RestChannel channel, BiConsumer errorHandler) { return new ResponseListener<>() { diff --git a/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSQLQueryActionTest.java b/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSQLQueryActionTest.java index 2c963e1dd71..9ce1ac8f3a4 100644 --- a/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSQLQueryActionTest.java +++ b/legacy/src/test/java/org/opensearch/sql/legacy/plugin/RestSQLQueryActionTest.java @@ -137,57 +137,6 @@ public void queryThatNotSupportIsHandledByFallbackHandler() throws Exception { assertTrue(fallback.get()); } - @Test - public void queryWithoutStatementIsHandledByExecutionErrorHandler() throws Exception { - for (String path : new String[] {QUERY_API_ENDPOINT, EXPLAIN_API_ENDPOINT}) { - SQLQueryRequest request = - new SQLQueryRequest( - new JSONObject("{\"query\": \" /* c */ ; \"}"), - " /* c */ ; ", - path, - path.equals(QUERY_API_ENDPOINT) ? "jdbc" : "standard"); - - AtomicBoolean executionErrorHandler = new AtomicBoolean(false); - RestSQLQueryAction queryAction = new RestSQLQueryAction(injector); - queryAction - .prepareRequest( - request, - (channel, exception) -> { - fail(); - }, - (channel, exception) -> { - executionErrorHandler.set(true); - assertTrue(exception instanceof SyntaxCheckException); - }) - .accept(restChannel); - - assertTrue(path, executionErrorHandler.get()); - } - } - - @Test - public void invalidQueryWithStatementIsHandledByFallbackHandler() throws Exception { - SQLQueryRequest request = - new SQLQueryRequest( - new JSONObject("{\"query\": \"SELEC 1\"}"), "SELEC 1", QUERY_API_ENDPOINT, "jdbc"); - - AtomicBoolean fallback = new AtomicBoolean(false); - RestSQLQueryAction queryAction = new RestSQLQueryAction(injector); - queryAction - .prepareRequest( - request, - (channel, exception) -> { - fallback.set(true); - assertTrue(exception instanceof SyntaxCheckException); - }, - (channel, exception) -> { - fail(); - }) - .accept(restChannel); - - assertTrue(fallback.get()); - } - @Test public void queryExecutionFailedIsHandledByExecutionErrorHandler() throws Exception { SQLQueryRequest request = diff --git a/sql/src/main/antlr/OpenSearchSQLParser.g4 b/sql/src/main/antlr/OpenSearchSQLParser.g4 index 4f0b7eadafe..f33a086738e 100644 --- a/sql/src/main/antlr/OpenSearchSQLParser.g4 +++ b/sql/src/main/antlr/OpenSearchSQLParser.g4 @@ -51,7 +51,7 @@ options { tokenVocab = OpenSearchSQLLexer; } // Root rule root - : sqlStatement SEMI? EOF + : sqlStatement? SEMI? EOF ; // Only SELECT diff --git a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java index dfb58ee3c59..d1a6adc236a 100644 --- a/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java +++ b/sql/src/main/java/org/opensearch/sql/sql/antlr/SQLSyntaxParser.java @@ -6,7 +6,6 @@ package org.opensearch.sql.sql.antlr; import org.antlr.v4.runtime.CommonTokenStream; -import org.antlr.v4.runtime.Token; import org.antlr.v4.runtime.tree.ParseTree; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -30,7 +29,8 @@ public class SQLSyntaxParser implements Parser { public ParseTree parse(String query) { AnonymizerListener anonymizer = new AnonymizerListener(); - OpenSearchSQLParser parser = new OpenSearchSQLParser(new CommonTokenStream(createLexer(query))); + OpenSearchSQLLexer lexer = new OpenSearchSQLLexer(new CaseInsensitiveCharStream(query)); + OpenSearchSQLParser parser = new OpenSearchSQLParser(new CommonTokenStream(lexer)); parser.addErrorListener(new SyntaxAnalysisErrorListener()); parser.addParseListener(anonymizer); @@ -39,23 +39,4 @@ public ParseTree parse(String query) { return parseTree; } - - /** - * Check if a SQL query has no statement, i.e. the parser sees nothing in it but semicolons. - * Whitespace and comments are skipped by the lexer the same way as in {@link #parse(String)}. - * - * @param query a SQL query - * @return true if the query has no statement - */ - public static boolean hasNoStatement(String query) { - return createLexer(query).getAllTokens().stream() - .allMatch( - token -> - token.getChannel() != Token.DEFAULT_CHANNEL - || token.getType() == OpenSearchSQLLexer.SEMI); - } - - private static OpenSearchSQLLexer createLexer(String query) { - return new OpenSearchSQLLexer(new CaseInsensitiveCharStream(query)); - } } 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 parser.parse(query)); - assertTrue(e.getMessage().contains("is not a valid term"), e.getMessage()); - } - } - - @Test - public void queryWithOnlyWhitespaceCommentsOrSemiColonsHasNoStatement() { - for (String query : - new String[] {"", " \n\t ", "/* only a comment */", "-- line", ";", " ; "}) { - assertTrue(SQLSyntaxParser.hasNoStatement(query), "[" + query + "]"); - } - } - - @Test - public void queryWithAnyOtherTokenHasStatement() { - for (String query : new String[] {"SELECT 1", "/* c */ SELECT 1;", "SELEC 1"}) { - assertFalse(SQLSyntaxParser.hasNoStatement(query), "[" + query + "]"); - } - } - - @Test - public void canParseQueryWithSurroundingCommentsAndSemiColon() { - assertNotNull(parser.parse("/* leading */ SELECT 123 -- trailing\n;")); - } - @Test public void canParseSelectLiterals() { assertNotNull(parser.parse("SELECT 123, 'hello'")); diff --git a/sql/src/test/java/org/opensearch/sql/sql/parser/AstStatementBuilderTest.java b/sql/src/test/java/org/opensearch/sql/sql/parser/AstStatementBuilderTest.java new file mode 100644 index 00000000000..a7080dc80ee --- /dev/null +++ b/sql/src/test/java/org/opensearch/sql/sql/parser/AstStatementBuilderTest.java @@ -0,0 +1,50 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.opensearch.sql.sql.parser; + +import static java.util.Collections.emptyList; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.opensearch.sql.ast.dsl.AstDSL.alias; +import static org.opensearch.sql.ast.dsl.AstDSL.intLiteral; +import static org.opensearch.sql.ast.dsl.AstDSL.project; +import static org.opensearch.sql.ast.dsl.AstDSL.values; +import static org.opensearch.sql.executor.QueryType.SQL; + +import org.junit.jupiter.api.Test; +import org.opensearch.sql.ast.statement.Query; +import org.opensearch.sql.ast.statement.Statement; +import org.opensearch.sql.sql.antlr.SQLSyntaxParser; + +class AstStatementBuilderTest { + + private final SQLSyntaxParser parser = new SQLSyntaxParser(); + + @Test + public void queryWithoutStatementShouldFail() { + for (String query : new String[] {"", " \n\t ", "/* only a comment */", "-- line", ";"}) { + IllegalArgumentException e = + assertThrows(IllegalArgumentException.class, () -> 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())); + } +}