[GEO] Update ShapeBuilder and GeoPolygonQueryParser to accept unclosed GeoJSON

While the GeoJSON spec does say a polygon is represented as an array of LinearRings (where a LinearRing is defined as a 'closed' array of points), it is a simple change to close the polygon for users. This addresses situations like those integrated with twitter (where GeoJSON polygons are not closed) such that our users do not have to write extra code to close the polygon.

Author: nknize

src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java

@@ -36,6 +36,9 @@ public PolygonBuilder(Orientation orientation) {
     protected PolygonBuilder(ArrayList<Coordinate> points, Orientation orientation) {
         super(orientation);
         this.shell = new Ring<>(this, points);
+        if (!points.isEmpty()) {
+            this.close();
+        }
     }
 
     @Override

src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java

@@ -886,9 +886,6 @@ protected static LineStringBuilder parseLinearRing(CoordinateNode coordinates) {
             } else 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);
         }

src/main/java/org/elasticsearch/index/mapper/geo/GeoPointFieldMapper.java

@@ -538,9 +538,7 @@ public Mapper parse(ParseContext context) throws IOException {
                         double lon = context.parser().doubleValue();
                         token = context.parser().nextToken();
                         double lat = context.parser().doubleValue();
-                        while ((token = context.parser().nextToken()) != XContentParser.Token.END_ARRAY) {
-
-                        }
+                        while ((token = context.parser().nextToken()) != XContentParser.Token.END_ARRAY);
                         parse(context, sparse.reset(lat, lon), null);
                     } else {
                         while (token != XContentParser.Token.END_ARRAY) {

src/main/java/org/elasticsearch/index/query/GeoPolygonQueryParser.java

@@ -93,6 +93,9 @@ public Query parse(QueryParseContext parseContext) throws IOException, QueryPars
                             while ((token = parser.nextToken()) != Token.END_ARRAY) {
                                 shell.add(GeoUtils.parseGeoPoint(parser));
                             }
+                            if (!shell.get(shell.size()-1).equals(shell.get(0))) {
+                                shell.add(shell.get(0));
+                            }
                         } else {
                             throw new QueryParsingException(parseContext, "[geo_polygon] query does not support [" + currentFieldName
                                     + "]");

src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java

@@ -192,7 +192,6 @@ public void testParse_polygonNoHoles() throws IOException {
                 .startArray().value(101.0).value(1.0).endArray()
                 .startArray().value(101.0).value(0.0).endArray()
                 .startArray().value(100.0).value(0.0).endArray()
-                .startArray().value(100.0).value(1.0).endArray()
                 .endArray()
                 .endArray()
                 .endObject().string();

src/test/java/org/elasticsearch/search/geo/GeoPolygonTests.java

@@ -27,9 +27,8 @@
 import org.junit.Test;
 
 import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
+import static org.elasticsearch.index.query.QueryBuilders.boolQuery;
 import static org.elasticsearch.index.query.QueryBuilders.geoPolygonQuery;
-import static org.elasticsearch.index.query.QueryBuilders.filteredQuery;
-import static org.elasticsearch.index.query.QueryBuilders.matchAllQuery;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
 import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
 import static org.hamcrest.Matchers.anyOf;
@@ -88,7 +87,7 @@ protected void setupSuiteScopeCluster() throws Exception {
     public void simplePolygonTest() throws Exception {
 
         SearchResponse searchResponse = client().prepareSearch("test") // from NY
-                .setQuery(filteredQuery(matchAllQuery(), geoPolygonQuery("location")
+                .setQuery(boolQuery().must(geoPolygonQuery("location")
                         .addPoint(40.7, -74.0)
                         .addPoint(40.7, -74.1)
                         .addPoint(40.8, -74.1)
@@ -101,4 +100,20 @@ public void simplePolygonTest() throws Exception {
             assertThat(hit.id(), anyOf(equalTo("1"), equalTo("3"), equalTo("4"), equalTo("5")));
         }
     }
+
+    @Test
+    public void simpleUnclosedPolygon() throws Exception {
+        SearchResponse searchResponse = client().prepareSearch("test") // from NY
+                .setQuery(boolQuery().must(geoPolygonQuery("location")
+                        .addPoint(40.7, -74.0)
+                        .addPoint(40.7, -74.1)
+                        .addPoint(40.8, -74.1)
+                        .addPoint(40.8, -74.0)))
+                .execute().actionGet();
+        assertHitCount(searchResponse, 4);
+        assertThat(searchResponse.getHits().hits().length, equalTo(4));
+        for (SearchHit hit : searchResponse.getHits()) {
+            assertThat(hit.id(), anyOf(equalTo("1"), equalTo("3"), equalTo("4"), equalTo("5")));
+        }
+    }
 }