FrankChen021 commented on code in PR #19775:
URL: https://github.com/apache/druid/pull/19775#discussion_r3662435172
##########
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:
Addressed in 179dbd4bfc. BadJsonQueryException now walks the full cause
chain and uses the deepest non-empty message. Added
testBadJsonQueryExceptionUsesDeepestCauseMessage to cover an intermediate
wrapper with an actionable root cause.
##########
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:
Keeping this PR intentionally scoped to ValueInstantiationException from
native-query constructor validation, which is the reported failure mode.
Broadly catching every JsonProcessingException would reclassify additional
Jackson failures with potentially different semantics and is better handled
separately.
##########
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:
Keeping the exact assertion intentionally. The response text is a user- and
AI-facing contract in this PR, and the assertion prevents regressions back to
Jackson class and source-location details.
--
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]
