Skip to content

Commit

Permalink
Improve robustness of geo shape parser for malformed shapes
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 elastic#31428
  • Loading branch information
@imotov
imotov committed Jun 26, 2018
1 parent ee59761 commit eaa7fd1
Show file tree
Hide file tree
Showing 2 changed files with 114 additions and 48 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
46 changes: 46 additions & 0 deletions 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 {
XContentParser parser = createParser(multilinesGeoJson);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());

// test #4: "envelope" with empty coordinates
multilinesGeoJson = XContentFactory.jsonBuilder().startObject().field("type", "envelope")
Expand All @@ -205,6 +208,7 @@ public void testParseEnvelope() throws IOException {
parser = createParser(multilinesGeoJson);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

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

public void testParseInvalidPoint() throws IOException {
Expand All @@ -302,6 +307,7 @@ public void testParseInvalidPoint() throws IOException {
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
XContentBuilder invalidPoint2 = XContentFactory.jsonBuilder()
Expand All @@ -313,6 +319,7 @@ public void testParseInvalidPoint() throws IOException {
parser = createParser(invalidPoint2);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

public void testParseInvalidMultipoint() throws IOException {
Expand All @@ -325,6 +332,7 @@ public void testParseInvalidMultipoint() throws IOException {
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
XContentBuilder invalidMultipoint2 = XContentFactory.jsonBuilder()
Expand All @@ -336,6 +344,7 @@ public void testParseInvalidMultipoint() throws IOException {
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
XContentBuilder invalidMultipoint3 = XContentFactory.jsonBuilder()
Expand All @@ -348,6 +357,7 @@ public void testParseInvalidMultipoint() throws IOException {
parser = createParser(invalidMultipoint3);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

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

public void testParseInvalidDimensionalMultiPolygon() throws IOException {
Expand Down Expand Up @@ -422,6 +433,7 @@ public void testParseInvalidDimensionalMultiPolygon() throws IOException {
XContentParser parser = createParser(JsonXContent.jsonXContent, multiPolygonGeoJson);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}


Expand Down Expand Up @@ -630,6 +642,7 @@ public void testParseInvalidPolygon() throws IOException {
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
invalidPoly = Strings.toString(XContentFactory.jsonBuilder().startObject().field("type", "polygon")
Expand All @@ -643,6 +656,7 @@ public void testParseInvalidPolygon() throws IOException {
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
invalidPoly = Strings.toString(XContentFactory.jsonBuilder().startObject().field("type", "polygon")
Expand All @@ -656,6 +670,7 @@ public void testParseInvalidPolygon() throws IOException {
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
invalidPoly = Strings.toString(XContentFactory.jsonBuilder().startObject().field("type", "polygon")
Expand All @@ -669,6 +684,7 @@ public void testParseInvalidPolygon() throws IOException {
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
invalidPoly = Strings.toString(XContentFactory.jsonBuilder().startObject().field("type", "polygon")
Expand All @@ -680,6 +696,7 @@ public void testParseInvalidPolygon() throws IOException {
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
invalidPoly = Strings.toString(XContentFactory.jsonBuilder().startObject().field("type", "polygon")
Expand All @@ -689,6 +706,7 @@ public void testParseInvalidPolygon() throws IOException {
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
invalidPoly = Strings.toString(XContentFactory.jsonBuilder().startObject().field("type", "polygon")
Expand All @@ -700,6 +718,7 @@ public void testParseInvalidPolygon() throws IOException {
parser = createParser(JsonXContent.jsonXContent, invalidPoly);
parser.nextToken();
ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
assertNull(parser.nextToken());
}

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

@Override
Expand Down Expand Up @@ -1131,4 +1151,30 @@ 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();

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();

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

0 comments on commit eaa7fd1

Please sign in to comment.