Currency code parsing broken for everything but € and £

[marschall]

I recently stumbled over the SYMBOL branch in org.javamoney.moneta.internal.format.CurrencyToken.parse(ParseContext).

I believe this is broken for everything but € and £. It uses javax.money.Monetary.getCurrency(String, String...) as a fallback but passes a currency symbol as an ISO currency code argument, which will never work. Also there are a lot more currency symbols besides $ that are ambiguous, see https://en.wikipedia.org/wiki/Currency_symbol.

To illustrate consider the following test for Indian rupees which fails:

    /**
     * Test related to parsing currency symbols.
     */
    @Test
    public void testParseCurrencySymbol() {
        MonetaryAmountFormat format = MonetaryFormats.getAmountFormat(
                    AmountFormatQueryBuilder.of(Locale.GERMANY)
                        .set(CurrencyStyle.SYMBOL)
                        .build());
        Money money = Money.of(new BigDecimal("1234567.89"), "EUR");
        String expectedFormattedString = "1.234.567,89 €";
        assertEquals(expectedFormattedString, format.format(money));
        assertEquals(money, Money.parse(expectedFormattedString, format));

        money = Money.of(new BigDecimal("1234567.89"), "INR");
        expectedFormattedString = "1.234.567,89 ₹";
        assertEquals(expectedFormattedString, format.format(money));
        assertEquals(money, Money.parse(expectedFormattedString, format));
    }

[stokito]

Yes, this also should be implemented. Even more: we can easily solve ambiguous by locale. For example if locale was set to HK (Honk Kong) then the $ sign means HKD (Honk Kong Dollar)

[marschall]

Even more: we can easily solve ambiguous by locale. For example if locale was set to HK (Honk Kong) then the $ sign means HKD (Honk Kong Dollar)

I don't think we can make that assumption. AFAIK the locale just affects the parsing and formatting not the actual currency. If you have a multi currency application in Hong Kong it may display any currency formatted according to local display rules not just Honk Kong Dollar.

[keilw]

It doesn't affect the language either, take Switzerland or many other countries with multiple languages even official ones. There is however an official currency, so similar to the BG example we can make those assumptions where necessary.

[keilw]

Thanks to IntelliJ (Eclipse recently is no longer compatible with TestNG 👎 ) Debugging I was able to pinpoint a major part of the problem. For every currency symbol but $, € or £ the Monetary.getCurrency() method applied throws an exception: UnknownCurrencyException [currencyCode=₹] It tries to accidentally use the SYMBOL as CODE or the symbol really doesn't work in the underlying system, could be something with the Locale.

[keilw]

It can be fixed in Moneta, but the BP has a totally different behavior in Unit tests, so I don't see the need to fix this very special corner case any more. For a new version we should think about improving CurrencyQuery which does not accept a SYMBOL right now, nor does CurrencyToken support a numeric code right now either.

[keilw]

@stokito Actually HKD won't work, the CurrencyToken has a special exception case for "$" but Rupees or Yen and a few other currencies with unique symbols in the JDK will work now. @marschall Please both have a look, if you could add some more tests to MonetaryFormatsParseBySymbolTest it would be appreciated.

[marschall]

@keilw I had a quick look, here are my first impressions

• we're now using java.util.Currency to convert from symbol to currency code, this does not feel right to me because

  • it is very limited, it only supports ₹, $, ₩, ₪, £, ¥, ₫, €

    • java.util.Currency#getSymbol() returns the currency code if the symbol is not available. This means that when we set the currency style to symbol and try to parse a string that contains a currency code, but the JDK does not know the symbol, the parsing succeeds anyway. I believe the parsing should fail in this case. Consider this test

      
      /**

    • Test related to parsing currency symbols. */ @Test public void testParseCurrencySymbolNOKFailed() { MonetaryAmountFormat format = MonetaryFormats.getAmountFormat( AmountFormatQueryBuilder.of(Locale.GERMANY) .set(CurrencyStyle.SYMBOL) .build());

    String formattedString = "1.234.567,89 NOK"; assertThrows(MonetaryParseException.class, () -> Money.parse(formattedString, format)); }

  • I believe the correct way would be to have the symbol as an additional property in the javax.money.MonetaryContext of org.javamoney.moneta.internal.JDKCurrencyAdapter because
  • this would allow other javax.money.spi.CurrencyProviderSpi to contribute their own symbols
  • this would also allow us to run a javax.money.CurrencyQuery to search for currencies with this symbol. Ultimately this is what javax.money.Monetary#getCurrency does as well. Ideally this would allow us to remove the special casing for $ as well, as in this case a query would return multiple currency codes.
  • as org.javamoney.moneta.format.CurrencyStyle is part of the Moneta API and not part of the JSR-354 API this would not be a change to the JSR-354 API.
• I think we can now remove the special casing for € and £
• CurrencyToken has a lot of unused imports (minor)
• I would remove the commented our System.out.println (minor)

should I open a review?

[keilw]

@marschall I'm afraid for the MR1 this may have to do, because the source of all other currency symbols is totally unclear. We may have to tap into the Unicode CLDR beyond what Java SE makes use of it. If you would like to propose a PR for the last 3 bullet points, please go ahead.

[marschall]

@keilw I don't think this needs to be solved in MR1 of JSR-354. org.javamoney.moneta.format#CurrencyStyle is a Moneta extension, not part of JSR-354. Therefore this can be solved in Moneta without affecting MR1. So Moneta can just define under what property in the MonetaryContext it expects the currency symobl. MonetaryContext is intended for provider specific extensions like the symbol. What about things like "1.234.567,89 NOK" parsing with CurrencyStyle.SYMBOL? Should I create a bug for this?

[keilw]

Feel free, but please note, we must not squeeze all of that into the MR1. It should be out ASAP mostly to fix the long overdue license issue we were left to solve only after the Maintenance Lead Transfer. If you feel something can be done in a 1.5 or 1.6 update to Moneta after the MR, sure, why not, but there should be no changes in the API, especially something like CurrencyQuery.getCurrencySymbols() which does not exist at this point.