FasterXML / jackson-databind

General data-binding package for Jackson (2.x): works on streaming API (core) implementation(s)
Apache License 2.0
3.53k stars 1.38k forks source link

Cannot deserialize value of type `java.math.BigDecimal` from String "3." (not a valid representation) #4577

Closed dmelisso closed 5 months ago

dmelisso commented 5 months ago

Search before asking

Describe the bug

This bug probably heavily relates to https://github.com/FasterXML/jackson-databind/issues/4435 which was previously fixed using https://github.com/FasterXML/jackson-core/pull/1241

Since jackson 2.17 while deserializing BigDecimals (or integers) from JsonInput with a string value of the format ".f" e.g. {"value":"3."} we get an exception that looks like the following:

com.fasterxml.jackson.databind.exc.InvalidFormatException: Cannot deserialize value of type `java.math.BigDecimal` from String "5.": not a valid representation
 at [Source: REDACTED (`StreamReadFeature.INCLUDE_SOURCE_IN_LOCATION` disabled); line: 1, column: 10] (through reference chain: TestDTO["value"])

    at com.fasterxml.jackson.databind.exc.InvalidFormatException.from(InvalidFormatException.java:67)
    at com.fasterxml.jackson.databind.DeserializationContext.weirdStringException(DeserializationContext.java:1958)
    at com.fasterxml.jackson.databind.DeserializationContext.handleWeirdStringValue(DeserializationContext.java:1245)
    at com.fasterxml.jackson.databind.deser.std.NumberDeserializers$BigDecimalDeserializer.deserialize(NumberDeserializers.java:1058)
    at com.fasterxml.jackson.databind.deser.std.NumberDeserializers$BigDecimalDeserializer.deserialize(NumberDeserializers.java:990)
    at com.fasterxml.jackson.databind.deser.impl.MethodProperty.deserializeAndSet(MethodProperty.java:129)
    at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:310)
    at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:177)
    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:3848)
    at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3816)
    at BigDecimalTest.shouldDeserializeBigDecimalFromJacksonDataBind(BigDecimalTest.java:26)
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
    at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

I am not sure if this deserialization behavior change is intended or not. However, both Java and javascript appear to understand "5." as a number.

Version Information

2.17 and 2.17.1

Reproduction

The reproduction steps are equivalent to the steps described in https://github.com/FasterXML/jackson-databind/issues/4435

i.e., in https://github.com/EAlf91/jackson-issue simply switch the test case from ".05" to "5."

Expected behavior

If this change in behavior is not intended, I would expect jackson to parse "3." as a decimal, (i.e., 3.0), or as an integer (i.e., 3)

Additional context

See the related issue https://github.com/FasterXML/jackson-databind/issues/4435 that describes a similar bug that was previously fixed

Without knowing the jackson codebase, I would expect that the allowed pattern for https://github.com/pjfanning/jackson-core/blob/1c60872851893a8994bfa7131edcb13e2e67e4fe/src/main/java/com/fasterxml/jackson/core/io/NumberInput.java#L41 should be modified to also allow float number definitions which do not have numerals after the period.

JooHyukKim commented 5 months ago

Makes sense, the fix would most likely be in Jackson-core module tho.

pjfanning commented 5 months ago

I think we should consider removing looksLikeValidNumber. The check is going to get so complicated or so lax that it just acts as performance drain with little benefit.

cowtowncoder commented 5 months ago

No, not a bug unless "3." is a valid JSON number -- which I don't think it is. Java or Javascript rules are not the driver, JSON specification. This is different from ".05" which is valid JSON number representation

In some cases from-String conversions may allow some invalid numbers: this is unfortunate, but solution is not to allow more invalid numbers.

As to looksLikeValidNumber(), if I remember its existence has to do with maximum number length constraint; idea being that it catches most invalid number cases before attempt to parse. It is only used for "from-String" cases: native numbers are decoded by JsonParser itself.

cowtowncoder commented 5 months ago

Checked https://www.json.org/json-en.html: number ending with . is not a valid JSON Number. Closing.

pjfanning commented 5 months ago

@cowtowncoder Is it possible that this worked in older versions of Jackson - eg v2.16? If it used to work, maybe we should add a Feature that allows users to choose to accept '3.' as valid again.

cowtowncoder commented 5 months ago

It is possible, but I don't think there has been intention to support non-JSON numbers. It is regrettable if such numbers were not failing in the past but I don't want to go on making point changes since there is no good definition of what would be acceptable.

For this particular case I would recommend implementing custom BigDecimal deserializer.

dmelisso commented 5 months ago

Thank you all of the amazingly quick response. I agree it's not part of JSON standard and it complexifies implementation that it would not make sense to add this feature.

I have one last question though about https://github.com/FasterXML/jackson-databind/issues/4577 and https://www.json.org/json-en.html

If I read the diagram for "number" correctly in https://www.json.org/json-en.html then numbers like ".5" should also not be permitted. However, https://github.com/FasterXML/jackson-core/pull/1241 seems to have re-added the feature.

I am not sure if you truly intended to allow ".5" back into jackson, if you want to stay strict with the JSON specification standard.

cowtowncoder commented 5 months ago

Thank you for checking @dmelisso . I could have sworn that .5 was valid but if not, yes, that seems like something that ideally would not also not be supported. But then again blocking that such values in 2.x probably will affect someone (break someone's handling), similar to change discussed here (unintentional).

Given this information, I think what would make sense is doing 2 things:

  1. For 2.(17?), allow 3. again (change validation)
  2. For 3.0, dis-allow both .5 and .3.

I will re-open this issue, file new one for (2)

cowtowncoder commented 5 months ago

Hmmmh. Ok, re-thinking this one again: while for JSON processing this might make sense, I realize that other Jackson backends -- like XML and YAML -- have different number representations. And while this is fine for "native" numbers (values that are numbers within data formats) -- because there is specific YAMLParser or FromXmlParser to consider format-specific rules, same is NOT true for "stringified" numbers, because parser cannot do validation (needed coercion/conversion happens at high level). And instead, format-agnostic handling (with JsonSerializer implemnetation) is applied.

What this means is that for Stringified numbers we have no way to adapt to format-specific rules: and I think that -- just as an example -- both .3 and 3. are actually valid YAML numbers; and may well be (but I'm less sure about this) be legal XML (schema) numbers as well. If so, using stricter, JSON-based representation rules makes no sense with those backends.

So: while there is no quick solution here (other than adding support for such coercion via JsonParsre, something we could consider), it seems sensible to actually allow more lenient handling for Stringified numbers when decoded by databind, from native String value. Sort of superset of allowed representations.

I realize that this won't make it easier for users/developers to deduce rules, but I think it probably the most pragmatic thing to do. And at least for now apply this to 2.x as well as 3.0, not changing logic until we have something better to offer.

cowtowncoder commented 5 months ago

To be fixed via https://github.com/FasterXML/jackson-core/issues/1308

cowtowncoder commented 5 months ago

This is now fixed for Jackson 2.17; to be released as 2.17.2 (and 2.18.0).

Actually fix is in jackson-core; databind has tests to verify functioning.