Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 37 additions & 8 deletions core/src/main/java/org/elasticsearch/action/DocWriteResponse.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,12 @@
import org.elasticsearch.action.support.WriteResponse;
import org.elasticsearch.action.support.replication.ReplicationResponse;
import org.elasticsearch.common.Nullable;
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.ObjectParser;
import org.elasticsearch.common.xcontent.StatusToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.index.IndexSettings;
Expand All @@ -39,11 +42,23 @@
import java.net.URISyntaxException;
import java.util.Locale;

import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;

/**
* A base class for the response of a write operation that involves a single doc
*/
public abstract class DocWriteResponse extends ReplicationResponse implements WriteResponse, StatusToXContentObject {

private static final String _SHARDS = "_shards";
private static final String _INDEX = "_index";
private static final String _TYPE = "_type";
private static final String _ID = "_id";
private static final String _VERSION = "_version";
private static final String _SEQ_NO = "_seq_no";
private static final String RESULT = "result";
private static final String FORCED_REFRESH = "forced_refresh";

/**
* An enum that represents the the results of CRUD operations, primarily used to communicate the type of
* operation that occurred.
Expand Down Expand Up @@ -253,18 +268,32 @@ public final XContentBuilder toXContent(XContentBuilder builder, Params params)

public XContentBuilder innerToXContent(XContentBuilder builder, Params params) throws IOException {
ReplicationResponse.ShardInfo shardInfo = getShardInfo();
builder.field("_index", shardId.getIndexName())
.field("_type", type)
.field("_id", id)
.field("_version", version)
.field("result", getResult().getLowercase());
builder.field(_INDEX, shardId.getIndexName())
.field(_TYPE, type)
.field(_ID, id)
.field(_VERSION, version)
.field(RESULT, getResult().getLowercase());
if (forcedRefresh) {
builder.field("forced_refresh", true);
builder.field(FORCED_REFRESH, true);
}
shardInfo.toXContent(builder, params);
builder.field(_SHARDS, shardInfo);
if (getSeqNo() >= 0) {
builder.field("_seq_no", getSeqNo());
builder.field(_SEQ_NO, getSeqNo());
}
return builder;
}

/**
* Declare the {@link ObjectParser} fields to use when parsing a {@link DocWriteResponse}
*/
protected static void declareParserFields(ConstructingObjectParser<? extends DocWriteResponse, Void> objParser) {
objParser.declareString(constructorArg(), new ParseField(_INDEX));
objParser.declareString(constructorArg(), new ParseField(_TYPE));
objParser.declareString(constructorArg(), new ParseField(_ID));
objParser.declareLong(constructorArg(), new ParseField(_VERSION));
objParser.declareString(constructorArg(), new ParseField(RESULT));
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 @@ -20,13 +20,21 @@
package org.elasticsearch.action.index;

import org.elasticsearch.action.DocWriteResponse;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.Strings;
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;

/**
* A response of an index operation,
*
Expand All @@ -35,6 +43,8 @@
*/
public class IndexResponse extends DocWriteResponse {

private static final String CREATED = "created";

public IndexResponse() {
}

Expand Down Expand Up @@ -64,7 +74,34 @@ public String toString() {
@Override
public XContentBuilder innerToXContent(XContentBuilder builder, Params params) throws IOException {
super.innerToXContent(builder, params);
builder.field("created", result == Result.CREATED);
builder.field(CREATED, result == Result.CREATED);
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<IndexResponse, Void> PARSER;
static {
PARSER = new ConstructingObjectParser<>(IndexResponse.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);
Copy link
Member

Choose a reason for hiding this comment

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

it is quite odd that the only info we actually can parse back out of the ShardId is the index name. no index_uuid and no shard_id. Maybe we should look into carrying around the index name only as a follow-up in DocWriteResponse. I am not sure we need more, it doesn't seem like it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, that makes sense. I can change that in a follow up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

We talked about this with Luca and we both think that we should not do this change. This would impact the serialization of DocWriteResponse in core in order to simplify things and carry less information from a client side point of view.

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);
});
DocWriteResponse.declareParserFields(PARSER);
PARSER.declareBoolean(constructorArg(), new ParseField(CREATED));
}

public static IndexResponse fromXContent(XContentParser parser) throws IOException {
return PARSER.apply(parser, null);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public void setShardInfo(ShardInfo shardInfo) {
this.shardInfo = shardInfo;
}

public static class ShardInfo implements Streamable, ToXContent {
public static class ShardInfo implements Streamable, ToXContentObject {
Copy link
Member

Choose a reason for hiding this comment

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

thanks for changing this!


private static final String _SHARDS = "_shards";
private static final String TOTAL = "total";
Expand Down Expand Up @@ -179,7 +179,7 @@ public void writeTo(StreamOutput out) throws IOException {

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject(_SHARDS);
builder.startObject();
builder.field(TOTAL, total);
Copy link
Member Author

Choose a reason for hiding this comment

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

That's the only bit that changed since #22196

Copy link
Member

Choose a reason for hiding this comment

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

ok, I think I had made you revert this change before, but maybe it's ok because we only print this section out in one place, so the _SHARDS name doesn't get copied to other places. One thing: what does this specific change improve? That ShardInfo outputs a valid object alone?

Copy link
Member

Choose a reason for hiding this comment

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

I'm also curious to understand where the rendering of the field was moved, I don't seem to find it in this PR. What is the outer object that is calling this toXContent Method?

Copy link
Member Author

Choose a reason for hiding this comment

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

It has been moved to DocWriteResponse.innerToXContent() where

shardInfo.toXContent(builder, params);

has been changed to:

builder.field(_SHARDS, shardInfo);

so that ShardInfo is now a ToXContentObject.

This is important for the ObjectParser used to parse the IndexResponse which can then directly use ShardInfo.fromXContent(p).

builder.field(SUCCESSFUL, successful);
builder.field(FAILED, getFailed());
Expand All @@ -195,18 +195,12 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
}

public static ShardInfo fromXContent(XContentParser parser) throws IOException {
XContentParser.Token token = parser.nextToken();
ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser::getTokenLocation);

String currentFieldName = parser.currentName();
if (_SHARDS.equals(currentFieldName) == false) {
throwUnknownField(currentFieldName, parser.getTokenLocation());
}
token = parser.nextToken();
XContentParser.Token token = parser.currentToken();
ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser::getTokenLocation);

int total = 0, successful = 0;
List<Failure> failuresList = null;
String currentFieldName = null;
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public void testIndexResponse() {
assertEquals("IndexResponse[index=" + shardId.getIndexName() + ",type=" + type + ",id="+ id +
",version=" + version + ",result=" + (created ? "created" : "updated") +
",seqNo=" + SequenceNumbersService.UNASSIGNED_SEQ_NO +
",shards={\"_shards\":{\"total\":" + total + ",\"successful\":" + successful + ",\"failed\":0}}]",
",shards={\"total\":" + total + ",\"successful\":" + successful + ",\"failed\":0}]",
Copy link
Member

Choose a reason for hiding this comment

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

Also just to understand this: this change here only affects the toString() method, not the actual rest response?

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

indexResponse.toString());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
/*
* 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.index;

import org.elasticsearch.ElasticsearchException;
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 IndexResponseTests extends ESTestCase {

public void testToXContent() throws IOException {
{
IndexResponse indexResponse = new IndexResponse(new ShardId("index", "index_uuid", 0), "type", "id", 3, 5, true);
String output = Strings.toString(indexResponse);
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity: why does this test the toString() method, the tests seems to indicate its about the xContent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh right - this is because Strings.toString() uses the toXContent method internally. I can replace this by a direct usage of toXContent.

assertEquals("{\"_index\":\"index\",\"_type\":\"type\",\"_id\":\"id\",\"_version\":5,\"result\":\"created\",\"_shards\":null," +
"\"_seq_no\":3,\"created\":true}", output);
}
{
IndexResponse indexResponse = new IndexResponse(new ShardId("index", "index_uuid", 0), "type", "id", -1, 7, true);
indexResponse.setForcedRefresh(true);
indexResponse.setShardInfo(new ReplicationResponse.ShardInfo(10, 5));
String output = Strings.toString(indexResponse);
assertEquals("{\"_index\":\"index\",\"_type\":\"type\",\"_id\":\"id\",\"_version\":7,\"result\":\"created\"," +
"\"forced_refresh\":true,\"_shards\":{\"total\":10,\"successful\":5,\"failed\":0},\"created\":true}", output);
}
}

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

// Create a random IndexResponse and converts it to XContent in bytes
IndexResponse indexResponse = randomIndexResponse();
BytesReference indexResponseBytes = toXContent(indexResponse, xContentType);

// Parse the XContent bytes to obtain a parsed
IndexResponse parsedIndexResponse;
try (XContentParser parser = createParser(xContentType.xContent(), indexResponseBytes)) {
parsedIndexResponse = IndexResponse.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 parsedIndexResponseBytes = toXContent(parsedIndexResponse, xContentType);
try (XContentParser parser = createParser(xContentType.xContent(), parsedIndexResponseBytes)) {
assertIndexResponse(indexResponse, parser.map());
}
}

private static void assertIndexResponse(IndexResponse expected, Map<String, Object> 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<String, Object> actualShards = (Map<String, Object>) 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<Map<String, Object>> actualFailures = (List<Map<String, Object>>) 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<String, Object> 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<String, Object> actualClause = (Map<String, Object>) 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<String, Object> actualHeaders = (Map<String, Object>) actualClause.get("header");

// When a IndexResponse 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<String, Object>) actualClause.get("caused_by");
cause = cause.getCause();
}
}
}
}

private static IndexResponse randomIndexResponse() {
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();

IndexResponse indexResponse = new IndexResponse(shardId, type, id, seqNo, version, created);
indexResponse.setForcedRefresh(randomBoolean());
indexResponse.setShardInfo(RandomObjects.randomShardInfo(random(), randomBoolean()));
return indexResponse;
}

}
Loading