-
Notifications
You must be signed in to change notification settings - Fork 62
fix(JOHNZON-426): BigInteger/BigDecimal string adapter flags swapped #134
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,8 +85,8 @@ public Object from(final Object a) { | |
|
|
||
| private boolean useShortISO8601Format = true; | ||
| private DateTimeFormatter dateTimeFormatter; | ||
| private boolean useBigIntegerStringAdapter = true; | ||
| private boolean useBigDecimalStringAdapter = true; | ||
| private boolean useBigIntegerStringAdapter = false; // Jakarta JSON-B 3.0 Section 3.4.1 (BigDecimal MUST be a JSON number) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this default is super important, it is the only guarantee you can have round trips, disabling it make the read/write polymorphic when you overflow supported number range which is totally broken from a typed pr parser point of view
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In reality, I did not change the default, I made them respect the behavior we had :-) The "breaking change" is purely theoretical here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the record it got broken in johnzon 1.2.21, before it was working as expected and as set in the code (string). JSON-B intoduced this broken behavior (likely trying to get JSON-B compliant since they broke themselves) to consider BigDeciman a numbers (which makes it literally not interoperable with JS for example and some other parsers) and totally inconsistent with I-JSON support of the spec, but johnzon-mapper should keep its default (string) which is the only portable way to handle big numbers since they cant be parsed by common numbers (even on 64 bits systems). So you didn't break it in this PR but it got broken 2 years ago to make it epxlicit. So your system property is a good compromise to make both cases workable without a code review since it can break easily in prod (we rarely test real "Big" ranges in unit tests) |
||
| private boolean useBigDecimalStringAdapter = false; // Jakarta JSON-B 3.0 Section 3.4.1 (BigDecimal MUST be a JSON number) | ||
|
|
||
| public void setUseShortISO8601Format(final boolean useShortISO8601Format) { | ||
| this.useShortISO8601Format = useShortISO8601Format; | ||
|
|
@@ -163,10 +163,10 @@ public Set<AdapterKey> adapterKeys() { | |
| if (from == String.class) { | ||
| return add(key, new ConverterAdapter<>(new StringConverter(), String.class)); | ||
| } | ||
| if (from == BigDecimal.class && useBigIntegerStringAdapter) { | ||
| if (from == BigDecimal.class && useBigDecimalStringAdapter) { | ||
| return add(key, new ConverterAdapter<>(new BigDecimalConverter(), BigDecimal.class)); | ||
| } | ||
| if (from == BigInteger.class && useBigDecimalStringAdapter) { | ||
| if (from == BigInteger.class && useBigIntegerStringAdapter) { | ||
| return add(key, new ConverterAdapter<>(new BigIntegerConverter(), BigInteger.class)); | ||
| } | ||
| if (from == Locale.class) { | ||
|
|
||
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.
hmm, this one was expected originally
i'm not more 100% sure but think the length of the value can be too wide (no more sure if IEEE 754 or nother spec) and therefore not portable accross parsers whereas scientific notation was more portable
can be sane to keep the default in mapper (same for the toggles, no reason to break mapper for JSON-B there)
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.
Well, no reason to write a string version of the E notation if this is already the default for JsonNumber. It's a String here, so I think it makes sense.
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.
there are a few reasons
but agree it is not crazy but also probably not worth it, no?