Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix for ArithmeticException[/ by zero] when parsing a polygon #8475

Merged
merged 4 commits into from Nov 19, 2014
Merged

Fix for ArithmeticException[/ by zero] when parsing a polygon #8475

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c39ca47
[GEO] Fix for ArithmeticException[/ by zero] when parsing a "polygon"…
nknize Nov 13, 2014
345c06e
Correcting coordinate checks on LinearRing and LineString, updating test
nknize Nov 14, 2014
4993565
Adding parse gates for valid GeoJSON coordinates. Includes unit tests.
nknize Nov 14, 2014
0067a0c
Updating to throw IllegalArgument exception for null value coordinate…
nknize Nov 14, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 29 additions & 2 deletions src/main/java/org/elasticsearch/common/geo/builders/ShapeBuilder.java
View file
Open in desktop
Expand Up @@ -625,6 +625,16 @@ protected static MultiPointBuilder parseMultiPoint(CoordinateNode coordinates) {
}

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This statement and the message below don't seem to correlate. Should we not also check if coordinates.children.size() == 0 here? Also from the linked spec it looks like 0 points would not be valid anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. There's a discrepancy between JTS and GeoJSON. JTS treats 0 points as a valid linear ring/linestring where GeoJSON does not. If we're going to conform to GeoJSON (which I think is the right approach - but open for thoughts) then I should insert the check for 0 points.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, should not insert the check for 0 points. The current check would be valid per the GeoJSON spec - I'll change the exception message.

throw new ElasticsearchParseException("Invalid number of points in LineString (found " +
coordinates.children.size() + " - must be >= 2)");
}

LineStringBuilder line = newLineString();
for (CoordinateNode node : coordinates.children) {
line.point(node.coordinate);
Expand All @@ -640,11 +650,28 @@ protected static MultiLineStringBuilder parseMultiLine(CoordinateNode coordinate
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 >= 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)");
}
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;
}
Expand Down
33 changes: 33 additions & 0 deletions src/test/java/org/elasticsearch/common/geo/GeoJSONShapeParserTests.java
View file
Open in desktop
Expand Up @@ -155,6 +155,39 @@ public void testParse_polygonNoHoles() throws IOException {
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);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we now add a test for an invalid polygon with 0 points?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following coordinate arrays are considered valid GeoJSON:

{ 
  "shape": {
    "type": "polygon",
    "coordinates": [[[]]]
  }
}

{ 
  "shape": {
    "type": "polygon",
    "coordinates": [[[null, null]]]
  }
}

The code, however, threw jackson parse exception and number format exceptions, respectively. The code has been updated to throw a more meaningful ParseException and IllegalArgumentException for these cases to help the end user diagnose the actual issue.


@Test
public void testParse_polygonWithHole() throws IOException {
String polygonGeoJson = XContentFactory.jsonBuilder().startObject().field("type", "Polygon")
Expand Down
11 changes: 11 additions & 0 deletions src/test/java/org/elasticsearch/test/hamcrest/ElasticsearchGeoAssertions.java
View file
Open in desktop
Expand Up @@ -26,9 +26,12 @@
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;

Expand Down Expand Up @@ -246,4 +249,12 @@ private static double distance(double lat1, double lon1, double lat2, double lon
return GeoDistance.ARC.calculate(lat1, lon1, lat2, lon2, DistanceUnit.DEFAULT);
}

public static void assertValidParseException(XContentParser parser) {
try {
ShapeBuilder.parse(parser);
Assert.fail("process completed successfully when parse exception expected");
} catch (Exception e) {
assert(e instanceof ElasticsearchParseException): "expected ElasticsearchParse exception but found " + e.getClass().getName();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could possibly use assertThat(e, org.hamcrest.Matchers.isA(ElasticsearchParseException)); here instead but I don't have a strong opinion about this either way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main thing here is that the test fails rather than errors on this line

}
}
}