Handle case where the hole vertex is south of the containing polygon(s) (#27685)

Normally the hole is assigned to the component of the first edge to the south
of one of its vertices, but if the chosen hole vertex is south of everything
then the binary search returns -1 yielding an ArrayIndexOutOfBoundsException.
Instead, assign the vertex to the component of the first edge to its north.
Subsequent validation catches the fact that the hole is outside its component.

Fixes #25933

core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java

@@ -469,20 +469,56 @@ public class PolygonBuilder extends ShapeBuilder<JtsGeometry, PolygonBuilder> {
             LOGGER.debug("Holes: {}", Arrays.toString(holes));
         }
         for (int i = 0; i < numHoles; i++) {
+            // To do the assignment we assume (and later, elsewhere, check) that each hole is within
+            // a single component, and the components do not overlap. Based on this assumption, it's
+            // enough to find a component that contains some vertex of the hole, and
+            // holes[i].coordinate is such a vertex, so we use that one.
+
+            // First, we sort all the edges according to their order of intersection with the line
+            // of longitude through holes[i].coordinate, in order from south to north. Edges that do
+            // not intersect this line are sorted to the end of the array and of no further interest
+            // here.
             final Edge current = new Edge(holes[i].coordinate, holes[i].next);
-            // the edge intersects with itself at its own coordinate.  We need intersect to be set this way so the binary search
-            // will get the correct position in the edge list and therefore the correct component to add the hole
             current.intersect = current.coordinate;
             final int intersections = intersections(current.coordinate.x, edges);
-            // 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;
-            boolean sharedVertex = false;
-            if (intersections == 0 || ((pos = Arrays.binarySearch(edges, 0, intersections, current, INTERSECTION_ORDER)) >= 0)
-                            && !(sharedVertex = (edges[pos].intersect.compareTo(current.coordinate) == 0)) ) {
+
+            if (intersections == 0) {
+                // There were no edges that intersect the line of longitude through
+                // holes[i].coordinate, so there's no way this hole is within the polygon.
                 throw new InvalidShapeException("Invalid shape: Hole is not within polygon");
             }
-            final int index = -((sharedVertex) ? 0 : pos+2);
+
+            // Next we do a binary search to find the position of holes[i].coordinate in the array.
+            // The binary search returns the index of an exact match, or (-insertionPoint - 1) if
+            // the vertex lies between the intersections of edges[insertionPoint] and
+            // edges[insertionPoint+1]. The latter case is vastly more common.
+
+            final int pos;
+            boolean sharedVertex = false;
+            if (((pos = Arrays.binarySearch(edges, 0, intersections, current, INTERSECTION_ORDER)) >= 0)
+                && !(sharedVertex = (edges[pos].intersect.compareTo(current.coordinate) == 0))) {
+                // The binary search returned an exact match, but we checked again using compareTo()
+                // and it didn't match after all.
+
+                // TODO Can this actually happen? Needs a test to exercise it, or else needs to be removed.
+                throw new InvalidShapeException("Invalid shape: Hole is not within polygon");
+            }
+
+            final int index;
+            if (sharedVertex) {
+                // holes[i].coordinate lies exactly on an edge.
+                index = 0; // TODO Should this be pos instead of 0? This assigns exact matches to the southernmost component.
+            } else if (pos == -1) {
+                // holes[i].coordinate is strictly south of all intersections. Assign it to the
+                // southernmost component, and allow later validation to spot that it is not
+                // entirely within the chosen component.
+                index = 0;
+            } else {
+                // holes[i].coordinate is strictly north of at least one intersection. Assign it to
+                // the component immediately to its south.
+                index = -(pos + 2);
+            }
+
             final int component = -edges[index].component - numHoles - 1;
 
             if(debugEnabled()) {

core/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java

@@ -20,10 +20,10 @@
 package org.elasticsearch.common.geo.builders;
 
 import com.vividsolutions.jts.geom.Coordinate;
-
 import org.elasticsearch.common.geo.builders.ShapeBuilder.Orientation;
 import org.elasticsearch.test.geo.RandomShapeGenerator;
 import org.elasticsearch.test.geo.RandomShapeGenerator.ShapeType;
+import org.locationtech.spatial4j.exception.InvalidShapeException;
 
 import java.io.IOException;
 
@@ -124,4 +124,23 @@ public class PolygonBuilderTests extends AbstractShapeBuilderTestCase<PolygonBui
         assertThat("hole should have been closed via coerce", pb.holes().get(0).coordinates(false).length, equalTo(4));
     }
 
+    public void testHoleThatIsSouthOfPolygon() {
+        InvalidShapeException e = expectThrows(InvalidShapeException.class, () -> {
+            PolygonBuilder pb = new PolygonBuilder(new CoordinatesBuilder().coordinate(4, 3).coordinate(3, 2).coordinate(3, 3).close());
+            pb.hole(new LineStringBuilder(new CoordinatesBuilder().coordinate(4, 2).coordinate(3, 1).coordinate(4, 1).close()));
+            pb.build();
+        });
+
+        assertEquals("Hole lies outside shell at or near point (4.0, 1.0, NaN)", e.getMessage());
+    }
+
+    public void testHoleThatIsNorthOfPolygon() {
+        InvalidShapeException e = expectThrows(InvalidShapeException.class, () -> {
+            PolygonBuilder pb = new PolygonBuilder(new CoordinatesBuilder().coordinate(3, 2).coordinate(4, 1).coordinate(3, 1).close());
+            pb.hole(new LineStringBuilder(new CoordinatesBuilder().coordinate(3, 3).coordinate(4, 2).coordinate(4, 3).close()));
+            pb.build();
+        });
+
+        assertEquals("Hole lies outside shell at or near point (4.0, 3.0, NaN)", e.getMessage());
+    }
 }