Skip to content

Commit

Permalink
Geo Point parse error fix
Browse files Browse the repository at this point in the history 
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 elastic#17617
  • Loading branch information
@henningandersen
henningandersen committed Mar 26, 2019
1 parent d5015c1 commit fd192dc
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 7 deletions.
37 changes: 30 additions & 7 deletions server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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();
Expand All @@ -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");
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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() {
}
}
10 changes: 10 additions & 0 deletions server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
32 changes: 32 additions & 0 deletions server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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);
Expand All @@ -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)) {
Expand All @@ -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());
}
}

Expand All @@ -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());
}
}

Expand All @@ -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());
}
}

Expand All @@ -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());
}
}

Expand All @@ -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());
}
}

Expand Down Expand Up @@ -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());
}
}

Expand Down

0 comments on commit fd192dc

Please sign in to comment.