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.

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

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

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


Usually should extend either StdDeserializer or (if applicable) StdScalarDeserializer.

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 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