Output a consistent format when generating error json

docs/changelog/90529.yaml

@@ -0,0 +1,23 @@
+pr: 90529
+summary: Output a consistent format when generating error json
+area: Infra/REST API
+type: "breaking, bug"
+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.
+  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."
+  notable: false

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

@@ -543,24 +543,22 @@ public static void generateThrowableXContent(XContentBuilder builder, Params par
      */
     public static void generateFailureXContent(XContentBuilder builder, Params params, @Nullable Exception e, boolean detailed)
         throws IOException {
-        // No exception to render as an error
         if (e == null) {
-            builder.field(ERROR, "unknown");
+            // No exception to render as an error
+            builder.startObject(ERROR);
+            builder.field(TYPE, "unknown");
+            builder.field(REASON, "unknown");
+            builder.endObject();
             return;
         }
 
-        // 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();
-            }
-            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());
+            builder.endObject();
             return;
         }
 
@@ -663,8 +661,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);
         }

server/src/test/java/org/elasticsearch/ElasticsearchExceptionTests.java

@@ -505,12 +505,12 @@ public void testGetDetailedMessage() {
     public void testToXContent() throws IOException {
         {
             ElasticsearchException e = new ElasticsearchException("test");
-            assertExceptionAsJson(e, """
+            assertThrowableAsJson(e, """
                 {"type":"exception","reason":"test"}""");
         }
         {
             ElasticsearchException e = new IndexShardRecoveringException(new ShardId("_test", "_0", 5));
-            assertExceptionAsJson(e, """
+            assertThrowableAsJson(e, """
                 {
                   "type": "index_shard_recovering_exception",
                   "reason": "CurrentState[RECOVERING] Already recovering",
@@ -525,15 +525,15 @@ public void testToXContent() throws IOException {
                 "foo",
                 new IllegalStateException("bar")
             );
-            assertExceptionAsJson(e, """
+            assertThrowableAsJson(e, """
                 {
                   "type": "illegal_state_exception",
                   "reason": "bar"
                 }""");
         }
         {
             ElasticsearchException e = new ElasticsearchException(new IllegalArgumentException("foo"));
-            assertExceptionAsJson(e, """
+            assertThrowableAsJson(e, """
                 {
                   "type": "exception",
                   "reason": "java.lang.IllegalArgumentException: foo",
@@ -546,15 +546,15 @@ public void testToXContent() throws IOException {
         {
             ElasticsearchException e = new SearchParseException(SHARD_TARGET, "foo", new XContentLocation(1, 0));
 
-            assertExceptionAsJson(e, """
+            assertThrowableAsJson(e, """
                 {"type":"search_parse_exception","reason":"foo","line":1,"col":0}""");
         }
         {
             ElasticsearchException ex = new ElasticsearchException(
                 "foo",
                 new ElasticsearchException("bar", new IllegalArgumentException("index is closed", new RuntimeException("foobar")))
             );
-            assertExceptionAsJson(ex, """
+            assertThrowableAsJson(ex, """
                 {
                   "type": "exception",
                   "reason": "foo",
@@ -575,7 +575,7 @@ public void testToXContent() throws IOException {
         {
             ElasticsearchException e = new ElasticsearchException("foo", new IllegalStateException("bar"));
 
-            assertExceptionAsJson(e, """
+            assertThrowableAsJson(e, """
                 {
                   "type": "exception",
                   "reason": "foo",
@@ -610,21 +610,91 @@ public void testToXContent() throws IOException {
         }
     }
 
+    public void testGenerateFailureToXContentWithNoDetails() throws IOException {
+        {
+            Exception ex = new FileNotFoundException("foo not found");
+            for (int i = 0; i < randomInt(10); i++) {
+                ex = new RemoteTransportException("foobar", ex);
+            }
+            assertFailureAsJson(ex, """
+                {"error":{"type":"file_not_found_exception","reason":"foo not found"}}""", false);
+        }
+        {
+            ParsingException ex = new ParsingException(1, 2, "foobar", null);
+            assertFailureAsJson(ex, """
+                {"error":{"type":"parsing_exception","reason":"foobar"}}""", false);
+        }
+
+        { // header and metadata shouldn't be rendered
+            ParsingException ex = new ParsingException(1, 2, "foobar", null);
+            ex.addMetadata("es.test1", "value1");
+            ex.addMetadata("es.test2", "value2");
+            ex.addHeader("test", "some value");
+            ex.addHeader("test_multi", "some value", "another value");
+
+            String expected = """
+                {"error":{"type": "parsing_exception","reason": "foobar"}}""";
+            assertFailureAsJson(ex, expected, false);
+        }
+    }
+
+    public void testGenerateFailureToXContentWithDetails() throws IOException {
+        {
+            Exception ex = new FileNotFoundException("foo not found");
+            for (int i = 0; i < randomInt(10); i++) {
+                ex = new RemoteTransportException("foobar", ex);
+            }
+            assertFailureAsJson(ex, """
+                {"error":{"type":"file_not_found_exception","reason":"foo not found",
+                "root_cause":[{"type":"file_not_found_exception","reason":"foo not found"}]}}""", true);
+        }
+        {
+            ParsingException ex = new ParsingException(1, 2, "foobar", null);
+            assertFailureAsJson(ex, """
+                {"error":{"type":"parsing_exception","reason":"foobar","line":1,"col":2,
+                "root_cause":[{"type":"parsing_exception","reason":"foobar","line":1,"col":2}]}}""", true);
+        }
+
+        { // render header and metadata
+            ParsingException ex = new ParsingException(1, 2, "foobar", null);
+            ex.addMetadata("es.test1", "value1");
+            ex.addMetadata("es.test2", "value2");
+            ex.addHeader("test", "some value");
+            ex.addHeader("test_multi", "some value", "another value");
+
+            String expectedFragment = """
+                {
+                  "type": "parsing_exception",
+                  "reason": "foobar",
+                  "line": 1,
+                  "col": 2,
+                  "test1": "value1",
+                  "test2": "value2",
+                  "header": {
+                    "test_multi": [
+                      "some value",
+                      "another value"
+                    ],
+                    "test": "some value"
+                  }
+                """;
+            String expected = "{\"error\":" + expectedFragment + ",\"root_cause\":[" + expectedFragment + "}]}}";
+            assertFailureAsJson(ex, expected, true);
+        }
+    }
+
     public void testGenerateThrowableToXContent() throws IOException {
         {
-            Exception ex;
-            if (randomBoolean()) {
-                // just a wrapper which is omitted
-                ex = new RemoteTransportException("foobar", new FileNotFoundException("foo not found"));
-            } else {
-                ex = new FileNotFoundException("foo not found");
+            Exception ex = new FileNotFoundException("foo not found");
+            for (int i = 0; i < randomInt(10); i++) {
+                ex = new RemoteTransportException("foobar", ex);
             }
-            assertExceptionAsJson(ex, """
+            assertThrowableAsJson(ex, """
                 {"type":"file_not_found_exception","reason":"foo not found"}""");
         }
         {
             ParsingException ex = new ParsingException(1, 2, "foobar", null);
-            assertExceptionAsJson(ex, """
+            assertThrowableAsJson(ex, """
                 {"type":"parsing_exception","reason":"foobar","line":1,"col":2}""");
         }
 
@@ -664,7 +734,7 @@ public void testGenerateThrowableToXContent() throws IOException {
                     "test": "some value"
                   }
                 }""";
-            assertExceptionAsJson(ex, expected);
+            assertThrowableAsJson(ex, expected);
         }
     }
 
@@ -705,7 +775,7 @@ public void testToXContentWithHeadersAndMetadata() throws IOException {
               }
             }""";
 
-        assertExceptionAsJson(e, expectedJson);
+        assertThrowableAsJson(e, expectedJson);
 
         ElasticsearchException parsed;
         try (XContentParser parser = createParser(XContentType.JSON.xContent(), expectedJson)) {
@@ -825,7 +895,7 @@ public void testFromXContentWithHeadersAndMetadata() throws IOException {
         }
 
         assertNotNull(parsed);
-        assertEquals(parsed.getMessage(), "Elasticsearch exception [type=exception, reason=foo]");
+        assertEquals("Elasticsearch exception [type=exception, reason=foo]", parsed.getMessage());
         assertThat(parsed.getHeaderKeys(), hasSize(1));
         assertThat(parsed.getHeader("foo_1"), hasItem("foo1"));
         assertThat(parsed.getMetadataKeys(), hasSize(1));
@@ -978,7 +1048,7 @@ public void testUnknownFailureToAndFromXContent() throws IOException {
         }
 
         // Failure was null, expecting a "unknown" reason
-        assertEquals("Elasticsearch exception [type=exception, reason=unknown]", parsedFailure.getMessage());
+        assertEquals("Elasticsearch exception [type=unknown, reason=unknown]", parsedFailure.getMessage());
         assertEquals(0, parsedFailure.getHeaders().size());
         assertEquals(0, parsedFailure.getMetadata().size());
     }
@@ -1006,13 +1076,9 @@ public void testFailureToAndFromXContentWithNoDetails() throws IOException {
         }
         assertNotNull(parsedFailure);
 
-        String reason;
-        if (failure instanceof ElasticsearchException) {
-            reason = failure.getClass().getSimpleName() + "[" + failure.getMessage() + "]";
-        } else {
-            reason = "No ElasticsearchException found";
-        }
-        assertEquals(ElasticsearchException.buildMessage("exception", reason, null), parsedFailure.getMessage());
+        String type = ElasticsearchException.getExceptionName(failure);
+        String reason = failure.getMessage();
+        assertEquals(ElasticsearchException.buildMessage(type, reason, null), parsedFailure.getMessage());
         assertEquals(0, parsedFailure.getHeaders().size());
         assertEquals(0, parsedFailure.getMetadata().size());
         assertNull(parsedFailure.getCause());
@@ -1163,13 +1229,20 @@ private static void assertToXContentAsJson(ToXContent e, String expectedJson) th
         assertToXContentEquivalent(new BytesArray(expectedJson), actual, XContentType.JSON);
     }
 
-    private static void assertExceptionAsJson(Exception e, String expectedJson) throws IOException {
+    private static void assertThrowableAsJson(Throwable e, String expectedJson) throws IOException {
         assertToXContentAsJson((builder, params) -> {
             ElasticsearchException.generateThrowableXContent(builder, params, e);
             return builder;
         }, expectedJson);
     }
 
+    private static void assertFailureAsJson(Exception e, String expectedJson, boolean detailed) throws IOException {
+        assertToXContentAsJson((builder, params) -> {
+            ElasticsearchException.generateFailureXContent(builder, params, e, detailed);
+            return builder;
+        }, expectedJson);
+    }
+
     public static void assertDeepEquals(ElasticsearchException expected, ElasticsearchException actual) {
         do {
             if (expected == null) {