From a6df6883792bc6fcec27c32667135e41d3d568de Mon Sep 17 00:00:00 2001 From: Tim Brooks Date: Tue, 8 Aug 2023 17:25:51 -0600 Subject: [PATCH] Place the errors field first in bulk response The bulk response is a map of fields that is inherently unordered. However, we have some control over the order in which fields are serialized. This commit places the errors field first to allow clients who want to incrementally parse the response until the errors field is encountered potentially encounter the field earlier. There is also a test added to inform us if a future version of jackson were to change the order of serialization. --- .../action/bulk/BulkResponse.java | 2 +- .../action/bulk/BulkResponseTests.java | 73 ++++++++++++++++--- 2 files changed, 63 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/bulk/BulkResponse.java b/server/src/main/java/org/elasticsearch/action/bulk/BulkResponse.java index 5c94344b93606..0725e6c684abe 100644 --- a/server/src/main/java/org/elasticsearch/action/bulk/BulkResponse.java +++ b/server/src/main/java/org/elasticsearch/action/bulk/BulkResponse.java @@ -142,11 +142,11 @@ public RestStatus status() { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); + builder.field(ERRORS, hasFailures()); builder.field(TOOK, tookInMillis); if (ingestTookInMillis != BulkResponse.NO_INGEST_TOOK) { builder.field(INGEST_TOOK, ingestTookInMillis); } - builder.field(ERRORS, hasFailures()); builder.startArray(ITEMS); for (BulkItemResponse item : this) { item.toXContent(builder, params); diff --git a/server/src/test/java/org/elasticsearch/action/bulk/BulkResponseTests.java b/server/src/test/java/org/elasticsearch/action/bulk/BulkResponseTests.java index 8feedcc415aa4..5a1c7f1572e23 100644 --- a/server/src/test/java/org/elasticsearch/action/bulk/BulkResponseTests.java +++ b/server/src/test/java/org/elasticsearch/action/bulk/BulkResponseTests.java @@ -29,6 +29,7 @@ import static org.elasticsearch.action.bulk.BulkResponse.NO_INGEST_TOOK; import static org.elasticsearch.common.xcontent.XContentHelper.toXContent; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent; +import static org.hamcrest.Matchers.equalTo; public class BulkResponseTests extends ESTestCase { @@ -47,17 +48,7 @@ public void testToAndFromXContent() throws IOException { DocWriteRequest.OpType opType = randomFrom(DocWriteRequest.OpType.values()); if (frequently()) { - Tuple randomDocWriteResponses = null; - if (opType == DocWriteRequest.OpType.INDEX || opType == DocWriteRequest.OpType.CREATE) { - randomDocWriteResponses = IndexResponseTests.randomIndexResponse(); - } else if (opType == DocWriteRequest.OpType.DELETE) { - randomDocWriteResponses = DeleteResponseTests.randomDeleteResponse(); - } else if (opType == DocWriteRequest.OpType.UPDATE) { - randomDocWriteResponses = UpdateResponseTests.randomUpdateResponse(xContentType); - } else { - fail("Test does not support opType [" + opType + "]"); - } - + Tuple randomDocWriteResponses = success(opType, xContentType); bulkItems[i] = BulkItemResponse.success(i, opType, randomDocWriteResponses.v1()); expectedBulkItems[i] = BulkItemResponse.success(i, opType, randomDocWriteResponses.v2()); } else { @@ -97,4 +88,64 @@ public void testToAndFromXContent() throws IOException { BytesReference expectedFinalBytes = toXContent(parsedBulkResponse, xContentType, humanReadable); assertToXContentEquivalent(expectedFinalBytes, finalBytes, xContentType); } + + public void testToXContentPlacesErrorsFirst() throws IOException { + XContentType xContentType = randomFrom(XContentType.values()); + boolean humanReadable = randomBoolean(); + + boolean errors = false; + long took = randomFrom(randomNonNegativeLong(), -1L); + long ingestTook = randomFrom(randomNonNegativeLong(), NO_INGEST_TOOK); + int nbBulkItems = randomIntBetween(1, 10); + + BulkItemResponse[] bulkItems = new BulkItemResponse[nbBulkItems]; + + for (int i = 0; i < nbBulkItems; i++) { + DocWriteRequest.OpType opType = randomFrom(DocWriteRequest.OpType.values()); + if (frequently()) { + Tuple randomDocWriteResponses = success(opType, xContentType); + + BulkItemResponse success = BulkItemResponse.success(i, opType, randomDocWriteResponses.v1()); + bulkItems[i] = success; + } else { + errors = true; + String index = randomAlphaOfLength(5); + String id = randomAlphaOfLength(5); + + Tuple failures = randomExceptions(); + + Exception bulkItemCause = (Exception) failures.v1(); + bulkItems[i] = BulkItemResponse.failure(i, opType, new BulkItemResponse.Failure(index, id, bulkItemCause)); + } + } + + BulkResponse bulkResponse = new BulkResponse(bulkItems, took, ingestTook); + BytesReference originalBytes = toXContent(bulkResponse, xContentType, ToXContent.EMPTY_PARAMS, humanReadable); + + try (XContentParser parser = createParser(xContentType.xContent(), originalBytes)) { + assertThat(parser.nextToken(), equalTo(XContentParser.Token.START_OBJECT)); + XContentParser.Token firstField = parser.nextToken(); + assertThat(firstField, equalTo(XContentParser.Token.FIELD_NAME)); + assertThat(parser.currentName(), equalTo("errors")); + assertThat(parser.nextToken(), equalTo(XContentParser.Token.VALUE_BOOLEAN)); + assertThat(parser.booleanValue(), equalTo(errors)); + } + } + + private static Tuple success( + DocWriteRequest.OpType opType, + XContentType xContentType + ) { + Tuple randomDocWriteResponses = null; + if (opType == DocWriteRequest.OpType.INDEX || opType == DocWriteRequest.OpType.CREATE) { + randomDocWriteResponses = IndexResponseTests.randomIndexResponse(); + } else if (opType == DocWriteRequest.OpType.DELETE) { + randomDocWriteResponses = DeleteResponseTests.randomDeleteResponse(); + } else if (opType == DocWriteRequest.OpType.UPDATE) { + randomDocWriteResponses = UpdateResponseTests.randomUpdateResponse(xContentType); + } else { + fail("Test does not support opType [" + opType + "]"); + } + return randomDocWriteResponses; + } }