AM-241: Adopt Hibernate Envers auditing for appointment module

[Binayak490-cyber]

Summary

• Upgrades openmrs.platform.version from 2.4.2 to 2.8.4 to match the dev3 reference server (prerequisite for Envers)
• Fixes all compilation and test failures caused by the upgrade
• Adds Hibernate Envers auditing to the appointment module domain entities

Changes

Platform upgrade fixes:

• pom.xml: bump openmrs.platform.version 2.4.2 → 2.8.4
• api/pom.xml, atom-feed/pom.xml, omod/pom.xml: explicitly declare PowerMock and javax.mail dependencies (no longer transitive in openmrs-test 2.8.x)
• DefaultAppointmentStatusChangeValidator: wrap getGlobalProperty() with proxy privilege (AdministrationService now requires privilege in 2.8.x)
• appointmentTestData.xml: move test appointments from patient_id=1 to patient_id=2 (Hibernate 5 in 2.8.x is strict about Person/Patient session cache)
• AtomFeedAdviceActivatorComponentTest: add org.openmrs.logging.* to @PowerMockIgnore (new OpenmrsConfigurationFactory in 2.8.x causes NPE)

Hibernate Envers auditing:

• Added @Audited to Appointment, AppointmentServiceDefinition, AppointmentServiceType, AppointmentProvider, AppointmentRecurringPattern, Speciality
• Added @NotAudited on collection fields pointing to non-audited entities
• Added Liquibase changesets for revision_entity + 6 _AUD shadow tables
• Added AppointmentEnversAuditIT verifying Envers schema creation and error-free saves

Note: Envers is disabled by default in OpenMRS. To activate on a live server, set hibernate.integration.envers.enabled=true in openmrs-runtime.properties.

Test plan

• mvn clean package -Dmaven.test.skip=true → BUILD SUCCESS
• mvn clean package (Java 8, mirrors CI) → BUILD SUCCESS, 0 failures

[Binayak490-cyber]

Hi @druchniewicz, Raised PR-1 (prerequisite) for this ticket: https://openmrs.atlassian.net/browse/AM-241

This PR now upgrades the OpenMRS platform version from 2.4.2 → 2.8.4 to align the Bahmni Appointments module with the latest omrs-core version running on the dev3 reference server.

Once this PR is reviewed and merged, we can proceed with PR-2 - enabling Hibernate Envers auditing (@Audited annotations + _AUD shadow tables + Liquibase changelog) as the main deliverable of this ticket https://openmrs.atlassian.net/browse/AM-241 which is as we planned to proceed.

Please check it out now…!

[CLAassistant]

All committers have signed the CLA.

[druchniewicz]

Hi @druchniewicz, Raised PR-1 (prerequisite) for this ticket: https://openmrs.atlassian.net/browse/AM-241

This PR now upgrades the OpenMRS platform version from 2.4.2 → 2.8.4 to align the Bahmni Appointments module with the latest omrs-core version running on the dev3 reference server.

Once this PR is reviewed and merged, we can proceed with PR-2 - enabling Hibernate Envers auditing (@Audited annotations + _AUD shadow tables + Liquibase changelog) as the main deliverable of this ticket https://openmrs.atlassian.net/browse/AM-241 which is as we planned to proceed.

Please check it out now…!

Hi @Binayak490-cyber I think you can enable the Hibernate envers auditing in this PR. I think it comes down to mark all required entities as @Audited. After that you can create an appointment and check if new entry in appointment audit table appears - I think this will be the best test

[Binayak490-cyber]

Hi @druchniewicz, Raised PR-1 (prerequisite) for this ticket: https://openmrs.atlassian.net/browse/AM-241
This PR now upgrades the OpenMRS platform version from 2.4.2 → 2.8.4 to align the Bahmni Appointments module with the latest omrs-core version running on the dev3 reference server.
Once this PR is reviewed and merged, we can proceed with PR-2 - enabling Hibernate Envers auditing (@Audited annotations + _AUD shadow tables + Liquibase changelog) as the main deliverable of this ticket https://openmrs.atlassian.net/browse/AM-241 which is as we planned to proceed.
Please check it out now…!

