[GEO] Add LinearRing and LineString validity checks as defined by http://geojson.org/geojson-spec.html to ensure valid polygons are specified at parse time.

Closes #8433

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
+        // Base cases
-        if (token != XContentParser.Token.START_ARRAY) {
+        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<CoordinateNode> 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;
         }

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")

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