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

Add JsonNodeFeature.FAIL_ON_NAN_TO_BIG_DECIMAL_COERCION option to fail on attempting to coerce NaN into BigDecimal #4194

Closed
cowtowncoder opened this issue Nov 8, 2023 · 5 comments · Fixed by #4195
Labels
2.17 Issues planned at earliest for 2.17

Comments

@cowtowncoder
Copy link
Member

Describe your Issue

(follow up to #1770)

So, we should not (by default) coerce "NaN"s (infinity, NaN) for JsonNode as is done with #1770 fixed.
There should probably be a JsonNodeFeature to allow such coercion, however.

@cowtowncoder cowtowncoder added to-evaluate Issue that has been received but not yet evaluated 2.17 Issues planned at earliest for 2.17 and removed to-evaluate Issue that has been received but not yet evaluated labels Nov 8, 2023
@JooHyukKim
Copy link
Member

+1 Adding an option for backward compatibility and moving-forward to 3.0.

I am guessing that...

  • Considering backward-compatibility, allow coercion by default in Jackson 2.x
  • Disallow by default in Jackson 3.x

Then users who want to keep allowing by explicitly enabling the option

@cowtowncoder
Copy link
Member Author

@JooHyukKim Yes. For 3.0 probably want to split JsonNode specific configuration into JsonNodeFeature, and leave/refactor other setting to only affect Object / Number case.

@pjfanning
Copy link
Member

pjfanning commented Nov 9, 2023

Just my opinion - but I think we don't need another config. Support for NaN/Infinity are not default in Jackson. You need to enable JsonReadFeature.ALLOW_NON_NUMERIC_NUMBERS (on the JsonFactory).

So users who want BigDecimals should not enable JsonReadFeature.ALLOW_NON_NUMERIC_NUMBERS.

@cowtowncoder
Copy link
Member Author

This is bit different @pjfanning -- it has to do with what to do when NaNs are accepted from input but there is need to coerce floating-point numbers to BigDecimal.
This could work in at least 2 ways:

  1. Quietly use DoubleNode instead of BigDecimalNode (i.e. prevent impossible coercion)
  2. Fail on encountering case that cannot be supported

while we could technically just select one of these cases always, it seems like a thing that users might want to select differently.

@cowtowncoder cowtowncoder changed the title Add JsonNodeFeature option to fail on attempting to coerce NaN into BigDecimal (if DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS enabled) Add JsonNodeFeature.FAIL_ON_NAN_TO_BIG_DECIMAL_COERCION option to fail on attempting to coerce NaN into BigDecimal Nov 29, 2023
cowtowncoder added a commit that referenced this issue Nov 29, 2023
@cowtowncoder
Copy link
Member Author

Ok, after re-reading relevant issues, refreshing my memory, I concur with @pjfanning that isNaN() is (in combination with getNumberType()) unreliable.

I filed FasterXML/jackson-core#1149 for some additional work; and there is FasterXML/jackson-core#1137 already.
With those, I think we can solve the issue.

I also think that with changes, we probably want to rename this feature. Why? Because:

  1. There is already case of explicit NaN coming optionally from input (NaN, +/-INFINITY) BUT
  2. There is ALSO possibility of +INFINITY/-INFINITY from "too big/small Double value" -- so question is, whether to automatically Coerce (Promote?) Double into BigDecimal to allow value to be represented.

Or maybe we want 2 features? This one for NaN -> Double override (even if otherwise prefer BigDecimal), and then new one for Too Big/Small Double -> BigDecimal promotion.

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
3 participants