Hi @Binayak490-cyber I think you can enable the Hibernate envers auditing in this PR. I think it comes down to mark all required entities as @Audited. After that you can create an appointment and check if new entry in appointment audit table appears - I think this will be the best test

Hi @druchniewicz, thanks for the feedback!

So it means that I will add the @Audited annotations to all the required entities directly in this PR itself, so both the platform upgrade and the Hibernate Envers auditing will be delivered together in PR #194...?

[druchniewicz]

Hi @druchniewicz, thanks for the feedback!

So it means that I will add the @Audited annotations to all the required entities directly in this PR itself, so both the platform upgrade and the Hibernate Envers auditing will be delivered together in PR #194...?

@Binayak490-cyber yes exactly. AM-241 is about adopting appointment module to use hibernate envers auditing so it should all be done in scope of this ticket. Upgrading OMRS version is just a part of this ticket because without it we won't be able to enable auditing in appointments module

[Binayak490-cyber]

Hi @druchniewicz, now I added the Hibernate Envers auditing (commit ae642ff) as part of AM-241.

Changes:

• @Audited added to 6 domain entities: Appointment, AppointmentServiceDefinition, AppointmentServiceType, AppointmentProvider, AppointmentRecurringPattern, Speciality
• @NotAudited on collection fields pointing to non-audited entities to prevent mapping exceptions
• Liquibase changesets for revision_entity + 6 _AUD shadow tables
• AppointmentEnversAuditIT with 5 integration tests verifying schema creation and error-free saves

One thing to say, Note: Envers is off by default in OpenMRS. To activate on a live server, add hibernate.integration.envers.enabled=true to openmrs-runtime.properties. After that, any create/update/delete on an appointment will produce an entry in patient_appointment_AUD.

[druchniewicz]

I think audit tables creation should not be done this way. I think it should work out of the box when you start the application with hibernate.integration.envers.enabled=true the same like all others tables. We don't have liquibase migrations for every single table in OpenMRS, it is done automatically by Envers. It looks for @Audited tables and creates audit tables for them.
What's more these migrations are wrong because you create tables with hardcoded "_AUD" suffix what is wrong

[Binayak490-cyber]

Yes @druchniewicz you are correct - removed all the manual Liquibase changesets for the AUD tables. Envers will create them automatically when hibernate.integration.envers.enabled=true is set, the same way it handles all other audited tables in OpenMRS. The @Audited annotations on the entities are all that's needed. I actually added it while testing but forgot to remove it while committing it.

[Binayak490-cyber]

Any update @druchniewicz...?

[druchniewicz]

Any update @druchniewicz...?

I should be able to verify it today

[druchniewicz]

@Binayak490-cyber at the first glance seems it looks good. Audit tables are created and when I change something in patient appointment new entry is saved in patient_appointment_aud table. Surprising thing is that I can see these entities in Audit log web page:

but results are empty when I select e.g. org.openmrs.module.appointments.model.Appointment even though I have data in these appointment audit tables. AI gives me hints that it can be related to NotAudited annotations in appointments module. @Binayak490-cyber can you deploy auditlog module from this my PR - openmrs/openmrs-module-auditlogweb#31 and test on that?

1. Deploy this auditlog web module
2. Deploy your changes in appointments module
3. Trigger some basic appointments action e.g. just add appointment for patient
4. Go to admin page, View audit trails - there should be logs related to appointments. For all other entities it works so probably issue is related to configuration of annotations in appointments module I guess.

Another important thing that I noticed - default suffix for audit tables is "_audit" so for patient_appointment table audit table will be "patient_appointment_audit" <- created by Envers BUT appointments module already provides own patient_appointment_table so there is a name collision and there can be an issue like

This is a quite specific case and probably we have to use custom suffix different than "_audit", but probably in most of cases there will be such "_audit" suffix used. Can you check if we can somehow handle it? At least catch issues like these and not to break whole migration process?

[Binayak490-cyber]

@Binayak490-cyber at the first glance seems it looks good. Audit tables are created and when I change something in patient appointment new entry is saved in patient_appointment_aud table. Surprising thing is that I can see these entities in Audit log web page:

