Merge branch '7.17'

Bapt Abl · Bapt Abl · commit 6dddfdcfa2db · 2022-08-25T16:29:32.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -0,0 +1,5 @@
+### 7.17.1.1
+
+* Fix deduplication of points
+* Fix handling of GeometryCollections
+* Add tests
diff --git a/gradle.properties b/gradle.properties
@@ -1,2 +1,2 @@
 elastic_version = 7.17.1
-plugin_version = 7.17.1.0
+plugin_version = 7.17.1.1
diff --git a/src/main/java/org/opendatasoft/elasticsearch/ingest/GeoExtensionProcessor.java b/src/main/java/org/opendatasoft/elasticsearch/ingest/GeoExtensionProcessor.java
@@ -19,11 +19,12 @@
 import org.locationtech.jts.geom.Coordinate;
 import org.locationtech.jts.geom.Geometry;
 import org.locationtech.jts.geom.GeometryFactory;
-import org.locationtech.jts.geom.Point;
 import org.locationtech.jts.geom.PrecisionModel;
 import org.locationtech.jts.io.ParseException;
 import org.locationtech.jts.io.WKBWriter;
+import org.locationtech.jts.io.WKTReader;
 import org.locationtech.jts.io.WKTWriter;
+import org.locationtech.spatial4j.exception.InvalidShapeException;
 import org.locationtech.spatial4j.shape.Shape;
 import org.locationtech.spatial4j.shape.jts.JtsGeometry;
 import org.locationtech.spatial4j.shape.jts.JtsPoint;
