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..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 @@ -25,7 +25,7 @@ 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 @@ -39,8 +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"); + 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; } 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..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); @@ -354,6 +354,7 @@ public void testSubParser() throws IOException { // And sometimes skipping children subParser.skipChildren(); } + } finally { assertFalse(subParser.isClosed()); subParser.close(); @@ -367,6 +368,49 @@ public void testSubParser() throws IOException { } } + public void testSubParserArray() throws IOException { + XContentBuilder builder = XContentFactory.jsonBuilder(); + 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); + + 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(); + } + + } 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); @@ -377,7 +421,7 @@ public void testCreateSubParserAtAWrongPlace() throws IOException { 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()); + assertEquals("The sub parser has to be created on the start of an object or array", exception.getMessage()); } } 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..6dcaaaa7d6a29 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; @@ -435,51 +436,52 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina 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"); @@ -498,19 +500,21 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina } } 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); 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()); } }