From c39ca479c7f1fbe5bafa92dc53f8069105722d66 Mon Sep 17 00:00:00 2001 From: Nicholas Knize Date: Thu, 13 Nov 2014 11:45:04 -0600 Subject: [PATCH 1/4] [GEO] Fix for ArithmeticException[/ by zero] when parsing a "polygon" with one pair of coordinates While this commit is primariy a fix for issue/8433 it adds more rigor to ShapeBuilder for parsing against the GeoJSON specification. Specifically, this adds LinearRing and LineString validity checks as defined in http://geojson.org/geojson-spec.html to ensure valid polygons are specified. The benefit of this fix is to provide a gate check at parse time to avoid any further processing if an invalid GeoJSON is provided. More parse checks like these will be necessary going forward to ensure full compliance with the GeoJSON specification. Closes #8433 --- .../common/geo/builders/ShapeBuilder.java | 32 ++++++++++++++++-- .../common/geo/GeoJSONShapeParserTests.java | 33 +++++++++++++++++++ .../hamcrest/ElasticsearchGeoAssertions.java | 11 +++++++ 3 files changed, 74 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java b/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java index 42e566eca93..f90f4fa660b 100644 --- a/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java +++ b/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java @@ -625,6 +625,16 @@ public abstract class ShapeBuilder implements ToXContent { } protected static LineStringBuilder parseLineString(CoordinateNode coordinates) { + /** + * Per GeoJSON spec (http://geojson.org/geojson-spec.html#linestring) + * "coordinates" member must be an array of two or more positions + * LineStringBuilder should throw a graceful exception if < 2 coordinates/points are provided + */ + if (coordinates.children.size() < 2) { + throw new ElasticsearchParseException("Invalid number of points in LineString (found " + + coordinates.children.size() + " - must be 0 or >= 2)"); + } + LineStringBuilder line = newLineString(); for (CoordinateNode node : coordinates.children) { line.point(node.coordinate); @@ -640,11 +650,29 @@ public abstract class ShapeBuilder implements ToXContent { return multiline; } + protected static LineStringBuilder parseLinearRing(CoordinateNode coordinates) { + /** + * Per GeoJSON spec (http://geojson.org/geojson-spec.html#linestring) + * A LinearRing is closed LineString with 4 or more positions. The first and last positions + * are equivalent (they represent equivalent points). Though a LinearRing is not explicitly + * represented as a GeoJSON geometry type, it is referred to in the Polygon geometry type definition. + */ + if (coordinates.children.size() < 4) { + throw new ElasticsearchParseException("Invalid number of points in LinearRing (found " + + coordinates.children.size() + " - must be 0 or >= 4)"); + } else if (coordinates.children.size() != 0 && + !coordinates.children.get(0).coordinate.equals( + coordinates.children.get(coordinates.children.size() - 1).coordinate)) { + throw new ElasticsearchParseException("Invalid LinearRing found (coordinates are not closed)"); + } + return parseLineString(coordinates); + } + protected static PolygonBuilder parsePolygon(CoordinateNode coordinates) { - LineStringBuilder shell = parseLineString(coordinates.children.get(0)); + LineStringBuilder shell = parseLinearRing(coordinates.children.get(0)); PolygonBuilder polygon = new PolygonBuilder(shell.points); for (int i = 1; i < coordinates.children.size(); i++) { - polygon.hole(parseLineString(coordinates.children.get(i))); + polygon.hole(parseLinearRing(coordinates.children.get(i))); } return polygon; } diff --git a/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java b/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java index 211f7a54b62..6b6e5079167 100644 --- a/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java +++ b/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java @@ -155,6 +155,39 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase { assertGeometryEquals(jtsGeom(expected), polygonGeoJson); } + @Test + public void testParse_invalidPolygon() throws IOException { + /** + * TODO parser should fail if poly is not composed of an array of LinearRings + * This test only checks number of coordinates, not the validity of the LinearRing + */ + // test case 1: create an invalid polygon with only 2 points + String invalidPoly1 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") + .startArray("coordinates") + .startArray() + .startArray().value(-74.011).value(40.753).endArray() + .startArray().value(-75.022).value(41.783).endArray() + .endArray() + .endArray() + .endObject().string(); + XContentParser parser = JsonXContent.jsonXContent.createParser(invalidPoly1); + parser.nextToken(); + ElasticsearchGeoAssertions.assertValidParseException(parser); + + // test case 2: create an invalid polygon with only 1 point + String invalidPolyGeoJson1 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") + .startArray("coordinates") + .startArray() + .startArray().value(-74.011).value(40.753).endArray() + .endArray() + .endArray() + .endObject().string(); + + parser = JsonXContent.jsonXContent.createParser(invalidPolyGeoJson1); + parser.nextToken(); + ElasticsearchGeoAssertions.assertValidParseException(parser); + } + @Test public void testParse_polygonWithHole() throws IOException { String polygonGeoJson = XContentFactory.jsonBuilder().startObject().field("type", "Polygon") diff --git a/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java b/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java index 1c3c636beee..7f29fe855e2 100644 --- a/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java +++ b/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java @@ -26,9 +26,12 @@ import com.spatial4j.core.shape.impl.RectangleImpl; import com.spatial4j.core.shape.jts.JtsGeometry; import com.spatial4j.core.shape.jts.JtsPoint; import com.vividsolutions.jts.geom.*; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.geo.GeoDistance; import org.elasticsearch.common.geo.GeoPoint; +import org.elasticsearch.common.geo.builders.ShapeBuilder; import org.elasticsearch.common.unit.DistanceUnit; +import org.elasticsearch.common.xcontent.XContentParser; import org.hamcrest.Matcher; import org.junit.Assert; @@ -246,4 +249,12 @@ public class ElasticsearchGeoAssertions { return GeoDistance.ARC.calculate(lat1, lon1, lat2, lon2, DistanceUnit.DEFAULT); } + public static void assertValidParseException(XContentParser parser) { + try { + ShapeBuilder.parse(parser); + throw new RuntimeException("process completed successfully when parse exception expected"); + } catch (Exception e) { + assert(e instanceof ElasticsearchParseException): "expected ElasticsearchParse exception but found " + e.getClass().getName(); + } + } } From 345c06e5e8e3d1da981c997006b5bddb512ce25d Mon Sep 17 00:00:00 2001 From: Nicholas Knize Date: Fri, 14 Nov 2014 08:12:38 -0600 Subject: [PATCH 2/4] Correcting coordinate checks on LinearRing and LineString, updating test --- .../elasticsearch/common/geo/builders/ShapeBuilder.java | 7 +++---- .../test/hamcrest/ElasticsearchGeoAssertions.java | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java b/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java index f90f4fa660b..9a754dc7460 100644 --- a/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java +++ b/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java @@ -632,7 +632,7 @@ public abstract class ShapeBuilder implements ToXContent { */ if (coordinates.children.size() < 2) { throw new ElasticsearchParseException("Invalid number of points in LineString (found " + - coordinates.children.size() + " - must be 0 or >= 2)"); + coordinates.children.size() + " - must be >= 2)"); } LineStringBuilder line = newLineString(); @@ -659,9 +659,8 @@ public abstract class ShapeBuilder implements ToXContent { */ if (coordinates.children.size() < 4) { throw new ElasticsearchParseException("Invalid number of points in LinearRing (found " + - coordinates.children.size() + " - must be 0 or >= 4)"); - } else if (coordinates.children.size() != 0 && - !coordinates.children.get(0).coordinate.equals( + coordinates.children.size() + " - must be >= 4)"); + } else if (!coordinates.children.get(0).coordinate.equals( coordinates.children.get(coordinates.children.size() - 1).coordinate)) { throw new ElasticsearchParseException("Invalid LinearRing found (coordinates are not closed)"); } diff --git a/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java b/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java index 7f29fe855e2..ac8efff76b9 100644 --- a/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java +++ b/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java @@ -252,7 +252,7 @@ public class ElasticsearchGeoAssertions { public static void assertValidParseException(XContentParser parser) { try { ShapeBuilder.parse(parser); - throw new RuntimeException("process completed successfully when parse exception expected"); + Assert.fail("process completed successfully when parse exception expected"); } catch (Exception e) { assert(e instanceof ElasticsearchParseException): "expected ElasticsearchParse exception but found " + e.getClass().getName(); } From 49935659e490b09e4a87bc5ea67d7802e453db26 Mon Sep 17 00:00:00 2001 From: Nicholas Knize Date: Fri, 14 Nov 2014 09:58:34 -0600 Subject: [PATCH 3/4] Adding parse gates for valid GeoJSON coordinates. Includes unit tests. --- .../common/geo/builders/ShapeBuilder.java | 8 +++- .../common/geo/GeoJSONShapeParserTests.java | 45 +++++++++++++++++-- 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java b/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java index 9a754dc7460..e186e2bb9c0 100644 --- a/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java +++ b/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java @@ -206,13 +206,17 @@ public abstract class ShapeBuilder implements ToXContent { private static CoordinateNode parseCoordinates(XContentParser parser) throws IOException { XContentParser.Token token = parser.nextToken(); - // Base case - if (token != XContentParser.Token.START_ARRAY) { + // Base cases + if (token != XContentParser.Token.START_ARRAY && + token != XContentParser.Token.END_ARRAY && + token != XContentParser.Token.VALUE_NULL) { double lon = parser.doubleValue(); token = parser.nextToken(); double lat = parser.doubleValue(); token = parser.nextToken(); return new CoordinateNode(new Coordinate(lon, lat)); + } else if (token == XContentParser.Token.VALUE_NULL) { + return null; } List nodes = new ArrayList<>(); diff --git a/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java b/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java index 6b6e5079167..1b1505ec09a 100644 --- a/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java +++ b/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java @@ -158,8 +158,8 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase { @Test public void testParse_invalidPolygon() throws IOException { /** - * TODO parser should fail if poly is not composed of an array of LinearRings - * This test only checks number of coordinates, not the validity of the LinearRing + * The following 3 test cases ensure proper error handling of invalid polygons + * per the GeoJSON specification */ // test case 1: create an invalid polygon with only 2 points String invalidPoly1 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") @@ -175,7 +175,7 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase { ElasticsearchGeoAssertions.assertValidParseException(parser); // test case 2: create an invalid polygon with only 1 point - String invalidPolyGeoJson1 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") + String invalidPoly2 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") .startArray("coordinates") .startArray() .startArray().value(-74.011).value(40.753).endArray() @@ -183,7 +183,44 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase { .endArray() .endObject().string(); - parser = JsonXContent.jsonXContent.createParser(invalidPolyGeoJson1); + parser = JsonXContent.jsonXContent.createParser(invalidPoly2); + parser.nextToken(); + ElasticsearchGeoAssertions.assertValidParseException(parser); + + // test case 3: create an invalid polygon with 0 points + String invalidPoly3 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") + .startArray("coordinates") + .startArray() + .startArray().endArray() + .endArray() + .endArray() + .endObject().string(); + + parser = JsonXContent.jsonXContent.createParser(invalidPoly3); + parser.nextToken(); + ElasticsearchGeoAssertions.assertValidParseException(parser); + + // test case 4: create an invalid polygon with null value points + String invalidPoly4 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") + .startArray("coordinates") + .startArray() + .startArray().nullValue().nullValue().endArray() + .endArray() + .endArray() + .endObject().string(); + + parser = JsonXContent.jsonXContent.createParser(invalidPoly4); + parser.nextToken(); + ElasticsearchGeoAssertions.assertValidParseException(parser); + + // test case 5: create an invalid polygon with 1 invalid LinearRing + String invalidPoly5 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") + .startArray("coordinates") + .nullValue().nullValue() + .endArray() + .endObject().string(); + + parser = JsonXContent.jsonXContent.createParser(invalidPoly5); parser.nextToken(); ElasticsearchGeoAssertions.assertValidParseException(parser); } From 0067a0cb7e54fcda166709d62d22e149756ca832 Mon Sep 17 00:00:00 2001 From: Nicholas Knize Date: Fri, 14 Nov 2014 10:28:30 -0600 Subject: [PATCH 4/4] Updating to throw IllegalArgument exception for null value coordinates. Tests included. --- .../common/geo/builders/ShapeBuilder.java | 2 +- .../common/geo/GeoJSONShapeParserTests.java | 12 +++++++----- .../test/hamcrest/ElasticsearchGeoAssertions.java | 7 ++++--- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java b/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java index e186e2bb9c0..edfee171fc2 100644 --- a/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java +++ b/src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java @@ -216,7 +216,7 @@ public abstract class ShapeBuilder implements ToXContent { token = parser.nextToken(); return new CoordinateNode(new Coordinate(lon, lat)); } else if (token == XContentParser.Token.VALUE_NULL) { - return null; + throw new ElasticsearchIllegalArgumentException("coordinates cannot contain NULL values)"); } List nodes = new ArrayList<>(); diff --git a/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java b/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java index 1b1505ec09a..e3b4eff2ecd 100644 --- a/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java +++ b/src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java @@ -26,6 +26,8 @@ import com.spatial4j.core.shape.ShapeCollection; import com.spatial4j.core.shape.jts.JtsGeometry; import com.spatial4j.core.shape.jts.JtsPoint; import com.vividsolutions.jts.geom.*; +import org.elasticsearch.ElasticsearchIllegalArgumentException; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.geo.builders.ShapeBuilder; import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; @@ -172,7 +174,7 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase { .endObject().string(); XContentParser parser = JsonXContent.jsonXContent.createParser(invalidPoly1); parser.nextToken(); - ElasticsearchGeoAssertions.assertValidParseException(parser); + ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); // test case 2: create an invalid polygon with only 1 point String invalidPoly2 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") @@ -185,7 +187,7 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase { parser = JsonXContent.jsonXContent.createParser(invalidPoly2); parser.nextToken(); - ElasticsearchGeoAssertions.assertValidParseException(parser); + ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); // test case 3: create an invalid polygon with 0 points String invalidPoly3 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") @@ -198,7 +200,7 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase { parser = JsonXContent.jsonXContent.createParser(invalidPoly3); parser.nextToken(); - ElasticsearchGeoAssertions.assertValidParseException(parser); + ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchParseException.class); // test case 4: create an invalid polygon with null value points String invalidPoly4 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") @@ -211,7 +213,7 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase { parser = JsonXContent.jsonXContent.createParser(invalidPoly4); parser.nextToken(); - ElasticsearchGeoAssertions.assertValidParseException(parser); + ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchIllegalArgumentException.class); // test case 5: create an invalid polygon with 1 invalid LinearRing String invalidPoly5 = XContentFactory.jsonBuilder().startObject().field("type", "polygon") @@ -222,7 +224,7 @@ public class GeoJSONShapeParserTests extends ElasticsearchTestCase { parser = JsonXContent.jsonXContent.createParser(invalidPoly5); parser.nextToken(); - ElasticsearchGeoAssertions.assertValidParseException(parser); + ElasticsearchGeoAssertions.assertValidException(parser, ElasticsearchIllegalArgumentException.class); } @Test diff --git a/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java b/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java index ac8efff76b9..7ba04cb9d78 100644 --- a/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java +++ b/src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java @@ -249,12 +249,13 @@ public class ElasticsearchGeoAssertions { return GeoDistance.ARC.calculate(lat1, lon1, lat2, lon2, DistanceUnit.DEFAULT); } - public static void assertValidParseException(XContentParser parser) { + public static void assertValidException(XContentParser parser, Class expectedException) { try { ShapeBuilder.parse(parser); - Assert.fail("process completed successfully when parse exception expected"); + Assert.fail("process completed successfully when " + expectedException.getName() + " expected"); } catch (Exception e) { - assert(e instanceof ElasticsearchParseException): "expected ElasticsearchParse exception but found " + e.getClass().getName(); + assert(e.getClass().equals(expectedException)): + "expected " + expectedException.getName() + " but found " + e.getClass().getName(); } } }