From 6dc74c0285896cac3b6bc4a0aabde4746b316d7a Mon Sep 17 00:00:00 2001 From: Jason Pickering Date: Wed, 7 Oct 2026 06:55:34 +0200 Subject: [PATCH 1/2] fix: Do not load unloaded collections when building metadata audit entries [DHIS2-22235] (2.43) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit createAuditEntry treated an unloaded PersistentCollection like an entity proxy and read it, so every update of a data set or org unit group loaded all of its org units after commit just to list their UIDs in the audit entry. This breaks the method's documented rule 2 ("Do not include any lazy HibernateProxy or PersistentCollection that is not loaded"). A collection the request did not load is now left out; loaded collections are still written as before. On a copy of a production database, changing only the sharing of a data set or org unit group with ~138k org units went from 2.7-3.3 s and a ~1.9 MB audit entry to 0.03-0.16 s and ~1 KB. Backport of #25534. (cherry picked from commit 786822a87e4fd29e6bb56d5b0c62d0223b8cd176) 🤖 AI Assisted --- .../listener/AbstractHibernateListener.java | 9 +- .../listener/AuditHibernateListenerTest.java | 87 ++++++++++++++++++- 2 files changed, 94 insertions(+), 2 deletions(-) diff --git a/dhis-2/dhis-support/dhis-support-artemis/src/main/java/org/hisp/dhis/artemis/audit/listener/AbstractHibernateListener.java b/dhis-2/dhis-support/dhis-support-artemis/src/main/java/org/hisp/dhis/artemis/audit/listener/AbstractHibernateListener.java index e05daa102ec7..e011c431ba0c 100644 --- a/dhis-2/dhis-support/dhis-support-artemis/src/main/java/org/hisp/dhis/artemis/audit/listener/AbstractHibernateListener.java +++ b/dhis-2/dhis-support/dhis-support-artemis/src/main/java/org/hisp/dhis/artemis/audit/listener/AbstractHibernateListener.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-2022, University of Oslo + * Copyright (c) 2004-2026, University of Oslo * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -41,6 +41,7 @@ import java.util.Optional; import lombok.extern.slf4j.Slf4j; import org.hibernate.Hibernate; +import org.hibernate.collection.spi.PersistentCollection; import org.hibernate.event.spi.EventSource; import org.hibernate.event.spi.PostDeleteEvent; import org.hibernate.event.spi.PostInsertEvent; @@ -191,6 +192,12 @@ protected Object createAuditEntry( continue; } + // Rule 2: a collection the request did not load is left out, as loading it here would read + // every element from the database (hundreds of thousands for large org unit sets) + if (value instanceof PersistentCollection collection && !collection.wasInitialized()) { + continue; + } + if (shouldInitializeProxy(value) || property.isEmbeddedObject()) { if (entityProxy == null) { entityProxy = createProxy(id, session, persister); diff --git a/dhis-2/dhis-support/dhis-support-artemis/src/test/java/org/hisp/dhis/artemis/audit/listener/AuditHibernateListenerTest.java b/dhis-2/dhis-support/dhis-support-artemis/src/test/java/org/hisp/dhis/artemis/audit/listener/AuditHibernateListenerTest.java index cc1246f47bb8..20f978ff83e3 100644 --- a/dhis-2/dhis-support/dhis-support-artemis/src/test/java/org/hisp/dhis/artemis/audit/listener/AuditHibernateListenerTest.java +++ b/dhis-2/dhis-support/dhis-support-artemis/src/test/java/org/hisp/dhis/artemis/audit/listener/AuditHibernateListenerTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-2022, University of Oslo + * Copyright (c) 2004-2026, University of Oslo * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -30,18 +30,26 @@ package org.hisp.dhis.artemis.audit.listener; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.Set; +import org.hibernate.collection.internal.PersistentSet; import org.hibernate.event.spi.EventSource; import org.hibernate.persister.entity.EntityPersister; +import org.hibernate.proxy.HibernateProxy; import org.hisp.dhis.artemis.audit.AuditManager; import org.hisp.dhis.artemis.audit.configuration.AuditMatrix; import org.hisp.dhis.artemis.audit.legacy.AuditObjectFactory; import org.hisp.dhis.artemis.config.UsernameSupplier; +import org.hisp.dhis.organisationunit.OrganisationUnit; +import org.hisp.dhis.organisationunit.OrganisationUnitGroup; import org.hisp.dhis.schema.Property; import org.hisp.dhis.schema.Schema; import org.hisp.dhis.schema.SchemaService; @@ -95,6 +103,83 @@ void testSavePassword() { assertEquals("userA", auditObjectMap.get("userName").toString()); } + @Test + @SuppressWarnings("unchecked") + void testUnloadedCollectionIsNotLoadedOrIncluded() { + OrganisationUnitGroup group = new OrganisationUnitGroup("groupA"); + // A collection that was not loaded by the request. It has no session, so any read of it + // (size, isEmpty, iteration) throws LazyInitializationException + PersistentSet members = new PersistentSet(null); + + Object[] state = new Object[] {"groupA", members}; + EventSource session = mock(EventSource.class); + EntityPersister persister = mock(EntityPersister.class); + when(persister.getPropertyNames()).thenReturn(new String[] {"name", "members"}); + // Reading the property through an entity proxy returns the same unloaded collection + HibernateProxy proxy = mock(HibernateProxy.class); + lenient().when(persister.createProxy(1, session)).thenReturn(proxy); + lenient().when(persister.getPropertyValue(proxy, "members")).thenReturn(members); + + mockSchema( + OrganisationUnitGroup.class, + createProperty("name", true), + createCollectionProperty("members", OrganisationUnit.class)); + + Map auditObjectMap = + (Map) + postInsertAuditListener.createAuditEntry(group, state, session, 1, persister); + + assertFalse(members.wasInitialized()); + assertFalse(auditObjectMap.containsKey("members")); + assertEquals("groupA", auditObjectMap.get("name")); + } + + @Test + @SuppressWarnings("unchecked") + void testLoadedCollectionIsIncludedAsUids() { + OrganisationUnitGroup group = new OrganisationUnitGroup("groupA"); + OrganisationUnit unit = new OrganisationUnit("unitA"); + unit.setUid("ouabcdefghA"); + PersistentSet members = new PersistentSet(null, Set.of(unit)); + + Object[] state = new Object[] {"groupA", members}; + EventSource session = mock(EventSource.class); + EntityPersister persister = mock(EntityPersister.class); + when(persister.getPropertyNames()).thenReturn(new String[] {"name", "members"}); + + mockSchema( + OrganisationUnitGroup.class, + createProperty("name", true), + createCollectionProperty("members", OrganisationUnit.class)); + + Map auditObjectMap = + (Map) + postInsertAuditListener.createAuditEntry(group, state, session, 1, persister); + + assertEquals(List.of("ouabcdefghA"), auditObjectMap.get("members")); + } + + private void mockSchema(Class klass, Property... properties) { + Map map = new HashMap<>(); + for (Property property : properties) { + map.put(property.getFieldName(), property); + } + Schema schema = mock(Schema.class); + when(schema.getFieldNameMapProperties()).thenReturn(map); + when(schemaService.getSchema(klass)).thenReturn(schema); + } + + private Property createCollectionProperty(String name, Class itemKlass) { + Property property = new Property(Set.class); + property.setReadable(true); + property.setFieldName(name); + property.setOwner(true); + property.setEmbeddedObject(false); + property.setCollection(true); + property.setItemKlass(itemKlass); + return property; + } + private User createUser(int id, String userName, String password) { User user = new User(); user.setId(id); From b3fcead09dfd9c3ef9ea7ed23f4e7a6c3e9d9bba Mon Sep 17 00:00:00 2001 From: Jason Pickering Date: Wed, 7 Oct 2026 08:37:38 +0200 Subject: [PATCH 2/2] fix: Write loaded empty collections as [] in metadata audit entries [DHIS2-22235] (2.43) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback: with unloaded collections left out, a loaded collection that the request emptied was also missing from the entry (putValueToMap dropped empty collections), so "removed every org unit" and "changed only the sharing" produced the same entry. A loaded empty collection is now written as [], so a missing key only means "not loaded". This matches the delete path, which already writes empty collections. Also moves the unloaded-collection check into a helper to keep createAuditEntry within the cognitive complexity limit. (cherry picked from commit 8bb35d3c10a31ffd9e9ed04e79f349081ce17aaf) 🤖 AI Assisted --- .../listener/AbstractHibernateListener.java | 26 ++++++++--------- .../listener/AuditHibernateListenerTest.java | 28 +++++++++++++++++++ 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/dhis-2/dhis-support/dhis-support-artemis/src/main/java/org/hisp/dhis/artemis/audit/listener/AbstractHibernateListener.java b/dhis-2/dhis-support/dhis-support-artemis/src/main/java/org/hisp/dhis/artemis/audit/listener/AbstractHibernateListener.java index e011c431ba0c..241f04c25383 100644 --- a/dhis-2/dhis-support/dhis-support-artemis/src/main/java/org/hisp/dhis/artemis/audit/listener/AbstractHibernateListener.java +++ b/dhis-2/dhis-support/dhis-support-artemis/src/main/java/org/hisp/dhis/artemis/audit/listener/AbstractHibernateListener.java @@ -188,13 +188,7 @@ protected Object createAuditEntry( String pName = persister.getPropertyNames()[i]; Property property = properties.get(pName); - if (shouldIgnoreProperty(property)) { - continue; - } - - // Rule 2: a collection the request did not load is left out, as loading it here would read - // every element from the database (hundreds of thousands for large org unit sets) - if (value instanceof PersistentCollection collection && !collection.wasInitialized()) { + if (shouldIgnoreProperty(property) || isUnloadedCollection(value)) { continue; } @@ -261,17 +255,11 @@ private void putValueToMap(Property property, Map objectMap, Obj if (value == null) return; if (property.isCollection() && (value instanceof Collection collectionValue)) { - - if (collectionValue.isEmpty()) return; - + // a loaded empty collection is written as [], so a missing key only means "not loaded" Collection collection = (Collection) collectionValue; if (IdentifiableObject.class.isAssignableFrom(property.getItemKlass())) { - List uids = IdentifiableObjectUtils.getUids(collection); - - if (uids != null && !uids.isEmpty()) { - objectMap.put(property.getFieldName(), uids); - } + objectMap.put(property.getFieldName(), IdentifiableObjectUtils.getUids(collection)); } else { handleNonIdentifiableCollection(property, value, objectMap); } @@ -301,6 +289,14 @@ private boolean shouldInitializeProxy(Object value) { return true; } + /** + * Rule 2: a collection the request did not load is left out, as loading it here would read every + * element from the database (hundreds of thousands for large org unit sets). + */ + private static boolean isUnloadedCollection(Object value) { + return value instanceof PersistentCollection collection && !collection.wasInitialized(); + } + private Object getId(Object object) { if (object instanceof IdentifiableObject obj) return obj.getUid(); return object; diff --git a/dhis-2/dhis-support/dhis-support-artemis/src/test/java/org/hisp/dhis/artemis/audit/listener/AuditHibernateListenerTest.java b/dhis-2/dhis-support/dhis-support-artemis/src/test/java/org/hisp/dhis/artemis/audit/listener/AuditHibernateListenerTest.java index 20f978ff83e3..5c5d35fb8dcd 100644 --- a/dhis-2/dhis-support/dhis-support-artemis/src/test/java/org/hisp/dhis/artemis/audit/listener/AuditHibernateListenerTest.java +++ b/dhis-2/dhis-support/dhis-support-artemis/src/test/java/org/hisp/dhis/artemis/audit/listener/AuditHibernateListenerTest.java @@ -37,6 +37,7 @@ import static org.mockito.Mockito.when; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -159,6 +160,33 @@ void testLoadedCollectionIsIncludedAsUids() { assertEquals(List.of("ouabcdefghA"), auditObjectMap.get("members")); } + /** + * A collection that was loaded and is empty (e.g. every member removed) is written as an empty + * list, so that a missing key only ever means "not loaded". + */ + @Test + @SuppressWarnings("unchecked") + void testLoadedEmptyCollectionIsIncludedAsEmptyList() { + OrganisationUnitGroup group = new OrganisationUnitGroup("groupA"); + PersistentSet members = new PersistentSet(null, new HashSet<>()); + + Object[] state = new Object[] {"groupA", members}; + EventSource session = mock(EventSource.class); + EntityPersister persister = mock(EntityPersister.class); + when(persister.getPropertyNames()).thenReturn(new String[] {"name", "members"}); + + mockSchema( + OrganisationUnitGroup.class, + createProperty("name", true), + createCollectionProperty("members", OrganisationUnit.class)); + + Map auditObjectMap = + (Map) + postInsertAuditListener.createAuditEntry(group, state, session, 1, persister); + + assertEquals(List.of(), auditObjectMap.get("members")); + } + private void mockSchema(Class klass, Property... properties) { Map map = new HashMap<>(); for (Property property : properties) {