Output a consistent format when generating error json (#90529)

Now, error fields will always have 'type' and 'reason' fields, and the information in those fields is the same regardless of whether the output is detailed or not

Author: thecoop

Diff for: docs/changelog/90529.yaml

@@ -0,0 +1,26 @@
+pr: 90529
+summary: Output a consistent format when generating error json
+area: Infra/REST API
+type: "breaking"
+issues:
+ - 89387
+breaking:
+  title: Error JSON structure has changed when detailed errors are disabled
+  area: REST API
+  details: |-
+    This change modifies the JSON format of error messages returned to REST clients
+    when detailed messages are turned off.
+    Previously, JSON returned when an exception occurred, and `http.detailed_errors.enabled: false` was set,
+    just consisted of a single `"error"` text field with some basic information.
+    Setting `http.detailed_errors.enabled: true` (the default) changed this field
+    to an object with more detailed information.
+    With this change, non-detailed errors now have the same structure as detailed errors. `"error"` will now always
+    be an object with, at a minimum, a `"type"` and `"reason"` field. Additional fields are included when detailed
+    errors are enabled.
+    To use the previous structure for non-detailed errors, use the v8 REST API.
+  impact: |-
+    If you have set `http.detailed_errors.enabled: false` (the default is `true`)
+    the structure of JSON when any exceptions occur now matches the structure when
+    detailed errors are enabled.
+    To use the previous structure for non-detailed errors, use the v8 REST API.
+  notable: false

Diff for: docs/reference/modules/http.asciidoc

@@ -145,11 +145,9 @@ NOTE: This header is only returned when the setting is set to `true`.
 
 `http.detailed_errors.enabled`::
 (<<static-cluster-setting,Static>>, boolean)
-Configures whether detailed error reporting in HTTP responses is enabled.
-Defaults to `true`, which means that HTTP requests that include the
-<<common-options-error-options,`?error_trace` parameter>> will return a
-detailed error message including a stack trace if they encounter an exception.
-If set to `false`, requests with the `?error_trace` parameter are rejected.
+Configures whether detailed error reporting in HTTP responses is enabled. Defaults to `true`.
+When this option is set to `false`, only basic information is returned if an error occurs in the request,
+and requests with <<common-options-error-options,`?error_trace` parameter>> set are rejected.
 
 `http.pipelining.max_events`::
 (<<static-cluster-setting,Static>>, integer)

Diff for: server/src/main/java/org/elasticsearch/ElasticsearchException.java

@@ -25,7 +25,9 @@
 import org.elasticsearch.common.util.concurrent.EsRejectedExecutionException;
 import org.elasticsearch.core.CheckedFunction;
 import org.elasticsearch.core.Nullable;
+import org.elasticsearch.core.RestApiVersion;
 import org.elasticsearch.core.Tuple;
+import org.elasticsearch.core.UpdateForV10;
 import org.elasticsearch.health.node.action.HealthNodeNotDiscoveredException;
 import org.elasticsearch.index.Index;
 import org.elasticsearch.index.mapper.DocumentParsingException;
@@ -611,23 +613,31 @@ protected static void generateThrowableXContent(XContentBuilder builder, Params
      */
     public static XContentBuilder generateFailureXContent(XContentBuilder builder, Params params, @Nullable Exception e, boolean detailed)
         throws IOException {
-        // No exception to render as an error
+        if (builder.getRestApiVersion() == RestApiVersion.V_8) {
+            if (e == null) {
+                return builder.field(ERROR, "unknown");
+            }
+            if (detailed == false) {
+                return generateNonDetailedFailureXContentV8(builder, e);
+            }
+            // else fallthrough
+        }
+
         if (e == null) {
-            return builder.field(ERROR, "unknown");
+            // No exception to render as an error
+            builder.startObject(ERROR);
+            builder.field(TYPE, "unknown");
+            builder.field(REASON, "unknown");
+            return builder.endObject();
         }
 
-        // Render the exception with a simple message
         if (detailed == false) {
-            String message = "No ElasticsearchException found";
-            Throwable t = e;
-            for (int counter = 0; counter < 10 && t != null; counter++) {
-                if (t instanceof ElasticsearchException) {
-                    message = t.getClass().getSimpleName() + "[" + t.getMessage() + "]";
-                    break;
-                }
-                t = t.getCause();
-            }
-            return builder.field(ERROR, message);
+            // just render the type & message
+            Throwable t = ExceptionsHelper.unwrapCause(e);
+            builder.startObject(ERROR);
+            builder.field(TYPE, getExceptionName(t));
+            builder.field(REASON, t.getMessage());
+            return builder.endObject();
         }
 
         // Render the exception with all details
@@ -646,6 +656,20 @@ public static XContentBuilder generateFailureXContent(XContentBuilder builder, P
         return builder.endObject();
     }
 
+    @UpdateForV10(owner = UpdateForV10.Owner.CORE_INFRA)    // remove V8 API
+    private static XContentBuilder generateNonDetailedFailureXContentV8(XContentBuilder builder, @Nullable Exception e) throws IOException {
+        String message = "No ElasticsearchException found";
+        Throwable t = e;
+        for (int counter = 0; counter < 10 && t != null; counter++) {
+            if (t instanceof ElasticsearchException) {
+                message = t.getClass().getSimpleName() + "[" + t.getMessage() + "]";
+                break;
+            }
+            t = t.getCause();
+        }
+        return builder.field(ERROR, message);
+    }
+
     /**
      * Parses the output of {@link #generateFailureXContent(XContentBuilder, Params, Exception, boolean)}
      */
@@ -729,8 +753,8 @@ public static String getExceptionName(Throwable ex) {
 
     static String buildMessage(String type, String reason, String stack) {
         StringBuilder message = new StringBuilder("Elasticsearch exception [");
-        message.append(TYPE).append('=').append(type).append(", ");
-        message.append(REASON).append('=').append(reason);
+        message.append(TYPE).append('=').append(type);
+        message.append(", ").append(REASON).append('=').append(reason);
         if (stack != null) {
             message.append(", ").append(STACK_TRACE).append('=').append(stack);
         }

Diff for: server/src/main/java/org/elasticsearch/common/xcontent/XContentHelper.java

@@ -20,6 +20,7 @@
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.core.CheckedFunction;
 import org.elasticsearch.core.Nullable;
+import org.elasticsearch.core.RestApiVersion;
 import org.elasticsearch.core.Tuple;
 import org.elasticsearch.plugins.internal.XContentParserDecorator;
 import org.elasticsearch.xcontent.DeprecationHandler;
@@ -626,7 +627,22 @@ public static BytesReference toXContent(ChunkedToXContent toXContent, XContentTy
      */
     public static BytesReference toXContent(ToXContent toXContent, XContentType xContentType, Params params, boolean humanReadable)
         throws IOException {
-        try (XContentBuilder builder = XContentBuilder.builder(xContentType.xContent())) {
+        return toXContent(toXContent, xContentType, RestApiVersion.current(), params, humanReadable);
+    }
+
+    /**
+     * Returns the bytes that represent the XContent output of the provided {@link ToXContent} object, using the provided
+     * {@link XContentType}. Wraps the output into a new anonymous object according to the value returned
+     * by the {@link ToXContent#isFragment()} method returns.
+     */
+    public static BytesReference toXContent(
+        ToXContent toXContent,
+        XContentType xContentType,
+        RestApiVersion restApiVersion,
+        Params params,
+        boolean humanReadable
+    ) throws IOException {
+        try (XContentBuilder builder = XContentBuilder.builder(xContentType.xContent(), restApiVersion)) {
             builder.humanReadable(humanReadable);
             if (toXContent.isFragment()) {
                 builder.startObject();

Diff for: server/src/main/java/org/elasticsearch/rest/RestResponse.java

@@ -22,6 +22,7 @@
 import org.elasticsearch.core.Nullable;
 import org.elasticsearch.core.Releasable;
 import org.elasticsearch.core.Releasables;
+import org.elasticsearch.core.RestApiVersion;
 import org.elasticsearch.xcontent.ToXContent;
 import org.elasticsearch.xcontent.XContentBuilder;
 
@@ -146,12 +147,12 @@ public RestResponse(RestChannel channel, RestStatus status, Exception e) throws
             params = new ToXContent.DelegatingMapParams(singletonMap(REST_EXCEPTION_SKIP_STACK_TRACE, "false"), params);
         }
 
-        if (channel.detailedErrorsEnabled() == false) {
+        if (channel.request().getRestApiVersion() == RestApiVersion.V_8 && channel.detailedErrorsEnabled() == false) {
             deprecationLogger.warn(
                 DeprecationCategory.API,
                 "http_detailed_errors",
-                "The JSON format of non-detailed errors will change in Elasticsearch 9.0 to match the JSON structure"
-                    + " used for detailed errors. To keep using the existing format, use the V8 REST API."
+                "The JSON format of non-detailed errors has changed in Elasticsearch 9.0 to match the JSON structure"
+                    + " used for detailed errors."
             );
         }