This is an automated email from the ASF dual-hosted git repository.
FrankChen021 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/druid.git
The following commit(s) were added to refs/heads/master by this push:
new 42f49fd80a9 fix: return 400 for native queries that fail JSON binding
(#20830)
42f49fd80a9 is described below
commit 42f49fd80a9e31a35e254e554507423dab355f06
Author: Frank Chen <[email protected]>
AuthorDate: Sat Nov 7 11:08:38 2026 +0800
fix: return 400 for native queries that fail JSON binding (#20830)
* fix: return 400 for native queries that fail JSON binding
QueryResource#readQuery converts JsonParseException and
ValueInstantiationException
into BadJsonQueryException, but other JsonMappingExceptions raised for
well-formed JSON
(e.g. an unknown queryType, or a field of the wrong type) fell through to
the catch-all
in doPost. The broker logged a full stack trace at WARN and returned 500
"Unhandled exception made it to the top" without saying what was wrong.
Catch JsonMappingException as well, and report it as a 400 "Json parse
failed" with the
original message and the reference chain of the offending field.
* Rename getErrorMessage overload to avoid confusing overloading
* Add BadJsonQueryException unit tests for JsonMappingException
* Test JSON binding errors with real queries instead of hand-built
exceptions
* Keep InvalidDefinitionException as a server error when reading native
queries
* Keep server errors wrapped in JsonMappingException as 500 when reading
native queries
---
.../apache/druid/query/BadJsonQueryException.java | 15 +++
.../druid/query/BadJsonQueryExceptionTest.java | 53 +++++++++
.../org/apache/druid/server/QueryResource.java | 35 ++++++
.../org/apache/druid/server/QueryResourceTest.java | 122 +++++++++++++++++++++
4 files changed, 225 insertions(+)
diff --git
a/processing/src/main/java/org/apache/druid/query/BadJsonQueryException.java
b/processing/src/main/java/org/apache/druid/query/BadJsonQueryException.java
index 7722bb70be5..c89f45b8864 100644
--- a/processing/src/main/java/org/apache/druid/query/BadJsonQueryException.java
+++ b/processing/src/main/java/org/apache/druid/query/BadJsonQueryException.java
@@ -22,6 +22,7 @@ package org.apache.druid.query;
import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.JsonParseException;
+import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.exc.ValueInstantiationException;
import com.google.common.base.Strings;
@@ -39,6 +40,11 @@ public class BadJsonQueryException extends BadQueryException
this(e, JSON_PARSE_ERROR_CODE, getErrorMessage(e), e.getClass().getName());
}
+ public BadJsonQueryException(JsonMappingException e)
+ {
+ this(e, JSON_PARSE_ERROR_CODE, getMappingErrorMessage(e),
e.getClass().getName());
+ }
+
@JsonCreator
private BadJsonQueryException(
@JsonProperty("error") String errorCode,
@@ -75,4 +81,13 @@ public class BadJsonQueryException extends BadQueryException
}
return "Invalid native query: " + errorMessage;
}
+
+ private static String getMappingErrorMessage(JsonMappingException e)
+ {
+ final String path = e.getPathReference();
+ if (Strings.isNullOrEmpty(path)) {
+ return "Invalid native query: " + e.getOriginalMessage();
+ }
+ return "Invalid native query: " + e.getOriginalMessage() + " (through
reference chain: " + path + ")";
+ }
}
diff --git
a/processing/src/test/java/org/apache/druid/query/BadJsonQueryExceptionTest.java
b/processing/src/test/java/org/apache/druid/query/BadJsonQueryExceptionTest.java
index 2e73eaed002..350d79a3266 100644
---
a/processing/src/test/java/org/apache/druid/query/BadJsonQueryExceptionTest.java
+++
b/processing/src/test/java/org/apache/druid/query/BadJsonQueryExceptionTest.java
@@ -19,8 +19,12 @@
package org.apache.druid.query;
+import com.fasterxml.jackson.databind.JsonMappingException;
+import com.fasterxml.jackson.databind.exc.InvalidTypeIdException;
+import com.fasterxml.jackson.databind.exc.MismatchedInputException;
import com.fasterxml.jackson.databind.exc.ValueInstantiationException;
import com.fasterxml.jackson.databind.type.TypeFactory;
+import org.apache.druid.segment.TestHelper;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
@@ -61,6 +65,55 @@ public class BadJsonQueryExceptionTest
);
}
+ @Test
+ public void testUnknownQueryType()
+ {
+ final BadJsonQueryException exception = readBadQuery(
+
"{\"queryType\":\"noSuchQueryType\",\"dataSource\":\"ds\",\"intervals\":[\"2000/P1D\"]}"
+ );
+
+ Assertions.assertEquals(QueryException.JSON_PARSE_ERROR_CODE,
exception.getErrorCode());
+ Assertions.assertEquals(InvalidTypeIdException.class.getName(),
exception.getErrorClass());
+ // Fails at the root, so there is no reference chain to report.
+ Assertions.assertTrue(exception.getMessage().startsWith("Invalid native
query: "), exception.getMessage());
+ Assertions.assertTrue(
+ exception.getMessage().contains("Could not resolve type id
'noSuchQueryType'"),
+ exception.getMessage()
+ );
+ Assertions.assertFalse(exception.getMessage().contains("reference chain"),
exception.getMessage());
+ }
+
+ @Test
+ public void testFieldOfWrongType()
+ {
+ final BadJsonQueryException exception = readBadQuery(
+
"{\"queryType\":\"timeseries\",\"dataSource\":\"ds\",\"intervals\":[\"2000/P1D\"],\"aggregations\":\"rows\"}"
+ );
+
+ Assertions.assertEquals(QueryException.JSON_PARSE_ERROR_CODE,
exception.getErrorCode());
+ Assertions.assertEquals(MismatchedInputException.class.getName(),
exception.getErrorClass());
+ Assertions.assertTrue(exception.getMessage().startsWith("Invalid native
query: "), exception.getMessage());
+ Assertions.assertTrue(
+ exception.getMessage().endsWith(
+ "(through reference chain:
org.apache.druid.query.timeseries.TimeseriesQuery[\"aggregations\"])"
+ ),
+ exception.getMessage()
+ );
+ }
+
+ /**
+ * Reads a query that is well-formed JSON but cannot be bound to a {@link
Query}, and wraps the resulting
+ * {@link JsonMappingException} the same way QueryResource does.
+ */
+ private static BadJsonQueryException readBadQuery(final String json)
+ {
+ final JsonMappingException e = Assertions.assertThrows(
+ JsonMappingException.class,
+ () -> TestHelper.JSON_MAPPER.readValue(json, Query.class)
+ );
+ return new BadJsonQueryException(e);
+ }
+
private static ValueInstantiationException
valueInstantiationException(Throwable cause)
{
return ValueInstantiationException.from(
diff --git a/server/src/main/java/org/apache/druid/server/QueryResource.java
b/server/src/main/java/org/apache/druid/server/QueryResource.java
index 97679d1edcf..7d15050437c 100644
--- a/server/src/main/java/org/apache/druid/server/QueryResource.java
+++ b/server/src/main/java/org/apache/druid/server/QueryResource.java
@@ -21,8 +21,11 @@ package org.apache.druid.server;
import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.core.JsonParseException;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.SerializerProvider;
+import com.fasterxml.jackson.databind.exc.InvalidDefinitionException;
import com.fasterxml.jackson.databind.exc.ValueInstantiationException;
import com.fasterxml.jackson.jaxrs.smile.SmileMediaTypes;
import com.google.common.annotations.VisibleForTesting;
@@ -287,6 +290,25 @@ public class QueryResource implements
QueryCountStatsProvider
void incrementTimedOut();
}
+ /**
+ * Whether {@code e} wraps an exception that Druid code threw while binding
a query, such as a
+ * {@link NullPointerException} from a deserializer or setter, rather than a
rejection of the input. Jackson wraps
+ * those in a plain {@link JsonMappingException} with the original exception
as its cause, while its own input
+ * errors are subclasses such as {@link
com.fasterxml.jackson.databind.exc.MismatchedInputException}.
+ */
+ private static boolean isServerErrorDuringBinding(final JsonMappingException
e)
+ {
+ final Throwable cause = e.getCause();
+ if (e.getClass() != JsonMappingException.class || cause == null) {
+ return false;
+ }
+ if (cause instanceof IllegalArgumentException || cause instanceof
JsonProcessingException) {
+ return false;
+ }
+ return !(cause instanceof DruidException
+ && ((DruidException) cause).getTargetPersona() ==
DruidException.Persona.USER);
+ }
+
private Query<?> readQuery(
final HttpServletRequest req,
final InputStream in,
@@ -300,6 +322,19 @@ public class QueryResource implements
QueryCountStatsProvider
catch (ValueInstantiationException e) {
throw new BadJsonQueryException(e);
}
+ catch (InvalidDefinitionException e) {
+ // A query class that Jackson cannot bind to is a server-side defect,
not a bad request, so leave it to the
+ // catch-all in doPost to log and return as a 500.
+ throw e;
+ }
+ catch (JsonMappingException e) {
+ if (isServerErrorDuringBinding(e)) {
+ // Leave it to the catch-all in doPost to log and return as a 500.
+ throw e;
+ }
+ // Well-formed JSON that cannot be bound to a query, such as an unknown
query type or a field of the wrong type.
+ throw new BadJsonQueryException(e);
+ }
catch (JsonParseException e) {
throw new BadJsonQueryException(e);
}
diff --git
a/server/src/test/java/org/apache/druid/server/QueryResourceTest.java
b/server/src/test/java/org/apache/druid/server/QueryResourceTest.java
index b37593b14ff..5d228b17adb 100644
--- a/server/src/test/java/org/apache/druid/server/QueryResourceTest.java
+++ b/server/src/test/java/org/apache/druid/server/QueryResourceTest.java
@@ -19,10 +19,18 @@
package org.apache.druid.server;
+import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.JsonMappingException;
import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.exc.InvalidDefinitionException;
+import com.fasterxml.jackson.databind.exc.InvalidTypeIdException;
import com.fasterxml.jackson.databind.exc.ValueInstantiationException;
+import com.fasterxml.jackson.databind.jsontype.NamedType;
+import com.fasterxml.jackson.databind.module.SimpleModule;
import com.fasterxml.jackson.jaxrs.smile.SmileMediaTypes;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
@@ -42,9 +50,11 @@ import org.apache.druid.guice.GuiceInjectors;
import org.apache.druid.guice.annotations.Smile;
import org.apache.druid.jackson.DefaultObjectMapper;
import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.IAE;
import org.apache.druid.java.util.common.Intervals;
import org.apache.druid.java.util.common.StringUtils;
import org.apache.druid.java.util.common.concurrent.Execs;
+import org.apache.druid.java.util.common.granularity.Granularity;
import org.apache.druid.java.util.common.guava.Accumulator;
import org.apache.druid.java.util.common.guava.BaseSequence;
import org.apache.druid.java.util.common.guava.LazySequence;
@@ -1054,6 +1064,83 @@ public class QueryResourceTest
Assertions.assertEquals("Invalid native query: dataSource can't be null",
e.getMessage());
}
+ @Test
+ public void testQueryThatFailsJsonBinding() throws IOException
+ {
+ // Well-formed JSON that cannot be bound to a query is a user error, not a
500 from the catch-all handler.
+ // The error message itself is covered by BadJsonQueryExceptionTest.
+ final Response response = queryResource.doPost(
+ new ByteArrayInputStream(
+ StringUtils.replace(SIMPLE_TIMESERIES_QUERY, "\"timeseries\"",
"\"noSuchQueryType\"")
+ .getBytes(StandardCharsets.UTF_8)
+ ),
+ null /*pretty*/,
+ testServletRequest
+ );
+ Assertions.assertNotNull(response);
+ Assertions.assertEquals(Status.BAD_REQUEST.getStatusCode(),
response.getStatus());
+ final QueryException e = jsonMapper.readValue((byte[])
response.getEntity(), QueryException.class);
+ Assertions.assertEquals(QueryException.JSON_PARSE_ERROR_CODE,
e.getErrorCode());
+ Assertions.assertEquals(InvalidTypeIdException.class.getName(),
e.getErrorClass());
+ }
+
+ @Test
+ public void testQueryWithBrokenDefinitionIsServerError() throws IOException
+ {
+ // A registered query type that Jackson cannot instantiate is a
server-side defect, so it must not be reported
+ // as a bad request.
+ jsonMapper = jsonMapper.copy();
+ jsonMapper.registerSubtypes(new NamedType(BrokenQuery.class,
"brokenQuery"));
+ queryResource = createQueryResource(ResponseContextConfig.newConfig(true));
+ final MockHttpServletResponse asyncResponse =
MockHttpServletResponse.forRequest(testServletRequest);
+ testServletRequest.remoteHost = "localhost";
+
+ final Response response = queryResource.doPost(
+ new ByteArrayInputStream(
+ StringUtils.replace(SIMPLE_TIMESERIES_QUERY, "\"timeseries\"",
"\"brokenQuery\"")
+ .getBytes(StandardCharsets.UTF_8)
+ ),
+ null /*pretty*/,
+ testServletRequest
+ );
+ Assertions.assertNull(response);
+ Assertions.assertEquals(Status.INTERNAL_SERVER_ERROR.getStatusCode(),
asyncResponse.getStatus());
+ final QueryException e =
jsonMapper.readValue(asyncResponse.baos.toByteArray(), QueryException.class);
+ Assertions.assertEquals(QueryException.UNKNOWN_EXCEPTION_ERROR_CODE,
e.getErrorCode());
+ Assertions.assertEquals(InvalidDefinitionException.class.getName(),
e.getErrorClass());
+ }
+
+ @Test
+ public void testBadInputRejectedByDeserializerIsBadRequest() throws
IOException
+ {
+ for (final RuntimeException exception : List.of(
+ new IAE("bad granularity"),
+ InvalidInput.exception("bad granularity")
+ )) {
+ final Response response =
postWithFailingGranularityDeserializer(exception);
+ Assertions.assertNotNull(response);
+ Assertions.assertEquals(Status.BAD_REQUEST.getStatusCode(),
response.getStatus());
+ final QueryException e = jsonMapper.readValue((byte[])
response.getEntity(), QueryException.class);
+ Assertions.assertEquals(QueryException.JSON_PARSE_ERROR_CODE,
e.getErrorCode());
+ Assertions.assertEquals(JsonMappingException.class.getName(),
e.getErrorClass());
+ Assertions.assertTrue(e.getMessage().contains("bad granularity"),
e.getMessage());
+ }
+ }
+
+ @Test
+ public void testBugInDeserializerIsServerError() throws IOException
+ {
+ final MockHttpServletResponse asyncResponse =
MockHttpServletResponse.forRequest(testServletRequest);
+ testServletRequest.remoteHost = "localhost";
+
+ final Response response = postWithFailingGranularityDeserializer(new
NullPointerException());
+ Assertions.assertNull(response);
+ Assertions.assertEquals(Status.INTERNAL_SERVER_ERROR.getStatusCode(),
asyncResponse.getStatus());
+ final QueryException e =
jsonMapper.readValue(asyncResponse.baos.toByteArray(), QueryException.class);
+ Assertions.assertEquals(QueryException.UNKNOWN_EXCEPTION_ERROR_CODE,
e.getErrorCode());
+ Assertions.assertEquals(JsonMappingException.class.getName(),
e.getErrorClass());
+ }
+
@Test
public void testResourceLimitExceeded() throws IOException
{
@@ -2119,4 +2206,39 @@ public class QueryResourceTest
{
return queryResource.doPost(new ByteArrayInputStream(bytes), null, req);
}
+
+ /**
+ * Posts {@link #SIMPLE_TIMESERIES_QUERY} with a granularity deserializer
that throws {@code exception}. Jackson wraps
+ * it in a plain {@link JsonMappingException}, as it does for any exception
thrown while deserializing a property.
+ */
+ private Response postWithFailingGranularityDeserializer(final
RuntimeException exception) throws IOException
+ {
+ jsonMapper = jsonMapper.copy();
+ jsonMapper.registerModule(
+ new SimpleModule().addDeserializer(
+ Granularity.class,
+ new JsonDeserializer<>()
+ {
+ @Override
+ public Granularity deserialize(final JsonParser p, final
DeserializationContext ctxt)
+ {
+ throw exception;
+ }
+ }
+ )
+ );
+ queryResource = createQueryResource(ResponseContextConfig.newConfig(true));
+ return queryResource.doPost(
+ new
ByteArrayInputStream(SIMPLE_TIMESERIES_QUERY.getBytes(StandardCharsets.UTF_8)),
+ null /*pretty*/,
+ testServletRequest
+ );
+ }
+
+ /**
+ * A query type with no creator, which Jackson rejects with an {@link
InvalidDefinitionException}.
+ */
+ private abstract static class BrokenQuery implements Query<Object>
+ {
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
