From 718a6b9be7807e8d158556862b3b67203f2b94c5 Mon Sep 17 00:00:00 2001 From: David Pilato Date: Wed, 18 Jan 2017 15:23:07 +0100 Subject: [PATCH 1/3] Add fromxcontent methods to delete response This commit adds the parsing fromXContent() methods to the IndexResponse class. It's a pale copy of what has been done in #22229. --- .../action/delete/DeleteResponse.java | 40 ++++- .../action/delete/DeleteResponseTests.java | 169 ++++++++++++++++++ 2 files changed, 208 insertions(+), 1 deletion(-) create mode 100644 core/src/test/java/org/elasticsearch/action/delete/DeleteResponseTests.java diff --git a/core/src/main/java/org/elasticsearch/action/delete/DeleteResponse.java b/core/src/main/java/org/elasticsearch/action/delete/DeleteResponse.java index 11d06c166b6a0..4804c2a443cee 100644 --- a/core/src/main/java/org/elasticsearch/action/delete/DeleteResponse.java +++ b/core/src/main/java/org/elasticsearch/action/delete/DeleteResponse.java @@ -20,12 +20,21 @@ package org.elasticsearch.action.delete; import org.elasticsearch.action.DocWriteResponse; +import org.elasticsearch.action.index.IndexResponse; +import org.elasticsearch.cluster.metadata.IndexMetaData; +import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.index.Index; +import org.elasticsearch.index.seqno.SequenceNumbersService; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.rest.RestStatus; import java.io.IOException; +import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg; + /** * The response of the delete action. * @@ -34,6 +43,8 @@ */ public class DeleteResponse extends DocWriteResponse { + private static final String FOUND = "found"; + public DeleteResponse() { } @@ -49,11 +60,38 @@ public RestStatus status() { @Override public XContentBuilder innerToXContent(XContentBuilder builder, Params params) throws IOException { - builder.field("found", result == Result.DELETED); + builder.field(FOUND, result == Result.DELETED); super.innerToXContent(builder, params); return builder; } + /** + * ConstructingObjectParser used to parse the {@link IndexResponse}. We use a ObjectParser here + * because most fields are parsed by the parent abstract class {@link DocWriteResponse} and it's + * not easy to parse part of the fields in the parent class and other fields in the children class + * using the usual streamed parsing method. + */ + private static final ConstructingObjectParser PARSER; + static { + PARSER = new ConstructingObjectParser<>(DeleteResponse.class.getName(), + args -> { + // index uuid and shard id are unknown and can't be parsed back for now. + ShardId shardId = new ShardId(new Index((String) args[0], IndexMetaData.INDEX_UUID_NA_VALUE), -1); + String type = (String) args[1]; + String id = (String) args[2]; + long version = (long) args[3]; + long seqNo = (args[5] != null) ? (long) args[5] : SequenceNumbersService.UNASSIGNED_SEQ_NO; + boolean found = (boolean) args[6]; + return new DeleteResponse(shardId, type, id, seqNo, version, found); + }); + DocWriteResponse.declareParserFields(PARSER); + PARSER.declareBoolean(constructorArg(), new ParseField(FOUND)); + } + + public static DeleteResponse fromXContent(XContentParser parser) throws IOException { + return PARSER.apply(parser, null); + } + @Override public String toString() { StringBuilder builder = new StringBuilder(); diff --git a/core/src/test/java/org/elasticsearch/action/delete/DeleteResponseTests.java b/core/src/test/java/org/elasticsearch/action/delete/DeleteResponseTests.java new file mode 100644 index 0000000000000..444d59003dd30 --- /dev/null +++ b/core/src/test/java/org/elasticsearch/action/delete/DeleteResponseTests.java @@ -0,0 +1,169 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.action.delete; + +import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.action.index.IndexResponse; +import org.elasticsearch.action.support.replication.ReplicationResponse; +import org.elasticsearch.common.Strings; +import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.util.CollectionUtils; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.index.shard.ShardId; +import org.elasticsearch.rest.RestStatus; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.RandomObjects; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +import static org.elasticsearch.common.xcontent.XContentHelper.toXContent; + +public class DeleteResponseTests extends ESTestCase { + + public void testToXContent() throws IOException { + { + DeleteResponse response = new DeleteResponse(new ShardId("index", "index_uuid", 0), "type", "id", 3, 5, true); + String output = Strings.toString(response); + assertEquals("{\"found\":true,\"_index\":\"index\",\"_type\":\"type\",\"_id\":\"id\",\"_version\":5,\"result\":\"deleted\"," + + "\"_shards\":null,\"_seq_no\":3}", output); + } + { + DeleteResponse response = new DeleteResponse(new ShardId("index", "index_uuid", 0), "type", "id", -1, 7, true); + response.setForcedRefresh(true); + response.setShardInfo(new ReplicationResponse.ShardInfo(10, 5)); + String output = Strings.toString(response); + assertEquals("{\"found\":true,\"_index\":\"index\",\"_type\":\"type\",\"_id\":\"id\",\"_version\":7,\"result\":\"deleted\"," + + "\"forced_refresh\":true,\"_shards\":{\"total\":10,\"successful\":5,\"failed\":0}}", output); + } + } + + public void testToAndFromXContent() throws IOException { + final XContentType xContentType = randomFrom(XContentType.values()); + + // Create a random DeleteResponse and converts it to XContent in bytes + DeleteResponse response = randomDeleteResponse(); + BytesReference responseBytes = toXContent(response, xContentType); + + // Parse the XContent bytes to obtain a parsed + DeleteResponse parsedDeleteResponse; + try (XContentParser parser = createParser(xContentType.xContent(), responseBytes)) { + parsedDeleteResponse = DeleteResponse.fromXContent(parser); + assertNull(parser.nextToken()); + } + + // We can't use equals() to compare the original and the parsed index response + // because the random index response can contain shard failures with exceptions, + // and those exceptions are not parsed back with the same types. + + // Print the parsed object out and test that the output is the same as the original output + BytesReference parsedDeleteResponseBytes = toXContent(parsedDeleteResponse, xContentType); + try (XContentParser parser = createParser(xContentType.xContent(), parsedDeleteResponseBytes)) { + assertDeleteResponse(response, parser.map()); + } + } + + private static void assertDeleteResponse(DeleteResponse expected, Map actual) { + assertEquals(expected.getIndex(), actual.get("_index")); + assertEquals(expected.getType(), actual.get("_type")); + assertEquals(expected.getId(), actual.get("_id")); + assertEquals(expected.getVersion(), ((Integer) actual.get("_version")).longValue()); + assertEquals(expected.getResult().getLowercase(), actual.get("result")); + if (expected.forcedRefresh()) { + assertTrue((Boolean) actual.get("forced_refresh")); + } else { + assertFalse(actual.containsKey("forced_refresh")); + } + if (expected.getSeqNo() >= 0) { + assertEquals(expected.getSeqNo(), ((Integer) actual.get("_seq_no")).longValue()); + } else { + assertFalse(actual.containsKey("_seq_no")); + } + + Map actualShards = (Map) actual.get("_shards"); + assertNotNull(actualShards); + assertEquals(expected.getShardInfo().getTotal(), actualShards.get("total")); + assertEquals(expected.getShardInfo().getSuccessful(), actualShards.get("successful")); + assertEquals(expected.getShardInfo().getFailed(), actualShards.get("failed")); + + List> actualFailures = (List>) actualShards.get("failures"); + if (CollectionUtils.isEmpty(expected.getShardInfo().getFailures())) { + assertNull(actualFailures); + } else { + assertEquals(expected.getShardInfo().getFailures().length, actualFailures.size()); + for (int i = 0; i < expected.getShardInfo().getFailures().length; i++) { + ReplicationResponse.ShardInfo.Failure failure = expected.getShardInfo().getFailures()[i]; + Map actualFailure = actualFailures.get(i); + + assertEquals(failure.index(), actualFailure.get("_index")); + assertEquals(failure.shardId(), actualFailure.get("_shard")); + assertEquals(failure.nodeId(), actualFailure.get("_node")); + assertEquals(failure.status(), RestStatus.valueOf((String) actualFailure.get("status"))); + assertEquals(failure.primary(), actualFailure.get("primary")); + + Throwable cause = failure.getCause(); + Map actualClause = (Map) actualFailure.get("reason"); + assertNotNull(actualClause); + while (cause != null) { + // The expected IndexResponse has been converted in XContent, then the resulting bytes have been + // parsed to create a new parsed IndexResponse. During this process, the type of the exceptions + // have been lost. + assertEquals("exception", actualClause.get("type")); + String expectedMessage = "Elasticsearch exception [type=" + ElasticsearchException.getExceptionName(cause) + + ", reason=" + cause.getMessage() + "]"; + assertEquals(expectedMessage, actualClause.get("reason")); + + if (cause instanceof ElasticsearchException) { + ElasticsearchException ex = (ElasticsearchException) cause; + Map actualHeaders = (Map) actualClause.get("header"); + + // When a DeleteResponse is converted to XContent, the exception headers that start with "es." + // are added to the XContent as fields with the prefix removed. Other headers are added under + // a "header" root object. + // In the test, the "es." prefix is lost when the XContent is generating, so when the parsed + // IndexResponse is converted back to XContent all exception headers are under the "header" object. + for (String name : ex.getHeaderKeys()) { + assertEquals(ex.getHeader(name).get(0), actualHeaders.get(name.replaceFirst("es.", ""))); + } + } + actualClause = (Map) actualClause.get("caused_by"); + cause = cause.getCause(); + } + } + } + } + + private static DeleteResponse randomDeleteResponse() { + ShardId shardId = new ShardId(randomAsciiOfLength(5), randomAsciiOfLength(5), randomIntBetween(0, 5)); + String type = randomAsciiOfLength(5); + String id = randomAsciiOfLength(5); + long seqNo = randomIntBetween(-2, 5); + long version = (long) randomIntBetween(0, 5); + boolean created = randomBoolean(); + + DeleteResponse response = new DeleteResponse(shardId, type, id, seqNo, version, created); + response.setForcedRefresh(randomBoolean()); + response.setShardInfo(RandomObjects.randomShardInfo(random(), randomBoolean())); + return response; + } + +} From 0315dcc30611d48e79385ee3a716ee639d24f53a Mon Sep 17 00:00:00 2001 From: David Pilato Date: Thu, 19 Jan 2017 16:10:13 +0100 Subject: [PATCH 2/3] Use now common methods with index/update Brought by #22229 --- .../action/delete/DeleteResponse.java | 15 ++-- .../action/delete/DeleteResponseTests.java | 83 ++----------------- 2 files changed, 13 insertions(+), 85 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/delete/DeleteResponse.java b/core/src/main/java/org/elasticsearch/action/delete/DeleteResponse.java index 4804c2a443cee..b5a4d74d620ea 100644 --- a/core/src/main/java/org/elasticsearch/action/delete/DeleteResponse.java +++ b/core/src/main/java/org/elasticsearch/action/delete/DeleteResponse.java @@ -65,12 +65,6 @@ public XContentBuilder innerToXContent(XContentBuilder builder, Params params) t return builder; } - /** - * ConstructingObjectParser used to parse the {@link IndexResponse}. We use a ObjectParser here - * because most fields are parsed by the parent abstract class {@link DocWriteResponse} and it's - * not easy to parse part of the fields in the parent class and other fields in the children class - * using the usual streamed parsing method. - */ private static final ConstructingObjectParser PARSER; static { PARSER = new ConstructingObjectParser<>(DeleteResponse.class.getName(), @@ -80,9 +74,12 @@ public XContentBuilder innerToXContent(XContentBuilder builder, Params params) t String type = (String) args[1]; String id = (String) args[2]; long version = (long) args[3]; - long seqNo = (args[5] != null) ? (long) args[5] : SequenceNumbersService.UNASSIGNED_SEQ_NO; - boolean found = (boolean) args[6]; - return new DeleteResponse(shardId, type, id, seqNo, version, found); + ShardInfo shardInfo = (ShardInfo) args[5]; + long seqNo = (args[6] != null) ? (long) args[6] : SequenceNumbersService.UNASSIGNED_SEQ_NO; + boolean found = (boolean) args[7]; + DeleteResponse deleteResponse = new DeleteResponse(shardId, type, id, seqNo, version, found); + deleteResponse.setShardInfo(shardInfo); + return deleteResponse; }); DocWriteResponse.declareParserFields(PARSER); PARSER.declareBoolean(constructorArg(), new ParseField(FOUND)); diff --git a/core/src/test/java/org/elasticsearch/action/delete/DeleteResponseTests.java b/core/src/test/java/org/elasticsearch/action/delete/DeleteResponseTests.java index 444d59003dd30..588f00238bfb5 100644 --- a/core/src/test/java/org/elasticsearch/action/delete/DeleteResponseTests.java +++ b/core/src/test/java/org/elasticsearch/action/delete/DeleteResponseTests.java @@ -36,6 +36,7 @@ import java.util.List; import java.util.Map; +import static org.elasticsearch.action.index.IndexResponseTests.assertDocWriteResponse; import static org.elasticsearch.common.xcontent.XContentHelper.toXContent; public class DeleteResponseTests extends ESTestCase { @@ -61,12 +62,12 @@ public void testToAndFromXContent() throws IOException { final XContentType xContentType = randomFrom(XContentType.values()); // Create a random DeleteResponse and converts it to XContent in bytes - DeleteResponse response = randomDeleteResponse(); - BytesReference responseBytes = toXContent(response, xContentType); + DeleteResponse deleteResponse = randomDeleteResponse(); + BytesReference deleteResponseBytes = toXContent(deleteResponse, xContentType); // Parse the XContent bytes to obtain a parsed DeleteResponse parsedDeleteResponse; - try (XContentParser parser = createParser(xContentType.xContent(), responseBytes)) { + try (XContentParser parser = createParser(xContentType.xContent(), deleteResponseBytes)) { parsedDeleteResponse = DeleteResponse.fromXContent(parser); assertNull(parser.nextToken()); } @@ -78,77 +79,7 @@ public void testToAndFromXContent() throws IOException { // Print the parsed object out and test that the output is the same as the original output BytesReference parsedDeleteResponseBytes = toXContent(parsedDeleteResponse, xContentType); try (XContentParser parser = createParser(xContentType.xContent(), parsedDeleteResponseBytes)) { - assertDeleteResponse(response, parser.map()); - } - } - - private static void assertDeleteResponse(DeleteResponse expected, Map actual) { - assertEquals(expected.getIndex(), actual.get("_index")); - assertEquals(expected.getType(), actual.get("_type")); - assertEquals(expected.getId(), actual.get("_id")); - assertEquals(expected.getVersion(), ((Integer) actual.get("_version")).longValue()); - assertEquals(expected.getResult().getLowercase(), actual.get("result")); - if (expected.forcedRefresh()) { - assertTrue((Boolean) actual.get("forced_refresh")); - } else { - assertFalse(actual.containsKey("forced_refresh")); - } - if (expected.getSeqNo() >= 0) { - assertEquals(expected.getSeqNo(), ((Integer) actual.get("_seq_no")).longValue()); - } else { - assertFalse(actual.containsKey("_seq_no")); - } - - Map actualShards = (Map) actual.get("_shards"); - assertNotNull(actualShards); - assertEquals(expected.getShardInfo().getTotal(), actualShards.get("total")); - assertEquals(expected.getShardInfo().getSuccessful(), actualShards.get("successful")); - assertEquals(expected.getShardInfo().getFailed(), actualShards.get("failed")); - - List> actualFailures = (List>) actualShards.get("failures"); - if (CollectionUtils.isEmpty(expected.getShardInfo().getFailures())) { - assertNull(actualFailures); - } else { - assertEquals(expected.getShardInfo().getFailures().length, actualFailures.size()); - for (int i = 0; i < expected.getShardInfo().getFailures().length; i++) { - ReplicationResponse.ShardInfo.Failure failure = expected.getShardInfo().getFailures()[i]; - Map actualFailure = actualFailures.get(i); - - assertEquals(failure.index(), actualFailure.get("_index")); - assertEquals(failure.shardId(), actualFailure.get("_shard")); - assertEquals(failure.nodeId(), actualFailure.get("_node")); - assertEquals(failure.status(), RestStatus.valueOf((String) actualFailure.get("status"))); - assertEquals(failure.primary(), actualFailure.get("primary")); - - Throwable cause = failure.getCause(); - Map actualClause = (Map) actualFailure.get("reason"); - assertNotNull(actualClause); - while (cause != null) { - // The expected IndexResponse has been converted in XContent, then the resulting bytes have been - // parsed to create a new parsed IndexResponse. During this process, the type of the exceptions - // have been lost. - assertEquals("exception", actualClause.get("type")); - String expectedMessage = "Elasticsearch exception [type=" + ElasticsearchException.getExceptionName(cause) - + ", reason=" + cause.getMessage() + "]"; - assertEquals(expectedMessage, actualClause.get("reason")); - - if (cause instanceof ElasticsearchException) { - ElasticsearchException ex = (ElasticsearchException) cause; - Map actualHeaders = (Map) actualClause.get("header"); - - // When a DeleteResponse is converted to XContent, the exception headers that start with "es." - // are added to the XContent as fields with the prefix removed. Other headers are added under - // a "header" root object. - // In the test, the "es." prefix is lost when the XContent is generating, so when the parsed - // IndexResponse is converted back to XContent all exception headers are under the "header" object. - for (String name : ex.getHeaderKeys()) { - assertEquals(ex.getHeader(name).get(0), actualHeaders.get(name.replaceFirst("es.", ""))); - } - } - actualClause = (Map) actualClause.get("caused_by"); - cause = cause.getCause(); - } - } + assertDocWriteResponse(deleteResponse, parser.map()); } } @@ -158,9 +89,9 @@ private static DeleteResponse randomDeleteResponse() { String id = randomAsciiOfLength(5); long seqNo = randomIntBetween(-2, 5); long version = (long) randomIntBetween(0, 5); - boolean created = randomBoolean(); + boolean found = randomBoolean(); - DeleteResponse response = new DeleteResponse(shardId, type, id, seqNo, version, created); + DeleteResponse response = new DeleteResponse(shardId, type, id, seqNo, version, found); response.setForcedRefresh(randomBoolean()); response.setShardInfo(RandomObjects.randomShardInfo(random(), randomBoolean())); return response; From 5be8bd76e24bfba1fb15af2a446bfcc905464411 Mon Sep 17 00:00:00 2001 From: David Pilato Date: Thu, 19 Jan 2017 17:28:31 +0100 Subject: [PATCH 3/3] Also test found field And optimize imports --- .../action/delete/DeleteResponseTests.java | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/core/src/test/java/org/elasticsearch/action/delete/DeleteResponseTests.java b/core/src/test/java/org/elasticsearch/action/delete/DeleteResponseTests.java index 588f00238bfb5..368add2a850ad 100644 --- a/core/src/test/java/org/elasticsearch/action/delete/DeleteResponseTests.java +++ b/core/src/test/java/org/elasticsearch/action/delete/DeleteResponseTests.java @@ -19,21 +19,17 @@ package org.elasticsearch.action.delete; -import org.elasticsearch.ElasticsearchException; -import org.elasticsearch.action.index.IndexResponse; +import org.elasticsearch.action.DocWriteResponse; import org.elasticsearch.action.support.replication.ReplicationResponse; import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesReference; -import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.index.shard.ShardId; -import org.elasticsearch.rest.RestStatus; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.RandomObjects; import java.io.IOException; -import java.util.List; import java.util.Map; import static org.elasticsearch.action.index.IndexResponseTests.assertDocWriteResponse; @@ -79,7 +75,16 @@ public void testToAndFromXContent() throws IOException { // Print the parsed object out and test that the output is the same as the original output BytesReference parsedDeleteResponseBytes = toXContent(parsedDeleteResponse, xContentType); try (XContentParser parser = createParser(xContentType.xContent(), parsedDeleteResponseBytes)) { - assertDocWriteResponse(deleteResponse, parser.map()); + assertDeleteResponse(deleteResponse, parser.map()); + } + } + + private static void assertDeleteResponse(DeleteResponse expected, Map actual) { + assertDocWriteResponse(expected, actual); + if (expected.getResult() == DocWriteResponse.Result.DELETED) { + assertTrue((boolean) actual.get("found")); + } else { + assertFalse((boolean) actual.get("found")); } }