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.
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
Fix for ArithmeticException[/ by zero] when parsing a polygon #8475
Conversation
… 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 elastic#8433
* "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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@nknize I left a few comments. I think it's looking pretty good though |
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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@nknize left a couple of minor comments but I think its almost ready |
LGTM |
@nknize @colings86 I think this should be ported to the |
pushed to branch |
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