@@ -50,6 +51,10 @@ public class GeoExtensionProcessor extends AbstractProcessor {
     private final String bboxField;
     private final String centroidField;
 
+    private final GeometryFactory geomFactory;
+    private final WKTWriter wktWriter;
+    private final WKTReader wktReader;
+
     private GeoExtensionProcessor(String tag, String description, String field, String path, Boolean keepShape, String shapeField,
                                   String fixedField, String wkbField, String hashField, String typeField,
                                   String areaField, String bboxField, String centroidField)  {
@@ -65,6 +70,12 @@ private GeoExtensionProcessor(String tag, String description, String field, Stri
         this.areaField = areaField;
         this.bboxField = bboxField;
         this.centroidField = centroidField;
+
+        PrecisionModel precisionModel = new PrecisionModel(PrecisionModel.FLOATING);
+        this.geomFactory = new GeometryFactory(precisionModel, 0);
+
+        this.wktWriter = new WKTWriter();
+        this.wktReader = new WKTReader();
     }
 
     @SuppressWarnings("unchecked")
@@ -102,6 +113,72 @@ private List<String> getGeoShapeFieldsFromDoc(IngestDocument ingestDocument) {
         return ShapeParser.parse(parser);
     }
 
+    private class GeometryWithWkt {
+        public Geometry geometry;
+        public String wkt;
+    }
+
+    private GeometryWithWkt s4jToJts(Shape shape) {
+        // Convert a Spatial4J shape into a JTSGeometry
+        // A Spatial4J shape is basically either:
+        // - a JtsPoint
+        // - a JtsGeometry
+        // - an XShapeCollection (multi-* and GeometryCollection)
+        // There is a special case for MultiPoint since the WKTWriter of JTS
+        // is not compliant with the wkt reader of ES.
+
+        GeometryWithWkt geomWithWkt = new GeometryWithWkt();
+        Geometry geom = null;
+
+        String altWKT = null;
+        if (shape instanceof JtsPoint) {
+            geom = ((JtsPoint) shape).getGeom();
+        } else if (shape instanceof JtsGeometry) {
+            geom = ((JtsGeometry) shape).getGeom();
+        } else if (shape instanceof XShapeCollection) {
+            XShapeCollection collection = (XShapeCollection) shape;
+            ArrayList<Geometry> geoms = new ArrayList<>(collection.size());
+            ArrayList<String> wkts = new ArrayList<>(collection.size());
+
+            boolean hasWkt = false;
+            for (int i = 0; i < collection.size(); i++) {
+                GeometryWithWkt g = s4jToJts(collection.get(i));
+                geoms.add(g.geometry);
+                wkts.add(g.wkt);
+                if (g.wkt != null) {
+                    hasWkt = true;
+                }
+            }
+
+            if (hasWkt) {
+                // if it has a WKT, it means it is a geometrycolleciton
+                // containing a multipoint
+                altWKT = "GEOMETRYCOLLECTION(";
+                for (int i = 0; i < collection.size(); i++) {
+                    if (wkts.get(i) != null) {
+                        altWKT += wkts.get(i);
+                    } else {
+                        altWKT += wktWriter.write(geoms.get(i));
+                    }
+                    if (i < collection.size() - 1) {
+                        altWKT += ",";
+                    }
+                }
+                altWKT += ")";
+            }
+            geom = geomFactory.buildGeometry(geoms);
+
+            if (geom.getGeometryType() == "MultiPoint") {
+                // special case of multipoint where the WKT must be corrected
+                // ES wants multipoint without extra parenthesis between points
+                altWKT = wktWriter.write(geom).replace("((", "(").replace("))", ")").replace("), (", ", ");
+            }
+        }
+        geomWithWkt.geometry = geom;
+        geomWithWkt.wkt = altWKT;
+        return geomWithWkt;
+    }
+
     @SuppressWarnings("unchecked")
     @Override
     public IngestDocument execute(IngestDocument ingestDocument) throws IOException, ParseException {
@@ -114,47 +191,50 @@ public IngestDocument execute(IngestDocument ingestDocument) throws IOException,
                 continue;
             }
 
+            // From an input Object that represents a GeoJSON we need to:
+            // - store a WKT version of the geometry, that will also be indexed as a geometry by ES
+            // - make sure this WKT does not have duplicated points, since ES refuse them
+            // - make sure this WKT geometry is equal (or close enough) to the geometry indexed by ES (why ?).
+            //   It means if ES splits the geometry around the dateline, we want to do the same
+            // - compute its area
+            // - compute its WKB representation
+            // - compute its centroid
+            //
+            // We currently do this by:
+            // 1/ creating a Spatial4J shape from an Object (thanks to legacygeo.ShapeBuilder)
+            // 2/ converting this Spatial4J shape to a JTSGeometry
+            // 3/ call area(), wkbwriter(), centroid() and wktwriter on the JTSGeometry
+            //
+            // 2/ could be avoided but the S4J WKTWriter is of poor quality (e.g. a multipoint can be represented as a geometrycollection of points)
+            // 1/ could be avoided if we find how to create a JTSGeometry from an Object
+
             ShapeBuilder<?,?, ?> shapeBuilder = getShapeBuilderFromObject(geoShapeObject);
 
             // buildS4J() will try to clean up and fix the shape. If it fails, an exception is raised
             // Included fixes:
-            // - point deduplication
             // - dateline warping (enforce lon in [-180,180])
-            Shape shape = shapeBuilder.buildS4J();
-
-            Geometry geom = null;
-            PrecisionModel precisionModel = new PrecisionModel(PrecisionModel.FLOATING);
-            GeometryFactory geomFactory = new GeometryFactory(precisionModel, 0);
-            String altWKT = null;
-            if (shape instanceof JtsPoint) {
-                geom = ((JtsPoint)shape).getGeom();
-            }
-            else if (shape instanceof JtsGeometry) {
-                geom = ((JtsGeometry) shape).getGeom();
-            }
-            else if (shape instanceof XShapeCollection) {
-                List<?> shapes = ((XShapeCollection) shape).getShapes();
-                switch (shapeBuilder.type()) {
-                    case MULTIPOINT:
-                        Point[] points = new Point[shapes.size()];
-                        for (int i = 0; i < shapes.size(); i++) {
-                            points[i] = (Point)((JtsPoint)(shapes.get(i))).getGeom();
-                        }
-                        geom = geomFactory.createMultiPoint(points);
-                        // ES wants multipoint without extra parenthesis between points
-                        altWKT = new WKTWriter().write(geom).replace("((","(").replace("))",")").replace("), (",", ");
-                        break;
-                    case MULTILINESTRING:
-                    case MULTIPOLYGON:
-                    case GEOMETRYCOLLECTION:
-                        ArrayList<Geometry> geoms = new ArrayList<>(shapes.size());
-                        for (int i = 0; i < shapes.size(); i++) {
-                            geoms.add(((JtsGeometry)(shapes.get(i))).getGeom());
-                        }
-                        geom = geomFactory.buildGeometry(geoms);
-                        break;
+            Shape shape = null;
+            try {
+                try {
+                    shape = shapeBuilder.buildS4J();
+                } catch (InvalidShapeException e) {
+                    // buildS4J does not always deduplicate points
+                    shapeBuilder = GeoUtils.removeDuplicateCoordinates(shapeBuilder);
+                    shape = shapeBuilder.buildS4J();
                 }
             }
+            catch (Throwable e) {
+                // sometimes it is still not enough
+                // e.g. when non-EPSG:4326 coordinates are input
+                // buildS4J will try to warp date lines and may generate lots of small components
+                // which could yield a GeometryCollection, which raises an AssertionError
+                // So, we catch here both Error (AssertionError) and Exceptions
+                throw new IllegalArgumentException("Unable to parse shape [" + shapeBuilder.toWKT() + "]: " + e.getMessage());
+            }
+
+            GeometryWithWkt geometryWithWkt = s4jToJts(shape);
+            Geometry geom = geometryWithWkt.geometry;
+            String altWKT = geometryWithWkt.wkt;
 
             if (geom == null) {
                 throw new IllegalArgumentException("Unable to parse shape [" + shapeBuilder.toWKT() + "]");
@@ -168,7 +248,7 @@ else if (shape instanceof XShapeCollection) {
 
             if (fixedField != null) {
                 ingestDocument.setFieldValue(geoShapeField + "." + fixedField,
-                        altWKT != null ? altWKT : new WKTWriter().write(geom));
+                        altWKT != null ? altWKT : wktWriter.write(geom));
             }
 
             // compute and add extra geo sub-fields
diff --git a/src/main/java/org/opendatasoft/elasticsearch/plugin/GeoUtils.java b/src/main/java/org/opendatasoft/elasticsearch/plugin/GeoUtils.java
@@ -2,7 +2,14 @@
 
 import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.common.geo.GeoPoint;
+import org.elasticsearch.common.geo.Orientation;
 import org.elasticsearch.common.hash.MurmurHash3;
+import org.elasticsearch.geometry.Point;
+import org.elasticsearch.legacygeo.builders.GeometryCollectionBuilder;
+import org.elasticsearch.legacygeo.builders.LineStringBuilder;
+import org.elasticsearch.legacygeo.builders.MultiPolygonBuilder;
+import org.elasticsearch.legacygeo.builders.PolygonBuilder;
+import org.elasticsearch.legacygeo.builders.ShapeBuilder;
 import org.locationtech.jts.geom.Coordinate;
 import org.locationtech.jts.geom.Geometry;
 import org.locationtech.jts.geom.GeometryCollection;
@@ -13,9 +20,16 @@
 import org.locationtech.jts.io.WKBWriter;
 import org.locationtech.jts.io.WKTWriter;
 import org.locationtech.jts.io.geojson.GeoJsonWriter;
+import org.elasticsearch.geometry.Line;
 
+import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.Iterator;
 import java.util.List;
+import java.util.ListIterator;
+import java.util.Vector;
 
 public class GeoUtils {
     public enum OutputFormat {
@@ -112,23 +126,62 @@ public static String exportGeoTo(Geometry geom, OutputFormat outputFormat, GeoJs
         }
     }
 
-    public static Geometry removeDuplicateCoordinates(Geometry geom) {
-        if (geom.isEmpty()) {
-            return geom;
+    public static LineStringBuilder removeDuplicateCoordinates(LineStringBuilder builder) {
+        Line line = (Line)builder.buildGeometry(); // no direct access to coordinates
+        Vector<Coordinate> newCoordinates = new Vector<Coordinate>();
+
+        Point previous = null;
+        for (int i = 0; i < line.length(); i++) {
+            Point current = new Point(line.getX(i), line.getY(i));
+            if ((previous != null) && (previous.equals(current))) {
+                continue;
+            }
+            newCoordinates.add(new Coordinate(current.getX(), current.getY()));
+            previous = current;
         }
+        return new LineStringBuilder(newCoordinates);
+    }
 
-        if (geom instanceof Polygon) {
-            return removeDuplicateCoordinates((Polygon) geom);
+    public static PolygonBuilder removeDuplicateCoordinates(PolygonBuilder builder) {
+        LineStringBuilder exteriorBuilder = removeDuplicateCoordinates(builder.shell());
+        PolygonBuilder pb = new PolygonBuilder(exteriorBuilder, /* unused */Orientation.RIGHT, true);
+        List<LineStringBuilder> holes = builder.holes();
+        for (int i = 0; i < holes.size(); i++) {
+            pb.hole(removeDuplicateCoordinates(holes.get(i)), true);
         }
+        return pb;
+    }
 
-        if (geom instanceof MultiPolygon) {
-            return removeDuplicateCoordinates((MultiPolygon) geom);
+    public static MultiPolygonBuilder removeDuplicateCoordinates(MultiPolygonBuilder builder) {
+        MultiPolygonBuilder mpb = new MultiPolygonBuilder();
+        List<PolygonBuilder> polygons = builder.polygons();
+        for (int i = 0; i < polygons.size(); i++) {
+            mpb.polygon(removeDuplicateCoordinates(polygons.get(i)));
         }
+        return mpb;
+    }
 
-        if (geom instanceof GeometryCollection) {
-            return removeDuplicateCoordinates((GeometryCollection) geom);
+    public static GeometryCollectionBuilder removeDuplicateCoordinates(GeometryCollectionBuilder builder) {
+        GeometryCollectionBuilder gcb = new GeometryCollectionBuilder();
+        for (int i = 0; i < builder.numShapes(); i++) {
+            gcb.shape(removeDuplicateCoordinates(builder.getShapeAt(i)));
         }
+        return gcb;
+    }
 
-        return geom;
+    public static ShapeBuilder removeDuplicateCoordinates(ShapeBuilder builder){
+        if (builder instanceof LineStringBuilder) {
+            return removeDuplicateCoordinates((LineStringBuilder)builder);
+        }
+        if (builder instanceof PolygonBuilder) {
+            return removeDuplicateCoordinates((PolygonBuilder)builder);
+        }
+        if (builder instanceof MultiPolygonBuilder) {
+            return removeDuplicateCoordinates((MultiPolygonBuilder)builder);
+        }
+        if (builder instanceof GeometryCollectionBuilder) {
+            return removeDuplicateCoordinates((GeometryCollectionBuilder) builder);
+        }
+        return builder;
     }
 }
diff --git a/src/yamlRestTest/resources/rest-api-spec/test/GeoExtension/20_geo_ingest_processor.yml b/src/yamlRestTest/resources/rest-api-spec/test/GeoExtension/20_geo_ingest_processor.yml

Original file line number	Diff line number	Diff line change
`@@ -1,2 +1,2 @@`
`1`	`1`	`elastic_version = 7.17.1`
`2`		`-plugin_version = 7.17.1.0`
	`2`	`+plugin_version = 7.17.1.1`