but results are empty when I select e.g. org.openmrs.module.appointments.model.Appointment even though I have data in these appointment audit tables. AI gives me hints that it can be related to NotAudited annotations in appointments module. @Binayak490-cyber can you deploy auditlog module from this my PR - openmrs/openmrs-module-auditlogweb#31 and test on that?

1. Deploy this auditlog web module
2. Deploy your changes in appointments module
3. Trigger some basic appointments action e.g. just add appointment for patient
4. Go to admin page, View audit trails - there should be logs related to appointments. For all other entities it works so probably issue is related to configuration of annotations in appointments module I guess.

Another important thing that I noticed - default suffix for audit tables is "_audit" so for patient_appointment table audit table will be "patient_appointment_audit" <- created by Envers BUT appointments module already provides own patient_appointment_table so there is a name collision and there can be an issue like

This is a quite specific case and probably we have to use custom suffix different than "_audit", but probably in most of cases there will be such "_audit" suffix used. Can you check if we can somehow handle it? At least catch issues like these and not to break whole migration process?

@druchniewicz, Fixed the name collision - added @AuditTable("patient_appointment_revisions") to Appointment so Envers no longer clashes with the existing patient_appointment_audit table. Unfortunately I don't have a local OpenMRS 2.8.4 server set up to test and it is giving some startup error while deploying the module in .omod files into OpenMRS modules folder. Could you re-test with the updated module since you already have the environment running?

[druchniewicz]

@druchniewicz, Fixed the name collision - added @AuditTable("patient_appointment_revisions") to Appointment so Envers no longer clashes with the existing patient_appointment_audit table. Unfortunately I don't have a local OpenMRS 2.8.4 server set up to test and it is giving some startup error while deploying the module in .omod files into OpenMRS modules folder. Could you re-test with the updated module since you already have the environment running?

@Binayak490-cyber good idea to use custom name for Appointment entity to not clash with the existing patient_appointment_audit.
Regarding the first issue it still exists because nothing has been changed in your commit related to this. There are probably some issues with NotAudited annotations that break fetching data in audit log module. It does not work only for patient appointments so probably there is a problem in appointments module. What problem do you have with deploying audit log module? Maybe we will figure it out.

[Binayak490-cyber]

@druchniewicz, found the root cause of the empty Appointment audit results which you also said and I also think that this was only the prime reason for that.

Two fields were missing @NotAudited:

provider - commented out in HBM but still visible to Envers (no DB column)
notificationResults - a transient non-entity placeholder, not a Hibernate entity
Both caused Envers to silently fail when querying Appointment revisions. Also added a Liquibase precondition to warn on any future _AUD table name collisions which you also demanded so that we can be able to handle and catch those issues so that it does not break whole migration process.

Regarding my previous commit - I intentionally held back the @NotAudited fix to avoid breaking anything without live server verification, since you had the environment running and I am facing some issues while running the server.

Could you please re-test it on your end?

[Binayak490-cyber]

@druchniewicz, Fixed the name collision - added @AuditTable("patient_appointment_revisions") to Appointment so Envers no longer clashes with the existing patient_appointment_audit table. Unfortunately I don't have a local OpenMRS 2.8.4 server set up to test and it is giving some startup error while deploying the module in .omod files into OpenMRS modules folder. Could you re-test with the updated module since you already have the environment running?

@Binayak490-cyber good idea to use custom name for Appointment entity to not clash with the existing patient_appointment_audit. Regarding the first issue it still exists because nothing has been changed in your commit related to this. There are probably some issues with NotAudited annotations that break fetching data in audit log module. It does not work only for patient appointments so probably there is a problem in appointments module. What problem do you have with deploying audit log module? Maybe we will figure it out.

@druchniewicz, I think the issue which I am facing to run and deploy the audit log module is related to java versions, my JAVA_HOME is setup with java 17 or more as all other repositories of OpenMRS where I have worked, all requires that only but the bahmni openmrs-module-appointments still runs on java 8 but my desktop is not yet configured for java 8, so I think my all modules for this repo is configured and built upon java 17 or more and therefore I am facing the issue while running and deploying the audit appointment log module of Bahmni.

[druchniewicz]

