Add configurable amount representations for Joda Money module #17

apankowski · 2021-10-03T15:38:31Z

This PR adds the possibility to configure desired representation of (de)serialized Money instances to the Joda Money Jackson module.

It allows selection of one of 3 representations via the JodaMoneyModule.withAmountRepresentation(AmountRepresentation) method:

DECIMAL_NUMBER - the default; amounts are (de)serialized as decimal numbers equal to the monetary amount, e.g. 12.34 for EUR 12.34,
DECIMAL_STRING - amounts are (de)serialized as strings containing decimal number equal to the monetary amount, e.g. "12.34" for EUR 12.34,
MINOR_CURRENCY_UNIT - amounts are (de)serialized as long integers equal to the monetary amount expressed in minor currency unit, e.g. 1234 for EUR 12.34, 12345 for KWD 12.345 or 12 for JPY 12.

The motivation for having such feature is that:

depending on the client implementation, passing monetary amounts as decimal numbers poses a risk of involving floating-point number representation, which may lead to loss of precision & rounding errors, especially in cases when some form of manipulation or arithmetic on the amounts is necessary,
some APIs as well as quite popular frontend monetary amount handling library Dinero.js require providing monetary amounts in minor currency units,
there are numerous APIs that use the decimal string (quoted decimal numbers) representation.

Noteworthy points:

I tried to align with the existing code style, but may have missed something.
I took the liberty of using parametrized tests with help of JUnitParams.
I tried to be careful to design the public parts of the new code to not expose too much and allow extensibility, but it would be good to review it thoroughly.
I believe the changes don't change the existing API in any breaking way nor do they change the existing behavior.
I have branched off of 2.13 (since it was the default) and made changes to code on that branch - not sure if that's correct.
I will leave comments for discussion wherever I'm not sure of something.
I'm not sure about the minimal JDK version we're targeting. I assumed JDK 8+ and therefore used Objects.requireNonNull and lambdas.

I will be helpful for pointers, suggestions and remarks and will try to align with all of them 🙂

joda-money/src/test/java/com/fasterxml/jackson/datatype/jodamoney/ModuleTestBase.java

cowtowncoder · 2021-10-03T21:38:25Z

joda-money/src/main/java/com/fasterxml/jackson/datatype/jodamoney/AmountRepresenter.java

+
+public interface AmountRepresenter<T> {
+
+    T write(Money money);


Either name is sub-optimal (nothing gets written), or (more likely I think) this should do actual writing via JsonGenerator directly. If former, I think this is something like AmountConverter, and methods could be something like fromMoney() and toMoney().

yeah, I struggled with the name so it's definitely sub-optimal. your proposal is great, I will use it, thanks

Sounds good!

cowtowncoder · 2021-10-03T21:41:27Z

joda-money/src/main/java/com/fasterxml/jackson/datatype/jodamoney/MoneySerializer.java

-        g.writeNumberField("amount", decimal.setScale(scale, RoundingMode.UNNECESSARY));
-        g.writeFieldName("currency");
-        context.defaultSerializeValue(money.getCurrencyUnit(), g);
+        g.writeObjectField("amount", amountRepresenter.write(money));


But doesn't this always chnge value from JSON Number into JSON String? And be essentially backwards-incompatible change?

This is why I think instead of write() conversion method it should be method that does write.
So, first g.writeFieldName("amount"), then value write by helper object.

it isn't output as JSON string. it goes like this:

BigDecimal is serialized using NumberSerializer (which does JsonGenerator.writeNumber(BigDecimal) just as we would do directly),

String is serialized using StringSerializer,

Long - using NumberSerializers.LongSerializer.

this seems to be confirmed by both manual tests and automatic tests in MoneySerializerTest.java

Ah. I did miss the paramaterization, yes, Long and BigDecimal will then be written using configured serializer which is fine.

cowtowncoder · 2021-10-03T21:47:37Z

First of all: thank you for contributing this patch! I haven't as much time to spend on things as I'd like, but I hope to provide some feedback.

I think the goals make sense, and I appreciate care taken. I did have some concerns (which may be due to my misreading?), added comments.

One practical thing is that due to changes to API, this would need to go in 2.14. I haven't yet created a branch for it, though, so for time being 2.13 makes most sense (master is for Jackson 3.0 that is further out).

Another thing is that more commonly formatting aspects would be handled via @JsonFormat annotation (esp. its shape), but I realize it may not be extensible enough here. It would allow differentiating between Number and String representation. It might work, since we do have:

Shape.STRING
Shape.NUMBER_FLOAT
Shape.NUMBER_INT (for "minor")
Shape.NUMBER would be ambiguous; I assume that'd be same as (2)

The main benefits would be that everything (?) would be configurable by type and/or property; you can annotate properties explicitly, or indicate default for given type.

So this would be something to consider if you have time. I can try to help fill in blanks on usage; there isn't much tutorial on how to do these unfortunately.

apankowski

thank you for the pointers, I've answered the comments and pushed an update 🙂

regarding:

more commonly formatting aspects would be handled via @jsonformat annotation (esp. its shape), but I realize it may not be extensible enough here. It would allow differentiating between Number and String representation. It might work, since we do have:

Shape.STRING

Shape.NUMBER_FLOAT

Shape.NUMBER_INT (for "minor")

Shape.NUMBER would be ambiguous; I assume that'd be same as (2)

The main benefits would be that everything (?) would be configurable by type and/or property; you can annotate properties explicitly, or indicate default for given type.

I actually wanted to approach something similar in a follow-up PR (to keep them smaller), but I can do it in this one if you prefer.

what I wasn't sure was if it will be natural for users (or: won't they be surprised) that a Money field is always serialized as JSON object, but @JsonFormat(shape = ) on such field controls not serialization of the field itself, but just the "amount" field inside of it. (I guess we could just document it of course.)

