Closed matteobertozzi closed 7 months ago
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()
(https://github.com/FasterXML/jackson-core/issues/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?
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
)
Added overrides for all textual format backends: will see if I can add better defaulting for getNumberTypeFP()
.
@matteobertozzi As per
https://github.com/FasterXML/jackson-core/pull/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.
Hi,
Thanks for the explanation. It make sense.
Also the default getNumberTypeFP() implementation from https://github.com/FasterXML/jackson-core/pull/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 https://github.com/FasterXML/jackson-core/pull/1235 as default implementation. unless _currToken has the wrong VALUE_NUMBER_INT/VALUE_NUMBER_FLOAT value.
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):
BigDecimal
sDouble
sthis does not affect other cases like:
Map
value type) defined as one number types (float
/Float
/double
/Double
/BigDecimal
): if so that type is usedDouble
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.
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
cool, thanks for your time and the detailed explanations. really appreciated!
Describe your Issue
2.17 introduced .getNumberTypeFP() in JsonParser, which defaults to NumberTypeFP.UNKNOWN
when trying to run this code
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
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
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.
What do you think?