issue with parsing '-.125' when leading decimal points allowed #777
Conversation
| @@ -115,6 +115,12 @@ private void _testLeadingDotInDecimalAllowed(JsonFactory f, int mode) throws Exc | |||
| assertEquals("0.125", p.getDecimalValue().toString()); | |||
| assertEquals(".125", p.getText()); | |||
| } | |||
| try (JsonParser p = createParser(f, mode, " -.125 ")) { | |||
| assertEquals(JsonToken.VALUE_NUMBER_FLOAT, p.nextToken()); | |||
| assertEquals(-0.125, p.getValueAsDouble()); | |||
There was a problem hiding this comment.
I think there is also getDoubleValue(), and variants with Float.
There was a problem hiding this comment.
this is just a copy of the previous few lines but testing a different input - the issue is not with me using the wrong method - the jackson code has a bug (in my opinion) that leads to it ignoring the sign on '-.125'
There was a problem hiding this comment.
Yes, I realized that. What I mean is just that test itself should probably also cover other accessors -- but did not mean to imply that was causing problems or relevant to the actual problem. Methods used are fine, just a subset of all available. So for now we can ignore that.
|
Yes, the issue seems real; let me know if you could use more help with low-level handling. Things do get bit messy with the new options, esp. as much of existing code tries to optimize things to avoid re-constructing textual values etc. |
So far, I haven't made any progress. Any change I make introduces a different issue. |
|
@pjfanning Ok. Maybe we can then start with tests located on a separate individual test class, located under |
I've moved the failing tests into a new java file. I left the partial refactor of _parseFloatThatStartsWithPeriod because it doesn't make the issue better or worse but knowing whether the number is negative is important information that the full fix will need. |
|
Sounds good; I will merge this. Could you also file a separate issue for the problem itself, for tracking purposes? (not 100% sure if it was already failing or not but good to keep track of it). |
|
@cowtowncoder I raised #782 |
|
Ok. I hope to find time to look into this today; seems like an important thing to fix. |
It's easier to describe the issue with a broken test case. See NonStandardNumberParsingTest and the tests that call _testLeadingDotInDecimalAllowed - I've added an extra scenario for '-.125'.
All of the tests get back 0.125 instead of -0.125.
One issue is that _parseFloatThatStartsWithPeriod has hardcode when calling _parseFloat that says the result is non-negative. I have partially corrected that in this PR but the code still does not work.
@cowtowncoder could you have a look and confirm that this is an issue before I spend more time trying to fix it?
Even after adjusting _parseFloatThatStartsWithPeriod to take a boolean param for the negative flag, the code is running into issues with the TextBuffer and the start pointer that _parseFloat sets on the TextBuffer (that essentially cause the TextBuffer to drop the sign). Messing around with the code for calculating the start pointer has so far caused more problems than it solves.