Fix hole intersection at tangential coordinate

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

@@ -19,15 +19,20 @@
 
 package org.elasticsearch.common.geo.builders;
 
+import com.google.common.collect.Lists;
+import com.google.common.collect.Sets;
+import com.spatial4j.core.exception.InvalidShapeException;
 import com.spatial4j.core.shape.Shape;
 import com.vividsolutions.jts.geom.*;
-import org.elasticsearch.ElasticsearchParseException;
+import org.apache.commons.lang3.tuple.Pair;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.HashSet;
 import java.util.Iterator;
+import java.util.WeakHashMap;
 
 /**
  * The {@link BasePolygonBuilder} implements the groundwork to create polygons. This contains
@@ -111,6 +116,18 @@ public ShapeBuilder close() {
         return shell.close();
     }
 
+    /**
+     * Validates only 1 vertex is tangential (shared) between the interior and exterior of a polygon
+     */
+    protected void validateHole(BaseLineStringBuilder shell, BaseLineStringBuilder hole) {
+        HashSet exterior = Sets.newHashSet(shell.points);
+        HashSet interior = Sets.newHashSet(hole.points);
+        exterior.retainAll(interior);
+        if (exterior.size() >= 2) {
+            throw new InvalidShapeException("Invalid polygon, interior cannot share more than one point");
+        }
+    }
+
     /**
      * The coordinates setup by the builder will be assembled to a polygon. The result will consist of
      * a set of polygons. Each of these components holds a list of linestrings defining the polygon: the
@@ -125,6 +142,7 @@ public Coordinate[][][] coordinates() {
         int numEdges = shell.points.size()-1; // Last point is repeated 
         for (int i = 0; i < holes.size(); i++) {
             numEdges += holes.get(i).points.size()-1;
+            validateHole(shell, this.holes.get(i));
         }
 
         Edge[] edges = new Edge[numEdges];
@@ -253,28 +271,62 @@ private static int component(final Edge edge, final int id, final ArrayList<Edge
             }
         }
 
-        double shift = any.coordinate.x > DATELINE ? DATELINE : (any.coordinate.x < -DATELINE ? -DATELINE : 0);
+        double shiftOffset = any.coordinate.x > DATELINE ? DATELINE : (any.coordinate.x < -DATELINE ? -DATELINE : 0);
         if (debugEnabled()) {
-            LOGGER.debug("shift: {[]}", shift);
+            LOGGER.debug("shift: {[]}", shiftOffset);
         }
 
         // run along the border of the component, collect the
         // edges, shift them according to the dateline and
         // update the component id
-        int length = 0;
+        int length = 0, connectedComponents = 0;
+        // if there are two connected components, splitIndex keeps track of where to split the edge array
+        // start at 1 since the source coordinate is shared
+        int splitIndex = 1;
         Edge current = edge;
+        Edge prev = edge;
+        // bookkeep the source and sink of each visited coordinate
+        WeakHashMap<Coordinate, Pair<Edge, Edge>> visitedEdge = new WeakHashMap<>();
         do {
-
-            current.coordinate = shift(current.coordinate, shift); 
+            current.coordinate = shift(current.coordinate, shiftOffset);
             current.component = id;
-            if(edges != null) {
+
+            if (edges != null) {
+                // found a closed loop - we have two connected components so we need to slice into two distinct components
+                if (visitedEdge.containsKey(current.coordinate)) {
+                    if (connectedComponents > 0 && current.next != edge) {
+                        throw new InvalidShapeException("Shape contains more than one tangential point");
+                    }
+
+                    // a negative id flags the edge as visited for the edges(...) method.
+                    // since we're splitting connected components, we want the edges method to visit
+                    // the newly separated component
+                    final int visitID = -id;
+                    Edge firstAppearance = visitedEdge.get(current.coordinate).getRight();
+                    // correct the graph pointers by correcting the 'next' pointer for both the
+                    // first appearance and this appearance of the edge
+                    Edge temp = firstAppearance.next;
+                    firstAppearance.next = current.next;
+                    current.next = temp;
+                    current.component = visitID;
+                    // backtrack until we get back to this coordinate, setting the visit id to
+                    // a non-visited value (anything positive)
+                    do {
+                        prev.component = visitID;
+                        prev = visitedEdge.get(prev.coordinate).getLeft();
+                        ++splitIndex;
+                    } while (!current.coordinate.equals(prev.coordinate));
+                    ++connectedComponents;
+                } else {
+                    visitedEdge.put(current.coordinate, Pair.of(prev, current));
+                }
                 edges.add(current);
+                prev = current;
             }
-
             length++;
-        } while((current = current.next) != edge);
+        } while(connectedComponents == 0 && (current = current.next) != edge);
 
-        return length;
+        return (splitIndex != 1) ? length-splitIndex: length;
     }
 
     /**
@@ -364,11 +416,12 @@ private static void assign(Edge[] holes, Coordinate[][] points, int numHoles, Ed
             // if no intersection is found then the hole is not within the polygon, so
             // don't waste time calling a binary search
             final int pos;
-            if (intersections == 0 ||
-               (pos = Arrays.binarySearch(edges, 0, intersections, current, INTERSECTION_ORDER)) >= 0) {
-                throw new ElasticsearchParseException("Invalid shape: Hole is not within polygon");
+            boolean sharedVertex = false;
+            if (intersections == 0 || ((pos = Arrays.binarySearch(edges, 0, intersections, current, INTERSECTION_ORDER)) >= 0)
+                            && !(sharedVertex = (edges[pos].intersect.compareTo(current.coordinate) == 0)) ) {
+                throw new InvalidShapeException("Invalid shape: Hole is not within polygon");
             }
-            final int index = -(pos+2);
+            final int index = -((sharedVertex) ? 0 : pos+2);
             final int component = -edges[index].component - numHoles - 1;
 
             if(debugEnabled()) {
@@ -465,7 +518,7 @@ private static int createEdges(int component, Orientation orientation, BaseLineS
                                    Edge[] edges, int offset) {
         // inner rings (holes) have an opposite direction than the outer rings
         // XOR will invert the orientation for outer ring cases (Truth Table:, T/T = F, T/F = T, F/T = T, F/F = F)
-        boolean direction = (component != 0 ^ orientation == Orientation.RIGHT);
+        boolean direction = (component == 0 ^ orientation == Orientation.RIGHT);
         // set the points array accordingly (shell or hole)
         Coordinate[] points = (hole != null) ? hole.coordinates(false) : shell.coordinates(false);
         Edge.ring(component, direction, orientation == Orientation.LEFT, shell, points, 0, edges, offset, points.length-1);

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

@@ -20,6 +20,7 @@
 package org.elasticsearch.common.geo.builders;
 
 import com.spatial4j.core.context.jts.JtsSpatialContext;
+import com.spatial4j.core.exception.InvalidShapeException;
 import com.spatial4j.core.shape.Shape;
 import com.spatial4j.core.shape.jts.JtsGeometry;
 import com.vividsolutions.jts.geom.Coordinate;
@@ -446,7 +447,8 @@ protected static final class Edge {
 
         protected Edge(Coordinate coordinate, Edge next, Coordinate intersection) {
             this.coordinate = coordinate;
-            this.next = next;
+            // use setter to catch duplicate point cases
+            this.setNext(next);
             this.intersect = intersection;
             if (next != null) {
                 this.component = next.component;
@@ -457,6 +459,17 @@ protected Edge(Coordinate coordinate, Edge next) {
             this(coordinate, next, Edge.MAX_COORDINATE);
         }
 
+        protected void setNext(Edge next) {
+            // don't bother setting next if its null
+            if (next != null) {
+                // self-loop throws an invalid shape
+                if (this.coordinate.equals(next.coordinate)) {
+                    throw new InvalidShapeException("Provided shape has duplicate consecutive coordinates at: " + this.coordinate);
+                }
+                this.next = next;
+            }
+        }
+
         private static final int top(Coordinate[] points, int offset, int length) {
             int top = 0; // we start at 1 here since top points to 0
             for (int i = 1; i < length; i++) {
@@ -522,17 +535,19 @@ private static Edge[] concat(int component, boolean direction, Coordinate[] poin
                 if (direction) {
                     edges[edgeOffset + i] = new Edge(points[pointOffset + i], edges[edgeOffset + i - 1]);
                     edges[edgeOffset + i].component = component;
-                } else {
+                } else if(!edges[edgeOffset + i - 1].coordinate.equals(points[pointOffset + i])) {
                     edges[edgeOffset + i - 1].next = edges[edgeOffset + i] = new Edge(points[pointOffset + i], null);
                     edges[edgeOffset + i - 1].component = component;
+                } else {
+                    throw new InvalidShapeException("Provided shape has duplicate consecutive coordinates at: " + points[pointOffset + i]);
                 }
             }
 
             if (direction) {
-                edges[edgeOffset].next = edges[edgeOffset + length - 1];
+                edges[edgeOffset].setNext(edges[edgeOffset + length - 1]);
                 edges[edgeOffset].component = component;
             } else {
-                edges[edgeOffset + length - 1].next = edges[edgeOffset];
+                edges[edgeOffset + length - 1].setNext(edges[edgeOffset]);
                 edges[edgeOffset + length - 1].component = component;
             }
 

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

@@ -267,7 +267,7 @@ public void testParse_invalidMultiPolygon() throws IOException {
 
         XContentParser parser = JsonXContent.jsonXContent.createParser(multiPolygonGeoJson);
         parser.nextToken();
-        ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class);
+        ElasticsearchGeoAssertions.assertValidException(parser, InvalidShapeException.class);
     }
 
     public void testParse_OGCPolygonWithoutHoles() throws IOException {