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

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

Closed
l-ray opened this issue Jan 18, 2022 · 7 comments · Fixed by #390
Closed

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

l-ray opened this issue Jan 18, 2022 · 7 comments · Fixed by #390

Comments

@l-ray
Copy link

l-ray commented Jan 18, 2022

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 l-ray changed the title Jan 18, 2022
@keilw
Copy link
Member

keilw commented Jan 18, 2022

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 keilw transferred this issue from JavaMoney/jsr354-api Jan 18, 2022
@l-ray
Copy link
Author

l-ray commented Jan 18, 2022

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
Copy link
Member

keilw commented Feb 23, 2023

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 keilw self-assigned this Feb 23, 2023
@keilw keilw moved this to In Progress in Backlog Feb 23, 2023
@keilw
Copy link
Member

keilw commented Feb 24, 2023

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
Copy link
Member

keilw commented Feb 24, 2023

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
Copy link
Member

keilw commented Apr 23, 2023

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 added a commit that referenced this issue Apr 24, 2023
@keilw keilw linked a pull request Apr 24, 2023 that will close this issue
@keilw
Copy link
Member

keilw commented Apr 24, 2023

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 added a commit that referenced this issue Apr 24, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Backlog Apr 24, 2023
keilw added a commit that referenced this issue Apr 24, 2023
keilw added a commit that referenced this issue Apr 25, 2023
@keilw keilw added the Exchange label Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants