Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a feature to allow leading plus sign (JsonReadFeature.ALLOW_LEADING_PLUS_SIGN_FOR_NUMBERS) #774

Merged
merged 16 commits into from
Jun 23, 2022

Conversation

pjfanning
Copy link
Member

@pjfanning pjfanning commented Jun 22, 2022

Another of the 'missing' features mentioned in #707

@pjfanning pjfanning marked this pull request as draft June 22, 2022 00:11
p.nextToken();
fail("Should not pass");
} catch (JsonParseException e) {
verifyException(e, "expected digit (0-9) to follow minus sign, for valid numeric value");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cowtowncoder
Copy link
Member

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 switch block with case '+': preceding cases for digits 0...9, reading next character (which will then not allow spaces between minus and number), requiring that to be valid number start character, then calling _parsePosNumber().

The one thing that might be tricky is to ensure that JsonParser.getText() returns String with leading plus, it should not be trimmed off, ideally (at least I think intent is to represent exact token even if not JSON compliant).

Btw, great work I like that we might be able to support more use cases yet!

@pjfanning
Copy link
Member Author

pjfanning commented Jun 22, 2022

@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.

@pjfanning pjfanning marked this pull request as ready for review June 22, 2022 11:24
@pjfanning
Copy link
Member Author

@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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally here we would get +0.125. But that's fine for now.

@cowtowncoder cowtowncoder merged commit c2ca290 into FasterXML:2.14 Jun 23, 2022
@cowtowncoder cowtowncoder changed the title Leading plus sign (non-standard support - optional) Add a feature to allow leading plus sign (JsonReadFeature.ALLOW_LEADING_PLUS_SIGN_FOR_NUMBERS) Jun 23, 2022
cowtowncoder added a commit that referenced this pull request Jun 23, 2022
@cowtowncoder
Copy link
Member

cowtowncoder commented Jun 28, 2022

Quick note: while things work wrt values, as far as I can see, the problem of getText() dropping + is trickier to resolve.

I think that it mostly/completely works for floating-point numbers but does not -- and unfortunately, cannot -- be easily resolved for integer numbers.
The problem is due to TextBuffer being used to store value, and hard-coded assumptions that only negative sign may exist (to be ignored on decoding). So while + may be prepended, it would then be used for calculations getting integer/long values badly off.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants