Skip to content
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

Improve detection of "is a NaN" to only consider explicit cases, not double overflow/underflow #1137

Closed
cowtowncoder opened this issue Nov 8, 2023 · 4 comments
Labels
2.17 Issues planned (at earliest) for 2.17
Milestone

Comments

@cowtowncoder
Copy link
Member

Currently it appears that too big (overflow) and too small (underflow) values can produce "Not a Number" (NaN) values from JsonParser, in addition to optionally supported tokens -- only allowed if enabled -- like +INF / -INF / NaN).

But doing this causes problem with BigDecimal handling since such "too big"/"too small" values CAN be represented as BigDecimal, whereas "true" Not-a-Numbers cannot.

So we should improve handling of JsonParser.isNaN() method so that it ONLY indicates explicit Not-a-Number cases, and not overflows.

@pjfanning
Copy link
Member

There is similar NaN checking in many Jackson modules. If we want to sort this for 2.16, we should delay the release to look at all the modules. I think NaN handling is a rarely needed use case and that it be better to leave this till after 2.16 release.

@pjfanning
Copy link
Member

pjfanning commented Nov 8, 2023

If we were to tackle this, I would suggest adding isNaNOrInfinite(String) public static method to NumberInput class that does simple String checks instead of trying to parse the input as a number. This could be used in different Jackson modules.

Most of our parsers keep the data as Strings/char[] and only parse to number representation if needed.

@cowtowncoder cowtowncoder added the 2.17 Issues planned (at earliest) for 2.17 label Nov 9, 2023
@cowtowncoder
Copy link
Member Author

Forgot to tag this as 2.17, I 100% agree that due to timing we would increase risk by making changes now.

I don't have a complete idea on how to go about this, due to differences between Double/double/Float/float (with NaN marker values, but limited ranges) vs BigDecimal (unlimited range but no NaN values).

One existing example of what might be doable are methods canConvertToXxx in JsonNode (canConvertToInt(), canConvertToLong(), canConvertToExactIntegral()).
So could potentially add something like

JsonParser.isInDoubleRange();

which would be roughly same as !isNaNOrInfiniteForDouble(). But also potentially expensive depending on conversion.

But then again there is the question of changing observed behavior of isNaN even if not changing documented one -- Javadocs do not say that isNaN should return true in case value is outside range of Double and that just occurs as a side-effect of parsing into double.

@cowtowncoder
Copy link
Member Author

Was implemented a while ago (forgot to add reference from commit). Tested via

src/test/java/com/fasterxml/jackson/core/read/NonStandardNumberParsingTest.java

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.17 Issues planned (at earliest) for 2.17
Projects
None yet
Development

No branches or pull requests

2 participants