Copilot commented on code in PR #19775:
URL: https://github.com/apache/druid/pull/19775#discussion_r3662372828
##########
processing/src/main/java/org/apache/druid/query/BadJsonQueryException.java:
##########
@@ -51,4 +58,13 @@ private BadJsonQueryException(
{
super(cause, errorCode, errorMessage, errorClass, null);
}
+
+ private static String getErrorMessage(ValueInstantiationException e)
+ {
+ final Throwable cause = e.getCause();
+ if (cause == null || Strings.isNullOrEmpty(cause.getMessage())) {
+ return "Invalid native query: the request contains invalid or missing
fields";
+ }
+ return "Invalid native query: " + cause.getMessage();
+ }
Review Comment:
BadJsonQueryException#getErrorMessage only looks at e.getCause(). If the
immediate cause has no message (e.g., InvocationTargetException), the response
falls back to the generic text even though a deeper/root cause may contain the
actionable validation message. Consider walking to the root cause before
extracting the message.
##########
server/src/main/java/org/apache/druid/server/QueryResource.java:
##########
@@ -260,6 +261,9 @@ private Query<?> readQuery(
try {
baseQuery = ioReaderWriter.getRequestMapper().readValue(in, Query.class);
}
+ catch (ValueInstantiationException e) {
+ throw new BadJsonQueryException(e);
+ }
catch (JsonParseException e) {
throw new BadJsonQueryException(e);
}
Review Comment:
QueryResource.readQuery now handles ValueInstantiationException, but other
Jackson mapping/instantiation failures (e.g., MismatchedInputException,
InvalidTypeIdException, UnrecognizedPropertyException) will still escape
readQuery as IOException and be turned into an HTTP 500 by the outer catch-all.
If the goal is to classify invalid native-query payloads as 400 with actionable
messages, consider catching JsonProcessingException/JsonMappingException here
and wrapping them into BadJsonQueryException as well.
##########
server/src/test/java/org/apache/druid/server/QueryResourceTest.java:
##########
@@ -1043,6 +1044,23 @@ public void testBadQuery() throws IOException
Assert.assertEquals(BadJsonQueryException.ERROR_CLASS, e.getErrorClass());
}
+ @Test
+ public void testIncompleteQuery() throws IOException
+ {
+ final Response response = queryResource.doPost(
+ new
ByteArrayInputStream("{\"queryType\":\"scan\"}".getBytes(StandardCharsets.UTF_8)),
+ null /*pretty*/,
+ testServletRequest
+ );
+
+ Assert.assertNotNull(response);
+ Assert.assertEquals(Status.BAD_REQUEST.getStatusCode(),
response.getStatus());
+ final QueryException e = jsonMapper.readValue((byte[])
response.getEntity(), QueryException.class);
+ Assert.assertEquals(QueryException.JSON_PARSE_ERROR_CODE,
e.getErrorCode());
+ Assert.assertEquals(ValueInstantiationException.class.getName(),
e.getErrorClass());
+ Assert.assertEquals("Invalid native query: dataSource can't be null",
e.getMessage());
Review Comment:
testIncompleteQuery asserts the full validation message string exactly.
Since this message ultimately comes from constructor/validation exceptions, it
can change with refactors without changing the intent of the behavior (return
400 + actionable message). Using prefix/substring assertions would keep the
test focused on the contract while reducing brittleness.
--
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]
