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

Ignore USE_BIG_DECIMAL_FOR_FLOATS for NaN/Infinity #1028

Closed
lightoze opened this issue Nov 29, 2015 · 6 comments
Closed

Ignore USE_BIG_DECIMAL_FOR_FLOATS for NaN/Infinity #1028

lightoze opened this issue Nov 29, 2015 · 6 comments
Milestone

Comments

@lightoze
Copy link

When calling valueToTree on a mapper with enabled USE_BIG_DECIMAL_FOR_FLOATS, it produces NumberFormatException while trying to convert NaN to BigDecimal.

ObjectMapper m = new ObjectMapper();
m.enable(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS);
Object o = new Object() {
    private double x = Double.NaN;

    public double getX() {
        return x;
    }

    public void setX(double x) {
        this.x = x;
    }
};
JsonNode tree = m.valueToTree(o);
System.out.println(tree);

In my data I need BigDecimal in some places where exact precision is required. Because Jackson does not automatically use BigDecimal for values which will not fit into Double (like it does for integers), I have to enable USE_BIG_DECIMAL_FOR_FLOATS, but still have to use doubles (which can be NaN) in other parts of the data structure.

I suggest that tokens with NumberType.FLOAT and NumberType.DOUBLE are tested for NaN/Infinity before calling getDecimalValue(). If positive, USE_BIG_DECIMAL_FOR_FLOATS should be ignored.

@cowtowncoder
Copy link
Member

Sounds like a bug. I am not sure what would be the best way to deal with this: quietly swallowing the problem may not be the best way to deal with this, given that caller would be expecting BigDecimal, not Double. But at very least the exception should be more descriptive.

@lightoze
Copy link
Author

The idea is that it must be possible to process JSON handling NaN/Infinity as Double and others as BigDecimal to assure 100% precision retention.
The problem also appears when e.g. you try to deserialize a NaN string to a tree and both USE_BIG_DECIMAL_FOR_FLOATS and ALLOW_NON_NUMERIC_NUMBERS are enabled.

@cowtowncoder
Copy link
Member

@lightoze unfortunately I am not sure if this is fully possible -- since, as per say:

http://stackoverflow.com/questions/28065158/java-bigdecimal-and-double-nan

there is no way to actually store these values in BigDecimal, it is more a matter of choosing exactly how the system fails. Quietly blocking coercion is a possibility, but I have learnt that hiding problems rarely solves the underlying issue; quite often it leads to even more difficult to track failures later on.
Add to this the fact that JSON format does not officially support non-numeric values either, and things get quite complex.

I will send a question on jackson-dev list, see what others think. I am not fully opposed to changing the behavior here, but this seems like a big mess. :)

@lightoze
Copy link
Author

Right, I also think it's not currently possible. The idea is to deserialize decimals as BigDecimal, but NaN as Double/Float. Of course, this applies only to the case when we deserialize to an Object or JsonNode, etc.
We could do that only when ALLOW_NON_NUMERIC_NUMBERS is enabled, so by default there will be no behaviour changes.

@cowtowncoder
Copy link
Member

Perhaps it is possible to improve things relatively easily by small modifications to TokenBuffer, so that at least it would not fail on trying to do coercion from non-numeric Double to BigDecimal.

@lightoze
Copy link
Author

There is also similar logic in BaseNodeDeserializer#_fromFloat, NumberDeserializer and UntypedObjectDeserializer(.Vanilla).
Interesting observation about NumberDeserializer - when deserializing from String token it already returns Double for NaN/Infinity ignoring USE_BIG_DECIMAL_FOR_FLOATS value.

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

No branches or pull requests

2 participants