Skip to content

fix: make numeric boolean coercion consistent - #7804

Open
annoymous3 wants to merge 1 commit into
alibaba:mainfrom
annoymous3:fix/boolean-number-notation-consistency
Open

annoymous3 wants to merge 1 commit into
alibaba:mainfrom
annoymous3:fix/boolean-number-notation-consistency

Conversation

@annoymous3

Copy link
Copy Markdown

What this PR does / why we need it?

Fixes #7803

Equivalent JSON number representations currently produce different boolean
values. For example, 1 is converted to true, while the mathematically
equivalent 1.0 is silently converted to false.

Decimal values also bypass
JSONReader.Feature.NonZeroNumberCastToBooleanAsTrue, causing non-zero values
such as 0.5 and 2.0 to be converted to false.

Summary of your change

  • Centralize numeric-to-boolean conversion for UTF-16 and UTF-8 readers.
  • Preserve the existing default rule where values with an absolute value of
    one are converted to true.
  • Make integer, decimal, and exponent representations behave consistently.
  • Apply NonZeroNumberCastToBooleanAsTrue to decimal values.
  • Preserve the allocation-free integer path.
  • Add regression tests for String, char[], and UTF-8 byte[] inputs.

Please indicate you've done the following:

  • Made sure tests are passing and test coverage is added if needed.
  • Made sure commit message follow the rule of Conventional Commits specification.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Comment on lines +3881 to +3887
private boolean isOneNumber() {
if (valueType == JSON_TYPE_INT) {
return isOneInt();
}

return isOneDecimal();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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
Suggested change
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)

Comment on lines +3896 to +3900
private boolean isOneDecimal() {
return getBigDecimal()
.abs()
.compareTo(BigDecimal.ONE) == 0;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 to true, while the same value written {"enabled":1.0000000000000000001} coerces to false. Likewise 99999999999999999999e-20 → true, expected false. Equivalent notations, opposite booleans.
  • Exponents beyond double range (the reader admits up to MAX_EXP = 2047) overflow to Infinity and escape as a raw NumberFormatException (Infinity → writeSpecial writes null → new BigDecimal("null")); before this PR, {"enabled":1e400} coerced to false.

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:

Suggested change
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)

Comment on lines +3897 to +3899
return getBigDecimal()
.abs()
.compareTo(BigDecimal.ONE) == 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] 等价 JSON 数值转 boolean 时结果依赖书写形式并静默降级

4 participants