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

Support BigInteger and BigDecimal creators in StdValueInstantiator #2215

Closed
eatdrinksleepcode opened this issue Jan 3, 2019 · 6 comments
Closed
Milestone

Comments

@eatdrinksleepcode
Copy link

eatdrinksleepcode commented Jan 3, 2019

Currently using 2.9.6; have searched and don't see any planned changes in this area.

Basically, I would like this test to pass by default:

private static class BigDecimalWrapper {
    public final BigDecimal value;
    public BigDecimalWrapper(BigDecimal value) { this.value = value; }
}
@Test
public void createFromBigDecimal() throws IOException {
    assertThat(new ObjectMapper().readValue("42.5", BigDecimalWrapper.class).value, is(new BigDecimal("42.5")));
}

the same way that this one does:

private static class DoubleWrapper {
    public final Double value;
    public DoubleWrapper(Double value) { this.value = value; }
}
@Test
public void createFromDouble() throws IOException {
    assertThat(new ObjectMapper().readValue("42.5", DoubleWrapper.class).value, is(new Double("42.5")));
}

Currently, StdValueInstantiator does not support BigDecimal (or BigInteger) at all. I would like to add support for BigDecimal creators to createFromDouble, createFromLong, and createFromInt (and BigInteger creators to createFromLong and createFromInt) in StdValueInstantiator as widening conversions, the same way that createFromInt supports using long creators as a widening conversion.

@cowtowncoder
Copy link
Member

First of all, thank you for checking existing issues to see if there was previous work (there isn't AFAIK).
I can also see how this could be beneficial.

The only concern I have is regarding extension of auto-detection, and whether it is something that would be best supported by figuring out an even more general extension point for allowing single-argument auto-detected constructors (and factory methods -- currently only hard-coded valueOf() name, but could obviously be customizable as well).

But for 2.10, we could start by allowing BigDecimal as an "alias" of sort for double (and perhaps float too, if it is not yet), and similarly perhaps BigInteger.

One practical concern is that I would prefer not to add yet more _fromXxxCreator entries in StdValueInstantiator, if feasible -- but not sure if that can be avoided without adding other types of complexities.

@eatdrinksleepcode
Copy link
Author

I think a more general extension point is definitely worth discussion. In trying to work around this, I have been trying for the last couple of days to define an extension that I can add to my ObjectMapper to do this automatically (I don't want to use @JsonCreator or similar because I have a lot of these types in my code). I have so far been unsuccessful; neither custom deserializers nor custom value instantiators seem capable of solving this at a general level.

I understand the concern about adding creators to StdValueInstantiator, but I'm not sure I see another way of achieving this that is remotely consistent with the current implementation.

@cowtowncoder cowtowncoder added 2.12 and removed 2.10 labels Apr 12, 2020
@cowtowncoder
Copy link
Member

cowtowncoder commented May 8, 2020

Another thing I'll add as a note is the problem wrt double expanding to BigDecimal: that is, using 2-based double, with possible lossy value, into fully accurate BigDecimal.
Ideally value would be parsed as BigDecimal, passed, and no (possibly lossy) conversion occurs.
This for use cases like monetary values where 0.1 can be accurately represented as BigDecimal but NOT as 2-based double or float (although in practice conversion in Java may seem safe... but actual value is off by a bit).

But technically speaking it seems quite doable to just add new overload for createFromDouble() (or createFromBigDecimal()), as well as add createFromBigInteger(), add default implementation similar to existing methods.

In fact I'll tag this as "new contributor friendly"; if anyone wants to take a look.
If not I may come back to this before 2.12.0 finalized.

@cowtowncoder cowtowncoder added the good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project label May 8, 2020
@verkhovin
Copy link

verkhovin commented May 14, 2020

@cowtowncoder Hi! if you haven't picked it up yet, i would work on this

@cowtowncoder
Copy link
Member

@verkhovin I have not, so you are welcome to try it!

@cowtowncoder cowtowncoder removed the good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project label Jul 23, 2020
@cowtowncoder cowtowncoder added this to the 2.12.0 milestone Jul 23, 2020
@cowtowncoder
Copy link
Member

Implementation contributed by @upsidedownsmile, just merged.

@cowtowncoder cowtowncoder changed the title Support BigDecimal in StdValueInstantiator Support BigInteger and BigDecimal creators in StdValueInstantiator Jul 23, 2020
cowtowncoder added a commit that referenced this issue Jul 23, 2020
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

3 participants