Move jackson-datatype-money module from zalando #48

sri-adarsh-kumar · 2024-10-18T14:02:12Z

Moves jackson-datatype-money module from zalando

Context

Related to #5 and zalando/jackson-datatype-money#224

From the above conversations, there is a consensus to move the zalando/jackson-datatype-money library as a sub-module of this repository.

In order to achieve this, I have moved the files almost 1:1 from the source repository.

Only major changes were related to tests. The source repo had Junit Jupiter tests with @ParameterizedTest. The destination library already had other tests using junitParams.Parameters, so this was adopted.

Review Suggestions

Please focus on

How to use correct License
How to attribute credits correctly
Package naming conventions

javax-money/src/main/resources/META-INF/LICENSE

sri-adarsh-kumar · 2024-10-18T14:05:04Z

javax-money/src/moditect/module-info.java

@@ -0,0 +1,13 @@
+//TODO how is this generated


I am not sure how this file is generated.
Is there some documentation for this?

I think it would be running

../mvnw moditect:generate-module-info

from new modules project dir. Let me try this on PR.

This provides the base, but must be modified; looking at other modules's module-info.java for inspiration.

Hmmh. Alas, that command throws NPE.

on-behalf-of: Zalando OSS Community Co-authored-by: Willi Schönborn <[email protected]> Co-authored-by: Philipp Hirch <[email protected]> Co-authored-by: Jörn Horstmann <[email protected]> Co-authored-by: Lauri at Zalando <[email protected]> Co-authored-by: msparer <[email protected]> Co-authored-by: Alexander Yastrebov <[email protected]> Co-authored-by: Alexey Venderov <[email protected]> Co-authored-by: Alexander Yastrebov <[email protected]> Co-authored-by: Arnaud BOIVIN <[email protected]> Co-authored-by: Bartosz Ocytko <[email protected]> Co-authored-by: Carlos Freund <[email protected]> Co-authored-by: Dario Seidl <[email protected]> Co-authored-by: Georgios Andrianakis <[email protected]> Co-authored-by: Lauri at Zalando <[email protected]> Co-authored-by: Martin Prebio <[email protected]> Co-authored-by: Sean Sullivan <[email protected]> Co-authored-by: Touko Vainio-Kaila <[email protected]> Co-authored-by: lukasniemeier-zalando <[email protected]>

sri-adarsh-kumar · 2024-10-24T13:31:59Z

@cowtowncoder Please review when you have time.

javax-money/pom.xml

...x-money/src/test/java/com/fasterxml/jackson/datatype/money/CurrencyUnitDeserializerTest.java

...ney/src/test/java/com/fasterxml/jackson/datatype/money/CurrencyUnitSchemaSerializerTest.java

...money/src/test/java/com/fasterxml/jackson/datatype/money/MonetaryAmountDeserializerTest.java

cowtowncoder · 2024-10-25T01:58:54Z

javax-money/pom.xml

+
+        <dependency>
+            <groupId>org.javamoney.moneta</groupId>
+            <artifactId>moneta-core</artifactId>


Is this test-only dependency? Or does module require specific Money API implementation?

The module supports Money API implementations like moneta's FastMoney, Money and RoundedMoney.

Right: I was wondering if inclusion of specific implementation was problematic wrt using something else -- but I guess one would just use Maven dependency exclude, or some other mechanism.

Or put another way: I can see such dependency necessary for testing but wasn't sure it was really needed as regular ("compile") dependency. If you think it is needed that's fine: just double-checking.

Hi !
First, thanks for this jackson-datatype-money module to the Zalando team, we're using it for a long time and are really pleased that it's about to be more integrated to the Jackson ecosystem :)

Specifically on this thread :
I would say that if the module is about to be named javax-money, maybe it's worth reconsidering the support of the deserialization of org.javamoney.moneta.FastMoney, org.javamoney.moneta.Money, org.javamoney.moneta.RoundedMoney, 3 concrete classes belonging to Moneta, the only implementation of javax-money spec and support only the deserialization of javax.money.MonetaryAmount ?
Except for tests, the usage of the package org.javamoney is only located in MoneyModule.java so it seems quite straightforward to remove the support and get rid of moneta-core as a compile time dependency.
For us who use only javax.money.MonetaryAmount in our code, it would ease the dependency management since we won't have to fear to break this module if we update Moneta.
Maybe that would mean an additional module javax-money-moneta if deserializing those 3 classes is still needed by some ? This module could depend and reuse some code from the javax-money module though

