Skip to content

Commit ce3c354

Browse files
PeteGillinElasticPeteGillinElastic
authored
Apply more strict parsing of actions in bulk API (#115923)
Previously, the following classes of malformed input were deprecated but not rejected in the action lines of the a bulk request: - Missing closing brace; - Additional keys after the action (which were ignored); - Additional data after the closing brace (which was ignored). They will now be considered errors and rejected. The existing behaviour is preserved in v8 compatibility mode. (N.B. The deprecation warnings were added in 8.1. The normal guidance to deprecate for a whole major version before removing does not apply here, since this was never a supported API feature. There is a risk to the lenient approach since it results in input being ignored, which is likely not the user's intention.)
1 parent a3615f0 commit ce3c354

File tree

4 files changed

+171
-39
lines changed

4 files changed

+171
-39
lines changed

‎docs/changelog/115923.yaml‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
pr: 115923
2+
summary: Apply more strict parsing of actions in bulk API
3+
area: Indices APIs
4+
type: breaking
5+
issues: [ ]
6+
breaking:
7+
title: Apply more strict parsing of actions in bulk API
8+
area: REST API
9+
details: >-
10+
Previously, the following classes of malformed input were deprecated but not rejected in the action lines of the a
11+
bulk request: missing closing brace; additional keys after the action (which were ignored); additional data after
12+
the closing brace (which was ignored). They will now be considered errors and rejected.
13+
impact: >-
14+
Users must provide well-formed input when using the bulk API. (They can request REST API compatibility with v8 to
15+
get the previous behaviour back as an interim measure.)
16+
notable: false

‎server/src/main/java/org/elasticsearch/action/bulk/BulkRequestParser.java‎

Lines changed: 48 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
2020
import org.elasticsearch.core.Nullable;
2121
import org.elasticsearch.core.RestApiVersion;
22-
import org.elasticsearch.core.UpdateForV9;
22+
import org.elasticsearch.core.UpdateForV10;
2323
import org.elasticsearch.index.VersionType;
2424
import org.elasticsearch.index.seqno.SequenceNumbers;
2525
import org.elasticsearch.search.fetch.subphase.FetchSourceContext;
@@ -44,7 +44,11 @@
4444
* Helper to parse bulk requests. This should be considered an internal class.
4545
*/
4646
public final class BulkRequestParser {
47+
48+
@UpdateForV10(owner = UpdateForV10.Owner.DATA_MANAGEMENT)
49+
// Remove deprecation logger when its usages in checkBulkActionIsProperlyClosed are removed
4750
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(BulkRequestParser.class);
51+
4852
private static final Set<String> SUPPORTED_ACTIONS = Set.of("create", "index", "update", "delete");
4953
private static final String STRICT_ACTION_PARSING_WARNING_KEY = "bulk_request_strict_action_parsing";
5054

@@ -348,7 +352,7 @@ public int incrementalParse(
348352
+ "]"
349353
);
350354
}
351-
checkBulkActionIsProperlyClosed(parser);
355+
checkBulkActionIsProperlyClosed(parser, line);
352356

353357
if ("delete".equals(action)) {
354358
if (dynamicTemplates.isEmpty() == false) {
@@ -446,35 +450,55 @@ public int incrementalParse(
446450
return isIncremental ? consumed : from;
447451
}
448452

449-
@UpdateForV9(owner = UpdateForV9.Owner.DISTRIBUTED_INDEXING)
450-
// Warnings will need to be replaced with XContentEOFException from 9.x
451-
private static void warnBulkActionNotProperlyClosed(String message) {
452-
deprecationLogger.compatibleCritical(STRICT_ACTION_PARSING_WARNING_KEY, message);
453-
}
454-
455-
private static void checkBulkActionIsProperlyClosed(XContentParser parser) throws IOException {
453+
@UpdateForV10(owner = UpdateForV10.Owner.DATA_MANAGEMENT) // Remove lenient parsing in V8 BWC mode
454+
private void checkBulkActionIsProperlyClosed(XContentParser parser, int line) throws IOException {
456455
XContentParser.Token token;
457456
try {
458457
token = parser.nextToken();
459-
} catch (XContentEOFException ignore) {
460-
warnBulkActionNotProperlyClosed(
461-
"A bulk action wasn't closed properly with the closing brace. Malformed objects are currently accepted but will be "
462-
+ "rejected in a future version."
463-
);
464-
return;
458+
} catch (XContentEOFException e) {
459+
if (config.restApiVersion() == RestApiVersion.V_8) {
460+
deprecationLogger.compatibleCritical(
461+
STRICT_ACTION_PARSING_WARNING_KEY,
462+
"A bulk action wasn't closed properly with the closing brace. Malformed objects are currently accepted but will be"
463+
+ " rejected in a future version."
464+
);
465+
return;
466+
} else {
467+
throw e;
468+
}
465469
}
466470
if (token != XContentParser.Token.END_OBJECT) {
467-
warnBulkActionNotProperlyClosed(
468-
"A bulk action object contained multiple keys. Additional keys are currently ignored but will be rejected in a "
469-
+ "future version."
470-
);
471-
return;
471+
if (config.restApiVersion() == RestApiVersion.V_8) {
472+
deprecationLogger.compatibleCritical(
473+
STRICT_ACTION_PARSING_WARNING_KEY,
474+
"A bulk action object contained multiple keys. Additional keys are currently ignored but will be rejected in a future"
475+
+ " version."
476+
);
477+
return;
478+
} else {
479+
throw new IllegalArgumentException(
480+
"Malformed action/metadata line ["
481+
+ line
482+
+ "], expected "
483+
+ XContentParser.Token.END_OBJECT
484+
+ " but found ["
485+
+ token
486+
+ "]"
487+
);
488+
}
472489
}
473490
if (parser.nextToken() != null) {
474-
warnBulkActionNotProperlyClosed(
475-
"A bulk action contained trailing data after the closing brace. This is currently ignored but will be rejected in a "
476-
+ "future version."
477-
);
491+
if (config.restApiVersion() == RestApiVersion.V_8) {
492+
deprecationLogger.compatibleCritical(
493+
STRICT_ACTION_PARSING_WARNING_KEY,
494+
"A bulk action contained trailing data after the closing brace. This is currently ignored but will be rejected in a"
495+
+ " future version."
496+
);
497+
} else {
498+
throw new IllegalArgumentException(
499+
"Malformed action/metadata line [" + line + "], unexpected data after the closing brace"
500+
);
501+
}
478502
}
479503
}
480504

‎server/src/test/java/org/elasticsearch/action/bulk/BulkRequestParserTests.java‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import org.elasticsearch.action.index.IndexRequest;
1313
import org.elasticsearch.common.bytes.BytesArray;
1414
import org.elasticsearch.core.RestApiVersion;
15+
import org.elasticsearch.core.UpdateForV10;
1516
import org.elasticsearch.test.ESTestCase;
1617
import org.elasticsearch.xcontent.XContentType;
1718
import org.hamcrest.Matchers;
@@ -20,9 +21,15 @@
2021
import java.util.ArrayList;
2122
import java.util.List;
2223
import java.util.concurrent.atomic.AtomicBoolean;
24+
import java.util.stream.Stream;
2325

2426
public class BulkRequestParserTests extends ESTestCase {
2527

28+
@UpdateForV10(owner = UpdateForV10.Owner.DATA_MANAGEMENT) // Replace with just RestApiVersion.values() when V8 no longer exists
29+
public static final List<RestApiVersion> REST_API_VERSIONS_POST_V8 = Stream.of(RestApiVersion.values())
30+
.filter(v -> v.compareTo(RestApiVersion.V_8) > 0)
31+
.toList();
32+
2633
public void testIndexRequest() throws IOException {
2734
BytesArray request = new BytesArray("""
2835
{ "index":{ "_id": "bar" } }
@@ -260,6 +267,90 @@ public void testFailOnInvalidAction() {
260267
);
261268
}
262269

270+
public void testFailMissingCloseBrace() {
271+
BytesArray request = new BytesArray("""
272+
{ "index":{ }
273+
{}
274+
""");
275+
BulkRequestParser parser = new BulkRequestParser(randomBoolean(), randomFrom(REST_API_VERSIONS_POST_V8));
276+
277+
IllegalArgumentException ex = expectThrows(
278+
IllegalArgumentException.class,
279+
() -> parser.parse(
280+
request,
281+
null,
282+
null,
283+
null,
284+
null,
285+
null,
286+
null,
287+
null,
288+
false,
289+
XContentType.JSON,
290+
(req, type) -> fail("expected failure before we got this far"),
291+
req -> fail("expected failure before we got this far"),
292+
req -> fail("expected failure before we got this far")
293+
)
294+
);
295+
assertEquals("[1:14] Unexpected end of file", ex.getMessage());
296+
}
297+
298+
public void testFailExtraKeys() {
299+
BytesArray request = new BytesArray("""
300+
{ "index":{ }, "something": "unexpected" }
301+
{}
302+
""");
303+
BulkRequestParser parser = new BulkRequestParser(randomBoolean(), randomFrom(REST_API_VERSIONS_POST_V8));
304+
305+
IllegalArgumentException ex = expectThrows(
306+
IllegalArgumentException.class,
307+
() -> parser.parse(
308+
request,
309+
null,
310+
null,
311+
null,
312+
null,
313+
null,
314+
null,
315+
null,
316+
false,
317+
XContentType.JSON,
318+
(req, type) -> fail("expected failure before we got this far"),
319+
req -> fail("expected failure before we got this far"),
320+
req -> fail("expected failure before we got this far")
321+
)
322+
);
323+
assertEquals("Malformed action/metadata line [1], expected END_OBJECT but found [FIELD_NAME]", ex.getMessage());
324+
}
325+
326+
public void testFailContentAfterClosingBrace() {
327+
BytesArray request = new BytesArray("""
328+
{ "index":{ } } { "something": "unexpected" }
329+
{}
330+
""");
331+
BulkRequestParser parser = new BulkRequestParser(randomBoolean(), randomFrom(REST_API_VERSIONS_POST_V8));
332+
333+
IllegalArgumentException ex = expectThrows(
334+
IllegalArgumentException.class,
335+
() -> parser.parse(
336+
request,
337+
null,
338+
null,
339+
null,
340+
null,
341+
null,
342+
null,
343+
null,
344+
false,
345+
XContentType.JSON,
346+
(req, type) -> fail("expected failure before we got this far"),
347+
req -> fail("expected failure before we got this far"),
348+
req -> fail("expected failure before we got this far")
349+
)
350+
);
351+
assertEquals("Malformed action/metadata line [1], unexpected data after the closing brace", ex.getMessage());
352+
}
353+
263354
public void testListExecutedPipelines() throws IOException {
264355
BytesArray request = new BytesArray("""
265356
{ "index":{ "_id": "bar" } }

‎server/src/test/java/org/elasticsearch/action/bulk/BulkRequestTests.java‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import static org.hamcrest.Matchers.equalTo;
4343
import static org.hamcrest.Matchers.hasSize;
4444
import static org.hamcrest.Matchers.instanceOf;
45+
import static org.hamcrest.Matchers.is;
4546
import static org.hamcrest.Matchers.not;
4647
import static org.hamcrest.Matchers.notNullValue;
4748

@@ -426,12 +427,12 @@ public void testBulkActionWithoutCurlyBrace() throws Exception {
426427
{ "field1" : "value1" }
427428
""";
428429
BulkRequest bulkRequest = new BulkRequest();
429-
bulkRequest.add(bulkAction.getBytes(StandardCharsets.UTF_8), 0, bulkAction.length(), null, XContentType.JSON);
430-
431-
assertWarnings(
432-
"A bulk action wasn't closed properly with the closing brace. Malformed objects are currently accepted"
433-
+ " but will be rejected in a future version."
430+
IllegalArgumentException ex = expectThrows(
431+
IllegalArgumentException.class,
432+
() -> bulkRequest.add(bulkAction.getBytes(StandardCharsets.UTF_8), 0, bulkAction.length(), null, XContentType.JSON)
434433
);
434+
435+
assertThat(ex.getMessage(), containsString("Unexpected end of file"));
435436
}
436437

437438
public void testBulkActionWithAdditionalKeys() throws Exception {
@@ -440,12 +441,12 @@ public void testBulkActionWithAdditionalKeys() throws Exception {
440441
{ "field1" : "value1" }
441442
""";
442443
BulkRequest bulkRequest = new BulkRequest();
443-
bulkRequest.add(bulkAction.getBytes(StandardCharsets.UTF_8), 0, bulkAction.length(), null, XContentType.JSON);
444-
445-
assertWarnings(
446-
"A bulk action object contained multiple keys. Additional keys are currently ignored but will be "
447-
+ "rejected in a future version."
444+
IllegalArgumentException ex = expectThrows(
445+
IllegalArgumentException.class,
446+
() -> bulkRequest.add(bulkAction.getBytes(StandardCharsets.UTF_8), 0, bulkAction.length(), null, XContentType.JSON)
448447
);
448+
449+
assertThat(ex.getMessage(), is("Malformed action/metadata line [1], expected END_OBJECT but found [FIELD_NAME]"));
449450
}
450451

451452
public void testBulkActionWithTrailingData() throws Exception {
@@ -454,12 +455,12 @@ public void testBulkActionWithTrailingData() throws Exception {
454455
{ "field1" : "value1" }
455456
""";
456457
BulkRequest bulkRequest = new BulkRequest();
457-
bulkRequest.add(bulkAction.getBytes(StandardCharsets.UTF_8), 0, bulkAction.length(), null, XContentType.JSON);
458-
459-
assertWarnings(
460-
"A bulk action contained trailing data after the closing brace. This is currently ignored "
461-
+ "but will be rejected in a future version."
458+
IllegalArgumentException ex = expectThrows(
459+
IllegalArgumentException.class,
460+
() -> bulkRequest.add(bulkAction.getBytes(StandardCharsets.UTF_8), 0, bulkAction.length(), null, XContentType.JSON)
462461
);
462+
463+
assertThat(ex.getMessage(), is("Malformed action/metadata line [1], unexpected data after the closing brace"));
463464
}
464465

465466
public void testUnsupportedAction() {

0 commit comments

Comments
 (0)