Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add parsing methods for UpdateResponse #22586

Merged
merged 7 commits into from
Jan 19, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -292,8 +292,8 @@ protected static void declareParserFields(ConstructingObjectParser<? extends Doc
objParser.declareString(constructorArg(), new ParseField(_ID));
objParser.declareLong(constructorArg(), new ParseField(_VERSION));
objParser.declareString(constructorArg(), new ParseField(RESULT));
objParser.declareObject(constructorArg(), (p, c) -> ShardInfo.fromXContent(p), new ParseField(_SHARDS));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did ShardInfo become a constructor argument?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - I changed it to optionalConstructorArg() since it can be an argument of the UpdateResponse.

objParser.declareLong(optionalConstructorArg(), new ParseField(_SEQ_NO));
objParser.declareBoolean(DocWriteResponse::setForcedRefresh, new ParseField(FORCED_REFRESH));
objParser.declareObject(DocWriteResponse::setShardInfo, (p, c) -> ShardInfo.fromXContent(p), new ParseField(_SHARDS));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,13 @@ 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 created = (boolean) args[6];
return new IndexResponse(shardId, type, id, seqNo, version, created);
ShardInfo shardInfo = (ShardInfo) args[5];
long seqNo = (args[6] != null) ? (long) args[6] : SequenceNumbersService.UNASSIGNED_SEQ_NO;
boolean created = (boolean) args[7];

IndexResponse indexResponse = new IndexResponse(shardId, type, id, seqNo, version, created);
indexResponse.setShardInfo(shardInfo);
return indexResponse;
});
DocWriteResponse.declareParserFields(PARSER);
PARSER.declareBoolean(constructorArg(), new ParseField(CREATED));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,30 @@
package org.elasticsearch.action.update;

import org.elasticsearch.action.DocWriteResponse;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.common.xcontent.XContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.get.GetResult;
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.optionalConstructorArg;

