High-level client: list tasks failure to not lose nodeId (#31001)

This commit reworks testing for `ListTasksResponse` so that random
fields insertion can be tested and xcontent equivalence can be checked
too. Proper exclusions need to be configured, and failures need to be
tested separately. This helped finding a little problem, whenever there
is a node failure returned, the nodeId was lost as it was never printed
out as part of the exception toXContent.

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

@@ -269,7 +269,6 @@ public class ElasticsearchException extends RuntimeException implements ToXConte
         }
     }
 
-
     /**
      * Retrieve the innermost cause of this exception, if none, returns the current exception.
      */
@@ -292,7 +291,7 @@ public class ElasticsearchException extends RuntimeException implements ToXConte
             out.writeMapOfLists(headers, StreamOutput::writeString, StreamOutput::writeString);
             out.writeMapOfLists(metadata, StreamOutput::writeString, StreamOutput::writeString);
         } else {
-            HashMap<String, List<String>> finalHeaders = new HashMap<>(headers.size() + metadata.size());
+            Map<String, List<String>> finalHeaders = new HashMap<>(headers.size() + metadata.size());
             finalHeaders.putAll(headers);
             finalHeaders.putAll(metadata);
             out.writeMapOfLists(finalHeaders, StreamOutput::writeString, StreamOutput::writeString);

server/src/main/java/org/elasticsearch/action/FailedNodeException.java

@@ -22,6 +22,7 @@ package org.elasticsearch.action;
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
+import org.elasticsearch.common.xcontent.XContentBuilder;
 
 import java.io.IOException;
 
@@ -48,4 +49,9 @@ public class FailedNodeException extends ElasticsearchException {
         super.writeTo(out);
         out.writeOptionalString(nodeId);
     }
+
+    @Override
+    protected void metadataToXContent(XContentBuilder builder, Params params) throws IOException {
+        builder.field("node_id", nodeId);
+    }
 }

server/src/main/java/org/elasticsearch/action/admin/cluster/node/tasks/list/ListTasksResponse.java

@@ -266,6 +266,6 @@ public class ListTasksResponse extends BaseTasksResponse implements ToXContentOb
 
     @Override
     public String toString() {
-        return Strings.toString(this);
+        return Strings.toString(this, true, true);
     }
 }

server/src/main/java/org/elasticsearch/tasks/TaskInfo.java

@@ -29,7 +29,6 @@ import org.elasticsearch.common.io.stream.Writeable;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.common.xcontent.ConstructingObjectParser;
 import org.elasticsearch.common.xcontent.ObjectParserHelper;
-import org.elasticsearch.common.xcontent.ToXContent.Params;
 import org.elasticsearch.common.xcontent.ToXContentFragment;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
