Upgrade to Spatial4j 0.4.1 and JTS 1.13

Fixes #5279

pom.xml

@@ -146,14 +146,14 @@
         <dependency>
             <groupId>com.spatial4j</groupId>
             <artifactId>spatial4j</artifactId>
-            <version>0.3</version>
+            <version>0.4.1</version>
             <scope>compile</scope>
             <optional>true</optional>
         </dependency>
         <dependency>
             <groupId>com.vividsolutions</groupId>
             <artifactId>jts</artifactId>
-            <version>1.12</version>
+            <version>1.13</version>
             <scope>compile</scope>
             <optional>true</optional>
             <exclusions>

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

@@ -23,10 +23,10 @@ import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
 
+import com.spatial4j.core.shape.ShapeCollection;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 
 import com.spatial4j.core.shape.Shape;
-import com.spatial4j.core.shape.jts.JtsGeometry;
 import com.vividsolutions.jts.geom.Coordinate;
 import com.vividsolutions.jts.geom.Geometry;
 import com.vividsolutions.jts.geom.GeometryFactory;
@@ -64,7 +64,7 @@ public abstract class BaseLineStringBuilder<E extends BaseLineStringBuilder<E>>
         } else {
             geometry = FACTORY.createLineString(coordinates);
         }
-        return new JtsGeometry(geometry, SPATIAL_CONTEXT, !wrapdateline);
+        return jtsGeometry(geometry);
     }
 
     protected static ArrayList<LineString> decompose(GeometryFactory factory, Coordinate[] coordinates, ArrayList<LineString> strings) {

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

@@ -27,7 +27,6 @@ import java.util.Iterator;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 
 import com.spatial4j.core.shape.Shape;
-import com.spatial4j.core.shape.jts.JtsGeometry;
 import com.vividsolutions.jts.geom.Coordinate;
 import com.vividsolutions.jts.geom.Geometry;
 import com.vividsolutions.jts.geom.GeometryFactory;
@@ -149,8 +148,7 @@ public abstract class BasePolygonBuilder<E extends BasePolygonBuilder<E>> extend
 
     @Override
     public Shape build() {
-        Geometry geometry = buildGeometry(FACTORY, wrapdateline);
+        return jtsGeometry(buildGeometry(FACTORY, wrapdateline));
-        return new JtsGeometry(geometry, SPATIAL_CONTEXT, !wrapdateline);
     }
 
     protected XContentBuilder coordinatesArray(XContentBuilder builder, Params params) throws IOException {

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

@@ -97,7 +97,7 @@ public class MultiLineStringBuilder extends ShapeBuilder {
             }
             geometry = FACTORY.createMultiLineString(lineStrings);
         }
-        return new JtsGeometry(geometry, SPATIAL_CONTEXT, true);
+        return jtsGeometry(geometry);
     }
 
     public static class InternalLineStringBuilder extends BaseLineStringBuilder<InternalLineStringBuilder> {

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

@@ -19,13 +19,15 @@
 
 package org.elasticsearch.common.geo.builders;
 
+import com.spatial4j.core.shape.Point;
 import com.spatial4j.core.shape.Shape;
-import com.spatial4j.core.shape.jts.JtsGeometry;
+import com.spatial4j.core.shape.ShapeCollection;
 import com.vividsolutions.jts.geom.Coordinate;
-import com.vividsolutions.jts.geom.MultiPoint;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 
 import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
 
 public class MultiPointBuilder extends PointCollection<MultiPointBuilder> {
 
@@ -43,8 +45,13 @@ public class MultiPointBuilder extends PointCollection<MultiPointBuilder> {
 
     @Override
     public Shape build() {
-        MultiPoint geometry = FACTORY.createMultiPoint(points.toArray(new Coordinate[points.size()]));
+        //Could wrap JtsGeometry but probably slower due to conversions to/from JTS in relate()
-        return new JtsGeometry(geometry, SPATIAL_CONTEXT, true);
+        //MultiPoint geometry = FACTORY.createMultiPoint(points.toArray(new Coordinate[points.size()]));
+        List<Point> shapes = new ArrayList<Point>(points.size());
+        for (Coordinate coord : points) {
+            shapes.add(SPATIAL_CONTEXT.makePoint(coord.x, coord.y));
+        }
+        return new ShapeCollection<Point>(shapes, SPATIAL_CONTEXT);
     }
 
     @Override

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

@@ -21,15 +21,13 @@ package org.elasticsearch.common.geo.builders;
 
 import java.io.IOException;
 import java.util.ArrayList;
-import java.util.Iterator;
+import java.util.List;
 
+import com.spatial4j.core.shape.ShapeCollection;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 
 import com.spatial4j.core.shape.Shape;
-import com.spatial4j.core.shape.jts.JtsGeometry;
 import com.vividsolutions.jts.geom.Coordinate;
-import com.vividsolutions.jts.geom.Geometry;
-import com.vividsolutions.jts.geom.Polygon;
 
 public class MultiPolygonBuilder extends ShapeBuilder {
 
@@ -70,30 +68,24 @@ public class MultiPolygonBuilder extends ShapeBuilder {
     @Override
     public Shape build() {
 
-        Polygon[] polygons;
+        List<Shape> shapes = new ArrayList<Shape>(this.polygons.size());
         
         if(wrapdateline) {
-            ArrayList<Polygon> polygonSet = new ArrayList<Polygon>(this.polygons.size());
             for (BasePolygonBuilder<?> polygon : this.polygons) {
                 for(Coordinate[][] part : polygon.coordinates()) {
-                    polygonSet.add(PolygonBuilder.polygon(FACTORY, part));
+                    shapes.add(jtsGeometry(PolygonBuilder.polygon(FACTORY, part)));
                 }
             }
-
-            polygons = polygonSet.toArray(new Polygon[polygonSet.size()]);
         } else {
-            polygons = new Polygon[this.polygons.size()];
+            for (BasePolygonBuilder<?> polygon : this.polygons) {
-            Iterator<BasePolygonBuilder<?>> iterator = this.polygons.iterator();
+                shapes.add(jtsGeometry(polygon.toPolygon(FACTORY)));
-            for (int i = 0; iterator.hasNext(); i++) {
-                polygons[i] = iterator.next().toPolygon(FACTORY);
             }
         }
-
+        if (shapes.size() == 1)
-        Geometry geometry = polygons.length == 1
+            return shapes.get(0);
-                ? polygons[0]
+        else
-                : FACTORY.createMultiPolygon(polygons);
+            return new ShapeCollection<Shape>(shapes, SPATIAL_CONTEXT);
-
+        //note: ShapeCollection is probably faster than a Multi* geom.
-        return new JtsGeometry(geometry, SPATIAL_CONTEXT, !wrapdateline);
     }
 
     public static class InternalPolygonBuilder extends BasePolygonBuilder<InternalPolygonBuilder> {

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

@@ -19,6 +19,8 @@
 
 package org.elasticsearch.common.geo.builders;
 
+import com.spatial4j.core.shape.jts.JtsGeometry;
+import com.vividsolutions.jts.geom.Geometry;
 import org.elasticsearch.ElasticsearchIllegalArgumentException;
 import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.logging.ESLogger;
@@ -54,10 +56,21 @@ public abstract class ShapeBuilder implements ToXContent {
     }
 
     public static final double DATELINE = 180;
-    public static final GeometryFactory FACTORY = new GeometryFactory();
+    // TODO how might we use JtsSpatialContextFactory to configure the context (esp. for non-geo)?
-    public static final JtsSpatialContext SPATIAL_CONTEXT = new JtsSpatialContext(true);
+    public static final JtsSpatialContext SPATIAL_CONTEXT = JtsSpatialContext.GEO;
+    public static final GeometryFactory FACTORY = SPATIAL_CONTEXT.getGeometryFactory();
 
-    protected final boolean wrapdateline = true;
+    /** We're expecting some geometries might cross the dateline. */
+    protected final boolean wrapdateline = SPATIAL_CONTEXT.isGeo();
+
+    /** It's possible that some geometries in a MULTI* shape might overlap. With the possible exception of GeometryCollection,
+     * this normally isn't allowed.
+     */
+    protected final boolean multiPolygonMayOverlap = false;
+    /** @see com.spatial4j.core.shape.jts.JtsGeometry#validate() */
+    protected final boolean autoValidateJtsGeometry = true;
+    /** @see com.spatial4j.core.shape.jts.JtsGeometry#index() */
+    protected final boolean autoIndexJtsGeometry = true;//may want to turn off once SpatialStrategy impls do it.
 
     protected ShapeBuilder() {
 
@@ -67,6 +80,16 @@ public abstract class ShapeBuilder implements ToXContent {
         return new Coordinate(longitude, latitude);
     }
 
+    protected JtsGeometry jtsGeometry(Geometry geom) {
+        //dateline180Check is false because ElasticSearch does it's own dateline wrapping
+        JtsGeometry jtsGeometry = new JtsGeometry(geom, SPATIAL_CONTEXT, false, multiPolygonMayOverlap);
+        if (autoValidateJtsGeometry)
+            jtsGeometry.validate();
+        if (autoIndexJtsGeometry)
+            jtsGeometry.index();
+        return jtsGeometry;
+    }
+
     /**
      * Create a new point
      * 

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

@@ -20,6 +20,7 @@
 package org.elasticsearch.common.geo;
 
 import com.spatial4j.core.shape.Shape;
+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.*;
@@ -33,15 +34,18 @@ import org.junit.Test;
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 
+import static org.elasticsearch.common.geo.builders.ShapeBuilder.SPATIAL_CONTEXT;
+
 
 /**
  * Tests for {@link GeoJSONShapeParser}
  */
 public class GeoJSONShapeParserTests extends ElasticsearchTestCase {
 
-    private final static GeometryFactory GEOMETRY_FACTORY = new GeometryFactory();
+    private final static GeometryFactory GEOMETRY_FACTORY = SPATIAL_CONTEXT.getGeometryFactory();
 
     @Test
     public void testParse_simplePoint() throws IOException {
@@ -50,7 +54,7 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase {
                 .endObject().string();
 
         Point expected = GEOMETRY_FACTORY.createPoint(new Coordinate(100.0, 0.0));
-        assertGeometryEquals(new JtsPoint(expected, ShapeBuilder.SPATIAL_CONTEXT), pointGeoJson);
+        assertGeometryEquals(new JtsPoint(expected, SPATIAL_CONTEXT), pointGeoJson);
     }
 
     @Test
@@ -68,7 +72,7 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase {
 
         LineString expected = GEOMETRY_FACTORY.createLineString(
                 lineCoordinates.toArray(new Coordinate[lineCoordinates.size()]));
-        assertGeometryEquals(new JtsGeometry(expected, ShapeBuilder.SPATIAL_CONTEXT, false), lineGeoJson);
+        assertGeometryEquals(jtsGeom(expected), lineGeoJson);
     }
 
     @Test
@@ -94,7 +98,7 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase {
 
         LinearRing shell = GEOMETRY_FACTORY.createLinearRing(shellCoordinates.toArray(new Coordinate[shellCoordinates.size()]));
         Polygon expected = GEOMETRY_FACTORY.createPolygon(shell, null);
-        assertGeometryEquals(new JtsGeometry(expected, ShapeBuilder.SPATIAL_CONTEXT, false), polygonGeoJson);
+        assertGeometryEquals(jtsGeom(expected), polygonGeoJson);
     }
 
     @Test
@@ -138,7 +142,7 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase {
         holes[0] = GEOMETRY_FACTORY.createLinearRing(
                 holeCoordinates.toArray(new Coordinate[holeCoordinates.size()]));
         Polygon expected = GEOMETRY_FACTORY.createPolygon(shell, holes);
-        assertGeometryEquals(new JtsGeometry(expected, ShapeBuilder.SPATIAL_CONTEXT, false), polygonGeoJson);
+        assertGeometryEquals(jtsGeom(expected), polygonGeoJson);
     }
 
     @Test
@@ -150,20 +154,17 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase {
                 .endArray()
                 .endObject().string();
 
-        List<Coordinate> multiPointCoordinates = new ArrayList<Coordinate>();
+        ShapeCollection expected = shapeCollection(
-        multiPointCoordinates.add(new Coordinate(100, 0));
+                SPATIAL_CONTEXT.makePoint(100, 0),
-        multiPointCoordinates.add(new Coordinate(101, 1));
+                SPATIAL_CONTEXT.makePoint(101, 1.0));
-
+        assertGeometryEquals(expected, multiPointGeoJson);
-        MultiPoint expected = GEOMETRY_FACTORY.createMultiPoint(
-                multiPointCoordinates.toArray(new Coordinate[multiPointCoordinates.size()]));
-        assertGeometryEquals(new JtsGeometry(expected, ShapeBuilder.SPATIAL_CONTEXT, false), multiPointGeoJson);
     }
 
     @Test
     public void testParse_multiPolygon() throws IOException {
         String multiPolygonGeoJson = XContentFactory.jsonBuilder().startObject().field("type", "MultiPolygon")
                 .startArray("coordinates")
-                .startArray()
+                .startArray()//first poly (without holes)
                 .startArray()
                 .startArray().value(102.0).value(2.0).endArray()
                 .startArray().value(103.0).value(2.0).endArray()
@@ -172,7 +173,7 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase {
                 .startArray().value(102.0).value(2.0).endArray()
                 .endArray()
                 .endArray()
-                .startArray()
+                .startArray()//second poly (with hole)
                 .startArray()
                 .startArray().value(100.0).value(0.0).endArray()
                 .startArray().value(101.0).value(0.0).endArray()
@@ -180,7 +181,7 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase {
                 .startArray().value(100.0).value(1.0).endArray()
                 .startArray().value(100.0).value(0.0).endArray()
                 .endArray()
-                .startArray()
+                .startArray()//hole
                 .startArray().value(100.2).value(0.8).endArray()
                 .startArray().value(100.2).value(0.2).endArray()
                 .startArray().value(100.8).value(0.2).endArray()
@@ -221,9 +222,9 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase {
         shell = GEOMETRY_FACTORY.createLinearRing(shellCoordinates.toArray(new Coordinate[shellCoordinates.size()]));
         Polygon withoutHoles = GEOMETRY_FACTORY.createPolygon(shell, null);
 
-        MultiPolygon expected = GEOMETRY_FACTORY.createMultiPolygon(new Polygon[] {withoutHoles, withHoles});
+        Shape expected = shapeCollection(withoutHoles, withHoles);
 
-        assertGeometryEquals(new JtsGeometry(expected, ShapeBuilder.SPATIAL_CONTEXT, false), multiPolygonGeoJson);
+        assertGeometryEquals(expected, multiPolygonGeoJson);
     }
 
     @Test
@@ -244,13 +245,29 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase {
                 .endObject().string();
 
         Point expected = GEOMETRY_FACTORY.createPoint(new Coordinate(100.0, 0.0));
-        assertGeometryEquals(new JtsPoint(expected, ShapeBuilder.SPATIAL_CONTEXT), pointGeoJson);
+        assertGeometryEquals(new JtsPoint(expected, SPATIAL_CONTEXT), pointGeoJson);
     }
 
     private void assertGeometryEquals(Shape expected, String geoJson) throws IOException {
         XContentParser parser = JsonXContent.jsonXContent.createParser(geoJson);
         parser.nextToken();
-        ElasticsearchGeoAssertions.assertEquals(ShapeBuilder.parse(parser).build(), expected);
+        ElasticsearchGeoAssertions.assertEquals(expected, ShapeBuilder.parse(parser).build());
+    }
+
+    private ShapeCollection<Shape> shapeCollection(Shape... shapes) {
+        return new ShapeCollection<Shape>(Arrays.asList(shapes), SPATIAL_CONTEXT);
+    }
+
+    private ShapeCollection<Shape> shapeCollection(Geometry... geoms) {
+        List<Shape> shapes = new ArrayList<Shape>(geoms.length);
+        for (Geometry geom : geoms) {
+            shapes.add(jtsGeom(geom));
+        }
+        return new ShapeCollection<Shape>(shapes, SPATIAL_CONTEXT);
+    }
+
+    private JtsGeometry jtsGeom(Geometry geom) {
+        return new JtsGeometry(geom, SPATIAL_CONTEXT, false, false);
     }
 
 }

src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java

@@ -19,7 +19,9 @@
 
 package org.elasticsearch.test.hamcrest;
 
+import com.carrotsearch.randomizedtesting.RandomizedTest;
 import com.spatial4j.core.shape.Shape;
+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.*;
@@ -173,6 +175,13 @@ public class ElasticsearchGeoAssertions {
         assertEquals(g1.getGeom(), g2.getGeom());
     }
 
+    public static void assertEquals(ShapeCollection s1, ShapeCollection s2) {
+        Assert.assertEquals(s1.size(), s2.size());
+        for (int i = 0; i < s1.size(); i++) {
+            assertEquals(s1.get(i), s2.get(i));
+        }
+    }
+
     public static void assertEquals(Shape s1, Shape s2) {
         if(s1 instanceof JtsGeometry && s2 instanceof JtsGeometry) {
             assertEquals((JtsGeometry) s1, (JtsGeometry) s2);
@@ -180,8 +189,13 @@ public class ElasticsearchGeoAssertions {
             JtsPoint p1 = (JtsPoint) s1;
             JtsPoint p2 = (JtsPoint) s2;
             Assert.assertEquals(p1, p2);
+        } else if (s1 instanceof ShapeCollection && s2 instanceof ShapeCollection) {
+            assertEquals((ShapeCollection)s1, (ShapeCollection)s2);
         } else {
-            throw new RuntimeException("equality of shape types not supported [" + s1.getClass().getName() + " and " + s2.getClass().getName() + "]");
+            //We want to know the type of the shape because we test shape equality in a special way...
+            //... in particular we test that one ring is equivalent to another ring even if the points are rotated or reversed.
+            throw new RuntimeException(
+                    "equality of shape types not supported [" + s1.getClass().getName() + " and " + s2.getClass().getName() + "]");
         }
     }