Throw JWTDecodeException when date claim format is invalid#241
Throw JWTDecodeException when date claim format is invalid#241lbalmaceda merged 2 commits intomasterfrom
Conversation
| return null; | ||
| } | ||
| if (!node.canConvertToLong()) { | ||
| throw new JWTDecodeException(String.format("The claim '%s' contained an unexpected value.", claimName)); |
uwinkelvos
left a comment
uwinkelvos
left a comment
There was a problem hiding this comment.
i just did pretty much exactly the same thing, until i saw you alread fixed it. Looks good to me, apart from that comment regarding the ex message.
|
|
||
| Date date = deserializer.getDateFromSeconds(tree, "key"); | ||
| assertThat(date, is(nullValue())); | ||
| deserializer.getDateFromSeconds(tree, "key"); |
There was a problem hiding this comment.
So no assertions as now caught by the exception.expect? if getDateFromSeconds explodes?
There was a problem hiding this comment.
Exactly. Because the ExpectedException is "none" for every method but for those in which we override it, when we run tests they expect (or don't) to fail with X exception and Y message.
| Date getDateFromSeconds(Map<String, JsonNode> tree, String claimName) { | ||
| JsonNode node = tree.get(claimName); | ||
| if (node == null || node.isNull() || !node.canConvertToLong()) { | ||
| if (node == null || node.isNull()) { |
There was a problem hiding this comment.
So non Java person here, what is the difference here in checking for null?
There was a problem hiding this comment.
node is the instance of the JsonNode we're checking. The check is asserting that the instance of this object is null OR that the parsed JSON value is null. This second check wouldn't be possible if the instance is null as that would throw a NPE when calling some method on it.
3 participants
WDYT about the exception message?
Fixes #240