better handling of geojson parsing exceptions

[FabiKo117]

Description

catching geojson parsing exception to give a proper 400 - BadRequestException
fix of not properly working GetControllerTest class in case no port1 parameter was given

Corresponding issue

Closes #55

Checklist

• My code follows the code-style rules, and I have checked on the static analyses
  I have commented my code
  I have written javadoc (required for public methods)
• I have added sufficient unit and API tests
  I have made corresponding changes to the documentation
• I have updated the CHANGELOG.md
  I have adjusted the examples in the check-ohsome-api script or created an issue in the corresponding repository. More Information here.

[tyrasd]

catching geojson parsing exception to give a proper 400 - BadRequestException

currently, there already is a 400 error code returned if the bpolys parameter is invalid for a count request:

Does this fix only affect some endpoints and not others? Or just special cases of (geo)json parsing errors?

fix of not properly working GetControllerTest class in case no port1 parameter was given

Are these two independent bugfixes combined in one PR, or is this somehow related to the fix for the geojson parsing? It would be fine either way, just asking to understand.

[tyrasd]

This doesn't fix all null pointer dereferencing. Try this:

{
  "type": "FeatureCollection"
}

Even after applying this PR I still get a NullPointerException at GeometryBuilder.createGeometryFromGeoJson(GeometryBuilder.java:221):

{
  "timestamp" : "2021-03-25T11:23:28.923+0000",
  "status" : 500,
  "error" : "Internal Server Error",
  "message" : "No message available",
  "path" : "/elements/count"
}

---

also, for an empty feature collection ({"type": "FeatureCollection", "features": []}), the error code could be improved. Currently, it falsely states that the boundary does not lie withing the covered area:

{
  "timestamp": "2021-03-25T11:20:29.676167",
  "status": 404,
  "message": "The provided boundary parameter does not lie completely within the underlying data-extract polygon.",
  "requestUrl": "https://api.ohsome.org/v1/elements/count?bpolys=%7B%20%20%20%22type%22%3A%20%22FeatureCollection%22%2C%20%20%20%22features%22%3A%20%5B%5D%20%7D&filter=type%3Away%20and%20natural%3D*&format=json&time=2014-01-01%2F2017-01-01%2FP1Y"
}

[tyrasd]

fix of not properly working GetControllerTest class in case no port1 parameter was given

Is silently skipping of tests really the best approach? I mean tests can be skipped on purpose by adding -DskipTests. So if a user wants to execute the tests but forgets to provide all necessary settings (i.e. port1), the tests would not be executed, and no fail/error code be returned? IMHO that's not a good implementation. Or do I understand this incorrectly?

[tyrasd]

Even after applying this PR I still get a NullPointerException at GeometryBuilder.createGeometryFromGeoJson(GeometryBuilder.java:221):

btw, the actual line of code is src/lombok/…/GeometryBuilder.java#L237:

ohsome-api/src/main/lombok/org/heigit/ohsome/ohsomeapi/inputprocessing/GeometryBuilder.java

Lines 236 to 237 in 68d9484

	JsonArray features = root.getJsonArray("features");
	Object[] boundaryIds = new Object[features.size()];

Sadly because of lombok (#57), the reported code lines in error messages don't align with our sources anymore. 😢

[FabiKo117]

catching geojson parsing exception to give a proper 400 - BadRequestException

currently, there already is a 400 error code returned if the bpolys parameter is invalid for a count request:

Does this fix only affect some endpoints and not others? Or just special cases of (geo)json parsing errors?

fix of not properly working GetControllerTest class in case no port1 parameter was given

Are these two independent bugfixes combined in one PR, or is this somehow related to the fix for the geojson parsing? It would be fine either way, just asking to understand.

It's a fix for the given example of @SlowMo24 in #55, which was before not caught and just returned an error message that was not helpful.

[tyrasd]

It's a fix for the given example of @SlowMo24 in #55, which was before not caught and just returned an error message that was not helpful.

I understood that. However it would be nice to summarize what exactly was fixed here in this PR. For example, #55 only gives one single failing query, but the actual problem is a bit bigger it seems to me.

[FabiKo117]

also, for an empty feature collection ({"type": "FeatureCollection", "features": []}), the error code could be improved. Currently, it falsely states that the boundary does not lie withing the covered area:

I disagree here. The statement The provided boundary parameter does not lie completely within the underlying data-extract polygon. is also true if a GeoJSON containing no features is given. In a similar case, if no boundary parameter at all is given, it already returns a respective message. If we start to make very specific exception message for more and more use-cases, we will get old while doing that :D

This doesn't fix all null pointer dereferencing. Try this:

It does not say anywhere that this branch fixes every possible GeoJSON parsing exception. Of course we can make this functionality more robust within this branch.

Is silently skipping of tests really the best approach?

This approach with executing the tests when the required command line parameters are given has already been discussed in the past as far as I know. You get the message in the commandline stating e.g. Tests run: 65, Failures: 0, Errors: 0, Skipped: 2 when you only defined one of the port parameters (in this case port_data). The inputprocessing tests are always executed, unless -DskipTests or Djunit=no is set. It should be sufficiently described here: https://github.com/GIScience/ohsome-api#testing

[tyrasd]

also, for an empty feature collection ({"type": "FeatureCollection", "features": []}), the error code could be improved. Currently, it falsely states that the boundary does not lie withing the covered area:

I disagree here. The statement The provided boundary parameter does not lie completely within the underlying data-extract polygon. is also true if a GeoJSON containing no features is given. In a similar case, if no boundary parameter at all is given, it already returns a respective message. If we start to make very specific exception message for more and more use-cases, we will get old while doing that :D

Well, this depends on how you handle empty collections in general. One commonly found approach is the so called vacuous truth, which would assign the answer true to the question whether an empty feature collection lies within a polygon. Btw: also the JDK uses this convention (e.g. in Stream.allMatch).

I agree that it's definitely not something critical, and I can also live with the status quo, but IMHO it would be nicer to show the same or a similar error message to NO_BOUNDARY when an empty feature collection is encountered.

[tyrasd]

This doesn't fix all null pointer dereferencing. Try this:

It does not say anywhere that this branch fixes every possible GeoJSON parsing exception. Of course we can make this functionality more robust within this branch.

But I (the reviewer of this PR) say so. 😝

[FabiKo117]

Hm ok.. viewing it through this vacuous truth statement, the currently returned error message would actually not be fitting.

Still the question is how specific we want to be with our exception messages and on how much "common sense" of the user we can rely on. Because also with the currently returned exception message, it should be clear that something with the given boundary parameter is wrong.

But I (the reviewer of this PR) say so.

:D Yeah that's why I said we can potentially fix more stuff within this branch. I'll put it back to WIP then to adress the discussed stuff and check for potential further parsing issues that might arise. Initially I just thought of fixing the linked issue, but yeah it would make sense to invest more time into that branch.

[tyrasd]

Because also with the currently returned exception message, it should be clear that something with the given boundary parameter is wrong.

You are right. I also think that people should be clever enough to figure this out. Let's keep it as it is.

[FabiKo117]

I've moved more lines into the try-catch block and extended the error response message. Please have a look again @tyrasd.

[tyrasd]

Overall it looks good to me now, just two small-ish suggestions. See below.