@druchniewicz, I think the issue which I am facing to run and deploy the audit log module is related to java versions, my JAVA_HOME is setup with java 17 or more as all other repositories of OpenMRS where I have worked, all requires that only but the bahmni openmrs-module-appointments still runs on java 8 but my desktop is not yet configured for java 8, so I think my all modules for this repo is configured and built upon java 17 or more and therefore I am facing the issue while running and deploying the audit appointment log module of Bahmni.

@Binayak490-cyber I think it should not be a problem, I'm currently using java21 and I can deploy auditlog module, appointments module and others. You may probably have problems when you build module and tests fail. Please use mvn clean install -DskipTests and audit log module should be built. Then deploy this module in your local instance and that's all. Without it, it will be difficult to test changes in appointments module properly.

[Binayak490-cyber]

@druchniewicz, I think the issue which I am facing to run and deploy the audit log module is related to java versions, my JAVA_HOME is setup with java 17 or more as all other repositories of OpenMRS where I have worked, all requires that only but the bahmni openmrs-module-appointments still runs on java 8 but my desktop is not yet configured for java 8, so I think my all modules for this repo is configured and built upon java 17 or more and therefore I am facing the issue while running and deploying the audit appointment log module of Bahmni.

@Binayak490-cyber I think it should not be a problem, I'm currently using java21 and I can deploy auditlog module, appointments module and others. You may probably have problems when you build module and tests fail. Please use mvn clean install -DskipTests and audit log module should be built. Then deploy this module in your local instance and that's all. Without it, it will be difficult to test changes in appointments module properly.

@druchniewicz, yes I think that as there are many extensions installed in my IDE, like metals and many more which is overwriting and breaking the openmrs module updates and dependencies and I think that due to the broken dependencies and as you said the failing tests for that when doing mvn clean install -DskipTests is the issue for this.

[druchniewicz]

@druchniewicz, yes I think that as there are many extensions installed in my IDE, like metals and many more which is overwriting and breaking the openmrs module updates and dependencies and I think that due to the broken dependencies and as you said the failing tests for that when doing mvn clean install -DskipTests is the issue for this.

@Binayak490-cyber audit log PR has been merged so you can just use version from the main branch - https://github.com/openmrs/openmrs-module-auditlogweb
Try to build it using mvn clean install -DskipTests and deploy this module to your local instance. Thanks to this you will be able to test your changes in appointments module as a whole. Your current PR in appointments module looks good but it does not work in 100% together with audit log module - but this is most important thing - to be able to track changes in appointments entities in audit log module.

[Binayak490-cyber]

@druchniewicz, yes I think that as there are many extensions installed in my IDE, like metals and many more which is overwriting and breaking the openmrs module updates and dependencies and I think that due to the broken dependencies and as you said the failing tests for that when doing mvn clean install -DskipTests is the issue for this.

@Binayak490-cyber audit log PR has been merged so you can just use version from the main branch - https://github.com/openmrs/openmrs-module-auditlogweb Try to build it using mvn clean install -DskipTests and deploy this module to your local instance. Thanks to this you will be able to test your changes in appointments module as a whole. Your current PR in appointments module looks good but it does not work in 100% together with audit log module - but this is most important thing - to be able to track changes in appointments entities in audit log module.

Okk @druchniewicz.

[druchniewicz]

@druchniewicz, yes I think that as there are many extensions installed in my IDE, like metals and many more which is overwriting and breaking the openmrs module updates and dependencies and I think that due to the broken dependencies and as you said the failing tests for that when doing mvn clean install -DskipTests is the issue for this.

@Binayak490-cyber audit log PR has been merged so you can just use version from the main branch - https://github.com/openmrs/openmrs-module-auditlogweb Try to build it using mvn clean install -DskipTests and deploy this module to your local instance. Thanks to this you will be able to test your changes in appointments module as a whole. Your current PR in appointments module looks good but it does not work in 100% together with audit log module - but this is most important thing - to be able to track changes in appointments entities in audit log module.

Okk @druchniewicz.

@Binayak490-cyber were you able to set it up locally and test it?

[druchniewicz]

@Binayak490-cyber reminder