Add a feature to allow leading plus sign (JsonReadFeature.ALLOW_LEADING_PLUS_SIGN_FOR_NUMBERS)
#774
Conversation
| p.nextToken(); | ||
| fail("Should not pass"); | ||
| } catch (JsonParseException e) { | ||
| verifyException(e, "expected digit (0-9) to follow minus sign, for valid numeric value"); |
There was a problem hiding this comment.
@cowtowncoder I've left this in draft because the existing code throws an exception with a confusing message if a plus sign precedes a number - is it ok if I use this PR to change the message to say that plus signs are not allowed in front of JSON numbers?
There was a problem hiding this comment.
Yes, absolutely, improvements to error messages are good.
| @@ -740,6 +740,10 @@ public final JsonToken nextToken() throws IOException | |||
|
|
|||
| // Ok: we must have a value... what is it? | |||
|
|
|||
| if (i == '+' && isEnabled(JsonReadFeature.ALLOW_LEADING_PLUS_SIGN_FOR_NUMBERS.mappedFeature())) { | |||
| return nextToken(); | |||
There was a problem hiding this comment.
Ok, this is problematic as it allows something like ++++4 I think. Or, potentially, DoS with 10,000 plus signs. :)
But I think it should be possible to handle this as a new entry in switch below, maybe adding case right above 0 ... 9, reading next character, falling through?
Same goes for all parsers.
|
Ok, so, improvement(s) to exception message, +1. But I think handling of leading positive marker should avoid use of recursion. It should be relatively straight-forward to instead add handling in The one thing that might be tricky is to ensure that Btw, great work I like that we might be able to support more use cases yet! |
|
@cowtowncoder I've refactored the code to stop the recursion (so multiple plus signs will not be valid). I've run into an issue with NonStandardParserFeaturesTest and its testAllowInfinity. The problem is that my PR has an explicit check for '+' and if the new parser feature is not enabled, the number is rejected. My check means '+INF' is not allowed unless the new parser feature is enabled. I need to work out a way to allow +INF when ALLOW_NON_NUMERIC_NUMBERS is enabled but still fail generally when '+' appears in numbers. |
|
@cowtowncoder I have decided to delay trying to fix the issue with default JSON mapper case and the misleading error message when leading plus signs are used (to another later PR). I think trying to solve that issue and also adding this new feature will lead to a large PR. I think this PR is ready for review. |
| assertEquals(JsonToken.VALUE_NUMBER_FLOAT, p.nextToken()); | ||
| assertEquals(0.125, p.getValueAsDouble()); | ||
| assertEquals("0.125", p.getDecimalValue().toString()); | ||
| assertEquals("0.125", p.getText()); |
There was a problem hiding this comment.
Ideally here we would get +0.125. But that's fine for now.
cowtowncoder
changed the title
JsonReadFeature.ALLOW_LEADING_PLUS_SIGN_FOR_NUMBERS)
|
Quick note: while things work wrt values, as far as I can see, the problem of I think that it mostly/completely works for floating-point numbers but does not -- and unfortunately, cannot -- be easily resolved for integer numbers. I'll have to think of a good way to resolve this; until that I may need to accept that plus sign will not be accessible as part of the text value for these cases. Not the worst thing in the world of course :) Filed #784 for sake of consistency. |
Another of the 'missing' features mentioned in #707