clintropolis commented on a change in pull request #11711:
URL: https://github.com/apache/druid/pull/11711#discussion_r709885043
##########
File path:
services/src/main/java/org/apache/druid/server/AsyncQueryForwardingServlet.java
##########
@@ -350,10 +369,18 @@ private void handleQueryParseException(
// Write to the response
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
response.setContentType(MediaType.APPLICATION_JSON);
- objectMapper.writeValue(
- response.getOutputStream(),
- ImmutableMap.of("error", errorMessage)
- );
+ if (serverConfig.isFilterResponse()
+ && serverConfig.getResponseWhitelistRegex().stream().noneMatch(pattern
-> pattern.matcher(errorMessage).matches())) {
+ objectMapper.writeValue(
+ response.getOutputStream(),
+ ImmutableMap.of("error", DEFAULT_QUERY_PARSE_EXCEPTION_MESSAGE)
Review comment:
Maybe it is finally time to make a basic `ErrorResponse` class to
capture this `ImmutableMap.of("error", ...)` pattern, searching the code-base I
see it in a lot of places...
On the other hand, it is somewhat inconsistent with `QueryException` though,
which uses `error` for the errorCode and has separate message, class, and some
additional information. Since this is specifically forwarding queries, I wonder
if this should be a type of `QueryException`? (Especially if we bake a method
into `QueryException` to make them clean themselves up if configured as such)
##########
File path:
server/src/main/java/org/apache/druid/server/initialization/ServerConfig.java
##########
@@ -145,6 +150,13 @@ public ServerConfig()
@NotNull
private List<String> allowedHttpMethods = ImmutableList.of();
+ @JsonProperty
+ @NotNull
+ private List<Pattern> responseWhitelistRegex = ImmutableList.of();
Review comment:
this config applies to the http servers, not queries specifically, so I
think if it is going to live here it might need a name that indicates it only
applies to queries, unless you imagine that at some point this setting will
apply to all HTTP error responses?
Alternatively, if it lived on a more query centric config object this
wouldn't be necessary, though i'm not quite sure off the top of my head what is
most appropriate.
##########
File path:
server/src/test/java/org/apache/druid/initialization/ServerConfigTest.java
##########
@@ -57,13 +58,16 @@ public void testSerde() throws Exception
defaultConfig.getInflateBufferSize(),
defaultConfig.getCompressionLevel(),
true,
- ImmutableList.of(HttpMethod.OPTIONS)
+ ImmutableList.of(HttpMethod.OPTIONS),
+ true,
+ ImmutableList.of(Pattern.compile(".*"))
);
String modifiedConfigJson =
OBJECT_MAPPER.writeValueAsString(modifiedConfig);
ServerConfig modifiedConfig2 = OBJECT_MAPPER.readValue(modifiedConfigJson,
ServerConfig.class);
Assert.assertEquals(modifiedConfig, modifiedConfig2);
Assert.assertEquals(999, modifiedConfig2.getNumThreads());
- Assert.assertEquals(888, modifiedConfig2.getQueueSize());
Review comment:
nit: why remove?
##########
File path: sql/src/main/java/org/apache/druid/sql/http/SqlResource.java
##########
@@ -271,4 +332,14 @@ public Response cancelQuery(
return Response.status(Status.FORBIDDEN).build();
}
}
+
+ @VisibleForTesting
+ String applyErrorMessageFilter(final String errorMessage, final
List<Pattern> whitelistRegex)
Review comment:
Maybe this should be a utility method on `QueryException` or someplace
similar so it can be better shared for when JDBC and native JSON queries are
added (also a few other places i see this `.stream().noneMatch(pattern ->
pattern.matcher(errorMessage).matches()))` pattern in this PR that could
probably be shared). If you imagine this will only ever be done to queries,
maybe it makes sense to just bake method into `QueryException` so that all
implementations just know how to trim the information they contain, then
everything just has to make sure they throw some sort of `QueryException` and
everything should be good.
Thinking further though, if this functionality only applies to queries, do
we really need to use regex? Could we instead just configure a set of exception
message by errorCode or maybe the classes of `QueryException` (not errorClass,
but like java class name) to allow, and anything that doesn't match would be
sanitized? I worry that regex based on the error messages might be a bit
tricky/steep learning curve, and if we go this way we should probably update
the documents to include several examples of the types of messages that query
errors can encounter, and example configs.
--
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]
