Skip to content

Improve exception handling for JsonXContentParser #123439

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

package org.elasticsearch.benchmark.xcontent;

import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.Streams;
import org.elasticsearch.xcontent.XContentParser;
import org.elasticsearch.xcontent.XContentParserConfiguration;
import org.elasticsearch.xcontent.XContentType;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Level;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Warmup;
import org.openjdk.jmh.infra.Blackhole;

import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Random;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;

@Fork(1)
@Warmup(iterations = 5)
@Measurement(iterations = 10)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@State(Scope.Thread)
public class JsonParserBenchmark {
private Map<String, BytesReference> sourceBytes;
private BytesReference source;
private Random random;
private List<String> sourcesRandomized;

final String[] sources = new String[] { "monitor_cluster_stats.json", "monitor_index_stats.json", "monitor_node_stats.json" };

@Setup(Level.Iteration)
public void randomizeSource() {
sourcesRandomized = Arrays.asList(sources);
Collections.shuffle(sourcesRandomized, random);
}

@Setup(Level.Trial)
public void setup() throws IOException {
random = new Random();
sourceBytes = Arrays.stream(sources).collect(Collectors.toMap(s -> s, s -> {
try {
return readSource(s);
} catch (IOException e) {
throw new RuntimeException(e);
}
}));
}

@Benchmark
public void parseJson(Blackhole bh) throws IOException {
sourcesRandomized.forEach(source -> {
try {
final XContentParser parser = XContentType.JSON.xContent()
.createParser(XContentParserConfiguration.EMPTY, sourceBytes.get(source).streamInput());
bh.consume(parser.mapOrdered());
} catch (IOException e) {
throw new RuntimeException(e);
}
});
}

private BytesReference readSource(String fileName) throws IOException {
return Streams.readFully(JsonParserBenchmark.class.getResourceAsStream(fileName));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@
import org.elasticsearch.xcontent.XContent;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentGenerator;
import org.elasticsearch.xcontent.XContentParseException;
import org.elasticsearch.xcontent.XContentParser;
import org.elasticsearch.xcontent.XContentParserConfiguration;
import org.elasticsearch.xcontent.XContentType;
import org.elasticsearch.xcontent.provider.XContentImplUtils;

import java.io.CharConversionException;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
Expand Down Expand Up @@ -96,21 +98,37 @@ private XContentParser createParser(XContentParserConfiguration config, JsonPars

@Override
public XContentParser createParser(XContentParserConfiguration config, String content) throws IOException {
return createParser(config, jsonFactory.createParser(content));
try {
return createParser(config, jsonFactory.createParser(content));
} catch (CharConversionException e) {
throw new XContentParseException(null, e.getMessage(), e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JsonFactory.createParser can throw JsonParseException, so shouldn't this catch also use handleParseException?

}
}

@Override
public XContentParser createParser(XContentParserConfiguration config, InputStream is) throws IOException {
return createParser(config, jsonFactory.createParser(is));
try {
return createParser(config, jsonFactory.createParser(is));
} catch (CharConversionException e) {
throw new XContentParseException(null, e.getMessage(), e);
}
}

@Override
public XContentParser createParser(XContentParserConfiguration config, byte[] data, int offset, int length) throws IOException {
return createParser(config, jsonFactory.createParser(data, offset, length));
try {
return createParser(config, jsonFactory.createParser(data, offset, length));
} catch (CharConversionException e) {
throw new XContentParseException(null, e.getMessage(), e);
}
}

@Override
public XContentParser createParser(XContentParserConfiguration config, Reader reader) throws IOException {
return createParser(config, jsonFactory.createParser(reader));
try {
return createParser(config, jsonFactory.createParser(reader));
} catch (CharConversionException e) {
throw new XContentParseException(null, e.getMessage(), e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.core.JsonToken;
import com.fasterxml.jackson.core.exc.InputCoercionException;
import com.fasterxml.jackson.core.exc.StreamConstraintsException;
import com.fasterxml.jackson.core.io.JsonEOFException;

import org.elasticsearch.core.IOUtils;
Expand All @@ -26,6 +27,7 @@
import org.elasticsearch.xcontent.provider.XContentParserConfigurationImpl;
import org.elasticsearch.xcontent.support.AbstractXContentParser;

import java.io.CharConversionException;
import java.io.IOException;
import java.nio.CharBuffer;

Expand All @@ -48,29 +50,51 @@ public void allowDuplicateKeys(boolean allowDuplicateKeys) {
parser.configure(JsonParser.Feature.STRICT_DUPLICATE_DETECTION, allowDuplicateKeys == false);
}

private static XContentParseException newXContentParseException(JsonProcessingException e) {
private static XContentLocation getLocation(JsonProcessingException e) {
JsonLocation loc = e.getLocation();
throw new XContentParseException(new XContentLocation(loc.getLineNr(), loc.getColumnNr()), e.getMessage(), e);
if (loc != null) {
return new XContentLocation(loc.getLineNr(), loc.getColumnNr());
} else {
return null;
}
}

private static XContentParseException newXContentParseException(JsonProcessingException e) {
return new XContentParseException(getLocation(e), e.getMessage(), e);
}

/**
* Handle parser exception depending on type.
* This converts known exceptions to XContentParseException and rethrows them.
*/
private static IOException handleParserException(IOException e) throws IOException {
switch (e) {
case JsonEOFException eof -> throw new XContentEOFException(getLocation(eof), "Unexpected end of file", e);
case JsonParseException pe -> throw newXContentParseException(pe);
case InputCoercionException ice -> throw newXContentParseException(ice);
case CharConversionException cce -> throw new XContentParseException(null, cce.getMessage(), cce);
case StreamConstraintsException sce -> throw newXContentParseException(sce);
default -> {
return e;
}
}
}

@Override
public Token nextToken() throws IOException {
try {
return convertToken(parser.nextToken());
} catch (JsonEOFException 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);
} catch (IOException e) {
throw handleParserException(e);
}
}

@Override
public String nextFieldName() throws IOException {
try {
return parser.nextFieldName();
} catch (JsonParseException e) {
throw newXContentParseException(e);
} catch (IOException e) {
throw handleParserException(e);
}
}

Expand Down Expand Up @@ -98,8 +122,8 @@ public String currentName() throws IOException {
protected boolean doBooleanValue() throws IOException {
try {
return parser.getBooleanValue();
} catch (JsonParseException e) {
throw newXContentParseException(e);
} catch (IOException e) {
throw handleParserException(e);
}
}

Expand All @@ -110,8 +134,8 @@ public String text() throws IOException {
}
try {
return parser.getText();
} catch (JsonParseException e) {
throw newXContentParseException(e);
} catch (IOException e) {
throw handleParserException(e);
}
}

Expand All @@ -123,8 +147,8 @@ private void throwOnNoText() {
public CharBuffer charBuffer() throws IOException {
try {
return CharBuffer.wrap(parser.getTextCharacters(), parser.getTextOffset(), parser.getTextLength());
} catch (JsonParseException e) {
throw newXContentParseException(e);
} catch (IOException e) {
throw handleParserException(e);
}
}

Expand Down Expand Up @@ -173,89 +197,89 @@ public boolean hasTextCharacters() {
public char[] textCharacters() throws IOException {
try {
return parser.getTextCharacters();
} catch (JsonParseException e) {
throw newXContentParseException(e);
} catch (IOException e) {
throw handleParserException(e);
}
}

@Override
public int textLength() throws IOException {
try {
return parser.getTextLength();
} catch (JsonParseException e) {
throw newXContentParseException(e);
} catch (IOException e) {
throw handleParserException(e);
}
}

@Override
public int textOffset() throws IOException {
try {
return parser.getTextOffset();
} catch (JsonParseException e) {
throw newXContentParseException(e);
} catch (IOException e) {
throw handleParserException(e);
}
}

@Override
public Number numberValue() throws IOException {
try {
return parser.getNumberValue();
} catch (InputCoercionException | JsonParseException e) {
throw newXContentParseException(e);
} catch (IOException e) {
throw handleParserException(e);
}
}

@Override
public short doShortValue() throws IOException {
try {
return parser.getShortValue();
} catch (InputCoercionException | JsonParseException e) {
throw newXContentParseException(e);
} catch (IOException e) {
throw handleParserException(e);
}
}

@Override
public int doIntValue() throws IOException {
try {
return parser.getIntValue();
} catch (InputCoercionException | JsonParseException e) {
throw newXContentParseException(e);
} catch (IOException e) {
throw handleParserException(e);
}
}

@Override
public long doLongValue() throws IOException {
try {
return parser.getLongValue();
} catch (InputCoercionException | JsonParseException e) {
throw newXContentParseException(e);
} catch (IOException e) {
throw handleParserException(e);
}
}

@Override
public float doFloatValue() throws IOException {
try {
return parser.getFloatValue();
} catch (InputCoercionException | JsonParseException e) {
throw newXContentParseException(e);
} catch (IOException e) {
throw handleParserException(e);
}
}

@Override
public double doDoubleValue() throws IOException {
try {
return parser.getDoubleValue();
} catch (InputCoercionException | JsonParseException e) {
throw newXContentParseException(e);
} catch (IOException e) {
throw handleParserException(e);
}
}

@Override
public byte[] binaryValue() throws IOException {
try {
return parser.getBinaryValue();
} catch (JsonParseException e) {
throw newXContentParseException(e);
} catch (IOException e) {
throw handleParserException(e);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ public void testInitialRequestContainsInvalidPayload() throws Exception {
XContentParseException.class,
() -> reportingAttachmentParser.toAttachment(createWatchExecutionContext(), Payload.EMPTY, attachment)
);
assertThat(e.getMessage(), containsString("Unexpected end-of-input"));
assertThat(e.getMessage(), containsString("Unexpected end of file"));
}

public void testInitialRequestContainsPathAsObject() throws Exception {
Expand Down