Skip to content

Commit

Permalink
Improve robustness of geo shape parser for malformed shapes (#31449)
Browse files Browse the repository at this point in the history 
Ensures that malformed geoshapes are more reliably ignored if
"ignore_malformed" is set to true instead of failing the entire
document.

Closes #31428
  • Loading branch information
@imotov
imotov authored Jun 26, 2018
1 parent 232c71b commit 544e473
Show file tree
Hide file tree
Showing 2 changed files with 117 additions and 49 deletions.
116 changes: 68 additions & 48 deletions server/src/main/java/org/elasticsearch/common/geo/parsers/GeoJsonParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,57 +55,66 @@ protected static ShapeBuilder parse(XContentParser parser, GeoShapeFieldMapper s
String malformedException = null;

XContentParser.Token token;
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
String fieldName = parser.currentName();

if (ShapeParser.FIELD_TYPE.match(fieldName, parser.getDeprecationHandler())) {
parser.nextToken();
final GeoShapeType type = GeoShapeType.forName(parser.text());
if (shapeType != null && shapeType.equals(type) == false) {
malformedException = ShapeParser.FIELD_TYPE + " already parsed as ["
+ shapeType + "] cannot redefine as [" + type + "]";
try {
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
String fieldName = parser.currentName();

if (ShapeParser.FIELD_TYPE.match(fieldName, parser.getDeprecationHandler())) {
parser.nextToken();
final GeoShapeType type = GeoShapeType.forName(parser.text());
if (shapeType != null && shapeType.equals(type) == false) {
malformedException = ShapeParser.FIELD_TYPE + " already parsed as ["
+ shapeType + "] cannot redefine as [" + type + "]";
} else {
shapeType = type;
}
} else if (ShapeParser.FIELD_COORDINATES.match(fieldName, parser.getDeprecationHandler())) {
parser.nextToken();
CoordinateNode tempNode = parseCoordinates(parser, ignoreZValue.value());
if (coordinateNode != null && tempNode.numDimensions() != coordinateNode.numDimensions()) {
throw new ElasticsearchParseException("Exception parsing coordinates: " +
"number of dimensions do not match");
}
coordinateNode = tempNode;
} else if (ShapeParser.FIELD_GEOMETRIES.match(fieldName, parser.getDeprecationHandler())) {
if (shapeType == null) {
shapeType = GeoShapeType.GEOMETRYCOLLECTION;
} else if (shapeType.equals(GeoShapeType.GEOMETRYCOLLECTION) == false) {
malformedException = "cannot have [" + ShapeParser.FIELD_GEOMETRIES + "] with type set to ["
+ shapeType + "]";
}
parser.nextToken();
geometryCollections = parseGeometries(parser, shapeMapper);
} else if (CircleBuilder.FIELD_RADIUS.match(fieldName, parser.getDeprecationHandler())) {
if (shapeType == null) {
shapeType = GeoShapeType.CIRCLE;
} else if (shapeType != null && shapeType.equals(GeoShapeType.CIRCLE) == false) {
malformedException = "cannot have [" + CircleBuilder.FIELD_RADIUS + "] with type set to ["
+ shapeType + "]";
}
parser.nextToken();
radius = DistanceUnit.Distance.parseDistance(parser.text());
} else if (ShapeParser.FIELD_ORIENTATION.match(fieldName, parser.getDeprecationHandler())) {
if (shapeType != null
&& (shapeType.equals(GeoShapeType.POLYGON) || shapeType.equals(GeoShapeType.MULTIPOLYGON)) == false) {
malformedException = "cannot have [" + ShapeParser.FIELD_ORIENTATION + "] with type set to [" + shapeType + "]";
}
parser.nextToken();
requestedOrientation = ShapeBuilder.Orientation.fromString(parser.text());
} else {
shapeType = type;
parser.nextToken();
parser.skipChildren();
}
} else if (ShapeParser.FIELD_COORDINATES.match(fieldName, parser.getDeprecationHandler())) {
parser.nextToken();
CoordinateNode tempNode = parseCoordinates(parser, ignoreZValue.value());
if (coordinateNode != null && tempNode.numDimensions() != coordinateNode.numDimensions()) {
throw new ElasticsearchParseException("Exception parsing coordinates: " +
"number of dimensions do not match");
}
coordinateNode = tempNode;
} else if (ShapeParser.FIELD_GEOMETRIES.match(fieldName, parser.getDeprecationHandler())) {
if (shapeType == null) {
shapeType = GeoShapeType.GEOMETRYCOLLECTION;
} else if (shapeType.equals(GeoShapeType.GEOMETRYCOLLECTION) == false) {
malformedException = "cannot have [" + ShapeParser.FIELD_GEOMETRIES + "] with type set to ["
+ shapeType + "]";
}
parser.nextToken();
geometryCollections = parseGeometries(parser, shapeMapper);
} else if (CircleBuilder.FIELD_RADIUS.match(fieldName, parser.getDeprecationHandler())) {
if (shapeType == null) {
shapeType = GeoShapeType.CIRCLE;
} else if (shapeType != null && shapeType.equals(GeoShapeType.CIRCLE) == false) {
malformedException = "cannot have [" + CircleBuilder.FIELD_RADIUS + "] with type set to ["
+ shapeType + "]";
}
parser.nextToken();
radius = DistanceUnit.Distance.parseDistance(parser.text());
} else if (ShapeParser.FIELD_ORIENTATION.match(fieldName, parser.getDeprecationHandler())) {
if (shapeType != null
&& (shapeType.equals(GeoShapeType.POLYGON) || shapeType.equals(GeoShapeType.MULTIPOLYGON)) == false) {
malformedException = "cannot have [" + ShapeParser.FIELD_ORIENTATION + "] with type set to [" + shapeType + "]";
}
parser.nextToken();
requestedOrientation = ShapeBuilder.Orientation.fromString(parser.text());
} else {
parser.nextToken();
parser.skipChildren();
}
}
} catch (Exception ex) {
// Skip all other fields until the end of the object
while (parser.currentToken() != XContentParser.Token.END_OBJECT && parser.currentToken() != null) {
parser.nextToken();
parser.skipChildren();
}
throw ex;
}

if (malformedException != null) {
Expand Down Expand Up @@ -144,6 +153,12 @@ protected static ShapeBuilder parse(XContentParser parser, GeoShapeFieldMapper s
* XContentParser
*/
private static CoordinateNode parseCoordinates(XContentParser parser, boolean ignoreZValue) throws IOException {
if (parser.currentToken() == XContentParser.Token.START_OBJECT) {
parser.skipChildren();
parser.nextToken();
throw new ElasticsearchParseException("coordinates cannot be specified as objects");
}

XContentParser.Token token = parser.nextToken();
// Base cases
if (token != XContentParser.Token.START_ARRAY &&
Expand All @@ -168,8 +183,13 @@ private static CoordinateNode parseCoordinates(XContentParser parser, boolean ig
}

private static Coordinate parseCoordinate(XContentParser parser, boolean ignoreZValue) throws IOException {
if (parser.currentToken() != XContentParser.Token.VALUE_NUMBER) {
throw new ElasticsearchParseException("geo coordinates must be numbers");
}
double lon = parser.doubleValue();
parser.nextToken();
if (parser.nextToken() != XContentParser.Token.VALUE_NUMBER) {
throw new ElasticsearchParseException("geo coordinates must be numbers");
}
double lat = parser.doubleValue();
XContentParser.Token token = parser.nextToken();
// alt (for storing purposes only - future use includes 3d shapes)
Expand Down
50 changes: 49 additions & 1 deletion server/src/test/java/org/elasticsearch/common/geo/GeoJsonShapeParserTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ public void testParseMultiDimensionShapes() throws IOException {
XContentParser parser = createParser(pointGeoJson);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());

// multi dimension linestring
XContentBuilder lineGeoJson = XContentFactory.jsonBuilder()
Expand All @@ -159,6 +160,7 @@ public void testParseMultiDimensionShapes() throws IOException {
parser = createParser(lineGeoJson);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

@Override
Expand Down Expand Up @@ -196,6 +198,7 @@ public void testParseEnvelope() throws IOException {
try (XContentParser parser = createParser(multilinesGeoJson)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

// test #4: "envelope" with empty coordinates
Expand All @@ -206,6 +209,7 @@ public void testParseEnvelope() throws IOException {
try (XContentParser parser = createParser(multilinesGeoJson)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}
}

Expand Down Expand Up @@ -291,6 +295,7 @@ public void testInvalidDimensionalPolygon() throws IOException {
try (XContentParser parser = createParser(polygonGeoJson)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}
}

Expand All @@ -306,6 +311,7 @@ public void testParseInvalidPoint() throws IOException {
try (XContentParser parser = createParser(invalidPoint1)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

// test case 2: create an invalid point object with an empty number of coordinates
Expand All @@ -318,6 +324,7 @@ public void testParseInvalidPoint() throws IOException {
try (XContentParser parser = createParser(invalidPoint2)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}
}

Expand All @@ -331,6 +338,7 @@ public void testParseInvalidMultipoint() throws IOException {
try (XContentParser parser = createParser(invalidMultipoint1)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

// test case 2: create an invalid multipoint object with null coordinate
Expand All @@ -343,6 +351,7 @@ public void testParseInvalidMultipoint() throws IOException {
try (XContentParser parser = createParser(invalidMultipoint2)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

// test case 3: create a valid formatted multipoint object with invalid number (0) of coordinates
Expand All @@ -356,6 +365,7 @@ public void testParseInvalidMultipoint() throws IOException {
try (XContentParser parser = createParser(invalidMultipoint3)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}
}

Expand Down Expand Up @@ -392,6 +402,7 @@ public void testParseInvalidMultiPolygon() throws IOException {
try (XContentParser parser = createParser(JsonXContent.jsonXContent, multiPolygonGeoJson)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, InvalidShapeException.class);
assertNull(parser.nextToken());
}
}

Expand Down Expand Up @@ -432,8 +443,9 @@ public void testParseInvalidDimensionalMultiPolygon() throws IOException {
try (XContentParser parser = createParser(JsonXContent.jsonXContent, multiPolygonGeoJson)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}
}
}


public void testParseOGCPolygonWithoutHoles() throws IOException {
Expand Down Expand Up @@ -650,6 +662,7 @@ public void testParseInvalidPolygon() throws IOException {
try (XContentParser parser = createParser(JsonXContent.jsonXContent, invalidPoly)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

// test case 2: create an invalid polygon with only 1 point
Expand All @@ -664,6 +677,7 @@ public void testParseInvalidPolygon() throws IOException {
try (XContentParser parser = createParser(JsonXContent.jsonXContent, invalidPoly)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

// test case 3: create an invalid polygon with 0 points
Expand All @@ -678,6 +692,7 @@ public void testParseInvalidPolygon() throws IOException {
try (XContentParser parser = createParser(JsonXContent.jsonXContent, invalidPoly)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

// test case 4: create an invalid polygon with null value points
Expand All @@ -692,6 +707,7 @@ public void testParseInvalidPolygon() throws IOException {
try (XContentParser parser = createParser(JsonXContent.jsonXContent, invalidPoly)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, IllegalArgumentException.class);
assertNull(parser.nextToken());
}

// test case 5: create an invalid polygon with 1 invalid LinearRing
Expand All @@ -704,6 +720,7 @@ public void testParseInvalidPolygon() throws IOException {
try (XContentParser parser = createParser(JsonXContent.jsonXContent, invalidPoly)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, IllegalArgumentException.class);
assertNull(parser.nextToken());
}

// test case 6: create an invalid polygon with 0 LinearRings
Expand All @@ -714,6 +731,7 @@ public void testParseInvalidPolygon() throws IOException {
try (XContentParser parser = createParser(JsonXContent.jsonXContent, invalidPoly)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

// test case 7: create an invalid polygon with 0 LinearRings
Expand All @@ -726,6 +744,7 @@ public void testParseInvalidPolygon() throws IOException {
try (XContentParser parser = createParser(JsonXContent.jsonXContent, invalidPoly)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}
}

Expand Down Expand Up @@ -794,6 +813,7 @@ public void testParseSelfCrossingPolygon() throws IOException {
try (XContentParser parser = createParser(JsonXContent.jsonXContent, polygonGeoJson)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, InvalidShapeException.class);
assertNull(parser.nextToken());
}
}

Expand Down Expand Up @@ -1165,4 +1185,32 @@ public void testParseOrientationOption() throws IOException {
ElasticsearchGeoAssertions.assertMultiPolygon(shape);
}
}

public void testParseInvalidShapes() throws IOException {
// single dimensions point
XContentBuilder tooLittlePointGeoJson = XContentFactory.jsonBuilder()
.startObject()
.field("type", "Point")
.startArray("coordinates").value(10.0).endArray()
.endObject();

try (XContentParser parser = createParser(tooLittlePointGeoJson)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

// zero dimensions point
XContentBuilder emptyPointGeoJson = XContentFactory.jsonBuilder()
.startObject()
.field("type", "Point")
.startObject("coordinates").field("foo", "bar").endObject()
.endObject();

try (XContentParser parser = createParser(emptyPointGeoJson)) {
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}
}
}

0 comments on commit 544e473

Please sign in to comment.