Improve error handling for geometry deserialization edge cases #13922
Conversation
jagill
changed the title
jagill
force-pushed
the
corrupted-geometry
branch
from
a4d8a11 to
58d5d39
Compare
There was a problem hiding this comment.
Two comments about untested error paths. The one in GeometryType appears redundant with the wrapping and throwing done in the function it calls. Is there a case for having it twice?
GeometrySerializationType.getForCode throws IllegalArgumentException, is that worth catching, wrapping, and rethrowing?
| @@ -83,6 +86,11 @@ public Object getObjectValue(ConnectorSession session, Block block, int position | |||
| return null; | |||
| } | |||
| Slice slice = block.getSlice(position, 0, block.getSliceLength(position)); | |||
| return deserialize(slice).asText(); | |||
| try { | |||
There was a problem hiding this comment.
Is this error path tested exercised by any test? Looks like deserialize catches the GeometryException and wraps it so is this necessary?
| return readGeometry(input, shape, type, length); | ||
| } | ||
| catch (GeometryException e) { | ||
| throw new PrestoException(INVALID_FUNCTION_ARGUMENT, e.getMessage(), e); |
There was a problem hiding this comment.
When I ran TestGeometrySerialization I didn't see this exercised in the debugger.
jagill
force-pushed
the
corrupted-geometry
branch
from
58d5d39 to
7ba109f
Compare
|
I'm not advocating for removal of the error handling if you can trace the code and see where it might throw the exception. I was just asking if it was possible to put together a test case so I could see that the exception is indeed thrown by it and it's not superflous exception handling. Do what you think is best. |
jagill
force-pushed
the
corrupted-geometry
branch
from
7ba109f to
58d5d39
Compare
Certain geometries could be constructed (via GeometryFromText), but could not be deserialized: it raised non-PrestoException exceptions that bubbled up as non-catchable GenericInternalExceptions. This is a bad user experience! This change catches those and raise catchable PrestoExceptions.
jagill
force-pushed
the
corrupted-geometry
branch
from
58d5d39 to
28b3d8e
Compare
|
Offline conversation with Ariel: Testing those exception paths is rather difficult with the current testing set up, but we know those exceptions can be thrown (and the cases that can throw them), so it's better to catch the exceptions and raise PrestoExceptions. @mbasmanova any comments? Or does this look good to you? |
|
@mbasmanova : Error messages for the test examples: We can put them in the test cases explicitly, but these messages comes from the upstream libraries, which might change them between releases, leading to spurious test breakages. It won't be too hard to fix those breakages, if we think having the messages tested is important. |
|
@jagill Thank you, James. |
Certain geometries could be constructed (via GeometryFromText), but
could not be deserialized: it raised non-PrestoException exceptions that
bubbled up as non-catchable GenericInternalExceptions. This is a bad
user experience! This change catches those and raise catchable
PrestoExceptions.