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.

Sign up for GitHub

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

Jump to bottom

CurrencyConversionException using exchange rate providers #161

Closed
McPringle opened this issue Jul 5, 2017 · 20 comments
Closed

CurrencyConversionException using exchange rate providers #161

McPringle opened this issue Jul 5, 2017 · 20 comments
Assignees
@atsticks
Labels
bug
Milestone
1.2

Comments

@McPringle
Copy link

I tried to convert a currency using the following piece of code:

final MonetaryAmount inEuro = Money.of(10, "EUR");
final ExchangeRateProvider rateProvider = MonetaryConversions.getExchangeRateProvider("ECB");
final CurrencyConversion dollarConversion = rateProvider.getCurrencyConversion("USD");
final MonetaryAmount inDollar = inEuro.with(dollarConversion);
System.out.println(String.format("%s: %s ≙ %s", rateProvider, inEuro, inDollar));

The result was the following exception:

Exception in thread "main" CurrencyConversionException [base=EUR, term=null, conversionContext=null]: Cannot convert EUR into null
	at org.javamoney.moneta.spi.AbstractCurrencyConversion.apply(AbstractCurrencyConversion.java:109)
	at org.javamoney.moneta.Money.with(Money.java:377)
	at org.javamoney.moneta.Money.with(Money.java:58)
	at CurrencyConversionDemo.main(CurrencyConversionDemo.java:34)

From my point of view this code should work.
@atsticks
Copy link
Member

atsticks commented Jul 5, 2017 via email

@keilw keilw added the analysis label Jul 5, 2017
@McPringle
Copy link
Author

I forgot to mention my versions:

macOS Sierra 10.12.5

java version "1.8.0_131"
Java(TM) SE Runtime Environment (build 1.8.0_131-b11)
Java HotSpot(TM) 64-Bit Server VM (build 25.131-b11, mixed mode)

<dependencies>
    <dependency>
        <groupId>javax.money</groupId>
        <artifactId>money-api</artifactId>
        <version>1.0.1</version>
    </dependency>
    <dependency>
        <groupId>org.javamoney</groupId>
        <artifactId>moneta</artifactId>
        <version>1.1</version>
    </dependency>
</dependencies>

<build>
    <plugins>
        <plugin>
            <artifactId>maven-compiler-plugin</artifactId>
            <version>3.3</version>
            <configuration>
                <source>1.8</source>
                <target>1.8</target>
            </configuration>
        </plugin>
    </plugins>
</build>

@McPringle
Copy link
Author

Different code, same exception, different stack trace:

final CurrencyConversion dollarConversion = MonetaryConversions.getConversion("USD");
final MonetaryAmount inEuro = Money.of(10, "EUR");
final MonetaryAmount inDollar = inEuro.with(dollarConversion);
System.out.println(String.format("%s ≙ %s", inEuro, inDollar));

Very crazy: I run this program 10 times where 2 of them finished successful (output: EUR 10 ≙ USD 11.329) and 8 of them crashed with the following exception:

Exception in thread "main" CurrencyConversionException [base=EUR, term=USD, conversionContext=null]: Cannot convert EUR into USD: All delegate prov iders failed to deliver rate, providers=[org.javamoney.moneta.internal.convert.IdentityRateProvider@2b546384, org.javamoney.moneta.internal.convert.ECBCurrentRateProvider{ context: ProviderContext (
{rateTypes=[DEFERRED], providerDescription=European Central Bank, days=1, provider=ECB})}, org.javamoney.moneta.internal.convert.IMFRateProvider{ context: ProviderContext (
{rateTypes=[DEFERRED], providerDescription=International Monetary Fond, days=1, provider=IMF})}, org.javamoney.moneta.internal.convert.IMFHistoricRateProvider{ context: ProviderContext (
{rateTypes=[HISTORIC], providerDescription=Historic International Monetary Fond, days=0, provider=IMF-HIST})}, org.javamoney.moneta.internal.convert.ECBHistoricRateProvider{ context: ProviderContext (
{rateTypes=[DEFERRED, HISTORIC], providerDescription=European Central Bank, days=1500, provider=ECB-HIST})}, org.javamoney.moneta.internal.convert.ECBHistoric90RateProvider{ context: ProviderContext (
{rateTypes=[DEFERRED, HISTORIC], providerDescription=European Central Bank (last 90 days), days=90, provider=ECB-HIST90})}], query=ConversionQuery (
{Query.termCurrency=USD, Query.baseCurrency=EUR})
	at org.javamoney.moneta.spi.CompoundRateProvider.getExchangeRate(CompoundRateProvider.java:121)
	at org.javamoney.moneta.spi.LazyBoundCurrencyConversion.getExchangeRate(LazyBoundCurrencyConversion.java:57)
	at org.javamoney.moneta.spi.AbstractCurrencyConversion.apply(AbstractCurrencyConversion.java:106)
	at org.javamoney.moneta.Money.with(Money.java:377)
	at org.javamoney.moneta.Money.with(Money.java:58)
	at CurrencyConversionDemo.main(CurrencyConversionDemo.java::51)

