AUDIT-28: Backfill pre-existing rows into Envers audit tables to fix "Unable to read" in audit UI#5933
Conversation
9d093dc to
a2bb48b
Compare
|
Hi @wikumChamith, @druchniewicz, @nsalifu, SonarCloud flagged a few SQL Injection security hotspots due to dynamically constructed queries in These queries only use table and column names sourced from Hibernate/JDBC metadata (not user input). To make this explicit, I added a If an identifier contains any unsafe characters, an Since SonarCloud hotspots require manual review, could a maintainer please review and mark them as Safe if this approach looks correct? |
a2bb48b to
6af3ad8
Compare
…'Unable to read' in audit UI
6af3ad8 to
fdf4ec8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5933 +/- ##
============================================
- Coverage 59.31% 59.30% -0.01%
- Complexity 9252 9274 +22
============================================
Files 686 687 +1
Lines 37123 37241 +118
Branches 5452 5475 +23
============================================
+ Hits 22018 22085 +67
- Misses 13141 13178 +37
- Partials 1964 1978 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @wikumChamith, @druchniewicz, @nsalifu, SonarCloud flagged a few SQL Injection security hotspots due to dynamically constructed queries in EnversAuditTableInitializer. These queries only use table and column names sourced from Hibernate/JDBC metadata (not user input). To make this explicit, I added a requireSafeIdentifier(String) check that validates identifiers against [a-zA-Z_][a-zA-Z0-9_]* before they are used in SQL. If an identifier contains any unsafe characters, an IllegalArgumentException is thrown and the query is not executed. Since SonarCloud hotspots require manual review, could a maintainer please review and mark them as Safe if this approach looks correct? |
|
@Binayak490-cyber did you run your code on real instance of OpenMRS? (real I mean e.g. on your localhost), because when I ran it on my local machine I got multiple errors like It is just an example, it occurs for all audit tables. |
Thanks for testing @druchniewicz ! The fix dynamically discovers the timestamp column from JDBC metadata instead of hardcoding REVTSTMP. Could you test the updated code on your local instance to confirm it resolves the errors? |
Did you test your changes? I think the best will be running clean distro on your local machine and check if your changes work. |
|
@Binayak490-cyber, as @druchniewicz pointed out earlier, your changes don't seem to be working. I can confirm the same. I'm still seeing empty audit tables. Make sure you actually run and manually verify your changes before making a PR. |
…nt revision_entity table
8e14571 to
09048e3
Compare
|
Thanks @druchniewicz and @wikumChamith for testing. In the last push, I’ve verified the fixes locally on a fresh OpenMRS distro using Docker + MariaDB. Fixed the On a clean DB, multiple audit tables were successfully backfilled (20+ tables), and no errors were observed in logs. Could you please re-test on your end? |
|
What do you think about using Liquibase CustomTaskChange rather than doing a JDBC backfill? |
Yeah, this. |
|
So, @ibacher, @wikumChamith, @dkayiwa, does it means that I will go for a rework, but before that want to clarify that:
What do you want...? |
We should keep EnversAuditTableInitializer as it is because it needs to run with any schema changes. However, the data backfill is only going to happen once. |
https://github.com/search?q=org%3Aopenmrs%20CustomTaskChange&type=code |
Okk, thanks @wikumChamith!👍 |
7e3c22f to
bf75000
Compare
|
|
@wikumChamith, @druchniewicz, @ibacher, @dkayiwa, can you take a look at it now, I have now moved the Envers audit table backfill from a JDBC Hibernate Integrator (runs every startup) into a Liquibase CustomTaskChange (runs exactly once per database), verified locally also and pushed it to the PR...? |
@Binayak490-cyber I'm trying to deploy your changes on my local instance but seems something does not work. I'm using openmrs-core 2.9.x version - from this branch I copied BackfillEnversAuditTablesChangeset.java, EnversAuditTableInitializer.java and your change set to liquibase-update-to-latest-2.9.x (3.0.x xml does not exist in omrs-core 2.9.x) built and deployed it to my local instance. Seems that this migration didn't run at all because I don't see the entry in liquibasechangelog table in database. And audit tables are also empty. Could you verify it? Maybe I miss something? Can you try to run these changes on omrs-core 2.9.x as and check if it works for you? |
@druchniewicz, thanks for testing! I wanted to clarify that this PR targets openmrs-core 3.0.x (master/3.0.0-SNAPSHOT), not 2.9.x. Our changeset is in liquibase-update-to-latest-3.0.x.xml, which only exists in the 3.0.x codebase. The reason the migration didn't run on your 2.9.x instance is that:
Could you test by building and running the full 3.0.x branch with our changes? That is the intended target for this PR. If a 2.9.x backport is needed, that would be a separate effort. |
I'm using 2.9.x because there were some issues with running the latest omrs core snapshot version (3.0.0-snapshot). I used OpenMRS 2.8.3 version and I wanted to test changes related to hibernate envers that were merged recently (in February). When I deployed the latest version of omrs-core from master branch (3.0.0-snapshot) then the run app does not start correctly because of some failing migrations. I needed to back to stable 2.9.x branch (it also contains hibernate envers commit) to run the app successfully. |
Thank you for understanding @druchniewicz. Actually, since the Envers commit is also in 2.9.x, the backfill should work there too. Since this PR is scoping for 3.0.0-SNAPSHOT only, I think it would be cleaner to keep it focused here. Once this gets accepted and merged, I can open a separate PR to add the AUDIT-28-2026-03-19 changeset to liquibase-update-to-latest-2.9.x.xml, rather than mixing both version changes in the same PR which will be very big for a single PR and also there are many tickets and issues in jira related around this, so separately doing all, I think will be good. Regarding the failing migrations on 3.0.0-SNAPSHOT — is that a known issue being tracked separately? Just want to make sure our changes aren't contributing to it. |
@Binayak490-cyber if you have time you can just use the same changes for 2.9.x and check if they works. Just run the refdistro app (use e.g. dev3 tag for docker image), build the omrs-core module from 2.9.x branch and check if audit tables will be backfilled properly. |
@druchniewicz, I tested the changes on 2.9.x using the refdistro app with the dev3 Docker image as you suggested. The backfill changeset works correctly on 2.9.x as well — AUDIT-28-2026-03-19 was recorded in liquibasechangelog and audit tables like concept_name_audit, concept_audit, concept_reference_map_audit were all populated with data. I have ported the changeset to liquibase-update-to-latest-2.9.x.xml and pushed it to my fork. Should I open a separate PR targeting the 2.9.x branch for this as you can also check and test it in that branch...??? |


Summary
Fixes AUDIT-28 where important fields appear as "Unable to read" in the audit UI.
Root Cause
When Envers auditing is enabled after data already exists, the *_audit tables are created empty. New audit records referencing those pre-existing entities cannot be resolved by Envers.
Fix
Added backfillAuditTables() in EnversAuditTableInitializer to populate audit tables with existing rows during initialization. Missing rows are inserted with REVTYPE=0 (ADD) under a synthetic revision.
Changes
Testing
Verified using H2 and unit tests that: