Skip to content

Commit 6bbbd2e

Browse files
committed
Addressing review comments
1 parent 5ed2c46 commit 6bbbd2e

File tree

6 files changed

+60
-55
lines changed

6 files changed

+60
-55
lines changed

core/src/main/java/org/elasticsearch/action/search/SearchResponse.java

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
import static org.elasticsearch.action.search.ShardSearchFailure.readShardSearchFailure;
4848
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
4949
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownField;
50+
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownToken;
5051
import static org.elasticsearch.search.internal.InternalSearchHits.readSearchHits;
5152

5253
/**
@@ -225,7 +226,7 @@ public static SearchResponse fromXContent(XContentParser parser) throws IOExcept
225226
InternalSearchHits hits = null;
226227
boolean timedOut = false;
227228
Boolean terminatedEarly = null;
228-
int tookInMillis = 0;
229+
long tookInMillis = 0;
229230
int successfulShards = 0;
230231
int totalShards = 0;
231232
String scrollId = null;
@@ -237,13 +238,12 @@ public static SearchResponse fromXContent(XContentParser parser) throws IOExcept
237238
if (Fields._SCROLL_ID.equals(currentFieldName)) {
238239
scrollId = parser.text();
239240
} else if (Fields.TOOK.equals(currentFieldName)) {
240-
tookInMillis = parser.intValue();
241+
tookInMillis = parser.longValue();
241242
} else if (Fields.TIMED_OUT.equals(currentFieldName)) {
242243
timedOut = parser.booleanValue();
243244
} else if (Fields.TERMINATED_EARLY.equals(currentFieldName)) {
244245
terminatedEarly = parser.booleanValue();
245-
}
246-
else {
246+
} else {
247247
throwUnknownField(currentFieldName, parser.getTokenLocation());
248248
}
249249
} else if (token == XContentParser.Token.START_OBJECT) {
@@ -258,37 +258,39 @@ public static SearchResponse fromXContent(XContentParser parser) throws IOExcept
258258
} else if (SearchProfileShardResults.PROFILE_NAME.equals(currentFieldName)) {
259259
// TODO parse "profile" section
260260
parser.skipChildren();
261-
} else if (RestActions.ShardsFields._SHARDS.equals(currentFieldName)) {
261+
} else if (RestActions._SHARDS_FIELD.equals(currentFieldName)) {
262262
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
263263
if (token == XContentParser.Token.FIELD_NAME) {
264264
currentFieldName = parser.currentName();
265265
} else if (token.isValue()) {
266-
if (RestActions.ShardsFields.FAILED.equals(currentFieldName)) {
266+
if (RestActions.FAILED_FIELD.equals(currentFieldName)) {
267267
parser.intValue(); // we don't need it but need to consume it
268-
} else if (RestActions.ShardsFields.SUCCESSFUL.equals(currentFieldName)) {
268+
} else if (RestActions.SUCCESSFUL_FIELD.equals(currentFieldName)) {
269269
successfulShards = parser.intValue();
270-
} else if (RestActions.ShardsFields.TOTAL.equals(currentFieldName)) {
270+
} else if (RestActions.TOTAL_FIELD.equals(currentFieldName)) {
271271
totalShards = parser.intValue();
272272
} else {
273273
throwUnknownField(currentFieldName, parser.getTokenLocation());
274274
}
275275
} else if (token == XContentParser.Token.START_ARRAY) {
276-
if (RestActions.ShardsFields.FAILURES.equals(currentFieldName)) {
276+
if (RestActions.FAILURES_FIELD.equals(currentFieldName)) {
277277
while((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
278278
failures.add(ShardSearchFailure.fromXContent(parser));
279279
}
280280
} else {
281281
throwUnknownField(currentFieldName, parser.getTokenLocation());
282282
}
283+
} else {
284+
throwUnknownToken(token, parser.getTokenLocation());
283285
}
284286
}
285287
} else {
286288
throwUnknownField(currentFieldName, parser.getTokenLocation());
287289
}
288290
}
289291
}
290-
return new SearchResponse(new InternalSearchResponse(hits, null, null, null, timedOut, terminatedEarly),
291-
scrollId, totalShards, successfulShards, tookInMillis, failures.toArray(new ShardSearchFailure[0]));
292+
return new SearchResponse(new InternalSearchResponse(hits, null, null, null, timedOut, terminatedEarly), scrollId, totalShards,
293+
successfulShards, tookInMillis, failures.toArray(new ShardSearchFailure[failures.size()]));
292294
}
293295

294296
@Override
@@ -375,13 +377,13 @@ public Map<String, ProfileShardResult> profile() {
375377
return profileResults.getShardResults();
376378
}
377379

378-
public static InternalSearchResponse readInternalSearchResponse(StreamInput in) throws IOException {
380+
static InternalSearchResponse readInternalSearchResponse(StreamInput in) throws IOException {
379381
InternalSearchResponse response = new InternalSearchResponse();
380382
response.readFrom(in);
381383
return response;
382384
}
383385

384-
public void readFrom(StreamInput in) throws IOException {
386+
void readFrom(StreamInput in) throws IOException {
385387
hits = readSearchHits(in);
386388
if (in.readBoolean()) {
387389
aggregations = InternalAggregations.readAggregations(in);
@@ -394,7 +396,7 @@ public void readFrom(StreamInput in) throws IOException {
394396
profileResults = in.readOptionalWriteable(SearchProfileShardResults::new);
395397
}
396398

397-
public void writeTo(StreamOutput out) throws IOException {
399+
void writeTo(StreamOutput out) throws IOException {
398400
hits.writeTo(out);
399401
if (aggregations == null) {
400402
out.writeBoolean(false);

core/src/main/java/org/elasticsearch/action/search/ShardSearchFailure.java

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
import org.elasticsearch.action.ShardOperationFailedException;
2525
import org.elasticsearch.cluster.metadata.IndexMetaData;
2626
import org.elasticsearch.common.Nullable;
27-
import org.elasticsearch.common.ParsingException;
2827
import org.elasticsearch.common.io.stream.StreamInput;
2928
import org.elasticsearch.common.io.stream.StreamOutput;
3029
import org.elasticsearch.common.xcontent.XContentBuilder;
@@ -39,6 +38,7 @@
3938

4039
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
4140
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownField;
41+
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownToken;
4242

4343
/**
4444
* Represents a failure to search on a specific shard.
@@ -208,8 +208,7 @@ public static ShardSearchFailure fromXContent(XContentParser parser) throws IOEx
208208
throwUnknownField(currentFieldName, parser.getTokenLocation());
209209
}
210210
} else {
211-
throw new ParsingException(parser.getTokenLocation(),
212-
"Failed to parse object: unsupported token found [" + token + "]");
211+
throwUnknownToken(token, parser.getTokenLocation());
213212
}
214213
}
215214
return new ShardSearchFailure(exception,

core/src/main/java/org/elasticsearch/rest/action/RestActions.java

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -69,23 +69,21 @@ public static void buildBroadcastShardsHeader(XContentBuilder builder, Params pa
6969
response.getShardFailures());
7070
}
7171

72-
public static final class ShardsFields {
73-
public static final String _SHARDS = "_shards";
74-
public static final String TOTAL = "total";
75-
public static final String SUCCESSFUL = "successful";
76-
public static final String FAILED = "failed";
77-
public static final String FAILURES = "failures";
78-
}
72+
public static final String _SHARDS_FIELD = "_shards";
73+
public static final String TOTAL_FIELD = "total";
74+
public static final String SUCCESSFUL_FIELD = "successful";
75+
public static final String FAILED_FIELD = "failed";
76+
public static final String FAILURES_FIELD = "failures";
7977

8078
public static void buildBroadcastShardsHeader(XContentBuilder builder, Params params,
8179
int total, int successful, int failed,
8280
ShardOperationFailedException[] shardFailures) throws IOException {
83-
builder.startObject(ShardsFields._SHARDS);
84-
builder.field(ShardsFields.TOTAL, total);
85-
builder.field(ShardsFields.SUCCESSFUL, successful);
86-
builder.field(ShardsFields.FAILED, failed);
81+
builder.startObject(_SHARDS_FIELD);
82+
builder.field(TOTAL_FIELD, total);
83+
builder.field(SUCCESSFUL_FIELD, successful);
84+
builder.field(FAILED_FIELD, failed);
8785
if (shardFailures != null && shardFailures.length > 0) {
88-
builder.startArray(ShardsFields.FAILURES);
86+
builder.startArray(FAILURES_FIELD);
8987
final boolean group = params.paramAsBoolean("group_shard_failures", true); // we group by default
9088
for (ShardOperationFailedException shardFailure : group ? ExceptionsHelper.groupBy(shardFailures) : shardFailures) {
9189
builder.startObject();

core/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121

2222
import com.carrotsearch.hppc.cursors.ObjectCursor;
2323
import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
24-
2524
import org.apache.logging.log4j.message.ParameterizedMessage;
2625
import org.apache.logging.log4j.util.Supplier;
2726
import org.apache.lucene.util.CollectionUtil;

core/src/test/java/org/elasticsearch/action/search/SearchResponseTests.java

Lines changed: 30 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@
2020
package org.elasticsearch.action.search;
2121

2222
import org.elasticsearch.action.search.SearchResponse.InternalSearchResponse;
23-
import org.elasticsearch.common.bytes.BytesReference;
23+
import org.elasticsearch.common.Strings;
2424
import org.elasticsearch.common.text.Text;
2525
import org.elasticsearch.common.xcontent.ToXContent;
2626
import org.elasticsearch.common.xcontent.XContentBuilder;
@@ -44,14 +44,14 @@
4444

4545
public class SearchResponseTests extends ESTestCase {
4646

47-
public static SearchResponse createTestItem() {
47+
public static SearchResponse createTestItem(boolean withFailures) {
4848
InternalSearchHits hits = InternalSearchHitsTests.createTestItem();
4949
boolean timedOut = randomBoolean();
5050
Boolean terminatedEarly = randomBoolean() ? null : randomBoolean();
51-
int tookInMillis = randomIntBetween(0, 1000);
52-
int successfulShards = tookInMillis;
51+
long tookInMillis = randomNonNegativeLong();
52+
int successfulShards = randomInt();
5353
int totalShards = randomInt();
54-
int numFailures = randomIntBetween(0, 3);
54+
int numFailures = withFailures ? randomIntBetween(1, 5) : 0;
5555
ShardSearchFailure[] failures = new ShardSearchFailure[numFailures];
5656
for (int i = 0; i < numFailures; i++) {
5757
failures[i] = ShardSearchFailureTests.createTestItem();
@@ -65,7 +65,8 @@ public static SearchResponse createTestItem() {
6565
}
6666

6767
public void testFromXContent() throws IOException {
68-
SearchResponse response = createTestItem();
68+
// the "_shard/total/failures" section makes if impossible to directly compare xContent, so we omit it here
69+
SearchResponse response = createTestItem(false);
6970
XContentType xcontentType = randomFrom(XContentType.values());
7071
XContentBuilder builder = XContentFactory.contentBuilder(xcontentType);
7172
builder = response.toXContent(builder, ToXContent.EMPTY_PARAMS);
@@ -74,33 +75,39 @@ public void testFromXContent() throws IOException {
7475
XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
7576
SearchResponse parsed = SearchResponse.fromXContent(parser);
7677

77-
// the "_shard/total/failures" section makes if impossible to directly compare xContent, because
78-
// the failures in the parsed SearchResponse are wrapped in an extra ElasticSearchException on the client side.
79-
// Because of this we compare the "top level" fields for equality and the subsections xContent equivalence independently
80-
assertEquals(response.getScrollId(), parsed.getScrollId());
81-
assertEquals(response.getTookInMillis(), parsed.getTookInMillis());
82-
assertEquals(response.getTook(), parsed.getTook());
83-
assertEquals(response.isTimedOut(), parsed.isTimedOut());
84-
assertEquals(response.isTerminatedEarly(), parsed.isTerminatedEarly());
85-
assertEquals(response.getFailedShards(), parsed.getFailedShards());
86-
assertEquals(response.getSuccessfulShards(), parsed.getSuccessfulShards());
87-
assertEquals(response.getTotalShards(), parsed.getTotalShards());
78+
assertToXContentEquivalent(builder.bytes(), toXContent(parsed, xcontentType), xcontentType);
79+
assertEquals(XContentParser.Token.END_OBJECT, parser.currentToken());
80+
assertNull(parser.nextToken());
81+
}
82+
83+
/**
84+
* The "_shard/total/failures" section makes if impossible to directly compare xContent, because
85+
* the failures in the parsed SearchResponse are wrapped in an extra ElasticSearchException on the client side.
86+
* Because of this, in this special test case we compare the "top level" fields for equality
87+
* and the subsections xContent equivalence independently
88+
*/
89+
public void testFromXContentWithFailures() throws IOException {
90+
SearchResponse response = createTestItem(true);
91+
XContentType xcontentType = randomFrom(XContentType.values());
92+
XContentBuilder builder = XContentFactory.contentBuilder(xcontentType);
93+
builder = response.toXContent(builder, ToXContent.EMPTY_PARAMS);
8894

89-
// compare the "hits" section by comparing xContent equivalence
90-
assertToXContentEquivalent(toXContent(response.getHits(), xcontentType), toXContent(parsed.getHits(), xcontentType),
91-
xcontentType);
95+
XContentParser parser = createParser(builder);
96+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
97+
SearchResponse parsed = SearchResponse.fromXContent(parser);
98+
// check that we at least get the same number of shardFailures
99+
assertEquals(response.getShardFailures().length, parsed.getShardFailures().length);
92100
assertEquals(XContentParser.Token.END_OBJECT, parser.currentToken());
93101
assertNull(parser.nextToken());
94102
}
95103

