jslt-152: parse-url error handling control#153
jslt-152: parse-url error handling control#153mrange wants to merge 3 commits intoschibsted:masterfrom
Conversation
| return objectNode; | ||
| } catch (MalformedURLException | UnsupportedEncodingException e) { | ||
| throw new JsltException("Can't parse " + urlString, e); | ||
| } catch (Throwable e) { |
There was a problem hiding this comment.
Possible regression as this catch is different. However, we ran into InvalidArgumentException when having trailing % which with the current code pattern gave no hints on the problem string.
There was a problem hiding this comment.
Throwable is perhaps overdoing it, since it's going to catch OutOfMemoryError and similar things. Perhaps just Exception?
There was a problem hiding this comment.
Throwable -> Exception makes sense 👍 We just rushed the fix out without considering that.
| "error": "MalformedURLException", | ||
| "message": "no protocol: this-is-an-invalid-url", | ||
| "input": "this-is-an-invalid-url" | ||
| } |
There was a problem hiding this comment.
We need a test of what happens if the second argument is not a boolean. IMHO that should be considered an error: ie, throw an exception.
We also need a test of null handling, with and without the second parameter. (Should have had a null test already, sorry.) I think null input should lead to null output, regardless of the value of the second parameter.
There was a problem hiding this comment.
I am unsure how you typically tests for failures. Do you have an example? I looked in the query-tests.yaml and found no obvious cases to me
There was a problem hiding this comment.
See function-error-tests.json for examples.
|
|
||
| If the optional `throwOnFailure` argument is not specified invalid URLs will generate an exception. If `throwOnFailure` is set to `false` the `parse-url` returns | ||
| an object indicating an error occurred. | ||
|
|
There was a problem hiding this comment.
We need to document the shape of that object, and have an example.
There was a problem hiding this comment.
I added some examples below but I agree we could add more documentation on the shape both when good and bad input
There was a problem hiding this comment.
Updated with more detaild examples.
| } else { | ||
| final ObjectNode errorNode = NodeUtils.mapper.createObjectNode(); | ||
| Class errorClass = e.getClass(); | ||
| errorNode.put("error", errorClass.getSimpleName()); |
There was a problem hiding this comment.
Why not the full class name?
There was a problem hiding this comment.
I don't feel strongly over this. I can change to full class name.
|
I updated the code as I understood the feedback. |
|
I would like to understand a little bit more the reasoning for using a My concern is that the proposed approach is not general and won't cleanly apply to cases where the return value is not an object. Is I suppose we're bound to find other cases where exception handling is warranted, and having a common way to handle errors would be preferable. Say a |
|
This "fix" was rushed as we had a pressing concern to address in our pipeline so I don't claim it's perfect as is. When it comes to I am personally not so much a favour of exceptions as no one expects the unexpected exception ;) |
5 participants
See: #152