Skip to content

Commit fd192dc

Browse files
henningandersenhenningandersen
committed
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
1 parent d5015c1 commit fd192dc

File tree

3 files changed

+72
-7
lines changed

3 files changed

+72
-7
lines changed

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

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -433,6 +433,7 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina
433433
double lon = Double.NaN;
434434
String geohash = null;
435435
NumberFormatException numberFormatException = null;
436+
ParseExceptionConsumer exceptionConsumer = new ParseExceptionConsumer();
436437

437438
if(parser.currentToken() == Token.START_OBJECT) {
438439
while(parser.nextToken() != Token.END_OBJECT) {
@@ -450,7 +451,7 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina
450451
}
451452
break;
452453
default:
453-
throw new ElasticsearchParseException("latitude must be a number");
454+
exceptionConsumer.consume(new ElasticsearchParseException("latitude must be a number"));
454455
}
455456
} else if (LONGITUDE.equals(field)) {
456457
parser.nextToken();
@@ -464,22 +465,25 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina
464465
}
465466
break;
466467
default:
467-
throw new ElasticsearchParseException("longitude must be a number");
468+
exceptionConsumer.consume(new ElasticsearchParseException("longitude must be a number"));
468469
}
469470
} else if (GEOHASH.equals(field)) {
470471
if(parser.nextToken() == Token.VALUE_STRING) {
471472
geohash = parser.text();
472473
} else {
473-
throw new ElasticsearchParseException("geohash must be a string");
474+
exceptionConsumer.consume(new ElasticsearchParseException("geohash must be a string"));
474475
}
475476
} else {
476-
throw new ElasticsearchParseException("field must be either [{}], [{}] or [{}]", LATITUDE, LONGITUDE, GEOHASH);
477+
exceptionConsumer.consume(new ElasticsearchParseException("field must be either [{}], [{}] or [{}]",
478+
LATITUDE, LONGITUDE, GEOHASH));
477479
}
478480
} else {
479-
throw new ElasticsearchParseException("token [{}] not allowed", parser.currentToken());
481+
exceptionConsumer.consume(new ElasticsearchParseException("token [{}] not allowed", parser.currentToken()));
480482
}
481483
}
482484

485+
exceptionConsumer.doThrow();
486+
483487
if (geohash != null) {
484488
if(!Double.isNaN(lat) || !Double.isNaN(lon)) {
485489
throw new ElasticsearchParseException("field must be either lat/lon or geohash");
@@ -507,12 +511,17 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina
507511
} else if(element == 2) {
508512
lat = parser.doubleValue();
509513
} else {
510-
GeoPoint.assertZValue(ignoreZValue, parser.doubleValue());
514+
try {
515+
GeoPoint.assertZValue(ignoreZValue, parser.doubleValue());
516+
} catch (ElasticsearchParseException e) {
517+
exceptionConsumer.consume(e);
518+
}
511519
}
512520
} else {
513-
throw new ElasticsearchParseException("numeric value expected");
521+
exceptionConsumer.consume(new ElasticsearchParseException("numeric value expected"));
514522
}
515523
}
524+
exceptionConsumer.doThrow();
516525
return point.reset(lat, lon);
517526
} else if(parser.currentToken() == Token.VALUE_STRING) {
518527
String val = parser.text();
@@ -695,6 +704,20 @@ public boolean advanceExact(int target) throws IOException {
695704
}
696705
}
697706

707+
private static class ParseExceptionConsumer {
708+
private ElasticsearchParseException exception;
709+
710+
public void consume(ElasticsearchParseException e) {
711+
if (exception == null)
712+
exception = e;
713+
}
714+
715+
public void doThrow() {
716+
if (exception != null)
717+
throw exception;
718+
}
719+
}
720+
698721
private GeoUtils() {
699722
}
700723
}

‎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
}
401403
json = jsonBuilder().startObject().field("lat", String.valueOf(lat)).field("lon", String.valueOf(lon)).endObject();
402404
try (XContentParser parser = createParser(json)) {
@@ -438,6 +440,21 @@ public void testParseGeoPointStringZValueError() throws IOException {
438440
}
439441
}
440442

