-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Review of number deserialization #4484
base: 2.18
Are you sure you want to change the base?
Conversation
Couple of random notes:
|
1e2 and 1.01e2 are definitely integers and it's easy to determine that a number in exponential form is an integer (1-based position of last non-zero figure after decimal point must be <= exponent). We should treat them as such IMO. The JSON spec describes a range of number formats but this should have no impact on what we do with them, we should just deal with the mathematical number presented regardless of the format. I'll have to take your word for it in terms of complexity but I imagine there would be a very small performance hit to do those extra checks. |
Hmmh. No, I think disagree here... this path leads to no complexity overall. Or at very least, let's tackle other issues first -- I think this is not one of lower hanging fruits and there are other fish to fry. |
Yep, I agree. Can you pick which of the items I reported you'd like to progress, and an order? Happy to make PRs. BigDecimal fixes? |
@davidmoten yes, I think And one more thing on my thinking wrt Int-vs-Float: I think of Java/C style coercion, so:
and this somewhat rigid system is predictable and easy/easier to explain. It's not quite as convenient as more advanced detection of "actually integral number despite using E-notation" or "integral since fractional part is all zeroes", but at this point implementation is complicated enough that I prefer not adding this complexity. Even if I see why others would choose differently. But that's enough about philosophical/design aspect. :) |
Thanks @cowtowncoder, sounds good. BTW I found this quote from json-schema.org that explains my position on "actually an integer" better than I did:
|
This PR for discussion only, not merge (but could happen if deemed useful).
Related to discussions started in #4453, this PR includes a
NumericDeserializationReviewMain
class that checks various number deserialization scenarios using both @JsonProperty deserialization and single-arg constructor deserialization.After running that Main class and inspecting the output this is my summary:
Conclusions for property deserialization
Conclusions for single-arg deserialization
I'd like to get some agreement from project owners about what is good and what is bad behaviour (my opinions are above), and then we can plan some fixes.
Output: