diff --git a/docs/configuration/index.md b/docs/configuration/index.md index f46f919e4546..f31dabef8c4e 100644 --- a/docs/configuration/index.md +++ b/docs/configuration/index.md @@ -631,7 +631,7 @@ On the other hand, if `druid.server.http.errorResponseTransform.allowedRegex` is ##### Persona based error response transform strategy -In this mode, Druid transforms any exceptions which are targeted at non-users personas. Instead of returning such exception directly, the strategy logs the exception against a random id and returns the id along with a generic error message to the user. +In this mode, Druid transforms any exceptions which are targeted at non-users personas. Instead of returning such exception directly, Druid logs the exception against an error ID, and returns the ID along with a generic error message to the user. A user could then share the ID with an operator to assist in troubleshooting further. Errors that users can reasonably understand and potentially act on, such as invalid queries, query timeouts, and capacity limits, are returned unchanged. To enable this strategy, set `druid.server.http.errorResponseTransform.strategy` to `persona`. diff --git a/embedded-tests/src/test/java/org/apache/druid/testing/embedded/msq/EmbeddedDartErrorResponseTransformTest.java b/embedded-tests/src/test/java/org/apache/druid/testing/embedded/msq/EmbeddedDartErrorResponseTransformTest.java new file mode 100644 index 000000000000..d315ec1c09f3 --- /dev/null +++ b/embedded-tests/src/test/java/org/apache/druid/testing/embedded/msq/EmbeddedDartErrorResponseTransformTest.java @@ -0,0 +1,106 @@ +/* + * 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.testing.embedded.msq; + +import com.google.common.base.Throwables; +import org.apache.druid.msq.dart.controller.sql.DartSqlEngine; +import org.apache.druid.query.QueryContexts; +import org.apache.druid.query.http.ClientSqlQuery; +import org.apache.druid.rpc.HttpResponseException; +import org.apache.druid.sql.http.ResultFormat; +import org.apache.druid.testing.embedded.EmbeddedBroker; +import org.apache.druid.testing.embedded.EmbeddedCoordinator; +import org.apache.druid.testing.embedded.EmbeddedDruidCluster; +import org.apache.druid.testing.embedded.EmbeddedHistorical; +import org.apache.druid.testing.embedded.junit5.EmbeddedClusterTestBase; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.Map; + +/** + * Dart controllers here have too little memory, so every Dart query fails during execution with an operator fault. + */ +public class EmbeddedDartErrorResponseTransformTest extends EmbeddedClusterTestBase +{ + private final EmbeddedBroker personaBroker = new EmbeddedBroker(); + private final EmbeddedBroker defaultBroker = new EmbeddedBroker(); + private final EmbeddedHistorical historical = new EmbeddedHistorical(); + private final EmbeddedCoordinator coordinator = new EmbeddedCoordinator(); + + @Override + protected EmbeddedDruidCluster createCluster() + { + personaBroker.addProperty("druid.msq.dart.controller.heapFraction", "0.000001") + .addProperty("druid.server.http.errorResponseTransform.strategy", "persona") + .addProperty("druid.plaintextPort", "7082"); + defaultBroker.addProperty("druid.msq.dart.controller.heapFraction", "0.000001") + .addProperty("druid.plaintextPort", "7083"); + + return EmbeddedDruidCluster.withEmbeddedDerbyAndZookeeper() + .addCommonProperty("druid.msq.dart.enabled", "true") + .useLatchableEmitter() + .addServer(coordinator) + .addServer(personaBroker) + .addServer(defaultBroker) + .addServer(historical); + } + + @Test + public void test_dartExecutionFailure_isHiddenByPersonaStrategy() + { + final HttpResponseException e = runFailingDartQuery(personaBroker, "persona-query"); + + Assertions.assertEquals(500, e.getResponse().getStatus().code()); + Assertions.assertTrue(e.getMessage().contains("Error ID [persona-query]"), e.getMessage()); + Assertions.assertFalse(e.getMessage().contains("NotEnoughMemory"), e.getMessage()); + Assertions.assertFalse(e.getMessage().contains("exceptionStackTrace"), e.getMessage()); + } + + @Test + public void test_dartExecutionFailure_isReturnedUnchangedByDefaultStrategy() + { + final HttpResponseException e = runFailingDartQuery(defaultBroker, "default-query"); + + Assertions.assertTrue(e.getMessage().contains("NotEnoughMemory"), e.getMessage()); + } + + private HttpResponseException runFailingDartQuery(final EmbeddedBroker broker, final String sqlQueryId) + { + final Exception e = Assertions.assertThrows( + Exception.class, + () -> cluster.callApi().onTargetBroker( + broker, + b -> b.submitSqlQuery( + new ClientSqlQuery( + "SELECT 1", + ResultFormat.CSV.name(), + false, + false, + false, + Map.of(QueryContexts.ENGINE, DartSqlEngine.NAME, QueryContexts.CTX_SQL_QUERY_ID, sqlQueryId), + null + ) + ) + ) + ); + return Assertions.assertInstanceOf(HttpResponseException.class, Throwables.getRootCause(e)); + } +} diff --git a/processing/src/main/java/org/apache/druid/common/exception/PersonaBasedErrorTransformStrategy.java b/processing/src/main/java/org/apache/druid/common/exception/PersonaBasedErrorTransformStrategy.java index 3f83dc592f1e..df25a86fb973 100644 --- a/processing/src/main/java/org/apache/druid/common/exception/PersonaBasedErrorTransformStrategy.java +++ b/processing/src/main/java/org/apache/druid/common/exception/PersonaBasedErrorTransformStrategy.java @@ -20,7 +20,12 @@ package org.apache.druid.common.exception; import org.apache.druid.error.DruidException; +import org.apache.druid.error.QueryExceptionCompat; +import org.apache.druid.java.util.common.ISE; import org.apache.druid.java.util.common.StringUtils; +import org.apache.druid.java.util.common.UOE; +import org.apache.druid.java.util.common.logger.Logger; +import org.apache.druid.query.QueryException; import java.util.Optional; import java.util.UUID; @@ -33,6 +38,7 @@ */ public class PersonaBasedErrorTransformStrategy implements ErrorResponseTransformStrategy { + private static final Logger log = new Logger(PersonaBasedErrorTransformStrategy.class); private static final String ERROR_WITH_ID_TEMPLATE = "Internal server error, please contact your administrator " + "with Error ID [%s] if the issue persists."; public static final PersonaBasedErrorTransformStrategy INSTANCE = new PersonaBasedErrorTransformStrategy(); @@ -54,10 +60,51 @@ public Optional maybeTransform(DruidException druidException, Op .build(StringUtils.format(ERROR_WITH_ID_TEMPLATE, errorId))); } + /** + * Hides a {@link SanitizableException} that is not meant for users in the same way as {@link #maybeTransform}, and + * logs it against the generated error id. See {@link #shouldHide} for which exceptions are hidden. + */ + @Override + public Exception transformIfNeeded(SanitizableException exception) + { + if (!shouldHide(exception)) { + return (Exception) exception; + } + final String errorId = UUID.randomUUID().toString(); + log.error((Throwable) exception, "External Error ID: [%s]", errorId); + return exception.sanitize(message -> StringUtils.format(ERROR_WITH_ID_TEMPLATE, errorId)); + } + + /** + * Not used, since {@link #transformIfNeeded} decides how to transform each exception. + */ @Override public Function getErrorMessageTransformFunction() { - throw new UnsupportedOperationException(); + return Function.identity(); + } + + /** + * Whether a {@link SanitizableException} is hidden from the client: + * + */ + private static boolean shouldHide(SanitizableException exception) + { + final Throwable cause = ((Throwable) exception).getCause(); + if (cause instanceof DruidException) { + return ((DruidException) cause).getTargetPersona() != DruidException.Persona.USER; + } else if (exception instanceof QueryException) { + return QueryExceptionCompat.getPersona(((QueryException) exception).getFailType()) != DruidException.Persona.USER; + } else { + return exception instanceof ISE; + } } @Override diff --git a/processing/src/main/java/org/apache/druid/error/QueryExceptionCompat.java b/processing/src/main/java/org/apache/druid/error/QueryExceptionCompat.java index 5782ca55ee0f..df888bc41b9f 100644 --- a/processing/src/main/java/org/apache/druid/error/QueryExceptionCompat.java +++ b/processing/src/main/java/org/apache/druid/error/QueryExceptionCompat.java @@ -45,7 +45,7 @@ public QueryExceptionCompat( @Override protected DruidException makeException(DruidException.DruidExceptionBuilder bob) { - return bob.forPersona(DruidException.Persona.OPERATOR) + return bob.forPersona(getPersona(exception.getFailType())) .ofCategory(convertFailType(exception.getFailType())) .build(exception, "%s", exception.getMessage()) .withContext("host", exception.getHost()) @@ -53,6 +53,20 @@ protected DruidException makeException(DruidException.DruidExceptionBuilder bob) .withContext("legacyErrorCode", exception.getErrorCode()); } + /** + * Returns the persona that a {@link QueryException} with the given {@link QueryException.FailType} targets. Failures + * the user can act on, such as invalid queries, timeouts and capacity limits, target + * {@link DruidException.Persona#USER}. Runtime failures and unknown errors target + * {@link DruidException.Persona#OPERATOR}. + */ + public static DruidException.Persona getPersona(QueryException.FailType failType) + { + return switch (failType) { + case USER_ERROR, UNAUTHORIZED, CAPACITY_EXCEEDED, CANCELED, UNSUPPORTED, TIMEOUT -> DruidException.Persona.USER; + default -> DruidException.Persona.OPERATOR; + }; + } + private DruidException.Category convertFailType(QueryException.FailType failType) { switch (failType) { diff --git a/processing/src/test/java/org/apache/druid/common/exception/PersonaBasedErrorTransformStrategyTest.java b/processing/src/test/java/org/apache/druid/common/exception/PersonaBasedErrorTransformStrategyTest.java index 6fb18d27e35b..7df66f37b296 100644 --- a/processing/src/test/java/org/apache/druid/common/exception/PersonaBasedErrorTransformStrategyTest.java +++ b/processing/src/test/java/org/apache/druid/common/exception/PersonaBasedErrorTransformStrategyTest.java @@ -22,6 +22,11 @@ import nl.jqno.equalsverifier.EqualsVerifier; import org.apache.druid.error.DruidException; import org.apache.druid.error.DruidExceptionMatcher; +import org.apache.druid.java.util.common.ISE; +import org.apache.druid.java.util.common.UOE; +import org.apache.druid.query.QueryException; +import org.apache.druid.query.QueryInterruptedException; +import org.apache.druid.query.QueryTimeoutException; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -79,6 +84,56 @@ public void testErrorIdIsGeneratedWhenAbsent() ); } + @Test + public void testUserQueryExceptionRemainsUnchanged() + { + final QueryTimeoutException exception = new QueryTimeoutException("Query timed out"); + Assertions.assertSame(exception, target.transformIfNeeded(exception)); + } + + @Test + public void testOperatorQueryExceptionIsTransformed() + { + final Exception transformed = + target.transformIfNeeded(new QueryInterruptedException(new RuntimeException("internal detail"))); + + final QueryException queryException = Assertions.assertInstanceOf(QueryException.class, transformed); + Assertions.assertEquals(QueryException.UNKNOWN_EXCEPTION_ERROR_CODE, queryException.getErrorCode()); + Assertions.assertTrue( + queryException.getMessage().contains("please contact your administrator with Error ID ["), + queryException.getMessage() + ); + Assertions.assertNull(queryException.getErrorClass()); + Assertions.assertNull(queryException.getHost()); + } + + @Test + public void testQueryExceptionWrappingUserDruidExceptionRemainsUnchanged() + { + final QueryInterruptedException exception = QueryInterruptedException.wrapIfNeeded( + DruidException.forPersona(DruidException.Persona.USER) + .ofCategory(DruidException.Category.INVALID_INPUT) + .build("bad interval") + ); + Assertions.assertSame(exception, target.transformIfNeeded(exception)); + } + + @Test + public void testIllegalStateExceptionIsTransformed() + { + final Exception transformed = target.transformIfNeeded(new ISE("internal detail")); + + final ISE ise = Assertions.assertInstanceOf(ISE.class, transformed); + Assertions.assertTrue(ise.getMessage().contains("please contact your administrator with Error ID ["), ise.getMessage()); + } + + @Test + public void testUnsupportedOperationExceptionRemainsUnchanged() + { + final UOE exception = new UOE("Batch statements not supported"); + Assertions.assertSame(exception, target.transformIfNeeded(exception)); + } + @Test public void testEqualsAndHashCode() { diff --git a/processing/src/test/java/org/apache/druid/error/ErrorResponseTest.java b/processing/src/test/java/org/apache/druid/error/ErrorResponseTest.java index a5f1874d4bd9..eb38798e8a47 100644 --- a/processing/src/test/java/org/apache/druid/error/ErrorResponseTest.java +++ b/processing/src/test/java/org/apache/druid/error/ErrorResponseTest.java @@ -76,7 +76,7 @@ public void testQueryExceptionCompat() "legacyQueryException", "persona", - "OPERATOR", + "USER", "category", "TIMEOUT", @@ -103,7 +103,7 @@ public void testQueryExceptionCompat() DruidExceptionMatcher.assertThat( recomposed.getUnderlyingException(), - new DruidExceptionMatcher(DruidException.Persona.OPERATOR, DruidException.Category.TIMEOUT, "legacyQueryException") + new DruidExceptionMatcher(DruidException.Persona.USER, DruidException.Category.TIMEOUT, "legacyQueryException") .expectMessageIs("Query did not complete within configured timeout period. You can increase query timeout or tune the performance of query.") ); } @@ -125,7 +125,7 @@ public void testQueryExceptionCompatWithNullMessage() "legacyQueryException", "persona", - "OPERATOR", + "USER", "category", "TIMEOUT", 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 97679d1edcf2..ede457ab3cea 100644 --- a/server/src/main/java/org/apache/druid/server/QueryResource.java +++ b/server/src/main/java/org/apache/druid/server/QueryResource.java @@ -173,7 +173,10 @@ public Response doPost( query = readQuery(req, in, io); } catch (QueryException e) { - return io.getResponseWriter().buildNonOkResponse(e.getFailType().getExpectedStatus(), e); + return io.getResponseWriter().buildNonOkResponse( + e.getFailType().getExpectedStatus(), + serverConfig.getErrorResponseTransformStrategy().transformIfNeeded(e) + ); } final QueryLifecycle queryLifecycle = queryLifecycleFactory.factorize(); diff --git a/server/src/main/java/org/apache/druid/server/QueryResourceQueryResultPusherFactory.java b/server/src/main/java/org/apache/druid/server/QueryResourceQueryResultPusherFactory.java index a9d70c9ac09c..9ed1473e3536 100644 --- a/server/src/main/java/org/apache/druid/server/QueryResourceQueryResultPusherFactory.java +++ b/server/src/main/java/org/apache/druid/server/QueryResourceQueryResultPusherFactory.java @@ -23,9 +23,11 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableMap; import com.google.inject.Inject; +import org.apache.druid.common.exception.ErrorResponseTransformStrategy; import org.apache.druid.guice.annotations.Json; import org.apache.druid.guice.annotations.Self; import org.apache.druid.query.context.ResponseContext; +import org.apache.druid.server.initialization.ServerConfig; import javax.servlet.http.HttpServletRequest; import javax.ws.rs.core.MediaType; @@ -41,17 +43,20 @@ public class QueryResourceQueryResultPusherFactory protected final ObjectMapper jsonMapper; private final ResponseContextConfig responseContextConfig; private final DruidNode selfNode; + private final ServerConfig serverConfig; @Inject public QueryResourceQueryResultPusherFactory( @Json ObjectMapper jsonMapper, ResponseContextConfig responseContextConfig, - @Self DruidNode selfNode + @Self DruidNode selfNode, + ServerConfig serverConfig ) { this.jsonMapper = jsonMapper; this.responseContextConfig = responseContextConfig; this.selfNode = selfNode; + this.serverConfig = serverConfig; } /** @@ -71,7 +76,8 @@ public QueryResourceQueryResultPusher factorize( counter, req, queryLifecycle, - io + io, + serverConfig.getErrorResponseTransformStrategy() ); } @@ -94,7 +100,8 @@ public QueryResourceQueryResultPusher( final QueryResource.QueryMetricCounter counter, final HttpServletRequest req, final QueryLifecycle queryLifecycle, - final ResourceIOReaderWriterFactory.ResourceIOReaderWriter io + final ResourceIOReaderWriterFactory.ResourceIOReaderWriter io, + final ErrorResponseTransformStrategy errorResponseTransformStrategy ) { super( @@ -106,7 +113,8 @@ public QueryResourceQueryResultPusher( queryLifecycle.getQueryId(), MediaType.valueOf(io.getResponseWriter().getResponseType()), ImmutableMap.of(), - queryLifecycle.getQuery().getContext() + queryLifecycle.getQuery().getContext(), + errorResponseTransformStrategy ); this.req = req; this.queryLifecycle = queryLifecycle; diff --git a/server/src/main/java/org/apache/druid/server/QueryResultPusher.java b/server/src/main/java/org/apache/druid/server/QueryResultPusher.java index 6f0b3ee272a7..e919cb83a8cd 100644 --- a/server/src/main/java/org/apache/druid/server/QueryResultPusher.java +++ b/server/src/main/java/org/apache/druid/server/QueryResultPusher.java @@ -24,6 +24,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.io.CountingOutputStream; import org.apache.druid.client.DirectDruidClient; +import org.apache.druid.common.exception.ErrorResponseTransformStrategy; import org.apache.druid.error.DruidException; import org.apache.druid.error.ErrorResponse; import org.apache.druid.error.QueryExceptionCompat; @@ -68,6 +69,7 @@ public abstract class QueryResultPusher private final MediaType contentType; private final Map extraHeaders; private final Map queryContext; + private final ErrorResponseTransformStrategy errorResponseTransformStrategy; private final Map trailerFields; private StreamingHttpResponseAccumulator accumulator; @@ -83,7 +85,8 @@ public QueryResultPusher( String queryId, MediaType contentType, Map extraHeaders, - Map queryContext + Map queryContext, + ErrorResponseTransformStrategy errorResponseTransformStrategy ) { this.request = request; @@ -95,6 +98,7 @@ public QueryResultPusher( this.contentType = contentType; this.extraHeaders = extraHeaders; this.queryContext = queryContext; + this.errorResponseTransformStrategy = errorResponseTransformStrategy; this.trailerFields = new HashMap<>(); } @@ -257,10 +261,23 @@ static void incrementQueryCounterForException( } } + /** + * Records the failure {@code e}, and sends the client the exception returned for it by the + * {@link ErrorResponseTransformStrategy}. + */ private Response handleDruidException(ResultsWriter resultsWriter, DruidException e) { incrementQueryCounterForException(counter, e); + final DruidException clientException = errorResponseTransformStrategy.sanitizeForClient(e, queryId); + if (clientException != e) { + log.noStackTrace().error( + e, + "External Error ID: [%s]. Search the logs for this ID for the full failure details.", + queryId + ); + } + if (resultsWriter != null) { final long bytesWritten = accumulator != null ? accumulator.getNumBytesSent() : 0; resultsWriter.recordFailure(e, bytesWritten); @@ -274,7 +291,7 @@ private Response handleDruidException(ResultsWriter resultsWriter, DruidExceptio if (queryContext != null && Boolean.parseBoolean(String.valueOf(queryContext.get(QueryResource.WRITE_EXCEPTION_BODY_AS_RESPONSE_ROW)))) { try { - accumulator.writer.writeRow(e); + accumulator.writer.writeRow(clientException); accumulator.writer.writeResponseEnd(); } catch (IOException ioException) { @@ -285,7 +302,7 @@ private Response handleDruidException(ResultsWriter resultsWriter, DruidExceptio ); } } - trailerFields.put(QueryResource.ERROR_MESSAGE_TRAILER_HEADER, e.getMessage()); + trailerFields.put(QueryResource.ERROR_MESSAGE_TRAILER_HEADER, clientException.getMessage()); trailerFields.put(QueryResource.RESPONSE_COMPLETE_TRAILER_HEADER, "false"); return null; } @@ -293,7 +310,7 @@ private Response handleDruidException(ResultsWriter resultsWriter, DruidExceptio if (response == null) { return handleDruidExceptionBeforeResponseStarted( - e, + clientException, contentType, ImmutableMap.builder() .putAll(extraHeaders) @@ -305,10 +322,10 @@ private Response handleDruidException(ResultsWriter resultsWriter, DruidExceptio QueryResource.NO_STACK_LOGGER.warn(e, "Response was committed without the accumulator writing anything!?"); } - response.setStatus(e.getStatusCode()); + response.setStatus(clientException.getStatusCode()); response.setHeader("Content-Type", contentType.toString()); try (ServletOutputStream out = response.getOutputStream()) { - writeException(e, out); + writeException(clientException, out); } catch (IOException ioException) { log.warn( 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 6a62b4d77c19..f01b6bd1258f 100644 --- a/server/src/test/java/org/apache/druid/server/QueryResourceTest.java +++ b/server/src/test/java/org/apache/druid/server/QueryResourceTest.java @@ -33,7 +33,9 @@ import com.google.inject.Injector; import com.google.inject.Key; import org.apache.druid.client.BrokerViewOfBrokerConfig; +import org.apache.druid.common.exception.AllowedRegexErrorResponseTransformStrategy; import org.apache.druid.common.exception.ErrorResponseTransformStrategy; +import org.apache.druid.common.exception.PersonaBasedErrorTransformStrategy; import org.apache.druid.error.DruidException; import org.apache.druid.error.DruidExceptionMatcher; import org.apache.druid.error.ErrorResponse; @@ -216,6 +218,9 @@ public QueryRunner getQueryRunnerForSegments(Query query, Iterable QueryRunner getQueryRunnerForIntervals( + Query query, + Iterable intervals + ) + { + return (queryPlus, responseContext) -> new Sequence() + { + @Override + public OutType accumulate(OutType initValue, Accumulator accumulator) + { + accumulator.accumulate( + null, + (T) new TimeBoundaryResultValue(ImmutableMap.of("maxTime", DateTimes.of("2014-08-02"))) + ); + throw failure; + } + + @Override + public Yielder toYielder( + OutType initValue, + YieldingAccumulator accumulator + ) + { + throw new UnsupportedOperationException(); + } + }; + } + + @Override + public QueryRunner getQueryRunnerForSegments( + Query query, + Iterable specs + ) + { + throw new UnsupportedOperationException(); + } + }, + new DefaultGenericQueryMetricsFactory(), + emitter, + testRequestLogger, + new AuthConfig(), + NoopPolicyEnforcer.instance(), + AuthTestUtils.TEST_AUTHORIZER_MAPPER, + new DefaultQueryConfig(Map.of()), + null + ), + null, + queryScheduler, + ResponseContextConfig.newConfig(true), + smileMapper, + serverConfig + ); + } + @Test public void testResponseWithIncludeTrailerHeader() throws IOException { @@ -1214,7 +1353,7 @@ public QueryRunner getQueryRunnerForSegments(Query query, Iterable recordedFailure = new AtomicReference<>(); - HttpServletRequest request = new MockHttpServletRequest(); - ObjectMapper jsonMapper = new DefaultObjectMapper(); - ResponseContextConfig responseContextConfig = ResponseContextConfig.newConfig(true); - DruidNode selfNode = DRUID_NODE; - QueryResource.QueryMetricCounter counter = new NoopQueryMetricCounter(); - String queryId = "someQuery"; - MediaType contentType = MediaType.APPLICATION_JSON_TYPE; - Map extraHeaders = new HashMap<>(); - AtomicBoolean recordFailureInvoked = new AtomicBoolean(); - - String embeddedExceptionMessage = "Embedded Exception Message!"; - RuntimeException embeddedException = new RuntimeException(embeddedExceptionMessage); - RuntimeException topException = new RuntimeException("Where's the party?", embeddedException); - - ResultsWriter resultWriter = new ResultsWriter() - { + makeFailingPusher(topException, recordedFailure, NoErrorResponseTransformStrategy.INSTANCE).push(); + + Assertions.assertNotNull(recordedFailure.get(), "recordFailure(e) should have been invoked!"); + Assertions.assertTrue(Throwables.getStackTraceAsString(recordedFailure.get()).contains(embeddedExceptionMessage)); + } + + @Test + public void testExecutionFailureIsTransformedForClient() + { + final DruidException original = DruidException.forPersona(DruidException.Persona.OPERATOR) + .ofCategory(DruidException.Category.RUNTIME_FAILURE) + .build("internal detail"); + final AtomicReference recordedFailure = new AtomicReference<>(); + final Response response = + makeFailingPusher(original, recordedFailure, PersonaBasedErrorTransformStrategy.INSTANCE).push(); + + Assertions.assertEquals(500, response.getStatus()); + DruidExceptionMatcher.assertThat(((ErrorResponse) response.getEntity()).getUnderlyingException(), HIDDEN_ERROR); + Assertions.assertSame(original, recordedFailure.get()); + } + + @Test + public void testUnexpectedExecutionFailureIsTransformedForClient() + { + final Response response = makeFailingPusher( + new IllegalStateException("internal detail"), + new AtomicReference<>(), + PersonaBasedErrorTransformStrategy.INSTANCE + ).push(); + + Assertions.assertEquals(500, response.getStatus()); + DruidExceptionMatcher.assertThat(((ErrorResponse) response.getEntity()).getUnderlyingException(), HIDDEN_ERROR); + } + + @Test + public void testExecutionTimeoutIsNotTransformed() + { + final Response response = makeFailingPusher( + new QueryTimeoutException("Query timed out"), + new AtomicReference<>(), + PersonaBasedErrorTransformStrategy.INSTANCE + ).push(); + + Assertions.assertEquals(504, response.getStatus()); + DruidExceptionMatcher.assertThat( + ((ErrorResponse) response.getEntity()).getUnderlyingException(), + new DruidExceptionMatcher( + DruidException.Persona.USER, + DruidException.Category.TIMEOUT, + QueryExceptionCompat.ERROR_CODE + ).expectMessageIs("Query timed out") + ); + } + + private static QueryResultPusher makeFailingPusher( + final RuntimeException failure, + final AtomicReference recordedFailure, + final ErrorResponseTransformStrategy strategy + ) + { + final ResultsWriter resultsWriter = new ResultsWriter() + { @Override public void close() { @@ -79,7 +142,7 @@ public void close() @Override public ResponseBuilder start() { - throw topException; + throw failure; } @Override @@ -90,8 +153,7 @@ public void recordSuccess(long numBytes) @Override public void recordFailure(Exception e, long bytesWritten) { - Assertions.assertTrue(Throwables.getStackTraceAsString(e).contains(embeddedExceptionMessage)); - recordFailureInvoked.set(true); + recordedFailure.set(e); } @Override @@ -106,18 +168,20 @@ public QueryResponse getQueryResponse() return null; } }; - QueryResultPusher pusher = new QueryResultPusher( - request, - jsonMapper, - responseContextConfig, - selfNode, - counter, - queryId, - contentType, - extraHeaders, - Collections.emptyMap()) - { + return new QueryResultPusher( + new MockHttpServletRequest(), + new DefaultObjectMapper(), + ResponseContextConfig.newConfig(true), + DRUID_NODE, + new NoopQueryMetricCounter(), + QUERY_ID, + MediaType.APPLICATION_JSON_TYPE, + new HashMap<>(), + Collections.emptyMap(), + strategy + ) + { @Override public void writeException(Exception e, OutputStream out) { @@ -126,13 +190,9 @@ public void writeException(Exception e, OutputStream out) @Override public ResultsWriter start() { - return resultWriter; + return resultsWriter; } }; - - pusher.push(); - - Assertions.assertTrue(recordFailureInvoked.get(), "recordFailure(e) should have been invoked!"); } static class NoopQueryMetricCounter implements QueryMetricCounter diff --git a/sql/src/main/java/org/apache/druid/sql/http/SqlResourceQueryResultPusher.java b/sql/src/main/java/org/apache/druid/sql/http/SqlResourceQueryResultPusher.java index 24c295c9ba5a..5654cb7aa4fb 100644 --- a/sql/src/main/java/org/apache/druid/sql/http/SqlResourceQueryResultPusher.java +++ b/sql/src/main/java/org/apache/druid/sql/http/SqlResourceQueryResultPusher.java @@ -71,7 +71,8 @@ public SqlResourceQueryResultPusher( stmt.sqlQueryId(), MediaType.APPLICATION_JSON_TYPE, headers, - sqlQuery.getContext() + sqlQuery.getContext(), + serverConfig.getErrorResponseTransformStrategy() ); this.serverConfig = serverConfig; this.jsonMapper = jsonMapper; diff --git a/sql/src/test/java/org/apache/druid/sql/avatica/ErrorHandlerTest.java b/sql/src/test/java/org/apache/druid/sql/avatica/ErrorHandlerTest.java index 274ccd8b7505..2bad26d82054 100644 --- a/sql/src/test/java/org/apache/druid/sql/avatica/ErrorHandlerTest.java +++ b/sql/src/test/java/org/apache/druid/sql/avatica/ErrorHandlerTest.java @@ -21,6 +21,8 @@ import com.google.common.collect.ImmutableList; import org.apache.druid.common.exception.AllowedRegexErrorResponseTransformStrategy; +import org.apache.druid.common.exception.PersonaBasedErrorTransformStrategy; +import org.apache.druid.error.DruidException; import org.apache.druid.query.QueryException; import org.apache.druid.query.QueryInterruptedException; import org.apache.druid.server.initialization.ServerConfig; @@ -79,6 +81,31 @@ public void testErrorHandlerHasAffectingErrorResponseTransformStrategyReturnsFal Assertions.assertFalse(errorHandler.hasAffectingErrorResponseTransformStrategy()); } + @Test + public void testErrorHandlerWithPersonaStrategyHidesNonUserDruidException() + { + ErrorHandler errorHandler = new ErrorHandler(new ServerConfig(PersonaBasedErrorTransformStrategy.INSTANCE)); + DruidException input = DruidException.forPersona(DruidException.Persona.OPERATOR) + .ofCategory(DruidException.Category.RUNTIME_FAILURE) + .build("internal detail"); + + RuntimeException output = errorHandler.sanitize(input); + Assertions.assertTrue(output.getMessage().contains("Error ID"), output.getMessage()); + Assertions.assertFalse(output.getMessage().contains("internal detail"), output.getMessage()); + } + + @Test + public void testErrorHandlerWithPersonaStrategyKeepsUserDruidException() + { + ErrorHandler errorHandler = new ErrorHandler(new ServerConfig(PersonaBasedErrorTransformStrategy.INSTANCE)); + DruidException input = DruidException.forPersona(DruidException.Persona.USER) + .ofCategory(DruidException.Category.INVALID_INPUT) + .build("bad interval"); + + RuntimeException output = errorHandler.sanitize(input); + Assertions.assertTrue(output.getMessage().contains("bad interval"), output.getMessage()); + } + @Test public void testErrorHandlerHandlesNonSanitizableExceptionCorrectly() { diff --git a/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java b/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java index 0909be0671c4..fca4e84c0df5 100644 --- a/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java +++ b/sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java @@ -33,6 +33,7 @@ import org.apache.calcite.avatica.SqlType; import org.apache.druid.common.exception.AllowedRegexErrorResponseTransformStrategy; import org.apache.druid.common.exception.ErrorResponseTransformStrategy; +import org.apache.druid.common.exception.PersonaBasedErrorTransformStrategy; import org.apache.druid.common.guava.SettableSupplier; import org.apache.druid.error.DruidException; import org.apache.druid.error.DruidExceptionMatcher; @@ -1814,6 +1815,50 @@ public ErrorResponseTransformStrategy getErrorResponseTransformStrategy() Assertions.assertTrue(lifecycleManager.getAll("id").isEmpty()); } + @Test + public void testExecutionFailureIsTransformedByStrategy() throws Exception + { + resource = new SqlResource( + CalciteTests.TEST_AUTHORIZER_MAPPER, + lifecycleManager, + new SqlEngineRegistry(Set.of(engine)), + new SqlResourceQueryResultPusherFactory( + JSON_MAPPER, + new ServerConfig(PersonaBasedErrorTransformStrategy.INSTANCE), + TEST_RESPONSE_CONTEXT_CONFIG, + DUMMY_DRUID_NODE + ), + DefaultQueryConfig.NIL, + new ServerConfig(PersonaBasedErrorTransformStrategy.INSTANCE) + ); + + onExecute = s -> { + throw DruidException.forPersona(DruidException.Persona.OPERATOR) + .ofCategory(DruidException.Category.RUNTIME_FAILURE) + .build("internal detail"); + }; + final ErrorResponse response = postSyncForException( + new SqlQuery( + "SELECT 1", + ResultFormat.OBJECT, + false, + false, + false, + ImmutableMap.of("sqlQueryId", "id"), + null + ), + 500 + ); + + validateErrorResponse( + response, + "general", + DruidException.Persona.USER, + DruidException.Category.RUNTIME_FAILURE, + "Internal server error, please contact your administrator with Error ID [id] if the issue persists." + ); + } + /** * See class-level javadoc for {@link org.apache.druid.sql.calcite.util.testoperator.AssertionErrorOperatorConversion} * for rationale as to why this test exists. @@ -2607,7 +2652,7 @@ private DruidException validateLegacyQueryExceptionErrorResponse( DruidException exception = validateErrorResponse( errorResponse, QueryExceptionCompat.ERROR_CODE, - DruidException.Persona.OPERATOR, + convertToPersona(legacyCode), convertToCategory(legacyCode), messageContainsString ); @@ -2618,6 +2663,22 @@ private DruidException validateLegacyQueryExceptionErrorResponse( return exception; } + private static DruidException.Persona convertToPersona(String legacyErrorCode) + { + // Copied from QueryExceptionCompat for the same reason as convertToCategory. + switch (QueryException.fromErrorCode(legacyErrorCode)) { + case USER_ERROR: + case UNAUTHORIZED: + case CAPACITY_EXCEEDED: + case CANCELED: + case UNSUPPORTED: + case TIMEOUT: + return DruidException.Persona.USER; + default: + return DruidException.Persona.OPERATOR; + } + } + private static DruidException.Category convertToCategory(String legacyErrorCode) { // This code is copied from QueryExceptionCompat at the time of writing. This is because these mappings