JavaMoney / jsr354-ri

JSR 354 - Moneta: Reference Implementation
Other
334 stars 100 forks source link

MonetaryConversions.getConversion loosing rate date for exchange rate providers #374

Closed l-ray closed 1 year ago

l-ray commented 2 years ago

Following the example to retrieve conversion rates for a given historic point in time

Money
    .of(BigDecimal.TEN, "EUR")
    .with(
      MonetaryConversions.getConversion(
          ConversionQueryBuilder.of()
        .setTermCurrency("USD")
        .set(LocalDate.of(2015, 1, 5))
        .build()
      )
)

the LocalDate information is lost within calling MonetaryConversions.getConversion (calling MonetaryConversions#getMonetaryConversionsSpi().getConversion(conversionQuery) -> calling MonetaryConversionsSingletonSpi#getConversion(ConversionQuery conversionQuery))

The code fragment in question is

    default CurrencyConversion getConversion(ConversionQuery conversionQuery) {
        return this.getExchangeRateProvider(conversionQuery).getCurrencyConversion((CurrencyUnit)Objects.requireNonNull(conversionQuery.getCurrency(), "Terminating Currency is required."));
    }

With a custom exchange rate provider, the issue manifests in

public class CustomExchangeRateProvider extends AbstractRateProvider {
   ...
       @Override
    public ExchangeRate getExchangeRate(ConversionQuery conversionQuery) {
        CurrencyUnit baseCurrency = conversionQuery.getBaseCurrency();
        CurrencyUnit currency = conversionQuery.getCurrency();
        LocalDate aDate = conversionQuery.get(LocalDate.class); // <- NullPointerException
        return ...;
    }
   ...
}

With the OTB exchange rate providers, it seems to work as those use the current LocalDate as fallback. I wrote the following test, that fails accordingly (minimal reproduceable example at https://github.com/l-ray/javamoney-poc/)

       @Test
    void selectsFromECBWithGivenDate() {
        MonetaryAmount inEUR = Money.of(BigDecimal.TEN, "EUR");

        CurrencyConversion conv2 = MonetaryConversions.getConversion(ConversionQueryBuilder.of()
                .setProviderName("ECB-HIST")
                .setTermCurrency("USD")
                .set(LocalDate.now())
                .build());

        CurrencyConversion conv1 = MonetaryConversions.getConversion(
                ConversionQueryBuilder.of()
                        .setProviderName("ECB-HIST")
                        .setTermCurrency("USD")
                        .set(LocalDate.of(2008, 1, 1))
                        .build()
        );

        assertEquals(inEUR.with(conv1), inEUR.with(conv1));
        assertEquals(inEUR.with(conv2), inEUR.with(conv2));
        assertNotEquals(inEUR.with(conv1), inEUR.with(conv2)); // <- failing step
    }

Hope, this is helpful.

keilw commented 2 years ago

Does that example contain relevant parts of the custom provider? Either way, with all those classes like Money, the ticket clearly does not belong to the API, will try to relocate it, if we can.

l-ray commented 2 years ago

Thanks for the super fast response.

The example basically holds the unit test from above as maven project and github actions - to easily see the failing test. I opened the issue here as IMO the source of the issue might be javax.money.spi.MonetaryConversionsSingletonSpi

This said, feel free .... whatever is the best location in your opinion. Again, thanks for the fast pick-up.

keilw commented 1 year ago

Sorry it took a bit before we could look into this, but I analyze this problem as part of anticipating a 1.4.3 release in the near future.

Since there are separate problems accessing the configuration in unit tests, I created an ECBExample for all 3 variants with "ECB" as the default. It works for "ECB" and "ECB-HIST90", but so far fails for "ECB-HIST", similar to the test case you mentioned above.

There could be an issue with the XML file eurofxref-hist.xml retrieved from ECB, but the error is rather fuzzy. I don't think the date is really lost, because it works for eurofxref-hist-90d.xml and results in different exchange rates for different dates, as long as it isn't a holiday, but the same day fails for ECBHistoric, although that date is included in the XML.

keilw commented 1 year ago

This seems like an encoding issue or problem with illegal characters at least in ECB-HIST:

Have to check that further, also analyzing exchange which is a Spring Boot API backed by ECB rates.

keilw commented 1 year ago

I created a fork of exchange here. And it works rather convincing althouth the original repo hasn't been touched in over 6 years. Even adding the full ECB-HIST URL works like a charm in addition to the ECB current and ECB-HIST90 ones. Allowing to use exchange rates in the original timeframe like April 2017. There seems no major delay despite that approach using DOM instead of SAX, but obviously it looks less error-prone for ECB data.

Although I haven't tried it for IMF and problems like #353, using a REST Client (in this case the Spring RestTemplate, something that should change into any of the client libraries mentioned in #353) instead of the old JDK 1.0 URLConnection should help both issues.

keilw commented 1 year ago

This is still a major blocker as the SAX Parser consistently fails for ECB-HIST with:

Exception in thread "main" javax.money.MonetaryException: Failed to load currency conversion data: Last Error during data load: Invalid byte 1 of 1-byte UTF-8 sequence.
    at org.javamoney.moneta.convert.ecb@1.4.3-SNAPSHOT/org.javamoney.moneta.convert.ecb.ECBAbstractRateProvider.getExchangeRate(ECBAbstractRateProvider.java:154)

It has nothing to do with modules and only occurs for the historic rates, but in theory if ECB does something to those files, it could also happen elsewhere.

keilw commented 1 year ago

There has been a major breakthrough. While the cache in moneta-core still uses the error-prone InputStream, which for ECB-HIST becomes a binary chunk instead of an XML file, the retrieval of the exchange rates as such already works by using new InputSource(new URL(<rateURl>).openStream())instead of an InputSource based on the InputSteeam.