From fd192dc37e266c02d379704bd3cfcd65ab1de262 Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Tue, 26 Mar 2019 11:31:09 +0100 Subject: [PATCH 1/4] Geo Point parse error fix When geo point parsing threw a parse exception, it did not consume remaining tokens from the parser. This in turn meant that indexing documents with malformed geo points into mappings with ignore_malformed=true would fail in some cases, since DocumentParser expects geo_point parsing to end on the END_OBJECT token. Related to #17617 --- .../elasticsearch/common/geo/GeoUtils.java | 37 +++++++++++++++---- .../mapper/GeoPointFieldMapperTests.java | 10 +++++ .../index/search/geo/GeoUtilsTests.java | 32 ++++++++++++++++ 3 files changed, 72 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java index a45667b908d74..76fe86a03c6be 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -433,6 +433,7 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina double lon = Double.NaN; String geohash = null; NumberFormatException numberFormatException = null; + ParseExceptionConsumer exceptionConsumer = new ParseExceptionConsumer(); if(parser.currentToken() == Token.START_OBJECT) { while(parser.nextToken() != Token.END_OBJECT) { @@ -450,7 +451,7 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina } break; default: - throw new ElasticsearchParseException("latitude must be a number"); + exceptionConsumer.consume(new ElasticsearchParseException("latitude must be a number")); } } else if (LONGITUDE.equals(field)) { parser.nextToken(); @@ -464,22 +465,25 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina } break; default: - throw new ElasticsearchParseException("longitude must be a number"); + exceptionConsumer.consume(new ElasticsearchParseException("longitude must be a number")); } } else if (GEOHASH.equals(field)) { if(parser.nextToken() == Token.VALUE_STRING) { geohash = parser.text(); } else { - throw new ElasticsearchParseException("geohash must be a string"); + exceptionConsumer.consume(new ElasticsearchParseException("geohash must be a string")); } } else { - throw new ElasticsearchParseException("field must be either [{}], [{}] or [{}]", LATITUDE, LONGITUDE, GEOHASH); + exceptionConsumer.consume(new ElasticsearchParseException("field must be either [{}], [{}] or [{}]", + LATITUDE, LONGITUDE, GEOHASH)); } } else { - throw new ElasticsearchParseException("token [{}] not allowed", parser.currentToken()); + exceptionConsumer.consume(new ElasticsearchParseException("token [{}] not allowed", parser.currentToken())); } } + exceptionConsumer.doThrow(); + if (geohash != null) { if(!Double.isNaN(lat) || !Double.isNaN(lon)) { throw new ElasticsearchParseException("field must be either lat/lon or geohash"); @@ -507,12 +511,17 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina } else if(element == 2) { lat = parser.doubleValue(); } else { - GeoPoint.assertZValue(ignoreZValue, parser.doubleValue()); + try { + GeoPoint.assertZValue(ignoreZValue, parser.doubleValue()); + } catch (ElasticsearchParseException e) { + exceptionConsumer.consume(e); + } } } else { - throw new ElasticsearchParseException("numeric value expected"); + exceptionConsumer.consume(new ElasticsearchParseException("numeric value expected")); } } + exceptionConsumer.doThrow(); return point.reset(lat, lon); } else if(parser.currentToken() == Token.VALUE_STRING) { String val = parser.text(); @@ -695,6 +704,20 @@ public boolean advanceExact(int target) throws IOException { } } + private static class ParseExceptionConsumer { + private ElasticsearchParseException exception; + + public void consume(ElasticsearchParseException e) { + if (exception == null) + exception = e; + } + + public void doThrow() { + if (exception != null) + throw exception; + } + } + private GeoUtils() { } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java b/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java index f5597ecb1f443..2142fca565c9b 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java @@ -523,5 +523,15 @@ public void testInvalidGeopointValuesIgnored() throws Exception { BytesReference.bytes(XContentFactory.jsonBuilder() .startObject().field("location", "NaN,12").endObject() ), XContentType.JSON)).rootDoc().getField("location"), nullValue()); + + assertThat(defaultMapper.parse(new SourceToParse("test", "type", "1", + BytesReference.bytes(XContentFactory.jsonBuilder() + .startObject().startObject("location").nullField("lat").field("lon", 1).endObject().endObject() + ), XContentType.JSON)).rootDoc().getField("location"), nullValue()); + + assertThat(defaultMapper.parse(new SourceToParse("test", "type", "1", + BytesReference.bytes(XContentFactory.jsonBuilder() + .startObject().startObject("location").nullField("lat").nullField("lon").endObject().endObject() + ), XContentType.JSON)).rootDoc().getField("location"), nullValue()); } } diff --git a/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java b/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java index ee916dd4c47dd..1a85e29f02090 100644 --- a/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java +++ b/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java @@ -397,6 +397,8 @@ public void testParseGeoPoint() throws IOException { parser.nextToken(); GeoPoint point = GeoUtils.parseGeoPoint(parser); assertThat(point, equalTo(new GeoPoint(lat, lon))); + assertThat(parser.currentToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); } json = jsonBuilder().startObject().field("lat", String.valueOf(lat)).field("lon", String.valueOf(lon)).endObject(); try (XContentParser parser = createParser(json)) { @@ -438,6 +440,21 @@ public void testParseGeoPointStringZValueError() throws IOException { } } + public void testParseGeoPointArrayZValueError() throws IOException { + double lat = randomDouble() * 180 - 90 + randomIntBetween(-1000, 1000) * 180; + double lon = randomDouble() * 360 - 180 + randomIntBetween(-1000, 1000) * 360; + double alt = randomDouble() * 1000; + XContentBuilder json = jsonBuilder().startArray().value(lat).value(lon).value(alt).endArray(); + try (XContentParser parser = createParser(json)) { + parser.nextToken(); + Exception e = expectThrows(ElasticsearchParseException.class, + () -> GeoUtils.parseGeoPoint(parser, new GeoPoint(), false)); + assertThat(e.getMessage(), containsString("but [ignore_z_value] parameter is [false]")); + assertThat(parser.currentToken(), is(Token.END_ARRAY)); + assertNull(parser.nextToken()); + } + } + public void testParseGeoPointGeohash() throws IOException { for (int i = 0; i < 100; i++) { int geoHashLength = randomIntBetween(1, GeoHashUtils.PRECISION); @@ -451,6 +468,8 @@ public void testParseGeoPointGeohash() throws IOException { GeoPoint point = GeoUtils.parseGeoPoint(parser); assertThat(point.lat(), allOf(lessThanOrEqualTo(90.0), greaterThanOrEqualTo(-90.0))); assertThat(point.lon(), allOf(lessThanOrEqualTo(180.0), greaterThanOrEqualTo(-180.0))); + assertThat(parser.currentToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); } json = jsonBuilder().startObject().field("geohash", geohashBuilder.toString()).endObject(); try (XContentParser parser = createParser(json)) { @@ -470,6 +489,8 @@ public void testParseGeoPointGeohashWrongType() throws IOException { parser.nextToken(); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); assertThat(e.getMessage(), containsString("geohash must be a string")); + assertThat(parser.currentToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); } } @@ -480,6 +501,8 @@ public void testParseGeoPointLatNoLon() throws IOException { parser.nextToken(); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); assertThat(e.getMessage(), is("field [lon] missing")); + assertThat(parser.currentToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); } } @@ -490,6 +513,8 @@ public void testParseGeoPointLonNoLat() throws IOException { parser.nextToken(); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); assertThat(e.getMessage(), is("field [lat] missing")); + assertThat(parser.currentToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); } } @@ -500,6 +525,8 @@ public void testParseGeoPointLonWrongType() throws IOException { parser.nextToken(); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); assertThat(e.getMessage(), is("longitude must be a number")); + assertThat(parser.currentToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); } } @@ -510,6 +537,8 @@ public void testParseGeoPointLatWrongType() throws IOException { parser.nextToken(); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); assertThat(e.getMessage(), is("latitude must be a number")); + assertThat(parser.currentToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); } } @@ -578,6 +607,9 @@ public void testParseGeoPointArrayWrongType() throws IOException { } Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); assertThat(e.getMessage(), is("numeric value expected")); + assertThat(parser.currentToken(), is(Token.END_ARRAY)); + assertThat(parser.nextToken(), is(Token.END_OBJECT)); + assertNull(parser.nextToken()); } } From e4791a9af7eaf7902a2ecd656bea767db11d4053 Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Tue, 26 Mar 2019 21:31:05 +0100 Subject: [PATCH 2/4] Geo Point parse error fix When geo point parsing threw a parse exception, it did not consume remaining tokens from the parser. This in turn meant that indexing documents with malformed geo points into mappings with ignore_malformed=true would fail in some cases, since DocumentParser expects geo_point parsing to end on the END_OBJECT token. Related to #17617 --- .../common/xcontent/XContentSubParser.java | 6 +-- .../elasticsearch/common/geo/GeoUtils.java | 46 +++++++------------ 2 files changed, 18 insertions(+), 34 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java index e02f9f176246e..5a77e67f0a672 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java @@ -39,10 +39,8 @@ public class XContentSubParser implements XContentParser { public XContentSubParser(XContentParser parser) { this.parser = parser; - if (parser.currentToken() != Token.START_OBJECT) { - throw new IllegalStateException("The sub parser has to be created on the start of an object"); - } - level = 1; + // we allow a non-object, non-array with semantics that it is the single item only. + this.level = parser.currentToken() == Token.START_OBJECT || parser.currentToken() == Token.START_ARRAY ? 1 : 0; } @Override diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java index 76fe86a03c6be..af8801d79b548 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -31,6 +31,7 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentParser.Token; +import org.elasticsearch.common.xcontent.XContentSubParser; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.common.xcontent.support.XContentMapValues; import org.elasticsearch.index.fielddata.FieldData; @@ -429,11 +430,18 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina */ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, final boolean ignoreZValue, EffectivePoint effectivePoint) throws IOException, ElasticsearchParseException { + try (XContentSubParser subParser = new XContentSubParser(parser)) { + return parseGeoPointUnsafe(subParser, point, ignoreZValue, effectivePoint); + } + } + + private static GeoPoint parseGeoPointUnsafe(XContentParser parser, GeoPoint point, final boolean ignoreZValue, + EffectivePoint effectivePoint) + throws IOException, ElasticsearchParseException { double lat = Double.NaN; double lon = Double.NaN; String geohash = null; NumberFormatException numberFormatException = null; - ParseExceptionConsumer exceptionConsumer = new ParseExceptionConsumer(); if(parser.currentToken() == Token.START_OBJECT) { while(parser.nextToken() != Token.END_OBJECT) { @@ -451,7 +459,7 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina } break; default: - exceptionConsumer.consume(new ElasticsearchParseException("latitude must be a number")); + throw new ElasticsearchParseException("latitude must be a number"); } } else if (LONGITUDE.equals(field)) { parser.nextToken(); @@ -465,25 +473,22 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina } break; default: - exceptionConsumer.consume(new ElasticsearchParseException("longitude must be a number")); + throw new ElasticsearchParseException("longitude must be a number"); } } else if (GEOHASH.equals(field)) { if(parser.nextToken() == Token.VALUE_STRING) { geohash = parser.text(); } else { - exceptionConsumer.consume(new ElasticsearchParseException("geohash must be a string")); + throw new ElasticsearchParseException("geohash must be a string"); } } else { - exceptionConsumer.consume(new ElasticsearchParseException("field must be either [{}], [{}] or [{}]", - LATITUDE, LONGITUDE, GEOHASH)); + throw new ElasticsearchParseException("field must be either [{}], [{}] or [{}]", LATITUDE, LONGITUDE, GEOHASH); } } else { - exceptionConsumer.consume(new ElasticsearchParseException("token [{}] not allowed", parser.currentToken())); + throw new ElasticsearchParseException("token [{}] not allowed", parser.currentToken()); } } - exceptionConsumer.doThrow(); - if (geohash != null) { if(!Double.isNaN(lat) || !Double.isNaN(lon)) { throw new ElasticsearchParseException("field must be either lat/lon or geohash"); @@ -511,17 +516,12 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina } else if(element == 2) { lat = parser.doubleValue(); } else { - try { - GeoPoint.assertZValue(ignoreZValue, parser.doubleValue()); - } catch (ElasticsearchParseException e) { - exceptionConsumer.consume(e); - } + GeoPoint.assertZValue(ignoreZValue, parser.doubleValue()); } } else { - exceptionConsumer.consume(new ElasticsearchParseException("numeric value expected")); + throw new ElasticsearchParseException("numeric value expected"); } } - exceptionConsumer.doThrow(); return point.reset(lat, lon); } else if(parser.currentToken() == Token.VALUE_STRING) { String val = parser.text(); @@ -704,20 +704,6 @@ public boolean advanceExact(int target) throws IOException { } } - private static class ParseExceptionConsumer { - private ElasticsearchParseException exception; - - public void consume(ElasticsearchParseException e) { - if (exception == null) - exception = e; - } - - public void doThrow() { - if (exception != null) - throw exception; - } - } - private GeoUtils() { } } From 0bbc9597f07d2ee4aaf69570a0d4608796fbd156 Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Wed, 27 Mar 2019 10:36:57 +0100 Subject: [PATCH 3/4] Geo Point parse error fix Improved XContentSubParser to allow any token, which is useful for wrapping in cases where both object and values are allowed. Related to #17617 --- .../common/xcontent/XContentSubParser.java | 53 +++++++++--- .../common/xcontent/XContentParserTests.java | 80 ++++++++++++++++--- 2 files changed, 112 insertions(+), 21 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java index 5a77e67f0a672..0e1523b8b833d 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java @@ -21,6 +21,9 @@ import java.io.IOException; import java.nio.CharBuffer; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.List; import java.util.Map; @@ -30,6 +33,9 @@ * The wrapper prevents the parsing logic to consume tokens outside of the wrapped object as well * as skipping to the end of the object in case of a parsing error. The wrapper is intended to be * used for parsing objects that should be ignored if they are malformed. + * + * The wrapper can be used on all token types. For non-start token types, the wrapper considers the + * single token as the wrapped object */ public class XContentSubParser implements XContentParser { @@ -59,6 +65,9 @@ public Token nextToken() throws IOException { } return token; } else { + if (level == 0) { + level = -1; + } return null; // we have reached the end of the wrapped object } } @@ -80,92 +89,98 @@ public void skipChildren() throws IOException { @Override public Token currentToken() { - return parser.currentToken(); + return level >= 0 ? parser.currentToken() : null; } @Override public String currentName() throws IOException { - return parser.currentName(); + return level >= 0 ? parser.currentName() : null; } @Override public Map map() throws IOException { - return parser.map(); + return level >= 0 ? parser.map() : new HashMap<>(); } @Override public Map mapOrdered() throws IOException { - return parser.mapOrdered(); + return level >= 0 ? parser.mapOrdered() : new LinkedHashMap<>(); } @Override public Map mapStrings() throws IOException { - return parser.mapStrings(); + return level >= 0 ? parser.mapStrings() : new HashMap<>(); } @Override public Map mapStringsOrdered() throws IOException { - return parser.mapStringsOrdered(); + return level >= 0 ? parser.mapStringsOrdered() : new LinkedHashMap<>(); } @Override public List list() throws IOException { - return parser.list(); + return level >= 0 ? parser.list() : new ArrayList<>(); } @Override public List listOrderedMap() throws IOException { - return parser.listOrderedMap(); + return level >= 0 ? parser.listOrderedMap() : new ArrayList<>(); } @Override public String text() throws IOException { + checkTextAccess(); return parser.text(); } @Override public String textOrNull() throws IOException { + checkTextAccess(); return parser.textOrNull(); } @Override public CharBuffer charBufferOrNull() throws IOException { + checkTextAccess(); return parser.charBufferOrNull(); } @Override public CharBuffer charBuffer() throws IOException { + checkTextAccess(); return parser.charBuffer(); } @Override public Object objectText() throws IOException { + checkTextAccess(); return parser.objectText(); } @Override public Object objectBytes() throws IOException { + checkTextAccess(); return parser.objectBytes(); } @Override public boolean hasTextCharacters() { - return parser.hasTextCharacters(); + return level >= 0 ? parser.hasTextCharacters() : false; } @Override public char[] textCharacters() throws IOException { - return parser.textCharacters(); + return level >= 0 ? parser.textCharacters() : null; } @Override public int textLength() throws IOException { - return parser.textLength(); + return level >= 0 ? parser.textLength() : 0; } @Override public int textOffset() throws IOException { - return parser.textOffset(); + return level >= 0 ? parser.textOffset() : 0; } @Override @@ -230,16 +245,19 @@ public double doubleValue() throws IOException { @Override public boolean isBooleanValue() throws IOException { + checkAccess("boolean"); return parser.isBooleanValue(); } @Override public boolean booleanValue() throws IOException { + checkAccess("boolean"); return parser.booleanValue(); } @Override public byte[] binaryValue() throws IOException { + checkAccess("binary"); return parser.binaryValue(); } @@ -250,6 +268,7 @@ public XContentLocation getTokenLocation() { @Override public T namedObject(Class categoryClass, String name, Object context) throws IOException { + checkAccess("namedObject"); return parser.namedObject(categoryClass, name, context); } @@ -279,4 +298,14 @@ public void close() throws IOException { } } } + + private void checkTextAccess() { + checkAccess("text"); + } + + private void checkAccess(String type) { + if (level < 0) { + throw new IllegalStateException("Can't get " + type + " on a " + currentToken() + " at " + getTokenLocation()); + } + } } diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java index 5dbe7be40f312..25cd2c1d22fab 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java @@ -348,7 +348,9 @@ public void testSubParser() throws IOException { int tokensToSkip = randomInt(numberOfTokens - 1); for (int i = 0; i < tokensToSkip; i++) { // Simulate incomplete parsing - assertNotNull(subParser.nextToken()); + XContentParser.Token nextToken = subParser.nextToken(); + assertNotNull(nextToken); + assertEquals(nextToken, subParser.currentToken()); } if (randomBoolean()) { // And sometimes skipping children @@ -367,21 +369,81 @@ public void testSubParser() throws IOException { } } - public void testCreateSubParserAtAWrongPlace() throws IOException { + public void testCreateSubParserOnAllFields() throws IOException { XContentBuilder builder = XContentFactory.jsonBuilder(); generateRandomObjectForMarking(builder); String content = Strings.toString(builder); - try (XContentParser parser = createParser(JsonXContent.jsonXContent, content)) { - assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); - assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // first field - assertEquals("first_field", parser.currentName()); - IllegalStateException exception = expectThrows(IllegalStateException.class, () -> new XContentSubParser(parser)); - assertEquals("The sub parser has to be created on the start of an object", exception.getMessage()); + for (XContentParser.Token testToken : XContentParser.Token.values()) { + try (XContentParser parser = createParser(JsonXContent.jsonXContent, content)) { + int openObjectCount = 0; + int openArrayCount = 0; + assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); + XContentParser.Token currentToken; + while ((currentToken = parser.currentToken()) != null) { + switch (currentToken) { + case START_OBJECT: + ++openObjectCount; + break; + case END_OBJECT: + --openObjectCount; + break; + case START_ARRAY: + ++openArrayCount; + break; + case END_ARRAY: + --openArrayCount; + break; + default: + break; + } + boolean mustAdvance = true; + if (currentToken == testToken && randomBoolean()) { + try (XContentSubParser subParser = new XContentSubParser(parser)) { + assertEquals(currentToken, subParser.currentToken()); + XContentParser.Token nextSubParserToken = subParser.nextToken(); + if (currentToken == XContentParser.Token.START_OBJECT || currentToken == XContentParser.Token.START_ARRAY) { + assertNotNull(nextSubParserToken); + if (nextSubParserToken.isValue()) { + assertNotNull(subParser.objectText()); + } else if (nextSubParserToken == XContentParser.Token.VALUE_NULL) { + assertNull(subParser.objectText()); + } else { + expectThrows(IllegalStateException.class, "Test token: " + testToken, () -> subParser.objectText()); + } + assertEquals("Test token: " + testToken, nextSubParserToken, subParser.currentToken()); + if (rarely()) { + XContentParser.Token lastSubParserToken = nextSubParserToken; + while (subParser.nextToken() != null) { + lastSubParserToken = subParser.currentToken(); + } + + assertEquals( + currentToken == XContentParser.Token.START_OBJECT + ? XContentParser.Token.END_OBJECT + : XContentParser.Token.END_ARRAY, + lastSubParserToken); + } + mustAdvance = false; + } else { + assertNull(nextSubParserToken); + expectThrows(IllegalStateException.class, "Test token: " + testToken, () -> subParser.objectText()); + assertEquals("Test token: " + testToken, nextSubParserToken, subParser.currentToken()); + } + } + } + + if (mustAdvance) { + parser.nextToken(); + } + } + + assertEquals("Test token: " + testToken, 0, openObjectCount); + assertEquals("Test token: " + testToken, 0, openArrayCount); + } } } - public void testCreateRootSubParser() throws IOException { XContentBuilder builder = XContentFactory.jsonBuilder(); int numberOfTokens = generateRandomObjectForMarking(builder); From c67c36885bc50e53b20c7fd3b26394e4664678e9 Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Thu, 28 Mar 2019 08:19:44 +0100 Subject: [PATCH 4/4] Geo Point parse error fix Reverted to a minimalistic change. Related to #17617 --- .../common/xcontent/XContentSubParser.java | 61 +++------ .../common/xcontent/XContentParserTests.java | 122 ++++++++---------- .../elasticsearch/common/geo/GeoUtils.java | 111 ++++++++-------- 3 files changed, 122 insertions(+), 172 deletions(-) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java index 0e1523b8b833d..adcbf6ef1bee0 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java @@ -21,21 +21,15 @@ import java.io.IOException; import java.nio.CharBuffer; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.LinkedHashMap; import java.util.List; import java.util.Map; /** - * Wrapper for a XContentParser that makes a single object to look like a complete document. + * Wrapper for a XContentParser that makes a single object/array look like a complete document. * * The wrapper prevents the parsing logic to consume tokens outside of the wrapped object as well * as skipping to the end of the object in case of a parsing error. The wrapper is intended to be * used for parsing objects that should be ignored if they are malformed. - * - * The wrapper can be used on all token types. For non-start token types, the wrapper considers the - * single token as the wrapped object */ public class XContentSubParser implements XContentParser { @@ -45,8 +39,10 @@ public class XContentSubParser implements XContentParser { public XContentSubParser(XContentParser parser) { this.parser = parser; - // we allow a non-object, non-array with semantics that it is the single item only. - this.level = parser.currentToken() == Token.START_OBJECT || parser.currentToken() == Token.START_ARRAY ? 1 : 0; + if (parser.currentToken() != Token.START_OBJECT && parser.currentToken() != Token.START_ARRAY) { + throw new IllegalStateException("The sub parser has to be created on the start of an object or array"); + } + level = 1; } @Override @@ -65,9 +61,6 @@ public Token nextToken() throws IOException { } return token; } else { - if (level == 0) { - level = -1; - } return null; // we have reached the end of the wrapped object } } @@ -89,98 +82,92 @@ public void skipChildren() throws IOException { @Override public Token currentToken() { - return level >= 0 ? parser.currentToken() : null; + return parser.currentToken(); } @Override public String currentName() throws IOException { - return level >= 0 ? parser.currentName() : null; + return parser.currentName(); } @Override public Map map() throws IOException { - return level >= 0 ? parser.map() : new HashMap<>(); + return parser.map(); } @Override public Map mapOrdered() throws IOException { - return level >= 0 ? parser.mapOrdered() : new LinkedHashMap<>(); + return parser.mapOrdered(); } @Override public Map mapStrings() throws IOException { - return level >= 0 ? parser.mapStrings() : new HashMap<>(); + return parser.mapStrings(); } @Override public Map mapStringsOrdered() throws IOException { - return level >= 0 ? parser.mapStringsOrdered() : new LinkedHashMap<>(); + return parser.mapStringsOrdered(); } @Override public List list() throws IOException { - return level >= 0 ? parser.list() : new ArrayList<>(); + return parser.list(); } @Override public List listOrderedMap() throws IOException { - return level >= 0 ? parser.listOrderedMap() : new ArrayList<>(); + return parser.listOrderedMap(); } @Override public String text() throws IOException { - checkTextAccess(); return parser.text(); } @Override public String textOrNull() throws IOException { - checkTextAccess(); return parser.textOrNull(); } @Override public CharBuffer charBufferOrNull() throws IOException { - checkTextAccess(); return parser.charBufferOrNull(); } @Override public CharBuffer charBuffer() throws IOException { - checkTextAccess(); return parser.charBuffer(); } @Override public Object objectText() throws IOException { - checkTextAccess(); return parser.objectText(); } @Override public Object objectBytes() throws IOException { - checkTextAccess(); return parser.objectBytes(); } @Override public boolean hasTextCharacters() { - return level >= 0 ? parser.hasTextCharacters() : false; + return parser.hasTextCharacters(); } @Override public char[] textCharacters() throws IOException { - return level >= 0 ? parser.textCharacters() : null; + return parser.textCharacters(); } @Override public int textLength() throws IOException { - return level >= 0 ? parser.textLength() : 0; + return parser.textLength(); } @Override public int textOffset() throws IOException { - return level >= 0 ? parser.textOffset() : 0; + return parser.textOffset(); } @Override @@ -245,19 +232,16 @@ public double doubleValue() throws IOException { @Override public boolean isBooleanValue() throws IOException { - checkAccess("boolean"); return parser.isBooleanValue(); } @Override public boolean booleanValue() throws IOException { - checkAccess("boolean"); return parser.booleanValue(); } @Override public byte[] binaryValue() throws IOException { - checkAccess("binary"); return parser.binaryValue(); } @@ -268,7 +252,6 @@ public XContentLocation getTokenLocation() { @Override public T namedObject(Class categoryClass, String name, Object context) throws IOException { - checkAccess("namedObject"); return parser.namedObject(categoryClass, name, context); } @@ -298,14 +281,4 @@ public void close() throws IOException { } } } - - private void checkTextAccess() { - checkAccess("text"); - } - - private void checkAccess(String type) { - if (level < 0) { - throw new IllegalStateException("Can't get " + type + " on a " + currentToken() + " at " + getTokenLocation()); - } - } } diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java index 25cd2c1d22fab..e98f1e3d58510 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java @@ -329,7 +329,7 @@ public void testNestedMapInList() throws IOException { } } - public void testSubParser() throws IOException { + public void testSubParserObject() throws IOException { XContentBuilder builder = XContentFactory.jsonBuilder(); int numberOfTokens; numberOfTokens = generateRandomObjectForMarking(builder); @@ -348,14 +348,13 @@ public void testSubParser() throws IOException { int tokensToSkip = randomInt(numberOfTokens - 1); for (int i = 0; i < tokensToSkip; i++) { // Simulate incomplete parsing - XContentParser.Token nextToken = subParser.nextToken(); - assertNotNull(nextToken); - assertEquals(nextToken, subParser.currentToken()); + assertNotNull(subParser.nextToken()); } if (randomBoolean()) { // And sometimes skipping children subParser.skipChildren(); } + } finally { assertFalse(subParser.isClosed()); subParser.close(); @@ -369,81 +368,64 @@ public void testSubParser() throws IOException { } } - public void testCreateSubParserOnAllFields() throws IOException { + public void testSubParserArray() throws IOException { XContentBuilder builder = XContentFactory.jsonBuilder(); - generateRandomObjectForMarking(builder); + int numberOfArrayElements = randomInt(10); + builder.startObject(); + builder.field("array"); + builder.startArray(); + int numberOfTokens = 0; + for (int i = 0; i < numberOfArrayElements; ++i) { + numberOfTokens += generateRandomObjectForMarking(builder); + } + builder.endArray(); + builder.endObject(); + String content = Strings.toString(builder); - for (XContentParser.Token testToken : XContentParser.Token.values()) { - try (XContentParser parser = createParser(JsonXContent.jsonXContent, content)) { - int openObjectCount = 0; - int openArrayCount = 0; - assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); - XContentParser.Token currentToken; - while ((currentToken = parser.currentToken()) != null) { - switch (currentToken) { - case START_OBJECT: - ++openObjectCount; - break; - case END_OBJECT: - --openObjectCount; - break; - case START_ARRAY: - ++openArrayCount; - break; - case END_ARRAY: - --openArrayCount; - break; - default: - break; - } - boolean mustAdvance = true; - if (currentToken == testToken && randomBoolean()) { - try (XContentSubParser subParser = new XContentSubParser(parser)) { - assertEquals(currentToken, subParser.currentToken()); - XContentParser.Token nextSubParserToken = subParser.nextToken(); - if (currentToken == XContentParser.Token.START_OBJECT || currentToken == XContentParser.Token.START_ARRAY) { - assertNotNull(nextSubParserToken); - if (nextSubParserToken.isValue()) { - assertNotNull(subParser.objectText()); - } else if (nextSubParserToken == XContentParser.Token.VALUE_NULL) { - assertNull(subParser.objectText()); - } else { - expectThrows(IllegalStateException.class, "Test token: " + testToken, () -> subParser.objectText()); - } - assertEquals("Test token: " + testToken, nextSubParserToken, subParser.currentToken()); - if (rarely()) { - XContentParser.Token lastSubParserToken = nextSubParserToken; - while (subParser.nextToken() != null) { - lastSubParserToken = subParser.currentToken(); - } - - assertEquals( - currentToken == XContentParser.Token.START_OBJECT - ? XContentParser.Token.END_OBJECT - : XContentParser.Token.END_ARRAY, - lastSubParserToken); - } - mustAdvance = false; - } else { - assertNull(nextSubParserToken); - expectThrows(IllegalStateException.class, "Test token: " + testToken, () -> subParser.objectText()); - assertEquals("Test token: " + testToken, nextSubParserToken, subParser.currentToken()); - } - } - } - - if (mustAdvance) { - parser.nextToken(); - } + try (XContentParser parser = createParser(JsonXContent.jsonXContent, content)) { + assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); + assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // array field + assertEquals("array", parser.currentName()); + assertEquals(XContentParser.Token.START_ARRAY, parser.nextToken()); // [ + XContentParser subParser = new XContentSubParser(parser); + try { + int tokensToSkip = randomInt(numberOfTokens - 1); + for (int i = 0; i < tokensToSkip; i++) { + // Simulate incomplete parsing + assertNotNull(subParser.nextToken()); + } + if (randomBoolean()) { + // And sometimes skipping children + subParser.skipChildren(); } - assertEquals("Test token: " + testToken, 0, openObjectCount); - assertEquals("Test token: " + testToken, 0, openArrayCount); + } finally { + assertFalse(subParser.isClosed()); + subParser.close(); + assertTrue(subParser.isClosed()); } + assertEquals(XContentParser.Token.END_ARRAY, parser.currentToken()); + assertEquals(XContentParser.Token.END_OBJECT, parser.nextToken()); + assertNull(parser.nextToken()); + } + } + + public void testCreateSubParserAtAWrongPlace() throws IOException { + XContentBuilder builder = XContentFactory.jsonBuilder(); + generateRandomObjectForMarking(builder); + String content = Strings.toString(builder); + + try (XContentParser parser = createParser(JsonXContent.jsonXContent, content)) { + assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken()); + assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // first field + assertEquals("first_field", parser.currentName()); + IllegalStateException exception = expectThrows(IllegalStateException.class, () -> new XContentSubParser(parser)); + assertEquals("The sub parser has to be created on the start of an object or array", exception.getMessage()); } } + public void testCreateRootSubParser() throws IOException { XContentBuilder builder = XContentFactory.jsonBuilder(); int numberOfTokens = generateRandomObjectForMarking(builder); diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java index af8801d79b548..6dcaaaa7d6a29 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -430,65 +430,58 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina */ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, final boolean ignoreZValue, EffectivePoint effectivePoint) throws IOException, ElasticsearchParseException { - try (XContentSubParser subParser = new XContentSubParser(parser)) { - return parseGeoPointUnsafe(subParser, point, ignoreZValue, effectivePoint); - } - } - - private static GeoPoint parseGeoPointUnsafe(XContentParser parser, GeoPoint point, final boolean ignoreZValue, - EffectivePoint effectivePoint) - throws IOException, ElasticsearchParseException { double lat = Double.NaN; double lon = Double.NaN; String geohash = null; NumberFormatException numberFormatException = null; if(parser.currentToken() == Token.START_OBJECT) { - while(parser.nextToken() != Token.END_OBJECT) { - if(parser.currentToken() == Token.FIELD_NAME) { - String field = parser.currentName(); - if(LATITUDE.equals(field)) { - parser.nextToken(); - switch (parser.currentToken()) { - case VALUE_NUMBER: - case VALUE_STRING: - try { - lat = parser.doubleValue(true); - } catch (NumberFormatException e) { - numberFormatException = e; - } - break; - default: - throw new ElasticsearchParseException("latitude must be a number"); - } - } else if (LONGITUDE.equals(field)) { - parser.nextToken(); - switch (parser.currentToken()) { - case VALUE_NUMBER: - case VALUE_STRING: - try { - lon = parser.doubleValue(true); - } catch (NumberFormatException e) { - numberFormatException = e; - } - break; - default: - throw new ElasticsearchParseException("longitude must be a number"); - } - } else if (GEOHASH.equals(field)) { - if(parser.nextToken() == Token.VALUE_STRING) { - geohash = parser.text(); + try (XContentSubParser subParser = new XContentSubParser(parser)) { + while (subParser.nextToken() != Token.END_OBJECT) { + if (subParser.currentToken() == Token.FIELD_NAME) { + String field = subParser.currentName(); + if (LATITUDE.equals(field)) { + subParser.nextToken(); + switch (subParser.currentToken()) { + case VALUE_NUMBER: + case VALUE_STRING: + try { + lat = subParser.doubleValue(true); + } catch (NumberFormatException e) { + numberFormatException = e; + } + break; + default: + throw new ElasticsearchParseException("latitude must be a number"); + } + } else if (LONGITUDE.equals(field)) { + subParser.nextToken(); + switch (subParser.currentToken()) { + case VALUE_NUMBER: + case VALUE_STRING: + try { + lon = subParser.doubleValue(true); + } catch (NumberFormatException e) { + numberFormatException = e; + } + break; + default: + throw new ElasticsearchParseException("longitude must be a number"); + } + } else if (GEOHASH.equals(field)) { + if (subParser.nextToken() == Token.VALUE_STRING) { + geohash = subParser.text(); + } else { + throw new ElasticsearchParseException("geohash must be a string"); + } } else { - throw new ElasticsearchParseException("geohash must be a string"); + throw new ElasticsearchParseException("field must be either [{}], [{}] or [{}]", LATITUDE, LONGITUDE, GEOHASH); } } else { - throw new ElasticsearchParseException("field must be either [{}], [{}] or [{}]", LATITUDE, LONGITUDE, GEOHASH); + throw new ElasticsearchParseException("token [{}] not allowed", subParser.currentToken()); } - } else { - throw new ElasticsearchParseException("token [{}] not allowed", parser.currentToken()); } } - if (geohash != null) { if(!Double.isNaN(lat) || !Double.isNaN(lon)) { throw new ElasticsearchParseException("field must be either lat/lon or geohash"); @@ -507,19 +500,21 @@ private static GeoPoint parseGeoPointUnsafe(XContentParser parser, GeoPoint poin } } else if(parser.currentToken() == Token.START_ARRAY) { - int element = 0; - while(parser.nextToken() != Token.END_ARRAY) { - if(parser.currentToken() == Token.VALUE_NUMBER) { - element++; - if(element == 1) { - lon = parser.doubleValue(); - } else if(element == 2) { - lat = parser.doubleValue(); + try (XContentSubParser subParser = new XContentSubParser(parser)) { + int element = 0; + while (subParser.nextToken() != Token.END_ARRAY) { + if (subParser.currentToken() == Token.VALUE_NUMBER) { + element++; + if (element == 1) { + lon = subParser.doubleValue(); + } else if (element == 2) { + lat = subParser.doubleValue(); + } else { + GeoPoint.assertZValue(ignoreZValue, subParser.doubleValue()); + } } else { - GeoPoint.assertZValue(ignoreZValue, parser.doubleValue()); + throw new ElasticsearchParseException("numeric value expected"); } - } else { - throw new ElasticsearchParseException("numeric value expected"); } } return point.reset(lat, lon);