From bc9a4707db16a97c5202ba2b6e1c2e4e3b73eaff Mon Sep 17 00:00:00 2001 From: Nicholas Knize Date: Wed, 13 May 2015 22:20:17 -0500 Subject: [PATCH] [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 --- .../common/geo/builders/ShapeBuilder.java | 50 ++++++++++------ .../index/mapper/geo/GeoPointFieldMapper.java | 4 +- .../index/mapper/geo/GeoShapeFieldMapper.java | 59 ++++++++++++++++++- .../index/query/GeoPolygonQueryParser.java | 3 + .../mapper/geo/GeoShapeFieldMapperTests.java | 35 +++++++++++ .../search/geo/GeoPolygonTests.java | 21 ++++++- 6 files changed, 146 insertions(+), 26 deletions(-) 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 index 4cea9c21b78..2443e5156e3 100644 --- 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.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 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 @@ 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( - 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 @@ 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); 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 index 8ac4fe259c9..0458c410b5b 100644 --- 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 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) { 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 index 4abd48c68f7..3e7aa39e42e 100644 --- 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.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 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 { + 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 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; @@ -246,7 +273,7 @@ public class GeoShapeFieldMapper extends FieldMapper { termStrategy.setDistErrPct(distanceErrorPct()); defaultStrategy = resolveStrategy(strategyName); } - + @Override public void checkCompatibility(MappedFieldType fieldType, List conflicts, boolean strict) { super.checkCompatibility(fieldType, conflicts, strict); @@ -357,8 +384,12 @@ public class GeoShapeFieldMapper extends FieldMapper { } - public GeoShapeFieldMapper(String simpleName, MappedFieldType fieldType, Settings indexSettings, MultiFields multiFields, CopyTo copyTo) { + protected Explicit coerce; + + public GeoShapeFieldMapper(String simpleName, MappedFieldType fieldType, Explicit 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 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 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 index fad3828bda6..43d368666e1 100644 --- 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 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 + "]"); 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 index e9a54feadd2..24e9be49b67 100644 --- 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 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") diff --git a/core/src/test/java/org/elasticsearch/search/geo/GeoPolygonTests.java b/core/src/test/java/org/elasticsearch/search/geo/GeoPolygonTests.java index 8b7b505059f..014143a2be9 100644 --- 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.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"))); + } + } }