@McPringle
Copy link
Author

I cloned this repo and run the tests. As you can see, there are some failures related to conversion:

Results :

Failed tests: 
  DefaultNumberValueTest.shouldReturnAmountFractionDenominator:132 expected:<21> but was:<100>
  DefaultNumberValueTest.shouldReturnAmountFractionNumerator:126 expected:<132> but was:<21>
  IMFHistoricRateProviderTest.shouldConvertsBrazilianToDollar:143 » CurrencyConversion
  IMFHistoricRateProviderTest.shouldConvertsDollarToBrazilian:156 » CurrencyConversion
  IMFHistoricRateProviderTest.shouldConvertsDollarToEuro:117 » CurrencyConversion
  IMFHistoricRateProviderTest.shouldConvertsEuroToDollar:130 » CurrencyConversion
  IMFHistoricRateProviderTest.shouldSetTimeInLocalDateTime:226 » CurrencyConversion
  IMFHistoricRateProviderTest.shouldSetTimeInLocalDateTime2:176 » CurrencyConversion
  IMFRateProviderTest.shouldConvertsBrazilianToDollar:132 » CurrencyConversion C...
  IMFRateProviderTest.shouldConvertsDollarToBrazilian:145 » CurrencyConversion C...
  IMFRateProviderTest.shouldConvertsDollarToEuro:106 » CurrencyConversion Cannot...
  IMFRateProviderTest.shouldConvertsEuroToDollar:119 » CurrencyConversion Cannot...
  MonetaryConversionTest.testGetExchangeRateDefault:45 » CurrencyConversion Cann...
  MonetaryConversionTest.testGetExchangeRateProvider_Chained:61 expected [true] but found [false]
  MonetaryConversionTest.testMonetaryConversionsIMF:84 » CurrencyConversion Cann...

Tests run: 85, Failures: 15, Errors: 0, Skipped: 0

[ERROR] There are test failures.

