clintropolis commented on a change in pull request #11711:
URL: https://github.com/apache/druid/pull/11711#discussion_r712588136
##########
File path: sql/src/main/java/org/apache/druid/sql/http/SqlResource.java
##########
@@ -170,39 +174,50 @@ public Response doPost(
}
catch (QueryCapacityExceededException cap) {
endLifecycle(sqlQueryId, lifecycle, cap, remoteAddr, -1);
- return buildNonOkResponse(QueryCapacityExceededException.STATUS_CODE,
cap);
+ return buildNonOkResponse(
+ QueryCapacityExceededException.STATUS_CODE,
+
serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(cap)
+ );
}
catch (QueryUnsupportedException unsupported) {
endLifecycle(sqlQueryId, lifecycle, unsupported, remoteAddr, -1);
- return buildNonOkResponse(QueryUnsupportedException.STATUS_CODE,
unsupported);
+ return buildNonOkResponse(
+ QueryUnsupportedException.STATUS_CODE,
+
serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(unsupported)
+ );
}
catch (QueryTimeoutException timeout) {
endLifecycle(sqlQueryId, lifecycle, timeout, remoteAddr, -1);
- return buildNonOkResponse(QueryTimeoutException.STATUS_CODE, timeout);
+ return buildNonOkResponse(
+ QueryTimeoutException.STATUS_CODE,
+
serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(timeout)
+ );
}
catch (SqlPlanningException | ResourceLimitExceededException e) {
endLifecycle(sqlQueryId, lifecycle, e, remoteAddr, -1);
- return buildNonOkResponse(BadQueryException.STATUS_CODE, e);
+ return buildNonOkResponse(
+ BadQueryException.STATUS_CODE,
+ serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(e)
+ );
}
catch (ForbiddenException e) {
endLifecycleWithoutEmittingMetrics(sqlQueryId, lifecycle);
- throw e; // let ForbiddenExceptionMapper handle this
+ throw (ForbiddenException)
serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(e); // let
ForbiddenExceptionMapper handle this
}
catch (Exception e) {
log.warn(e, "Failed to handle query: %s", sqlQuery);
endLifecycle(sqlQueryId, lifecycle, e, remoteAddr, -1);
- final Exception exceptionToReport;
+ QueryInterruptedException exceptionToReport;
if (e instanceof RelOptPlanner.CannotPlanException) {
- exceptionToReport = new ISE("Cannot build plan for query: %s",
sqlQuery.getQuery());
+ exceptionToReport = QueryInterruptedException.wrapIfNeeded((new
ISE("Cannot build plan for query: %s", sqlQuery.getQuery())));
Review comment:
nit: this isn't new or your change, but I think it is semi confusing
that there exists a`SqlPlanningException` that isn't being used here. The
reason for this is because I think that this is a different kind of planning
exception - one that occurs for some query which we couldn't translate to a
native Druid query, which we seem to be treating as a server-side error, where
the other one is currently used for SQL parsing and validation errors and
considered client side errors since they are things the user can fix.
I wonder if we should have some sort of unsupported query exception to give
this a bit more prominence, but I think that is a tangential discussion to this
PRs changed.
##########
File path:
core/src/main/java/org/apache/druid/common/exception/NoErrorResponseTransformStrategy.java
##########
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.common.exception;
+
+import org.apache.druid.java.util.common.IAE;
+
+import java.util.function.Function;
+
+/**
+ * Error response transform strategy that does nothing and simply return the
same Exception back without any change
+ */
+public class NoErrorResponseTransformStrategy implements
ErrorResponseTransformStrategy
+{
+ public static final NoErrorResponseTransformStrategy INSTANCE = new
NoErrorResponseTransformStrategy();
+
+ @Override
+ public Exception transformIfNeeded(SanitizableException exception)
+ {
+ if (exception instanceof Exception) {
+ return (Exception) exception;
+ } else {
+ return new IAE(
+ "Cannot sanitize error response as given argument[%s] is not an
Exception",
+ exception.getClass().getName()
+ );
+ }
Review comment:
why not just `return exception;` and be a no-op like the javadoc says?
##########
File path: sql/src/main/java/org/apache/druid/sql/http/SqlResource.java
##########
@@ -170,39 +174,50 @@ public Response doPost(
}
catch (QueryCapacityExceededException cap) {
endLifecycle(sqlQueryId, lifecycle, cap, remoteAddr, -1);
- return buildNonOkResponse(QueryCapacityExceededException.STATUS_CODE,
cap);
+ return buildNonOkResponse(
+ QueryCapacityExceededException.STATUS_CODE,
+
serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(cap)
+ );
}
catch (QueryUnsupportedException unsupported) {
endLifecycle(sqlQueryId, lifecycle, unsupported, remoteAddr, -1);
- return buildNonOkResponse(QueryUnsupportedException.STATUS_CODE,
unsupported);
+ return buildNonOkResponse(
+ QueryUnsupportedException.STATUS_CODE,
+
serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(unsupported)
+ );
}
catch (QueryTimeoutException timeout) {
endLifecycle(sqlQueryId, lifecycle, timeout, remoteAddr, -1);
- return buildNonOkResponse(QueryTimeoutException.STATUS_CODE, timeout);
+ return buildNonOkResponse(
+ QueryTimeoutException.STATUS_CODE,
+
serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(timeout)
+ );
}
catch (SqlPlanningException | ResourceLimitExceededException e) {
endLifecycle(sqlQueryId, lifecycle, e, remoteAddr, -1);
- return buildNonOkResponse(BadQueryException.STATUS_CODE, e);
+ return buildNonOkResponse(
+ BadQueryException.STATUS_CODE,
+ serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(e)
+ );
}
catch (ForbiddenException e) {
endLifecycleWithoutEmittingMetrics(sqlQueryId, lifecycle);
- throw e; // let ForbiddenExceptionMapper handle this
+ throw (ForbiddenException)
serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(e); // let
ForbiddenExceptionMapper handle this
Review comment:
at some point in the future, assuming we eventually make this
functionality apply to all API response errors, it probably makes sense to push
this transformation functionality into the error handlers so that we everything
that lets exceptions fall through doesn't need to do this itself. It is
probably ok to leave here for now since this is the only thing doing it
##########
File path: sql/src/main/java/org/apache/druid/sql/http/SqlResource.java
##########
@@ -170,39 +174,50 @@ public Response doPost(
}
catch (QueryCapacityExceededException cap) {
endLifecycle(sqlQueryId, lifecycle, cap, remoteAddr, -1);
- return buildNonOkResponse(QueryCapacityExceededException.STATUS_CODE,
cap);
+ return buildNonOkResponse(
Review comment:
it looks like all callers of `buildNonOkResponse` are calling
`transformIfNeeded`, it makes sense to update the signature to just accept
`SanitizableException`
##########
File path:
server/src/main/java/org/apache/druid/server/initialization/ServerConfig.java
##########
@@ -235,14 +257,15 @@ public boolean equals(Object o)
queueSize == that.queueSize &&
enableRequestLimit == that.enableRequestLimit &&
defaultQueryTimeout == that.defaultQueryTimeout &&
- maxScatterGatherBytes.equals(that.maxScatterGatherBytes) &&
maxSubqueryRows == that.maxSubqueryRows &&
maxQueryTimeout == that.maxQueryTimeout &&
maxRequestHeaderSize == that.maxRequestHeaderSize &&
inflateBufferSize == that.inflateBufferSize &&
compressionLevel == that.compressionLevel &&
enableForwardedRequestCustomizer ==
that.enableForwardedRequestCustomizer &&
+ sanitizeJettyErrorResponse == that.sanitizeJettyErrorResponse &&
Review comment:
hm, I wonder if technically `ErrorResponseTransformStrategy` should
implement equals and be checked here
##########
File path:
core/src/main/java/org/apache/druid/common/exception/SanitizableException.java
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.common.exception;
+
+import java.util.function.Function;
+
+public interface SanitizableException
+{
+ /**
+ * Apply the function for transforming the error message then
+ * return new Exception with sanitized fields and transformed message.
+ */
+ Exception applyErrorMessageTransformAndSanitizeFields(
Review comment:
nit: i think `transform` would be descriptive enough of a method name,
between the parameter name and the javadocs.
##########
File path:
core/src/main/java/org/apache/druid/common/exception/SanitizableException.java
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.common.exception;
+
+import java.util.function.Function;
+
+public interface SanitizableException
Review comment:
nit: if a longer term goal is to make sure all surface exceptions are
wrapped in this to allow operators to tidy up the error responses, i wonder if
we should take a stronger name for this interface like `ServiceApiException` or
.. idk naming is hard.
##########
File path: sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java
##########
@@ -968,6 +973,44 @@ public void testUnsupportedQueryThrowsException() throws
Exception
Assert.assertTrue(lifecycleManager.getAll("id").isEmpty());
}
+ @Test
+ public void testUnsupportedQueryThrowsExceptionWithFilterResponse() throws
Exception
+ {
+ resource = new SqlResource(
+ JSON_MAPPER,
+ CalciteTests.TEST_AUTHORIZER_MAPPER,
+ sqlLifecycleFactory,
+ lifecycleManager,
+ new ServerConfig() {
+ @Override
+ public boolean isSanitizeJettyErrorResponse()
+ {
+ return true;
+ }
+ @Override
+ public ErrorResponseTransformStrategy
getErrorResponseTransformStrategy()
+ {
+ return new
AllowedRegexErrorResponseTransformStrategy(ImmutableList.of());
+ }
+ }
+ );
+
+ String errorMessage = "This will be support in Druid 9999";
Review comment:
<img src="https://i.imgur.com/VlP5fgB.gif";></img>
##########
File path: sql/src/main/java/org/apache/druid/sql/http/SqlResource.java
##########
@@ -170,39 +174,50 @@ public Response doPost(
}
catch (QueryCapacityExceededException cap) {
endLifecycle(sqlQueryId, lifecycle, cap, remoteAddr, -1);
- return buildNonOkResponse(QueryCapacityExceededException.STATUS_CODE,
cap);
+ return buildNonOkResponse(
+ QueryCapacityExceededException.STATUS_CODE,
+
serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(cap)
+ );
}
catch (QueryUnsupportedException unsupported) {
endLifecycle(sqlQueryId, lifecycle, unsupported, remoteAddr, -1);
- return buildNonOkResponse(QueryUnsupportedException.STATUS_CODE,
unsupported);
+ return buildNonOkResponse(
+ QueryUnsupportedException.STATUS_CODE,
+
serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(unsupported)
+ );
}
catch (QueryTimeoutException timeout) {
endLifecycle(sqlQueryId, lifecycle, timeout, remoteAddr, -1);
- return buildNonOkResponse(QueryTimeoutException.STATUS_CODE, timeout);
+ return buildNonOkResponse(
+ QueryTimeoutException.STATUS_CODE,
+
serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(timeout)
+ );
}
catch (SqlPlanningException | ResourceLimitExceededException e) {
endLifecycle(sqlQueryId, lifecycle, e, remoteAddr, -1);
- return buildNonOkResponse(BadQueryException.STATUS_CODE, e);
+ return buildNonOkResponse(
+ BadQueryException.STATUS_CODE,
+ serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(e)
+ );
}
catch (ForbiddenException e) {
endLifecycleWithoutEmittingMetrics(sqlQueryId, lifecycle);
- throw e; // let ForbiddenExceptionMapper handle this
+ throw (ForbiddenException)
serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(e); // let
ForbiddenExceptionMapper handle this
Review comment:
in fact, whenever in the future we do that, it might also make sense to
move `QueryException` handling out to an `ExceptionMapper<QueryException>` and
just rethrow all exceptions so that error response handling can be shared
between sql and native json queries. it doesn't gain too much since will still
need to handle internally so that metrics can be emitted, but it would cover
the response writing.
--
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]
![https://i.imgur.com/VlP5fgB.gif"](https://i.imgur.com/VlP5fgB.gif"){kind=link}