EMRAPI support for OpenMRS 2.0

@mseaton, I did a bit of debugging and indeed if you try to start openmrs-core 2.0 with this set of modules, you get the exception that @preethi_s shared:

calculation-1.2.omod			providermanagement-2.5.0.omod
emrapi-1.18-SNAPSHOT.omod		reporting-0.10.2.omod
event-2.5-SNAPSHOT.omod			serialization.xstream-0.2.12.omod
htmlwidgets-1.7.3-SNAPSHOT.omod		uiframework-3.7.omod
legacyui-1.2-SNAPSHOT.omod		uilibrary-2.0.4.omod
metadatamapping-1.2.0.omod		webservices.rest-2.16.omod
metadatasharing-1.2.2.omod

Further it comes from loading the bean dataExportDataSetDefinitionPersister, which may come from this line:

https://github.com/openmrs/openmrs-module-reporting/blob/master/api/src/main/java/org/openmrs/module/reporting/dataset/definition/persister/DataExportDataSetDefinitionPersister.java#L71

(I don’t yet know why this behaves different now from how it did prior to 2.0.)

FWIW, this class was deprecated https://github.com/openmrs/openmrs-core/blob/1.12.x/api/src/main/java/org/openmrs/reporting/AbstractReportObject.java and then removed from platform 2.0 During this ticket TRUNK-4337 and this commit, it was moved to the reportingcompatibility module https://github.com/openmrs/openmrs-module-reportingcompatibility/commit/59fe85db6d2f0e45041e3bf434b4d3e4c07dbc83

This doesn’t surprise me at all, and I would expect this to be a problem. The bigger issue is that there is not profile that tests the reporting module at all against version of OpenMRS beyond 1.10. There is the default setting that runs tests against 1.9, and there is a 1.10 profile (which I’m not sure is ever run in an automated or regular fashion). Beyond that, there is nothing that does any kind of unit testing against OpenMRS 1.11, 1.12, or anything in the 2.x line. If we had that, this problem would have been identified long ago. Thoughts on how we can do a better job with this? Is this something that Travis CI can help us with?

There was some effort put into adding 2.x compatiblity in the reporting module at one point - @dkayiwa I believe was involved in doing that. So there is a 2.x maven submodule where we could move this particular class. Should be straight-forward to do that. Adding the additional testing profiles for 1.11+ will be considerably more work I think.

Regardless, this should be considered a bug caused by 2.0 API changes, and should not be solved by adding reportingcompatibility as a dependency.

Mike

I should also mention that this particular feature (exposing data exports from the reportingcompatiblity module as data sets in the reporting module) was always a bit gimmiky and there more to demonstrate the concept of different data persisters than anything else. I would sincerely doubt that anyone actually uses this feature (or knows that it exists). We could likely simply remove it, but I have not done so in order to preserve backwards compatibility.

Perhaps we could simply remove it and release the next version of reporting as (finally) 1.0.0 :slight_smile:

Mike

At first glance it seems easy to test against the different versions, per this: https://wiki.openmrs.org/x/aIC5AQ (at least, I was able to get things to fail to compile against openmrs-core 2.0 because of the removed classes – I don’t know know much of a pain it would be to tweak the test datasets).

Would we want to be running builds and tests against the different supported openmrs-core versions in CI? (I assume the answer is yes, though this will of course make the reporting build take 5x longer, for 1.9, 1.10, 1.11, 1.12, and 2.0…)

Yes, @darius I assume it will be easy for us to set up more profiles in the pom. I do think it will be tedious, though not particularly hard, to update the test datasets for each new version of the platform. And I share your question about how and when to run these. There has been a 1.10 profile for a while that is never run, for example, so it isn’t doing much good. This is where I was wondering whether Travis CI might be a solution that could help while not running every profile on Bamboo.

Mike

I can confirm after spending hours getting the test to run against openmrs-core 1.11 that this is very tedious. (Particularly because prior to 1.11 we never checked referential integrity of test datasets, and the one in reporting has problems.) I have committed this work under https://issues.openmrs.org/browse/REPORT-772

I also created maven profiles for compiling against 1.12 and 2.0, but I haven’t updated the tests to work for these yet. However you can see the compile errors against openmrs-core 2.0 if you pull the latest version of the module and do

mvn -P2.0 clean package

I do think we should start running these in CI (i.e. bamboo) for now, and we can think about moving them to another plan, or to travis, later. (I’ll create a plan for the 1.10 and 1.11 builds, which do pass now.)

I added 1.10 and 1.11 jobs to the existing reporting module build in CI. The build took 7 minutes instead of 5 for the prior one.

