Skip to content

Commit

Permalink
Fix #1028
Browse files Browse the repository at this point in the history
  • Loading branch information
cowtowncoder committed May 20, 2016
1 parent 073fc19 commit c809c0c
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 2 deletions.
4 changes: 4 additions & 0 deletions release-notes/CREDITS
Original file line number Diff line number Diff line change
Expand Up @@ -478,3 +478,7 @@ Maarten Billemont (lhunath@github)
* Suggested #1184: Allow overriding of `transient` with explicit inclusion with `@JsonProperty`
(2.8.0)

Vladimir Kulev (lightoze@github)
* Reported #1028: Ignore USE_BIG_DECIMAL_FOR_FLOATS for NaN/Infinity
(2.8.0)

2 changes: 2 additions & 0 deletions release-notes/VERSION
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ Project: jackson-databind
(contributed by mkokho@github)
#1017: Add new mapping exception type ('InvalidTypeIdException') for subtype resolution errors
(suggested by natnan@github)
#1028: Ignore USE_BIG_DECIMAL_FOR_FLOATS for NaN/Infinity
(reported by Vladimir K, lightoze@github)
#1082: Can not use static Creator factory methods for `Enum`s, with JsonCreator.Mode.PROPERTIES
(contributed by Lokesh K)
#1084: Change `TypeDeserializerBase` to take `JavaType` for `defaultImpl`, NOT `Class`
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.fasterxml.jackson.databind.deser.std;

import java.io.IOException;
import java.math.BigDecimal;

import com.fasterxml.jackson.core.*;
import com.fasterxml.jackson.databind.*;
Expand Down Expand Up @@ -364,10 +365,18 @@ protected final JsonNode _fromFloat(JsonParser p, DeserializationContext ctxt,
final JsonNodeFactory nodeFactory) throws IOException
{
JsonParser.NumberType nt = p.getNumberType();
if (nt == JsonParser.NumberType.BIG_DECIMAL
|| ctxt.isEnabled(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS)) {
if (nt == JsonParser.NumberType.BIG_DECIMAL) {
return nodeFactory.numberNode(p.getDecimalValue());
}
if (ctxt.isEnabled(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS)) {
// 20-May-2016, tatu: As per [databind#1028], need to be careful
// (note: JDK 1.8 would have `Double.isFinite()`)
double d = p.getDoubleValue();

This comment has been minimized.

Copy link
@asnare

asnare Jul 22, 2016

This line introduces a bug, I believe. In particular, it causes a loss of precision that you're probably trying to avoid if you're using BigDecimal.

This comment has been minimized.

Copy link
@cowtowncoder

cowtowncoder Jul 22, 2016

Author Member

Certainly seems that way... not sure what I was thinking. What did #1028 say?

This comment has been minimized.

Copy link
@asnare

asnare Jul 22, 2016

So that was all about detecting NaN/Infinity, and falling back to Double because BigDecimal can't handle them.

I must confess I'm not familiar with the parser state… if NumberType above is BIG_DECIMAL then everything should be golden. But I haven't dug into everything here… I'm working back from a failing test in my code due to precision loss and it follows this path.

At a glance, I also miss a feature check on ALLOW_NON_NUMERIC_NUMBERS: without that NaN/Inf are illegal.

This comment has been minimized.

Copy link
@cowtowncoder

cowtowncoder Jul 22, 2016

Author Member

Yup, something is fishy here. Could you please file a new issue, referencing this commit as context?

This comment has been minimized.

Copy link
@asnare

asnare Jul 27, 2016

@cowtowncoder Done, as #1315.

(Sorry for the delay… holidays.)

if (Double.isInfinite(d) || Double.isNaN(d)) {
return nodeFactory.numberNode(d);
}
return nodeFactory.numberNode(BigDecimal.valueOf(d));
}
return nodeFactory.numberNode(p.getDoubleValue());
}

Expand Down
7 changes: 7 additions & 0 deletions src/test/java/com/fasterxml/jackson/databind/BaseMapTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ protected static class LongWrapper {
public LongWrapper() { }
public LongWrapper(long value) { l = value; }
}

protected static class DoubleWrapper {
public double d;

public DoubleWrapper() { }
public DoubleWrapper(double value) { d = value; }
}

/**
* Simple wrapper around String type, usually to test value
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
package com.fasterxml.jackson.databind.node;

import com.fasterxml.jackson.databind.*;

public class NotANumberConversionTest extends BaseMapTest
{
public void testBigDecimalWithNaN() throws Exception
{
ObjectMapper m = new ObjectMapper();
m.enable(DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS);

JsonNode tree = m.valueToTree(new DoubleWrapper(Double.NaN));
assertNotNull(tree);
String json = m.writeValueAsString(tree);
assertNotNull(json);

tree = m.valueToTree(new DoubleWrapper(Double.NEGATIVE_INFINITY));
assertNotNull(tree);
json = m.writeValueAsString(tree);
assertNotNull(json);

tree = m.valueToTree(new DoubleWrapper(Double.POSITIVE_INFINITY));
assertNotNull(tree);
json = m.writeValueAsString(tree);
assertNotNull(json);
}
}

0 comments on commit c809c0c

Please sign in to comment.