-
Notifications
You must be signed in to change notification settings - Fork 4.2k
TRUNK-6469: Add support for automatic initialization of Envers audit tables (#5468) (#5831) #5834
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.8.x
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -27,11 +27,15 @@ | |||||
| import org.hibernate.boot.Metadata; | ||||||
| import org.hibernate.engine.spi.SessionFactoryImplementor; | ||||||
| import org.hibernate.integrator.spi.Integrator; | ||||||
| import org.hibernate.service.ServiceRegistry; | ||||||
| import org.hibernate.service.spi.SessionFactoryServiceRegistry; | ||||||
| import org.jspecify.annotations.NonNull; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Prefer
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the one used by the parent class: spring-projects/spring-framework@2691489/spring-orm/src/main/java/org/springframework/orm/jpa/hibernate/LocalSessionFactoryBean.java#L39 |
||||||
| import org.openmrs.api.APIException; | ||||||
| import org.openmrs.api.cache.CacheConfig; | ||||||
| import org.openmrs.api.context.Context; | ||||||
| import org.openmrs.module.Module; | ||||||
| import org.openmrs.module.ModuleFactory; | ||||||
| import org.openmrs.util.EnversAuditTableInitializer; | ||||||
| import org.openmrs.util.OpenmrsUtil; | ||||||
| import org.slf4j.Logger; | ||||||
| import org.slf4j.LoggerFactory; | ||||||
|
|
@@ -75,7 +79,7 @@ public class HibernateSessionFactoryBean extends LocalSessionFactoryBean impleme | |||||
| * as 'private' instead of 'protected' | ||||||
| */ | ||||||
| @Override | ||||||
| public void setMappingResources(String... mappingResources) { | ||||||
| public void setMappingResources(String @NonNull ... mappingResources) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ibacher Since this method uses varargs, we need to annotate it as If we annotate it as When I used That’s why I annotated it as |
||||||
| Collections.addAll(this.mappingResources, mappingResources); | ||||||
|
|
||||||
| super.setMappingResources(this.mappingResources.toArray(new String[] {})); | ||||||
|
|
@@ -87,7 +91,7 @@ public void setMappingResources(String... mappingResources) { | |||||
| * It adds to the set instead of overwriting it with each call. | ||||||
| */ | ||||||
| @Override | ||||||
| public void setPackagesToScan(String... packagesToScan) { | ||||||
| public void setPackagesToScan(String @NonNull ... packagesToScan) { | ||||||
| this.packagesToScan.addAll(Arrays.asList(packagesToScan)); | ||||||
|
|
||||||
| super.setPackagesToScan(this.packagesToScan.toArray(new String[0])); | ||||||
|
|
@@ -129,7 +133,7 @@ public void afterPropertiesSet() throws IOException { | |||||
| Object key = entry.getKey(); | ||||||
| String prop = (String) key; | ||||||
| String value = (String) entry.getValue(); | ||||||
| log.trace("Setting module property: " + prop + ":" + value); | ||||||
| log.trace("Setting module property: {}:{}", prop, value); | ||||||
| config.setProperty(prop, value); | ||||||
| if (!prop.startsWith("hibernate")) { | ||||||
| config.setProperty("hibernate." + prop, value); | ||||||
|
|
@@ -143,7 +147,7 @@ public void afterPropertiesSet() throws IOException { | |||||
| Object key = entry.getKey(); | ||||||
| String prop = (String) key; | ||||||
| String value = (String) entry.getValue(); | ||||||
| log.trace("Setting property: " + prop + ":" + value); | ||||||
| log.trace("Setting property: {}:{}", prop, value); | ||||||
| config.setProperty(prop, value); | ||||||
| if (!prop.startsWith("hibernate")) { | ||||||
| config.setProperty("hibernate." + prop, value); | ||||||
|
|
@@ -186,8 +190,8 @@ public void afterPropertiesSet() throws IOException { | |||||
| value = value.replace("%APPLICATION_DATA_DIRECTORY%", applicationDataDirectory); | ||||||
| entry.setValue(value); | ||||||
| } | ||||||
| log.debug("Setting global Hibernate Session Interceptor for SessionFactory, Interceptor: " + chainingInterceptor); | ||||||
|
|
||||||
| log.debug("Setting global Hibernate Session Interceptor for SessionFactory, Interceptor: {}", chainingInterceptor); | ||||||
|
|
||||||
| // make sure all autowired interceptors are put onto our chaining interceptor | ||||||
| // sort on the keys so that the devs/modules have some sort of control over the order of the interceptors | ||||||
|
|
@@ -221,6 +225,7 @@ public void destroy() throws HibernateException { | |||||
| public void integrate(Metadata metadata, SessionFactoryImplementor sessionFactory, | ||||||
| SessionFactoryServiceRegistry serviceRegistry) { | ||||||
| this.metadata = metadata; | ||||||
| generateEnversAuditTables(metadata, serviceRegistry); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Throwing APIException from inside integrate() will crash the entire SessionFactory initialization with a generic error — giving no clear signal that Envers is the cause. Should log the failure clearly and consider making it non-fatal so startup can proceed. |
||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
|
|
@@ -234,4 +239,13 @@ public void disintegrate(SessionFactoryImplementor sessionFactory, SessionFactor | |||||
| public Metadata getMetadata() { | ||||||
| return metadata; | ||||||
| } | ||||||
|
|
||||||
| private void generateEnversAuditTables(Metadata metadata, ServiceRegistry serviceRegistry) { | ||||||
| try { | ||||||
| Properties hibernateProperties = getHibernateProperties(); | ||||||
| EnversAuditTableInitializer.initialize(metadata, hibernateProperties, serviceRegistry); | ||||||
| } catch (Exception e) { | ||||||
| throw new APIException("An error occurred while initializing the Envers audit tables", e); | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,186 @@ | ||
| /** | ||
| * This Source Code Form is subject to the terms of the Mozilla Public License, | ||
| * v. 2.0. If a copy of the MPL was not distributed with this file, You can | ||
| * obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under | ||
| * the terms of the Healthcare Disclaimer located at http://openmrs.org/license. | ||
| * | ||
| * Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS | ||
| * graphic logo is a trademark of OpenMRS Inc. | ||
| */ | ||
| package org.openmrs.util; | ||
|
|
||
| import java.util.EnumSet; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
| import java.util.Properties; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
|
|
||
| import org.hibernate.boot.Metadata; | ||
| import org.hibernate.boot.model.relational.Namespace; | ||
| import org.hibernate.boot.model.relational.Sequence; | ||
| import org.hibernate.mapping.Table; | ||
| import org.hibernate.service.ServiceRegistry; | ||
| import org.hibernate.tool.schema.TargetType; | ||
| import org.hibernate.tool.schema.spi.ExceptionHandler; | ||
| import org.hibernate.tool.schema.spi.ExecutionOptions; | ||
| import org.hibernate.tool.schema.spi.SchemaFilter; | ||
| import org.hibernate.tool.schema.spi.SchemaFilterProvider; | ||
| import org.hibernate.tool.schema.spi.SchemaManagementTool; | ||
| import org.hibernate.tool.schema.spi.SchemaMigrator; | ||
| import org.hibernate.tool.schema.spi.ScriptTargetOutput; | ||
| import org.hibernate.tool.schema.spi.TargetDescriptor; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| /** | ||
| * Initializes Hibernate Envers audit tables when auditing is enabled. This class is responsible for | ||
| * conditionally creating audit tables only when hibernate.integration.envers.enabled=true. | ||
| */ | ||
| public class EnversAuditTableInitializer { | ||
|
|
||
| private static final Logger log = LoggerFactory.getLogger(EnversAuditTableInitializer.class); | ||
|
|
||
| private EnversAuditTableInitializer() { | ||
|
|
||
| } | ||
|
|
||
| /** | ||
| * Checks if Envers is enabled and creates/updates audit tables as needed. This will Create or | ||
| * Update audit tables if they don't exist - Update existing audit tables if the schema has | ||
| * changed | ||
| * | ||
| * @param metadata Hibernate metadata containing entity mappings | ||
| * @param hibernateProperties properties containing Envers configuration | ||
| * @param serviceRegistry Hibernate service registry | ||
| */ | ||
| public static void initialize(Metadata metadata, Properties hibernateProperties, | ||
| ServiceRegistry serviceRegistry) { | ||
|
|
||
| if (!isEnversEnabled(hibernateProperties)) { | ||
| log.debug("Hibernate Envers is not enabled. Skipping audit table initialization."); | ||
| return; | ||
| } | ||
|
|
||
| updateAuditTables(metadata, hibernateProperties, serviceRegistry); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if Hibernate Envers is enabled in the configuration. | ||
| * | ||
| * @param properties Hibernate properties | ||
| * @return true if Envers is enabled, false otherwise | ||
| */ | ||
| private static boolean isEnversEnabled(Properties properties) { | ||
| String enversEnabled = properties.getProperty("hibernate.integration.envers.enabled"); | ||
| return "true".equalsIgnoreCase(enversEnabled); | ||
| } | ||
|
|
||
| /** | ||
| * Creates or updates audit tables using Hibernate's {@link SchemaMigrator}. This method filters | ||
| * to only process audit tables. | ||
| * | ||
| * @param metadata Hibernate metadata containing entity mappings (includes Envers audit | ||
| * entities) | ||
| * @param hibernateProperties Hibernate configuration properties | ||
| * @param serviceRegistry Hibernate service registry | ||
| */ | ||
| private static void updateAuditTables(Metadata metadata, Properties hibernateProperties, | ||
| ServiceRegistry serviceRegistry) { | ||
| String auditTablePrefix = hibernateProperties.getProperty("org.hibernate.envers.audit_table_prefix", ""); | ||
| String auditTableSuffix = hibernateProperties.getProperty("org.hibernate.envers.audit_table_suffix", "_audit"); | ||
|
|
||
| Map<String, Object> settings = new HashMap<>((Map) hibernateProperties); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an unchecked raw type cast that will generate a compiler warning and could throw a ClassCastException at runtime if any property value is not a String. Should use explicit population instead. |
||
| settings.put("hibernate.hbm2ddl.schema_filter_provider", buildSchemaFilterProvider(auditTablePrefix, auditTableSuffix)); | ||
|
|
||
| AtomicBoolean hasErrors = new AtomicBoolean(false); | ||
| ExecutionOptions executionOptions = getExecutionOptions(settings, hasErrors); | ||
| SchemaMigrator schemaMigrator = serviceRegistry.getService(SchemaManagementTool.class).getSchemaMigrator(settings); | ||
|
|
||
| schemaMigrator.doMigration(metadata, executionOptions, getTargetDescriptor()); | ||
|
|
||
| if (hasErrors.get()) { | ||
| log.warn("Envers audit table migration completed with errors."); | ||
| } else { | ||
| log.info("Successfully created/updated Envers audit tables using Hibernate SchemaManagementTool."); | ||
| } | ||
| } | ||
|
|
||
| private static SchemaFilterProvider buildSchemaFilterProvider(String auditTablePrefix, String auditTableSuffix) { | ||
| String lowerPrefix = auditTablePrefix.toLowerCase(); | ||
| String lowerSuffix = auditTableSuffix.toLowerCase(); | ||
|
|
||
| SchemaFilter auditFilter = new SchemaFilter() { | ||
| @Override | ||
| public boolean includeNamespace(Namespace namespace) { | ||
| return true; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean includeTable(Table table) { | ||
| String tableName = table.getName(); | ||
| if (tableName == null) { | ||
| return false; | ||
| } | ||
|
|
||
| String lowerTableName = tableName.toLowerCase(); | ||
|
|
||
| if (lowerTableName.contains("revision") || lowerTableName.equals("revinfo")) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wikumChamith, Any table whose name happens to contain the word "revision" would be incorrectly pulled into the audit schema migration. This is too broad — should match only the exact Envers-generated revision table name based on the configured prefix/suffix. |
||
| return true; | ||
| } | ||
|
|
||
| boolean hasPrefix = lowerPrefix.isEmpty() || lowerTableName.startsWith(lowerPrefix); | ||
| boolean hasSuffix = lowerSuffix.isEmpty() || lowerTableName.endsWith(lowerSuffix); | ||
|
|
||
| return hasPrefix && hasSuffix; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean includeSequence(Sequence sequence) { | ||
| return false; | ||
| } | ||
| }; | ||
|
|
||
| return new SchemaFilterProvider() { | ||
| @Override public SchemaFilter getCreateFilter() { return auditFilter; } | ||
| @Override public SchemaFilter getDropFilter() { return auditFilter; } | ||
| @Override public SchemaFilter getMigrateFilter() { return auditFilter; } | ||
| @Override public SchemaFilter getValidateFilter() { return auditFilter; } | ||
| }; | ||
| } | ||
|
|
||
| private static TargetDescriptor getTargetDescriptor() { | ||
| return new TargetDescriptor() { | ||
| @Override | ||
| public EnumSet<TargetType> getTargetTypes() { | ||
| return EnumSet.of(TargetType.DATABASE); | ||
| } | ||
|
|
||
| @Override | ||
| public ScriptTargetOutput getScriptTargetOutput() { | ||
| return null; | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| private static ExecutionOptions getExecutionOptions(Map<String, Object> settings, AtomicBoolean hasErrors) { | ||
| return new ExecutionOptions() { | ||
| @Override | ||
| public Map<String, Object> getConfigurationValues() { | ||
| return settings; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean shouldManageNamespaces() { | ||
| return false; | ||
| } | ||
|
|
||
| @Override | ||
| public ExceptionHandler getExceptionHandler() { | ||
| return throwable -> { | ||
| hasErrors.set(true); | ||
| log.warn("Schema migration encountered an issue: {}", throwable.getMessage()); | ||
|
Comment on lines
+180
to
+181
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Errors during audit table creation are only logged as warnings and execution continues. If a critical table fails to create, the system starts up silently broken — this should either throw or at minimum log at ERROR level so it doesn't go unnoticed in production. |
||
| }; | ||
| } | ||
| }; | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wikumChamith, as I was also working on the same issue around and they are #5933 and #5952 and was finding this PR only where the backfill changeset only makes sense if the audit tables already exist. So, I want to point out some points that the criteria column was silently reduced from 65535 to 255 with no Liquibase migration — any existing values longer than 255 chars would be truncated on update. Was this intentional? A migration changeset is needed here.