Please refer to /Users/mcpringle/GitHub/other/jsr354-ri/moneta-convert/moneta-convert-imf/target/surefire-reports for the individual test results.
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] Building Moneta (JSR 354 RI) 1.2-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO] 
[INFO] Moneta Parent ...................................... SUCCESS [  0.124 s]
[INFO] Moneta Core ........................................ SUCCESS [ 19.036 s]
[INFO] Moneta Currency Conversion (Parent) ................ SUCCESS [  0.001 s]
[INFO] Moneta Currency Conversion ......................... SUCCESS [  2.216 s]
[INFO] Moneta Currency Conversion - ECB Provider .......... SUCCESS [ 37.363 s]
[INFO] Moneta Currency Conversion - IMF Provider .......... SUCCESS [  9.014 s]
[INFO] Moneta (JSR 354 RI) ................................ SUCCESS [  0.000 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:09 min
[INFO] Finished at: 2017-07-06T04:05:31+02:00
[INFO] Final Memory: 24M/308M
[INFO] ------------------------------------------------------------------------

Complete build log: https://gist.github.com/McPringle/16bc55b579b76f3749c0e13a45cbffd7

Interesting side fact: There are some failures but the build was considered successful. Looks like the failsafe plugin fails the build only on errors, not on failures. Is this what you expect?

@atsticks
Copy link
Member

atsticks commented Jul 6, 2017 via email

@keilw
Copy link
Member

keilw commented Jul 6, 2017

Any chance to test the same against https://github.com/JavaMoney/jsr354-ri-bp ?

@McPringle
Copy link
Author

My test code

final MonetaryAmount inEuro = Money.of(10, "EUR");
final ExchangeRateProvider rateProvider = MonetaryConversions.getExchangeRateProvider("ECB");
final CurrencyConversion dollarConversion = rateProvider.getCurrencyConversion("USD");
final MonetaryAmount inDollar = inEuro.with(dollarConversion);
System.out.println(String.format("%s: %s ≙ %s", rateProvider, inEuro, inDollar));

I'll run this code ten times with each of three different versions of the implementation.

Test with Moneta backport v1.1

<dependencies>
    <dependency>
        <groupId>javax.money</groupId>
        <artifactId>money-api-bp</artifactId>
        <version>1.0.1</version>
    </dependency>
    <dependency>
        <groupId>org.javamoney</groupId>
        <artifactId>moneta-bp</artifactId>
        <version>1.1</version>
    </dependency>
</dependencies>

Mostly works (about 8 out of 10 runs):

org.javamoney.moneta.internal.convert.ECBCurrentRateProvider@1bc6a36e: EUR 10 ≙ USD 10.861

Sometimes crashes (2 out of 10 runs):

Exception in thread "main" CurrencyConversionException [base=EUR, term=USD, conversionContext=ConversionContext (
{provider=ECB, javax.money.convert.RateType=DEFERRED})]: Cannot convert EUR into USD
	at org.javamoney.moneta.spi.AbstractCurrencyConversion.apply(AbstractCurrencyConversion.java:103)
	at org.javamoney.moneta.Money.with(Money.java:387)
	at org.javamoney.moneta.Money.with(Money.java:68)
	at CurrencyConversionDemo.main(CurrencyConversionDemo.java:41)

Test with Moneta v1.1

<dependencies>
    <dependency>
        <groupId>javax.money</groupId>
        <artifactId>money-api</artifactId>
        <version>1.0.1</version>
    </dependency>
    <dependency>
        <groupId>org.javamoney</groupId>
        <artifactId>moneta</artifactId>
        <version>1.1</version>
    </dependency>
</dependencies>

Crashes on every run:

Exception in thread "main" CurrencyConversionException [base=EUR, term=null, conversionContext=null]: Cannot convert EUR into null
	at org.javamoney.moneta.spi.AbstractCurrencyConversion.apply(AbstractCurrencyConversion.java:109)
	at org.javamoney.moneta.Money.with(Money.java:377)
	at org.javamoney.moneta.Money.with(Money.java:58)
	at CurrencyConversionDemo.main(CurrencyConversionDemo.java:41)

Test with Moneta v1.0

<dependencies>
    <dependency>
        <groupId>javax.money</groupId>
        <artifactId>money-api</artifactId>
        <version>1.0.1</version>
    </dependency>
    <dependency>
        <groupId>org.javamoney</groupId>
        <artifactId>moneta</artifactId>
        <version>1.0</version>
    </dependency>
</dependencies>

Works on every run:

org.javamoney.moneta.internal.convert.ECBCurrentRateProvider@36d64342: EUR 10 ≙ USD 11.385

@keilw keilw added bug and removed analysis labels Jul 6, 2017
@keilw keilw added this to the 1.2 milestone Jul 6, 2017
@keilw
Copy link
Member

keilw commented Jul 6, 2017 •
edited
Loading

Thanks for the detailed analysis. Looks like Moneta 1.1 introduced an issue here. Must be solved before seriously considering the next release.

@McPringle
Copy link
Author

On July 13th I'll be in Zürich. If you can't reproduce or need help, maybe there is some time to meet @atsticks?

@keilw
Copy link
Member

keilw commented Jul 6, 2017

@McPringle @atsticks that would be great. Initially hoped 1.2 could be out by July 12, but at least this and possible other showstoppers must be fixed. It seems unrelated to modularization, because 1.1 was still a "monolith". Note, the Moneta 1.0 test with API 1.0.1 makes little sense, because you'd get API 1.0 as a transient dependency under the hood, but the API has not changed, so there's only a minor difference for Moneta-BP because the API was build against Java 7.

@atsticks
Copy link
Member

atsticks commented Jul 6, 2017 •
edited
Loading

OK, my assumption has been correct: the ECB provider (the others providers as well), does throw an error until the currencies have been successfully loaded:

CurrencyConversionException [base=EUR, term=USD, conversionContext=null]: Cannot convert EUR into USD
	at org.javamoney.moneta.spi.AbstractCurrencyConversion.apply(AbstractCurrencyConversion.java:108)
	at org.javamoney.moneta.Money.with(Money.java:377)
	at org.javamoney.moneta.Money.with(Money.java:58)
	at org.javamoney.moneta.convert.ProviderTest.testAccess(ProviderTest.java:24)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:497)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:45)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:42)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:263)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:68)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:47)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:231)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:60)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:229)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:50)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:222)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:300)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:157)
	at com.intellij.junit4.JUnit4IdeaTestRunner.startRunnerWithArgs(JUnit4IdeaTestRunner.java:68)
	at com.intellij.rt.execution.junit.IdeaTestRunner$Repeater.startRunnerWithArgs(IdeaTestRunner.java:51)
	at com.intellij.rt.execution.junit.JUnitStarter.prepareStreamsAndStart(JUnitStarter.java:242)
	at com.intellij.rt.execution.junit.JUnitStarter.main(JUnitStarter.java:70)
