Skip to content

Commit f46a4aa

Browse files
SwiddisSwiddis
committed
Add some ITs
Signed-off-by: Simeon Widdis <sawiddis@amazon.com>
1 parent 0f63e35 commit f46a4aa

3 files changed

Lines changed: 221 additions & 2 deletions

File tree

‎common/src/main/java/org/opensearch/sql/common/error/ErrorReport.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ public Map<String, Object> toJsonMap() {
172172
Map<String, Object> contextMap = new LinkedHashMap<>(context);
173173
if (stage != null) {
174174
contextMap.put("stage", stage.toJsonKey());
175-
contextMap.put("stage_name", stage.getDisplayName());
175+
contextMap.put("stage_description", stage.getDisplayName());
176176
}
177177
json.put("context", contextMap);
178178
}

‎common/src/test/java/org/opensearch/sql/common/error/ErrorReportTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ public void testErrorReportJsonMapWithStageInContext() {
6464
@SuppressWarnings("unchecked")
6565
Map<String, Object> context = (Map<String, Object>) json.get("context");
6666
assertEquals("analyzing", context.get("stage"));
67-
assertEquals("Checking Query Against Your Data", context.get("stage_name"));
67+
assertEquals("Checking Query Against Your Data", context.get("stage_description"));
6868
assertEquals("test", context.get("field_name"));
6969
}
7070

‎integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteErrorReportStageIT.java‎

Lines changed: 219 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,219 @@
1+
/*
2+
* Copyright OpenSearch Contributors
3+
* SPDX-License-Identifier: Apache-2.0
4+
*/
5+
6+
package org.opensearch.sql.calcite.remote;
7+
8+
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT;
9+
import static org.opensearch.sql.util.TestUtils.getResponseBody;
10+
11+
import java.io.IOException;
12+
import org.json.JSONObject;
13+
import org.junit.jupiter.api.Test;
14+
import org.opensearch.client.ResponseException;
15+
import org.opensearch.sql.ppl.PPLIntegTestCase;
16+
17+
/**
18+
* Integration tests for error report builder with stage tracking. Validates that errors include
19+
* stage information and user-friendly messages.
20+
*/
21+
public class CalciteErrorReportStageIT extends PPLIntegTestCase {
22+
23+
@Override
24+
public void init() throws Exception {
25+
super.init();
26+
loadIndex(Index.ACCOUNT);
27+
enableCalcite();
28+
}
29+
30+
@Test
31+
public void testFieldNotFoundErrorIncludesStage() throws IOException {
32+
ResponseException exception =
33+
assertThrows(
34+
ResponseException.class,
35+
() -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields nonexistent_field"));
36+
37+
String responseBody = getResponseBody(exception.getResponse());
38+
JSONObject response = new JSONObject(responseBody);
39+
JSONObject error = response.getJSONObject("error");
40+
41+
// Verify error has context with stage information
42+
assertTrue("Error should have context", error.has("context"));
43+
JSONObject context = error.getJSONObject("context");
44+
45+
assertTrue("Context should have stage", context.has("stage"));
46+
assertEquals("Stage should be 'analyzing'", "analyzing", context.getString("stage"));
47+
48+
assertTrue("Context should have stage_description", context.has("stage_description"));
49+
String stageDescription = context.getString("stage_description");
50+
assertTrue(
51+
"Stage description should be user-friendly",
52+
stageDescription.contains("Checking") || stageDescription.contains("Query"));
53+
54+
// Verify error has location chain
55+
assertTrue("Error should have location", error.has("location"));
56+
assertTrue("Location should be an array", error.get("location") instanceof org.json.JSONArray);
57+
58+
// Verify location message is user-friendly (not technical)
59+
org.json.JSONArray locationArray = error.getJSONArray("location");
60+
assertTrue("Location array should not be empty", locationArray.length() > 0);
61+
String location = locationArray.getString(0);
62+
assertFalse(
63+
"Location should not mention internal terms like 'Calcite'", location.contains("Calcite"));
64+
assertFalse(
65+
"Location should not mention internal terms like 'RelNode'", location.contains("RelNode"));
66+
}
67+
68+
@Test
69+
public void testIndexNotFoundErrorIncludesStage() throws IOException {
70+
ResponseException exception =
71+
assertThrows(
72+
ResponseException.class, () -> executeQuery("source=nonexistent_index | fields age"));
73+
74+
String responseBody = getResponseBody(exception.getResponse());
75+
JSONObject response = new JSONObject(responseBody);
76+
JSONObject error = response.getJSONObject("error");
77+
78+
// Verify error has context with stage
79+
assertTrue("Error should have context", error.has("context"));
80+
JSONObject context = error.getJSONObject("context");
81+
assertTrue("Context should have stage", context.has("stage"));
82+
83+
// Verify error has location
84+
assertTrue("Error should have location", error.has("location"));
85+
}
86+
87+
@Test
88+
public void testMultipleFieldErrorsIncludeStage() throws IOException {
89+
ResponseException exception =
90+
assertThrows(
91+
ResponseException.class,
92+
() ->
93+
executeQuery(
94+
"source="
95+
+ TEST_INDEX_ACCOUNT
96+
+ " | fields nonexistent1, nonexistent2, nonexistent3"));
97+
98+
String responseBody = getResponseBody(exception.getResponse());
99+
JSONObject response = new JSONObject(responseBody);
100+
JSONObject error = response.getJSONObject("error");
101+
102+
// Verify stage information is present
103+
assertTrue("Error should have context", error.has("context"));
104+
JSONObject context = error.getJSONObject("context");
105+
assertTrue("Context should have stage", context.has("stage"));
106+
assertTrue("Context should have stage_description", context.has("stage_description"));
107+
}
108+
109+
@Test
110+
public void testErrorReportTypeIsCorrect() throws IOException {
111+
ResponseException exception =
112+
assertThrows(
113+
ResponseException.class,
114+
() -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields bad_field_name"));
115+
116+
String responseBody = getResponseBody(exception.getResponse());
117+
JSONObject response = new JSONObject(responseBody);
118+
JSONObject error = response.getJSONObject("error");
119+
120+
// Verify error has type field
121+
assertTrue("Error should have type", error.has("type"));
122+
123+
// Verify error has details
124+
assertTrue("Error should have details", error.has("details"));
125+
}
126+
127+
@Test
128+
public void testErrorCodePresent() throws IOException {
129+
ResponseException exception =
130+
assertThrows(
131+
ResponseException.class,
132+
() -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields missing_field"));
133+
134+
String responseBody = getResponseBody(exception.getResponse());
135+
JSONObject response = new JSONObject(responseBody);
136+
JSONObject error = response.getJSONObject("error");
137+
138+
// Verify error may have code field (optional, but if present should be valid)
139+
if (error.has("code")) {
140+
String code = error.getString("code");
141+
assertFalse("Error code should not be empty", code.isEmpty());
142+
assertFalse("Error code should not be UNKNOWN", code.equals("UNKNOWN"));
143+
}
144+
}
145+
146+
@Test
147+
public void testLocationMessagesAreUserFriendly() throws IOException {
148+
ResponseException exception =
149+
assertThrows(
150+
ResponseException.class,
151+
() -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields xyz123"));
152+
153+
String responseBody = getResponseBody(exception.getResponse());
154+
JSONObject response = new JSONObject(responseBody);
155+
JSONObject error = response.getJSONObject("error");
156+
157+
assertTrue("Error should have location", error.has("location"));
158+
org.json.JSONArray locationArray = error.getJSONArray("location");
159+
160+
// Verify all location messages are user-friendly
161+
for (int i = 0; i < locationArray.length(); i++) {
162+
String location = locationArray.getString(i);
163+
164+
// Should not contain technical terms
165+
assertFalse(
166+
"Location should not contain 'AST'",
167+
location.toLowerCase().contains("ast") && !location.toLowerCase().contains("last"));
168+
assertFalse("Location should not contain 'RelNode'", location.contains("RelNode"));
169+
assertFalse(
170+
"Location should not contain 'semantic analysis' (too technical)",
171+
location.contains("semantic analysis"));
172+
173+
// Should use user-friendly language
174+
assertTrue(
175+
"Location should mention query, fields, data, cluster, or execution",
176+
location.toLowerCase().contains("query")
177+
|| location.toLowerCase().contains("field")
178+
|| location.toLowerCase().contains("data")
179+
|| location.toLowerCase().contains("cluster")
180+
|| location.toLowerCase().contains("execut"));
181+
}
182+
}
183+
184+
@Test
185+
public void testStageDescriptionIsUserFriendly() throws IOException {
186+
ResponseException exception =
187+
assertThrows(
188+
ResponseException.class,
189+
() -> executeQuery("source=" + TEST_INDEX_ACCOUNT + " | fields undefined_field"));
190+
191+
String responseBody = getResponseBody(exception.getResponse());
192+
JSONObject response = new JSONObject(responseBody);
193+
JSONObject error = response.getJSONObject("error");
194+
195+
assertTrue("Error should have context", error.has("context"));
196+
JSONObject context = error.getJSONObject("context");
197+
assertTrue("Context should have stage_description", context.has("stage_description"));
198+
199+
String stageDescription = context.getString("stage_description");
200+
201+
// Stage description should not use compiler/technical terminology
202+
assertFalse(
203+
"Stage description should not contain 'Semantic'", stageDescription.contains("Semantic"));
204+
assertFalse(
205+
"Stage description should not contain 'Calcite'", stageDescription.contains("Calcite"));
206+
assertFalse(
207+
"Stage description should not contain 'AST'",
208+
stageDescription.contains("AST") && !stageDescription.contains("Last"));
209+
210+
// Should use analyst-friendly language
211+
assertTrue(
212+
"Stage description should be user-friendly",
213+
stageDescription.toLowerCase().contains("check")
214+
|| stageDescription.toLowerCase().contains("validat")
215+
|| stageDescription.toLowerCase().contains("prepar")
216+
|| stageDescription.toLowerCase().contains("run")
217+
|| stageDescription.toLowerCase().contains("query"));
218+
}
219+
}

0 commit comments

Comments
 (0)