honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

[Test] Extend parsing checks for DocWriteResponses (#25257) 

This commit changes the parsing logic of DocWriteResponse, ReplicationResponse
and GetResult so that it skips any unknown additional fields (for forward compatibility 
reasons). This affects the IndexResponse, UpdateResponse,DeleteResponse and 
GetResponse objects.
This commit is contained in:
Tanguy Leroux 2017-06-19 13:19:09 +02:00 committed by GitHub
parent bcaa413b0b
commit e4f4886d40
10 changed files with 198 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
client/rest-high-level/src/main/java/org/elasticsearch/client/RestHighLevelClient.java
View File

@ -395,6 +395,10 @@ public class RestHighLevelClient {
try {
return responseConverter.apply(e.getResponse());
} catch (Exception innerException) {
//the exception is ignored as we now try to parse the response as an error.
//this covers cases like get where 404 can either be a valid document not found response,
//or an error for which parsing is completely different. We try to consider the 404 response as a valid one
//first. If parsing of the response breaks, we fall back to parsing it as an error.
throw parseResponseException(e);
}
}

10
core/src/main/java/org/elasticsearch/action/DocWriteResponse.java
View File

@ -43,8 +43,6 @@ import java.net.URLEncoder;
import java.util.Locale;
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownField;
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownToken;
/**
* A base class for the response of a write operation that involves a single doc
@ -351,17 +349,15 @@ public abstract class DocWriteResponse extends ReplicationResponse implements Wr
context.setSeqNo(parser.longValue());
} else if (_PRIMARY_TERM.equals(currentFieldName)) {
context.setPrimaryTerm(parser.longValue());
} else {
throwUnknownField(currentFieldName, parser.getTokenLocation());
}
} else if (token == XContentParser.Token.START_OBJECT) {
if (_SHARDS.equals(currentFieldName)) {
context.setShardInfo(ShardInfo.fromXContent(parser));
} else {
throwUnknownField(currentFieldName, parser.getTokenLocation());
parser.skipChildren(); // skip potential inner objects for forward compatibility
}
} else {
throwUnknownToken(token, parser.getTokenLocation());
} else if (token == XContentParser.Token.START_ARRAY) {
parser.skipChildren(); // skip potential inner arrays for forward compatibility
}
}

26
core/src/main/java/org/elasticsearch/action/get/GetResponse.java
View File

@ -21,6 +21,7 @@ package org.elasticsearch.action.get;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.StreamInput;
@ -33,6 +34,7 @@ import org.elasticsearch.index.get.GetResult;
import java.io.IOException;
import java.util.Iterator;
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
@ -158,8 +160,32 @@ public class GetResponse extends ActionResponse implements Iterable<GetField>, T
return getResult.toXContent(builder, params);
}
/**
* This method can be used to parse a {@link GetResponse} object when it has been printed out
* as a xcontent using the {@link #toXContent(XContentBuilder, Params)} method.
* <p>
* For forward compatibility reason this method might not fail if it tries to parse a field it
* doesn't know. But before returning the result it will check that enough information were
* parsed to return a valid {@link GetResponse} instance and throws a {@link ParsingException}
* otherwise. This is the case when we get a 404 back, which can be parsed as a normal
* {@link GetResponse} with found set to false, or as an elasticsearch exception. The caller
* of this method needs a way to figure out whether we got back a valid get response, which
* can be done by catching ParsingException.
*
* @param parser {@link XContentParser} to parse the response from
* @return a {@link GetResponse}
* @throws IOException is an I/O exception occurs during the parsing
*/
public static GetResponse fromXContent(XContentParser parser) throws IOException {
GetResult getResult = GetResult.fromXContent(parser);
// At this stage we ensure that we parsed enough information to return
// a valid GetResponse instance. If it's not the case, we throw an
// exception so that callers know it and can handle it correctly.
if (getResult.getIndex() == null && getResult.getType() == null && getResult.getId() == null) {
throw new ParsingException(parser.getTokenLocation(),
String.format(Locale.ROOT, "Missing required fields [%s,%s,%s]", GetResult._INDEX, GetResult._TYPE, GetResult._ID));
}
return new GetResponse(getResult);
}

15
core/src/main/java/org/elasticsearch/action/support/replication/ReplicationResponse.java
View File

@ -40,7 +40,6 @@ import java.util.Arrays;
import java.util.List;
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownField;
/**
* Base class for write action responses.
@ -187,8 +186,8 @@ public class ReplicationResponse extends ActionResponse {
total = parser.intValue();
} else if (SUCCESSFUL.equals(currentFieldName)) {
successful = parser.intValue();
} else if (FAILED.equals(currentFieldName) == false) {
throwUnknownField(currentFieldName, parser.getTokenLocation());
} else {
parser.skipChildren();
}
} else if (token == XContentParser.Token.START_ARRAY) {
if (FAILURES.equals(currentFieldName)) {
@ -197,8 +196,10 @@ public class ReplicationResponse extends ActionResponse {
failuresList.add(Failure.fromXContent(parser));
}
} else {
throwUnknownField(currentFieldName, parser.getTokenLocation());
parser.skipChildren(); // skip potential inner arrays for forward compatibility
}
} else if (token == XContentParser.Token.START_OBJECT) {
parser.skipChildren(); // skip potential inner arrays for forward compatibility
}
}
Failure[] failures = EMPTY;
@ -365,15 +366,15 @@ public class ReplicationResponse extends ActionResponse {
status = RestStatus.valueOf(parser.text());
} else if (PRIMARY.equals(currentFieldName)) {
primary = parser.booleanValue();
} else {
throwUnknownField(currentFieldName, parser.getTokenLocation());
}
} else if (token == XContentParser.Token.START_OBJECT) {
if (REASON.equals(currentFieldName)) {
reason = ElasticsearchException.fromXContent(parser);
} else {
throwUnknownField(currentFieldName, parser.getTokenLocation());
parser.skipChildren(); // skip potential inner objects for forward compatibility
}
} else if (token == XContentParser.Token.START_ARRAY) {
parser.skipChildren(); // skip potential inner arrays for forward compatibility
}
}
return new Failure(new ShardId(shardIndex, IndexMetaData.INDEX_UUID_NA_VALUE, shardId), nodeId, reason, status, primary);

14
core/src/main/java/org/elasticsearch/index/get/GetResult.java
View File

@ -20,6 +20,7 @@
package org.elasticsearch.index.get;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.compress.CompressorFactory;
import org.elasticsearch.common.io.stream.StreamInput;
@ -43,14 +44,13 @@ import java.util.Objects;
import static java.util.Collections.emptyMap;
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownField;
import static org.elasticsearch.index.get.GetField.readGetField;
public class GetResult implements Streamable, Iterable<GetField>, ToXContentObject {
private static final String _INDEX = "_index";
private static final String _TYPE = "_type";
private static final String _ID = "_id";
public static final String _INDEX = "_index";
public static final String _TYPE = "_type";
public static final String _ID = "_id";
private static final String _VERSION = "_version";
private static final String FOUND = "found";
private static final String FIELDS = "fields";
@ -273,7 +273,7 @@ public class GetResult implements Streamable, Iterable<GetField>, ToXContentObje
String currentFieldName = parser.currentName();
String index = null, type = null, id = null;
long version = -1;
boolean found = false;
Boolean found = null;
BytesReference source = null;
Map<String, GetField> fields = new HashMap<>();
while((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
@ -307,8 +307,10 @@ public class GetResult implements Streamable, Iterable<GetField>, ToXContentObje
fields.put(getField.getName(), getField);
}
} else {
throwUnknownField(currentFieldName, parser.getTokenLocation());
parser.skipChildren(); // skip potential inner objects for forward compatibility
}
} else if (token == XContentParser.Token.START_ARRAY) {
parser.skipChildren(); // skip potential inner arrays for forward compatibility
}
}
return new GetResult(index, type, id, version, found, source, fields);

10
core/src/test/java/org/elasticsearch/action/bulk/BulkItemResponseTests.java
View File

@ -26,6 +26,7 @@ import org.elasticsearch.action.DocWriteResponse;
import org.elasticsearch.action.bulk.BulkItemResponse.Failure;
import org.elasticsearch.action.delete.DeleteResponseTests;
import org.elasticsearch.action.index.IndexResponseTests;
import org.elasticsearch.action.update.UpdateResponse;
import org.elasticsearch.action.update.UpdateResponseTests;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.collect.Tuple;
@ -137,11 +138,12 @@ public class BulkItemResponseTests extends ESTestCase {
assertDeepEquals((ElasticsearchException) expectedFailure.getCause(), (ElasticsearchException) actualFailure.getCause());
} else {
DocWriteResponse expectedDocResponse = expected.getResponse();
DocWriteResponse actualDocResponse = expected.getResponse();
IndexResponseTests.assertDocWriteResponse(expectedDocResponse, actualDocResponse);
if (expected.getOpType() == DocWriteRequest.OpType.UPDATE) {
UpdateResponseTests.assertUpdateResponse(expected.getResponse(), actual.getResponse());
} else {
// assertDocWriteResponse check the result for INDEX/CREATE and DELETE operations
IndexResponseTests.assertDocWriteResponse(expected.getResponse(), actual.getResponse());
assertEquals(((UpdateResponse) expectedDocResponse).getGetResult(), ((UpdateResponse)actualDocResponse).getGetResult());
}
}
}

30
core/src/test/java/org/elasticsearch/action/delete/DeleteResponseTests.java
View File

@ -32,9 +32,11 @@ import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.RandomObjects;
import java.io.IOException;
import java.util.function.Predicate;
import static org.elasticsearch.action.index.IndexResponseTests.assertDocWriteResponse;
import static org.elasticsearch.cluster.metadata.IndexMetaData.INDEX_UUID_NA_VALUE;
import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;
public class DeleteResponseTests extends ESTestCase {
@ -56,16 +58,40 @@ public class DeleteResponseTests extends ESTestCase {
}
public void testToAndFromXContent() throws IOException {
doFromXContentTestWithRandomFields(false);
}
/**
* This test adds random fields and objects to the xContent rendered out to
* ensure we can parse it back to be forward compatible with additions to
* the xContent
*/
public void testFromXContentWithRandomFields() throws IOException {
doFromXContentTestWithRandomFields(true);
}
private void doFromXContentTestWithRandomFields(boolean addRandomFields) throws IOException {
final Tuple<DeleteResponse, DeleteResponse> tuple = randomDeleteResponse();
DeleteResponse deleteResponse = tuple.v1();
DeleteResponse expectedDeleteResponse = tuple.v2();
boolean humanReadable = randomBoolean();
final XContentType xContentType = randomFrom(XContentType.values());
BytesReference deleteResponseBytes = toShuffledXContent(deleteResponse, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);
BytesReference originalBytes = toShuffledXContent(deleteResponse, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);
BytesReference mutated;
if (addRandomFields) {
// The ShardInfo.Failure's exception is rendered out in a "reason" object. We shouldn't add anything random there
// because exception rendering and parsing are very permissive: any extra object or field would be rendered as
// a exception custom metadata and be parsed back as a custom header, making it impossible to compare the results
// in this test.
Predicate<String> excludeFilter = path -> path.contains("reason");
mutated = insertRandomFields(xContentType, originalBytes, excludeFilter, random());
} else {
mutated = originalBytes;
}
DeleteResponse parsedDeleteResponse;
try (XContentParser parser = createParser(xContentType.xContent(), deleteResponseBytes)) {
try (XContentParser parser = createParser(xContentType.xContent(), mutated)) {
parsedDeleteResponse = DeleteResponse.fromXContent(parser);
assertNull(parser.nextToken());
}

44
core/src/test/java/org/elasticsearch/action/get/GetResponseTests.java
View File

@ -19,6 +19,7 @@
package org.elasticsearch.action.get;
import org.elasticsearch.common.ParsingException;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
@ -30,27 +31,53 @@ import org.elasticsearch.index.get.GetField;
import org.elasticsearch.index.get.GetResult;
import org.elasticsearch.test.ESTestCase;
import java.io.IOException;
import java.util.Collections;
import java.util.function.Predicate;
import static org.elasticsearch.common.xcontent.XContentHelper.toXContent;
import static org.elasticsearch.index.get.GetResultTests.copyGetResult;
import static org.elasticsearch.index.get.GetResultTests.mutateGetResult;
import static org.elasticsearch.index.get.GetResultTests.randomGetResult;
import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode;
import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent;
public class GetResponseTests extends ESTestCase {
public void testToAndFromXContent() throws Exception {
doFromXContentTestWithRandomFields(false);
}
/**
* This test adds random fields and objects to the xContent rendered out to
* ensure we can parse it back to be forward compatible with additions to
* the xContent
*/
public void testFromXContentWithRandomFields() throws IOException {
doFromXContentTestWithRandomFields(true);
}
private void doFromXContentTestWithRandomFields(boolean addRandomFields) throws IOException {
XContentType xContentType = randomFrom(XContentType.values());
Tuple<GetResult, GetResult> tuple = randomGetResult(xContentType);
GetResponse getResponse = new GetResponse(tuple.v1());
GetResponse expectedGetResponse = new GetResponse(tuple.v2());
boolean humanReadable = randomBoolean();
BytesReference originalBytes = toShuffledXContent(getResponse, xContentType, ToXContent.EMPTY_PARAMS, humanReadable, "_source");
//test that we can parse what we print out
BytesReference mutated;
if (addRandomFields) {
// "_source" and "fields" just consists of key/value pairs, we shouldn't add anything random there. It is already
// randomized in the randomGetResult() method anyway. Also, we cannot add anything in the root object since this is
// where GetResult's metadata fields are rendered out while // other fields are rendered out in a "fields" object.
Predicate<String> excludeFilter = (s) -> s.isEmpty() || s.contains("fields") || s.contains("_source");
mutated = insertRandomFields(xContentType, originalBytes, excludeFilter, random());
} else {
mutated = originalBytes;
}
GetResponse parsedGetResponse;
try (XContentParser parser = createParser(xContentType.xContent(), originalBytes)) {
try (XContentParser parser = createParser(xContentType.xContent(), mutated)) {
parsedGetResponse = GetResponse.fromXContent(parser);
assertNull(parser.nextToken());
}
@ -90,6 +117,19 @@ public class GetResponseTests extends ESTestCase {
checkEqualsAndHashCode(new GetResponse(randomGetResult(XContentType.JSON).v1()), GetResponseTests::copyGetResponse,
GetResponseTests::mutateGetResponse);
}
public void testFromXContentThrowsParsingException() throws IOException {
GetResponse getResponse = new GetResponse(new GetResult(null, null, null, randomIntBetween(1, 5), randomBoolean(), null, null));
XContentType xContentType = randomFrom(XContentType.values());
BytesReference originalBytes = toShuffledXContent(getResponse, xContentType, ToXContent.EMPTY_PARAMS, randomBoolean());
try (XContentParser parser = createParser(xContentType.xContent(), originalBytes)) {
ParsingException exception = expectThrows(ParsingException.class, () -> GetResponse.fromXContent(parser));
assertEquals("Missing required fields [_index,_type,_id]", exception.getMessage());
}
}
private static GetResponse copyGetResponse(GetResponse getResponse) {
return new GetResponse(copyGetResult(getResponse.getResult));
}

30
core/src/test/java/org/elasticsearch/action/index/IndexResponseTests.java
View File

@ -33,9 +33,11 @@ import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.RandomObjects;
import java.io.IOException;
import java.util.function.Predicate;
import static org.elasticsearch.action.support.replication.ReplicationResponseTests.assertShardInfo;
import static org.elasticsearch.cluster.metadata.IndexMetaData.INDEX_UUID_NA_VALUE;
import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;
public class IndexResponseTests extends ESTestCase {
@ -57,16 +59,40 @@ public class IndexResponseTests extends ESTestCase {
}
public void testToAndFromXContent() throws IOException {
doFromXContentTestWithRandomFields(false);
}
/**
* This test adds random fields and objects to the xContent rendered out to
* ensure we can parse it back to be forward compatible with additions to
* the xContent
*/
public void testFromXContentWithRandomFields() throws IOException {
doFromXContentTestWithRandomFields(true);
}
private void doFromXContentTestWithRandomFields(boolean addRandomFields) throws IOException {
final Tuple<IndexResponse, IndexResponse> tuple = randomIndexResponse();
IndexResponse indexResponse = tuple.v1();
IndexResponse expectedIndexResponse = tuple.v2();
boolean humanReadable = randomBoolean();
XContentType xContentType = randomFrom(XContentType.values());
BytesReference indexResponseBytes = toShuffledXContent(indexResponse, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);
BytesReference originalBytes = toShuffledXContent(indexResponse, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);
BytesReference mutated;
if (addRandomFields) {
// The ShardInfo.Failure's exception is rendered out in a "reason" object. We shouldn't add anything random there
// because exception rendering and parsing are very permissive: any extra object or field would be rendered as
// a exception custom metadata and be parsed back as a custom header, making it impossible to compare the results
// in this test.
Predicate<String> excludeFilter = path -> path.contains("reason");
mutated = insertRandomFields(xContentType, originalBytes, excludeFilter, random());
} else {
mutated = originalBytes;
}
IndexResponse parsedIndexResponse;
try (XContentParser parser = createParser(xContentType.xContent(), indexResponseBytes)) {
try (XContentParser parser = createParser(xContentType.xContent(), mutated)) {
parsedIndexResponse = IndexResponse.fromXContent(parser);
assertNull(parser.nextToken());
}

56
core/src/test/java/org/elasticsearch/action/update/UpdateResponseTests.java
View File

@ -40,11 +40,15 @@ import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Predicate;
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.cluster.metadata.IndexMetaData.INDEX_UUID_NA_VALUE;
import static org.elasticsearch.common.xcontent.XContentHelper.toXContent;
import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent;
public class UpdateResponseTests extends ESTestCase {
@ -81,29 +85,59 @@ public class UpdateResponseTests extends ESTestCase {
}
public void testToAndFromXContent() throws IOException {
final XContentType xContentType = randomFrom(XContentType.values());
doFromXContentTestWithRandomFields(false);
}
/**
* This test adds random fields and objects to the xContent rendered out to
* ensure we can parse it back to be forward compatible with additions to
* the xContent
*/
public void testFromXContentWithRandomFields() throws IOException {
doFromXContentTestWithRandomFields(true);
}
private void doFromXContentTestWithRandomFields(boolean addRandomFields) throws IOException {
final XContentType xContentType = randomFrom(XContentType.JSON);
final Tuple<UpdateResponse, UpdateResponse> tuple = randomUpdateResponse(xContentType);
UpdateResponse updateResponse = tuple.v1();
UpdateResponse expectedUpdateResponse = tuple.v2();
boolean humanReadable = randomBoolean();
BytesReference updateResponseBytes = toShuffledXContent(updateResponse, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);
BytesReference originalBytes = toShuffledXContent(updateResponse, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);
BytesReference mutated;
if (addRandomFields) {
// - The ShardInfo.Failure's exception is rendered out in a "reason" object. We shouldn't add anything random there
// because exception rendering and parsing are very permissive: any extra object or field would be rendered as
// a exception custom metadata and be parsed back as a custom header, making it impossible to compare the results
// in this test.
// - The GetResult's "_source" and "fields" just consists of key/value pairs, we shouldn't add anything random there.
// It is already randomized in the randomGetResult() method anyway. Also, we cannot add anything within the "get"
// object since this is where GetResult's metadata fields are rendered out and they would be parsed back as
// extra metadata fields.
Predicate<String> excludeFilter = path -> path.contains("reason") || path.contains("get");
mutated = insertRandomFields(xContentType, originalBytes, excludeFilter, random());
} else {
mutated = originalBytes;
}
UpdateResponse parsedUpdateResponse;
try (XContentParser parser = createParser(xContentType.xContent(), updateResponseBytes)) {
try (XContentParser parser = createParser(xContentType.xContent(), mutated)) {
parsedUpdateResponse = UpdateResponse.fromXContent(parser);
assertNull(parser.nextToken());
}
// We can't use equals() to compare the original and the parsed delete response
// because the random delete response can contain shard failures with exceptions,
// and those exceptions are not parsed back with the same types.
assertUpdateResponse(expectedUpdateResponse, parsedUpdateResponse);
}
IndexResponseTests.assertDocWriteResponse(expectedUpdateResponse, parsedUpdateResponse);
if (addRandomFields == false) {
assertEquals(expectedUpdateResponse.getGetResult(), parsedUpdateResponse.getGetResult());
}
public static void assertUpdateResponse(UpdateResponse expected, UpdateResponse actual) {
IndexResponseTests.assertDocWriteResponse(expected, actual);
assertEquals(expected.getGetResult(), actual.getGetResult());
// Prints out the parsed UpdateResponse object to verify that it is the same as the expected output.
// If random fields have been inserted, it checks that they have been filtered out and that they do
// not alter the final output of the parsed object.
BytesReference parsedBytes = toXContent(parsedUpdateResponse, xContentType, humanReadable);
BytesReference expectedBytes = toXContent(expectedUpdateResponse, xContentType, humanReadable);
assertToXContentEquivalent(expectedBytes, parsedBytes, xContentType);
}
/**