Browse Source

Fix infinite loop when polygonizing a circle with centre on the pole (#70875)

This PR prevents the algorithm to run on circles that contain a pole.
Ignacio Vera 4 years ago
parent
commit
a35563aaaf

+ 2 - 0
docs/reference/ingest/processors/circle.asciidoc

@@ -57,6 +57,8 @@ The circle can be represented as either a WKT circle or a GeoJSON circle. The re
 polygon will be represented and indexed using the same format as the input circle. WKT will
 be translated to a WKT polygon, and GeoJSON circles will be translated to GeoJSON polygons.
 
+IMPORTANT: Circles that contain a pole are not supported.
+
 ==== Example: Circle defined in Well Known Text
 
 In this example a circle defined in WKT format is indexed

+ 4 - 3
test/framework/src/main/java/org/elasticsearch/geo/GeometryTestUtils.java

@@ -46,11 +46,12 @@ public class GeometryTestUtils {
     }
 
     public static Circle randomCircle(boolean hasAlt) {
+        org.apache.lucene.geo.Circle luceneCircle = GeoTestUtil.nextCircle();
         if (hasAlt) {
-            return new Circle(randomLon(), randomLat(), ESTestCase.randomDouble(),
-                ESTestCase.randomDoubleBetween(0, 100, false));
+            return new Circle(luceneCircle.getLon(), luceneCircle.getLat(), ESTestCase.randomDouble(),
+                luceneCircle.getRadius());
         } else {
-            return new Circle(randomLon(), randomLat(), ESTestCase.randomDoubleBetween(0, 100, false));
+            return new Circle(luceneCircle.getLon(), luceneCircle.getLat(), luceneCircle.getRadius());
         }
     }
 

+ 11 - 0
x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/SpatialUtils.java

@@ -23,16 +23,27 @@ public class SpatialUtils {
      * Makes an n-gon, centered at the provided circle's center, and each vertex approximately
      * {@link Circle#getRadiusMeters()} away from the center.
      *
+     * It throws an IllegalArgumentException if the circle contains a pole.
+     *
      * This does not split the polygon across the date-line. Relies on {@link GeoShapeIndexer} to
      * split prepare polygon for indexing.
      *
      * Adapted from from org.apache.lucene.geo.GeoTestUtil
      * */
     public static Polygon createRegularGeoShapePolygon(Circle circle, int gons) {
+        if (SloppyMath.haversinMeters(circle.getLat(), circle.getLon(), 90, 0) < circle.getRadiusMeters()) {
+            throw new IllegalArgumentException("circle [" + circle.toString() + "] contains the north pole. " +
+                "It cannot be translated to a polygon");
+        }
+        if (SloppyMath.haversinMeters(circle.getLat(), circle.getLon(), -90, 0) < circle.getRadiusMeters()) {
+            throw new IllegalArgumentException("circle [" + circle.toString() + "] contains the south pole. " +
+                "It cannot be translated to a polygon");
+        }
         double[][] result = new double[2][];
         result[0] = new double[gons+1];
         result[1] = new double[gons+1];
         for(int i=0; i<gons; i++) {
+            // make sure we do not start at angle 0 or we have issues at the poles
             double angle = i * (360.0 / gons);
             double x = Math.cos(SloppyMath.toRadians(angle));
             double y = Math.sin(SloppyMath.toRadians(angle));

+ 27 - 5
x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/SpatialUtilsTests.java

@@ -7,21 +7,43 @@
 package org.elasticsearch.xpack.spatial;
 
 import org.apache.lucene.util.SloppyMath;
+import org.elasticsearch.geo.GeometryTestUtils;
 import org.elasticsearch.geometry.Circle;
 import org.elasticsearch.geometry.LinearRing;
 import org.elasticsearch.geometry.Polygon;
 import org.elasticsearch.test.ESTestCase;
 
 import static org.hamcrest.Matchers.closeTo;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 
 public class SpatialUtilsTests extends ESTestCase {
 
     public void testCreateRegularGeoShapePolygon() {
-        double lon = randomDoubleBetween(-20, 20, true);
-        double lat = randomDoubleBetween(-20, 20, true);
-        double radiusMeters = randomDoubleBetween(10, 10000, true);
-        Circle circle = new Circle(lon, lat, radiusMeters);
+        final Circle circle = randomValueOtherThanMany(
+            c -> SloppyMath.haversinMeters(c.getLat(), c.getLon(), 90, 0) < c.getRadiusMeters()
+                || SloppyMath.haversinMeters(c.getLat(), c.getLon(), -90, 0) < c.getRadiusMeters(),
+            () -> GeometryTestUtils.randomCircle(true));
+        doRegularGeoShapePolygon(circle);
+    }
+
+    public void testCircleContainsNorthPole() {
+        final Circle circle = randomValueOtherThanMany(
+            c -> SloppyMath.haversinMeters(c.getLat(), c.getLon(), 90, 0) >= c.getRadiusMeters(),
+            () -> GeometryTestUtils.randomCircle(true));
+        IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> doRegularGeoShapePolygon(circle));
+        assertThat(ex.getMessage(), containsString("contains the north pole"));
+    }
+
+    public void testCircleContainsSouthPole() {
+        final Circle circle = randomValueOtherThanMany(
+            c -> SloppyMath.haversinMeters(c.getLat(), c.getLon(), -90, 0) >= c.getRadiusMeters(),
+            () -> GeometryTestUtils.randomCircle(true));
+        IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> doRegularGeoShapePolygon(circle));
+        assertThat(ex.getMessage(), containsString("contains the south pole"));
+    }
+
+    private void doRegularGeoShapePolygon(Circle circle) {
         int numSides = randomIntBetween(4, 1000);
         Polygon polygon = SpatialUtils.createRegularGeoShapePolygon(circle, numSides);
         LinearRing outerShell = polygon.getPolygon();
@@ -35,7 +57,7 @@ public class SpatialUtilsTests extends ESTestCase {
         for (int i = 0; i < numPoints ; i++) {
             double actualDistance = SloppyMath
                 .haversinMeters(circle.getY(), circle.getX(), outerShell.getY(i), outerShell.getX(i));
-            assertThat(actualDistance, closeTo(radiusMeters, 0.1));
+            assertThat(actualDistance, closeTo(circle.getRadiusMeters(), 0.1));
         }
     }