Remove ExceptionHelper.detailedMessage (#45878)

Closes #19069. This method tries to summarize an exception in a single
string, but the entire point of the structured exception work was to get
real stack traces. This method just hurts, and does not help. Remove this
method, replacing it with a mix of ` e.getMessage()`,
`ExceptionsHelper.stackTrace`.

In some cases, I rearranged some message formats so that exceptions are
reported at the end of the message, instead of in the middle.

Other changes:

   * Rewrite DetailedErrorsEnabledIT to parse responses. Instead of relying
     on substring matching, parse the response JSON and check fields
     explicitly. This uses Jackson, which has been added to the build
     dependencies for :qa:smoke-test-http.
   * Add a getter to `VerificationFailure` so that the `cause` field can be
     accessed and added as suppressed exceptions elsewhere.

client/rest-high-level/src/test/java/org/elasticsearch/client/security/InvalidateApiKeyResponseTests.java

@@ -71,8 +71,8 @@ public void testFromXContent() throws IOException {
                 containsInAnyOrder(previouslyInvalidatedApiKeys.toArray(Strings.EMPTY_ARRAY)));
         assertThat(response.getErrors(), is(notNullValue()));
         assertThat(response.getErrors().size(), is(errors.size()));
-        assertThat(response.getErrors().get(0).toString(), containsString("type=illegal_argument_exception"));
-        assertThat(response.getErrors().get(1).toString(), containsString("type=illegal_argument_exception"));
+        assertThat(response.getErrors().get(0).getCause().toString(), containsString("type=illegal_argument_exception"));
+        assertThat(response.getErrors().get(1).getCause().toString(), containsString("type=illegal_argument_exception"));
     }
 
     public void testEqualsHashCode() {

client/rest-high-level/src/test/java/org/elasticsearch/client/security/InvalidateTokenResponseTests.java

@@ -83,9 +83,9 @@ public void testFromXContentWithErrors() throws IOException {
             assertThat(response.getPreviouslyInvalidatedTokens(), Matchers.equalTo(previouslyInvalidatedTokens));
             assertThat(response.getErrorsCount(), Matchers.equalTo(2));
             assertThat(response.getErrors().get(0).toString(), containsString("type=exception, reason=foo"));
-            assertThat(response.getErrors().get(0).toString(), containsString("type=illegal_argument_exception, reason=bar"));
+            assertThat(response.getErrors().get(0).getCause().getMessage(), containsString("type=illegal_argument_exception, reason=bar"));
             assertThat(response.getErrors().get(1).toString(), containsString("type=exception, reason=boo"));
-            assertThat(response.getErrors().get(1).toString(), containsString("type=illegal_argument_exception, reason=far"));
+            assertThat(response.getErrors().get(1).getCause().getMessage(), containsString("type=illegal_argument_exception, reason=far"));
         }
     }
 }

modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/IngestRestartIT.java

@@ -99,7 +99,7 @@ public Settings onNodeStopped(String nodeName) {
             }
 
         });
-        
+
         checkPipelineExists.accept(pipelineIdWithoutScript);
         checkPipelineExists.accept(pipelineIdWithScript);
 
@@ -119,12 +119,9 @@ public Settings onNodeStopped(String nodeName) {
         assertThat(exception.getHeader("processor_type"), equalTo(Arrays.asList("unknown")));
         assertThat(exception.getRootCause().getMessage(),
             equalTo("pipeline with id [" + pipelineIdWithScript + "] could not be loaded, caused by " +
-                "[ElasticsearchParseException[Error updating pipeline with id [" + pipelineIdWithScript + "]]; " +
-                "nested: ElasticsearchException[java.lang.IllegalArgumentException: cannot execute [inline] scripts]; " +
-                "nested: IllegalArgumentException[cannot execute [inline] scripts];; " +
-                "ElasticsearchException[java.lang.IllegalArgumentException: cannot execute [inline] scripts]; " +
-                "nested: IllegalArgumentException[cannot execute [inline] scripts];; java.lang.IllegalArgumentException: " +
-                "cannot execute [inline] scripts]"));
+                "[org.elasticsearch.ElasticsearchParseException: Error updating pipeline with id [" + pipelineIdWithScript + "]; " +
+                "org.elasticsearch.ElasticsearchException: java.lang.IllegalArgumentException: cannot execute [inline] scripts; " +
+                "java.lang.IllegalArgumentException: cannot execute [inline] scripts]"));
 
         Map<String, Object> source = client().prepareGet("index", "doc", "1").get().getSource();
         assertThat(source.get("x"), equalTo(0));

modules/reindex/src/test/java/org/elasticsearch/index/reindex/ReindexFailureTests.java

@@ -32,6 +32,7 @@
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.either;
 import static org.hamcrest.Matchers.greaterThan;
+import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.lessThanOrEqualTo;
 
 /**
@@ -61,7 +62,8 @@ public void testFailuresCauseAbortDefault() throws Exception {
                 .batches(1)
                 .failures(both(greaterThan(0)).and(lessThanOrEqualTo(maximumNumberOfShards()))));
         for (Failure failure: response.getBulkFailures()) {
-            assertThat(failure.getMessage(), containsString("IllegalArgumentException[For input string: \"words words\"]"));
+            assertThat(failure.getCause().getCause(), instanceOf(IllegalArgumentException.class));
+            assertThat(failure.getCause().getCause().getMessage(), containsString("For input string: \"words words\""));
         }
     }
 
@@ -79,7 +81,7 @@ public void testAbortOnVersionConflict() throws Exception {
         BulkByScrollResponse response = copy.get();
         assertThat(response, matcher().batches(1).versionConflicts(1).failures(1).created(99));
         for (Failure failure: response.getBulkFailures()) {
-            assertThat(failure.getMessage(), containsString("VersionConflictEngineException[["));
+            assertThat(failure.getMessage(), containsString("VersionConflictEngineException: ["));
         }
     }
 

qa/smoke-test-http/build.gradle

@@ -23,6 +23,7 @@ apply plugin: 'elasticsearch.rest-test'
 apply plugin: 'elasticsearch.test-with-dependencies'
 
 dependencies {
+    testCompile "com.fasterxml.jackson.core:jackson-databind:2.8.11"
     testCompile project(path: ':modules:transport-netty4', configuration: 'runtime') // for http
     testCompile project(path: ':plugins:transport-nio', configuration: 'runtime') // for http
 }

qa/smoke-test-http/src/test/java/org/elasticsearch/http/DetailedErrorsEnabledIT.java

@@ -19,44 +19,65 @@
 
 package org.elasticsearch.http;
 
-import org.apache.http.util.EntityUtils;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
 import org.elasticsearch.client.Request;
 import org.elasticsearch.client.Response;
 import org.elasticsearch.client.ResponseException;
 
 import java.io.IOException;
 
 import static org.hamcrest.Matchers.containsString;
-import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.startsWith;
 
 /**
  * Tests that by default the error_trace parameter can be used to show stacktraces
  */
 public class DetailedErrorsEnabledIT extends HttpSmokeTestCase {
 
-    public void testThatErrorTraceWorksByDefault() throws IOException {
+    public void testThatErrorTraceCanBeEnabled() throws IOException {
+        ObjectMapper mapper = new ObjectMapper();
+
         try {
             Request request = new Request("DELETE", "/");
             request.addParameter("error_trace", "true");
             getRestClient().performRequest(request);
             fail("request should have failed");
-        } catch(ResponseException e) {
+        } catch (ResponseException e) {
             Response response = e.getResponse();
             assertThat(response.getHeader("Content-Type"), containsString("application/json"));
-            assertThat(EntityUtils.toString(response.getEntity()),
-                    containsString("\"stack_trace\":\"[Validation Failed: 1: index / indices is missing;]; " +
-                    "nested: ActionRequestValidationException[Validation Failed: 1:"));
+
+            JsonNode jsonNode = mapper.readTree(response.getEntity().getContent());
+
+            assertThat(
+                jsonNode.get("error").get("stack_trace").asText(),
+                startsWith("org.elasticsearch.action.ActionRequestValidationException: Validation Failed: 1: index / indices is missing"));
+
+            // An ActionRequestValidationException isn't an ElasticsearchException, so when the code tries
+            // to work out the root cause, all it actually achieves is wrapping the actual exception in
+            // an ElasticsearchException. At least this proves that the root cause logic is executing.
+            assertThat(
+                jsonNode.get("error").get("root_cause").get(0).get("stack_trace").asText(),
+                startsWith("org.elasticsearch.ElasticsearchException$1: Validation Failed: 1: index / indices is missing"));
         }
+    }
+
+    public void testThatErrorTraceDefaultsToDisabled() throws IOException {
 
         try {
             getRestClient().performRequest(new Request("DELETE", "/"));
             fail("request should have failed");
-        } catch(ResponseException e) {
+        } catch (ResponseException e) {
             Response response = e.getResponse();
-            assertThat(response.getHeader("Content-Type"), containsString("application/json; charset=UTF-8"));
-            assertThat(EntityUtils.toString(response.getEntity()),
-                    not(containsString("\"stack_trace\":\"[Validation Failed: 1: index / indices is missing;]; "
-                    + "nested: ActionRequestValidationException[Validation Failed: 1:")));
+            assertThat(response.getHeader("Content-Type"), containsString("application/json"));
+
+            ObjectMapper mapper = new ObjectMapper();
+            JsonNode jsonNode = mapper.readTree(response.getEntity().getContent());
+
+            assertFalse("Unexpected .stack_trace in JSON response", jsonNode.get("error").has("stack_trace"));
+            assertFalse(
+                "Unexpected .error.root_cause[0].stack_trace in JSON response",
+                jsonNode.get("error").get("root_cause").get(0).has("stack_trace"));
         }
     }
 }

server/src/main/java/org/elasticsearch/ElasticsearchException.java

@@ -681,7 +681,7 @@ public String toString() {
             }
             builder.append(' ');
         }
