Provider incorrectly marked as available + doesn't fallback to next provider in the chain

[basarito]

Hello, I'm not sure if I've misunderstood how this works, but I'm trying to get exchange rate on a given date (which can be current or historic) and I'm following the suggestion from the docs of ECBHistoricRateProvider / ECBHistoric90RateProvider:

To uses exchange rate from a specific date, you can use this way: CurrencyUnit termCurrency = ...; LocalDate localDate = ...; ConversionQuery conversionQuery = ConversionQueryBuilder.of().setTermCurrency(euro).set(localDate).build(); CurrencyConversion currencyConversion = provider.getCurrencyConversion(conversionQuery); MonetaryAmount money = ...; MonetaryAmount result = currencyConversion.apply(money)

The default chain is defined like: conversion.default-chain=IDENT,ECB,ECB-HIST90,ECB-HIST

I would assume that this means that provider.getCurrencyConversion(conversionQuery) would attempt each provider in the chain and move on if no CurrencyConversion is available.

However, looking a bit into the source code, I see that the localDate that gets set on the query isn't even inspected when searching for available providers. Also isAvailable method (javax.money.convert.ExchangeRateProvider#isAvailable(javax.money.convert.ConversionQuery)) doesn't really do what is expected, and as a result this error gets thrown:

CurrencyConversionException [base=USD, term=EUR, conversionContext=null]: Cannot convert USD into EUR: Rate Provider did not return data though at check before data was flagged as available, provider=ECB, query=ConversionQuery (
{Query.termCurrency=EUR, Query.baseCurrency=USD, Query.rateTypes=[DEFERRED, HISTORIC], java.time.LocalDate=2022-01-03})

for this query:

ConversionQueryBuilder.of()
            .setBaseCurrency(base)
            .setTermCurrency(term)
            .setRateTypes(RateType.DEFERRED, RateType.HISTORIC)
            .set(onDate);

Basically ECB is the first one to try conversion and fails instantly because, as expected, it doesn't have data for historic date 2022-01-03.

My questions are:

1. Why was ECB provider flagged as available in the first place?
2. How come it doesn't fallback to next provider in line ECB-HIST90 and attempt conversion there?

Version: 1.4.2

[keilw]

Well in my impression isAvailable() checks if a certain provider is part of the conversion.default-chain so in this example it would be false for IMF, but true for the mentioned ECB providers.

[keilw]

Yes, that's the case, ExchangeRateProvider has a default method isAvailable()

 Checks if an {@link ExchangeRate} between two {@link CurrencyUnit} is
 available from this provider. This method should check, if a given rate
  is <i>currently</i> defined.

It is unrelated to any date or time. "defined" means as I hinted above that it is declared in the configuration.

Will close this, as it works as designed. Feel free to create an enhancement request to try use the ProviderContext and analyze it for certain aspects like the days, but I don't think it is realistic before a new JSR version (.Next) so try to set the milestone accordingly if you can.

[basarito]

Ok, but in this case for org.javamoney.moneta.spi.CompoundRateProvider#getExchangeRate, if isAvailable just checks the presence in the chain,getExchangeRate is bound to fail for some providers. Why does it re-throw CurrencyConversionException and not give the next provider in the chain a chance to return an ExchangeRate?

The final exception message also doesn't make sense because All delegate providers failed to deliver rate isn't quite true, 'cause not all providers tried to deliver rate, if another one failed before their turn.

[keilw]

Maybe @fprochazka remembers why he added that "fail early" exception instead of looping to another provider. I'm afraid, Anatole, the main author of the class is no longer available since he got lost in a QAnon rabbit hole some while ago...

Let's see, what @fprochazka might say and if he thinks there could be a different solution, but either way this would have to be an improvement for Moneta 1.5 or even a 2.x release. If he agrees it could skip the exception or somehow "cache" it (because throwing it there preserves the information, but maybe one could find some special treatment if the chain is not empty yet) feel free to reopen it then, but for now let's wait for his input.

[fprochazka]

I wanted the entire operation (the business operation that is calling the exchange rates) to fail if one of the delegate providers threw an error because I've implemented a bunch of completely custom providers that sometimes could throw an exception if they failed for some critical reason (I've had e.g. database provider and if there were the table missing, it would rightfully throw a pretty critical exception)

The problem is that the CompoundRateProvider catches every exception. If you want to allow controlling the code flow using exceptions, there should be a clear distinction between exceptions that should kill the operation/app (missing table, something other that is completely broken and should sound all the alerts in a live system) and exceptions that can be silenced (rates for a given day are missing, but loading and processing them worked correctly)

[basarito]

@fprochazka but in that case isAvailable needs to do a different kind of check or have getExchangeRate always return null for non-critical failures.. the way it is at the moment, there's no way to chain providers and attempt several ones until you get one to return a result, and I think that kind of beats the purpose of a provider chain and CompoundRateProvider in general

[fprochazka]

I don't think it beats the purpose, the problem is that the CompoundRateProvider is expected to do different things when it should do fewer things. This entire mechanism should be reworked and configured by code, not a single stringy property.

The problem starts with the fact, that the library as a whole doesn't have a very good definition of how to work with error states.