-
Notifications
You must be signed in to change notification settings - Fork 76
IDGEN-134: Update to OpenMRS Platform 2.7.x and Support Java 21 #130
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
base: master
Are you sure you want to change the base?
Conversation
|
@wikumChamith for the errors that you are getting in this pull request, in the class |
ffcefae to
5174684
Compare
5174684 to
208d215
Compare
|
@dkayiwa thank you. Now this PR is ready to get reviewed. |
|
Aren't we also going to bump the version and create a branch for old versions? |
pom.xml
Outdated
|
|
||
| <properties> | ||
| <openMRSVersion>1.10.2</openMRSVersion> | ||
| <openMRSVersion>2.7.4</openMRSVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why you did not use 2.7.6?
| @Test | ||
| public void importIdentifiers_shouldAcceptJson() throws Exception { | ||
| Mockito.doNothing().when(iss).addIdentifiersToPool(Mockito.any(IdentifierPool.class), (List<String>) Mockito.anyCollectionOf(String.class)); | ||
| Mockito.doNothing().when(iss).addIdentifiersToPool(Mockito.any(IdentifierPool.class), (List<String>) Mockito.any()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why you did not use anyCollection?
| </bean> | ||
|
|
||
|
|
||
| <bean id="LocationBasedSuffixProvider" class="org.openmrs.module.idgen.suffixprovider.LocationBasedSuffixProvider" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these beans for?
| sleep(100); | ||
| Thread thread = new Thread(() -> { | ||
| Context.openSession(); | ||
| Context.authenticate("admin", "test"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why we needed to make all the changes here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since multiple threads are writing to the same ArrayList at the same time, it’s not thread-safe. That’s why the test started failing randomly after the update. I fixed it by wrapping the add logic in a synchronized block.
Not totally sure why this didn’t happen before, though.
|
@dkayiwa I updated the PR. I am not seeing the current build error locally. |
| * @param processor | ||
| * @return | ||
| */ | ||
| @Transactional(propagation = Propagation.REQUIRES_NEW) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mseaton I'm getting the following error when running this line:
[ERROR] org.openmrs.module.idgen.integration.RemoteWithLocalPoolIntegrationTest.testConfigurePoolFilledFromRemoteSource -- Time elapsed: 0.127 s <<< ERROR!
java.lang.NullPointerException
at org.openmrs.module.idgen.service.BaseIdentifierSourceService.getProcessor(BaseIdentifierSourceService.java:152)
at org.openmrs.module.idgen.service.BaseIdentifierSourceService.generateIdentifiersInternal(BaseIdentifierSourceService.java:195)
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:566)
at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:344)
at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:198)
at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:163)
at org.springframework.transaction.interceptor.TransactionInterceptor$1.proceedWithInvocation(TransactionInterceptor.java:123)
at org.springframework.transaction.interceptor.TransactionAspectSupport.invokeWithinTransaction(TransactionAspectSupport.java:388)
at org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:119)
at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186)
at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:241)
at com.sun.proxy.$Proxy254.generateIdentifiersInternal(Unknown Source)
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:566)
at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:344)
at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:198)
at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:163)
at org.springframework.transaction.interceptor.TransactionInterceptor$1.proceedWithInvocation(TransactionInterceptor.java:123)
at org.springframework.transaction.interceptor.TransactionAspectSupport.invokeWithinTransaction(TransactionAspectSupport.java:388)
at org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:119)
at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186)
at org.springframework.aop.framework.adapter.MethodBeforeAdviceInterceptor.invoke(MethodBeforeAdviceInterceptor.java:58)
at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186)
at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:241)
at com.sun.proxy.$Proxy255.generateIdentifiersInternal(Unknown Source)
at org.openmrs.module.idgen.service.BaseIdentifierSourceService.generateIdentifiers(BaseIdentifierSourceService.java:177)
at org.openmrs.module.idgen.service.BaseIdentifierSourceService.generateIdentifier(BaseIdentifierSourceService.java:252)
at org.openmrs.module.idgen.service.BaseIdentifierSourceService.generateIdentifier(BaseIdentifierSourceService.java:227)
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:566)
at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:344)
at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:198)
at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:163)
at org.springframework.transaction.interceptor.TransactionInterceptor$1.proceedWithInvocation(TransactionInterceptor.java:123)
at org.springframework.transaction.interceptor.TransactionAspectSupport.invokeWithinTransaction(TransactionAspectSupport.java:388)
at org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:119)
at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186)
at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:241)
at com.sun.proxy.$Proxy254.generateIdentifier(Unknown Source)
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:566)
at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:344)
at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:198)
at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:163)
at org.springframework.transaction.interceptor.TransactionInterceptor$1.proceedWithInvocation(TransactionInterceptor.java:123)
at org.springframework.transaction.interceptor.TransactionAspectSupport.invokeWithinTransaction(TransactionAspectSupport.java:388)
at org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:119)
at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186)
at org.springframework.aop.framework.adapter.MethodBeforeAdviceInterceptor.invoke(MethodBeforeAdviceInterceptor.java:58)
at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186)
at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:241)
at com.sun.proxy.$Proxy255.generateIdentifier(Unknown Source)
at org.openmrs.module.idgen.integration.RemoteWithLocalPoolIntegrationTest.testConfigurePoolFilledFromRemoteSource(RemoteWithLocalPoolIntegrationTest.java:50)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at org.springframework.test.context.junit4.statements.RunBeforeTestExecutionCallbacks.evaluate(RunBeforeTestExecutionCallbacks.java:74)
at org.springframework.test.context.junit4.statements.RunAfterTestExecutionCallbacks.evaluate(RunAfterTestExecutionCallbacks.java:84)
at org.springframework.test.context.junit4.statements.RunBeforeTestMethodCallbacks.evaluate(RunBeforeTestMethodCallbacks.java:75)
at org.springframework.test.context.junit4.statements.RunAfterTestMethodCallbacks.evaluate(RunAfterTestMethodCallbacks.java:86)
at org.springframework.test.context.junit4.statements.SpringRepeat.evaluate(SpringRepeat.java:84)
at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:251)
at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:97)
at org.springframework.test.context.junit4.statements.RunBeforeTestClassCallbacks.evaluate(RunBeforeTestClassCallbacks.java:61)
at org.springframework.test.context.junit4.statements.RunAfterTestClassCallbacks.evaluate(RunAfterTestClassCallbacks.java:70)
at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.run(SpringJUnit4ClassRunner.java:190)
I'm not exactly sure what’s causing this issue. However, I suspect that the proxy call doesn’t see the registered processor because the processors haven’t been populated in that proxy context. We might not have encountered this in older versions because Spring's proxying and transaction handling used to be more permissive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Requires_New transactional annotation is very important (or at least it's goal is) which is to ensure that identifier generation does not result in duplicate identifiers being issued. It should not be removed without understanding it's purpose and ensuring with sufficient testing (which is difficult in our unit test framework) that the goals continue to be met.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @mseaton 100%. We should not be removing annotations just as a way to fix tests. Though admittedly this could be a difficult one to solve.
|
@wikumChamith i am getting the exact same build error, locally. |
Surprisingly, I’m not seeing this error locally. It only shows up in here when running against Platform 2.7.6. I just reverted to 2.7.4, and the error no longer occurs here. |
@dkayiwa, even though 2.7.6 is still a snapshot, we have a 2.7.6 maven artifact. Why is that? |
|
I think the release did not complete by updating the pom.xml file |
|
@wikumChamith can you look at the failing tests? They seem not to be caused by the other transaction annotation. |
@dkayiwa, for me, the same tests fail. However, the errors are different. |
|
@wikumChamith Can you fix the other errors and i deal with the transactional one? |
Ticket: https://openmrs.atlassian.net/browse/IDGEN-134