-        return builder.append(ExceptionsHelper.detailedMessage(this).trim()).toString();
+        return builder.append(super.toString().trim()).toString();
     }
 
     /**

server/src/main/java/org/elasticsearch/ExceptionsHelper.java

@@ -99,35 +99,6 @@ public static Throwable unwrapCause(Throwable t) {
         return result;
     }
 
-    /**
-     * @deprecated Don't swallow exceptions, allow them to propagate.
-     */
-    @Deprecated
-    public static String detailedMessage(Throwable t) {
-        if (t == null) {
-            return "Unknown";
-        }
-        if (t.getCause() != null) {
-            StringBuilder sb = new StringBuilder();
-            while (t != null) {
-                sb.append(t.getClass().getSimpleName());
-                if (t.getMessage() != null) {
-                    sb.append("[");
-                    sb.append(t.getMessage());
-                    sb.append("]");
-                }
-                sb.append("; ");
-                t = t.getCause();
-                if (t != null) {
-                    sb.append("nested: ");
-                }
-            }
-            return sb.toString();
-        } else {
-            return t.getClass().getSimpleName() + "[" + t.getMessage() + "]";
-        }
-    }
-
     public static String stackTrace(Throwable e) {
         StringWriter stackTraceStringWriter = new StringWriter();
         PrintWriter printWriter = new PrintWriter(stackTraceStringWriter);

server/src/main/java/org/elasticsearch/action/search/ShardSearchFailure.java

@@ -72,7 +72,7 @@ public ShardSearchFailure(Exception e) {
     public ShardSearchFailure(Exception e, @Nullable SearchShardTarget shardTarget) {
         super(shardTarget == null ? null : shardTarget.getFullyQualifiedIndexName(),
             shardTarget == null ? -1 : shardTarget.getShardId().getId(),
-            ExceptionsHelper.detailedMessage(e),
+            ExceptionsHelper.stackTrace(e),
             ExceptionsHelper.status(ExceptionsHelper.unwrapCause(e)),
             ExceptionsHelper.unwrapCause(e));
 

server/src/main/java/org/elasticsearch/action/support/DefaultShardOperationFailedException.java

@@ -33,7 +33,6 @@
 
 import java.io.IOException;
 
-import static org.elasticsearch.ExceptionsHelper.detailedMessage;
 import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
 
 public class DefaultShardOperationFailedException extends ShardOperationFailedException implements Writeable {
@@ -63,11 +62,11 @@ protected DefaultShardOperationFailedException(StreamInput in) throws IOExceptio
 
     public DefaultShardOperationFailedException(ElasticsearchException e) {
         super(e.getIndex() == null ? null : e.getIndex().getName(), e.getShardId() == null ? -1 : e.getShardId().getId(),
-            detailedMessage(e), e.status(), e);
+            ExceptionsHelper.stackTrace(e), e.status(), e);
     }
 
     public DefaultShardOperationFailedException(String index, int shardId, Throwable cause) {
-        super(index, shardId, detailedMessage(cause), ExceptionsHelper.status(cause), cause);
+        super(index, shardId, ExceptionsHelper.stackTrace(cause), ExceptionsHelper.status(cause), cause);
     }
 
     public static DefaultShardOperationFailedException readShardOperationFailed(StreamInput in) throws IOException {
@@ -79,7 +78,7 @@ public static void readFrom(StreamInput in, DefaultShardOperationFailedException
         f.shardId = in.readVInt();
         f.cause = in.readException();
         f.status = RestStatus.readFrom(in);
-        f.reason = detailedMessage(f.cause);
+        f.reason = ExceptionsHelper.stackTrace(f.cause);
     }
 
     @Override

server/src/main/java/org/elasticsearch/action/support/replication/ReplicationResponse.java

@@ -238,7 +238,7 @@ public Failure(StreamInput in) throws IOException {
             }
 
             public Failure(ShardId  shardId, @Nullable String nodeId, Exception cause, RestStatus status, boolean primary) {
-                super(shardId.getIndexName(), shardId.getId(), ExceptionsHelper.detailedMessage(cause), status, cause);
+                super(shardId.getIndexName(), shardId.getId(), ExceptionsHelper.stackTrace(cause), status, cause);
                 this.shardId = shardId;
                 this.nodeId = nodeId;
                 this.primary = primary;

server/src/main/java/org/elasticsearch/cluster/action/shard/ShardStateAction.java

@@ -444,10 +444,10 @@ public String toString() {
             components.add("allocation id [" + allocationId + "]");
             components.add("primary term [" + primaryTerm + "]");
             components.add("message [" + message + "]");
+            components.add("markAsStale [" + markAsStale + "]");
             if (failure != null) {
-                components.add("failure [" + ExceptionsHelper.detailedMessage(failure) + "]");
+                components.add("failure [" + ExceptionsHelper.stackTrace(failure) + "]");
             }
-            components.add("markAsStale [" + markAsStale + "]");
             return String.join(", ", components);
         }
 

server/src/main/java/org/elasticsearch/cluster/routing/UnassignedInfo.java

@@ -337,7 +337,7 @@ public String getDetails() {
         if (message == null) {
             return null;
         }
-        return message + (failure == null ? "" : ", failure " + ExceptionsHelper.detailedMessage(failure));
+        return message + (failure == null ? "" : ", failure " + ExceptionsHelper.stackTrace(failure));
     }
 
     /**

server/src/main/java/org/elasticsearch/cluster/routing/allocation/FailedShard.java

@@ -42,8 +42,8 @@ public FailedShard(ShardRouting routingEntry, String message, Exception failure,
 
     @Override
     public String toString() {
-        return "failed shard, shard " + routingEntry + ", message [" + message + "], failure [" +
-                   ExceptionsHelper.detailedMessage(failure) + "], markAsStale [" + markAsStale + "]";
+        return "failed shard, shard " + routingEntry + ", message [" + message + "], markAsStale [" + markAsStale + "], failure ["
+            + ExceptionsHelper.stackTrace(failure) + "]";
     }
 
     /**

server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/RestoreInProgressAllocationDecider.java

@@ -59,10 +59,10 @@ public Decision canAllocate(final ShardRouting shardRouting, final RoutingAlloca
                 }
             }
         }
-        return allocation.decision(Decision.NO, NAME, "shard has failed to be restored from the snapshot [%s] because of [%s] - " +
+        return allocation.decision(Decision.NO, NAME, "shard has failed to be restored from the snapshot [%s] - " +
             "manually close or delete the index [%s] in order to retry to restore the snapshot again or use the reroute API to force the " +
-            "allocation of an empty primary shard",
-            source.snapshot(), shardRouting.unassignedInfo().getDetails(), shardRouting.getIndexName());
+            "allocation of an empty primary shard. Details: [%s]",
+            source.snapshot(), shardRouting.getIndexName(), shardRouting.unassignedInfo().getDetails());
     }
 
     @Override

server/src/main/java/org/elasticsearch/index/query/MoreLikeThisQueryBuilder.java

@@ -443,7 +443,7 @@ public String toString() {
                 toXContent(builder, EMPTY_PARAMS);
                 return Strings.toString(builder);
             } catch (Exception e) {
-                return "{ \"error\" : \"" + ExceptionsHelper.detailedMessage(e) + "\"}";
+                throw new RuntimeException(e);
             }
         }
 

server/src/main/java/org/elasticsearch/index/shard/StoreRecovery.java

@@ -367,8 +367,7 @@ private void internalRecoverFromStore(IndexShard indexShard) throws IndexShardRe
                     try {
                         files = Arrays.toString(store.directory().listAll());
                     } catch (Exception inner) {
-                        inner.addSuppressed(e);
-                        files += " (failure=" + ExceptionsHelper.detailedMessage(inner) + ")";
+                        files += " (failure=" + ExceptionsHelper.stackTrace(inner) + ")";
                     }
                     if (indexShouldExists) {
                         throw new IndexShardRecoveryException(shardId,

server/src/main/java/org/elasticsearch/repositories/VerificationFailure.java

@@ -34,4 +34,8 @@ public class VerificationFailure {
     public String toString() {
         return "[" + nodeId + ", '" + cause + "']";
     }
+
+    public Exception getCause() {
+        return cause;
+    }
 }

server/src/main/java/org/elasticsearch/repositories/VerifyNodeRepositoryAction.java

@@ -115,7 +115,11 @@ public void handleException(TransportException exp) {
     private static void finishVerification(String repositoryName, ActionListener<List<DiscoveryNode>> listener, List<DiscoveryNode> nodes,
                                    CopyOnWriteArrayList<VerificationFailure> errors) {
         if (errors.isEmpty() == false) {
-            listener.onFailure(new RepositoryVerificationException(repositoryName, errors.toString()));
+            RepositoryVerificationException e = new RepositoryVerificationException(repositoryName, errors.toString());
+            for (VerificationFailure error : errors) {
+                e.addSuppressed(error.getCause());
+            }
+            listener.onFailure(e);
         } else {
             listener.onResponse(nodes);
         }

server/src/main/java/org/elasticsearch/repositories/blobstore/BlobStoreRepository.java

@@ -929,7 +929,7 @@ public void snapshotShard(Store store, MapperService mapperService, SnapshotId s
         final long startTime = threadPool.absoluteTimeInMillis();
         final StepListener<Void> snapshotDoneListener = new StepListener<>();
         snapshotDoneListener.whenComplete(listener::onResponse, e -> {
-            snapshotStatus.moveToFailed(threadPool.absoluteTimeInMillis(), ExceptionsHelper.detailedMessage(e));
+            snapshotStatus.moveToFailed(threadPool.absoluteTimeInMillis(), ExceptionsHelper.stackTrace(e));
             listener.onFailure(e instanceof IndexShardSnapshotFailedException ? (IndexShardSnapshotFailedException) e
                 : new IndexShardSnapshotFailedException(store.shardId(), e));
         });

server/src/main/java/org/elasticsearch/search/suggest/phrase/DirectCandidateGeneratorBuilder.java

@@ -27,7 +27,6 @@
 import org.apache.lucene.search.spell.StringDistance;
 import org.apache.lucene.search.spell.SuggestMode;
 import org.apache.lucene.util.automaton.LevenshteinAutomata;
-import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.common.ParseField;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.stream.StreamInput;
@@ -490,7 +489,7 @@ public String toString() {
             toXContent(builder, EMPTY_PARAMS);
             return Strings.toString(builder);
         } catch (Exception e) {
-            return "{ \"error\" : \"" + ExceptionsHelper.detailedMessage(e) + "\"}";
+            throw new RuntimeException(e);
         }
     }
 

server/src/main/java/org/elasticsearch/snapshots/SnapshotShardsService.java

@@ -308,7 +308,7 @@ public void onResponse(final Void aVoid) {
                     @Override
                     public void onFailure(Exception e) {
                         logger.warn(() -> new ParameterizedMessage("[{}][{}] failed to snapshot shard", shardId, snapshot), e);
-                        notifyFailedSnapshotShard(snapshot, shardId, ExceptionsHelper.detailedMessage(e));
+                        notifyFailedSnapshotShard(snapshot, shardId, ExceptionsHelper.stackTrace(e));
                     }
                 });
             }

server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

@@ -567,7 +567,7 @@ private void cleanupAfterError(Exception exception) {
                             .finalizeSnapshot(snapshot.snapshot().getSnapshotId(),
                                 snapshot.indices(),
                                 snapshot.startTime(),
-                                ExceptionsHelper.detailedMessage(exception),
+                                ExceptionsHelper.stackTrace(exception),
                                 0,
                                 Collections.emptyList(),
                                 snapshot.getRepositoryStateId(),