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

Fix problems of MonetaryConfig #370

Open
keilw opened this issue Sep 8, 2021 · 4 comments
Open

Fix problems of MonetaryConfig #370

keilw opened this issue Sep 8, 2021 · 4 comments

Comments

@keilw
Copy link
Member

keilw commented Sep 8, 2021

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

keilw commented Jan 18, 2023

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

kewne commented Feb 25, 2023

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

keilw commented Feb 26, 2023

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

keilw commented Jan 23, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

No branches or pull requests

2 participants