Cleanup ElasticsearchException a little (#127713)

Looked into this logic a bit since I suspect it of causing some OOMs potentially.
We should probably be using chunked writing for these in REST responses, but for now
a couple smaller cleanups and savings only.

Author: original-brownbear

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

@@ -294,7 +294,7 @@ public Throwable unwrapCause() {
     public String getDetailedMessage() {
         if (getCause() != null) {
             StringBuilder sb = new StringBuilder();
-            sb.append(toString()).append("; ");
+            sb.append(this).append("; ");
             if (getCause() instanceof ElasticsearchException) {
                 sb.append(((ElasticsearchException) getCause()).getDetailedMessage());
             } else {
@@ -384,7 +384,7 @@ protected XContentBuilder toXContent(XContentBuilder builder, Params params, int
         if (ex != this) {
             generateThrowableXContent(builder, params, this, nestedLevel);
         } else {
-            innerToXContent(builder, params, this, getExceptionName(), getMessage(), headers, metadata, getCause(), nestedLevel);
+            innerToXContent(builder, params, this, headers, metadata, getCause(), nestedLevel);
         }
         return builder;
     }
@@ -393,8 +393,6 @@ protected static void innerToXContent(
         XContentBuilder builder,
         Params params,
         Throwable throwable,
-        String type,
-        String message,
         Map<String, List<String>> headers,
         Map<String, List<String>> metadata,
         Throwable cause,
@@ -408,16 +406,12 @@ protected static void innerToXContent(
             return;
         }
 
-        builder.field(TYPE, type);
-        builder.field(REASON, message);
+        builder.field(TYPE, throwable instanceof ElasticsearchException e ? e.getExceptionName() : getExceptionName(throwable));
+        builder.field(REASON, throwable.getMessage());
 
-        boolean timedOut = false;
-        if (throwable instanceof ElasticsearchException exception) {
-            // TODO: we could walk the exception chain to see if _any_ causes are timeouts?
-            timedOut = exception.isTimeout();
-        }
-        if (timedOut) {
-            builder.field(TIMED_OUT, timedOut);
+        // TODO: we could walk the exception chain to see if _any_ causes are timeouts?
+        if (throwable instanceof ElasticsearchException exception && exception.isTimeout()) {
+            builder.field(TIMED_OUT, true);
         }
 
         for (Map.Entry<String, List<String>> entry : metadata.entrySet()) {
@@ -428,13 +422,10 @@ protected static void innerToXContent(
             exception.metadataToXContent(builder, params);
         }
 
-        if (params.paramAsBoolean(REST_EXCEPTION_SKIP_CAUSE, REST_EXCEPTION_SKIP_CAUSE_DEFAULT) == false) {
-            if (cause != null) {
-                builder.field(CAUSED_BY);
-                builder.startObject();
-                generateThrowableXContent(builder, params, cause, nestedLevel + 1);
-                builder.endObject();
-            }
+        if (cause != null && params.paramAsBoolean(REST_EXCEPTION_SKIP_CAUSE, REST_EXCEPTION_SKIP_CAUSE_DEFAULT) == false) {
+            builder.startObject(CAUSED_BY);
+            generateThrowableXContent(builder, params, cause, nestedLevel + 1);
+            builder.endObject();
         }
 
         if (headers.isEmpty() == false) {
@@ -607,7 +598,7 @@ public static ElasticsearchException innerFromXContent(XContentParser parser, bo
     /**
      * Static toXContent helper method that renders {@link org.elasticsearch.ElasticsearchException} or {@link Throwable} instances
      * as XContent, delegating the rendering to {@link #toXContent(XContentBuilder, Params)}
-     * or {@link #innerToXContent(XContentBuilder, Params, Throwable, String, String, Map, Map, Throwable, int)}.
+     * or {@link #innerToXContent(XContentBuilder, Params, Throwable, Map, Map, Throwable, int)}.
      *
      * This method is usually used when the {@link Throwable} is rendered as a part of another XContent object, and its result can
      * be parsed back using the {@link #fromXContent(XContentParser)} method.
@@ -627,7 +618,7 @@ protected static void generateThrowableXContent(XContentBuilder builder, Params
         if (t instanceof ElasticsearchException) {
             ((ElasticsearchException) t).toXContent(builder, params, nestedLevel);
         } else {
-            innerToXContent(builder, params, t, getExceptionName(t), t.getMessage(), emptyMap(), emptyMap(), t.getCause(), nestedLevel);
+            innerToXContent(builder, params, t, emptyMap(), emptyMap(), t.getCause(), nestedLevel);
         }
     }
 
@@ -723,8 +714,8 @@ public static ElasticsearchException failureFromXContent(XContentParser parser)
      */
     public ElasticsearchException[] guessRootCauses() {
         final Throwable cause = getCause();
-        if (cause != null && cause instanceof ElasticsearchException) {
-            return ((ElasticsearchException) cause).guessRootCauses();
+        if (cause instanceof ElasticsearchException ese) {
+            return ese.guessRootCauses();
         }
         return new ElasticsearchException[] { this };
     }
@@ -773,35 +764,28 @@ protected String getExceptionName() {
      */
     public static String getExceptionName(Throwable ex) {
         String simpleName = ex.getClass().getSimpleName();
-        if (simpleName.startsWith("Elasticsearch")) {
-            simpleName = simpleName.substring("Elasticsearch".length());
-        }
         // TODO: do we really need to make the exception name in underscore casing?
-        return toUnderscoreCase(simpleName);
+        return toUnderscoreCase(simpleName, simpleName.startsWith("Elasticsearch") ? "Elasticsearch".length() : 0);
     }
 
     static String buildMessage(String type, String reason, String stack) {
-        StringBuilder message = new StringBuilder("Elasticsearch exception [");
-        message.append(TYPE).append('=').append(type);
-        message.append(", ").append(REASON).append('=').append(reason);
-        if (stack != null) {
-            message.append(", ").append(STACK_TRACE).append('=').append(stack);
-        }
-        message.append(']');
-        return message.toString();
+        return "Elasticsearch exception ["
+            + TYPE
+            + "="
+            + type
+            + ", "
+            + REASON
+            + "="
+            + reason
+            + (stack == null ? "" : (", " + STACK_TRACE + "=" + stack))
+            + "]";
     }
 
     @Override
     public String toString() {
-        StringBuilder builder = new StringBuilder();
-        if (metadata.containsKey(INDEX_METADATA_KEY)) {
-            builder.append(getIndex());
-            if (metadata.containsKey(SHARD_METADATA_KEY)) {
-                builder.append('[').append(getShardId()).append(']');
-            }
-            builder.append(' ');
-        }
-        return builder.append(super.toString().trim()).toString();
+        return (metadata.containsKey(INDEX_METADATA_KEY)
+            ? (getIndex() + (metadata.containsKey(SHARD_METADATA_KEY) ? "[" + getShardId() + "] " : " "))
+            : "") + super.toString().trim();
     }
 
     /**
@@ -2106,19 +2090,17 @@ public String getResourceType() {
     }
 
     // lower cases and adds underscores to transitions in a name
-    private static String toUnderscoreCase(String value) {
+    private static String toUnderscoreCase(String value, final int offset) {
         StringBuilder sb = new StringBuilder();
         boolean changed = false;
-        for (int i = 0; i < value.length(); i++) {
+        for (int i = offset; i < value.length(); i++) {
             char c = value.charAt(i);
             if (Character.isUpperCase(c)) {
                 if (changed == false) {
                     // copy it over here
-                    for (int j = 0; j < i; j++) {
-                        sb.append(value.charAt(j));
-                    }
+                    sb.append(value, offset, i);
                     changed = true;
-                    if (i == 0) {
+                    if (i == offset) {
                         sb.append(Character.toLowerCase(c));
                     } else {
                         sb.append('_');
@@ -2135,7 +2117,7 @@ private static String toUnderscoreCase(String value) {
             }
         }
         if (changed == false) {
-            return value;
+            return offset == 0 ? value : value.substring(offset);
         }
         return sb.toString();
     }

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

@@ -106,22 +106,6 @@ public Throwable getCause() {
         return cause;
     }
 
-    private static String buildMessage(String phaseName, String msg, ShardSearchFailure[] shardFailures) {
-        StringBuilder sb = new StringBuilder();
-        sb.append("Failed to execute phase [").append(phaseName).append("], ").append(msg);
-        if (CollectionUtils.isEmpty(shardFailures) == false) {
-            sb.append("; shardFailures ");
-            for (ShardSearchFailure shardFailure : shardFailures) {
-                if (shardFailure.shard() != null) {
-                    sb.append("{").append(shardFailure.shard()).append(": ").append(shardFailure.reason()).append("}");
-                } else {
-                    sb.append("{").append(shardFailure.reason()).append("}");
-                }
-            }
-        }
-        return sb.toString();
-    }
-
     @Override
     protected void metadataToXContent(XContentBuilder builder, Params params) throws IOException {
         builder.field("phase", phaseName);
@@ -144,17 +128,7 @@ protected XContentBuilder toXContent(XContentBuilder builder, Params params, int
             // We don't have a cause when all shards failed, but we do have shards failures so we can "guess" a cause
             // (see {@link #getCause()}). Here, we use super.getCause() because we don't want the guessed exception to
             // be rendered twice (one in the "cause" field, one in "failed_shards")
-            innerToXContent(
-                builder,
-                params,
-                this,
-                getExceptionName(),
-                getMessage(),
-                getHeaders(),
-                getMetadata(),
-                super.getCause(),
-                nestedLevel
-            );
+            innerToXContent(builder, params, this, getHeaders(), getMetadata(), super.getCause(), nestedLevel);
         }
         return builder;
     }
@@ -172,7 +146,23 @@ public ElasticsearchException[] guessRootCauses() {
 
     @Override
     public String toString() {
-        return buildMessage(phaseName, getMessage(), shardFailures);
+        return "Failed to execute phase ["
+            + phaseName
+            + "], "
+            + getMessage()
+            + (CollectionUtils.isEmpty(shardFailures) ? "" : buildShardFailureString());
+    }
+
+    private String buildShardFailureString() {
+        StringBuilder sb = new StringBuilder("; shardFailures ");
+        for (ShardSearchFailure shardFailure : shardFailures) {
+            sb.append("{");
+            if (shardFailure.shard() != null) {
+                sb.append(shardFailure.shard()).append(": ");
+            }
+            sb.append(shardFailure.reason()).append("}");
+        }
+        return sb.toString();
     }
 
     public String getPhaseName() {