I’d appreciate an opinion from @cintiadr’s or others as to whether this is the right approach: https://ci.openmrs.org/chain/admin/config/defaultStages.action?buildKey=REP-REP

Thanks @darius.

I have followed on from your work and gotten a 1.12 profile working as well.

For 2.x, I have fixed the issue around reportingcompatibility by moving this code from the main API and into the pre-2.x API only. This seems to have solved that initially reported issue and enables the module to built successfully for the 2.0 profile as long as one skips tests.

For getting tests to pass for the 2.0 profile, I have done some work towards overcoming a bunch of issues, but not all are fully resolved. I have created REPORT-773 to address these remaining issues. I would welcome someone willing to take this on.

Thanks, Mike

That’s a pretty good approach, IMO. I mean, you can have a separate plan (totally independent) instead of job, and that would make it not block a release in case it’s red. But I suppose that’s not what you want in this case.

Having a profile is not a bad approach at all as well. It’s pretty popular to enable profiles if certain flags are set on the command line, but I think the -P works as good as flags. Also, I know you are not using it, but as you started with maven profiles do not use activeByDefault. https://dzone.com/articles/maven-profile-best-practices

There are few things to keep in mind:

  • The current build can possibly upload snapshots to nexus even if the 1.10/1.11/1.12 jobs are red. As Jobs are concurrent, this is a side effect; other builds will use those more recent snapshots even if some of their builds are red. If you want to prevent that, you’d need to make the deployment a serial stage after the others.
  • I suppose you don’t have integration-tests on those extra jobs, because integration-tests phase comes after package in maven lifecycle (https://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html)
  • The first time a certain job runs on a specific agent, it might take quite a lot of time to download maven dependencies. I’d expect things to be more or less the same time after a couple of runs on the same machine/user.

We don’t have any integration tests that I know of but it seems like good practice to change to verify instead of package.

Just to check, the secondary jobs that do ‘verify’ should not write to the local maven repo because we haven’t ever done install, right? (Really I mean that there won’t be any bad interaction where anyone else accidentally uses the artifacts built by the secondary jobs, not the default one.)

-Darius (by phone)

That’s absolutely correct.

That shouldn’t be the case anyway, even if you do a ‘mvn install’. Bamboo is configured to remove any -SNAPSHOT from the .m2 folder before any build (https://ci.openmrs.org/admin/viewPredatorPlugin.action), so you only have to care about the ‘mvn deploy’ ones, not really ‘mvn install’.

As we have two agents running per machine/VM, I forced them to run in two different users, so you never have one build cache affecting the other (assuming we are talking here about cache on the user home).

@mseaton @dkayiwa @darius I see that there is an new version of reporting omod (0.10.3) released with the changes mentioned above. Does this mean we can use reporting-omod 0.10.3 and ignore reportingcompatibility omod to make it work with Openmrs 2.x?

@mseaton When I tried to use reporting omod (0.10.3) with openmrs 2, I get the following exception:

WARN - AbstractApplicationContext.refresh(487) |2016-10-05 11:55:31,262| Exception encountered during context initialization - cancelling refresh attempt org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'dataExportDataSetDefinitionMappingHandler' defined in URL [jar:file:/opt/openmrs/.openmrs-lib-cache/reporting/reporting.jar!/org/openmrs/module/reporting/web/controller/mapping/DataExportDataSetDefinitionMappingHandler.class]: Initialization of bean failed; nested exception is java.lang.IllegalStateException: Failed to introspect annotations: class org.openmrs.module.reporting.web.controller.mapping.DataExportDataSetDefinitionMappingHandler at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:547) at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:476) at org.springframework.beans.factory.support.AbstractBeanFactory$1.getObject(AbstractBeanFactory.java:303) at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:230) at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:299) at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:194) at org.springframework.beans.factory.support.DefaultListableBeanFactory.preInstantiateSingletons(DefaultListableBeanFactory.java:762) at org.springframework.context.support.AbstractApplicationContext.finishBeanFactoryInitialization(AbstractApplicationContext.java:757) at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:480) at org.openmrs.module.ModuleUtil.refreshApplicationContext(ModuleUtil.java:842) at org.openmrs.module.web.WebModuleUtil.refreshWAC(WebModuleUtil.java:866) at org.openmrs.web.Listener.performWebStartOfModules(Listener.java:653) at org.openmrs.web.Listener.performWebStartOfModules(Listener.java:632) at org.openmrs.web.Listener.startOpenmrs(Listener.java:267) at org.openmrs.web.WebDaemon$1.run(WebDaemon.java:42) Caused by: java.lang.IllegalStateException: Failed to introspect annotations: class org.openmrs.module.reporting.web.controller.mapping.DataExportDataSetDefinitionMappingHandler at org.springframework.core.annotation.AnnotatedElementUtils.process(AnnotatedElementUtils.java:166) at org.springframework.core.annotation.AnnotatedElementUtils.getAnnotationAttributes(AnnotatedElementUtils.java:91) at org.springframework.core.annotation.AnnotatedElementUtils.getAnnotationAttributes(AnnotatedElementUtils.java:85) at org.springframework.transaction.annotation.SpringTransactionAnnotationParser.parseTransactionAnnotation(SpringTransactionAnnotationParser.java:42) at org.springframework.transaction.annotation.AnnotationTransactionAttributeSource.determineTransactionAttribute(AnnotationTransactionAttributeSource.java:154) at org.springframework.transaction.annotation.AnnotationTransactionAttributeSource.findTransactionAttribute(AnnotationTransactionAttributeSource.java:138) at org.springframework.transaction.interceptor.AbstractFallbackTransactionAttributeSource.computeTransactionAttribute(AbstractFallbackTransactionAttributeSource.java:155) at org.springframework.transaction.interceptor.AbstractFallbackTransactionAttributeSource.getTransactionAttribute(AbstractFallbackTransactionAttributeSource.java:100) at org.springframework.transaction.interceptor.TransactionAttributeSourcePointcut.matches(TransactionAttributeSourcePointcut.java:38) at org.springframework.aop.support.AopUtils.canApply(AopUtils.java:225) at org.springframework.aop.support.AopUtils.canApply(AopUtils.java:262) at org.springframework.aop.support.AopUtils.findAdvisorsThatCanApply(AopUtils.java:294) at org.springframework.aop.framework.autoproxy.AbstractAdvisorAutoProxyCreator.findAdvisorsThatCanApply(AbstractAdvisorAutoProxyCreator.java:118) at org.springframework.aop.framework.autoproxy.AbstractAdvisorAutoProxyCreator.findEligibleAdvisors(AbstractAdvisorAutoProxyCreator.java:88) at org.springframework.aop.framework.autoproxy.AbstractAdvisorAutoProxyCreator.getAdvicesAndAdvisorsForBean(AbstractAdvisorAutoProxyCreator.java:69) at org.springframework.aop.framework.autoproxy.AbstractAutoProxyCreator.wrapIfNecessary(AbstractAutoProxyCreator.java:330) at org.springframework.aop.framework.autoproxy.AbstractAutoProxyCreator.postProcessAfterInitialization(AbstractAutoProxyCreator.java:293) at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.applyBeanPostProcessorsAfterInitialization(AbstractAutowireCapableBeanFactory.java:422) at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1571) at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:539) ... 14 more Caused by: java.lang.ArrayStoreException: sun.reflect.annotation.TypeNotPresentExceptionProxy at sun.reflect.annotation.AnnotationParser.parseClassArray(AnnotationParser.java:724) at sun.reflect.annotation.AnnotationParser.parseArray(AnnotationParser.java:531) at sun.reflect.annotation.AnnotationParser.parseMemberValue(AnnotationParser.java:355) at sun.reflect.annotation.AnnotationParser.parseAnnotation2(AnnotationParser.java:286) at sun.reflect.annotation.AnnotationParser.parseAnnotations2(AnnotationParser.java:120) at sun.reflect.annotation.AnnotationParser.parseAnnotations(AnnotationParser.java:72) at java.lang.Class.createAnnotationData(Class.java:3521) at java.lang.Class.annotationData(Class.java:3510) at java.lang.Class.getAnnotations(Class.java:3446) at org.springframework.core.annotation.AnnotatedElementUtils.doProcess(AnnotatedElementUtils.java:192) at org.springframework.core.annotation.AnnotatedElementUtils.process(AnnotatedElementUtils.java:162) ... 33 more

I see that DataExportDataSetDefinition has been moved from api to api 1.9 but this is being used by DataExportDataSetDefinitionMappingHandler in omod submodule. I think it is throwing error since DataExportDataSetDefinition is not available in runtime. How should we go about fixing this?

@mseaton same thing happens with Reference Application on http://int02.openmrs.org/

This is happening because DataExportDataSetDefinitionMappingHandler references DataExportDataSetDefinition which in turn references DataExportReportObject that was moved to the reportingcompatibility module.

@preethi_s @gutkowski i have just released a new version (0.10.4) of the reporting module with a fix for this.

1 Like

Thank you @dkayiwa!!

Thanks @dkayiwa.