Jul 07, 2017 12:43:24 AM org.javamoney.moneta.internal.convert.ECBAbstractRateProvider newDataLoaded
INFORMATION: Loaded ECBCurrentRateProvider exchange rates for days:1
RUN: 
 - 70: org.javamoney.moneta.internal.convert.ECBCurrentRateProvider{ context: ProviderContext (
{rateTypes=[DEFERRED], providerDescription=European Central Bank, days=1, provider=ECB})} ≙ EUR 10

Code used:

    @Test
    public void testAccess() throws InterruptedException {
        final MonetaryAmount inEuro = Money.of(10, "EUR");
        for(int i=0; i<100;i++){
            try {
                final ExchangeRateProvider rateProvider = MonetaryConversions.getExchangeRateProvider("ECB");
                final CurrencyConversion dollarConversion = rateProvider.getCurrencyConversion("USD");
                final MonetaryAmount inDollar = inEuro.with(dollarConversion);
                System.out.println(String.format("RUN: %n - %s: %s ≙ %s", i, rateProvider, inEuro, inDollar));
            }catch(Exception e){
                e.printStackTrace();
            }
        }
        for(int i=0; i<100;i++){
            final ExchangeRateProvider rateProvider = MonetaryConversions.getExchangeRateProvider("ECB");
            final CurrencyConversion dollarConversion = rateProvider.getCurrencyConversion("USD");
            Thread.sleep(100L);
            final MonetaryAmount inDollar = inEuro.with(dollarConversion);
            System.out.println(String.format("RUN: %n - %s: %s ≙ %s", i, rateProvider, inEuro, inDollar));
        }

    }

Best solution to be implemented IMO would be:

  • Ensure loading is happening asynchronously.
  • Wait (block callers) a certain amount of time until, either defaults or remote data has been successfully loaded.
  • Alternatively we can throw an exception, which better describes what the conversion failed ;-)

WDYT?

@McPringle
Copy link
Author

My opinion: If you throw an exception if I try to use an exchange provider that is not completely initialized, as a user of Moneta I will have to check for this exception everywhere I need the excchange rates. My preferred usage of Moneta would be to do the loading asynchronously and if I try to access the exchange rates while the loading is still in progress, these calls will be blocked until the loading is finished or a timeout occurs (with a descriptive exception).

I make a difference between these two exceptions and how to handle them. The first one (rate provider not completely initialized) will happen quite often. The second one (timeout) will only occur if there are errors somewhere (e.g. broken internet access, network problems, rate exchange data server down etc). If the first exceptions occurs I will try again a few times or for a certain amount of time, if the second exception occurs I will do nothing and accept that it does not work for now: my business code will look cleaner and contain less technical code.

@keilw
Copy link
Member

keilw commented Jul 7, 2017

OK, so it's a concurrency or if you want "Transactional" issue here. Hope, a generic enough solution can be found, either directly in the RI or as a well-documented instruction for end users and applications. Any idea why the backports are not affected?

@atsticks
Copy link
Member

atsticks commented Jul 7, 2017 via email

@keilw keilw mentioned this issue Jul 7, 2017
Closed
@atsticks atsticks self-assigned this Jul 11, 2017
@McPringle
Copy link
Author

Any news about this issue? On August 3rd we'll have a Hackergarten in Lucerne starting at 6pm, maybe you (@atsticks) are able to attend? It would be a nice project for the evening but I think we are not able to dive deep enough into the topic without any help. Or it could be a reason to organise a Hackergarten in Zürich, too…

@keilw
Copy link
Member

keilw commented Jul 24, 2017

@McPringle @atsticks Yes, please do if you can. There are 4 showstoppers here (at least 1 also in the BP, but if e.g. a service call has race conditions etc. it is very likely also affected by this one) and until all of them are fixed no new fix pack is justified.

@atsticks
Copy link
Member

atsticks commented Jul 24, 2017 via email

@atsticks
Copy link
Member

Checked in today the fixes for jsr354-ri (Java 8 version). Please test and provide feedback.

@keilw
Copy link
Member

keilw commented Jul 29, 2017

Thanks, great work. How about a backport?

@atsticks
Copy link
Member

atsticks commented Jul 29, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@atsticks atsticks

Labels
bug
Projects
None yet
Milestone
1.2
Development

No branches or pull requests
3 participants
@keilw @McPringle @atsticks