-
Notifications
You must be signed in to change notification settings - Fork 23
[json-org] fix issue with parsing big numbers #32
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
Conversation
@@ -66,7 +66,7 @@ public JSONArray deserialize(JsonParser p, DeserializationContext ctxt) | |||
array.put(p.getNumberValue()); | |||
continue; | |||
case VALUE_NUMBER_FLOAT: | |||
array.put(p.getNumberValue()); | |||
array.put(p.getDecimalValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only now realizing that this is actually behavioral change too. Hmmh.
So even if improvement, might break code if the exposed number changes from Double
to BigDecimal
?
Or maybe JSONP API encapsulates so this is not the case?
Since I already merged changes for JSR-353/JSONP will merge this too, just noting that we may get bug reports for 2.15.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pjfanning Doh. Not JSONP/JSR-353. Still, similar concerns.
But quick question: would it make sense to instead call p.getNumberValueExact()
? This would at least retain exact Float
/Double
for binary formats? Would change it to BigDecimal
for JSON and other textual formats, which is probably good overall.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if p.getNumberValueExact()
works, then I'd be happy with that change - let me try a PR with that instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b.add(name, p.getNumberValueExact());
doesn't compile
ie b.add does not support Number
I looked at the code in the json libs and they tend to use BigDecimal under the hood - so I think the risks of the jackson changes are low.
I think it is better to take the risk that someone will report an edge case than to add complicated logic using p.getNumberValueExact() and then lots of instanceof checks with casts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh. Yes, I forgot the challenge wrt non-typed Number
. I concur with your assessment.
Ideally we'd have more time for testing but TBH this module probably won't get all that much beta-testing compared to jackson-databind
or jackson-core
.
FYI. I bumped to this PR/change as the result of our team investigations on change in behavior of JSONObject.get("field") when the actual value is double/float. Our team hit this behavior change with 2.15.2 version uptake and we couldn't find any workaround. Do you have any suggestion? The code: I was hoping that we can, at least, control the behavior DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS but it has no effect. Do you still plan to change the behavior to use getNumberValueExact() so that it returns Double correctly as before? |
@dlipin can you open a a new issue and provide a reproducible test case? |
this test passes:
JSONObject has methods that let you choose the format that you want the number in. If you want a double, you call |
@dlipin like @pjfanning suggested, we'd really need a separate issue with reproduction. I think I understand the problem but there is always possibility of misunderstanding. |
Sorry for delay, I've filed a new issue: |
Thank you @dlipin. As per my other note, I think we are going to go with the new behavior as it resolves an issue and there is a way to get |
BigInteger
handling #31