yashmayya commented on code in PR #18515:
URL: https://github.com/apache/pinot/pull/18515#discussion_r3261125032
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -176,6 +178,7 @@ public void processSqlQueryGet(@ApiParam(value = "Query",
required = true) @Quer
@ApiOperation(value = "Querying pinot")
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Query response"),
+ @ApiResponse(code = 400, message = "Bad Request"),
Review Comment:
**Backward-incompat surfacing — add release notes / doc updates.**
This PR is correctly labeled `backward-incompat`, but the operator-facing
surface hasn't been updated. Clients/dashboards keyed on
`UNCAUGHT_POST_EXCEPTIONS` or HTTP-500 error-rate alarms will see a
step-change, and any client that previously retried on 5xx for malformed JSON /
missing fields will now get a non-retriable 400.
Please add a release-notes block / CHANGELOG entry calling out:
1. The 500→400 status code change for the five affected endpoints.
2. The new `BAD_REQUEST_EXCEPTIONS` meter.
3. The expected drop in `UNCAUGHT_POST_EXCEPTIONS` counts.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -219,29 +230,40 @@ public void processSqlQueryPost(String query, @Suspended
AsyncResponse asyncResp
+ "Supports both single-stage and multi-stage queries.")
@ApiResponses(value = {
@ApiResponse(code = 200, message = "Query fingerprint"),
+ @ApiResponse(code = 400, message = "Bad Request"),
@ApiResponse(code = 500, message = "Internal Server Error")
})
@ManualAuthorization
public Response getQueryFingerprint(String query,
@Context org.glassfish.grizzly.http.server.Request requestContext,
@Context HttpHeaders httpHeaders) {
try {
- JsonNode requestJson = JsonUtils.stringToJsonNode(query);
+ JsonNode requestJson;
+ try {
+ requestJson = JsonUtils.stringToJsonNode(query);
+ } catch (JsonProcessingException e) {
+ throw new BadRequestException("Invalid JSON: " + e.getMessage(), e);
+ }
if (!requestJson.has(Request.SQL)) {
- throw new IllegalStateException("Payload is missing the query string
field 'sql'");
+ throw new BadRequestException("Payload is missing the query string
field 'sql'");
}
QueryFingerprint fingerprint = generateQueryFingerprint((ObjectNode)
requestJson);
return Response.ok(fingerprint).build();
+ } catch (BadRequestException bre) {
+ _brokerMetrics.addMeteredGlobalValue(BrokerMeter.BAD_REQUEST_EXCEPTIONS,
1L);
+ throw bre;
Review Comment:
**Inconsistent error response body between 400 and 500 paths.**
The 500 path a few lines below now correctly emits `{"error": "..."}` JSON
(good — fixes the JSON-injection bug). But this new 400 path re-throws the raw
`BadRequestException`, which goes through Jersey's default exception mapper and
produces a different body shape (often plain text or empty body depending on
configured providers).
Callers that previously parsed `{"error": "..."}` for *all* failures now get
a structurally different body for 400s — a contract break for any tooling that
parses error responses.
Suggest wrapping so the body shape stays consistent across status codes:
```java
ObjectNode errorJson = JsonUtils.newObjectNode();
errorJson.put("error", bre.getMessage());
throw new WebApplicationException(bre,
Response.status(Response.Status.BAD_REQUEST).entity(errorJson.toString()).build());
```
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -472,10 +520,15 @@ public void
processSqlQueryWithBothEnginesAndCompareResults(String query, @Suspe
CompletableFuture.allOf(v1Response, v2Response).join();
asyncResponse.resume(getPinotQueryComparisonResponse(v1Query,
v1Response.get(), v2Response.get()));
+ } catch (BadRequestException bre) {
Review Comment:
**Async-path `BadRequestException`s still get reclassified as 500 here.**
The `v1Response` / `v2Response` `supplyAsync` lambdas above wrap thrown
exceptions as `new RuntimeException(e)`. When
`CompletableFuture.allOf(...).join()` rethrows, the result is
`CompletionException` → `RuntimeException` → original cause. This `catch
(BadRequestException bre)` clause will NOT match that wrapped chain, so a
400-worthy error raised deep inside `executeSqlQuery` still falls into the
generic `catch (Exception e)` below and emits 500 + `UNCAUGHT_POST_EXCEPTIONS`.
Three options:
1. Unwrap `CompletionException` / `RuntimeException` causes here before
classifying.
2. Refactor the `supplyAsync` lambdas not to double-wrap.
3. If `executeSqlQuery` is contractually guaranteed not to throw
`BadRequestException` (it does return error responses via
`BrokerResponseNative` rather than throw), document that with a comment so
future readers don't trip on it. Please confirm which case applies.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -493,8 +546,10 @@ public void
processSqlQueryWithBothEnginesAndCompareResults(String query, @Suspe
+ "given id on the requested broker. Query may continue to run for a
short while after calling cancel as "
+ "it's done in a non-blocking manner. The cancel method can be called
multiple times.")
@ApiResponses(value = {
- @ApiResponse(code = 200, message = "Success"), @ApiResponse(code = 500,
message = "Internal server error"),
- @ApiResponse(code = 404, message = "Query not found on the requested
broker")
+ @ApiResponse(code = 200, message = "Success"),
+ @ApiResponse(code = 400, message = "Bad Request"),
Review Comment:
**Swagger 400 message is too generic across endpoints.**
The same `"Bad Request"` text is used here and on all four SQL endpoints,
which makes the generated API docs less useful. Suggest endpoint-specific
descriptions, e.g.:
- Here: `"Invalid query id format"`
- SQL endpoints: `"Missing/invalid SQL field or malformed JSON"`
Also worth documenting that for `cancelQuery`, the 400 only applies to
internal-id parsing — `cancelQueryByClientId` (the `isClient=true` branch)
doesn't parse a `Long` and won't surface 400 for malformed client ids.
##########
pinot-broker/src/test/java/org/apache/pinot/broker/api/resources/PinotClientRequestTest.java:
##########
@@ -171,16 +173,65 @@ public void testPinotQueryComparisonApiDifferentQuery()
throws Exception {
}
@Test
- public void testPinotQueryComparisonApiMissingSql() throws Exception {
+ public void testPinotQueryComparisonApiMissingV2Sql() throws Exception {
Review Comment:
**Status-code contract isn't validated end-to-end.**
All new tests assert on `WebApplicationException.getResponse().getStatus()`
— the *exception object's* status — which proves the right exception was raised
but not that Jersey actually returns HTTP 400 on the wire. Since this PR's
whole purpose is the HTTP wire-level contract, the assertions are necessary but
not sufficient.
Please either:
1. Add at least one Jersey-level integration test (or `JerseyTest`-based
test) that POSTs a malformed body and asserts the actual HTTP response status;
or
2. Add a comment at the catch sites in `PinotClientRequest` documenting that
the wire-level 400 relies on JAX-RS's default `BadRequestException → 400`
mapping, so future refactors don't accidentally break it.
##########
pinot-broker/src/test/java/org/apache/pinot/broker/api/resources/PinotClientRequestTest.java:
##########
@@ -274,10 +325,103 @@ public void testGetQueryFingerprintWithInvalidSql()
throws Exception {
when(request.getRequestURL()).thenReturn(new StringBuilder());
String requestJson = "{\"sql\": \"INVALID SQL QUERY\"}";
- Response response = _pinotClientRequest.getQueryFingerprint(requestJson,
request, null);
+ try {
+ _pinotClientRequest.getQueryFingerprint(requestJson, request, null);
+ Assert.fail("Expected BadRequestException");
+ } catch (WebApplicationException wae) {
+ assertEquals(wae.getResponse().getStatus(),
Response.Status.BAD_REQUEST.getStatusCode(),
+ "Invalid SQL query should return BAD_REQUEST status");
+ }
+
verify(_brokerMetrics).addMeteredGlobalValue(BrokerMeter.BAD_REQUEST_EXCEPTIONS,
1L);
+ verify(_brokerMetrics,
never()).addMeteredGlobalValue(BrokerMeter.UNCAUGHT_POST_EXCEPTIONS, 1L);
+ }
+
+ @Test
+ public void testProcessSqlQueryPostMissingSql() {
+ AsyncResponse asyncResponse = mock(AsyncResponse.class);
+ Request request = mock(Request.class);
+ when(request.getRequestURL()).thenReturn(new StringBuilder());
+
+ _pinotClientRequest.processSqlQueryPost("{}", asyncResponse, false, 0,
request, null);
+
+ ArgumentCaptor<WebApplicationException> captor =
ArgumentCaptor.forClass(WebApplicationException.class);
+ verify(asyncResponse).resume(captor.capture());
+ assertEquals(captor.getValue().getResponse().getStatus(),
Response.Status.BAD_REQUEST.getStatusCode());
+
verify(_brokerMetrics).addMeteredGlobalValue(BrokerMeter.BAD_REQUEST_EXCEPTIONS,
1L);
+ verify(_brokerMetrics,
never()).addMeteredGlobalValue(BrokerMeter.UNCAUGHT_POST_EXCEPTIONS, 1L);
+ }
+
+ @Test
+ public void testProcessSqlQueryPostInvalidJson() {
+ AsyncResponse asyncResponse = mock(AsyncResponse.class);
+ Request request = mock(Request.class);
+ when(request.getRequestURL()).thenReturn(new StringBuilder());
+
+ _pinotClientRequest.processSqlQueryPost("{bad json", asyncResponse, false,
0, request, null);
+
+ ArgumentCaptor<WebApplicationException> captor =
ArgumentCaptor.forClass(WebApplicationException.class);
+ verify(asyncResponse).resume(captor.capture());
+ assertEquals(captor.getValue().getResponse().getStatus(),
Response.Status.BAD_REQUEST.getStatusCode());
+
verify(_brokerMetrics).addMeteredGlobalValue(BrokerMeter.BAD_REQUEST_EXCEPTIONS,
1L);
+ verify(_brokerMetrics,
never()).addMeteredGlobalValue(BrokerMeter.UNCAUGHT_POST_EXCEPTIONS, 1L);
+ }
+
+ @Test
+ public void testGetQueryFingerprintMissingSql() {
+ Request request = mock(Request.class);
+ when(request.getRequestURL()).thenReturn(new StringBuilder());
+
+ try {
+ _pinotClientRequest.getQueryFingerprint("{}", request, null);
+ Assert.fail("Expected BadRequestException");
+ } catch (WebApplicationException wae) {
+ assertEquals(wae.getResponse().getStatus(),
Response.Status.BAD_REQUEST.getStatusCode());
+ }
+
verify(_brokerMetrics).addMeteredGlobalValue(BrokerMeter.BAD_REQUEST_EXCEPTIONS,
1L);
+ verify(_brokerMetrics,
never()).addMeteredGlobalValue(BrokerMeter.UNCAUGHT_POST_EXCEPTIONS, 1L);
+ }
+
+ @Test
+ public void testGetQueryFingerprintInvalidJson() {
+ Request request = mock(Request.class);
+ when(request.getRequestURL()).thenReturn(new StringBuilder());
+
+ try {
+ _pinotClientRequest.getQueryFingerprint("{bad json", request, null);
+ Assert.fail("Expected BadRequestException");
+ } catch (WebApplicationException wae) {
+ assertEquals(wae.getResponse().getStatus(),
Response.Status.BAD_REQUEST.getStatusCode());
+ }
+
verify(_brokerMetrics).addMeteredGlobalValue(BrokerMeter.BAD_REQUEST_EXCEPTIONS,
1L);
+ verify(_brokerMetrics,
never()).addMeteredGlobalValue(BrokerMeter.UNCAUGHT_POST_EXCEPTIONS, 1L);
+ }
+
+ @Test
+ public void testCancelQueryWithInvalidId() {
+ try {
+ _pinotClientRequest.cancelQuery("not-a-number", false, 3000, false);
Review Comment:
**`cancelQuery` `isClient=true` branch is uncovered.**
This test only exercises `isClient=false`. The cancel-flow rewrite touches
both branches — `isClient=true` passes the raw `String` id to
`_requestHandler.cancelQueryByClientId` and never parses `Long`, so the new
`BadRequestException` path is dead code for that branch.
Please add a sibling test for `cancelQuery("anything", true, 3000, false)`
to lock in the expected behavior (likely 404 from `cancelQueryByClientId`
returning `false` against the unstubbed mock).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
