From 8b57e2e5ba7de60f07d411fec0300cb2178790ba Mon Sep 17 00:00:00 2001
From: David Turner <david.turner@elastic.co>
Date: Tue, 31 Jul 2018 08:25:21 +0100
Subject: [PATCH] Fix calculation of orientation of polygons (#27967)

The method for working out whether a polygon is clockwise or anticlockwise is
mostly correct but doesn't work in some rare cases such as the included test
case. This commit fixes that.
---
 .../common/geo/builders/PolygonBuilder.java   | 41 +++++++++++++++----
 .../geo/builders/PolygonBuilderTests.java     | 18 ++++++++
 2 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java b/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java
index dbcb46d9936f3..e4d48b8df4566 100644
--- a/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java
+++ b/server/src/main/java/org/elasticsearch/common/geo/builders/PolygonBuilder.java
@@ -46,6 +46,8 @@
 import java.util.Objects;
 import java.util.concurrent.atomic.AtomicBoolean;
 
+import static org.apache.lucene.geo.GeoUtils.orient;
+
 /**
  * The {@link PolygonBuilder} implements the groundwork to create polygons. This contains
  * Methods to wrap polygons at the dateline and building shapes from the data held by the
@@ -642,14 +644,8 @@ private static int createEdges(int component, Orientation orientation, LineStrin
      */
     private static Edge[] ring(int component, boolean direction, boolean handedness,
                                  Coordinate[] points, int offset, Edge[] edges, int toffset, int length, final AtomicBoolean translated) {
-        // calculate the direction of the points:
-        // find the point a the top of the set and check its
-        // neighbors orientation. So direction is equivalent
-        // to clockwise/counterclockwise
-        final int top = top(points, offset, length);
-        final int prev = (offset + ((top + length - 1) % length));
-        final int next = (offset + ((top + 1) % length));
-        boolean orientation = points[offset + prev].x > points[offset + next].x;
+
+        boolean orientation = getOrientation(points, offset, length);
 
         // OGC requires shell as ccw (Right-Handedness) and holes as cw (Left-Handedness)
         // since GeoJSON doesn't specify (and doesn't need to) GEO core will assume OGC standards
@@ -678,6 +674,35 @@ private static Edge[] ring(int component, boolean direction, boolean handedness,
         return concat(component, direction ^ orientation, points, offset, edges, toffset, length);
     }
 
+    /**
+     * @return whether the points are clockwise (true) or anticlockwise (false)
+     */
+    private static boolean getOrientation(Coordinate[] points, int offset, int length) {
+        // calculate the direction of the points: find the southernmost point
+        // and check its neighbors orientation.
+
+        final int top = top(points, offset, length);
+        final int prev = (top + length - 1) % length;
+        final int next = (top + 1) % length;
+
+        final int determinantSign = orient(
+            points[offset + prev].x, points[offset + prev].y,
+            points[offset + top].x, points[offset + top].y,
+            points[offset + next].x, points[offset + next].y);
+
+        if (determinantSign == 0) {
+            // Points are collinear, but `top` is not in the middle if so, so the edges either side of `top` are intersecting.
+            throw new InvalidShapeException("Cannot determine orientation: edges adjacent to ("
+                + points[offset + top].x + "," + points[offset +top].y + ") coincide");
+        }
+
+        return determinantSign < 0;
+    }
+
+    /**
+     * @return the (offset) index of the point that is furthest west amongst
+     * those points that are the furthest south in the set.
+     */
     private static 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++) {
diff --git a/server/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java b/server/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java
index 7f8b893caf085..83bf2bae6a66f 100644
--- a/server/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java
+++ b/server/src/test/java/org/elasticsearch/common/geo/builders/PolygonBuilderTests.java
@@ -143,4 +143,22 @@ public void testHoleThatIsNorthOfPolygon() {
 
         assertEquals("Hole lies outside shell at or near point (4.0, 3.0, NaN)", e.getMessage());
     }
+
+    public void testWidePolygonWithConfusingOrientation() {
+        // A valid polygon that is oriented correctly (anticlockwise) but which
+        // confounds a naive algorithm for determining its orientation leading
+        // ES to believe that it crosses the dateline and "fixing" it in a way
+        // that self-intersects.
+
+        PolygonBuilder pb = new PolygonBuilder(new CoordinatesBuilder()
+            .coordinate(10, -20).coordinate(100, 0).coordinate(-100, 0).coordinate(20, -45).coordinate(40, -60).close());
+        pb.build(); // Should not throw an exception
+    }
+
+    public void testPolygonWithUndefinedOrientationDueToCollinearPoints() {
+        PolygonBuilder pb = new PolygonBuilder(new CoordinatesBuilder()
+            .coordinate(0.0, 0.0).coordinate(1.0, 1.0).coordinate(-1.0, -1.0).close());
+        InvalidShapeException e = expectThrows(InvalidShapeException.class, pb::build);
+        assertEquals("Cannot determine orientation: edges adjacent to (-1.0,-1.0) coincide", e.getMessage());
+    }
 }