public class UpdateResponse extends DocWriteResponse {

private static final String GET = "get";

private GetResult getResult;

public UpdateResponse() {
Expand Down Expand Up @@ -82,15 +94,11 @@ public void writeTo(StreamOutput out) throws IOException {
}
}

static final class Fields {
static final String GET = "get";
}

@Override
public XContentBuilder innerToXContent(XContentBuilder builder, Params params) throws IOException {
super.innerToXContent(builder, params);
if (getGetResult() != null) {
builder.startObject(Fields.GET);
builder.startObject(GET);
getGetResult().toXContentEmbedded(builder, params);
builder.endObject();
}
Expand All @@ -109,4 +117,65 @@ public String toString() {
builder.append(",shards=").append(getShardInfo());
return builder.append("]").toString();
}


/**
* ConstructingObjectParser used to parse the {@link UpdateResponse}. 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.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove this comment ;) no need to explain why you are using ObjectParser I think :)

private static final ConstructingObjectParser<UpdateResponse, Void> PARSER;
static {
PARSER = new ConstructingObjectParser<>(UpdateResponse.class.getName(),
args -> {
// index uuid and shard id are unknown and can't be parsed back for now.
String index = (String) args[0];
ShardId shardId = new ShardId(new Index(index, IndexMetaData.INDEX_UUID_NA_VALUE), -1);
String type = (String) args[1];
String id = (String) args[2];
long version = (long) args[3];
ShardInfo shardInfo = (ShardInfo) args[5];
Long seqNo = (Long) args[6];

Result result = null;
for (Result r : Result.values()) {
if (r.getLowercase().equals(args[4])) {
result = r;
break;
}
}

UpdateResponse updateResponse = null;
if (shardInfo != null && seqNo != null) {
updateResponse = new UpdateResponse(shardInfo, shardId, type, id, seqNo, version, result);
} else {
updateResponse = new UpdateResponse(shardId, type, id, version, result);
}

GetResult get = (GetResult) args[7];
if (get != null) {
GetResult getResult = new GetResult(index, type, id, version, get.isExists(), get.sourceRef(), get.getFields());
updateResponse.setGetResult(getResult);
}
return updateResponse;
});

DocWriteResponse.declareParserFields(PARSER);
PARSER.declareObject(optionalConstructorArg(), (p, c) -> {
XContent xContent = p.contentType().xContent();
try (XContentBuilder builder = XContentBuilder.builder(xContent)) {
// "get" field contains an embedded version of {@link GetResult} and requires
// to be parsed as if it was a full XContent object.
XContentHelper.copyCurrentStructure(builder.generator(), p);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we copying this? because the xContentType can differ between the response and the get section?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same question

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because the xContentType can differ between the response and the get section?

No, this has been a crappy hack around the fact that GetResult can be rendered as a ToXContentObject using toXContent() or as a fragment using toXContentEmbedded().

I changed that so that GetResult now has a fromXContentEmbedded() parsing method that can be used in UpdateResponse's object parser.

Note that GetResult.toXContent() delegates all the work to fromXContentEmbedded(). Please let me know what you think about this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ thanks

try (XContentParser parser = xContent.createParser(NamedXContentRegistry.EMPTY, builder.bytes())) {
return GetResult.fromXContent(parser);
}
}
}, new ParseField(GET));
}

public static UpdateResponse fromXContent(XContentParser parser) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: IOException is not thrown

return PARSER.apply(parser, null);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.elasticsearch.action.index;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.action.DocWriteResponse;
import org.elasticsearch.action.support.replication.ReplicationResponse;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
Expand Down Expand Up @@ -81,7 +82,7 @@ public void testToAndFromXContent() throws IOException {
}
}

private static void assertIndexResponse(IndexResponse expected, Map<String, Object> actual) {
public static void assertDocWriteResponse(DocWriteResponse expected, Map<String, Object> actual) {
assertEquals(expected.getIndex(), actual.get("_index"));
assertEquals(expected.getType(), actual.get("_type"));
assertEquals(expected.getId(), actual.get("_id"));
Expand Down Expand Up @@ -151,6 +152,15 @@ private static void assertIndexResponse(IndexResponse expected, Map<String, Obje
}
}

private static void assertIndexResponse(IndexResponse expected, Map<String, Object> actual) {
assertDocWriteResponse(expected, actual);
if (expected.getResult() == DocWriteResponse.Result.CREATED) {
assertTrue((boolean) actual.get("created"));
} else {
assertFalse((boolean) actual.get("created"));
}
}

private static IndexResponse randomIndexResponse() {
ShardId shardId = new ShardId(randomAsciiOfLength(5), randomAsciiOfLength(5), randomIntBetween(0, 5));
String type = randomAsciiOfLength(5);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
/*
* 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.update;

import org.elasticsearch.action.DocWriteResponse;
import org.elasticsearch.action.index.IndexResponseTests;
import org.elasticsearch.action.support.replication.ReplicationResponse;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.index.get.GetField;
import org.elasticsearch.index.get.GetResult;
import org.elasticsearch.index.get.GetResultTests;
import org.elasticsearch.index.shard.ShardId;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.RandomObjects;

import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

import static org.elasticsearch.action.DocWriteResponse.Result.DELETED;
import static org.elasticsearch.action.DocWriteResponse.Result.NOT_FOUND;
import static org.elasticsearch.action.DocWriteResponse.Result.UPDATED;
import static org.elasticsearch.common.xcontent.XContentHelper.toXContent;

public class UpdateResponseTests extends ESTestCase {

public void testToXContent() throws IOException {
{
UpdateResponse updateResponse = new UpdateResponse(new ShardId("index", "index_uuid", 0), "type", "id", 0, NOT_FOUND);
String output = Strings.toString(updateResponse);
assertEquals("{\"_index\":\"index\",\"_type\":\"type\",\"_id\":\"id\",\"_version\":0,\"result\":\"not_found\"," +
"\"_shards\":{\"total\":0,\"successful\":0,\"failed\":0}}", output);
}
{
UpdateResponse updateResponse = new UpdateResponse(new ReplicationResponse.ShardInfo(10, 6),
new ShardId("index", "index_uuid", 1), "type", "id", 3, 1, DELETED);
String output = Strings.toString(updateResponse);
assertEquals("{\"_index\":\"index\",\"_type\":\"type\",\"_id\":\"id\",\"_version\":1,\"result\":\"deleted\"," +
"\"_shards\":{\"total\":10,\"successful\":6,\"failed\":0},\"_seq_no\":3}", output);
}
{
BytesReference source = new BytesArray("{\"title\":\"Book title\",\"isbn\":\"ABC-123\"}");
Map<String, GetField> fields = new HashMap<>();
fields.put("title", new GetField("title", Collections.singletonList("Book title")));
fields.put("isbn", new GetField("isbn", Collections.singletonList("ABC-123")));

UpdateResponse updateResponse = new UpdateResponse(new ReplicationResponse.ShardInfo(3, 2),
new ShardId("books", "books_uuid", 2), "book", "1", 7, 2, UPDATED);
updateResponse.setGetResult(new GetResult("books", "book", "1", 2, true, source, fields));

String output = Strings.toString(updateResponse);
assertEquals("{\"_index\":\"books\",\"_type\":\"book\",\"_id\":\"1\",\"_version\":2,\"result\":\"updated\"," +
"\"_shards\":{\"total\":3,\"successful\":2,\"failed\":0},\"_seq_no\":7,\"get\":{\"found\":true," +
"\"_source\":{\"title\":\"Book title\",\"isbn\":\"ABC-123\"},\"fields\":{\"isbn\":[\"ABC-123\"],\"title\":[\"Book " +
"title\"]}}}", output);
}
}

public void testToAndFromXContent() throws IOException {
final XContentType xContentType = randomFrom(XContentType.values());

// Create a random UpdateResponse and converts it to XContent in bytes
UpdateResponse updateResponse = randomUpdateResponse();
BytesReference updateResponseBytes = toXContent(updateResponse, xContentType);

// Parse the XContent bytes to obtain a parsed UpdateResponse
UpdateResponse parsedUpdateResponse;
try (XContentParser parser = createParser(xContentType.xContent(), updateResponseBytes)) {
parsedUpdateResponse = UpdateResponse.fromXContent(parser);
assertNull(parser.nextToken());
}

// We can't use equals() to compare the original and the parsed update response
// because the random update response can contain shard failures with exceptions,
// and those exceptions are not parsed back with the same types.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we rather generate responses without exceptions and test them like we usually do, and test exceptions separately?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed that part to do something slightly different. We can't compare UpdateResponse using equals() because it can have shard failures AND the GetResult object embedded in the update response is a bit different after parsing (it does not have index/type/id/version).

For the shard failures, we already have IndexResponseTests.assertDocWriteResponse() which takes care of checking shard failures, so I reused it.

For the GetResult, I used something similar to the GetResultTests where it generates and returns a Tuple of actual and expected GetResult, and I set to null the index/type/etc values expected for the parsed GetResult.


// Print the parsed object out and test that the output is the same as the original output
BytesReference parsedUpdateResponseBytes = toXContent(parsedUpdateResponse, xContentType);
try (XContentParser parser = createParser(xContentType.xContent(), parsedUpdateResponseBytes)) {
assertUpdateResponse(updateResponse, parser.map());
}
}

private static void assertUpdateResponse(UpdateResponse expected, Map<String, Object> actual) {
IndexResponseTests.assertDocWriteResponse(expected, actual);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we have to check also the get result part?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course, how can I forgot that? It's like running a 100 meters and forget to pass the finish line...

}

private static UpdateResponse randomUpdateResponse() {
ShardId shardId = new ShardId(randomAsciiOfLength(5), randomAsciiOfLength(5), randomIntBetween(0, 5));
String type = randomAsciiOfLength(5);
String id = randomAsciiOfLength(5);
DocWriteResponse.Result result = randomFrom(DocWriteResponse.Result.values());
long version = (long) randomIntBetween(0, 5);
boolean forcedRefresh = randomBoolean();

UpdateResponse updateResponse = null;
if (rarely()) {
updateResponse = new UpdateResponse(shardId, type, id, version, result);
} else {
long seqNo = randomIntBetween(0, 5);
ReplicationResponse.ShardInfo shardInfo = RandomObjects.randomShardInfo(random(), randomBoolean());
updateResponse = new UpdateResponse(shardInfo, shardId, type, id, seqNo, version, result);

GetResult getResult = GetResultTests.randomGetResult(randomFrom(XContentType.values())).v1();
updateResponse.setGetResult(getResult);
}
updateResponse.setForcedRefresh(forcedRefresh);
return updateResponse;
}
}