[BugFix] Fix empty PPL query returning 500 NullPointerException instead of 400 (#5867)

[RyanL1997]

Description

An empty PPL query ({"query": ""}), or one that is only whitespace or comments, returned a 500 with NullPointerException: ... because "plan" is null.

Root cause: the grammar's root : pplStatement? EOF rule accepts input with no statement, so parsing succeeds and AstStatementBuilder returns a null Statement. PPLQueryDataAnonymizer.anonymizeStatement(null) then dereferences it. NPE isn't classified as a client error, so it maps to 500.

Fix: AstStatementBuilder.visitRoot now throws SyntaxCheckException("Query is empty") when the root has no pplStatement. SyntaxCheckException is already mapped to 400. Because the check sits at the parse-tree-to-statement boundary, it covers execute, explain and analyze in PPLService, and the unified-API PPLQueryParser. The grammar is unchanged.

After the fix:

{"error": {"reason": "Invalid Query", "details": "Query is empty", "type": "SyntaxCheckException"}, "status": 400}

Related Issues

Resolves #5867

Check List

• New functionality includes testing
• Commits signed per DCO (-s)
• spotlessCheck passed
• Unit tests passed
• Integration tests passed

[RyanL1997]

Decision Log

Root Cause: root : pplStatement? EOF matches input with no statement: empty, whitespace-only, or only comments (comments go to the HIDDEN channel). AstStatementBuilder then returns null, and PPLQueryDataAnonymizer.anonymizeStatement(null) throws an NPE, which SyncErrorReportRenderer maps to 500. I confirmed this two ways. New PPLServiceTest cases failed with the exact NPE from the issue. On a test cluster, QueryAnalysisIT got expected:<400> but was:<500> with the fix reverted.

Approach: Override visitRoot in the PPL AstStatementBuilder and throw SyntaxCheckException("Query is empty") when ctx.pplStatement() == null. This uses the existing exception that already maps to 400, the same one SyntaxAnalysisErrorListener throws for other syntax errors. Doing it at the parse-tree-to-statement boundary covers every caller: PPLService.execute/explain/analyze and api's PPLQueryParser, which would otherwise NPE on statement.getClass().

Alternatives Rejected:

• Make pplStatement required in the grammar. The resulting ANTLR message ("mismatched input ''...") is less clear, and grammar changes are riskier because autocomplete and other tooling depend on it.
• Null-check in PPLService before anonymizing. This would need three copies (execute/explain/analyze) and would miss the api parser.
• Reject query.isBlank() in PPLQueryRequestFactory. That only covers the REST path and misses comment-only queries.

Pitfalls: In the YAML REST runner, $body regex matching is in COMMENTS mode, so literal spaces are ignored. The test uses Query\s+is\s+empty, following issues/5060.yml. Also, a request with no query field at all already returns 400 ("Failed to parse request payload") from jsonContent.getString, so it is not affected by this change.

Things to Watch: The SQL grammar has the same sqlStatement? SEMI? EOF shape, and SQL's AstStatementBuilder has no equivalent guard. I didn't verify whether an empty SQL query reaches it or gets rejected or falls back to legacy earlier. It's outside the scope of this issue and could be a follow-up.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit d2ea329)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Breaking Change Changing the grammar rule from pplStatement? EOF to pplStatement EOF makes the optional statement mandatory. This breaks backward compatibility for any code or tests that relied on parsing empty input without error at the grammar level. While the PR description states the grammar is unchanged, this is a functional change that now rejects empty input during parsing rather than accepting it and handling it later. : pplStatement EOF Incorrect Assertion The test expects the error message to start with [<EOF>] is not a valid term, but with the grammar change making pplStatement mandatory, the actual parser error for empty input will be different (likely a missing token error before EOF). The assertion may fail because the error message format from ANTLR for a missing mandatory rule differs from the custom SyntaxCheckException message pattern being tested. assertTrue(e.getMessage(), e.getMessage().startsWith("[<EOF>] is not a valid term"));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to d2ea329
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Fix incorrect assertion message usage The assertion message parameter is used incorrectly. The first parameter of assertTrue should be the failure message shown when the assertion fails, not the actual value being tested. Move e.getMessage() to the assertion condition only. ppl/src/test/java/org/opensearch/sql/ppl/antlr/PPLSyntaxParserTest.java [391-395] for (String query : new String[] {"", " \n\t ", "/* block */ // line"}) { SyntaxCheckException e = assertThrows(SyntaxCheckException.class, () -> new PPLSyntaxParser().parse(query)); - assertTrue(e.getMessage(), e.getMessage().startsWith("[<EOF>] is not a valid term")); + assertTrue("Expected error message to start with '[<EOF>] is not a valid term', but got: " + e.getMessage(), + e.getMessage().startsWith("[<EOF>] is not a valid term")); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that assertTrue(e.getMessage(), ...) uses the message parameter incorrectly. The first parameter should be a custom failure message, not the value being tested. The improved code provides a more informative failure message that includes the actual error message received.	Medium

---

Previous suggestions

Suggestions up to commit 45655bd

Category	Suggestion	Impact
Possible issue	Add null check for statement result Add null safety check for the return value of super.visitRoot(ctx). If the parent visitor returns null despite pplStatement() being non-null, this could lead to unexpected null propagation. Consider validating the result before returning. ppl/src/main/java/org/opensearch/sql/ppl/parser/AstStatementBuilder.java [40-46] @Override public Statement visitRoot(OpenSearchPPLParser.RootContext ctx) { if (ctx.pplStatement() == null) { throw new SyntaxCheckException("Query is empty"); } - return super.visitRoot(ctx); + Statement statement = super.visitRoot(ctx); + if (statement == null) { + throw new SyntaxCheckException("Query is empty"); + } + return statement; } Suggestion importance[1-10]: 3 __ Why: While adding defensive null checking is generally good practice, this suggestion addresses a hypothetical scenario that's unlikely given the existing ctx.pplStatement() == null check. The parent visitor's behavior should be consistent with the grammar structure, making the additional null check redundant. The suggestion provides marginal defensive programming value but doesn't address a critical issue.	Low

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.87%. Comparing base (ccb58da) to head (d2ea329).
⚠️ Report is 3 commits behind head on main.

❌ Your project check has failed because the head coverage (63.87%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5868      +/-   ##
============================================
+ Coverage     63.78%   63.87%   +0.09%     
- Complexity     9063     9091      +28     
============================================
  Files           962      962              
  Lines         40974    41033      +59     
  Branches       4653     4668      +15     
============================================
+ Hits          26136    26211      +75     
+ Misses        13962    13944      -18     
- Partials        876      878       +2     

Flag	Coverage Δ
sql-engine	63.87% <ø> (+0.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[dai-chen]

Decision Log

...

Things to Watch: The SQL grammar has the same sqlStatement? SEMI? EOF shape, and SQL's AstStatementBuilder has no equivalent guard. I didn't verify whether an empty SQL query reaches it or gets rejected or falls back to legacy earlier. It's outside the scope of this issue and could be a follow-up.

Can we double check if similar issue on the SQL side?

[dai-chen]

The root cause is in grammar rule root : pplStatement? EOF, but we cannot simply remove the ??

[github-actions]

Persistent review updated to latest commit d2ea329

[RyanL1997]

@dai-chen just replying you in the separate thread since the previous review comment disappeared once i updated the commit:

The root cause is in grammar rule root : pplStatement? EOF, but we cannot simply remove the ??

This works - I have modified it to root : pplStatement EOF

[RyanL1997]

Can we double check if similar issue on the SQL side?

Yes - im thinking if we need to separate the sql fix into another PR - cause the symptom for sql is that it returns 400 but it exposed an NPE in its error msg.