@@ -259,7 +258,7 @@ public final class TaskInfo implements Writeable, ToXContentFragment {
 
     @Override
     public String toString() {
-        return Strings.toString(this);
+        return Strings.toString(this, true, true);
     }
 
     // Implements equals and hashCode for testing

server/src/test/java/org/elasticsearch/tasks/ListTasksResponseTests.java

@@ -23,29 +23,29 @@ import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.action.FailedNodeException;
 import org.elasticsearch.action.TaskOperationFailure;
 import org.elasticsearch.action.admin.cluster.node.tasks.list.ListTasksResponse;
-import org.elasticsearch.common.bytes.BytesReference;
+import org.elasticsearch.common.Strings;
-import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
-import org.elasticsearch.common.xcontent.XContentFactory;
-import org.elasticsearch.common.xcontent.XContentHelper;
 import org.elasticsearch.common.xcontent.XContentParser;
-import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.test.AbstractXContentTestCase;
 
 import java.io.IOException;
+import java.net.ConnectException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
-import java.util.Objects;
+import java.util.function.Predicate;
+import java.util.function.Supplier;
 
 import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
 import static org.hamcrest.Matchers.equalTo;
-import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.instanceOf;
 
 public class ListTasksResponseTests extends AbstractXContentTestCase<ListTasksResponse> {
 
     public void testEmptyToString() {
-        assertEquals("{\"tasks\":[]}", new ListTasksResponse().toString());
+        assertEquals("{\n" +
+                "  \"tasks\" : [ ]\n" +
+                "}", new ListTasksResponse().toString());
     }
 
     public void testNonEmptyToString() {
@@ -53,48 +53,113 @@ public class ListTasksResponseTests extends AbstractXContentTestCase<ListTasksRe
             new TaskId("node1", 1), "dummy-type", "dummy-action", "dummy-description", null, 0, 1, true, new TaskId("node1", 0),
             Collections.singletonMap("foo", "bar"));
         ListTasksResponse tasksResponse = new ListTasksResponse(singletonList(info), emptyList(), emptyList());
-        assertEquals("{\"tasks\":[{\"node\":\"node1\",\"id\":1,\"type\":\"dummy-type\",\"action\":\"dummy-action\","
+        assertEquals("{\n" +
-                + "\"description\":\"dummy-description\",\"start_time_in_millis\":0,\"running_time_in_nanos\":1,\"cancellable\":true,"
+                "  \"tasks\" : [\n" +
-                + "\"parent_task_id\":\"node1:0\",\"headers\":{\"foo\":\"bar\"}}]}", tasksResponse.toString());
+                "    {\n" +
+                "      \"node\" : \"node1\",\n" +
+                "      \"id\" : 1,\n" +
+                "      \"type\" : \"dummy-type\",\n" +
+                "      \"action\" : \"dummy-action\",\n" +
+                "      \"description\" : \"dummy-description\",\n" +
+                "      \"start_time\" : \"1970-01-01T00:00:00.000Z\",\n" +
+                "      \"start_time_in_millis\" : 0,\n" +
+                "      \"running_time\" : \"1nanos\",\n" +
+                "      \"running_time_in_nanos\" : 1,\n" +
+                "      \"cancellable\" : true,\n" +
+                "      \"parent_task_id\" : \"node1:0\",\n" +
+                "      \"headers\" : {\n" +
+                "        \"foo\" : \"bar\"\n" +
+                "      }\n" +
+                "    }\n" +
+                "  ]\n" +
+                "}", tasksResponse.toString());
     }
 
     @Override
     protected ListTasksResponse createTestInstance() {
+        //failures are tested separately, so we can test xcontent equivalence at least when we have no failures
+        return new ListTasksResponse(randomTasks(), Collections.emptyList(), Collections.emptyList());
+    }
+
+    private static List<TaskInfo> randomTasks() {
         List<TaskInfo> tasks = new ArrayList<>();
         for (int i = 0; i < randomInt(10); i++) {
             tasks.add(TaskInfoTests.randomTaskInfo());
         }
-        List<TaskOperationFailure> taskFailures = new ArrayList<>();
+        return tasks;
-        for (int i = 0; i < randomInt(5); i++) {
-            taskFailures.add(new TaskOperationFailure(
-                    randomAlphaOfLength(5), randomNonNegativeLong(), new IllegalStateException("message")));
-        }
-        return new ListTasksResponse(tasks, taskFailures, Collections.singletonList(new FailedNodeException("", "message", null)));
     }
 
     @Override
-    protected ListTasksResponse doParseInstance(XContentParser parser) throws IOException {
+    protected ListTasksResponse doParseInstance(XContentParser parser) {
         return ListTasksResponse.fromXContent(parser);
     }
 
     @Override
     protected boolean supportsUnknownFields() {
-        return false;
+        return true;
+    }
+
+    @Override
+    protected Predicate<String> getRandomFieldsExcludeFilter() {
+        //status and headers hold arbitrary content, we can't inject random fields in them
+        return field -> field.endsWith("status") || field.endsWith("headers");
     }
 
     @Override
     protected void assertEqualInstances(ListTasksResponse expectedInstance, ListTasksResponse newInstance) {
         assertNotSame(expectedInstance, newInstance);
         assertThat(newInstance.getTasks(), equalTo(expectedInstance.getTasks()));
-        assertThat(newInstance.getNodeFailures().size(), equalTo(1));
+        assertThat(newInstance.getNodeFailures().size(), equalTo(expectedInstance.getNodeFailures().size()));
-        for (ElasticsearchException failure : newInstance.getNodeFailures()) {
+        for (int i = 0; i < newInstance.getNodeFailures().size(); i++) {
-            assertThat(failure, notNullValue());
+            ElasticsearchException newException = newInstance.getNodeFailures().get(i);
-            assertThat(failure.getMessage(), equalTo("Elasticsearch exception [type=failed_node_exception, reason=message]"));
+            ElasticsearchException expectedException = expectedInstance.getNodeFailures().get(i);
+            assertThat(newException.getMetadata("es.node_id").get(0), equalTo(((FailedNodeException)expectedException).nodeId()));
+            assertThat(newException.getMessage(), equalTo("Elasticsearch exception [type=failed_node_exception, reason=error message]"));
+            assertThat(newException.getCause(), instanceOf(ElasticsearchException.class));
+            ElasticsearchException cause = (ElasticsearchException) newException.getCause();
+            assertThat(cause.getMessage(), equalTo("Elasticsearch exception [type=connect_exception, reason=null]"));
+        }
+        assertThat(newInstance.getTaskFailures().size(), equalTo(expectedInstance.getTaskFailures().size()));
+        for (int i = 0; i < newInstance.getTaskFailures().size(); i++) {
+            TaskOperationFailure newFailure = newInstance.getTaskFailures().get(i);
+            TaskOperationFailure expectedFailure = expectedInstance.getTaskFailures().get(i);
+            assertThat(newFailure.getNodeId(), equalTo(expectedFailure.getNodeId()));
+            assertThat(newFailure.getTaskId(), equalTo(expectedFailure.getTaskId()));
+            assertThat(newFailure.getStatus(), equalTo(expectedFailure.getStatus()));
+            assertThat(newFailure.getCause(), instanceOf(ElasticsearchException.class));
+            ElasticsearchException cause = (ElasticsearchException) newFailure.getCause();
+            assertThat(cause.getMessage(), equalTo("Elasticsearch exception [type=illegal_state_exception, reason=null]"));
         }
     }
 
-    @Override
+    /**
-    protected boolean assertToXContentEquivalence() {
+     * Test parsing {@link ListTasksResponse} with inner failures as they don't support asserting on xcontent equivalence, given that
-        return false;
+     * exceptions are not parsed back as the same original class. We run the usual {@link AbstractXContentTestCase#testFromXContent()}
+     * without failures, and this other test with failures where we disable asserting on xcontent equivalence at the end.
+     */
+    public void testFromXContentWithFailures() throws IOException {
+        Supplier<ListTasksResponse> instanceSupplier = ListTasksResponseTests::createTestInstanceWithFailures;
+        //with random fields insertion in the inner exceptions, some random stuff may be parsed back as metadata,
+        //but that does not bother our assertions, as we only want to test that we don't break.
+        boolean supportsUnknownFields = true;
+        //exceptions are not of the same type whenever parsed back
+        boolean assertToXContentEquivalence = false;
+        AbstractXContentTestCase.testFromXContent(NUMBER_OF_TEST_RUNS, instanceSupplier, supportsUnknownFields, Strings.EMPTY_ARRAY,
+                getRandomFieldsExcludeFilter(), this::createParser, this::doParseInstance,
+                this::assertEqualInstances, assertToXContentEquivalence);
+    }
+
+    private static ListTasksResponse createTestInstanceWithFailures() {
+        int numNodeFailures = randomIntBetween(0, 3);
+        List<FailedNodeException> nodeFailures = new ArrayList<>(numNodeFailures);
+        for (int i = 0; i < numNodeFailures; i++) {
+            nodeFailures.add(new FailedNodeException(randomAlphaOfLength(5), "error message", new ConnectException()));
+        }
+        int numTaskFailures = randomIntBetween(0, 3);
+        List<TaskOperationFailure> taskFailures = new ArrayList<>(numTaskFailures);
+        for (int i = 0; i < numTaskFailures; i++) {
+            taskFailures.add(new TaskOperationFailure(randomAlphaOfLength(5), randomLong(), new IllegalStateException()));
+        }
+        return new ListTasksResponse(randomTasks(), taskFailures, nodeFailures);
     }
 }

server/src/test/java/org/elasticsearch/tasks/TaskInfoTests.java

@@ -63,11 +63,12 @@ public class TaskInfoTests extends AbstractSerializingTestCase<TaskInfo> {
 
     @Override
     protected Predicate<String> getRandomFieldsExcludeFilter() {
+        //status and headers hold arbitrary content, we can't inject random fields in them
         return field -> "status".equals(field) || "headers".equals(field);
     }
 
     @Override
-    protected TaskInfo mutateInstance(TaskInfo info) throws IOException {
+    protected TaskInfo mutateInstance(TaskInfo info) {
         switch (between(0, 9)) {
             case 0:
                 TaskId taskId = new TaskId(info.getTaskId().getNodeId() + randomAlphaOfLength(5), info.getTaskId().getId());