jsr354-ri icon indicating copy to clipboard operation
jsr354-ri copied to clipboard

MonetaryConversions.getConversion loosing rate date for exchange rate providers

Open l-ray opened this issue 3 years ago • 2 comments

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.

l-ray avatar Jan 18 '22 12:01 l-ray

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.

keilw avatar Jan 18 '22 13:01 keilw

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.

l-ray avatar Jan 18 '22 13:01 l-ray

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 avatar Feb 23 '23 19:02 keilw

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

  • https://github.com/opencart/opencart/issues/11453

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

keilw avatar Feb 24 '23 16:02 keilw

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 avatar Feb 24 '23 19:02 keilw

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 [email protected]/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 avatar Apr 23 '23 17:04 keilw

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.

keilw avatar Apr 24 '23 18:04 keilw