96-
public void testToXContent() throws IOException {
104+
public void testToXContent() {
97105
InternalSearchHit hit = new InternalSearchHit(1, "id1", new Text("type"), Collections.emptyMap());
98106
hit.score(2.0f);
99107
InternalSearchHit[] hits = new InternalSearchHit[] { hit };
100108
SearchResponse response = new SearchResponse(
101109
new InternalSearchResponse(new InternalSearchHits(hits, 100, 1.5f), null, null, null, false, null), null, 0, 0, 0,
102110
new ShardSearchFailure[0]);
103-
BytesReference xContent = toXContent(response, XContentType.JSON);
104111
assertEquals(
105112
"{\"took\":0,"
106113
+ "\"timed_out\":false,"
@@ -114,7 +121,7 @@ public void testToXContent() throws IOException {
114121
+ "\"max_score\":1.5,"
115122
+ "\"hits\":[{\"_type\":\"type\",\"_id\":\"id1\",\"_score\":2.0}]"
116123
+ "}"
117-
+ "}", xContent.utf8ToString());
124+
+ "}", Strings.toString(response));
118125
}
119126

120127
}

core/src/test/java/org/elasticsearch/action/search/ShardSearchFailureTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ public static ShardSearchFailure createTestItem() {
5050

5151
public void testFromXContent() throws IOException {
5252
ShardSearchFailure response = createTestItem();
53-
XContentType xcontentType = XContentType.JSON; //randomFrom(XContentType.values());
53+
XContentType xcontentType = randomFrom(XContentType.values());
5454
XContentBuilder builder = XContentFactory.contentBuilder(xcontentType);
5555
builder.startObject();
5656
builder = response.toXContent(builder, ToXContent.EMPTY_PARAMS);
@@ -65,7 +65,7 @@ public void testFromXContent() throws IOException {
6565
assertEquals(response.shardId(), parsed.shardId());
6666

6767
// we cannot compare the cause, because it will be wrapped in an outer ElasticSearchException
68-
// best effort: try to check that the original message appears somewhere in the renderes xContent
68+
// best effort: try to check that the original message appears somewhere in the rendered xContent
6969
String originalMsg = response.getCause().getMessage();
7070
assertTrue(parsed.getCause().getMessage().contains(originalMsg));
7171

0 commit comments

Comments
 (0)