honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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
This commit is contained in:
Henning Andersen 2019-03-28 18:55:31 +01:00 committed by Henning Andersen
parent a0b02ce6ef
commit 92d07e9377
5 changed files with 145 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentSubParser.java
View File

@ -25,7 +25,7 @@ import java.util.List;
import java.util.Map; 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 * 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 * 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) { public XContentSubParser(XContentParser parser) {
this.parser = parser; this.parser = parser;
if (parser.currentToken() != Token.START_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"); throw new IllegalStateException("The sub parser has to be created on the start of an object or array");
} }
level = 1; level = 1;
} }

48
libs/x-content/src/test/java/org/elasticsearch/common/xcontent/XContentParserTests.java
View File

@ -329,7 +329,7 @@ public class XContentParserTests extends ESTestCase {
} }
} }
public void testSubParser() throws IOException { public void testSubParserObject() throws IOException {
XContentBuilder builder = XContentFactory.jsonBuilder(); XContentBuilder builder = XContentFactory.jsonBuilder();
int numberOfTokens; int numberOfTokens;
numberOfTokens = generateRandomObjectForMarking(builder); numberOfTokens = generateRandomObjectForMarking(builder);
@ -354,6 +354,7 @@ public class XContentParserTests extends ESTestCase {
// And sometimes skipping children // And sometimes skipping children
subParser.skipChildren(); subParser.skipChildren();
} }
} finally { } finally {
assertFalse(subParser.isClosed()); assertFalse(subParser.isClosed());
subParser.close(); subParser.close();
@ -367,6 +368,49 @@ public class XContentParserTests extends ESTestCase {
} }
} }
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 { public void testCreateSubParserAtAWrongPlace() throws IOException {
XContentBuilder builder = XContentFactory.jsonBuilder(); XContentBuilder builder = XContentFactory.jsonBuilder();
generateRandomObjectForMarking(builder); generateRandomObjectForMarking(builder);
@ -377,7 +421,7 @@ public class XContentParserTests extends ESTestCase {
assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // first field assertEquals(XContentParser.Token.FIELD_NAME, parser.nextToken()); // first field
assertEquals("first_field", parser.currentName()); assertEquals("first_field", parser.currentName());
IllegalStateException exception = expectThrows(IllegalStateException.class, () -> new XContentSubParser(parser)); 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());
} }
} }

104
server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java
View File

