From 4fe650c9e56d5f68ae9d0b0ca802d0827346c5c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Fri, 21 Jun 2019 14:29:46 +0200 Subject: [PATCH] Fix DefaultShardOperationFailedException subclass xcontent serialization (#43435) The current toXContent implementation can fail when the superclasses toXContent is called (see #43423). This change makes sure that DefaultShardOperationFailedException#toXContent is final and implementations need to add special fields in #innerToXContent. All implementations should write to self-contained xContent objects. Also adding a test for xContent deserialization to CloseIndexResponseTests. Closes #43423 --- .../indices/close/CloseIndexResponse.java | 13 +++--- .../shards/IndicesShardStoresResponse.java | 7 +-- .../DefaultShardOperationFailedException.java | 7 +-- .../close/CloseIndexResponseTests.java | 44 ++++++++++++++++++- 4 files changed, 55 insertions(+), 16 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/close/CloseIndexResponse.java b/server/src/main/java/org/elasticsearch/action/admin/indices/close/CloseIndexResponse.java index c653c264e95..b44d1072181 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/close/CloseIndexResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/close/CloseIndexResponse.java @@ -78,6 +78,7 @@ public class CloseIndexResponse extends ShardsAcknowledgedResponse { } } + @Override protected void addCustomFields(final XContentBuilder builder, final Params params) throws IOException { super.addCustomFields(builder, params); builder.startObject("indices"); @@ -190,7 +191,7 @@ public class CloseIndexResponse extends ShardsAcknowledgedResponse { public static class ShardResult implements Writeable, ToXContentFragment { private final int id; - private final ShardResult.Failure[] failures; + private final Failure[] failures; public ShardResult(final int id, final Failure[] failures) { this.id = id; @@ -199,7 +200,7 @@ public class CloseIndexResponse extends ShardsAcknowledgedResponse { ShardResult(final StreamInput in) throws IOException { this.id = in.readVInt(); - this.failures = in.readOptionalArray(Failure::readFailure, ShardResult.Failure[]::new); + this.failures = in.readOptionalArray(Failure::readFailure, Failure[]::new); } @Override @@ -227,9 +228,7 @@ public class CloseIndexResponse extends ShardsAcknowledgedResponse { builder.startArray("failures"); if (failures != null) { for (Failure failure : failures) { - builder.startObject(); failure.toXContent(builder, params); - builder.endObject(); } } builder.endArray(); @@ -242,7 +241,7 @@ public class CloseIndexResponse extends ShardsAcknowledgedResponse { return Strings.toString(this); } - public static class Failure extends DefaultShardOperationFailedException implements Writeable { + public static class Failure extends DefaultShardOperationFailedException { private @Nullable String nodeId; @@ -275,11 +274,11 @@ public class CloseIndexResponse extends ShardsAcknowledgedResponse { } @Override - public XContentBuilder toXContent(final XContentBuilder builder, final Params params) throws IOException { + public XContentBuilder innerToXContent(final XContentBuilder builder, final Params params) throws IOException { if (nodeId != null) { builder.field("node", nodeId); } - return super.toXContent(builder, params); + return super.innerToXContent(builder, params); } @Override diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/shards/IndicesShardStoresResponse.java b/server/src/main/java/org/elasticsearch/action/admin/indices/shards/IndicesShardStoresResponse.java index 0e2f9f752af..3f4510c8f24 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/indices/shards/IndicesShardStoresResponse.java +++ b/server/src/main/java/org/elasticsearch/action/admin/indices/shards/IndicesShardStoresResponse.java @@ -267,12 +267,9 @@ public class IndicesShardStoresResponse extends ActionResponse implements ToXCon } @Override - public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - builder.startObject(); + public XContentBuilder innerToXContent(XContentBuilder builder, Params params) throws IOException { builder.field("node", nodeId()); - super.innerToXContent(builder, params); - builder.endObject(); - return builder; + return super.innerToXContent(builder, params); } } diff --git a/server/src/main/java/org/elasticsearch/action/support/DefaultShardOperationFailedException.java b/server/src/main/java/org/elasticsearch/action/support/DefaultShardOperationFailedException.java index 7aa7dfb62a6..aa3e91c634a 100644 --- a/server/src/main/java/org/elasticsearch/action/support/DefaultShardOperationFailedException.java +++ b/server/src/main/java/org/elasticsearch/action/support/DefaultShardOperationFailedException.java @@ -25,6 +25,7 @@ import org.elasticsearch.action.ShardOperationFailedException; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; @@ -35,7 +36,7 @@ import java.io.IOException; import static org.elasticsearch.ExceptionsHelper.detailedMessage; import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg; -public class DefaultShardOperationFailedException extends ShardOperationFailedException { +public class DefaultShardOperationFailedException extends ShardOperationFailedException implements Writeable { private static final String INDEX = "index"; private static final String SHARD_ID = "shard"; @@ -90,13 +91,13 @@ public class DefaultShardOperationFailedException extends ShardOperationFailedEx } @Override - public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + public final XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); innerToXContent(builder, params); builder.endObject(); return builder; } - + protected XContentBuilder innerToXContent(XContentBuilder builder, Params params) throws IOException { builder.field("shard", shardId()); builder.field("index", index()); diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/close/CloseIndexResponseTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/close/CloseIndexResponseTests.java index 08516d3a882..c1e31e422bf 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/close/CloseIndexResponseTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/close/CloseIndexResponseTests.java @@ -22,9 +22,14 @@ package org.elasticsearch.action.admin.indices.close; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.Version; import org.elasticsearch.action.NoShardAvailableActionException; +import org.elasticsearch.action.admin.indices.close.CloseIndexResponse.IndexResult; import org.elasticsearch.action.support.master.AcknowledgedResponse; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.Index; import org.elasticsearch.index.IndexNotFoundException; import org.elasticsearch.index.shard.ShardId; @@ -32,8 +37,10 @@ import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.VersionUtils; import org.elasticsearch.transport.ActionNotFoundTransportException; +import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; import java.util.List; import static org.elasticsearch.test.VersionUtils.randomVersionBetween; @@ -46,6 +53,38 @@ import static org.hamcrest.Matchers.nullValue; public class CloseIndexResponseTests extends ESTestCase { + /** + * Test that random responses can be written to xcontent without errors. + * Also check some specific simple cases for output. + */ + public void testToXContent() throws IOException { + CloseIndexResponse response = randomResponse(); + XContentType xContentType = randomFrom(XContentType.values()); + try (XContentBuilder builder = XContentBuilder.builder(xContentType.xContent())) { + response.toXContent(builder, ToXContent.EMPTY_PARAMS); + } + + Index index = new Index("test", "uuid"); + IndexResult indexResult = new CloseIndexResponse.IndexResult(index); + CloseIndexResponse closeIndexResponse = new CloseIndexResponse(true, true, + Collections.singletonList(indexResult)); + assertEquals("{\"acknowledged\":true,\"shards_acknowledged\":true,\"indices\":{\"test\":{\"closed\":true}}}", + Strings.toString(closeIndexResponse)); + + CloseIndexResponse.ShardResult[] shards = new CloseIndexResponse.ShardResult[1]; + shards[0] = new CloseIndexResponse.ShardResult(0, new CloseIndexResponse.ShardResult.Failure[] { + new CloseIndexResponse.ShardResult.Failure("test", 0, new ActionNotFoundTransportException("test"), "nodeId") }); + indexResult = new CloseIndexResponse.IndexResult(index, shards); + closeIndexResponse = new CloseIndexResponse(true, true, + Collections.singletonList(indexResult)); + assertEquals("{\"acknowledged\":true,\"shards_acknowledged\":true," + + "\"indices\":{\"test\":{\"closed\":false,\"failedShards\":{\"0\":{" + + "\"failures\":[{\"node\":\"nodeId\",\"shard\":0,\"index\":\"test\",\"status\":\"INTERNAL_SERVER_ERROR\"," + + "\"reason\":{\"type\":\"action_not_found_transport_exception\"," + + "\"reason\":\"No handler for action [test]\"}}]}}}}}", + Strings.toString(closeIndexResponse)); + } + public void testSerialization() throws Exception { final CloseIndexResponse response = randomResponse(); try (BytesStreamOutput out = new BytesStreamOutput()) { @@ -131,7 +170,10 @@ public class CloseIndexResponseTests extends ESTestCase { acknowledged = false; failures = new CloseIndexResponse.ShardResult.Failure[randomIntBetween(1, 3)]; for (int j = 0; j < failures.length; j++) { - String nodeId = randomAlphaOfLength(5); + String nodeId = null; + if (frequently()) { + nodeId = randomAlphaOfLength(5); + } failures[j] = new CloseIndexResponse.ShardResult.Failure(indexName, i, randomException(index, i), nodeId); } }