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

Money.toString() and Money.parse(…) not symmetric anymore #357

Closed
odrotbohm opened this issue May 8, 2021 · 10 comments
Closed

Money.toString() and Money.parse(…) not symmetric anymore #357

odrotbohm opened this issue May 8, 2021 · 10 comments
Assignees
Labels
formatting Something about formatting and parsing Priority: High
Milestone

Comments

@odrotbohm
Copy link

odrotbohm commented May 8, 2021

Similarly to #319, the change made for #307 breaks print/parse symmetry without any kind of formatting involved. Assume you want to serialize (generally, not in a Java serialize way) a Money via toString() to eventually recreate it via Money.parse(…). Up until version 1.4, the following worked:

MonetaryAmount source = Money.of(1.23456, "EUR");
assertThat(Money.parse(source.toString)).isEqualTo(source);

This now fails with "Expected is <EUR 1.23> but was <EUR 1.23>". The reason for this is that as of 1.4 (#307) toString() now applies some default formatting, which I think is not only wrong, it's also highly misleading as you can see from the assertion message, as the source value is not 1.23, but 1.2345.

odrotbohm added a commit to st-tu-dresden/salespoint that referenced this issue May 8, 2021
Tweaked the implementation of MonetaryAmountAttributeConverter to workaround JavaMoney/jsr354-ri#357, as in 1.4 the toString() method returns a formatted value. We now extract currency code and numeric value explicitly ourselves.
@keilw
Copy link
Member

keilw commented May 8, 2021

Can you explain why you mentioned MonetaryAmount.toString()?
It is an interface and does not declare a toString() method, neither abstract nor default.
So I guess it's both of these methods in the Money class?

@odrotbohm
Copy link
Author

In the production code, I work with a MonetaryAmount but effectively it is a Money instance. I've updated the summary and description accordingly.

@odrotbohm odrotbohm changed the title MonetaryAmount.toString() and Money.parse(…) not symmetric anymore Money.toString() and Money.parse(…) not symmetric anymore May 10, 2021
@keilw
Copy link
Member

keilw commented May 14, 2021

Also here please be patient until https://issues.sonatype.org/browse/OSSRH-68317 can be resolved by Sonatype because for some totally unknown and strange reason the same Sonatype user which never had problems to sync from Bintray while that was still around suddenly fails to deploy snapshots directly. Hope they can fix that soon, because no other "technical" or personal account I use for other projects and JSRs ever failed, only the "JavaMoney" one.

svenkarol pushed a commit to svenkarol/salespoint that referenced this issue May 26, 2021
Tweaked the implementation of MonetaryAmountAttributeConverter to workaround JavaMoney/jsr354-ri#357, as in 1.4 the toString() method returns a formatted value. We now extract currency code and numeric value explicitly ourselves.
martinmo pushed a commit to st-tu-dresden/salespoint that referenced this issue Jun 1, 2021
Tweaked the implementation of MonetaryAmountAttributeConverter to workaround JavaMoney/jsr354-ri#357, as in 1.4 the toString() method returns a formatted value. We now extract currency code and numeric value explicitly ourselves.

[This is a cherry-pick of e99e67b in main, see issue #345.]
martinmo pushed a commit to st-tu-dresden/salespoint that referenced this issue Jun 1, 2021
Tweaked the implementation of MonetaryAmountAttributeConverter to workaround JavaMoney/jsr354-ri#357, as in 1.4 the toString() method returns a formatted value. We now extract currency code and numeric value explicitly ourselves.

[This is a cherry-pick of e99e67b in main, see issue #345.]
@keilw keilw added this to the 1.4.3 milestone Sep 8, 2021
@keilw keilw added the formatting Something about formatting and parsing label Jan 28, 2023
@keilw keilw added the analysis label Feb 12, 2023
@keilw
Copy link
Member

keilw commented Feb 12, 2023

I did reopen that, because I found what caused it, starting with at least 1.4.0 or even before, Anatole had hardcoded the scale of
ToStringMonetaryAmountFormat to 2 digits, hence toString() produces always a string with just 2 decimal digits as part of #307 among other changes.

It would be more or less a one-liner to use the scale of the BigDecimal, but that breaks a vast number of tests other than the 4 remaining failures in moneta-core (all of which seem a result of the modular ServiceLoader problem that only seems to affect those TestNG tests) and it would be a backward-incompatible change that may break production code.

Therefore I recommend making this customizable, but leaving the default number as 2 digits, a bit similar to overriding the default format as such via org.javamoney.moneta.useJDKdefaultFormat (there AFAIK no such scale limit exists).
Offering a similar property like "org.javamoney.moneta.ToStringMonetaryAmountFormat.scale" or similar, which allows to override it and has some "smart" value (like -1) that takes the scale of the BigDecimal number.

@odrotbohm, @otaviojava what do you think?
From a Maintenance Lead point I would say this is the most sensible change, and even if we never get to a 2.x release / whole new JSR, we might propose a MR2 or the already planned 1.5. It is possible, some issues planned only there like #277 could be related to this formatting cut-off.

And then we must document such backward-incompatible changes, giving users enough notice to migrate their code.

If that works here, and we can confirm, that #370 only affects a few tests but does not mess the runtime behavior (because the normal ConfigProvider etc. are all declared in a module-info, too) then all there is to do before we can release 1.4.3 is a solution for #353. Again destroying this feature is not acceptable, and I am confident, because the shell script works with curl instead of wget, that this can be solved, as usual help is appreciated if you like to get it out faster ;-)

@keilw keilw closed this as completed Feb 12, 2023
@keilw keilw removed the analysis label Feb 12, 2023
@keilw keilw reopened this Feb 13, 2023
@keilw keilw added this to Backlog Feb 14, 2023
@keilw keilw moved this to In Progress in Backlog Feb 14, 2023
@keilw
Copy link
Member

keilw commented Feb 14, 2023

There are two parts to this problem:

  1. Using MonetaryAmount amount = Monetary.getDefaultRounding().apply(this); in all relevant toString() operations, not just Money, which in the given example already rounds down the amount to 1.23. The only one or two unit tests taking the rounding down away and formatting this instead those for currency code "XXX", e.g. testToString(). I am not that deeply familiar with currency exchange as to if "XXX", see X currencies (funds, precious metals, supranationals, other) required a special rounding, but as that's commonly a wildcard for "no currency" I guess it also depends on the particular transaction, hence I don't see why toString() should automatically round down. At most, if anybody knows a strong usecase, it could again be configured, but the default should be false here IMO.
  2. applying a flexible/customizable scale in the ToStringMonetaryAmountFormat.

@keilw
Copy link
Member

keilw commented Feb 14, 2023

Actually the majority of test failures were pretty much the same, so avoiding the round-down plus an adjustment of the scale that also takes the maxScale and fixedScale `combination into consideration works pretty well.

Each of the three test cases now have a testParseFromToString() method.
By applying MonetaryContext built via MonetaryContextBuilder.of().setMaxScale(2).setFixedScale(true).build() it is possible to enforce the fixed two-digits in toString(), I guess that should do.

@odrotbohm, @otaviojava please review.

keilw added a commit that referenced this issue Feb 14, 2023
@keilw keilw moved this from In Progress to In Review in Backlog Feb 14, 2023
keilw added a commit that referenced this issue Feb 15, 2023
keilw added a commit that referenced this issue Feb 21, 2023
keilw added a commit that referenced this issue Feb 21, 2023
@keilw keilw self-assigned this Feb 23, 2023
@keilw
Copy link
Member

keilw commented Apr 23, 2023

Did not hear anything by either one, so I assume that is resolved.

@keilw keilw closed this as completed Apr 23, 2023
@github-project-automation github-project-automation bot moved this from In Review to Done in Backlog Apr 23, 2023
@stefanzilske
Copy link

Hey, I just noticed, after upgrading from 1.4.2, that the previously applied default rounding to a fixed scale=2 in toString() is not applied anymore. We heavily rely on this, e.g. whenever we serialize a Money value to JSON. So it's a quite invasive breaking change.

Is there a way to apply this default rounding? I stumbled upon MonetaryConfig, but found no usable documentation on how to best do this in SpringBoot.

@keilw
Copy link
Member

keilw commented Feb 5, 2024

The hardcoded scale for toString() was what had to be fixed here, so adding it back would only be possible as an optional property, Please create an enhancement request here, and we'll see, if we could add something into javamoney.properties to customize it.
In the meantime you should be able to enforce the scale of your choice by creating your Money like this:

Money money3 = Money.of(1234567.3, "EUR", MonetaryContextBuilder.of().setMaxScale(2).setFixedScale(true).build());
System.out.println(money3.toString());

prints "EUR 1234567.30".

@keilw
Copy link
Member

keilw commented Feb 20, 2024

@stefanzilske I saw #364 had been around for some time, we did not really plan to do this (especially the desire to just print "a" or something) until a much later version, but it seems fair to "hijack" it for your particular requirement and customize both
toString() and parse(), e.g. by restricting the scale.

I linked them this way, but it would be nice to mention a few words about the requirement in #364 if you can.

Btw, there are currently no plans to configure Moneta via the SpringBoot application.properties. Although the Spring Framework (and thus also Spring Boot) support formatting via JSR 354, the JSR is runtime-agnostic and works with all other Microframeworks just as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
formatting Something about formatting and parsing Priority: High
Projects
Status: Done
Development

No branches or pull requests

3 participants