@ -31,6 +31,7 @@ import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentParser.Token; 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.json.JsonXContent;
import org.elasticsearch.common.xcontent.support.XContentMapValues; import org.elasticsearch.common.xcontent.support.XContentMapValues;
import org.elasticsearch.index.fielddata.FieldData; import org.elasticsearch.index.fielddata.FieldData;
@ -435,51 +436,52 @@ public class GeoUtils {
NumberFormatException numberFormatException = null; NumberFormatException numberFormatException = null;
if(parser.currentToken() == Token.START_OBJECT) { if(parser.currentToken() == Token.START_OBJECT) {
while(parser.nextToken() != Token.END_OBJECT) { try (XContentSubParser subParser = new XContentSubParser(parser)) {
if(parser.currentToken() == Token.FIELD_NAME) { while (subParser.nextToken() != Token.END_OBJECT) {
String field = parser.currentName(); if (subParser.currentToken() == Token.FIELD_NAME) {
if(LATITUDE.equals(field)) { String field = subParser.currentName();
parser.nextToken(); if (LATITUDE.equals(field)) {
switch (parser.currentToken()) { subParser.nextToken();
case VALUE_NUMBER: switch (subParser.currentToken()) {
case VALUE_STRING: case VALUE_NUMBER:
try { case VALUE_STRING:
lat = parser.doubleValue(true); try {
} catch (NumberFormatException e) { lat = subParser.doubleValue(true);
numberFormatException = e; } catch (NumberFormatException e) {
} numberFormatException = e;
break; }
default: break;
throw new ElasticsearchParseException("latitude must be a number"); default:
} throw new ElasticsearchParseException("latitude must be a number");
} else if (LONGITUDE.equals(field)) { }
parser.nextToken(); } else if (LONGITUDE.equals(field)) {
switch (parser.currentToken()) { subParser.nextToken();
case VALUE_NUMBER: switch (subParser.currentToken()) {
case VALUE_STRING: case VALUE_NUMBER:
try { case VALUE_STRING:
lon = parser.doubleValue(true); try {
} catch (NumberFormatException e) { lon = subParser.doubleValue(true);
numberFormatException = e; } catch (NumberFormatException e) {
} numberFormatException = e;
break; }
default: break;
throw new ElasticsearchParseException("longitude must be a number"); default:
} throw new ElasticsearchParseException("longitude must be a number");
} else if (GEOHASH.equals(field)) { }
if(parser.nextToken() == Token.VALUE_STRING) { } else if (GEOHASH.equals(field)) {
geohash = parser.text(); if (subParser.nextToken() == Token.VALUE_STRING) {
geohash = subParser.text();
} else {
throw new ElasticsearchParseException("geohash must be a string");
}
} else { } else {
throw new ElasticsearchParseException("geohash must be a string"); throw new ElasticsearchParseException("field must be either [{}], [{}] or [{}]", LATITUDE, LONGITUDE, GEOHASH);
} }
} else { } 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 (geohash != null) {
if(!Double.isNaN(lat) || !Double.isNaN(lon)) { if(!Double.isNaN(lat) || !Double.isNaN(lon)) {
throw new ElasticsearchParseException("field must be either lat/lon or geohash"); throw new ElasticsearchParseException("field must be either lat/lon or geohash");
@ -498,19 +500,21 @@ public class GeoUtils {
} }
} else if(parser.currentToken() == Token.START_ARRAY) { } else if(parser.currentToken() == Token.START_ARRAY) {
int element = 0; try (XContentSubParser subParser = new XContentSubParser(parser)) {
while(parser.nextToken() != Token.END_ARRAY) { int element = 0;
if(parser.currentToken() == Token.VALUE_NUMBER) { while (subParser.nextToken() != Token.END_ARRAY) {
element++; if (subParser.currentToken() == Token.VALUE_NUMBER) {
if(element == 1) { element++;
lon = parser.doubleValue(); if (element == 1) {
} else if(element == 2) { lon = subParser.doubleValue();
lat = parser.doubleValue(); } else if (element == 2) {
lat = subParser.doubleValue();
} else {
GeoPoint.assertZValue(ignoreZValue, subParser.doubleValue());
}
} else { } else {
GeoPoint.assertZValue(ignoreZValue, parser.doubleValue()); throw new ElasticsearchParseException("numeric value expected");
} }
} else {
throw new ElasticsearchParseException("numeric value expected");
} }
} }
return point.reset(lat, lon); return point.reset(lat, lon);

10
server/src/test/java/org/elasticsearch/index/mapper/GeoPointFieldMapperTests.java
View File

@ -523,5 +523,15 @@ public class GeoPointFieldMapperTests extends ESSingleNodeTestCase {
BytesReference.bytes(XContentFactory.jsonBuilder() BytesReference.bytes(XContentFactory.jsonBuilder()
.startObject().field("location", "NaN,12").endObject() .startObject().field("location", "NaN,12").endObject()
), XContentType.JSON)).rootDoc().getField("location"), nullValue()); ), 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
server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java
View File

@ -397,6 +397,8 @@ public class GeoUtilsTests extends ESTestCase {
parser.nextToken(); parser.nextToken();
GeoPoint point = GeoUtils.parseGeoPoint(parser); GeoPoint point = GeoUtils.parseGeoPoint(parser);
assertThat(point, equalTo(new GeoPoint(lat, lon))); 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(); json = jsonBuilder().startObject().field("lat", String.valueOf(lat)).field("lon", String.valueOf(lon)).endObject();
try (XContentParser parser = createParser(json)) { try (XContentParser parser = createParser(json)) {
@ -438,6 +440,21 @@ public class GeoUtilsTests extends ESTestCase {
} }
} }
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 { public void testParseGeoPointGeohash() throws IOException {
for (int i = 0; i < 100; i++) { for (int i = 0; i < 100; i++) {
int geoHashLength = randomIntBetween(1, GeoHashUtils.PRECISION); int geoHashLength = randomIntBetween(1, GeoHashUtils.PRECISION);
@ -451,6 +468,8 @@ public class GeoUtilsTests extends ESTestCase {
GeoPoint point = GeoUtils.parseGeoPoint(parser); GeoPoint point = GeoUtils.parseGeoPoint(parser);
assertThat(point.lat(), allOf(lessThanOrEqualTo(90.0), greaterThanOrEqualTo(-90.0))); assertThat(point.lat(), allOf(lessThanOrEqualTo(90.0), greaterThanOrEqualTo(-90.0)));
assertThat(point.lon(), allOf(lessThanOrEqualTo(180.0), greaterThanOrEqualTo(-180.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(); json = jsonBuilder().startObject().field("geohash", geohashBuilder.toString()).endObject();
try (XContentParser parser = createParser(json)) { try (XContentParser parser = createParser(json)) {
@ -470,6 +489,8 @@ public class GeoUtilsTests extends ESTestCase {
parser.nextToken(); parser.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
assertThat(e.getMessage(), containsString("geohash must be a string")); assertThat(e.getMessage(), containsString("geohash must be a string"));
assertThat(parser.currentToken(), is(Token.END_OBJECT));
assertNull(parser.nextToken());
} }
} }
@ -480,6 +501,8 @@ public class GeoUtilsTests extends ESTestCase {
parser.nextToken(); parser.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
assertThat(e.getMessage(), is("field [lon] missing")); assertThat(e.getMessage(), is("field [lon] missing"));
assertThat(parser.currentToken(), is(Token.END_OBJECT));
assertNull(parser.nextToken());
} }
} }
@ -490,6 +513,8 @@ public class GeoUtilsTests extends ESTestCase {
parser.nextToken(); parser.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
assertThat(e.getMessage(), is("field [lat] missing")); assertThat(e.getMessage(), is("field [lat] missing"));
assertThat(parser.currentToken(), is(Token.END_OBJECT));
assertNull(parser.nextToken());
} }
} }
@ -500,6 +525,8 @@ public class GeoUtilsTests extends ESTestCase {
parser.nextToken(); parser.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
assertThat(e.getMessage(), is("longitude must be a number")); assertThat(e.getMessage(), is("longitude must be a number"));
assertThat(parser.currentToken(), is(Token.END_OBJECT));
assertNull(parser.nextToken());
} }
} }
@ -510,6 +537,8 @@ public class GeoUtilsTests extends ESTestCase {
parser.nextToken(); parser.nextToken();
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
assertThat(e.getMessage(), is("latitude must be a number")); assertThat(e.getMessage(), is("latitude must be a number"));
assertThat(parser.currentToken(), is(Token.END_OBJECT));
assertNull(parser.nextToken());
} }
} }
@ -578,6 +607,9 @@ public class GeoUtilsTests extends ESTestCase {
} }
Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser)); Exception e = expectThrows(ElasticsearchParseException.class, () -> GeoUtils.parseGeoPoint(parser));
assertThat(e.getMessage(), is("numeric value expected")); assertThat(e.getMessage(), is("numeric value expected"));
assertThat(parser.currentToken(), is(Token.END_ARRAY));
assertThat(parser.nextToken(), is(Token.END_OBJECT));
assertNull(parser.nextToken());
} }
} }