443+
public void testParseGeoPointArrayZValueError() throws IOException {
444+
double lat = randomDouble() * 180 - 90 + randomIntBetween(-1000, 1000) * 180;
445+
double lon = randomDouble() * 360 - 180 + randomIntBetween(-1000, 1000) * 360;
446+
double alt = randomDouble() * 1000;
447+
XContentBuilder json = jsonBuilder().startArray().value(lat).value(lon).value(alt).endArray();
448+
try (XContentParser parser = createParser(json)) {
449+
parser.nextToken();
450+
Exception e = expectThrows(ElasticsearchParseException.class,
451+
() -> GeoUtils.parseGeoPoint(parser, new GeoPoint(), false));
452+
assertThat(e.getMessage(), containsString("but [ignore_z_value] parameter is [false]"));
453+
assertThat(parser.currentToken(), is(Token.END_ARRAY));
454+
assertNull(parser.nextToken());
455+
}
456+
}
457+
441458
public void testParseGeoPointGeohash() throws IOException {
442459
for (int i = 0; i < 100; i++) {
443460
int geoHashLength = randomIntBetween(1, GeoHashUtils.PRECISION);
@@ -451,6 +468,8 @@ public void testParseGeoPointGeohash() throws IOException {
451468
GeoPoint point = GeoUtils.parseGeoPoint(parser);
452469
assertThat(point.lat(), allOf(lessThanOrEqualTo(90.0), greaterThanOrEqualTo(-90.0)));
453470
assertThat(point.lon(), allOf(lessThanOrEqualTo(180.0), greaterThanOrEqualTo(-180.0)));
471+
assertThat(parser.currentToken(), is(Token.END_OBJECT));
472+
assertNull(parser.nextToken());
454473
}
455474
json = jsonBuilder().startObject().field("geohash", geohashBuilder.toString()).endObject();
456475
try (XContentParser parser = createParser(json)) {
@@ -470,6 +489,8 @@ public void testParseGeoPointGeohashWrongType() throws IOException {
470489
parser.nextToken();
471490
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
472491
assertThat(e.getMessage(), containsString("geohash must be a string"));
492+
assertThat(parser.currentToken(), is(Token.END_OBJECT));
493+
assertNull(parser.nextToken());
473494
}
474495
}
475496

@@ -480,6 +501,8 @@ public void testParseGeoPointLatNoLon() throws IOException {
480501
parser.nextToken();
481502
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
482503
assertThat(e.getMessage(), is("field [lon] missing"));
504+
assertThat(parser.currentToken(), is(Token.END_OBJECT));
505+
assertNull(parser.nextToken());
483506
}
484507
}
485508

@@ -490,6 +513,8 @@ public void testParseGeoPointLonNoLat() throws IOException {
490513
parser.nextToken();
491514
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
492515
assertThat(e.getMessage(), is("field [lat] missing"));
516+
assertThat(parser.currentToken(), is(Token.END_OBJECT));
517+
assertNull(parser.nextToken());
493518
}
494519
}
495520

@@ -500,6 +525,8 @@ public void testParseGeoPointLonWrongType() throws IOException {
500525
parser.nextToken();
501526
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
502527
assertThat(e.getMessage(), is("longitude must be a number"));
528+
assertThat(parser.currentToken(), is(Token.END_OBJECT));
529+
assertNull(parser.nextToken());
503530
}
504531
}
505532

@@ -510,6 +537,8 @@ public void testParseGeoPointLatWrongType() throws IOException {
510537
parser.nextToken();
511538
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
512539
assertThat(e.getMessage(), is("latitude must be a number"));
540+
assertThat(parser.currentToken(), is(Token.END_OBJECT));
541+
assertNull(parser.nextToken());
513542
}
514543
}
515544

@@ -578,6 +607,9 @@ public void testParseGeoPointArrayWrongType() throws IOException {
578607
}
579608
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
580609
assertThat(e.getMessage(), is("numeric value expected"));
610+
assertThat(parser.currentToken(), is(Token.END_ARRAY));
611+
assertThat(parser.nextToken(), is(Token.END_OBJECT));
612+
assertNull(parser.nextToken());
581613
}
582614
}
583615

0 commit comments

Comments
 (0)