#292 - Error loading locations on OpenMRS 2.7

[mseaton]

This PR copies the existing LocationsLoaderIntegrationTest into the api-2.7 module to demonstrate that this test fails when run in an OpenMRS 2.7 environment.

The cause appears to be due to a change to Hibernate from v5 to v6, as well as a move from xml mapping files to JPA annotations, which is leading to slightly different behavior that previously. The particular error seems to stem from the way the BaseAttributeLineProcesser handles attributes - which is by removing any existing attributes from the collection and adding a new one in.

The change that has been implemented here changes this to the following:

• If an existing attribute with the given type exists as one being processed in a row, then:
• If the existing value matches the incoming value, do nothing
• If the existing value does not match the incoming value, void the existing value
• If the incoming value is not null, add the incoming value as a new attribute

This is a change to the previous behavior which always deleted old values, whether they had changed or not. This deletion was done by removing old values from the attributes collection on the object, rather than voiding the attributes in place. This seems to have been the root of the issue, as we were running into situations where Hibernate was trying to flush objects to the database that had been disconnected from their related objects (eg. trying to flush an update to a LocationAttribute that no longer was associated to a Location, thus trying to save null as the location_id column in the location_attribute table).

@rbuisson / @ibacher / @Ruhanga - FYI

[mseaton]

@ibacher / @Ruhanga - I was hoping to add @mogoodrich as a reviewer, but that does't seem allowed. Any way I can be able to add him in?

I'd like to be able to get this fix in before too long, as this is currently the only thing blocking our ability to upgrade to 2.7.x. Feel free to loop in others to review as appropriate.

[Ruhanga]

Overall, LGTM, and I trust it will work as expected at runtime. I do have a small comment below, though.

[Ruhanga]

I guess this was meant to point to attributeTypeIdentifier? Would be nice having a unit test covering this - maybe not strictly necessary?

Suggested change

	+ attributeType + "'");
	+ attributeTypeIdentifier + "'");

[mseaton]

Good catch @Ruhanga . I've updated that. This isn't new functionality, my main goal was to ensure all existing unit and integration tests pass, as well as any that I copied into the api-2.7 project, so my assumption is that the existing unit test coverage is sufficiently robust to handle a refactor.

[Ruhanga]

Hi @mseaton, there seems to be failures when running with OpenMRS < 2.7.0 the following error is thrown, might you have seen this already? Example trace below...

