fix: make numeric boolean coercion consistent - #7804
annoymous3 wants to merge 1 commit into
Conversation
|
|
| private boolean isOneNumber() { | ||
| if (valueType == JSON_TYPE_INT) { | ||
| return isOneInt(); | ||
| } | ||
|
|
||
| return isOneDecimal(); | ||
| } |
There was a problem hiding this comment.
[Critical] Type-suffixed number literals (1B, 1S, 1L, 1F, 1D) in a boolean position now throw JSONException("TODO : N") instead of coercing to a boolean.
readNumber0() unconditionally assigns JSON_TYPE_INT8/INT16/INT64/FLOAT/DOUBLE for suffixed literals in both readers, but isOneNumber() only special-cases JSON_TYPE_INT, so the suffixed types fall into isOneDecimal() → getBigDecimal(), whose switch has no case for them and hits default: throw new JSONException("TODO : " + valueType). Before this PR the removed fall-through returned false for these inputs, so previously-parseable lenient input now fails to parse — including fastjson2's own writer output: with WriteClassName, JSON.toJSONString emits 1L, and re-parsing that into a boolean field throws. The behavior is also self-inconsistent: with NonZeroNumberCastToBooleanAsTrue enabled, the same 1L returns true via the mag path instead of throwing.
Witness (probe on this PR's compiled code; String, char[] and byte[] inputs all identical):
1L default=THROWS JSONException: TODO : 11 nonZeroFeature=true
1B default=THROWS JSONException: TODO : 9 nonZeroFeature=true
1S default=THROWS JSONException: TODO : 10 nonZeroFeature=true
1F default=THROWS JSONException: TODO : 12 nonZeroFeature=true
1D default=THROWS JSONException: TODO : 13 nonZeroFeature=true
| private boolean isOneNumber() { | |
| if (valueType == JSON_TYPE_INT) { | |
| return isOneInt(); | |
| } | |
| return isOneDecimal(); | |
| } | |
| private boolean isOneNumber() { | |
| switch (valueType) { | |
| case JSON_TYPE_INT: | |
| case JSON_TYPE_INT8: | |
| case JSON_TYPE_INT16: | |
| case JSON_TYPE_INT64: | |
| return isOneInt(); | |
| case JSON_TYPE_DEC: | |
| case JSON_TYPE_BIG_DEC: | |
| return isOneDecimal(); | |
| default: | |
| return false; | |
| } | |
| } |
The integer-family suffixed types share the mag layout, so isOneInt() works for them as-is (1L → true, matching the feature path); FLOAT/DOUBLE keep the pre-PR no-throw fallback. If you apply this, please add cases like assertValue(true, "1L") / assertValue(false, "1F") to JSONReaderBooleanTest.numberNotation() and confirm they fail with JSONException if the old dispatch is restored.
— qwen3.8-max via Qwen Code /review (v0.22.0)
| private boolean isOneDecimal() { | ||
| return getBigDecimal() | ||
| .abs() | ||
| .compareTo(BigDecimal.ONE) == 0; | ||
| } |
There was a problem hiding this comment.
[Critical] isOneDecimal() routes the exact ±1 test through getBigDecimal(), whose JSON_TYPE_DEC branch applies a non-zero exponent by round-tripping through Double.parseDouble (JSONReader.java:4333-4337). This reintroduces notation-dependent booleans for exponent forms — the defect class the linked issue was filed for — and turns large exponents into a hard parse failure.
Two shapes, both reproduced on the UTF-16 and UTF-8 readers:
- Values within half an ulp of 1.0 snap to exactly 1.0:
{"enabled":10000000000000000001e-19}(exact value 1.0000000000000000001 ≠ 1) coerces totrue, while the same value written{"enabled":1.0000000000000000001}coerces tofalse. Likewise99999999999999999999e-20→true, expectedfalse. Equivalent notations, opposite booleans. - Exponents beyond double range (the reader admits up to
MAX_EXP = 2047) overflow toInfinityand escape as a rawNumberFormatException(Infinity→writeSpecialwritesnull→new BigDecimal("null")); before this PR,{"enabled":1e400}coerced tofalse.
Witness (probe on this PR's compiled code):
1.0000000000000000001 => false 10000000000000000001e-19 => true
99999999999999999999e-20 => true (expected false)
1e400 default=THROWS NumberFormatException: Character n is neither a decimal digit number, ...
-1e400, 9.9e400 => same; 1e400 with NonZeroNumberCastToBooleanAsTrue => true (no throw)
Root-cause fix: in getBigDecimal()'s JSON_TYPE_DEC case, replace the three-line Double.parseDouble round-trip with return decimal.scaleByPowerOfTen(exponent); — exact, no throw, and it repairs the same lossy conversion for existing callers of that branch. If the change should stay scoped to this diff, compare exactly from the parsed state instead of delegating:
| private boolean isOneDecimal() { | |
| return getBigDecimal() | |
| .abs() | |
| .compareTo(BigDecimal.ONE) == 0; | |
| } | |
| private boolean isOneDecimal() { | |
| if (valueType == JSON_TYPE_BIG_DEC) { | |
| return getBigDecimal() | |
| .abs() | |
| .compareTo(BigDecimal.ONE) == 0; | |
| } | |
| int[] mag = mag0 == 0 | |
| ? mag1 == 0 | |
| ? mag2 == 0 | |
| ? new int[]{mag3} | |
| : new int[]{mag2, mag3} | |
| : new int[]{mag1, mag2, mag3} | |
| : new int[]{mag0, mag1, mag2, mag3}; | |
| BigDecimal decimal = new BigDecimal(BIG_INTEGER_CREATOR.apply(negative ? -1 : 1, mag), scale); | |
| return decimal.scaleByPowerOfTen(exponent) | |
| .abs() | |
| .compareTo(BigDecimal.ONE) == 0; | |
| } |
If you apply either fix, please add assertValue(false, "10000000000000000001e-19") and assertValue(false, "1e400") to JSONReaderBooleanTest.numberNotation() and confirm the first fails (returns true) and the second throws when the double round-trip is restored.
— qwen3.8-max via Qwen Code /review (v0.22.0)
| return getBigDecimal() | ||
| .abs() | ||
| .compareTo(BigDecimal.ONE) == 0; |
There was a problem hiding this comment.
[Suggestion] Every non-INT number in a boolean position now pays one extra BigDecimal allocation through getBigDecimal() here (plus a second one from abs() for negatives), where the removed code returned false without it. readNumber() → getNumber() already allocates one discarded Number for these inputs (pre-existing), so this adds a second per-value allocation.
Measured with a ThreadMXBean allocation probe against this PR's build: +40 B per parse for 1.0/2.0/0.5, +80 B for negatives, +352 B for exponent forms (toPlainString + string concat + Double.parseDouble + toBigDecimal(double)). The integer path is unaffected (1 and true both 232 B), consistent with the PR's stated goal of keeping it allocation-free.
The exact ±1 test is computable allocation-free from mag0..mag3/scale/exponent for JSON_TYPE_DEC (the mantissa must equal 10^(scale−exponent); powers of ten fit the mag representation up to 38 digits); for JSON_TYPE_BIG_DEC the source is stringValue, so a cheap character scan suffices. The impact is modest — this path only fires when a number appears where a boolean is expected — but an allocation-free fast path ahead of the getBigDecimal() fallback would keep a cold path cold in a performance-first library. If the exact-comparison fix from the Critical above is applied, the two naturally combine.
— qwen3.8-max via Qwen Code /review (v0.22.0)
What this PR does / why we need it?
Fixes #7803
Equivalent JSON number representations currently produce different boolean
values. For example,
1is converted totrue, while the mathematicallyequivalent
1.0is silently converted tofalse.Decimal values also bypass
JSONReader.Feature.NonZeroNumberCastToBooleanAsTrue, causing non-zero valuessuch as
0.5and2.0to be converted tofalse.Summary of your change
one are converted to
true.NonZeroNumberCastToBooleanAsTrueto decimal values.Please indicate you've done the following: