[GEO] Update ShapeBuilder and GeoPolygonQueryParser to accept unclosed GeoJSON

nknize · nknize · commit bc9a4707db16 · 2015-07-17T10:41:27.000-05:00
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
diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java
@@ -28,6 +28,7 @@
 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 static ShapeBuilder parse(XContentParser parser, GeoShapeFieldMapper shap
             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 static ShapeBuilder parse(XContentParser parser, GeoShapeFieldMapper shap
                         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 static ShapeBuilder parse(XContentParser parser, GeoShapeFieldMapper shap
                 case MULTIPOINT: return parseMultiPoint(node);
                 case LINESTRING: return parseLineString(node);
                 case MULTILINESTRING: return parseMultiLine(node);
-                case POLYGON: return parsePolygon(node, requestedOrientation);
-                case MULTIPOLYGON: return parseMultiPolygon(node, requestedOrientation);
+                case POLYGON: return parsePolygon(node, requestedOrientation, coerce);
+                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 @@ protected static CircleBuilder parseCircle(CoordinateNode coordinates, Distance
             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 @@ protected static MultiLineStringBuilder parseMultiLine(CoordinateNode coordinate
             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 @@ protected static LineStringBuilder parseLinearRing(CoordinateNode coordinates) {
                 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(
-                        coordinates.children.get(coordinates.children.size() - 1).coordinate)) {
-                throw new ElasticsearchParseException("invalid LinearRing found (coordinates are not closed)");
+            }
+
+            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 @@ protected static MultiPolygonBuilder parseMultiPolygon(CoordinateNode coordinate
          * @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);
diff --git a/core/src/main/java/org/elasticsearch/index/mapper/geo/GeoPointFieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/geo/GeoPointFieldMapper.java
@@ -636,9 +636,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) {
diff --git a/core/src/main/java/org/elasticsearch/index/mapper/geo/GeoShapeFieldMapper.java b/core/src/main/java/org/elasticsearch/index/mapper/geo/GeoShapeFieldMapper.java
@@ -29,6 +29,7 @@
 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.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.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 static class Names {
         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 static class Defaults {
         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 static class Defaults {
 
     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 GeoShapeFieldType fieldType() {
             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 GeoShapeFieldMapper build(BuilderContext context) {
             }
             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 Mapper.Builder parse(String name, Map<String, Object> node, ParserContext
                 } 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;
@@ -246,7 +273,7 @@ public void freeze() {
             termStrategy.setDistErrPct(distanceErrorPct());
             defaultStrategy = resolveStrategy(strategyName);
         }
-        
+
         @Override
         public void checkCompatibility(MappedFieldType fieldType, List<String> conflicts, boolean strict) {
             super.checkCompatibility(fieldType, conflicts, strict);
@@ -357,8 +384,12 @@ public String value(Object value) {
 
     }
 
-    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 Mapper parse(ParseContext context) throws IOException {
     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 @@ protected void doXContentBody(XContentBuilder builder, boolean includeDefaults,
         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
diff --git a/core/src/main/java/org/elasticsearch/index/query/GeoPolygonQueryParser.java b/core/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
                                     + "]");
diff --git a/core/src/test/java/org/elasticsearch/index/mapper/geo/GeoShapeFieldMapperTests.java b/core/src/test/java/org/elasticsearch/index/mapper/geo/GeoShapeFieldMapperTests.java
@@ -102,6 +102,41 @@ public void testOrientationParsing() throws IOException {
         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")
diff --git a/core/src/test/java/org/elasticsearch/search/geo/GeoPolygonTests.java b/core/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")));
+        }
+    }
 }
