Skip to content

Commit a0afa91

Browse files
cbueschercbuescher
authored
[Tests] Check QueryProfileShardResult parser robustness for new fields (#25130)
When parsing resonses we should be ignoring any new unknown fields or inner objects in most cases to be forward compatible with changes in core on the client side. This change adds test for this for QueryProfileShardResult and nested substructures and changes the parsing code where necessary to be able to ignore new fields and objects in the xContent.
1 parent 4a8c09c commit a0afa91

File tree

7 files changed

+97
-38
lines changed

7 files changed

+97
-38
lines changed

‎core/src/main/java/org/elasticsearch/search/profile/ProfileResult.java‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737
import java.util.concurrent.TimeUnit;
3838

3939
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
40-
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownField;
4140

4241
/**
4342
* This class is the internal representation of a profiled Query, corresponding
@@ -50,12 +49,12 @@
5049
*/
5150
public final class ProfileResult implements Writeable, ToXContentObject {
5251

53-
private static final ParseField TYPE = new ParseField("type");
54-
private static final ParseField DESCRIPTION = new ParseField("description");
55-
private static final ParseField NODE_TIME = new ParseField("time");
56-
private static final ParseField NODE_TIME_RAW = new ParseField("time_in_nanos");
57-
private static final ParseField CHILDREN = new ParseField("children");
58-
private static final ParseField BREAKDOWN = new ParseField("breakdown");
52+
static final ParseField TYPE = new ParseField("type");
53+
static final ParseField DESCRIPTION = new ParseField("description");
54+
static final ParseField NODE_TIME = new ParseField("time");
55+
static final ParseField NODE_TIME_RAW = new ParseField("time_in_nanos");
56+
static final ParseField CHILDREN = new ParseField("children");
57+
static final ParseField BREAKDOWN = new ParseField("breakdown");
5958

6059
private final String type;
6160
private final String description;
@@ -188,7 +187,7 @@ public static ProfileResult fromXContent(XContentParser parser) throws IOExcepti
188187
// skip, total time is calculate by adding up 'timings' values in ProfileResult ctor
189188
parser.longValue();
190189
} else {
191-
throwUnknownField(currentFieldName, parser.getTokenLocation());
190+
parser.skipChildren();
192191
}
193192
} else if (token == XContentParser.Token.START_OBJECT) {
194193
if (BREAKDOWN.match(currentFieldName)) {
@@ -200,15 +199,15 @@ public static ProfileResult fromXContent(XContentParser parser) throws IOExcepti
200199
timings.put(name, value);
201200
}
202201
} else {
203-
throwUnknownField(currentFieldName, parser.getTokenLocation());
202+
parser.skipChildren();
204203
}
205204
} else if (token == XContentParser.Token.START_ARRAY) {
206205
if (CHILDREN.match(currentFieldName)) {
207206
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
208207
children.add(ProfileResult.fromXContent(parser));
209208
}
210209
} else {
211-
throwUnknownField(currentFieldName, parser.getTokenLocation());
210+
parser.skipChildren();
212211
}
213212
}
214213
}

‎core/src/main/java/org/elasticsearch/search/profile/SearchProfileShardResults.java‎

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,6 @@
3939
import java.util.TreeSet;
4040

4141
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
42-
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureFieldName;
43-
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownField;
44-
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownToken;
4542

