Move p.isNaN() check inside try block, post #4195

[JooHyukKim]

part of #4194

Motivation

This PR addresses possible p.isNan() failure by adding a fix proposed by https://github.com/FasterXML/jackson-databind/pull/4195/files#r1408831213

Changes

• Moved the p.isNaN() check inside try block
• Added more comment about why it is there

[pjfanning]

this is not what I meant - p.isNan is unreliable and should not be used

this code code can be changed to remove this and put this in the catch

if (ctxt.isEnabled(JsonNodeFeature.FAIL_ON_NAN_TO_BIG_DECIMAL_COERCION)) {
                    ctxt.handleWeirdNumberValue(handledType(), p.getDoubleValue(),
                            "Cannot convert NaN into BigDecimal");
          return; //??
} else {
   // fall through
}

If you give me a week or 2, I can get back to this and try to fix it myself.

[cowtowncoder]

@pjfanning First thing would be to show how JsonNode serialization would be compromised by this change. I do not know why this would be, based on my reading of code.

Is there a case where BigDecimal value would now be incorrect?

[cowtowncoder]

@JooHyukKim As usual, it'd be necessary to have a failing case first before trying to solve a problem. Otherwise we have no verification of the fix, or check against regression.

And specifically, as far as I can see, p.isNaN() never throws an exception; at least for JSON parser. So enclosing that in try-catch block would serve no purpose.

[cowtowncoder]

Hmmh. Ok, I think I know what is likely problematic... Double value overflow becoming +INFINITY, in case of JsonNodeFeature.FAIL_ON_NAN_TO_BIG_DECIMAL_COERCION being true.
So this would be actual bug for this handling.

But we do need jackson-core test reproduction. Maybe:

FasterXML/jackson-core#1135

does that, I'll have another look.

[cowtowncoder]

Ok. So one issue relevant here is this:

1. JsonParser.isNaN() immediately after parser.nextToken() is accurate; only considering non-standard values (if allowed)
2. JsonParser.getNumberType() coerces all fp values into NumberType.DOUBLE and thus Double value overflow can induce NaNs
3. JsonNodeDeserializer should not call getNumberType() for JSON -- but! It should call it for binary formats that DO provide this information

so it's combo of (2) and (3) that are problematic.

[JooHyukKim]

I agree we need a reproduction. I will close this, because I think I missed lots of information here thus need to catch up on what's missing.

My mistake 🙏🏼 Thank you for explaining things @pjfanning @cowtowncoder !

[pjfanning]

I don't think this is a good change - it still uses the unreliable p.isNaN call