diff --git a/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java b/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java index 42e566eca93..edfee171fc2 100644 --- a/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java +++ b/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java @@ -206,13 +206,17 @@ public abstract class ShapeBuilder implements ToXContent { private static CoordinateNode parseCoordinates(XContentParser parser) throws IOException { XContentParser.Token token = parser.nextToken(); - // Base case - if (token != XContentParser.Token.START_ARRAY) { + // Base cases + if (token != XContentParser.Token.START_ARRAY && + token != XContentParser.Token.END_ARRAY && + token != XContentParser.Token.VALUE_NULL) { double lon = parser.doubleValue(); token = parser.nextToken(); double lat = parser.doubleValue(); token = parser.nextToken(); return new CoordinateNode(new Coordinate(lon, lat)); + } else if (token == XContentParser.Token.VALUE_NULL) { + throw new ElasticsearchIllegalArgumentException("coordinates cannot contain NULL values)"); } List nodes = new ArrayList<>(); @@ -625,6 +629,16 @@ public abstract class ShapeBuilder implements ToXContent { } protected static LineStringBuilder parseLineString(CoordinateNode coordinates) { + /** + * Per GeoJSON spec (http://geojson.org/geojson-spec.html#linestring) + * "coordinates" member must be an array of two or more positions + * LineStringBuilder should throw a graceful exception if < 2 coordinates/points are provided + */ + if (coordinates.children.size() < 2) { + throw new ElasticsearchParseException("Invalid number of points in LineString (found " + + coordinates.children.size() + " - must be >= 2)"); + } + LineStringBuilder line = newLineString(); for (CoordinateNode node : coordinates.children) { line.point(node.coordinate); @@ -640,11 +654,28 @@ public abstract class ShapeBuilder implements ToXContent { return multiline; } + protected static LineStringBuilder parseLinearRing(CoordinateNode coordinates) { + /** + * Per GeoJSON spec (http://geojson.org/geojson-spec.html#linestring) + * A LinearRing is closed LineString with 4 or more positions. The first and last positions + * are equivalent (they represent equivalent points). Though a LinearRing is not explicitly + * represented as a GeoJSON geometry type, it is referred to in the Polygon geometry type definition. + */ + if (coordinates.children.size() < 4) { + throw new ElasticsearchParseException("Invalid number of points in LinearRing (found " + + coordinates.children.size() + " - must be >= 4)"); + } else if (!coordinates.children.get(0).coordinate.equals( + coordinates.children.get(coordinates.children.size() - 1).coordinate)) { + throw new ElasticsearchParseException("Invalid LinearRing found (coordinates are not closed)"); + } + return parseLineString(coordinates); + } + protected static PolygonBuilder parsePolygon(CoordinateNode coordinates) { - LineStringBuilder shell = parseLineString(coordinates.children.get(0)); + LineStringBuilder shell = parseLinearRing(coordinates.children.get(0)); PolygonBuilder polygon = new PolygonBuilder(shell.points); for (int i = 1; i < coordinates.children.size(); i++) { - polygon.hole(parseLineString(coordinates.children.get(i))); + polygon.hole(parseLinearRing(coordinates.children.get(i))); } return polygon; } diff --git a/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java b/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java index 211f7a54b62..e3b4eff2ecd 100644 --- a/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java +++ b/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java @@ -26,6 +26,8 @@ import com.spatial4j.core.shape.ShapeCollection; import com.spatial4j.core.shape.jts.JtsGeometry; import com.spatial4j.core.shape.jts.JtsPoint; import com.vividsolutions.jts.geom.*; +import org.elasticsearch.ElasticsearchIllegalArgumentException; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.geo.builders.ShapeBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; @@ -155,6 +157,76 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase { assertGeometryEquals(jtsGeom(expected), polygonGeoJson); } + @Test + public void testParse_invalidPolygon() throws IOException { + /** + * The following 3 test cases ensure proper error handling of invalid polygons + * per the GeoJSON specification + */ + // test case 1: create an invalid polygon with only 2 points + String invalidPoly1 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") + .startArray("coordinates") + .startArray() + .startArray().value(-74.011).value(40.753).endArray() + .startArray().value(-75.022).value(41.783).endArray() + .endArray() + .endArray() + .endObject().string(); + XContentParser parser = JsonXContent.jsonXContent.createParser(invalidPoly1); + parser.nextToken(); + ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + + // test case 2: create an invalid polygon with only 1 point + String invalidPoly2 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") + .startArray("coordinates") + .startArray() + .startArray().value(-74.011).value(40.753).endArray() + .endArray() + .endArray() + .endObject().string(); + + parser = JsonXContent.jsonXContent.createParser(invalidPoly2); + parser.nextToken(); + ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + + // test case 3: create an invalid polygon with 0 points + String invalidPoly3 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") + .startArray("coordinates") + .startArray() + .startArray().endArray() + .endArray() + .endArray() + .endObject().string(); + + parser = JsonXContent.jsonXContent.createParser(invalidPoly3); + parser.nextToken(); + ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); + + // test case 4: create an invalid polygon with null value points + String invalidPoly4 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") + .startArray("coordinates") + .startArray() + .startArray().nullValue().nullValue().endArray() + .endArray() + .endArray() + .endObject().string(); + + parser = JsonXContent.jsonXContent.createParser(invalidPoly4); + parser.nextToken(); + ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchIllegalArgumentException.class); + + // test case 5: create an invalid polygon with 1 invalid LinearRing + String invalidPoly5 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") + .startArray("coordinates") + .nullValue().nullValue() + .endArray() + .endObject().string(); + + parser = JsonXContent.jsonXContent.createParser(invalidPoly5); + parser.nextToken(); + ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchIllegalArgumentException.class); + } + @Test public void testParse_polygonWithHole() throws IOException { String polygonGeoJson = XContentFactory.jsonBuilder().startObject().field("type", "Polygon") diff --git a/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java b/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java index 1c3c636beee..7ba04cb9d78 100644 --- a/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java +++ b/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java @@ -26,9 +26,12 @@ import com.spatial4j.core.shape.impl.RectangleImpl; import com.spatial4j.core.shape.jts.JtsGeometry; import com.spatial4j.core.shape.jts.JtsPoint; import com.vividsolutions.jts.geom.*; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.geo.GeoDistance; import org.elasticsearch.common.geo.GeoPoint; +import org.elasticsearch.common.geo.builders.ShapeBuilder; import org.elasticsearch.common.unit.DistanceUnit; +import org.elasticsearch.common.xcontent.XContentParser; import org.hamcrest.Matcher; import org.junit.Assert; @@ -246,4 +249,13 @@ public class ElasticsearchGeoAssertions { return GeoDistance.ARC.calculate(lat1, lon1, lat2, lon2, DistanceUnit.DEFAULT); } + public static void assertValidException(XContentParser parser, Class expectedException) { + try { + ShapeBuilder.parse(parser); + Assert.fail("process completed successfully when " + expectedException.getName() + " expected"); + } catch (Exception e) { + assert(e.getClass().equals(expectedException)): + "expected " + expectedException.getName() + " but found " + e.getClass().getName(); + } + } }