4643
/**
4744
* A container class to hold all the profile results across all shards. Internally
@@ -111,12 +108,19 @@ public static SearchProfileShardResults fromXContent(XContentParser parser) thro
111108
XContentParser.Token token = parser.currentToken();
112109
ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser::getTokenLocation);
113110
Map<String, ProfileShardResult> searchProfileResults = new HashMap<>();
114-
ensureFieldName(parser, parser.nextToken(), SHARDS_FIELD);
115-
ensureExpectedToken(XContentParser.Token.START_ARRAY, parser.nextToken(), parser::getTokenLocation);
116-
while((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
117-
parseSearchProfileResultsEntry(parser, searchProfileResults);
111+
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
112+
if (token == XContentParser.Token.START_ARRAY) {
113+
if (SHARDS_FIELD.equals(parser.currentName())) {
114+
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
115+
parseSearchProfileResultsEntry(parser, searchProfileResults);
116+
}
117+
} else {
118+
parser.skipChildren();
119+
}
120+
} else if (token == XContentParser.Token.START_OBJECT) {
121+
parser.skipChildren();
122+
}
118123
}
119-
ensureExpectedToken(XContentParser.Token.END_OBJECT, parser.nextToken(), parser::getTokenLocation);
120124
return new SearchProfileShardResults(searchProfileResults);
121125
}
122126

@@ -135,7 +139,7 @@ private static void parseSearchProfileResultsEntry(XContentParser parser,
135139
if (ID_FIELD.equals(currentFieldName)) {
136140
id = parser.text();
137141
} else {
138-
throwUnknownField(currentFieldName, parser.getTokenLocation());
142+
parser.skipChildren();
139143
}
140144
} else if (token == XContentParser.Token.START_ARRAY) {
141145
if (SEARCHES_FIELD.equals(currentFieldName)) {
@@ -145,10 +149,10 @@ private static void parseSearchProfileResultsEntry(XContentParser parser,
145149
} else if (AggregationProfileShardResult.AGGREGATIONS.equals(currentFieldName)) {
146150
aggProfileShardResult = AggregationProfileShardResult.fromXContent(parser);
147151
} else {
148-
throwUnknownField(currentFieldName, parser.getTokenLocation());
152+
parser.skipChildren();
149153
}
150154
} else {
151-
throwUnknownToken(token, parser.getTokenLocation());
155+
parser.skipChildren();
152156
}
153157
}
154158
searchProfileResults.put(id, new ProfileShardResult(queryProfileResults, aggProfileShardResult));

‎core/src/main/java/org/elasticsearch/search/profile/query/CollectorResult.java‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,6 @@
3434
import java.util.concurrent.TimeUnit;
3535

3636
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
37-
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownField;
38-
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownToken;
3937

4038
/**
4139
* Public interface and serialization container for profiled timings of the
@@ -181,18 +179,18 @@ public static CollectorResult fromXContent(XContentParser parser) throws IOExcep
181179
} else if (TIME_NANOS.match(currentFieldName)) {
182180
time = parser.longValue();
183181
} else {
184-
throwUnknownField(currentFieldName, parser.getTokenLocation());
182+
parser.skipChildren();
185183
}
186184
} else if (token == XContentParser.Token.START_ARRAY) {
187185
if (CHILDREN.match(currentFieldName)) {
188186
while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
189187
children.add(CollectorResult.fromXContent(parser));
190188
}
191189
} else {
192-
throwUnknownField(currentFieldName, parser.getTokenLocation());
190+
parser.skipChildren();
193191
}
194192
} else {
195-
throwUnknownToken(token, parser.getTokenLocation());
193+
parser.skipChildren();
196194
}
197195
}
198196
return new CollectorResult(name, reason, time, children);

‎core/src/main/java/org/elasticsearch/search/profile/query/QueryProfileShardResult.java‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,6 @@
3333
import java.util.List;
3434

3535
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
36-
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownField;
37-
import static org.elasticsearch.common.xcontent.XContentParserUtils.throwUnknownToken;
3836

3937
/**
4038
* A container class to hold the profile results for a single shard in the request.
@@ -127,7 +125,7 @@ public static QueryProfileShardResult fromXContent(XContentParser parser) throws
127125
if (REWRITE_TIME.equals(currentFieldName)) {
128126
rewriteTime = parser.longValue();
129127
} else {
130-
throwUnknownField(currentFieldName, parser.getTokenLocation());
128+
parser.skipChildren();
131129
}
132130
} else if (token == XContentParser.Token.START_ARRAY) {
133131
if (QUERY_ARRAY.equals(currentFieldName)) {
@@ -139,10 +137,10 @@ public static QueryProfileShardResult fromXContent(XContentParser parser) throws
139137
collector = CollectorResult.fromXContent(parser);
140138
}
141139
} else {
142-
throwUnknownField(currentFieldName, parser.getTokenLocation());
140+
parser.skipChildren();
143141
}
144142
} else {
145-
throwUnknownToken(token, parser.getTokenLocation());
143+
parser.skipChildren();
146144
}
147145
}
148146
return new QueryProfileShardResult(queryProfileResults, rewriteTime, collector);

‎core/src/test/java/org/elasticsearch/search/profile/ProfileResultTests.java‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,11 @@
3333
import java.util.HashMap;
3434
import java.util.List;
3535
import java.util.Map;
36+
import java.util.function.Predicate;
3637

3738
import static org.elasticsearch.common.xcontent.XContentHelper.toXContent;
3839
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
40+
import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;
3941
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent;
4042

4143
public class ProfileResultTests extends ESTestCase {
@@ -62,12 +64,32 @@ public static ProfileResult createTestItem(int depth) {
6264
}
6365

6466
public void testFromXContent() throws IOException {
67+
doFromXContentTestWithRandomFields(false);
68+
}
69+
70+
/**
71+
* This test adds random fields and objects to the xContent rendered out to ensure we can parse it
72+
* back to be forward compatible with additions to the xContent
73+
*/
74+
public void testFromXContentWithRandomFields() throws IOException {
75+
doFromXContentTestWithRandomFields(true);
76+
}
77+
78+
private void doFromXContentTestWithRandomFields(boolean addRandomFields) throws IOException {
6579
ProfileResult profileResult = createTestItem(2);
6680
XContentType xContentType = randomFrom(XContentType.values());
6781
boolean humanReadable = randomBoolean();
6882
BytesReference originalBytes = toShuffledXContent(profileResult, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);
83+
BytesReference mutated;
84+
if (addRandomFields) {
85+
// "breakdown" just consists of key/value pairs, we shouldn't add anything random there
86+
Predicate<String> excludeFilter = (s) -> s.endsWith(ProfileResult.BREAKDOWN.getPreferredName());
87+
mutated = insertRandomFields(xContentType, originalBytes, excludeFilter, random());
88+
} else {
89+
mutated = originalBytes;
90+
}
6991
ProfileResult parsed;
70-
try (XContentParser parser = createParser(xContentType.xContent(), originalBytes)) {
92+
try (XContentParser parser = createParser(xContentType.xContent(), mutated)) {
7193
ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
7294
parsed = ProfileResult.fromXContent(parser);
7395
assertEquals(XContentParser.Token.END_OBJECT, parser.currentToken());

‎core/src/test/java/org/elasticsearch/search/profile/SearchProfileShardResultsTests.java‎

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,12 @@
3434
import java.util.HashMap;
3535
import java.util.List;
3636
import java.util.Map;
37+
import java.util.function.Predicate;
3738

3839
import static org.elasticsearch.common.xcontent.XContentHelper.toXContent;
3940
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
4041
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureFieldName;
42+
import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;
4143
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent;;
4244

4345
public class SearchProfileShardResultsTests extends ESTestCase {
@@ -58,20 +60,43 @@ public static SearchProfileShardResults createTestItem() {
5860
}
5961

6062
public void testFromXContent() throws IOException {
63+
doFromXContentTestWithRandomFields(false);
64+
}
65+
66+
/**
67+
* This test adds random fields and objects to the xContent rendered out to ensure we can parse it
68+
* back to be forward compatible with additions to the xContent
69+
*/
70+
public void testFromXContentWithRandomFields() throws IOException {
71+
doFromXContentTestWithRandomFields(true);
72+
}
73+
74+
private void doFromXContentTestWithRandomFields(boolean addRandomFields) throws IOException {
6175
SearchProfileShardResults shardResult = createTestItem();
6276
XContentType xContentType = randomFrom(XContentType.values());
6377
boolean humanReadable = randomBoolean();
6478
BytesReference originalBytes = toShuffledXContent(shardResult, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);
79+
BytesReference mutated;
80+
if (addRandomFields) {
81+
// The ProfileResults "breakdown" section just consists of key/value pairs, we shouldn't add anything random there
82+
// also we don't want to insert into the root object here, its just the PROFILE_FIELD itself
83+
Predicate<String> excludeFilter = (s) -> (s.isEmpty() || s.endsWith(ProfileResult.BREAKDOWN.getPreferredName()));
84+
mutated = insertRandomFields(xContentType, originalBytes, excludeFilter, random());
85+
} else {
86+
mutated = originalBytes;
87+
}
6588
SearchProfileShardResults parsed;
66-
try (XContentParser parser = createParser(xContentType.xContent(), originalBytes)) {
89+
try (XContentParser parser = createParser(xContentType.xContent(), mutated)) {
6790
ensureExpectedToken(parser.nextToken(), XContentParser.Token.START_OBJECT, parser::getTokenLocation);
6891
ensureFieldName(parser, parser.nextToken(), SearchProfileShardResults.PROFILE_FIELD);
6992
ensureExpectedToken(parser.nextToken(), XContentParser.Token.START_OBJECT, parser::getTokenLocation);
7093
parsed = SearchProfileShardResults.fromXContent(parser);
94+
assertEquals(XContentParser.Token.END_OBJECT, parser.currentToken());
7195
assertEquals(XContentParser.Token.END_OBJECT, parser.nextToken());
7296
assertNull(parser.nextToken());
7397
}
7498
assertToXContentEquivalent(originalBytes, toXContent(parsed, xContentType, humanReadable), xContentType);
99+
75100
}
76101

77102
}

‎core/src/test/java/org/elasticsearch/search/profile/query/CollectorResultTests.java‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434

3535
import static org.elasticsearch.common.xcontent.XContentHelper.toXContent;
3636
import static org.elasticsearch.common.xcontent.XContentParserUtils.ensureExpectedToken;
37+
import static org.elasticsearch.test.XContentTestUtils.insertRandomFields;
3738
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent;
3839

3940
public class CollectorResultTests extends ESTestCase {
@@ -57,18 +58,30 @@ public static CollectorResult createTestItem(int depth) {
5758
}
5859

5960
public void testFromXContent() throws IOException {
61+
doFromXContentTestWithRandomFields(false);
62+
}
63+
64+
public void testFromXContentWithRandomFields() throws IOException {
65+
doFromXContentTestWithRandomFields(true);
66+
}
67+
68+
private void doFromXContentTestWithRandomFields(boolean addRandomFields) throws IOException {
6069
CollectorResult collectorResult = createTestItem(1);
6170
XContentType xContentType = randomFrom(XContentType.values());
6271
boolean humanReadable = randomBoolean();
6372
BytesReference originalBytes = toShuffledXContent(collectorResult, xContentType, ToXContent.EMPTY_PARAMS, humanReadable);
64-
65-
CollectorResult parsed;
66-
try (XContentParser parser = createParser(xContentType.xContent(), originalBytes)) {
73+
BytesReference mutated;
74+
if (addRandomFields) {
75+
mutated = insertRandomFields(xContentType, originalBytes, null, random());
76+
} else {
77+
mutated = originalBytes;
78+
}
79+
try (XContentParser parser = createParser(xContentType.xContent(), mutated)) {
6780
ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser::getTokenLocation);
68-
parsed = CollectorResult.fromXContent(parser);
81+
CollectorResult parsed = CollectorResult.fromXContent(parser);
6982
assertNull(parser.nextToken());
83+
assertToXContentEquivalent(originalBytes, toXContent(parsed, xContentType, humanReadable), xContentType);
7084
}
71-
assertToXContentEquivalent(originalBytes, toXContent(parsed, xContentType, humanReadable), xContentType);
7285
}
7386

7487
public void testToXContent() throws IOException {

0 commit comments

Comments
 (0)