TRUNK-6469: Add support for automatic initialization of Envers audit tables #5468
Conversation
wikumChamith
force-pushed
the
TRUNK-6469
branch
4 times, most recently
from
4e8f41a to
960a0af
Compare
| * @param hibernateProperties properties containing Envers configuration | ||
| * @param serviceRegistry Hibernate service registry | ||
| */ | ||
| public static void initializeAuditTables(Metadata metadata, Properties hibernateProperties, |
There was a problem hiding this comment.
What do you think of removing parts of the method name that are already in the class name?
There was a problem hiding this comment.
I changed the method name.
wikumChamith
force-pushed
the
TRUNK-6469
branch
from
960a0af to
a4974af
Compare
| private static void updateAuditTables(Metadata metadata, Properties hibernateProperties, | ||
| ServiceRegistry serviceRegistry) throws Exception { | ||
| String auditTablePrefix = hibernateProperties.getProperty("org.hibernate.envers.audit_table_prefix", ""); | ||
| String auditTableSuffix = hibernateProperties.getProperty("org.hibernate.envers.audit_table_suffix", "_aud"); |
There was a problem hiding this comment.
The main thing I might ask here is that we name the audit tables _audit. It's more consistent with how we name other DB tables and this isn't MUMPS where tables names are (IIRC) something like 10 characters.
| updateAuditTables(metadata, hibernateProperties, serviceRegistry); | ||
| log.info("Envers audit table initialization completed successfully."); | ||
| } | ||
| catch (Exception e) { |
There was a problem hiding this comment.
Shouldn't this exception potentially stop the server if someone opted into using Envers but the tables didn't generate?
There was a problem hiding this comment.
I tested this by maually throwing an exception from updateAuditTables and it did not stoped the server
There was a problem hiding this comment.
Had you done something with the other try / catch here? I can't see where that exception would be swallowed.
There was a problem hiding this comment.
I think we don't need try-catch here, so I removed it :)
wikumChamith
force-pushed
the
TRUNK-6469
branch
2 times, most recently
from
07955a5 to
76ef78d
Compare
|
@ibacher I updated the PR. |
wikumChamith
force-pushed
the
TRUNK-6469
branch
3 times, most recently
from
bfa3987 to
87ea66e
Compare
wikumChamith
force-pushed
the
TRUNK-6469
branch
from
86f745f to
aa101ef
Compare
nsalifu
left a comment
nsalifu
left a comment
There was a problem hiding this comment.
I've tested this and works as expected. thank you @wikumChamith
4 participants
Description of what I changed
This Implements a production-ready alternative to
hbm2ddl.auto=updatefor automatically generating Hibernate Envers audit tables when auditing is enabled.To test add
hibernate.integration.envers.enabled=truetoopenmrs-runtime.propertiesfile.This need to also get backported to 2.7.x and 2.8.x. However because they are using Hibernate 5 we could use
org.hibernate.envers.tools.hbm2ddl.EnversSchemaGenerator.Issue I worked on
see https://openmrs.atlassian.net/browse/TRUNK-6469
Checklist: I completed these to help reviewers :)
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master