Skip to content

Commit

Permalink
Geo Point parse error fix (#40447)
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 #17617
  • Loading branch information
@henningandersen
henningandersen committed Mar 29, 2019
1 parent a0b02ce commit 92d07e9
Show file tree
Hide file tree
Showing 5 changed files with 145 additions and 55 deletions.
6 changes: 3 additions & 3 deletions libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
}
Expand Down
48 changes: 46 additions & 2 deletions libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -354,6 +354,7 @@ public void testSubParser() throws IOException {
// And sometimes skipping children
subParser.skipChildren();
}

} finally {
assertFalse(subParser.isClosed());
subParser.close();
Expand All @@ -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);
Expand All @@ -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());
}
}

Expand Down
104 changes: 54 additions & 50 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 @@ -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;
Expand Down Expand Up @@ -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");
Expand All @@ -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);
Expand Down
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 92d07e9

Please sign in to comment.