second thing I was considering was a dedicated annotation, something like:

@JsonMoney(amountRepresentation = MINOR_CURRENCY_UNIT)
Money grandTotal;

but I'm not sure if reading such custom annotation is feasible in (de)serializers, won't break something (e.g. caching of (de)serializers), etc.

what do you think is the way to go in Jackson's world? 🙂

apankowski · 2021-10-04T19:50:59Z

joda-money/src/main/java/com/fasterxml/jackson/datatype/jodamoney/AmountRepresenter.java

+
+public interface AmountRepresenter<T> {
+
+    T write(Money money);


yeah, I struggled with the name so it's definitely sub-optimal. your proposal is great, I will use it, thanks

apankowski · 2021-10-04T19:57:10Z

joda-money/src/main/java/com/fasterxml/jackson/datatype/jodamoney/MoneySerializer.java

-        g.writeNumberField("amount", decimal.setScale(scale, RoundingMode.UNNECESSARY));
-        g.writeFieldName("currency");
-        context.defaultSerializeValue(money.getCurrencyUnit(), g);
+        g.writeObjectField("amount", amountRepresenter.write(money));


it isn't output as JSON string. it goes like this:

BigDecimal is serialized using NumberSerializer (which does JsonGenerator.writeNumber(BigDecimal) just as we would do directly),

String is serialized using StringSerializer,

Long - using NumberSerializers.LongSerializer.

this seems to be confirmed by both manual tests and automatic tests in MoneySerializerTest.java

apankowski · 2021-10-20T19:06:16Z

hey @cowtowncoder, friendly ping 🙂

would be great if you could state what you prefer as outlined in this comment. then I could align this PR with whatever we agree 🙂

cowtowncoder · 2021-10-20T19:30:45Z

Yeah sorry have had bit of burnout wrt OSS work so feedback has been slow. But I do want to help here as this looks like a good improvement.

So: on @JsonFormat yeah... maybe that'd be unexpected; there is no precedent here.
I think going with the initial idea is fine; even if annotation support was added it could be incremental feature and not replacement (so module can define global defaults).

On getting this merged: with 2.13.0 this needs to go in 2.14; I haven't yet created 2.14 branches but hope to do that soon.

I'll add this on my TODO list and try to remember update once I get 2.14 branch set up for jackson-databind and this module.

apankowski · 2021-10-21T18:53:06Z

no worries @cowtowncoder, I get it 🙂

let's wait for 2.14 and do let me know if you spot anything that I should change 👍

cowtowncoder · 2021-10-23T17:02:27Z

2.14 branches exists for all repos now.

…ralizers

…ializers

…ney module to module's README

…thods

apankowski · 2021-10-24T08:54:41Z

@cowtowncoder roger, rebased on top of 2.14, verified all tests pass and updated PR 👍

cowtowncoder · 2021-11-09T04:40:50Z

Ok still being slow -- this is on top of my review list, hoping to get it merged this week.

cowtowncoder · 2021-11-24T18:35:24Z

@apankowski Apologies for yet more delays, but I'm back. I think things are good; only had couple of suggestions / questions:

Javadocs would be good for couple of classes; I'll add a brief comment.
---Would it make sense to allow specific "withAmountConverter()" for fully custom translations; in addition to more convenient enum-alternative?--- (NO NEED)
For backwards-compatibility may want to keep deprecated 0-args constructors for Money[De]Serializer.

These are fully optional; just LMK what you think and I can merge things as-is or if you want to add/change something let me know when things are ready.

Actually -- after thinking about this bit more, no, maybe NOT exposing AmountConverters initially is better.
That avoids issues wrt interface design etc.

It also reduces need to describe implementations: so just adding Javadocs to enum values would be sufficient.

joda-money/README.md

joda-money/src/main/java/com/fasterxml/jackson/datatype/jodamoney/AmountRepresentation.java

joda-money/src/main/java/com/fasterxml/jackson/datatype/jodamoney/MoneyDeserializer.java

joda-money/src/main/java/com/fasterxml/jackson/datatype/jodamoney/MoneySerializer.java

joda-money/src/main/java/com/fasterxml/jackson/datatype/jodamoney/AmountConverter.java

apankowski · 2021-11-24T19:50:45Z

@cowtowncoder no worries - thank you for the feedback 🙂

Will apply all agreed changes and let you know soon. I could use some guidance with visibility of the AmountConverters:

leave as is: AmountConverter and implementations are public (we document them a bit since they become part of the contract)
leave (documented) AmountConverter interface public but keep specific implementations package-private
make AmountConverter and specific implementations package-private

I can't decide how much we'd like to make public at first and could use your experience here

cowtowncoder · 2021-11-24T21:01:35Z

Good questions! Since we do not really expose any way to use them except via enums, my slight preference would be for (3). That leaves us freedom to change things more should there be need for more customizations, in ways that we haven't anticipated.

…c to package-private

…kward compatibility

…of amount representation enum

apankowski · 2021-11-25T20:59:35Z

@cowtowncoder PR updated and ready for next round of feedback 🙂

Summary of changes:

Added module registration code sample in REAMDE.
Removed generics from AmountConverter.
Reduced visibility of the amount representation-related classes to package-private except AmountRepresentation enum and corresponding JodaMoneyModule.withAmountRepresentation() representation selection method.
Brought back default public constructors for Money(De)Serializers to maintain compatibility with existing public API. Didn't add the @Deprecated annotation on them since API clients won't have another option to construct the (de)serializers directly (if they do it with existing API). The only other (indirect) way is by registering the JodaMoneyModule. Do you think we should add @Deprecated on default constructors of the (de)serializers and point clients to registering the JodaMoneyModule instead?
Added simple (but hopefully readable) JavaDocs to amount representation-related classes.

cowtowncoder · 2021-11-30T21:45:54Z

Looks good! I'll try to add some @since annotations but that can be done post-merge.

cowtowncoder · 2021-11-30T21:59:49Z

Thank you again @apankowski !

cowtowncoder added the 2.14 Intended for version 2.14.x label Oct 3, 2021

cowtowncoder reviewed Oct 3, 2021

View reviewed changes

joda-money/src/test/java/com/fasterxml/jackson/datatype/jodamoney/ModuleTestBase.java Show resolved Hide resolved

cowtowncoder reviewed Oct 3, 2021

View reviewed changes

apankowski commented Oct 4, 2021

View reviewed changes

apankowski added 5 commits October 24, 2021 10:43

Add possibility to select amount representation for Joda Money (de)se…

364740a

…ralizers

Add tests for different amount representations for Joda Money (de)ser…

865bb15

…ializers

Add description of configuration of amount representation for Joda Mo…

f7b5149

…ney module to module's README

Rename vague Representer classes to Converter along with interface me…

f5eb862

…thods

Use default field serialization in Joda Money serializer

d789f73

apankowski force-pushed the feature/add-joda-money-amount-representations branch from b7c562a to d789f73 Compare October 24, 2021 08:45

apankowski changed the base branch from 2.13 to 2.14 October 24, 2021 08:45