From a2d8b325c852210261a9751f4d0bcb33bf8802b5 Mon Sep 17 00:00:00 2001 From: David Turner Date: Tue, 5 Dec 2017 19:42:00 +0000 Subject: [PATCH 1/3] Handle case where the hole vertex is south of the containing polygon(s) 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 --- .../common/geo/builders/PolygonBuilder.java | 2 +- .../geo/builders/PolygonBuilderTests.java | 21 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java index ffcb44c9e4627..f62b78fce39f3 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java @@ -482,7 +482,7 @@ private static void assign(Edge[] holes, Coordinate[][] points, int numHoles, Ed && !(sharedVertex = (edges[pos].intersect.compareTo(current.coordinate) == 0)) ) { throw new InvalidShapeException("Invalid shape: Hole is not within polygon"); } - final int index = -((sharedVertex) ? 0 : pos+2); + final int index = sharedVertex || pos == -1 ? 0 : -(pos+2); final int component = -edges[index].component - numHoles - 1; if(debugEnabled()) { diff --git a/core/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java b/core/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java index 04122229bc0f3..daa0a5d56d366 100644 --- a/core/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java @@ -24,6 +24,7 @@ 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 +125,24 @@ public void testCoerceHole() { assertThat("hole should have been closed via coerce", pb.holes().get(0).coordinates(false).length, equalTo(4)); } + public void testHoleThatIsSouthOfPolygon() { + try { + 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(); + } catch (InvalidShapeException e) { + assertEquals("Hole lies outside shell at or near point (4.0, 1.0, NaN)", e.getMessage()); + } + } + + public void testHoleThatIsNorthOfPolygon() { + try { + 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(); + } catch (InvalidShapeException e) { + assertEquals("Hole lies outside shell at or near point (4.0, 3.0, NaN)", e.getMessage()); + } + } + } From dc88dace9b58e1157aab7c4ed621fb8aee296ba9 Mon Sep 17 00:00:00 2001 From: David Turner Date: Wed, 6 Dec 2017 08:54:04 +0000 Subject: [PATCH 2/3] Use expectThrows() --- .../geo/builders/PolygonBuilderTests.java | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/core/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java b/core/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java index daa0a5d56d366..8501760d1e772 100644 --- a/core/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java +++ b/core/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java @@ -20,7 +20,6 @@ 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; @@ -126,23 +125,22 @@ public void testCoerceHole() { } public void testHoleThatIsSouthOfPolygon() { - try { + 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(); - } catch (InvalidShapeException e) { - assertEquals("Hole lies outside shell at or near point (4.0, 1.0, NaN)", e.getMessage()); - } + }); + + assertEquals("Hole lies outside shell at or near point (4.0, 1.0, NaN)", e.getMessage()); } public void testHoleThatIsNorthOfPolygon() { - try { + 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(); - } catch (InvalidShapeException e) { - assertEquals("Hole lies outside shell at or near point (4.0, 3.0, NaN)", e.getMessage()); - } - } + }); + assertEquals("Hole lies outside shell at or near point (4.0, 3.0, NaN)", e.getMessage()); + } } From 50efd543bd3073c38df3cb8f7c243c95478629f7 Mon Sep 17 00:00:00 2001 From: David Turner Date: Fri, 15 Dec 2017 10:39:17 +0000 Subject: [PATCH 3/3] Add comments and split up some of the conditionals for clarity --- .../common/geo/builders/PolygonBuilder.java | 50 ++++++++++++++++--- 1 file changed, 43 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java b/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java index f62b78fce39f3..b0b37dbafa9a3 100644 --- a/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java +++ b/core/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java @@ -469,20 +469,56 @@ private static void assign(Edge[] holes, Coordinate[][] points, int numHoles, Ed 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 + + 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"); + } + + // 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 (intersections == 0 || ((pos = Arrays.binarySearch(edges, 0, intersections, current, INTERSECTION_ORDER)) >= 0) - && !(sharedVertex = (edges[pos].intersect.compareTo(current.coordinate) == 0)) ) { + 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 = sharedVertex || pos == -1 ? 0 : -(pos+2); + + 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()) {