ERROR - BaseCsvLoader.lambda$load$0(117) |2025-02-05T19:23:15,508| An OpenMRS object could not be constructed or saved from the following CSV line:
+--------------------------------------+-------------+---------+-------------+--------+--------------------+--------------------+-----------------------+--------------------+------------------------+-----------------------+------------------------------------------------+---------------------------+-----------+-----------+-----------+-----------+-----------+-----------+--------------+-----------------+----------------+-------------+---------+-------------+
| Uuid                                 | Void/Retire | Name    | Description | Parent | Tag|Login Location | Tag|Visit Location | Tag|Facility Location | Tag|Queue Location | Tag|Admission Location | Tag|Transfer Location | Attribute|9eca4f4e-707f-4bb8-8289-2f9b6e93803c | Attribute|Last Audit Date | Address 1 | Address 2 | Address 3 | Address 4 | Address 5 | Address 6 | City/Village | County/District | State/Province | Postal Code | Country | _order:1000 |
+--------------------------------------+-------------+---------+-------------+--------+--------------------+--------------------+-----------------------+--------------------+------------------------+-----------------------+------------------------------------------------+---------------------------+-----------+-----------+-----------+-----------+-----------+-----------+--------------+-----------------+----------------+-------------+---------+-------------+
| 0fa578fc-301a-418c-9cf9-b35707fcb478 |             | Site 48 |     Site 48 |        |               TRUE |               TRUE |                       |                    |                        |                       |                                                |                           |           |           |           |           |           |           |              |                 |                |             |         |             |
+--------------------------------------+-------------+---------+-------------+--------+--------------------+--------------------+-----------------------+--------------------+------------------------+-----------------------+------------------------------------------------+---------------------------+-----------+-----------+-----------+-----------+-----------+-----------+--------------+-----------------+----------------+-------------+---------+-------------+
java.lang.IllegalArgumentException: An attribute value is specified ('') for an attribute type that cannot be resolved by the following identifier: '9eca4f4e-707f-4bb8-8289-2f9b6e93803c'
        at org.openmrs.module.initializer.api.BaseAttributeLineProcessor.fill(BaseAttributeLineProcessor.java:40) ~[initializer-api-2.9.0-SNAPSHOT.jar:?]
        at org.openmrs.module.initializer.api.BaseAttributeLineProcessor.fill(BaseAttributeLineProcessor.java:22) ~[initializer-api-2.9.0-SNAPSHOT.jar:?]
        at org.openmrs.module.initializer.api.CsvParser.initialize(CsvParser.java:247) ~[initializer-api-2.9.0-SNAPSHOT.jar:?]
        at org.openmrs.module.initializer.api.CsvParser.process(CsvParser.java:202) ~[initializer-api-2.9.0-SNAPSHOT.jar:?]
        at org.openmrs.module.initializer.api.loaders.BaseCsvLoader.load(BaseCsvLoader.java:99) ~[initializer-api-2.9.0-SNAPSHOT.jar:?]
        at org.openmrs.module.initializer.api.loaders.BaseInputStreamLoader.load(BaseInputStreamLoader.java:40) ~[initializer-api-2.9.0-SNAPSHOT.jar:?]
        at org.openmrs.module.initializer.api.loaders.BaseFileLoader.lambda$loadUnsafe$3(BaseFileLoader.java:86) ~[initializer-api-2.9.0-SNAPSHOT.jar:?]
        at java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183) ~[?:?]
        at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:177) ~[?:?]
        at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:195) ~[?:?]
        at java.util.stream.SortedOps$SizedRefSortingSink.end(SortedOps.java:357) ~[?:?]
        at java.util.stream.Sink$ChainedReference.end(Sink.java:258) ~[?:?]
        at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:485) ~[?:?]
        at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474) ~[?:?]
        at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) ~[?:?]
        at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) ~[?:?]
        at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[?:?]
        at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:497) ~[?:?]
        at org.openmrs.module.initializer.api.loaders.BaseFileLoader.loadUnsafe(BaseFileLoader.java:83) ~[initializer-api-2.9.0-SNAPSHOT.jar:?]
        at org.openmrs.module.initializer.api.InitializerServiceImpl.loadUnsafe(InitializerServiceImpl.java:102) ~[initializer-api-2.9.0-SNAPSHOT.jar:?]
        at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:?]
        at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[?:?]
        at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:?]
        at java.lang.reflect.Method.invoke(Method.java:566) ~[?:?]
        at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:344) ~[spring-aop-5.3.23.jar:5.3.23]
        at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:198) ~[spring-aop-5.3.23.jar:5.3.23]
        at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:163) ~[spring-aop-5.3.23.jar:5.3.23]
        at org.springframework.cache.interceptor.CacheInterceptor.lambda$invoke$0(CacheInterceptor.java:54) ~[spring-context-5.3.23.jar:5.3.23]
        at org.springframework.cache.interceptor.CacheAspectSupport.execute(CacheAspectSupport.java:351) ~[spring-context-5.3.23.jar:5.3.23]
        at org.springframework.cache.interceptor.CacheInterceptor.invoke(CacheInterceptor.java:64) ~[spring-context-5.3.23.jar:5.3.23]
        at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186) ~[spring-aop-5.3.23.jar:5.3.23]
        at org.openmrs.aop.LoggingAdvice.invoke(LoggingAdvice.java:123) ~[openmrs-api-2.6.16-SNAPSHOT.jar:?]
        at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186) ~[spring-aop-5.3.23.jar:5.3.23]
        at org.springframework.aop.framework.adapter.MethodBeforeAdviceInterceptor.invoke(MethodBeforeAdviceInterceptor.java:58) ~[spring-aop-5.3.23.jar:5.3.23]
        at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186) ~[spring-aop-5.3.23.jar:5.3.23]
        at org.springframework.aop.framework.adapter.MethodBeforeAdviceInterceptor.invoke(MethodBeforeAdviceInterceptor.java:58) ~[spring-aop-5.3.23.jar:5.3.23]
        at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:186) ~[spring-aop-5.3.23.jar:5.3.23]
        at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:215) ~[spring-aop-5.3.23.jar:5.3.23]
        at com.sun.proxy.$Proxy315.loadUnsafe(Unknown Source) ~[?:?]
        at org.openmrs.module.initializer.InitializerActivator.started(InitializerActivator.java:87) ~[initializer-api-2.9.0-SNAPSHOT.jar:?]
        at org.openmrs.module.ModuleUtil.refreshApplicationContext(ModuleUtil.java:922) ~[openmrs-api-2.6.16-SNAPSHOT.jar:?]
        at org.openmrs.module.web.WebModuleUtil.refreshWAC(WebModuleUtil.java:923) ~[openmrs-web-2.6.16-SNAPSHOT.jar:?]
        at org.openmrs.web.Listener.performWebStartOfModules(Listener.java:723) ~[openmrs-web-2.6.16-SNAPSHOT.jar:?]
        at org.openmrs.web.Listener.performWebStartOfModules(Listener.java:703) ~[openmrs-web-2.6.16-SNAPSHOT.jar:?]
        at org.openmrs.web.Listener.startOpenmrs(Listener.java:369) ~[openmrs-web-2.6.16-SNAPSHOT.jar:?]
        at org.openmrs.web.WebDaemon$1.run(WebDaemon.java:42) ~[openmrs-web-2.6.16-SNAPSHOT.jar:?]

[mseaton]

@Ruhanga I have not seen anything like this. First question I'd have to ask is whether you have a Location Attribute with UUID of 9eca4f4e-707f-4bb8-8289-2f9b6e93803c in your initializer configuration or already in the system.

[Ruhanga]

Ah, I see now. Thanks for highlighting this. It's probably that the attribute type (9eca4f4e-707f-4bb8-8289-2f9b6e93803c) is indeed missing. I encountered the error in the recent O3 Ref App, which I believe has been misconfigured. Apologies for the false alarm, @mseaton.