JavaMoney / jsr354-ri

JSR 354 - Moneta: Reference Implementation
Other
344 stars 101 forks source link

Fix problems of MonetaryConfig #370

Open keilw opened 3 years ago

keilw commented 3 years ago

There are build failures related to MonetaryConfig right now:

SEVERE: Error loading javamoney.properties, ignoring file:/home/travis/build/JavaMoney/jsr354-ri/moneta-convert/moneta-convert-ecb/target/test-classes/javamoney.properties
java.lang.IllegalStateException: AmbiguousConfiguration detected for 'theWinner3'.

This seems to affect the ECB exchange module:

javax.money.MonetaryException: Failed to load currency conversion data: null
    at org.javamoney.moneta.convert.ecb/org.javamoney.moneta.convert.ecb.ECBAbstractRateProvider.getExchangeRate(ECBAbstractRateProvider.java:130)
    at org.javamoney.moneta.convert.ecb/org.javamoney.moneta.convert.ecb.ECBCurrentRateProvider.getExchangeRate(ECBCurrentRateProvider.java:33)
    at org.javamoney.moneta/org.javamoney.moneta.spi.LazyBoundCurrencyConversion.getExchangeRate(LazyBoundCurrencyConversion.java:57)
    at org.javamoney.moneta/org.javamoney.moneta.spi.AbstractCurrencyConversion.apply(AbstractCurrencyConversion.java:105)
    at org.javamoney.moneta/org.javamoney.moneta.Money.with(Money.java:382)
    at org.javamoney.moneta/org.javamoney.moneta.Money.with(Money.java:63)
    at org.javamoney.moneta.convert.ecb/org.javamoney.moneta.convert.ecb.ProviderTest.testAccess_ECB(ProviderTest.java:24)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.base/java.lang.reflect.Method.invoke(Method.java:564)
    at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:124)
    at org.testng.internal.Invoker.invokeMethod(Invoker.java:571)
    at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:707)
    at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:979)
    at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:125)
    at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:109)
    at org.testng.TestRunner.privateRun(TestRunner.java:648)
    at org.testng.TestRunner.run(TestRunner.java:505)
    at org.testng.SuiteRunner.runTest(SuiteRunner.java:455)
    at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:450)
    at org.testng.SuiteRunner.privateRun(SuiteRunner.java:415)
    at org.testng.SuiteRunner.run(SuiteRunner.java:364)
    at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
    at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:84)
    at org.testng.TestNG.runSuitesSequentially(TestNG.java:1187)
    at org.testng.TestNG.runSuitesLocally(TestNG.java:1116)
    at org.testng.TestNG.runSuites(TestNG.java:1028)
    at org.testng.TestNG.run(TestNG.java:996)
    at org.apache.maven.surefire.testng.TestNGExecutor.run(TestNGExecutor.java:135)
    at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeMulti(TestNGDirectoryTestSuite.java:193)
    at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.execute(TestNGDirectoryTestSuite.java:94)
    at org.apache.maven.surefire.testng.TestNGProvider.invoke(TestNGProvider.java:146)
    at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384)
    at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345)
    at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)
    at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)
keilw commented 1 year ago

While it's odd, the problem does not happen in every situation, e.g. in a Maven build it always does, but debugging the TestNG tests in an IDE cannot replicate it the same way.

A possible remedy could be to log a warning for such configuration case, but not throw an exception, but while the "AmbiguousConfiguration" seems fine, to scale down to a warning, the missing configuration for exchange rate providers like ECB or others must be fixed anyway.

kewne commented 1 year ago

Hi, I've looked into this and the cause seems to be this particular line; according to the ClassLoader.getResources javadoc:

this method will only find resources in packages of named modules when the package is opened unconditionally (even if the caller of this method is in the same module as the resource)

This causes all of the javamoney.properties files inside named modules (like the one in the org.javamoney.moneta.convert.ecb module) to be ignored, with the result that:

  1. the "default" configuration is never loaded;
  2. because there is no default configuration, the provider's loader is never triggered;
  3. any calls to getExchangeRate will fail when waiting for the load lock.

I think this could be solved by, in each provider, registering a default configuration (the current one in javamoney.properties) if the LoaderService did not find an existing one.

keilw commented 1 year ago

Thanks, especially in case of ECB the loading fails for different reasons, at least for historic ones, but all providers (also current or HIST-90) failing tests might be addressed that way.

Unfortunately you introduced a bug, will have to comment the relevant parts out, because at least for 1.4.3 it is unlikely we'll also add Multi-Release-JARs yet and the minimal version (might raise it to 11 or 17 in 1.5) is still Java 8. That Map.of(array) did not exist until Java 9, that's why it fails because the classes represent the lowest JDK which is 8.

keilw commented 10 months ago

As this only seems to affect 4 JUnit tests, it can be done later.