Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combination of @JsonUnwrapped and @JsonAnySetter results in BigDecimal instead of Double #3277

Closed
hjohn opened this issue Sep 15, 2021 · 9 comments
Milestone

Comments

@hjohn
Copy link

hjohn commented Sep 15, 2021

Describe the bug
When deserializing a float value to a structure nested in JsonUnwrapped and JsonAnySetter, the float value becomes a BigDecimal instead of Double (USE_BIG_DECIMAL_FOR_FLOATS is left to its default setting).

Version information
2.12.4

To Reproduce

@Test
public void test() throws JsonParseException, JsonMappingException, IOException {
    ObjectMapper mapper = new ObjectMapper();

    Holder holder = mapper.readValue("{\"value1\": -60.0, \"value2\": -60.0}", Holder.class);

    assertEquals(Double.class, holder.value1.getClass());
    assertEquals(Double.class, holder.holder2.data.get("value2").getClass());
}

public static class Holder {
    private Object value1;
    @JsonUnwrapped private Holder2 holder2;

    public Object getValue1() {
        return value1;
    }

    public void setValue1(Object value1) {
        this.value1 = value1;
    }
}

public static class Holder2 {
    private Map<String, Object> data = new HashMap<>();

    @JsonAnyGetter
    public Map<String, Object> getData() {
        return data;
    }

    @JsonAnySetter
    public void setAny(String key, Object value) {
        data.put(key, value);
    }
}

Expected behavior
Would still expect a Double here.

@hjohn hjohn added the to-evaluate Issue that has been received but not yet evaluated label Sep 15, 2021
@hjohn
Copy link
Author

hjohn commented Sep 15, 2021

Workaround:

@JsonAnySetter
void setAny(String key, Object value) {
    parameters.put(key, value instanceof BigDecimal bd ? bd.doubleValue() : value);
}

@yawkat
Copy link
Member

yawkat commented Sep 16, 2021

Looks to be caused by the fix for #2644. For that fix, the TokenBuffer buffers the exact value because the necessary precision can't be determined beforehand (the target type might turn out to be BigDecimal later). When the value is then read into an Object field, like in your any setter case, it then simply uses the already buffered number value, meaning BigDecimal.

Not sure if changing back to double downstream is a good idea. After all, the main reason for USE_BIG_DECIMAL_FOR_FLOATS as false is to avoid the perf impact of BigDecimal, but in this case the BigDecimal is already present anyway.

@hjohn
Copy link
Author

hjohn commented Sep 16, 2021

True, however, code does sort of expect a double since USE_BIG_DECIMAL_FOR_FLOATS is set to false, so it was a bit surprising to see values, which used to be doubles, change to BigDecimal because of a structural change (using JsonUnwrapped/JsonAnySetter).

The work-around is fine for now, but I would think that when filling these values it should check the USE_BIG_DECIMAL_FOR_FLOATS setting and convert them back to doubles if it is set to false (or some other solution which avoids this in the first place obviously, but since there is some buffering involved that might be hard...

Perhaps it would have been a good idea to not do any interpretation in the TokenBuffer (leave it as strings/text) but I have insufficient knowledge of the internal workings of Jackson to say if that is a good idea or not.

@cowtowncoder
Copy link
Member

Quick note: this (double <-> BigDecimal coercions) is a known issue, and I think there are existing filed issue(s) for it.

The problem is tricky to solve for a few reasons. Basically:

  1. Some formats (binary ones) do have clear type which we can use, and should retain original type as "the" natural type. Others (JSON, XML, CSV, Properties) does not, and thereby it is all about what caller expects or asks for.
  2. For latter type ("contextual natural/expected type"), we should defer commitment to as late as possible -- up to and including TokenBuffer
  3. To support (2), we'd probably need to figure out a way for lazy/deferred decoding: so textual parser simply validates that representation is legit FP, but does not decode value (handling of double / BigDecimal is different)
  4. Although BigDecimal is a superset, and theoretically we could always start with it (minus overflows etc?), it also has, I think, more overhead so always defaulting to it can be expensive
  5. Support USE_BIG_DECIMAL_FOR_FLOATS == true is much easier; false is ... tricky.

So. Alas, it's a tricky problem altogether.

@cowtowncoder cowtowncoder removed the to-evaluate Issue that has been received but not yet evaluated label Sep 28, 2021
@seadbrane
Copy link

We ran into this issue when upgrading to 2.12 and I see that it is also not fixed in 2.13.
It is also a bit interesting that just calling p.getNumberType() changes the behavior and results in a double instead of defaulting to BigDecimal.
Are there any plans for fix? Maybe adding a new DeserializationFeature to preserve the existing behavior?

@seadbrane
Copy link

Any update on this issue?

@pjfanning
Copy link
Member

@seadbrane do you want to try Jackson 2.16.0 yourself? There have been some changes in number handling. This issue was not addressed directly but it's possible the behaviour has changed due to the other number changes in Jackson.

@JooHyukKim
Copy link
Member

JooHyukKim commented Dec 13, 2023

There have been some changes in number handling. This issue was not addressed directly but it's possible the behaviour has changed due to the other number changes in Jackson.

@pjfanning was right on point. It works now.

Please take a look at the #4259? @seadbrane 👍🏼

@seadbrane
Copy link

Yes, not able to repro anymore either. Thanks for the update.

@cowtowncoder cowtowncoder changed the title Combination of JsonUnwrapped and JsonAnySetter results in BigDecimal instead of Double Combination of @JsonUnwrapped and @JsonAnySetter results in BigDecimal instead of Double Dec 13, 2023
@cowtowncoder cowtowncoder added this to the 2.16.0 milestone Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants