Skip to content

Commit

Permalink
Use faster and cleaner map parsing loop in ObjectParser.parse (#86319)
Browse files Browse the repository at this point in the history 
Much easier to use the new API and not have to worry about all the manual
checks on the field name that were dead code anyway because they'd only be
hit on mal-formed input bytes which Jackson would throw on before entering
our code.
  • Loading branch information
@original-brownbear
original-brownbear authored May 3, 2022
1 parent cb41ed0 commit 017bf7f
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 40 deletions.
69 changes: 32 additions & 37 deletions libs/x-content/src/main/java/org/elasticsearch/xcontent/ObjectParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,6 @@ public Value parse(XContentParser parser, Value value, Context context) throws I
}
}

FieldParser fieldParser = null;
String currentFieldName = null;
XContentLocation currentPosition = null;
final List<String[]> requiredFields = this.requiredFieldSets.isEmpty() ? null : new ArrayList<>(this.requiredFieldSets);
final List<List<String>> exclusiveFields;
if (exclusiveFieldSets.isEmpty()) {
Expand All @@ -289,36 +286,32 @@ public Value parse(XContentParser parser, Value value, Context context) throws I
}
}

FieldParser fieldParser;
String currentFieldName;
XContentLocation currentPosition;
final Map<String, FieldParser> parsers = fieldParserMap.getOrDefault(parser.getRestApiVersion(), Collections.emptyMap());
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
currentPosition = parser.getTokenLocation();
fieldParser = parsers.get(currentFieldName);
while ((currentFieldName = parser.nextFieldName()) != null) {
currentPosition = parser.getTokenLocation();
fieldParser = parsers.get(currentFieldName);
token = parser.nextToken();
if (fieldParser == null) {
unknownFieldParser.acceptUnknownField(this, currentFieldName, currentPosition, parser, value, context);
} else {
if (currentFieldName == null) {
throwNoFieldFound(parser);
fieldParser.assertSupports(name, parser, token, currentFieldName);

if (requiredFields != null) {
// Check to see if this field is a required field, if it is we can
// remove the entry as the requirement is satisfied
maybeMarkRequiredField(currentFieldName, requiredFields);
}
if (fieldParser == null) {
unknownFieldParser.acceptUnknownField(this, currentFieldName, currentPosition, parser, value, context);
} else {
fieldParser.assertSupports(name, parser, currentFieldName);

if (requiredFields != null) {
// Check to see if this field is a required field, if it is we can
// remove the entry as the requirement is satisfied
maybeMarkRequiredField(currentFieldName, requiredFields);
}

if (exclusiveFields != null) {
// Check if this field is in an exclusive set, if it is then mark
// it as seen.
maybeMarkExclusiveField(currentFieldName, exclusiveFields);
}

parseSub(parser, fieldParser, currentFieldName, value, context);

if (exclusiveFields != null) {
// Check if this field is in an exclusive set, if it is then mark
// it as seen.
maybeMarkExclusiveField(currentFieldName, exclusiveFields);
}
fieldParser = null;

parseSub(parser, fieldParser, token, currentFieldName, value, context);
}
}

Expand All @@ -336,10 +329,6 @@ private void throwExpectedStartObject(XContentParser parser, XContentParser.Toke
throw new XContentParseException(parser.getTokenLocation(), "[" + name + "] Expected START_OBJECT but was: " + token);
}

private void throwNoFieldFound(XContentParser parser) {
throw new XContentParseException(parser.getTokenLocation(), "[" + name + "] no field found");
}

private static void throwMissingRequiredFields(List<String[]> requiredFields) {
final StringBuilder message = new StringBuilder();
for (String[] fields : requiredFields) {
Expand Down Expand Up @@ -626,8 +615,14 @@ private void throwFailedToParse(XContentParser parser, String currentFieldName,
throw new XContentParseException(parser.getTokenLocation(), "[" + name + "] failed to parse field [" + currentFieldName + "]", ex);
}

private void parseSub(XContentParser parser, FieldParser fieldParser, String currentFieldName, Value value, Context context) {
final XContentParser.Token token = parser.currentToken();
private void parseSub(
XContentParser parser,
FieldParser fieldParser,
XContentParser.Token token,
String currentFieldName,
Value value,
Context context
) {
switch (token) {
case START_OBJECT -> {
parseValue(parser, fieldParser, currentFieldName, value, context);
Expand Down Expand Up @@ -689,7 +684,7 @@ private class FieldParser {
this.type = type;
}

void assertSupports(String parserName, XContentParser xContentParser, String currentFieldName) {
void assertSupports(String parserName, XContentParser xContentParser, XContentParser.Token currentToken, String currentFieldName) {
boolean match = parseField.match(
parserName,
xContentParser::getTokenLocation,
Expand All @@ -705,7 +700,7 @@ void assertSupports(String parserName, XContentParser xContentParser, String cur
if (supportedTokens.contains(xContentParser.currentToken()) == false) {
throw new XContentParseException(
xContentParser.getTokenLocation(),
"[" + parserName + "] " + currentFieldName + " doesn't support values of type: " + xContentParser.currentToken()
"[" + parserName + "] " + currentFieldName + " doesn't support values of type: " + currentToken
);
}
}
Expand Down
5 changes: 2 additions & 3 deletions ...ticsearch/xpack/watcher/notification/email/attachment/ReportingAttachmentParserTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentEOFException;
import org.elasticsearch.xcontent.XContentParseException;
import org.elasticsearch.xcontent.XContentParser;
import org.elasticsearch.xpack.core.watcher.execution.WatchExecutionContext;
Expand Down Expand Up @@ -234,8 +233,8 @@ public void testInitialRequestContainsInvalidPayload() throws Exception {
// closing json bracket is missing
.thenReturn(new HttpResponse(200, "{\"path\":\"anything\""));
ReportingAttachment attachment = new ReportingAttachment("foo", dashboardUrl, randomBoolean(), null, null, null, null);
XContentEOFException e = expectThrows(
XContentEOFException.class,
XContentParseException e = expectThrows(
XContentParseException.class,
() -> reportingAttachmentParser.toAttachment(createWatchExecutionContext(), Payload.EMPTY, attachment)
);
assertThat(e.getMessage(), containsString("Unexpected end-of-input"));
Expand Down

0 comments on commit 017bf7f

Please sign in to comment.