[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), the coerce parameter provides users with flexibility to have ES automatically close polygons. 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. This code change adds the optional coerce parameter to the GeoShapeFieldMapper.

closes #11131

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

@@ -28,6 +28,7 @@ import com.vividsolutions.jts.geom.Geometry;
 import com.vividsolutions.jts.geom.GeometryFactory;
 import org.apache.commons.lang3.tuple.Pair;
 import org.elasticsearch.ElasticsearchParseException;
+import org.elasticsearch.common.Explicit;
 import org.elasticsearch.common.logging.ESLogger;
 import org.elasticsearch.common.logging.ESLoggerFactory;
 import org.elasticsearch.common.unit.DistanceUnit.Distance;
@@ -728,7 +729,9 @@ public abstract class ShapeBuilder implements ToXContent {
             Distance radius = null;
             CoordinateNode node = null;
             GeometryCollectionBuilder geometryCollections = null;
+
             Orientation requestedOrientation = (shapeMapper == null) ? Orientation.RIGHT : shapeMapper.fieldType().orientation();
+            boolean coerce = (shapeMapper == null) ? GeoShapeFieldMapper.Defaults.COERCE.value() : shapeMapper.coerce().value();
 
             XContentParser.Token token;
             while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
@@ -743,7 +746,7 @@ public abstract class ShapeBuilder implements ToXContent {
                         node = parseCoordinates(parser);
                     } else if (FIELD_GEOMETRIES.equals(fieldName)) {
                         parser.nextToken();
-                        geometryCollections = parseGeometries(parser, requestedOrientation);
+                        geometryCollections = parseGeometries(parser, shapeMapper);
                     } else if (CircleBuilder.FIELD_RADIUS.equals(fieldName)) {
                         parser.nextToken();
                         radius = Distance.parseDistance(parser.text());
@@ -772,8 +775,8 @@ public abstract class ShapeBuilder implements ToXContent {
                 case MULTIPOINT: return parseMultiPoint(node);
                 case LINESTRING: return parseLineString(node);
                 case MULTILINESTRING: return parseMultiLine(node);
-                case POLYGON: return parsePolygon(node, requestedOrientation);
+                case POLYGON: return parsePolygon(node, requestedOrientation, coerce);
-                case MULTIPOLYGON: return parseMultiPolygon(node, requestedOrientation);
+                case MULTIPOLYGON: return parseMultiPolygon(node, requestedOrientation, coerce);
                 case CIRCLE: return parseCircle(node, radius);
                 case ENVELOPE: return parseEnvelope(node, requestedOrientation);
                 case GEOMETRYCOLLECTION: return geometryCollections;
@@ -801,7 +804,7 @@ public abstract class ShapeBuilder implements ToXContent {
             return newCircleBuilder().center(coordinates.coordinate).radius(radius);
         }
 
-        protected static EnvelopeBuilder parseEnvelope(CoordinateNode coordinates, Orientation orientation) {
+        protected static EnvelopeBuilder parseEnvelope(CoordinateNode coordinates, final Orientation orientation) {
             // validate the coordinate array for envelope type
             if (coordinates.children.size() != 2) {
                 throw new ElasticsearchParseException("invalid number of points [{}] provided for " +
@@ -868,7 +871,7 @@ public abstract class ShapeBuilder implements ToXContent {
             return multiline;
         }
 
-        protected static LineStringBuilder parseLinearRing(CoordinateNode coordinates) {
+        protected static LineStringBuilder parseLinearRing(CoordinateNode coordinates, boolean coerce) {
             /**
              * 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
@@ -880,32 +883,43 @@ public abstract class ShapeBuilder implements ToXContent {
                 error += (coordinates.coordinate == null) ?
                         " No coordinate array provided" : " Found a single coordinate when expecting a coordinate array";
                 throw new ElasticsearchParseException(error);
-            } else if (coordinates.children.size() < 4) {
+            }
-                throw new ElasticsearchParseException("invalid number of points in LinearRing (found [{}] - must be >= 4)", coordinates.children.size());
+
-            } else if (!coordinates.children.get(0).coordinate.equals(
+            int numValidPts;
+            if (coordinates.children.size() < (numValidPts = (coerce) ? 3 : 4)) {
+                throw new ElasticsearchParseException("invalid number of points in LinearRing (found [{}] - must be >= " +  numValidPts + ")(",
+                        coordinates.children.size());
+            }
+
+            if (!coordinates.children.get(0).coordinate.equals(
                     coordinates.children.get(coordinates.children.size() - 1).coordinate)) {
+                if (coerce) {
+                    coordinates.children.add(coordinates.children.get(0));
+                } else {
                     throw new ElasticsearchParseException("invalid LinearRing found (coordinates are not closed)");
                 }
+            }
             return parseLineString(coordinates);
         }
 
-        protected static PolygonBuilder parsePolygon(CoordinateNode coordinates, Orientation orientation) {
+        protected static PolygonBuilder parsePolygon(CoordinateNode coordinates, final Orientation orientation, final boolean coerce) {
             if (coordinates.children == null || coordinates.children.isEmpty()) {
                 throw new ElasticsearchParseException("invalid LinearRing provided for type polygon. Linear ring must be an array of coordinates");
             }
 
-            LineStringBuilder shell = parseLinearRing(coordinates.children.get(0));
+            LineStringBuilder shell = parseLinearRing(coordinates.children.get(0), coerce);
             PolygonBuilder polygon = new PolygonBuilder(shell.points, orientation);
             for (int i = 1; i < coordinates.children.size(); i++) {
-                polygon.hole(parseLinearRing(coordinates.children.get(i)));
+                polygon.hole(parseLinearRing(coordinates.children.get(i), coerce));
             }
             return polygon;
         }
 
-        protected static MultiPolygonBuilder parseMultiPolygon(CoordinateNode coordinates, Orientation orientation) {
+        protected static MultiPolygonBuilder parseMultiPolygon(CoordinateNode coordinates, final Orientation orientation,
+                                                               final boolean coerce) {
             MultiPolygonBuilder polygons = newMultiPolygon(orientation);
             for (CoordinateNode node : coordinates.children) {
-                polygons.polygon(parsePolygon(node, orientation));
+                polygons.polygon(parsePolygon(node, orientation, coerce));
             }
             return polygons;
         }
@@ -917,13 +931,15 @@ public abstract class ShapeBuilder implements ToXContent {
          * @return Geometry[] geometries of the GeometryCollection
          * @throws IOException Thrown if an error occurs while reading from the XContentParser
          */
-        protected static GeometryCollectionBuilder parseGeometries(XContentParser parser, Orientation orientation) throws IOException {
+        protected static GeometryCollectionBuilder parseGeometries(XContentParser parser, GeoShapeFieldMapper mapper) throws
+                IOException {
             if (parser.currentToken() != XContentParser.Token.START_ARRAY) {
                 throw new ElasticsearchParseException("geometries must be an array of geojson objects");
             }
         
             XContentParser.Token token = parser.nextToken();
-            GeometryCollectionBuilder geometryCollection = newGeometryCollection(orientation);
+            GeometryCollectionBuilder geometryCollection = newGeometryCollection( (mapper == null) ? Orientation.RIGHT : mapper
+                    .fieldType().orientation());
             while (token != XContentParser.Token.END_ARRAY) {
                 ShapeBuilder shapeBuilder = GeoShapeType.parse(parser);
                 geometryCollection.shape(shapeBuilder);

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

@@ -636,9 +636,7 @@ public class GeoPointFieldMapper extends FieldMapper implements ArrayValueMapper
                         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) {

core/src/main/java/org/elasticsearch/index/mapper/geo/GeoShapeFieldMapper.java

@@ -29,6 +29,7 @@ import org.apache.lucene.spatial.prefix.tree.PackedQuadPrefixTree;
 import org.apache.lucene.spatial.prefix.tree.QuadPrefixTree;
 import org.apache.lucene.spatial.prefix.tree.SpatialPrefixTree;
 import org.elasticsearch.Version;
+import org.elasticsearch.common.Explicit;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.geo.GeoUtils;
 import org.elasticsearch.common.geo.SpatialStrategy;
@@ -41,6 +42,8 @@ import org.elasticsearch.index.mapper.FieldMapper;
 import org.elasticsearch.index.mapper.MappedFieldType;
 import org.elasticsearch.index.mapper.Mapper;
 import org.elasticsearch.index.mapper.MapperParsingException;
+import org.elasticsearch.index.mapper.MergeMappingException;
+import org.elasticsearch.index.mapper.MergeResult;
 import org.elasticsearch.index.mapper.ParseContext;
 
 import java.io.IOException;
@@ -49,6 +52,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 
+import static org.elasticsearch.common.xcontent.support.XContentMapValues.nodeBooleanValue;
 import static org.elasticsearch.index.mapper.MapperBuilders.geoShapeField;
 
 
@@ -81,6 +85,7 @@ public class GeoShapeFieldMapper extends FieldMapper {
         public static final String DISTANCE_ERROR_PCT = "distance_error_pct";
         public static final String ORIENTATION = "orientation";
         public static final String STRATEGY = "strategy";
+        public static final String COERCE = "coerce";
     }
 
     public static class Defaults {
@@ -90,6 +95,7 @@ public class GeoShapeFieldMapper extends FieldMapper {
         public static final int QUADTREE_LEVELS = GeoUtils.quadTreeLevelsForPrecision("50m");
         public static final double LEGACY_DISTANCE_ERROR_PCT = 0.025d;
         public static final Orientation ORIENTATION = Orientation.RIGHT;
+        public static final Explicit<Boolean> COERCE = new Explicit<>(false, false);
 
         public static final MappedFieldType FIELD_TYPE = new GeoShapeFieldType();
 
@@ -108,6 +114,8 @@ public class GeoShapeFieldMapper extends FieldMapper {
 
     public static class Builder extends FieldMapper.Builder<Builder, GeoShapeFieldMapper> {
 
+        private Boolean coerce;
+
         public Builder(String name) {
             super(name, Defaults.FIELD_TYPE);
         }
@@ -116,6 +124,21 @@ public class GeoShapeFieldMapper extends FieldMapper {
             return (GeoShapeFieldType)fieldType;
         }
 
+        public Builder coerce(boolean coerce) {
+            this.coerce = coerce;
+            return builder;
+        }
+
+        protected Explicit<Boolean> coerce(BuilderContext context) {
+            if (coerce != null) {
+                return new Explicit<>(coerce, true);
+            }
+            if (context.indexSettings() != null) {
+                return new Explicit<>(context.indexSettings().getAsBoolean("index.mapping.coerce", Defaults.COERCE.value()), false);
+            }
+            return Defaults.COERCE;
+        }
+
         @Override
         public GeoShapeFieldMapper build(BuilderContext context) {
             GeoShapeFieldType geoShapeFieldType = (GeoShapeFieldType)fieldType;
@@ -130,7 +153,8 @@ public class GeoShapeFieldMapper extends FieldMapper {
             }
             setupFieldType(context);
 
-            return new GeoShapeFieldMapper(name, fieldType, context.indexSettings(), multiFieldsBuilder.build(this, context), copyTo);
+            return new GeoShapeFieldMapper(name, fieldType, coerce(context), context.indexSettings(), multiFieldsBuilder.build(this,
+                    context), copyTo);
         }
     }
 
@@ -161,6 +185,9 @@ public class GeoShapeFieldMapper extends FieldMapper {
                 } else if (Names.STRATEGY.equals(fieldName)) {
                     builder.fieldType().setStrategyName(fieldNode.toString());
                     iterator.remove();
+                } else if (Names.COERCE.equals(fieldName)) {
+                    builder.coerce(nodeBooleanValue(fieldNode));
+                    iterator.remove();
                 }
             }
             return builder;
@@ -357,8 +384,12 @@ public class GeoShapeFieldMapper extends FieldMapper {
 
     }
 
-    public GeoShapeFieldMapper(String simpleName, MappedFieldType fieldType, Settings indexSettings, MultiFields multiFields, CopyTo copyTo) {
+    protected Explicit<Boolean> coerce;
+
+    public GeoShapeFieldMapper(String simpleName, MappedFieldType fieldType, Explicit<Boolean> coerce, Settings indexSettings,
+                               MultiFields multiFields, CopyTo copyTo) {
         super(simpleName, fieldType, Defaults.FIELD_TYPE, indexSettings, multiFields, copyTo);
+        this.coerce = coerce;
     }
 
     @Override
@@ -397,6 +428,21 @@ public class GeoShapeFieldMapper extends FieldMapper {
     protected void parseCreateField(ParseContext context, List<Field> fields) throws IOException {
     }
 
+    @Override
+    public void merge(Mapper mergeWith, MergeResult mergeResult) throws MergeMappingException {
+        super.merge(mergeWith, mergeResult);
+        if (!this.getClass().equals(mergeWith.getClass())) {
+            return;
+        }
+
+        GeoShapeFieldMapper gsfm = (GeoShapeFieldMapper)mergeWith;
+        if (mergeResult.simulate() == false && mergeResult.hasConflicts() == false) {
+            if (gsfm.coerce.explicit()) {
+                this.coerce = gsfm.coerce;
+            }
+        }
+    }
+
     @Override
     protected void doXContentBody(XContentBuilder builder, boolean includeDefaults, Params params) throws IOException {
         builder.field("type", contentType());
@@ -419,6 +465,13 @@ public class GeoShapeFieldMapper extends FieldMapper {
         if (includeDefaults || fieldType().orientation() != Defaults.ORIENTATION) {
             builder.field(Names.ORIENTATION, fieldType().orientation());
         }
+        if (includeDefaults || coerce.explicit()) {
+            builder.field("coerce", coerce.value());
+        }
+    }
+
+    public Explicit<Boolean> coerce() {
+        return coerce;
     }
 
     @Override

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

@@ -93,6 +93,9 @@ public class GeoPolygonQueryParser implements QueryParser {
                             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
                                     + "]");

core/src/test/java/org/elasticsearch/index/mapper/geo/GeoShapeFieldMapperTests.java

@@ -102,6 +102,41 @@ public class GeoShapeFieldMapperTests extends ElasticsearchSingleNodeTest {
         assertThat(orientation, equalTo(ShapeBuilder.Orientation.CCW));
     }
 
+    /**
+     * Test that orientation parameter correctly parses
+     * @throws IOException
+     */
+    public void testCoerceParsing() throws IOException {
+        String mapping = XContentFactory.jsonBuilder().startObject().startObject("type1")
+                .startObject("properties").startObject("location")
+                .field("type", "geo_shape")
+                .field("coerce", "true")
+                .endObject().endObject()
+                .endObject().endObject().string();
+
+        DocumentMapper defaultMapper = createIndex("test").mapperService().documentMapperParser().parse(mapping);
+        FieldMapper fieldMapper = defaultMapper.mappers().getMapper("location");
+        assertThat(fieldMapper, instanceOf(GeoShapeFieldMapper.class));
+
+        boolean coerce = ((GeoShapeFieldMapper)fieldMapper).coerce().value();
+        assertThat(coerce, equalTo(true));
+
+        // explicit false coerce test
+        mapping = XContentFactory.jsonBuilder().startObject().startObject("type1")
+                .startObject("properties").startObject("location")
+                .field("type", "geo_shape")
+                .field("coerce", "false")
+                .endObject().endObject()
+                .endObject().endObject().string();
+
+        defaultMapper = createIndex("test2").mapperService().documentMapperParser().parse(mapping);
+        fieldMapper = defaultMapper.mappers().getMapper("location");
+        assertThat(fieldMapper, instanceOf(GeoShapeFieldMapper.class));
+
+        coerce = ((GeoShapeFieldMapper)fieldMapper).coerce().value();
+        assertThat(coerce, equalTo(false));
+    }
+
     @Test
     public void testGeohashConfiguration() throws IOException {
         String mapping = XContentFactory.jsonBuilder().startObject().startObject("type1")

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

@@ -27,9 +27,8 @@ import org.elasticsearch.test.ElasticsearchIntegrationTest;
 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 @@ public class GeoPolygonTests extends ElasticsearchIntegrationTest {
     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 class GeoPolygonTests extends ElasticsearchIntegrationTest {
             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")));
+        }
+    }
 }