@KeepItSimpleStupid Sounds like a good idea to me; modules should focus on doing one concrete thing well and just that (as general guideline).

@sri-adarsh-kumar I can go over this once more but it all sounds good for now.

One more thing: I know CLAs needed in general for future contributions as we discussed few weeks back. But for just this PR, I think that I would need CLA from you to cover initial merge. Is this something you could do? As I said, I don't need anything more at this point.

According to this comment, my work should be covered with the CCLA.
Did I understand that correctly?

If there was an CCLA, yes, but I haven't gotten one from Zalando. And based on discussions it sounded like there was hesitation to go with CLA. Either one is fine with me, but I do want this first inclusion to be covered by one or the other.

Based on this comment, I think we will make a CCLA following the merge.

@bocytko Is my assumption correct?

javax-money/src/main/java/com/fasterxml/jackson/datatype/money/CurrencyUnitDeserializer.java

cowtowncoder · 2024-10-25T02:01:30Z

javax-money/src/main/java/com/fasterxml/jackson/datatype/money/CurrencyUnitSerializer.java

+import static org.apiguardian.api.API.Status.MAINTAINED;
+
+@API(status = MAINTAINED)
+public final class CurrencyUnitSerializer extends StdSerializer<CurrencyUnit> {


Possibly should use StdScalarSerializer.

javax-money/pom.xml

cowtowncoder · 2024-10-25T02:05:21Z

javax-money/src/main/java/com/fasterxml/jackson/datatype/money/MonetaryAmountDeserializer.java

+    }
+
+    @Override
+    public Object deserializeWithType(final JsonParser parser, final DeserializationContext context,


If extending StdDeserializer (or, StdScaralDeserializer), wouldn't need to implement this method, I think.

cowtowncoder · 2024-10-25T02:06:42Z

javax-money/src/main/java/com/fasterxml/jackson/datatype/money/MonetaryAmountDeserializer.java

+    private void checkPresent(final JsonParser parser, @Nullable final Object value, final String name)
+            throws JsonParseException {
+        if (value == null) {
+            throw new JsonParseException(parser, format("Missing property: '%s'", name));


Sub-optimal exception as it does not indicate value type that has the issue.
Should usually be passed DeserializationContext and call one of methods it provides for throwing more specific (semantic) exceptions.

cowtowncoder · 2024-10-25T02:07:41Z

javax-money/src/main/java/com/fasterxml/jackson/datatype/money/MonetaryAmountSerializer.java

+    public void serializeWithType(final MonetaryAmount value, final JsonGenerator generator,
+            final SerializerProvider provider, final TypeSerializer serializer) throws IOException {
+
+        // effectively assuming no type information at all


... which is basically wrong :-(

(meaning, won't work with @JsonTypeInfo)

Thanks for the review comment.
Do you see this as a blocker to merge this PR?

As this is not strictly related to moving the module from Zalando repo, I would prefer to treat this as a future improvement.

No, I don't think it is blocker: good point on "let's move it first, then improve".

javax-money/src/main/java/com/fasterxml/jackson/datatype/money/MoneyModule.java

cowtowncoder · 2024-10-25T02:13:52Z

@sri-adarsh-kumar Ok, so technically I think this looks ok (although I have suggestions and concerns wrt code actually).
But one question I have is whether we need someone else's approval for contribution -- or are you the author? (I understand you work for Zalando from profile?).

I think we need one or more CLAs (from https://github.com/FasterXML/jackson/ either individual or Corporate CLA):

From you (wrt directly adding files here), regardless of authorship of code
Additional CLA(s) from whoever can contribute code itself (if not you).

It might be simplest to get CCLA from Zalando, although that is not a requirement if authors can give individual CLAs.

Changed License reference (WIP) Removed cross-module dependency to jsonSchema Testing specific Modules instead of findAndRegisterModules Assert subtypes of JSONProcessingException CurrencyUnitDeserializer extends StdScalarDeserializer CurrencyUnitSerializer extends StdScalarSerializer MonetaryAmountDeserializer throws semantic exceptions using DeserializationContext MoneyModule version uses PackageVersion

cowtowncoder · 2024-10-30T00:33:04Z

javax-money/src/main/java/com/fasterxml/jackson/datatype/money/MoneyModule.java

+
+
+@API(status = STABLE)
+public final class MoneyModule extends Module {


Should we actually rename this as JavaxMoneyModule?

... also, as a side-note, is there need for "Jakarta Money module"? (wrt javax licensing resulting in "forking" of javax APIs)

Let me check on this.

javax-money/src/moditect/module-info.java

...x-money/src/test/java/com/fasterxml/jackson/datatype/money/CurrencyUnitDeserializerTest.java

cowtowncoder · 2024-10-30T00:42:04Z

@sri-adarsh-kumar Thank you for updates, good job so far!

The other thing then has to do with CLAs, permissions -- see my earlier note.
If we can get that through, I should be able to merge this pr!
(getting it merged into master for 3.0 will have its challenges but one thing at a time :) )

sri-adarsh-kumar · 2024-11-05T10:01:47Z

Thanks for all the support.
I am working on the CLA process with my organisation.

cowtowncoder · 2024-11-05T22:34:47Z

javax-money/README.md

+
+```java
+ObjectMapper mapper = new ObjectMapper()
+        .registerModule(new MoneyModule());


Let's change to use new-er "addModule":

ObjectMapper mapper = JsonMapper.builder() .addModule(...) .build();

cowtowncoder · 2024-11-05T22:35:03Z

javax-money/README.md

+
+```java
+ObjectMapper mapper = new ObjectMapper()
+        .registerModule(new MoneyModule().withQuotedDecimalNumbers());


same here (addModule())

cowtowncoder · 2024-11-05T22:40:00Z

I think code is looking almost ready for merge, so just need to deal with CLA question and it could be merged!

sri-adarsh-kumar · 2024-12-18T09:22:18Z

We are discussing the CLA internally.

Our source repository has some contributions made by non-Zalando employees, so we are wondering how this should be treated.

The contributors have licensed it to us (and everyone else) under the MIT license.

Section 7 of the CLA asks to submit the work separately but as this contribution is from multiple people, we are not sure how this should be handled.

Do you have any suggestions?

cowtowncoder · 2024-12-18T18:05:28Z

Maybe marking license as MIT (in pom.xml and README.md, and LICENSE file for sub-module) would be enough. And getting CLA from just Zalando employees (or just individuals who plan to contribute) -- so CLA for future work -- would work?
Put another way, CLA would only needed for changes after inclusion (and from individuals handling addition); and keeping MIT license to cover initial contributions.

I definitely do not want you to have to try to chase down all contributors.

I am not a lawyer obviously but it seems to me the intent for licensing and CLAs would be compatible with this approach.

javax-money/src/main/resources/META-INF/LICENSE

bocytko · 2025-01-14T12:01:44Z

Maybe marking license as MIT (in pom.xml and README.md, and LICENSE file for sub-module) would be enough. And getting CLA from just Zalando employees (or just individuals who plan to contribute) -- so CLA for future work -- would work? Put another way, CLA would only needed for changes after inclusion (and from individuals handling addition); and keeping MIT license to cover initial contributions.

I definitely do not want you to have to try to chase down all contributors.

I am not a lawyer obviously but it seems to me the intent for licensing and CLAs would be compatible with this approach.

Thank you for this proposal @cowtowncoder. We discussed it with our lawyers and got green light for: MIT + Corporate CLA for Zalando employee contributions following the inclusion.

@sri-adarsh-kumar will initiate any remaining steps, if required for the merge.

cowtowncoder · 2025-01-14T16:21:01Z

@bocytko Sounds like we just need to ensure MIT LICENSE information for module (ideally as part of PR). And then CCLA, although that can follow merge and just pre-date further PRs.

cowtowncoder · 2025-01-25T00:49:55Z

javax-money/pom.xml

+        <dependency>
+            <groupId>org.apiguardian</groupId>
+            <artifactId>apiguardian-api</artifactId>
+            <version>1.1.2</version>


Also, this... if these are annotations, should probably use scope of provided?

At least it would seem like this would not be typical compile dependency for actual functionality?

…tatypes-misc into 2.19

KeepItSimpleStupid

A few comments, thank you :)

KeepItSimpleStupid · 2025-01-31T17:35:59Z

javax-money/src/main/java/com/fasterxml/jackson/datatype/money/JavaxMoneyModule.java

+            // for reading into concrete implementation types
+            deserializers.addDeserializer(implementationClass, new MonetaryAmountDeserializer<>(amountFactory, names));


I believe we don't need those lines (et the implementationClass field so) ? If we need to register additional serializers, it will be donein the MonetaMoneyModule you just introduced ?

While that may be true for a Moneta implementation, this line is needed to support any custom implementation of MonetaryAmount (along with it's factory).

But if a new implementation of Javax Money appears (I don't know any, but maybe you've one internal to Zalando ?), don't you think it should be treated like Moneta has been in this PR, with a dedicated Jackson module ?
If you preserve the amountFactory field, and build the JavaMoneyModule this way (assuming you hava a MyCustomImplementation implementing MonetaryAmount)

new JavaMoneyModule().withMonetaryAmountFactory(MyCustomImplemention::of)

You won't be able to deserialize fields of type MyCustomImplementation directly, that's true, but you'll know that all your deserialized MonetaryAmount fields are of concrete type MyCustomImplementation. So with one cast you could get back your MyCustomImplementation if really needed. But isn't the point of a spec to mask these implementation details ?

I see what you mean.
I have removed reference to implementationClass so we deserialize with an amount factory to MonetaryAmount type.

Thank you for taking the time to consider my comments, I'll do a last round of review and then it should be good for me, at least from a Javax Money user point of view !
I leave the Jackson considerations to @cowtowncoder ;)

KeepItSimpleStupid · 2025-01-31T17:48:17Z

javax-money/src/main/java/com/fasterxml/jackson/datatype/money/JavaxMoneyModule.java

+        if (Objects.nonNull(this.amountFactory) && Objects.nonNull(this.implementationClass)) {
+            deserializers.addDeserializer(MonetaryAmount.class, new MonetaryAmountDeserializer<>(amountFactory, names));
+            // for reading into concrete implementation types
+            deserializers.addDeserializer(implementationClass, new MonetaryAmountDeserializer<>(amountFactory, names));
+        }


I believe it could be replaced with the canonical way to build a new MonetaryAmount, i.e.

javax.money.Monetary.getDefaultAmountFactory() .setNumber(number) .setCurrency(currency) .create()

It will then delegate to the implementation found in the classpath (org.javamoney.moneta.Money by default with Moneta in the classpath)

So something like

Suggested change

if (Objects.nonNull(this.amountFactory) && Objects.nonNull(this.implementationClass)) {

deserializers.addDeserializer(MonetaryAmount.class, new MonetaryAmountDeserializer<>(amountFactory, names));

// for reading into concrete implementation types

deserializers.addDeserializer(implementationClass, new MonetaryAmountDeserializer<>(amountFactory, names));

}

deserializers.addDeserializer(MonetaryAmount.class, new MonetaryAmountDeserializer<>((amount, currency) -> Monetary.getDefaultAmountFactory()

.setNumber(number)

.setCurrency(currency)

.create(),

names));

Thanks for the pointer. I edited the method to use the SPI capabilities.

I didn't see the amountFactory field at first but I now get its usage.
Maybe the constructor passing a null amountFactory should get a lambda based on Monetary.getDefaultAmountFactory()...create() instead ? This way you would unify the logic (no need to check if amountFactory is null or not)

From this

public JavaxMoneyModule() { this(new DecimalAmountWriter(), FieldNames.defaults(), MonetaryAmountFormatFactory.NONE, null, null); }

to this

public JavaxMoneyModule() { this(new DecimalAmountWriter(), FieldNames.defaults(), MonetaryAmountFormatFactory.NONE, null, (amount, currency) -> Monetary.getAmountFactory(c) .setNumber(amount) .setCurrency(currency) .create()); }

Thanks for the suggestion. I have refactored as mentioned.

KeepItSimpleStupid · 2025-01-31T17:50:28Z

...oney-moneta/src/main/java/com/fasterxml/jackson/datatype/money/moneta/MonetaMoneyModule.java

+        deserializers.addDeserializer(CurrencyUnit.class, new CurrencyUnitDeserializer());
+        deserializers.addDeserializer(MonetaryAmount.class, new MonetaryAmountDeserializer<>(amountFactory, names));


I believe those 2 serializers are already registered by the JavaxMoneyModule whose setupModule is called a few line above, so no need to duplicate them here ?

You are correct. With the SPI change, this line has been removed.

KeepItSimpleStupid

A few more comments

KeepItSimpleStupid · 2025-02-04T12:57:25Z

javax-money/src/main/java/com/fasterxml/jackson/datatype/money/JavaxMoneyModule.java

+        //Scan all registered MonetaryAmount types and add a default deserializer for them
+        for (Class c : Monetary.getAmountTypes()) {
+            deserializers.addDeserializer(c, new MonetaryAmountDeserializer<>((amount, currency) -> Monetary.getAmountFactory(c).setNumber(amount).setCurrency(currency).create(), names));
+        }


I see the intent here, but this for-loop completely by-passes the amountFactory field, making the module inconsistent because it doesn't let the user customize the way that these amountTypes would be deserialized.
=> I suggest that you remove this for-loop and let this JavaxMoneyModule focus on deserializing to MonetaryAmount fields only. Deserializing to fields of type org.javamoney.moneta.* (and customizing it) will be handled solely by the MonetaMoneyModule (as already implemented)

I have updated the code as requested.

KeepItSimpleStupid · 2025-02-04T13:32:10Z

...oney-moneta/src/main/java/com/fasterxml/jackson/datatype/money/moneta/MonetaMoneyModule.java

+public final class MonetaMoneyModule extends Module {
+
+    private final JavaxMoneyModule baseModule;
+    private final MonetaryAmountFactory<? extends MonetaryAmount> amountFactory;


I think this amountFactory field is never read so it can be removed ?

After the above requested change, this will be needed to support the appropriate amount factory for MonetaryAmount.

This will be Money by default, or FastMoney or others if configured as such.

KeepItSimpleStupid · 2025-02-04T16:17:38Z

...oney-moneta/src/main/java/com/fasterxml/jackson/datatype/money/moneta/MonetaMoneyModule.java

+        //Use provided amountFactory to deserialize a MonetaryAmount
+        deserializers.addDeserializer(MonetaryAmount.class, new MonetaryAmountDeserializer<>(amountFactory, names));


Thanks for the modifications :)

If the default constructor becomes

public MonetaMoneyModule() { this(new JavaxMoneyModule().withMonetaryAmountFactory(Money::of), FieldNames.defaults(), Money::of, FastMoney::of, Money::of, RoundedMoney::of); }

and the method withMonetaryAmountFactory becomes

private <T extends MonetaryAmount> MonetaMoneyModule withMonetaryAmountFactory(final MonetaryAmountFactory<T> amountFactory) { return new MonetaMoneyModule(baseModule.withMonetaryAmountFactory(amountFactory), names, amountFactory, fastMoneyFactory, moneyFactory, roundedMoneyFactory); }

I think you can get rid of both :

this extra deserializer for MonetaryAmount and rely only on the one defined in the baseModule

the amountFactory field of this MonetaMoneyModule class

You are correct. Changed now.

I had previously assumed that the deserializing logic in MonetaModule will overwrite the Base Module
But this appears not to be the case.

Thanks !
You can get rid of the extra dezerializer for CurrencyUnit as well ;)
And I think it will all look good to me after that !

D'oh, right again. Done.

sri-adarsh-kumar commented Oct 18, 2024

View reviewed changes

javax-money/src/main/resources/META-INF/LICENSE Outdated Show resolved Hide resolved

sri-adarsh-kumar commented Oct 18, 2024

View reviewed changes

sri-adarsh-kumar force-pushed the 2.19 branch from 1166e8e to f29f0e3 Compare October 22, 2024 14:26

sri-adarsh-kumar force-pushed the 2.19 branch from f29f0e3 to abaf36d Compare October 22, 2024 14:40

sri-adarsh-kumar marked this pull request as ready for review October 22, 2024 17:00