FasterXML / jackson-dataformats-binary

Uber-project for standard Jackson binary format backends: avro, cbor, ion, protobuf, smile
Apache License 2.0
316 stars 136 forks source link

Java float deserialized as `DoubleNode` instance #28

Closed teabot closed 8 years ago

teabot commented 8 years ago

When parsing JSON documents with a schema described by suitably annotated Java POJO classes, Jackson represents float values with DoubleNode instances, not FloatNode instances as might be expected. This causes problems with other downstream libraries that attempt to interpret the node tree. There are situations where they may call isFloat() on the respective nodes, expecting it to return true for nodes that were derived from float properties. However, in these circumstances DoubleNode.isFloat() is invoked yielding a return value of false and causing said libraries to raise an error.

I believe I have traced this behaviour back to: com.fasterxml.jackson.databind.deser.std.BaseNodeDeserializer._fromFloat(...)

The last call to the nodeFactory returns a DoubleNode not a FloatNode. I wonder if this should instead be implemented in a similar manner to _fromInt(...) like so:

    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)) {
            return nodeFactory.numberNode(p.getDecimalValue());
        }
        if (nt == JsonParser.NumberType.DOUBLE) {              // PROPOSED CHANGE
            return nodeFactory.numberNode(p.getDoubleValue()); // PROPOSED CHANGE
        }                                                      // PROPOSED CHANGE
        return nodeFactory.numberNode(p.getFloatValue());      // PROPOSED CHANGE
    }

I am currently working around this by patching the downstream library to do something like this:

if (node.isDouble() && node.doubleValue() >= -Float.MAX_VALUE
    && node.doubleValue() <= Float.MAX_VALUE || node.isFloat()) {
        return datum.floatValue();

Reference: problem discussion on jackson-user

cowtowncoder commented 8 years ago

Yes, sounds like a problem. Probably related to the fact that JSON does not express difference (since there's just textual representation), whereas binary formats typically do use different underlying encoding.

teabot commented 8 years ago

Is this not an issue for jackson-databind? It's not necessarily Avro specific, and the code in question resides in that project?

cowtowncoder commented 8 years ago

@teabot correct, may well belong in jackson-databind. But it needs to be verified against one of backends that do have native float type, so this repo should have something, although possibly smile, cbor and protobuf need verifications too (since there is some interaction between components).

cowtowncoder commented 8 years ago

Fixed for 2.7.8 / 2.8.3. May take a while until released as 2.7.7 and 2.8.2 just went out few days ago.