2.17-rc1 UntypedObjectDeserializerNR/_deserializeFP() behaviour change with getNumberTypeFP() UNKNOWN

[matteobertozzi]

Describe your Issue

2.17 introduced .getNumberTypeFP() in JsonParser, which defaults to NumberTypeFP.UNKNOWN

when trying to run this code

@Test
@SuppressWarnings("unchecked")
public void testNumberTypeFP() throws IOException {
  final byte[] enc = mapper.writeValueAsBytes(Map.of(
    "float", 10.7f,
    "double", 128.5
  ));
  final Map<String, Object> x = mapper.readValue(enc, Map.class);
  Assertions.assertEquals(Float.class, x.get("float").getClass());  // with 2.17-rc1 we get Double.class
  Assertions.assertEquals(Double.class, x.get("double").getClass());
}

with 2.16.1 everything is ok, with 2.17-rc1 the Float.class assert fails with Double.class

In 2.16.1 UntypedObjectDeserializerNR calls JsonParser.getNumberType() and the Parser is able to return FLOAT

at io.github.matteobertozzi.yajbe.YajbeParser.getNumberValue(YajbeParser.java:386)
at com.fasterxml.jackson.databind.deser.std.UntypedObjectDeserializerNR.deserialize(UntypedObjectDeserializerNR.java:92)
at com.fasterxml.jackson.databind.deser.std.MapDeserializer._readAndBindStringKeyMap(MapDeserializer.java:623)
at com.fasterxml.jackson.databind.deser.std.MapDeserializer.deserialize(MapDeserializer.java:449)
at com.fasterxml.jackson.databind.deser.std.MapDeserializer.deserialize(MapDeserializer.java:32)
at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:342)
at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4899)
at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3907)

without any changes to the parser 2.17-rc1 goes directly to JsonParser.getDoubleValue(), the _deserializeFP() does not consider the NumberTypeFP.UNKNOWN case and fallsback to .getDoubleValue()
https://github.com/FasterXML/jackson-databind/blob/2.17/src/main/java/com/fasterxml/jackson/databind/deser/std/UntypedObjectDeserializerNR.java#L358

at io.github.matteobertozzi.yajbe.YajbeParser.getDoubleValue(YajbeParser.java:459)
at com.fasterxml.jackson.databind.deser.std.UntypedObjectDeserializerNR._deserializeFP(UntypedObjectDeserializerNR.java:370)
at com.fasterxml.jackson.databind.deser.std.UntypedObjectDeserializerNR.deserialize(UntypedObjectDeserializerNR.java:89)
at com.fasterxml.jackson.databind.deser.std.MapDeserializer._readAndBindStringKeyMap(MapDeserializer.java:623)
at com.fasterxml.jackson.databind.deser.std.MapDeserializer.deserialize(MapDeserializer.java:449)
at com.fasterxml.jackson.databind.deser.std.MapDeserializer.deserialize(MapDeserializer.java:32)
at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:342)
at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4905)
at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3909)

so, it seems that with 2.17-rc1 every JsonParse must have to implement .getNumberTypeFP() to get the proper behaviour.
but I think that if we return .getNumberValue() when NumberTypeFP.UNKNOWN we get back to the behaviour we have in 2.16.

