Skip to content

Commit 9791320

Browse files
henningandersenhenningandersen
committed
Geo Point parse error fix (#40447)
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
1 parent 8f57be7 commit 9791320

File tree

5 files changed

+145
-55
lines changed

5 files changed

+145
-55
lines changed

‎libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
import java.util.Map;
2626

2727
/**
28-
* Wrapper for a XContentParser that makes a single object to look like a complete document.
28+
* Wrapper for a XContentParser that makes a single object/array look like a complete document.
2929
*
3030
* The wrapper prevents the parsing logic to consume tokens outside of the wrapped object as well
3131
* 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 {
3939

4040
public XContentSubParser(XContentParser parser) {
4141
this.parser = parser;
42-
if (parser.currentToken() != Token.START_OBJECT) {
43-
throw new IllegalStateException("The sub parser has to be created on the start of an object");
42+
if (parser.currentToken() != Token.START_OBJECT && parser.currentToken() != Token.START_ARRAY) {
43+
throw new IllegalStateException("The sub parser has to be created on the start of an object or array");
4444
}
4545
level = 1;
4646
}

‎libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java‎

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,7 @@ public void testNestedMapInList() throws IOException {
358358
}
359359
}
360360

361-
public void testSubParser() throws IOException {
361+
public void testSubParserObject() throws IOException {
362362
XContentBuilder builder = XContentFactory.jsonBuilder();
363363
int numberOfTokens;
364364
numberOfTokens = generateRandomObjectForMarking(builder);
@@ -383,6 +383,7 @@ public void testSubParser() throws IOException {
383383
// And sometimes skipping children
384384
subParser.skipChildren();
385385
}
386+
386387
} finally {
387388
assertFalse(subParser.isClosed());
388389
subParser.close();
@@ -396,6 +397,49 @@ public void testSubParser() throws IOException {
396397
}
397398
}
398399

400+
public void testSubParserArray() throws IOException {
401+
XContentBuilder builder = XContentFactory.jsonBuilder();
402+
int numberOfArrayElements = randomInt(10);
403+
builder.startObject();
404+
builder.field("array");
405+
builder.startArray();
406+
int numberOfTokens = 0;
407+
for (int i = 0; i < numberOfArrayElements; ++i) {
408+
numberOfTokens += generateRandomObjectForMarking(builder);
409+
}
410+
builder.endArray();
411+
builder.endObject();
412+
413+
String content = Strings.toString(builder);
414+
415+
try (XContentParser parser = createParser(JsonXContent.jsonXContent, content)) {
416+
assertEquals(XContentParser.Token.START_OBJECT, parser.nextToken());
417+
assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // array field
418+
assertEquals("array", parser.currentName());
419+
assertEquals(XContentParser.Token.START_ARRAY, parser.nextToken()); // [
420+
XContentParser subParser = new XContentSubParser(parser);
421+
try {
422+
int tokensToSkip = randomInt(numberOfTokens - 1);
423+
for (int i = 0; i < tokensToSkip; i++) {
424+
// Simulate incomplete parsing
425+
assertNotNull(subParser.nextToken());
426+
}
427+
if (randomBoolean()) {
428+
// And sometimes skipping children
429+
subParser.skipChildren();
430+
}
431+
432+
} finally {
433+
assertFalse(subParser.isClosed());
434+
subParser.close();
435+
assertTrue(subParser.isClosed());
436+
}
437+
assertEquals(XContentParser.Token.END_ARRAY, parser.currentToken());
438+
assertEquals(XContentParser.Token.END_OBJECT, parser.nextToken());
439+
assertNull(parser.nextToken());
440+
}
441+
}
442+
399443
public void testCreateSubParserAtAWrongPlace() throws IOException {
400444
XContentBuilder builder = XContentFactory.jsonBuilder();
401445
generateRandomObjectForMarking(builder);
@@ -406,7 +450,7 @@ public void testCreateSubParserAtAWrongPlace() throws IOException {
406450
assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // first field
407451
assertEquals("first_field", parser.currentName());
408452
IllegalStateException exception = expectThrows(IllegalStateException.class, () -> new XContentSubParser(parser));
409-
assertEquals("The sub parser has to be created on the start of an object", exception.getMessage());
453+
assertEquals("The sub parser has to be created on the start of an object or array", exception.getMessage());
410454
}
411455
}
412456

‎server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java‎

Lines changed: 54 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import org.elasticsearch.common.xcontent.XContentBuilder;
3232
import org.elasticsearch.common.xcontent.XContentParser;
3333
import org.elasticsearch.common.xcontent.XContentParser.Token;
34+
import org.elasticsearch.common.xcontent.XContentSubParser;
3435
import org.elasticsearch.common.xcontent.json.JsonXContent;
3536
import org.elasticsearch.common.xcontent.support.XContentMapValues;
3637
import org.elasticsearch.index.fielddata.FieldData;
@@ -416,51 +417,52 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina
416417
NumberFormatException numberFormatException = null;
417418

418419
if(parser.currentToken() == Token.START_OBJECT) {
419-
while(parser.nextToken() != Token.END_OBJECT) {
420-
if(parser.currentToken() == Token.FIELD_NAME) {
421-
String field = parser.currentName();
422-
if(LATITUDE.equals(field)) {
423-
parser.nextToken();
424-
switch (parser.currentToken()) {
425-
case VALUE_NUMBER:
426-
case VALUE_STRING:
427-
try {
428-
lat = parser.doubleValue(true);
429-
} catch (NumberFormatException e) {
430-
numberFormatException = e;
431-
}
432-
break;
433-
default:
434-
throw new ElasticsearchParseException("latitude must be a number");
435-
}
436-
} else if (LONGITUDE.equals(field)) {
437-
parser.nextToken();
438-
switch (parser.currentToken()) {
439-
case VALUE_NUMBER:
440-
case VALUE_STRING:
441-
try {
442-
lon = parser.doubleValue(true);
443-
} catch (NumberFormatException e) {
444-
numberFormatException = e;
445-
}
446-
break;
447-
default:
448-
throw new ElasticsearchParseException("longitude must be a number");
449-
}
450-
} else if (GEOHASH.equals(field)) {
451-
if(parser.nextToken() == Token.VALUE_STRING) {
452-
geohash = parser.text();
420+
try (XContentSubParser subParser = new XContentSubParser(parser)) {
421+
while (subParser.nextToken() != Token.END_OBJECT) {
422+
if (subParser.currentToken() == Token.FIELD_NAME) {
423+
String field = subParser.currentName();
424+
if (LATITUDE.equals(field)) {
425+
subParser.nextToken();
426+
switch (subParser.currentToken()) {
427+
case VALUE_NUMBER:
428+
case VALUE_STRING:
429+
try {
430+
lat = subParser.doubleValue(true);
431+
} catch (NumberFormatException e) {
432+
numberFormatException = e;
433+
}
434+
break;
435+
default:
436+
throw new ElasticsearchParseException("latitude must be a number");
437+
}
438+
} else if (LONGITUDE.equals(field)) {
439+
subParser.nextToken();
440+
switch (subParser.currentToken()) {
441+
case VALUE_NUMBER:
442+
case VALUE_STRING:
443+
try {
444+
lon = subParser.doubleValue(true);
445+
} catch (NumberFormatException e) {
446+
numberFormatException = e;
447+
}
448+
break;
449+
default:
450+
throw new ElasticsearchParseException("longitude must be a number");
451+
}
452+
} else if (GEOHASH.equals(field)) {
453+
if (subParser.nextToken() == Token.VALUE_STRING) {
454+
geohash = subParser.text();
455+
} else {
456+
throw new ElasticsearchParseException("geohash must be a string");
457+
}
453458
} else {
454-
throw new ElasticsearchParseException("geohash must be a string");
459+
throw new ElasticsearchParseException("field must be either [{}], [{}] or [{}]", LATITUDE, LONGITUDE, GEOHASH);
455460
}
456461
} else {
457-
throw new ElasticsearchParseException("field must be either [{}], [{}] or [{}]", LATITUDE, LONGITUDE, GEOHASH);
462+
throw new ElasticsearchParseException("token [{}] not allowed", subParser.currentToken());
458463
}
459-
} else {
460-
throw new ElasticsearchParseException("token [{}] not allowed", parser.currentToken());
461464
}
462465
}
463-
464466
if (geohash != null) {
465467
if(!Double.isNaN(lat) || !Double.isNaN(lon)) {
466468
throw new ElasticsearchParseException("field must be either lat/lon or geohash");
@@ -479,19 +481,21 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina
479481
}
480482

481483
} else if(parser.currentToken() == Token.START_ARRAY) {
482-
int element = 0;
483-
while(parser.nextToken() != Token.END_ARRAY) {
484-
if(parser.currentToken() == Token.VALUE_NUMBER) {
485-
element++;
486-
if(element == 1) {
487-
lon = parser.doubleValue();
488-
} else if(element == 2) {
489-
lat = parser.doubleValue();
484+
try (XContentSubParser subParser = new XContentSubParser(parser)) {
485+
int element = 0;
486+
while (subParser.nextToken() != Token.END_ARRAY) {
487+
if (subParser.currentToken() == Token.VALUE_NUMBER) {
488+
element++;
489+
if (element == 1) {
490+
lon = subParser.doubleValue();
491+
} else if (element == 2) {
492+
lat = subParser.doubleValue();
493+
} else {
494+
GeoPoint.assertZValue(ignoreZValue, subParser.doubleValue());
495+
}
490496
} else {
491-
GeoPoint.assertZValue(ignoreZValue, parser.doubleValue());
497+
throw new ElasticsearchParseException("numeric value expected");
492498
}
493-
} else {
494-
throw new ElasticsearchParseException("numeric value expected");
495499
}
496500
}
497501
return point.reset(lat, lon);

‎server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -523,5 +523,15 @@ public void testInvalidGeopointValuesIgnored() throws Exception {
523523
BytesReference.bytes(XContentFactory.jsonBuilder()
524524
.startObject().field("location", "NaN,12").endObject()
525525
), XContentType.JSON)).rootDoc().getField("location"), nullValue());
526+
527+
assertThat(defaultMapper.parse(new SourceToParse("test", "type", "1",
528+
BytesReference.bytes(XContentFactory.jsonBuilder()
529+
.startObject().startObject("location").nullField("lat").field("lon", 1).endObject().endObject()
530+
), XContentType.JSON)).rootDoc().getField("location"), nullValue());
531+
532+
assertThat(defaultMapper.parse(new SourceToParse("test", "type", "1",
533+
BytesReference.bytes(XContentFactory.jsonBuilder()
534+
.startObject().startObject("location").nullField("lat").nullField("lon").endObject().endObject()
535+
), XContentType.JSON)).rootDoc().getField("location"), nullValue());
526536
}
527537
}

‎server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -397,6 +397,8 @@ public void testParseGeoPoint() throws IOException {
397397
parser.nextToken();
398398
GeoPoint point = GeoUtils.parseGeoPoint(parser);
399399
assertThat(point, equalTo(new GeoPoint(lat, lon)));
400+
assertThat(parser.currentToken(), is(Token.END_OBJECT));
401+
assertNull(parser.nextToken());
400402
json = jsonBuilder().startObject().field("lat", String.valueOf(lat)).field("lon", String.valueOf(lon)).endObject();
401403
parser = createParser(json);
402404
parser.nextToken();
@@ -432,6 +434,21 @@ public void testParseGeoPointStringZValueError() throws IOException {
432434
assertThat(e.getMessage(), containsString("but [ignore_z_value] parameter is [false]"));
433435
}
434436

437+
public void testParseGeoPointArrayZValueError() throws IOException {
438+
double lat = randomDouble() * 180 - 90 + randomIntBetween(-1000, 1000) * 180;
439+
double lon = randomDouble() * 360 - 180 + randomIntBetween(-1000, 1000) * 360;
440+
double alt = randomDouble() * 1000;
441+
XContentBuilder json = jsonBuilder().startArray().value(lat).value(lon).value(alt).endArray();
442+
try (XContentParser parser = createParser(json)) {
443+
parser.nextToken();
444+
Exception e = expectThrows(ElasticsearchParseException.class,
445+
() -> GeoUtils.parseGeoPoint(parser, new GeoPoint(), false));
446+
assertThat(e.getMessage(), containsString("but [ignore_z_value] parameter is [false]"));
447+
assertThat(parser.currentToken(), is(Token.END_ARRAY));
448+
assertNull(parser.nextToken());
449+
}
450+
}
451+
435452
public void testParseGeoPointGeohash() throws IOException {
436453
for (int i = 0; i < 100; i++) {
437454
int geoHashLength = randomIntBetween(1, GeoHashUtils.PRECISION);
@@ -445,6 +462,8 @@ public void testParseGeoPointGeohash() throws IOException {
445462
GeoPoint point = GeoUtils.parseGeoPoint(parser);
446463
assertThat(point.lat(), allOf(lessThanOrEqualTo(90.0), greaterThanOrEqualTo(-90.0)));
447464
assertThat(point.lon(), allOf(lessThanOrEqualTo(180.0), greaterThanOrEqualTo(-180.0)));
465+
assertThat(parser.currentToken(), is(Token.END_OBJECT));
466+
assertNull(parser.nextToken());
448467
json = jsonBuilder().startObject().field("geohash", geohashBuilder.toString()).endObject();
449468
parser = createParser(json);
450469
while (parser.currentToken() != Token.VALUE_STRING) {
@@ -462,6 +481,8 @@ public void testParseGeoPointGeohashWrongType() throws IOException {
462481
parser.nextToken();
463482
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
464483
assertThat(e.getMessage(), containsString("geohash must be a string"));
484+
assertThat(parser.currentToken(), is(Token.END_OBJECT));
485+
assertNull(parser.nextToken());
465486
}
466487

467488
public void testParseGeoPointLatNoLon() throws IOException {
@@ -471,6 +492,8 @@ public void testParseGeoPointLatNoLon() throws IOException {
471492
parser.nextToken();
472493
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
473494
assertThat(e.getMessage(), is("field [lon] missing"));
495+
assertThat(parser.currentToken(), is(Token.END_OBJECT));
496+
assertNull(parser.nextToken());
474497
}
475498

476499
public void testParseGeoPointLonNoLat() throws IOException {
@@ -480,6 +503,8 @@ public void testParseGeoPointLonNoLat() throws IOException {
480503
parser.nextToken();
481504
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
482505
assertThat(e.getMessage(), is("field [lat] missing"));
506+
assertThat(parser.currentToken(), is(Token.END_OBJECT));
507+
assertNull(parser.nextToken());
483508
}
484509

485510
public void testParseGeoPointLonWrongType() throws IOException {
@@ -489,6 +514,8 @@ public void testParseGeoPointLonWrongType() throws IOException {
489514
parser.nextToken();
490515
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
491516
assertThat(e.getMessage(), is("longitude must be a number"));
517+
assertThat(parser.currentToken(), is(Token.END_OBJECT));
518+
assertNull(parser.nextToken());
492519
}
493520

494521
public void testParseGeoPointLatWrongType() throws IOException {
@@ -498,6 +525,8 @@ public void testParseGeoPointLatWrongType() throws IOException {
498525
parser.nextToken();
499526
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
500527
assertThat(e.getMessage(), is("latitude must be a number"));
528+
assertThat(parser.currentToken(), is(Token.END_OBJECT));
529+
assertNull(parser.nextToken());
501530
}
502531

503532
public void testParseGeoPointExtraField() throws IOException {
@@ -558,6 +587,9 @@ public void testParseGeoPointArrayWrongType() throws IOException {
558587
}
559588
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
560589
assertThat(e.getMessage(), is("numeric value expected"));
590+
assertThat(parser.currentToken(), is(Token.END_ARRAY));
591+
assertThat(parser.nextToken(), is(Token.END_OBJECT));
592+
assertNull(parser.nextToken());
561593
}
562594

563595
public void testParseGeoPointInvalidType() throws IOException {

0 commit comments

Comments
 (0)