Add JsonNodeFeature.FAIL_ON_NAN_TO_BIG_DECIMAL_COERCION to determine what happens on JsonNode coercion to BigDecimal with NaN

[JooHyukKim]

resolves #4194

Related issues

• test that shows that isNaN call is not reliable jackson-core#1135
• issue-1770 - big number not returned as DecimalNode #4191
• Incorrect deserialization for BigDecimal numbers #1770

[pjfanning]

isNaN check is broken. It needs to be fixed first. I think it is too late in 2.16 release to do this.

[JooHyukKim]

isNaN check is broken. It needs to be fixed first. I think it is too late in 2.16 release to do this.

Oh okay, will close for now since fixing isNaN may take a couple of discussions as per #1770 . Thank you for letting me know! 🙏🏼

[cowtowncoder]

Yeah, let's tackle this in 2.17 once we get 2.16.0 final out (still hoping to get Android-records module in and that's it).

[JooHyukKim]

isNaN check is broken. It needs to be fixed first.

Is there a open discussion about this that I can dig into/work on, @pjfanning ?

[cowtowncoder]

I don't think there is a Discussion for this yet.

[JooHyukKim]

If there is one, I assume FasterXML/jackson-core#1137 would be the one?

[pjfanning]

NaN/Infinite s not part of JSON spec. I think this is low priority and not of great benefit. Could we wait until some real world users ask for this? Jackson is complicated enough already.

[JooHyukKim]

True 👍🏼. No need for proactive action.

[cowtowncoder]

@pjfanning Actually there is one aspect that is within JSON spec: overflow/underflow for double (values beyond range) -- these become positive/negative infinity if decoded as double: cannot be converted to BigDecimal if decoded but could be decoded as BigDecimal directly.

@JooHyukKim There are existing failures due to problems with combination of:

1. Limited range of fixed-length floating-point types (float, double) -- but can represent NaN (NaN, +INF, -INF)
2. No support for NaNs by BigDecimal (but unlimited -- expect wrt max-number-length limit -- magnitude)

so this is not completely theoretical problem.

[JooHyukKim]

@JooHyukKim There are existing failures due to problems with combination of:

Thank you for the explanation, @cowtowncoder!. I digged into other related issues then added a list in the PR description, to save anyone's search time.

Depending on how we apprach the isNaN problem, this new USE_BIG_DECIMAL_FOR_FLOATS might not even make sense, or valid. Approaching from jackson-core also seems valid (I will see about this).

Turning into draft for now. Sorry for making a fuss by changing PR state 🥲.

[pjfanning]

@JooHyukKim p.isNaN is broken - I don't understand why this is merged - we need p.isNan to be fixed first - please can we revert this? FasterXML/jackson-core#1135

[pjfanning]

this code can be made to work if the code is moved to the catch block of the try - the try/catch is there because the p.isNaN does not work reliably

[JooHyukKim]

this code can be made to work if the code is moved to the catch block of the try - the try/catch is there because the p.isNaN does not work reliably

Right, makes sense. Thank you for providing a solution @pjfanning 🙏🏼
🔗PR

[cowtowncoder]

@pjfanning I looked through ParserBase.isNaN() and I am not sure why you think it is broken wrt usage here.
Not only are all tests passing (but there may be gaps in coverage), but I think logic makes sense: when a NaN value is encountered, value is immediately assigned with resetAsNaN(); otherwise parsing is deferred.
And ParserBase.isNaN() should not trigger parsing as double.

One thing to specifically note is that since JsonNodeDeserializer gets to call accessors first, there should not be cases for getDoubleValue() to get called first.

So... how specifically do you think isNaN() is broken wrt this specific use case?