Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/src/main/java/org/openmrs/ConceptReferenceRange.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public class ConceptReferenceRange extends BaseReferenceRange implements Openmrs
@GeneratedValue(strategy = GenerationType.IDENTITY)
private Integer conceptReferenceRangeId;

@Column(name = "criteria", length = 65535)
@Column(name = "criteria", length = 255)
Copy link
Contributor

@Binayak490-cyber Binayak490-cyber Mar 22, 2026

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.

private String criteria;

@ManyToOne(fetch = FetchType.LAZY)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer @Nonnull or at least the Spring version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Expand Down Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void setMappingResources(String @NonNull ... mappingResources) {
public void setMappingResources(@Nonnull String ... mappingResources) {

Copy link
Member Author

@wikumChamith wikumChamith Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ibacher Since this method uses varargs, we need to annotate it as String @NonNull ..., which means the array itself cannot be null.

If we annotate it as @NonNull String ..., that means each String element cannot be null.

When I used @NonNull String ..., I got the warning:
Not annotated parameter overrides @NonNullApi parameter.

That’s why I annotated it as String @NonNull ... instead.

Collections.addAll(this.mappingResources, mappingResources);

super.setMappingResources(this.mappingResources.toArray(new String[] {}));
Expand All @@ -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]));
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -221,6 +225,7 @@ public void destroy() throws HibernateException {
public void integrate(Metadata metadata, SessionFactoryImplementor sessionFactory,
SessionFactoryServiceRegistry serviceRegistry) {
this.metadata = metadata;
generateEnversAuditTables(metadata, serviceRegistry);
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand All @@ -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);
}
}
}
186 changes: 186 additions & 0 deletions api/src/main/java/org/openmrs/util/EnversAuditTableInitializer.java
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);
Copy link
Contributor

Choose a reason for hiding this comment

The 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")) {
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

};
}
};
}
}
1 change: 1 addition & 0 deletions api/src/main/resources/hibernate.default.properties
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,4 @@ hibernate.id.new_generator_mappings=false
# Hibernate envers options
hibernate.integration.envers.enabled=false
org.hibernate.envers.revision_listener=org.openmrs.api.db.hibernate.envers.OpenmrsRevisionEntityListener
org.hibernate.envers.audit_table_suffix=_audit
2 changes: 1 addition & 1 deletion api/src/test/java/org/openmrs/util/DatabaseIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public class DatabaseIT implements LiquibaseProvider {
protected static final String PASSWORD = "test";

@BeforeEach
public void setup() throws SQLException, ClassNotFoundException {
public void setup() throws Exception {
this.initializeDatabase();
}

Expand Down
Loading
Loading