fix: apply errorResponseTransform to query execution failures

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`.
 

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));
+  }
+}

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<DruidException> 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<String, String> getErrorMessageTransformFunction()
   {
-    throw new UnsupportedOperationException();
+    return Function.identity();
+  }
+
+  /**
+   * Whether a {@link SanitizableException} is hidden from the client:
+   * <ul>
+   *   <li>An exception that wraps a {@link DruidException} is hidden if that exception is not meant for users.</li>
+   *   <li>A {@link QueryException} is hidden if its {@link QueryException.FailType} is not meant for users, as
+   *   decided by {@link QueryExceptionCompat#getPersona}.</li>
+   *   <li>An {@link ISE} is hidden, since it describes internal state.</li>
+   *   <li>Other exceptions, such as {@link UOE} and authorization failures, describe what the client asked for, and
+   *   are not hidden.</li>
+   * </ul>
+   */
+  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

processing/src/main/java/org/apache/druid/error/QueryExceptionCompat.java

@@ -45,14 +45,28 @@ 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())
               .withContext("errorClass", exception.getErrorClass())
               .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) {

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()
   {

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",

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();

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;