Skip to content

Return 400 for user-facing planner validation errors - #22452

Open
finnegancarroll wants to merge 2 commits into
opensearch-project:mainfrom
finnegancarroll:fix/illegal-state-audit
Open

Return 400 for user-facing planner validation errors#22452
finnegancarroll wants to merge 2 commits into
opensearch-project:mainfrom
finnegancarroll:fix/illegal-state-audit

Conversation

@finnegancarroll

@finnegancarroll finnegancarroll commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Description

Changes two specific IllegalStateException throws to IllegalArgumentException in the analytics engine planner. These are error paths triggered by user-caused issues that should return HTTP 400 (actionable error) rather than 500 (redacted internal error).

Changes

File Error Rationale
FieldStorageResolver.java "Field [X] not found in field storage for index" User's query references a field that doesn't exist in the index mapping (e.g., typo in field name).
OpenSearchFilterRule.java "Unrecognized scalar function [X] in call [Y]" User used an unsupported UDF in a WHERE clause.

@finnegancarroll
finnegancarroll requested a review from a team as a code owner July 12, 2026 17:43
@github-actions

github-actions Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to aeaf843

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Keep wiring-error as internal exception

Per the surrounding comment, a missing DelegatedPredicateSerializer or missing
referencedFields() implementation is described as a wiring/configuration error, not
a user query error. Changing this to IllegalArgumentException would cause a 400
response for a server-side misconfiguration; it should remain an
IllegalStateException (or similar internal-error type) to correctly surface as a
5xx.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchFilterRule.java [185-190]

 DelegatedPredicateSerializer serializer = registry.predicateSerializer(function);
 FieldReferences refs = serializer == null ? null : serializer.referencedFields(predicate, fieldStorageInfos);
 if (refs == null) {
-    throw new IllegalArgumentException(
+    throw new IllegalStateException(
         "No field-reference extraction available for full-text function ["
             + predicate.getOperator().getName()
             + "]. A backend declaring this function's filter capability must provide a"
             + " DelegatedPredicateSerializer that implements referencedFields()."
Suggestion importance[1-10]: 6

__

Why: The suggestion makes a valid semantic distinction: the surrounding comment explicitly describes this as a "wiring error, not a query error", which aligns better with IllegalStateException than IllegalArgumentException. However, since the PR appears to intentionally standardize on IllegalArgumentException across the codebase, this may conflict with the PR's overall intent.

Low

Previous suggestions

Suggestions up to commit 5ca119e
CategorySuggestion                                                                                                                                    Impact
General
Remove redundant status parameter

The expectedStatus parameter is passed but the method name hard-codes 400, creating
a confusing API. Since the method's contract is specifically about 400 responses
(per its name), either remove the redundant parameter or rename the method to
assertQueryReturnsStatus for clarity. As written, callers could pass a different
status and get a misleading method name.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/SchemaValidationErrorsIT.java [58-61]

 // Querying the unstored field should produce a 400
-assertQueryReturns400(
-    "source=" + index + " | fields unstored",
-    400
-);
+assertQueryReturns400("source=" + index + " | fields unstored");
Suggestion importance[1-10]: 4

__

Why: Valid observation that the method name assertQueryReturns400 contradicts the parameterized expectedStatus. It's a minor API clarity improvement in test code.

Low
Make fail path independent of catch block

The fail(...) call inside the try block throws AssertionError, which is a
Throwable/Error — not caught by catch (ResponseException e), so this is fine, but if
performRequest succeeds with a 2xx, fail will correctly abort. However, note that
AssertionError propagation is only safe because ResponseException doesn't extend it.
Consider moving the fail(...) outside the try-catch by using a boolean flag or by
catching only around performRequest, to make the control flow explicit and avoid
future maintenance hazards if the catch is broadened.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/SchemaValidationErrorsIT.java [64-78]

 private void assertQueryReturns400(String ppl, int expectedStatus) throws IOException {
     Request request = new Request("POST", "/_analytics/ppl");
     request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}");
+    ResponseException caught = null;
     try {
         client().performRequest(request);
+    } catch (ResponseException e) {
+        caught = e;
+    }
+    if (caught == null) {
         fail("Expected " + expectedStatus + " error for query: " + ppl);
-    } catch (ResponseException e) {
-        int status = e.getResponse().getStatusLine().getStatusCode();
-        assertTrue(
-            "Expected " + expectedStatus + " but got " + status + ": "
-                + new String(e.getResponse().getEntity().getContent().readAllBytes(), StandardCharsets.UTF_8),
-            status == expectedStatus
-        );
     }
+    int status = caught.getResponse().getStatusLine().getStatusCode();
+    assertTrue(
+        "Expected " + expectedStatus + " but got " + status + ": "
+            + new String(caught.getResponse().getEntity().getContent().readAllBytes(), StandardCharsets.UTF_8),
+        status == expectedStatus
+    );
 }
Suggestion importance[1-10]: 2

__

Why: The current code works correctly since fail() throws AssertionError which is not caught by catch (ResponseException e). The suggestion is a minor stylistic refactor with negligible impact.

Low
Suggestions up to commit 91fbad3
CategorySuggestion                                                                                                                                    Impact
General
Preserve server-error semantics for missing mapping

Missing mapping metadata for an index is a server/cluster-state condition, not user
input; changing this from IllegalStateException to IllegalArgumentException will now
surface as HTTP 400 even though the user's request may be valid. Consider keeping
IllegalStateException for mapping == null (and possibly properties == null) so
genuine internal inconsistencies are not misreported as client errors.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/FieldStorageResolver.java [66-68]

 MappingMetadata mapping = indexMetadata.mapping();
 if (mapping == null) {
-    throw new IllegalArgumentException("No mapping found for index [" + indexName + "]");
+    throw new IllegalStateException("No mapping found for index [" + indexName + "]");
 }
Suggestion importance[1-10]: 7

__

Why: Valid semantic concern: a missing mapping is a server-side/cluster-state condition rather than invalid user input, so converting it to IllegalArgumentException may incorrectly surface as HTTP 400 instead of 500.

Medium
Align helper method name with parameter

After fail(...) is called inside the try block, control does not exit the method
because fail throws an AssertionError which is not a ResponseException — but if any
test framework wraps or if fail's exception is caught elsewhere, this could mask
real issues. More importantly, the method accepts expectedStatus as a parameter but
the caller passes 400 while the method name hardcodes "400"; either make the method
truly generic or drop the parameter. Also consider asserting the response body
includes a meaningful error message to guard against regressions.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/SchemaValidationErrorsIT.java [64-78]

-private void assertQueryReturns400(String ppl, int expectedStatus) throws IOException {
+private void assertQueryReturnsStatus(String ppl, int expectedStatus) throws IOException {
     Request request = new Request("POST", "/_analytics/ppl");
     request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}");
     try {
         client().performRequest(request);
         fail("Expected " + expectedStatus + " error for query: " + ppl);
     } catch (ResponseException e) {
         int status = e.getResponse().getStatusLine().getStatusCode();
-        assertTrue(
-            "Expected " + expectedStatus + " but got " + status + ": "
-                + new String(e.getResponse().getEntity().getContent().readAllBytes(), StandardCharsets.UTF_8),
-            status == expectedStatus
-        );
+        String body = new String(e.getResponse().getEntity().getContent().readAllBytes(), StandardCharsets.UTF_8);
+        assertEquals("Expected " + expectedStatus + " but got " + status + ": " + body, expectedStatus, status);
     }
 }
Suggestion importance[1-10]: 3

__

Why: Minor readability/consistency point about method naming and using assertEquals. The observation about fail() behavior is not a real issue. Low impact.

Low
Suggestions up to commit 91fbad3
CategorySuggestion                                                                                                                                    Impact
General
Keep wiring errors as internal failures

The surrounding comment explicitly states that a missing/incomplete serializer is "a
wiring error, not a query error — fail explicitly rather than under-validating".
Converting this to IllegalArgumentException will now surface backend
misconfiguration as a user-facing 400, masking a server-side bug. Keep this as
IllegalStateException (or a dedicated internal error) and only convert user-input
validation failures.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/rules/OpenSearchFilterRule.java [185-190]

 DelegatedPredicateSerializer serializer = registry.predicateSerializer(function);
 FieldReferences refs = serializer == null ? null : serializer.referencedFields(predicate, fieldStorageInfos);
 if (refs == null) {
-    throw new IllegalArgumentException(
+    throw new IllegalStateException(
         "No field-reference extraction available for full-text function ["
-        ...
Suggestion importance[1-10]: 7

__

Why: Valid observation: the code comment explicitly states this is a wiring/configuration error, not a user query error. Converting it to IllegalArgumentException (which likely maps to HTTP 400) would mask server misconfiguration as a user error, which contradicts the intent documented in the comment.

Medium
Reconcile helper name with parameter

The helper's second parameter is expectedStatus, but the method name hard-codes 400
and callers already pass 400 explicitly. Since a caller passing a different value
would be misleading (name says 400), either rename the helper or drop the redundant
parameter to avoid future confusion or misuse.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/SchemaValidationErrorsIT.java [58-61]

 // Querying the unstored field should produce a 400
-assertQueryReturns400(
+assertQueryReturnsStatus(
     "source=" + index + " | fields unstored",
     400
 );
Suggestion importance[1-10]: 3

__

Why: Minor readability improvement about API consistency between method name and parameter, but has no functional impact.

Low
Possible issue
Ensure JSON escape helper is available

The test calls escapeJson(ppl) but this method is not defined in the shown class. If
it isn't inherited from AnalyticsRestTestCase, the test will fail to compile. Ensure
escapeJson exists in the parent class or add a local implementation that escapes
backslashes and quotes.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/SchemaValidationErrorsIT.java [66]

-private String escapeJson(String s) {
-    ...
+private static String escapeJson(String s) {
+    return s.replace("\\", "\\\\").replace("\"", "\\\"");
 }
Suggestion importance[1-10]: 6

__

Why: The escapeJson method is referenced but not defined in the shown class. If not inherited from AnalyticsRestTestCase, this will cause a compilation error. However, it's plausible it's inherited from the parent test class.

Low
Suggestions up to commit 67ba41d
CategorySuggestion                                                                                                                                    Impact
General
Avoid mislabeling server-state errors as client errors

Missing mapping or missing top-level properties is typically an internal/server-side
condition (index metadata state) rather than something the caller directly provided.
Reclassifying these as IllegalArgumentException will now surface as HTTP 400 to end
users, which may misattribute server-side issues as client errors. Consider keeping
these as IllegalStateException and only reclassifying truly user-facing input issues
(e.g., an unknown field name provided in the query).

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/FieldStorageResolver.java [70-72]

 Map<String, Object> properties = (Map<String, Object>) mapping.sourceAsMap().get("properties");
 if (properties == null) {
-    throw new IllegalArgumentException("No properties in mapping for index [" + indexName + "]");
+    throw new IllegalStateException("No properties in mapping for index [" + indexName + "]");
 }
Suggestion importance[1-10]: 6

__

Why: Reasonable design concern: mapping without properties reflects index metadata state, not user input, so surfacing as HTTP 400 may misattribute the root cause. However, this depends on the PR's broader intent to classify all planner errors as client errors.

Low
Remove misleading unused parameter in test helper

The fail(...) call inside the try block throws AssertionError, which is not caught
by the catch (ResponseException e) clause, so the "expected but did not throw"
branch works — but if the response is a 2xx, no assertion of the status code occurs
and the test message references expectedStatus (always 400) misleadingly. Consider
removing the unused expectedStatus parameter or actually asserting on it, and ensure
the fail message is clear.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/SchemaValidationErrorsIT.java [64-78]

-private void assertQueryReturns400(String ppl, int expectedStatus) throws IOException {
+private void assertQueryReturns400(String ppl) throws IOException {
     Request request = new Request("POST", "/_analytics/ppl");
     request.setJsonEntity("{\"query\": \"" + escapeJson(ppl) + "\"}");
     try {
         client().performRequest(request);
-        fail("Expected " + expectedStatus + " error for query: " + ppl);
+        fail("Expected 400 error for query: " + ppl);
     } catch (ResponseException e) {
         int status = e.getResponse().getStatusLine().getStatusCode();
-        assertTrue(
-            "Expected " + expectedStatus + " but got " + status + ": "
-                + new String(e.getResponse().getEntity().getContent().readAllBytes(), StandardCharsets.UTF_8),
-            status == expectedStatus
+        assertEquals(
+            "Response body: " + new String(e.getResponse().getEntity().getContent().readAllBytes(), StandardCharsets.UTF_8),
+            400,
+            status
         );
     }
 }
Suggestion importance[1-10]: 4

__

Why: The observation is correct — expectedStatus is passed but the caller always uses 400, and the fail message references it but it's fine. This is a minor test-code cleanup with limited impact.

Low
Suggestions up to commit 9716136
CategorySuggestion                                                                                                                                    Impact
Possible issue
Ensure JSON escape helper is defined

The escapeJson method is referenced in assertQueryReturns400 but not defined in the
diff shown. If it's not defined elsewhere in the class or a parent class, this will
cause a compilation error. Ensure it exists in AnalyticsRestTestCase or add it
locally.

sandbox/qa/analytics-engine-rest/src/test/java/org/opensearch/analytics/qa/SchemaValidationErrorsIT.java [55]

+private String escapeJson(String s) {
+    return s.replace("\\", "\\\\").replace("\"", "\\\"");
+}
 
-
Suggestion importance[1-10]: 5

__

Why: The suggestion flags a potentially missing escapeJson helper method used in assertQueryReturns400. If not defined in the parent class, this would cause a compilation error, but the suggestion only asks to verify, which reduces its impact.

Low
General
Reconsider 400 for missing mapping state

A missing mapping or properties block typically indicates a server/index-state issue
rather than a user-input error. Consider whether these truly warrant 400 responses;
if they can occur due to internal state (empty/system indices), they may
misleadingly be reported as bad requests. Verify these paths are only reachable via
user-driven schema misconfiguration before converting to IllegalArgumentException.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/FieldStorageResolver.java [66-72]

+MappingMetadata mapping = indexMetadata.mapping();
+if (mapping == null) {
+    throw new IllegalArgumentException("No mapping found for index [" + indexName + "]");
+}
+Map<String, Object> properties = (Map<String, Object>) mapping.sourceAsMap().get("properties");
+if (properties == null) {
+    throw new IllegalArgumentException("No properties in mapping for index [" + indexName + "]");
+}
 
-
Suggestion importance[1-10]: 4

__

Why: Valid concern about semantic correctness of using IllegalArgumentException for internal state issues, but it only asks to verify and does not provide a concrete improvement (existing and improved code are identical).

Low

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 028a643: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions

github-actions Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 7f95ec2)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 028a643: SUCCESS

@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.45%. Comparing base (3f7e70d) to head (7f95ec2).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #22452      +/-   ##
============================================
- Coverage     71.47%   71.45%   -0.02%     
+ Complexity    76978    76936      -42     
============================================
  Files          6156     6156              
  Lines        358443   358443              
  Branches      52246    52246              
============================================
- Hits         256192   256143      -49     
+ Misses        81951    81919      -32     
- Partials      20300    20381      +81     

☔ 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.

@finnegancarroll
finnegancarroll force-pushed the fix/illegal-state-audit branch from 028a643 to 9716136 Compare July 13, 2026 19:21
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 9716136

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 9716136: SUCCESS

@finnegancarroll
finnegancarroll force-pushed the fix/illegal-state-audit branch 2 times, most recently from de97832 to 67ba41d Compare July 13, 2026 20:48
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 67ba41d

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 67ba41d: SUCCESS

@finnegancarroll
finnegancarroll force-pushed the fix/illegal-state-audit branch from 67ba41d to 91fbad3 Compare July 20, 2026 23:12
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 91fbad3

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 91fbad3: SUCCESS

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 91fbad3

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 91fbad3: SUCCESS

@finnegancarroll
finnegancarroll force-pushed the fix/illegal-state-audit branch from 91fbad3 to 5ca119e Compare July 21, 2026 22:38
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 5ca119e

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 5ca119e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@finnegancarroll
finnegancarroll force-pushed the fix/illegal-state-audit branch from 5ca119e to b0da0d5 Compare July 22, 2026 23:15
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit b0da0d5

@github-actions

Copy link
Copy Markdown
Contributor

❌ Gradle check result for b0da0d5: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@finnegancarroll
finnegancarroll force-pushed the fix/illegal-state-audit branch from b0da0d5 to 94a46cb Compare July 23, 2026 20:17
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f41d522

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for f41d522: SUCCESS

@finnegancarroll
finnegancarroll force-pushed the fix/illegal-state-audit branch from f41d522 to a87d57c Compare July 25, 2026 21:56
@finnegancarroll
finnegancarroll force-pushed the fix/illegal-state-audit branch from a87d57c to aeaf843 Compare August 4, 2026 17:24
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit aeaf843

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for aeaf843: SUCCESS

@finnegancarroll
finnegancarroll force-pushed the fix/illegal-state-audit branch from aeaf843 to c946cc2 Compare August 4, 2026 19:19
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit c946cc2

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit c946cc2

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for c946cc2: SUCCESS

@finnegancarroll
finnegancarroll force-pushed the fix/illegal-state-audit branch from c946cc2 to 6f3dea1 Compare August 5, 2026 17:46
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 6f3dea1

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

❕ Gradle check result for 6f3dea1: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@finnegancarroll
finnegancarroll force-pushed the fix/illegal-state-audit branch 2 times, most recently from 2b2be24 to f3f273f Compare August 7, 2026 16:52
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f3f273f

@finnegancarroll
finnegancarroll force-pushed the fix/illegal-state-audit branch from f3f273f to c54a98e Compare August 7, 2026 17:24
@finnegancarroll finnegancarroll changed the title Return 400 for user-facing analytics planner validation errors Return 400 for user-facing planner validation errors Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit c54a98e

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for c54a98e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Changes IllegalStateException to IllegalArgumentException for two specific
planner error paths that represent user-caused issues:

1. Field not found in field storage: user's query references a field that
   doesn't exist in the index mapping (e.g., typo in field name).

2. Unrecognized scalar function in filter: user used an unsupported UDF
   in a WHERE clause.

All other planner IllegalStateExceptions remain as-is since they represent
engine misconfiguration or bugs (no user action can trigger them).

Signed-off-by: Finn Carroll <carrofin@amazon.com>
@finnegancarroll
finnegancarroll force-pushed the fix/illegal-state-audit branch from c54a98e to 59dc795 Compare August 7, 2026 18:02
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 59dc795

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

❌ Gradle check result for 59dc795: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

…+ Jenkins timeout)

Signed-off-by: Finn Carroll <carrofin@amazon.com>
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 7f95ec2

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 7f95ec2: SUCCESS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants