Skip to content

Commit

Permalink
Json parsing exceptions should not cause 500 errors (elastic#111548)
Browse files Browse the repository at this point in the history
Currently we wrap JsonEOFException from advancing the json parser into our own
XContentEOFException, but this has the drawback that is results in 500 errors on
the client side. Instead this should be 400 errors.
This changes XContentEOFException to extend XContentParseException so we report
a 400 error instead.

Closes elastic#111542
  • Loading branch information
cbuescher authored Sep 6, 2024
1 parent f763960 commit 000ebaf
Show file tree
Hide file tree
Showing 7 changed files with 18 additions and 15 deletions.
6 changes: 6 additions & 0 deletions docs/changelog/111548.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 111548
summary: Json parsing exceptions should not cause 500 errors
area: Infra/Core
type: bug
issues:
- 111542
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ public Token nextToken() throws IOException {
try {
return convertToken(parser.nextToken());
} catch (JsonEOFException e) {
throw new XContentEOFException(e);
JsonLocation location = e.getLocation();
throw new XContentEOFException(new XContentLocation(location.getLineNr(), location.getColumnNr()), "Unexpected end of file", e);
} catch (JsonParseException e) {
throw newXContentParseException(e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,9 @@

package org.elasticsearch.xcontent;

import java.io.IOException;
public class XContentEOFException extends XContentParseException {

public class XContentEOFException extends IOException {

public XContentEOFException(IOException cause) {
super(cause);
public XContentEOFException(XContentLocation location, String message, Exception cause) {
super(location, message, cause);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -114,14 +114,14 @@ setup:

- match: { responses.0.hits.total: 2 }
- match: { responses.1.error.root_cause.0.type: x_content_e_o_f_exception }
- match: { responses.1.error.root_cause.0.reason: "/Unexpected.end.of.input/" }
- match: { responses.1.error.root_cause.0.reason: "/\\[1:22\\].Unexpected.end.of.file/" }
- match: { responses.2.hits.total: 1 }
- match: { responses.3.error.root_cause.0.type: parsing_exception }
- match: { responses.3.error.root_cause.0.reason: "/unknown.query.\\[unknown\\]/" }
- match: { responses.4.error.root_cause.0.type: illegal_argument_exception }
- match: { responses.4.error.root_cause.0.reason: "[rest_total_hits_as_int] cannot be used if the tracking of total hits is not accurate, got 1" }
- match: { responses.0.status: 200 }
- match: { responses.1.status: 500 }
- match: { responses.1.status: 400 }
- match: { responses.2.status: 200 }
- match: { responses.3.status: 400 }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,7 @@ public void testPositionParserAtTokenAfterField_ThrowsWithMalformedJSON() throws
XContentEOFException.class,
() -> XContentUtils.positionParserAtTokenAfterField(parser, missingField, errorFormat)
);

assertThat(exception.getMessage(), containsString("Unexpected end-of-input"));
assertThat(exception.getMessage(), containsString("[4:1] Unexpected end of file"));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -309,8 +309,8 @@ public void testFails_ResponseIsInvalidJson_MissingSquareBracket() {
new HttpResult(mock(HttpResponse.class), responseJson.getBytes(StandardCharsets.UTF_8))
)
);

assertThat(thrownException.getMessage(), containsString("expected close marker for Array (start marker at"));
assertThat(thrownException.getMessage(), containsString("[5:1] Unexpected end of file"));
assertThat(thrownException.getCause().getMessage(), containsString("expected close marker for Array (start marker at"));
}

public void testFails_ResponseIsInvalidJson_MissingField() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

import org.apache.logging.log4j.Logger;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.xcontent.XContentEOFException;
import org.elasticsearch.xcontent.XContentParseException;
import org.elasticsearch.xcontent.XContentParser;

Expand Down Expand Up @@ -196,7 +195,7 @@ protected void initArrays(String[] record, boolean[] gotFields) {
protected XContentParser.Token tryNextTokenOrReadToEndOnError() throws IOException {
try {
return parser.nextToken();
} catch (XContentEOFException | XContentParseException e) {
} catch (XContentParseException e) {
logger.warn("Attempting to recover from malformed JSON data.", e);
for (int i = 0; i <= nestedLevel; ++i) {
readToEndOfObject();
Expand All @@ -217,7 +216,7 @@ protected void readToEndOfObject() throws IOException {
do {
try {
token = parser.nextToken();
} catch (XContentEOFException | XContentParseException e) {
} catch (XContentParseException e) {
++errorCounter;
if (errorCounter >= PARSE_ERRORS_LIMIT) {
logger.error("Failed to recover from malformed JSON data.", e);
Expand Down

0 comments on commit 000ebaf

Please sign in to comment.