// @since 2.17
protected Object _deserializeFP(JsonParser p, DeserializationContext ctxt) throws IOException
{
	JsonParser.NumberTypeFP nt = p.getNumberTypeFP();
	if (nt == NumberTypeFP.UNKNOWN) {
		return p.getNumberValue();
	}

What do you think?

[cowtowncoder]

Short: I think that pre-2.17 behavior was wrong (as in: not intended) and if I understand this correctly 2.17.0-rc1 works along what I think it should do.
This for default handling with JSON JsonParser.
Bit longer explanation follows, regarding that handling.

Fundamentally JSON has no notion of type for numbers, but for integral types we have actual limits to check for to select "minimal" type (chosen as int, long or BigInteger, not smaller types).
For FP types there is nothing to tell what to use: traditionally default was Double, but there are configuration settings to force use of BigDecimal.
So: which type should be used in case of, say, Map<String, Object> (or Number etc)?
Originally the idea was to use getNumberType() since some formats do have specific type used for encoding (all binary formats, some textual).

But the original getNumberType() had the problem of essentially forcing type with conversion since it wasn't allowed to say "don't know" -- so it was not possible to know if type is truly what exists as the true number (binary formats), or simply preference by configuration (part of which is not even known to JsonParser, from databind level).

Addition of getNumberTypeFP() (FasterXML/jackson-core#1149) , then, is simply to allow returning either actual encoding type (like CBOR, Smile, Avro, Protobuf do) or UNKNOWN.
In case of UNKNOWN default conversion rules are used.
Change to existing behavior of getNumberType() seemed risky (... ironically enough considering addition of new method might not be any less so, in hindsight), so addition of the new method was chosen.
And unfortunately it could not default to old method due to new return type (needed for "unknown" option).

Now: If I understand your case correctly there is custom format parser backend, which had provided proper expected target type to use. You are correct in that
In such case you would indeed need to implement getNumberTypeFP() to indicate desired type.

I realize this is unfortunate wrt implementation requirement that is new and surprising: I must admit I was focused on format backends provided by Jackson project and did not consider external use cases, such as yours.
Otherwise it might have made sense to try to split implementation phase into 2 -- introduction of the new method, testing -- and then version + 1 (2.18) start using it.

Does this help?

[cowtowncoder]

Come to think of this now, there probably would be a way to implement default JsonParser.getNumberTypeFP() in JsonParser that would be more backwards compatible: calling getNumberType() and translating value.

As long as all backends that do NOT know the type (JSON, all textual format backends) override this with

    @Override // added in 2.17
    public NumberTypeFP getNumberTypeFP() throws IOException {
        return NumberTypeFP.UNKNOWN;
    }

things should then work. I am not 100% sure I have time to work on this, but present this idea if you had time to help?

I will add explicit override for JSON backend now, fwtw (jackson-core, class JsonParserBase)

[cowtowncoder]

Added overrides for all textual format backends: will see if I can add better defaulting for getNumberTypeFP().

[cowtowncoder]

@matteobertozzi As per

FasterXML/jackson-core#1235

I changed getNumberTypeFP() implementation to delegate to getNumberType() by default -- so if you have a chance to try out 2.17.0-SNAPSHOT (built locally or from Sonatype OSS snapshot repo), it should work better without overrides.

[matteobertozzi]

Hi,

Thanks for the explanation. It make sense.

Also the default getNumberTypeFP() implementation from FasterXML/jackson-core#1235 works and brings the behaviour closer to what was before. (In my code i did the override of .getNumberTypeFP() with the same logic based on .numberType() )

Only one thing, when you say In case of UNKNOWN default conversion rules are used.
Here you mean that we expect the number to be a floating point so we default to double, which is the reasonable default?

In case of UntypedObjectDeserializerNR._deserializeFP() I don't see a rule for UNKNOWN that allow a custom implementation to decide what to do like .getNumberValue() does, and it defaults to double.
https://github.com/FasterXML/jackson-databind/blob/2.17/src/main/java/com/fasterxml/jackson/databind/deser/std/UntypedObjectDeserializerNR.java#L358
but I don't think it is a problem, especially with FasterXML/jackson-core#1235 as default implementation. unless _currToken has the wrong VALUE_NUMBER_INT/VALUE_NUMBER_FLOAT value.

[cowtowncoder]

What I mean by "default rules" means configuration settings to use for non-specific number target type (Object, Number, JsonNode), if UNKNOWN is returned.
Mainly it's DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS (default: false):

1. If enabled, all FP values exposed as BigDecimals
2. If disabled, all FP values exposed as Doubles

this does not affect other cases like:

1. Target type (POJO property type, Map value type) defined as one number types (float/Float/double/Double/BigDecimal): if so that type is used
2. Format returns non-UNKNOWN; use type indicated
3. for NaN values, use Double since BigDecimal has no type (or in rare case of JsonNode, JsonNodeFeature.FAIL_ON_NAN_TO_BIG_DECIMAL_COERCION, just fail)

But yes, with default (vanilla) configuration settings, Double would be use for JSON floating-point numbers.

[cowtowncoder]

As to UntypedObjectDeserializerNR, UNKNOWN... yeah, it looks as if support for DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS was missing?
I'll file a new issue to look into this, thanks!

-> #4415

[matteobertozzi]

cool, thanks for your time and the detailed explanations. really appreciated!