Skip to content

Commit 523ba41

Browse files
JeremyDahlgrenJeremyDahlgren
authored
Fix SnapshotStats toXContent()/fromXContent() round trip serialization (#128928)
toXContent() has a check to omit the "processed" sub-object if the processedFileCount equals the incrementalFileCount. In the scenario where they are equal and greater than zero, if you were to read the JSON back in with fromXContent(), the SnapshotStats object would not be the same as the original since processedFileCount is not parsed and set. This change adds a fix and adjusts the unit test to increase the probability of creating this scenario.
1 parent 5d22ad6 commit 523ba41

File tree

5 files changed

+129
-108
lines changed

5 files changed

+129
-108
lines changed

‎server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotStats.java

Lines changed: 0 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,10 @@
1414
import org.elasticsearch.common.io.stream.StreamOutput;
1515
import org.elasticsearch.common.io.stream.Writeable;
1616
import org.elasticsearch.common.unit.ByteSizeValue;
17-
import org.elasticsearch.common.xcontent.XContentParserUtils;
1817
import org.elasticsearch.core.TimeValue;
1918
import org.elasticsearch.xcontent.ToXContent;
2019
import org.elasticsearch.xcontent.ToXContentObject;
2120
import org.elasticsearch.xcontent.XContentBuilder;
22-
import org.elasticsearch.xcontent.XContentParser;
2321

2422
import java.io.IOException;
2523

@@ -192,108 +190,6 @@ public XContentBuilder toXContent(XContentBuilder builder, ToXContent.Params par
192190
return builder.endObject();
193191
}
194192

195-
public static SnapshotStats fromXContent(XContentParser parser) throws IOException {
196-
// Parse this old school style instead of using the ObjectParser since there's an impedance mismatch between how the
197-
// object has historically been written as JSON versus how it is structured in Java.
198-
XContentParser.Token token = parser.currentToken();
199-
if (token == null) {
200-
token = parser.nextToken();
201-
}
202-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser);
203-
long startTime = 0;
204-
long time = 0;
205-
int incrementalFileCount = 0;
206-
int totalFileCount = 0;
207-
int processedFileCount = 0;
208-
long incrementalSize = 0;
209-
long totalSize = 0;
210-
long processedSize = 0;
211-
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
212-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser);
213-
String currentName = parser.currentName();
214-
token = parser.nextToken();
215-
if (currentName.equals(Fields.INCREMENTAL)) {
216-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser);
217-
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
218-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser);
219-
String innerName = parser.currentName();
220-
token = parser.nextToken();
221-
if (innerName.equals(Fields.FILE_COUNT)) {
222-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
223-
incrementalFileCount = parser.intValue();
224-
} else if (innerName.equals(Fields.SIZE_IN_BYTES)) {
225-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
226-
incrementalSize = parser.longValue();
227-
} else {
228-
// Unknown sub field, skip
229-
if (token == XContentParser.Token.START_OBJECT || token == XContentParser.Token.START_ARRAY) {
230-
parser.skipChildren();
231-
}
232-
}
233-
}
234-
} else if (currentName.equals(Fields.PROCESSED)) {
235-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser);
236-
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
237-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser);
238-
String innerName = parser.currentName();
239-
token = parser.nextToken();
240-
if (innerName.equals(Fields.FILE_COUNT)) {
241-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
242-
processedFileCount = parser.intValue();
243-
} else if (innerName.equals(Fields.SIZE_IN_BYTES)) {
244-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
245-
processedSize = parser.longValue();
246-
} else {
247-
// Unknown sub field, skip
248-
if (token == XContentParser.Token.START_OBJECT || token == XContentParser.Token.START_ARRAY) {
249-
parser.skipChildren();
250-
}
251-
}
252-
}
253-
} else if (currentName.equals(Fields.TOTAL)) {
254-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser);
255-
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
256-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser);
257-
String innerName = parser.currentName();
258-
token = parser.nextToken();
259-
if (innerName.equals(Fields.FILE_COUNT)) {
260-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
261-
totalFileCount = parser.intValue();
262-
} else if (innerName.equals(Fields.SIZE_IN_BYTES)) {
263-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
264-
totalSize = parser.longValue();
265-
} else {
266-
// Unknown sub field, skip
267-
if (token == XContentParser.Token.START_OBJECT || token == XContentParser.Token.START_ARRAY) {
268-
parser.skipChildren();
269-
}
270-
}
271-
}
272-
} else if (currentName.equals(Fields.START_TIME_IN_MILLIS)) {
273-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
274-
startTime = parser.longValue();
275-
} else if (currentName.equals(Fields.TIME_IN_MILLIS)) {
276-
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
277-
time = parser.longValue();
278-
} else {
279-
// Unknown field, skip
280-
if (token == XContentParser.Token.START_OBJECT || token == XContentParser.Token.START_ARRAY) {
281-
parser.skipChildren();
282-
}
283-
}
284-
}
285-
return new SnapshotStats(
286-
startTime,
287-
time,
288-
incrementalFileCount,
289-
totalFileCount,
290-
processedFileCount,
291-
incrementalSize,
292-
totalSize,
293-
processedSize
294-
);
295-
}
296-
297193
/**
298194
* Add stats instance to the total
299195
* @param stats Stats instance to add

‎server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotIndexShardStatusTests.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,11 @@ protected boolean supportsUnknownFields() {
9595
innerParser.declareString(constructorArg(), new ParseField(SnapshotIndexShardStatus.Fields.STAGE));
9696
innerParser.declareString(optionalConstructorArg(), new ParseField(SnapshotIndexShardStatus.Fields.NODE));
9797
innerParser.declareString(optionalConstructorArg(), new ParseField(SnapshotIndexShardStatus.Fields.REASON));
98-
innerParser.declareObject(constructorArg(), (p, c) -> SnapshotStats.fromXContent(p), new ParseField(SnapshotStats.Fields.STATS));
98+
innerParser.declareObject(
99+
constructorArg(),
100+
(p, c) -> SnapshotStatsTests.fromXContent(p),
101+
new ParseField(SnapshotStats.Fields.STATS)
102+
);
99103
PARSER = (p, indexId, shardName) -> {
100104
// Combine the index name in the context with the shard name passed in for the named object parser
101105
// into a ShardId to pass as context for the inner parser.

‎server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotIndexStatusTests.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,11 @@ public class SnapshotIndexStatusTests extends AbstractXContentTestCase<SnapshotI
5757
(p, c) -> SnapshotShardsStatsTests.PARSER.apply(p, null),
5858
new ParseField(SnapshotShardsStats.Fields.SHARDS_STATS)
5959
);
60-
innerParser.declareObject(constructorArg(), (p, c) -> SnapshotStats.fromXContent(p), new ParseField(SnapshotStats.Fields.STATS));
60+
innerParser.declareObject(
61+
constructorArg(),
62+
(p, c) -> SnapshotStatsTests.fromXContent(p),
63+
new ParseField(SnapshotStats.Fields.STATS)
64+
);
6165
innerParser.declareNamedObjects(
6266
constructorArg(),
6367
SnapshotIndexShardStatusTests.PARSER,

‎server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotStatsTests.java

Lines changed: 118 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
package org.elasticsearch.action.admin.cluster.snapshots.status;
1111

12+
import org.elasticsearch.common.xcontent.XContentParserUtils;
1213
import org.elasticsearch.test.AbstractXContentTestCase;
1314
import org.elasticsearch.xcontent.XContentParser;
1415

@@ -27,6 +28,14 @@ protected SnapshotStats createTestInstance() {
2728
long incrementalSize = ((long) randomIntBetween(0, Integer.MAX_VALUE)) * 2;
2829
long totalSize = ((long) randomIntBetween(0, Integer.MAX_VALUE)) * 2;
2930
long processedSize = ((long) randomIntBetween(0, Integer.MAX_VALUE)) * 2;
31+
32+
// toXContent() omits the "processed" sub-object if processedFileCount == incrementalFileCount, so here we increase the probability
33+
// of that scenario so we can make sure the processed values are set as expected in fromXContent().
34+
if (randomBoolean()) {
35+
processedFileCount = incrementalFileCount;
36+
processedSize = incrementalSize;
37+
}
38+
3039
return new SnapshotStats(
3140
startTime,
3241
time,
@@ -41,11 +50,119 @@ protected SnapshotStats createTestInstance() {
4150

4251
@Override
4352
protected SnapshotStats doParseInstance(XContentParser parser) throws IOException {
44-
return SnapshotStats.fromXContent(parser);
53+
return fromXContent(parser);
4554
}
4655

4756
@Override
4857
protected boolean supportsUnknownFields() {
4958
return true;
5059
}
60+
61+
static SnapshotStats fromXContent(XContentParser parser) throws IOException {
62+
// Parse this old school style instead of using the ObjectParser since there's an impedance mismatch between how the
63+
// object has historically been written as JSON versus how it is structured in Java.
64+
XContentParser.Token token = parser.currentToken();
65+
if (token == null) {
66+
token = parser.nextToken();
67+
}
68+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser);
69+
long startTime = 0;
70+
long time = 0;
71+
int incrementalFileCount = 0;
72+
int totalFileCount = 0;
73+
int processedFileCount = Integer.MIN_VALUE;
74+
long incrementalSize = 0;
75+
long totalSize = 0;
76+
long processedSize = Long.MIN_VALUE;
77+
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
78+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser);
79+
String currentName = parser.currentName();
80+
token = parser.nextToken();
81+
if (currentName.equals(SnapshotStats.Fields.INCREMENTAL)) {
82+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser);
83+
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
84+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser);
85+
String innerName = parser.currentName();
86+
token = parser.nextToken();
87+
if (innerName.equals(SnapshotStats.Fields.FILE_COUNT)) {
88+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
89+
incrementalFileCount = parser.intValue();
90+
} else if (innerName.equals(SnapshotStats.Fields.SIZE_IN_BYTES)) {
91+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
92+
incrementalSize = parser.longValue();
93+
} else {
94+
// Unknown sub field, skip
95+
if (token == XContentParser.Token.START_OBJECT || token == XContentParser.Token.START_ARRAY) {
96+
parser.skipChildren();
97+
}
98+
}
99+
}
100+
} else if (currentName.equals(SnapshotStats.Fields.PROCESSED)) {
101+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser);
102+
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
103+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser);
104+
String innerName = parser.currentName();
105+
token = parser.nextToken();
106+
if (innerName.equals(SnapshotStats.Fields.FILE_COUNT)) {
107+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
108+
processedFileCount = parser.intValue();
109+
} else if (innerName.equals(SnapshotStats.Fields.SIZE_IN_BYTES)) {
110+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
111+
processedSize = parser.longValue();
112+
} else {
113+
// Unknown sub field, skip
114+
if (token == XContentParser.Token.START_OBJECT || token == XContentParser.Token.START_ARRAY) {
115+
parser.skipChildren();
116+
}
117+
}
118+
}
119+
} else if (currentName.equals(SnapshotStats.Fields.TOTAL)) {
120+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser);
121+
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
122+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser);
123+
String innerName = parser.currentName();
124+
token = parser.nextToken();
125+
if (innerName.equals(SnapshotStats.Fields.FILE_COUNT)) {
126+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
127+
totalFileCount = parser.intValue();
128+
} else if (innerName.equals(SnapshotStats.Fields.SIZE_IN_BYTES)) {
129+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
130+
totalSize = parser.longValue();
131+
} else {
132+
// Unknown sub field, skip
133+
if (token == XContentParser.Token.START_OBJECT || token == XContentParser.Token.START_ARRAY) {
134+
parser.skipChildren();
135+
}
136+
}
137+
}
138+
} else if (currentName.equals(SnapshotStats.Fields.START_TIME_IN_MILLIS)) {
139+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
140+
startTime = parser.longValue();
141+
} else if (currentName.equals(SnapshotStats.Fields.TIME_IN_MILLIS)) {
142+
XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, token, parser);
143+
time = parser.longValue();
144+
} else {
145+
// Unknown field, skip
146+
if (token == XContentParser.Token.START_OBJECT || token == XContentParser.Token.START_ARRAY) {
147+
parser.skipChildren();
148+
}
149+
}
150+
}
151+
// Handle the case where the "processed" sub-object is omitted in toXContent() when processedFileCount == incrementalFileCount.
152+
if (processedFileCount == Integer.MIN_VALUE) {
153+
assert processedSize == Long.MIN_VALUE;
154+
processedFileCount = incrementalFileCount;
155+
processedSize = incrementalSize;
156+
}
157+
return new SnapshotStats(
158+
startTime,
159+
time,
160+
incrementalFileCount,
161+
totalFileCount,
162+
processedFileCount,
163+
incrementalSize,
164+
totalSize,
165+
processedSize
166+
);
167+
}
51168
}

‎server/src/test/java/org/elasticsearch/action/admin/cluster/snapshots/status/SnapshotStatusTests.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ public class SnapshotStatusTests extends AbstractChunkedSerializingTestCase<Snap
7777
PARSER.declareBoolean(optionalConstructorArg(), new ParseField(SnapshotStatus.INCLUDE_GLOBAL_STATE));
7878
PARSER.declareField(
7979
constructorArg(),
80-
SnapshotStats::fromXContent,
80+
SnapshotStatsTests::fromXContent,
8181
new ParseField(SnapshotStats.Fields.STATS),
8282
ObjectParser.ValueType.OBJECT
8383
);

0 commit comments

Comments
 (0)