From 39195a099008b26d752785b9897c1900b3ad0da6 Mon Sep 17 00:00:00 2001 From: dkayiwa Date: Tue, 3 Mar 2026 15:18:13 +0300 Subject: [PATCH 01/12] O3-5459: Void/purge queue entries when associated visit is voided or purged Add VisitWithQueueEntriesDeleteAdvice AOP advisor that intercepts VisitService.voidVisit() and VisitService.purgeVisit() to ensure associated queue entries are voided or purged accordingly. The existing VoidHandler in VisitWithQueueEntriesSaveHandler does not reliably fire when visitService.voidVisit() is called, so the AOP advice provides a reliable mechanism for both void and purge paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../VisitWithQueueEntriesDeleteAdvice.java | 73 +++++++++++++++++ ...VisitWithQueueEntriesDeleteAdviceTest.java | 80 +++++++++++++++++++ .../VisitWithQueueEntriesSaveHandlerTest.java | 19 +++++ omod/src/main/resources/config.xml | 5 ++ 4 files changed, 177 insertions(+) create mode 100644 api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java create mode 100644 integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java diff --git a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java new file mode 100644 index 00000000..670f35d6 --- /dev/null +++ b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java @@ -0,0 +1,73 @@ +/* + * 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.module.queue.api; + +import java.lang.reflect.Method; +import java.util.List; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.openmrs.Visit; +import org.openmrs.api.context.Context; +import org.openmrs.module.queue.api.search.QueueEntrySearchCriteria; +import org.openmrs.module.queue.model.QueueEntry; +import org.springframework.aop.MethodBeforeAdvice; + +/** + * AOP advice that intercepts {@link org.openmrs.api.VisitService} methods to manage associated + * queue entries when a visit is voided or purged. For voiding, this ensures queue entries are + * voided alongside the visit. For purging, this removes queue entries before the visit is + * permanently deleted to prevent orphaned entries and foreign key constraint violations. + */ +public class VisitWithQueueEntriesDeleteAdvice implements MethodBeforeAdvice { + + private final Log log = LogFactory.getLog(getClass()); + + @Override + public void before(Method method, Object[] args, Object target) throws Throwable { + if (args.length == 0 || !(args[0] instanceof Visit)) { + return; + } + Visit visit = (Visit) args[0]; + if (visit.getVisitId() == null) { + return; + } + + if ("voidVisit".equals(method.getName())) { + String voidReason = args.length > 1 && args[1] instanceof String ? (String) args[1] : null; + QueueEntryService queueEntryService = Context.getService(QueueEntryService.class); + QueueEntrySearchCriteria criteria = new QueueEntrySearchCriteria(); + criteria.setVisit(visit); + List queueEntries = queueEntryService.getQueueEntries(criteria); + if (!queueEntries.isEmpty()) { + log.debug("Voiding " + queueEntries.size() + " queue entries associated with voided visit"); + } + for (QueueEntry qe : queueEntries) { + if (!qe.getVoided()) { + queueEntryService.voidQueueEntry(qe, voidReason); + log.trace("Voided queue entry " + qe); + } + } + } else if ("purgeVisit".equals(method.getName())) { + QueueEntryService queueEntryService = Context.getService(QueueEntryService.class); + QueueEntrySearchCriteria criteria = new QueueEntrySearchCriteria(); + criteria.setVisit(visit); + criteria.setIncludedVoided(true); + List queueEntries = queueEntryService.getQueueEntries(criteria); + if (!queueEntries.isEmpty()) { + log.debug("Purging " + queueEntries.size() + " queue entries associated with purged visit"); + } + for (QueueEntry qe : queueEntries) { + queueEntryService.purgeQueueEntry(qe); + log.trace("Purged queue entry " + qe); + } + } + } +} diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java new file mode 100644 index 00000000..8689f74a --- /dev/null +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java @@ -0,0 +1,80 @@ +/* + * 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.module.queue.api; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.util.Arrays; +import java.util.List; + +import org.junit.Before; +import org.junit.Test; +import org.openmrs.Visit; +import org.openmrs.api.VisitService; +import org.openmrs.module.queue.SpringTestConfiguration; +import org.openmrs.module.queue.api.search.QueueEntrySearchCriteria; +import org.openmrs.module.queue.model.QueueEntry; +import org.openmrs.test.BaseModuleContextSensitiveTest; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.test.context.ContextConfiguration; + +@ContextConfiguration(classes = SpringTestConfiguration.class, inheritLocations = false) +public class VisitWithQueueEntriesDeleteAdviceTest extends BaseModuleContextSensitiveTest { + + private static final List INITIAL_DATASET_XML = Arrays.asList( + "org/openmrs/module/queue/api/dao/QueueDaoTest_locationInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_conceptsInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_patientInitialDataset.xml", + "org/openmrs/module/queue/api/dao/VisitQueueEntryDaoTest_visitInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueDaoTest_initialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_initialDataset.xml", + "org/openmrs/module/queue/validators/QueueEntryValidatorTest_globalPropertyInitialDataset.xml"); + + private Visit visit; + + private QueueEntry queueEntry; + + @Autowired + @Qualifier("queue.QueueEntryService") + private QueueEntryService queueEntryService; + + @Autowired + private VisitService visitService; + + @Before + public void setup() { + INITIAL_DATASET_XML.forEach(this::executeDataSet); + queueEntry = queueEntryService.getQueueEntryById(3).get(); + visit = queueEntry.getVisit(); + } + + @Test + public void shouldPurgeQueueEntriesWhenVisitIsPurged() throws Throwable { + assertFalse(queueEntry.getVoided()); + int queueEntryId = queueEntry.getId(); + + // Simulate what the AOP advice does before purgeVisit + VisitWithQueueEntriesDeleteAdvice advice = new VisitWithQueueEntriesDeleteAdvice(); + java.lang.reflect.Method purgeMethod = VisitService.class.getMethod("purgeVisit", Visit.class); + advice.before(purgeMethod, new Object[] { visit }, visitService); + + // Verify the queue entry was purged + QueueEntrySearchCriteria criteria = new QueueEntrySearchCriteria(); + criteria.setVisit(visit); + criteria.setIncludedVoided(true); + List remainingEntries = queueEntryService.getQueueEntries(criteria); + assertTrue("Queue entries should be purged before visit purge", remainingEntries.isEmpty()); + + // Now the visit can be purged without FK constraint violation + assertFalse(queueEntryService.getQueueEntryById(queueEntryId).isPresent()); + } +} diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java index b2c24ad1..2ec2bc41 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java @@ -114,4 +114,23 @@ public void shouldVoidQueueEntriesIfVisitIsVoided() { assertThat(queueEntry.getDateVoided(), equalTo(visit.getDateVoided())); assertThat(queueEntry.getVoidedBy(), equalTo(visit.getVoidedBy())); } + + @Test + public void shouldVoidQueueEntriesWhenVisitIsVoidedViaVoidVisitMethod() throws Throwable { + assertFalse(visit.getVoided()); + assertFalse(queueEntry.getVoided()); + String voidReason = "visit deleted"; + + // Simulate the AOP advice that fires before voidVisit + VisitWithQueueEntriesDeleteAdvice advice = new VisitWithQueueEntriesDeleteAdvice(); + java.lang.reflect.Method voidMethod = VisitService.class.getMethod("voidVisit", Visit.class, String.class); + advice.before(voidMethod, new Object[] { visit, voidReason }, visitService); + + visit = visitService.voidVisit(visit, voidReason); + queueEntry = queueEntryService.getQueueEntryById(queueEntry.getId()).get(); + assertTrue(visit.getVoided()); + assertThat(visit.getVoidReason(), equalTo(voidReason)); + assertTrue(queueEntry.getVoided()); + assertThat(queueEntry.getVoidReason(), equalTo(voidReason)); + } } diff --git a/omod/src/main/resources/config.xml b/omod/src/main/resources/config.xml index 7541fc1a..6b0efa9b 100644 --- a/omod/src/main/resources/config.xml +++ b/omod/src/main/resources/config.xml @@ -33,6 +33,11 @@ org.openmrs.module.legacyui + + org.openmrs.api.VisitService + org.openmrs.module.queue.api.VisitWithQueueEntriesDeleteAdvice + + ${project.parent.artifactId}.statusConceptSetName Status From 3fc1731f420c3ee228ef2625c91417cbf277c209 Mon Sep 17 00:00:00 2001 From: dkayiwa Date: Tue, 3 Mar 2026 15:24:08 +0300 Subject: [PATCH 02/12] Use SLF4J logging instead of Commons Logging Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../module/queue/api/VisitWithQueueEntriesDeleteAdvice.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java index 670f35d6..1c103ebf 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java +++ b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java @@ -12,12 +12,12 @@ import java.lang.reflect.Method; import java.util.List; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; import org.openmrs.Visit; import org.openmrs.api.context.Context; import org.openmrs.module.queue.api.search.QueueEntrySearchCriteria; import org.openmrs.module.queue.model.QueueEntry; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.aop.MethodBeforeAdvice; /** @@ -28,7 +28,7 @@ */ public class VisitWithQueueEntriesDeleteAdvice implements MethodBeforeAdvice { - private final Log log = LogFactory.getLog(getClass()); + private static final Logger log = LoggerFactory.getLogger(VisitWithQueueEntriesDeleteAdvice.class); @Override public void before(Method method, Object[] args, Object target) throws Throwable { From 36b2040b2bbd57b99b4d0aa024f247b95b5f4fc6 Mon Sep 17 00:00:00 2001 From: dkayiwa Date: Tue, 3 Mar 2026 15:41:36 +0300 Subject: [PATCH 03/12] Improve test coverage for void and purge advice - Purge test now actually calls visitService.purgeVisit() to verify the FK constraint issue is resolved end-to-end - Void test now isolates the advice behavior, proving the advice (not the VoidHandler) is what voids the queue entries Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../VisitWithQueueEntriesDeleteAdviceTest.java | 15 ++++++--------- .../api/VisitWithQueueEntriesSaveHandlerTest.java | 6 ++---- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java index 8689f74a..a41fd0a4 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java @@ -10,7 +10,7 @@ package org.openmrs.module.queue.api; import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertNull; import java.util.Arrays; import java.util.List; @@ -20,7 +20,6 @@ import org.openmrs.Visit; import org.openmrs.api.VisitService; import org.openmrs.module.queue.SpringTestConfiguration; -import org.openmrs.module.queue.api.search.QueueEntrySearchCriteria; import org.openmrs.module.queue.model.QueueEntry; import org.openmrs.test.BaseModuleContextSensitiveTest; import org.springframework.beans.factory.annotation.Autowired; @@ -60,6 +59,7 @@ public void setup() { @Test public void shouldPurgeQueueEntriesWhenVisitIsPurged() throws Throwable { assertFalse(queueEntry.getVoided()); + int visitId = visit.getVisitId(); int queueEntryId = queueEntry.getId(); // Simulate what the AOP advice does before purgeVisit @@ -68,13 +68,10 @@ public void shouldPurgeQueueEntriesWhenVisitIsPurged() throws Throwable { advice.before(purgeMethod, new Object[] { visit }, visitService); // Verify the queue entry was purged - QueueEntrySearchCriteria criteria = new QueueEntrySearchCriteria(); - criteria.setVisit(visit); - criteria.setIncludedVoided(true); - List remainingEntries = queueEntryService.getQueueEntries(criteria); - assertTrue("Queue entries should be purged before visit purge", remainingEntries.isEmpty()); - - // Now the visit can be purged without FK constraint violation assertFalse(queueEntryService.getQueueEntryById(queueEntryId).isPresent()); + + // Verify the visit can now be purged without FK constraint violation + visitService.purgeVisit(visit); + assertNull(visitService.getVisit(visitId)); } } diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java index 2ec2bc41..15e8aa97 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java @@ -121,15 +121,13 @@ public void shouldVoidQueueEntriesWhenVisitIsVoidedViaVoidVisitMethod() throws T assertFalse(queueEntry.getVoided()); String voidReason = "visit deleted"; - // Simulate the AOP advice that fires before voidVisit + // The AOP advice fires before voidVisit in production (registered via config.xml). + // In the test context, config.xml advice is not loaded, so we invoke it directly. VisitWithQueueEntriesDeleteAdvice advice = new VisitWithQueueEntriesDeleteAdvice(); java.lang.reflect.Method voidMethod = VisitService.class.getMethod("voidVisit", Visit.class, String.class); advice.before(voidMethod, new Object[] { visit, voidReason }, visitService); - visit = visitService.voidVisit(visit, voidReason); queueEntry = queueEntryService.getQueueEntryById(queueEntry.getId()).get(); - assertTrue(visit.getVoided()); - assertThat(visit.getVoidReason(), equalTo(voidReason)); assertTrue(queueEntry.getVoided()); assertThat(queueEntry.getVoidReason(), equalTo(voidReason)); } From 1c3a5e417cf4f78149fc2c719e8dbabc45199fe8 Mon Sep 17 00:00:00 2001 From: dkayiwa Date: Wed, 16 Sep 2026 14:24:43 +0300 Subject: [PATCH 04/12] O3-5459: Proxy the privilege purgeQueueEntry actually requires The advice granted Get/Manage Queue Entries, but QueueEntryService.purgeQueueEntry is annotated @Authorized(PURGE_QUEUE_ENTRIES), so a caller holding Purge Visits but no queue privileges hit APIAuthenticationException out of the before-advice and the visit was not purged. Grant Purge Queue Entries instead; Manage was never used on this path, which only calls getQueueEntries and purgeQueueEntry. Cover it with a test modelled on VisitWithQueueEntriesSaveHandlerTest#shouldVoidQueueEntriesForUserWithoutQueuePrivileges; both existing advice tests ran as the superuser, which is why this got through. Also qualify the class javadoc: VisitWithQueueEntriesSaveHandler only voids entries on the voidVisit path when it runs after core's BaseVoidHandler, and nothing pins that order, since both handlers carry the default @Handler order and the tie falls to the iteration order of the map ServiceContext.getRegisteredComponents builds. Co-Authored-By: Claude Opus 5 (1M context) --- .../VisitWithQueueEntriesDeleteAdvice.java | 15 ++++++++----- ...VisitWithQueueEntriesDeleteAdviceTest.java | 22 ++++++++++++++++++- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java index 476850ab..b477733f 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java +++ b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java @@ -27,10 +27,13 @@ * fails on the queue_entry foreign key to visit. *

* Purging cannot be done from a handler: core runs save/void handlers through RequiredDataAdvice, - * which is not consulted on a purge, so advice on the service is the only hook. Voiding needs no - * advice - {@link VisitWithQueueEntriesSaveHandler} already voids the entries on both the - * {@code saveVisit} and {@code voidVisit} paths, and stamps them with the visit's own void date and - * user so an unvoid can tell them apart from entries voided on their own. + * which is not consulted on a purge, so advice on the service is the only hook. Voiding is left to + * {@link VisitWithQueueEntriesSaveHandler}, which voids the entries on the {@code saveVisit} path, + * and on the {@code voidVisit} path whenever it runs after core's {@code BaseVoidHandler}. Nothing + * pins that order: both handlers carry the default {@code @Handler} order, and the tie is broken by + * the iteration order of the map {@code ServiceContext.getRegisteredComponents} builds. The handler + * stamps the entries with the visit's own void date and user so an unvoid can tell them apart from + * entries voided on their own. */ public class VisitWithQueueEntriesDeleteAdvice implements MethodBeforeAdvice { @@ -49,7 +52,7 @@ public void before(Method method, Object[] args, Object target) { // Purging a visit is driven by a core service whose callers need not hold queue privileges, // so grant them for the duration of this cascade, as the queue handlers do Context.addProxyPrivilege(PrivilegeConstants.GET_QUEUE_ENTRIES); - Context.addProxyPrivilege(PrivilegeConstants.MANAGE_QUEUE_ENTRIES); + Context.addProxyPrivilege(PrivilegeConstants.PURGE_QUEUE_ENTRIES); try { QueueEntryService queueEntryService = Context.getService(QueueEntryService.class); QueueEntrySearchCriteria criteria = new QueueEntrySearchCriteria(); @@ -67,7 +70,7 @@ public void before(Method method, Object[] args, Object target) { } finally { Context.removeProxyPrivilege(PrivilegeConstants.GET_QUEUE_ENTRIES); - Context.removeProxyPrivilege(PrivilegeConstants.MANAGE_QUEUE_ENTRIES); + Context.removeProxyPrivilege(PrivilegeConstants.PURGE_QUEUE_ENTRIES); } } } diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java index a8b62d4c..01309bd1 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java @@ -23,7 +23,6 @@ import org.openmrs.api.VisitService; import org.openmrs.api.context.Context; import org.openmrs.module.queue.SpringTestConfiguration; -import org.openmrs.module.queue.model.QueueEntry; import org.openmrs.test.BaseModuleContextSensitiveTest; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Qualifier; @@ -78,6 +77,27 @@ public void shouldPurgeQueueEntriesWhenVisitIsPurged() { assertFalse(queueEntryService.getQueueEntryById(3).isPresent()); } + @Test + public void shouldPurgeQueueEntriesForUserWithoutQueuePrivileges() { + // "butch" from the standard test dataset has the Provider role and no privileges; proxy only the + // core visit privileges so the advice has to obtain the queue privileges itself + Integer visitId = visit.getVisitId(); + Context.becomeUser("3-4"); + Context.addProxyPrivilege(org.openmrs.util.PrivilegeConstants.PURGE_VISITS); + Context.addProxyPrivilege(org.openmrs.util.PrivilegeConstants.GET_ENCOUNTERS); + try { + assertFalse(Context.hasPrivilege(org.openmrs.module.queue.utils.PrivilegeConstants.PURGE_QUEUE_ENTRIES)); + visitService.purgeVisit(visit); + } + finally { + Context.removeProxyPrivilege(org.openmrs.util.PrivilegeConstants.PURGE_VISITS); + Context.removeProxyPrivilege(org.openmrs.util.PrivilegeConstants.GET_ENCOUNTERS); + authenticate(); + } + assertNull(visitService.getVisit(visitId)); + assertFalse(queueEntryService.getQueueEntryById(3).isPresent()); + } + @Test public void shouldPurgeVoidedQueueEntriesOfAPurgedVisit() { // entry 10 on visit 101 is voided, and a voided entry is just as much of a foreign key to the visit From 95ed7457a5d84a6320ebbc5de6b0bea5df5fb454 Mon Sep 17 00:00:00 2001 From: dkayiwa Date: Wed, 16 Sep 2026 15:14:31 +0300 Subject: [PATCH 05/12] O3-5459: Flush in the purge tests so they reach the foreign key The three tests asserted through getQueueEntryById and getVisit, both of which read the Hibernate session, so nothing forced a flush and the DELETE FROM visit that queue_entry_visit_id_fk rejects never reached the database. They pinned that the advice removed the entries, not that the visit row can now be deleted, which is the failure this PR fixes. With the advice unregistered they now fail on queue_entry_visit_id_fk instead of on the entry-present assertion. Also correct the comment above setIncludedVoided(true). A voided visit does not hide its entries: the default search filters on qe.voided and, through the patient alias, on p.voided, while the visit appears only as an equality on qe.visit with no join to the visit table. Co-Authored-By: Claude Opus 5 (1M context) --- .../module/queue/api/VisitWithQueueEntriesDeleteAdvice.java | 2 +- .../queue/api/VisitWithQueueEntriesDeleteAdviceTest.java | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java index b477733f..39f8b26f 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java +++ b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java @@ -57,7 +57,7 @@ public void before(Method method, Object[] args, Object target) { QueueEntryService queueEntryService = Context.getService(QueueEntryService.class); QueueEntrySearchCriteria criteria = new QueueEntrySearchCriteria(); criteria.setVisit(visit); - // the visit or its patient may already be voided, which would hide the entries from the default search + // voided entries hold the same foreign key, and a voided patient hides them from the default search criteria.setIncludedVoided(true); List queueEntries = queueEntryService.getQueueEntries(criteria); if (!queueEntries.isEmpty()) { diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java index 01309bd1..418a2483 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java @@ -72,6 +72,7 @@ public void shouldPurgeQueueEntriesWhenVisitIsPurged() { assertTrue(queueEntryService.getQueueEntryById(3).isPresent()); visitService.purgeVisit(visit); + Context.flushSession(); assertNull(visitService.getVisit(visitId)); assertFalse(queueEntryService.getQueueEntryById(3).isPresent()); @@ -88,6 +89,7 @@ public void shouldPurgeQueueEntriesForUserWithoutQueuePrivileges() { try { assertFalse(Context.hasPrivilege(org.openmrs.module.queue.utils.PrivilegeConstants.PURGE_QUEUE_ENTRIES)); visitService.purgeVisit(visit); + Context.flushSession(); } finally { Context.removeProxyPrivilege(org.openmrs.util.PrivilegeConstants.PURGE_VISITS); @@ -105,6 +107,7 @@ public void shouldPurgeVoidedQueueEntriesOfAPurgedVisit() { assertTrue(queueEntryService.getQueueEntryById(10).isPresent()); visitService.purgeVisit(visitWithVoidedEntry); + Context.flushSession(); assertNull(visitService.getVisit(101)); assertFalse(queueEntryService.getQueueEntryById(10).isPresent()); From 77ff35b0586fab7d95af3a4c68c21d98c8f390fb Mon Sep 17 00:00:00 2001 From: dkayiwa Date: Wed, 16 Sep 2026 16:32:15 +0300 Subject: [PATCH 06/12] O3-5459: Void queue entries from a handler that ignores handler order The symptom this ticket reports arrives on the voidVisit path: the chart's "delete visit" calls VisitResource1_9.delete, which voids the visit. That half was left to VisitWithQueueEntriesSaveHandler, whose void branch is guarded by visit.getVoided() and so only fires when core's BaseVoidHandler happens to run first. Nothing pins that. Both carry the default @Handler order, HandlerUtil sorts stably, and the tie falls to the iteration order of the HashMap that ServiceContext.getRegisteredComponents builds over bean names, which moves with the number of VoidHandler beans a deployment registers. Driving the real voidVisit with core's five handlers plus the module's one, the shape queue 3.0.0 ships, the queue handler goes first and the entry is left active. Move the cascade to VisitWithQueueEntriesVoidHandler, which takes the void user, date and reason from the handler arguments instead of off the visit, so the order stops mattering. RequiredDataAdvice hands every handler the same values it gives BaseVoidHandler, so the entries still carry the visit's own void stamp. This is the shape PatientWithQueueEntriesVoidHandler already uses, and core's VisitVoidHandler with it. Retire the save handler's void branch so the cascade has one owner; that handler now only ends open entries when a visit is stopped. Purging stays with VisitWithQueueEntriesDeleteAdvice, which core cannot reach from a handler at all. shouldVoidQueueEntriesBeforeTheVisitItselfIsFlaggedVoided pins the property the old code lacked: it fails if the getVoided() guard comes back, while the tests that go through voidVisit stay green either way, since the module's own test context lands on the favourable order. Co-Authored-By: Claude Opus 5 (1M context) --- .../PatientWithQueueEntriesUnvoidHandler.java | 2 +- .../VisitWithQueueEntriesDeleteAdvice.java | 9 +- .../api/VisitWithQueueEntriesSaveHandler.java | 27 +-- .../api/VisitWithQueueEntriesVoidHandler.java | 94 ++++++++++ .../VisitWithQueueEntriesSaveHandlerTest.java | 78 -------- .../VisitWithQueueEntriesVoidHandlerTest.java | 176 ++++++++++++++++++ 6 files changed, 280 insertions(+), 106 deletions(-) create mode 100644 api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandler.java create mode 100644 integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandlerTest.java diff --git a/api/src/main/java/org/openmrs/module/queue/api/PatientWithQueueEntriesUnvoidHandler.java b/api/src/main/java/org/openmrs/module/queue/api/PatientWithQueueEntriesUnvoidHandler.java index c450a74c..0c541187 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/PatientWithQueueEntriesUnvoidHandler.java +++ b/api/src/main/java/org/openmrs/module/queue/api/PatientWithQueueEntriesUnvoidHandler.java @@ -85,7 +85,7 @@ public void handle(Patient patient, User originalVoidingUser, Date originalVoide /** * @return true if the entry was voided together with the patient and can be restored with it. An - * entry on a voided visit was taken down by VisitWithQueueEntriesSaveHandler, not by this + * entry on a voided visit was taken down by VisitWithQueueEntriesVoidHandler, not by this * cascade; restoring it would put an active entry back on a visit that is still voided. */ private static boolean shouldRestore(QueueEntry qe, User originalVoidingUser, Date originalVoidedDate) { diff --git a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java index 39f8b26f..66c95921 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java +++ b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java @@ -27,13 +27,8 @@ * fails on the queue_entry foreign key to visit. *

* Purging cannot be done from a handler: core runs save/void handlers through RequiredDataAdvice, - * which is not consulted on a purge, so advice on the service is the only hook. Voiding is left to - * {@link VisitWithQueueEntriesSaveHandler}, which voids the entries on the {@code saveVisit} path, - * and on the {@code voidVisit} path whenever it runs after core's {@code BaseVoidHandler}. Nothing - * pins that order: both handlers carry the default {@code @Handler} order, and the tie is broken by - * the iteration order of the map {@code ServiceContext.getRegisteredComponents} builds. The handler - * stamps the entries with the visit's own void date and user so an unvoid can tell them apart from - * entries voided on their own. + * which is not consulted on a purge, so advice on the service is the only hook. Voiding is owned by + * {@link VisitWithQueueEntriesVoidHandler}, which core does reach on the {@code voidVisit} path. */ public class VisitWithQueueEntriesDeleteAdvice implements MethodBeforeAdvice { diff --git a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandler.java b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandler.java index 4a11ac37..feeacee9 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandler.java +++ b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandler.java @@ -19,15 +19,19 @@ import org.openmrs.annotation.Handler; import org.openmrs.api.context.Context; import org.openmrs.api.handler.SaveHandler; -import org.openmrs.api.handler.VoidHandler; import org.openmrs.module.queue.api.search.QueueEntrySearchCriteria; import org.openmrs.module.queue.model.QueueEntry; import org.openmrs.module.queue.utils.PrivilegeConstants; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Qualifier; +/** + * Ends a visit's open queue entries when the visit is stopped. Voiding is owned by + * {@link VisitWithQueueEntriesVoidHandler}, which core reaches on the {@code voidVisit} path, and + * purging by {@link VisitWithQueueEntriesDeleteAdvice}. + */ @Handler(supports = Visit.class) -public class VisitWithQueueEntriesSaveHandler implements SaveHandler, VoidHandler { +public class VisitWithQueueEntriesSaveHandler implements SaveHandler { private final Log log = LogFactory.getLog(getClass()); @@ -40,7 +44,7 @@ public VisitWithQueueEntriesSaveHandler(@Qualifier("queue.QueueEntryService") Qu @Override public void handle(Visit visit, User user, Date date, String s) { - // Voiding or unvoiding is driven by core services whose callers need not hold queue privileges, + // Stopping a visit is driven by core services whose callers need not hold queue privileges, // so grant them for the duration of this cascade, as core's PatientDataVoidHandler does Context.addProxyPrivilege(PrivilegeConstants.GET_QUEUE_ENTRIES); Context.addProxyPrivilege(PrivilegeConstants.MANAGE_QUEUE_ENTRIES); @@ -59,23 +63,6 @@ public void handle(Visit visit, User user, Date date, String s) { log.trace("Closed queue entry " + qe + " on " + visit.getStopDatetime()); } } - if (visit.getVisitId() != null && visit.getVoided()) { - QueueEntrySearchCriteria criteria = new QueueEntrySearchCriteria(); - criteria.setVisit(visit); - // The visit's patient may itself be voided, which would hide its entries from the default search - criteria.setIncludedVoided(true); - List queueEntries = queueEntryService.getQueueEntries(criteria); - for (QueueEntry qe : queueEntries) { - if (!qe.getVoided()) { - qe.setVoided(true); - qe.setVoidReason(visit.getVoidReason()); - qe.setVoidedBy(visit.getVoidedBy()); - qe.setDateVoided(visit.getDateVoided()); - queueEntryService.saveQueueEntry(qe); - } - log.trace("Voided queue entry " + qe + " on " + date); - } - } } finally { Context.removeProxyPrivilege(PrivilegeConstants.GET_QUEUE_ENTRIES); diff --git a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandler.java b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandler.java new file mode 100644 index 00000000..27e067df --- /dev/null +++ b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandler.java @@ -0,0 +1,94 @@ +/* + * 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.module.queue.api; + +import java.util.Date; +import java.util.List; + +import org.openmrs.User; +import org.openmrs.Visit; +import org.openmrs.annotation.Handler; +import org.openmrs.api.context.Context; +import org.openmrs.api.handler.VoidHandler; +import org.openmrs.module.queue.api.search.QueueEntrySearchCriteria; +import org.openmrs.module.queue.model.QueueEntry; +import org.openmrs.module.queue.utils.PrivilegeConstants; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; + +/** + * Voids all queue entries of a visit when that visit is voided. Core knows nothing about queue + * entries, so nothing in its void cascade touches them; without this handler a deleted visit would + * leave its entries active and the patient would stay in the service queue. + *

+ * The void state is read from the handler arguments rather than from {@code visit.getVoided()}, so + * this does not depend on running after core's {@code BaseVoidHandler}. That matters: both handlers + * carry the default {@code @Handler} order, {@code HandlerUtil} sorts stably, and the tie is broken + * by the iteration order of the map {@code ServiceContext.getRegisteredComponents} builds, which + * moves with the number of {@code VoidHandler} beans a deployment happens to register. Core's own + * {@code VisitVoidHandler} takes the same approach for a visit's encounters. + *

+ * {@code RequiredDataAdvice} passes every handler the same void date and reason it hands to + * {@code BaseVoidHandler}, so the entries carry the visit's own void stamp whichever runs first. + * Entries that were already voided keep the stamp they had, so an unvoid can tell them apart from + * entries taken down with the visit. + */ +@Handler(supports = Visit.class) +public class VisitWithQueueEntriesVoidHandler implements VoidHandler { + + private static final Logger log = LoggerFactory.getLogger(VisitWithQueueEntriesVoidHandler.class); + + private final QueueEntryService queueEntryService; + + @Autowired + public VisitWithQueueEntriesVoidHandler(@Qualifier("queue.QueueEntryService") QueueEntryService queueEntryService) { + this.queueEntryService = queueEntryService; + } + + @Override + public void handle(Visit visit, User voidingUser, Date voidedDate, String voidReason) { + if (visit.getVisitId() == null) { + return; + } + // Voiding is driven by core services whose callers need not hold queue privileges, so grant + // them for the duration of this cascade, as core's PatientDataVoidHandler does + Context.addProxyPrivilege(PrivilegeConstants.GET_QUEUE_ENTRIES); + Context.addProxyPrivilege(PrivilegeConstants.MANAGE_QUEUE_ENTRIES); + try { + QueueEntrySearchCriteria criteria = new QueueEntrySearchCriteria(); + criteria.setVisit(visit); + // The visit's patient may itself be voided, which would hide its entries from the default search + criteria.setIncludedVoided(true); + List queueEntries = queueEntryService.getQueueEntries(criteria); + int voidedCount = 0; + for (QueueEntry qe : queueEntries) { + if (qe.getVoided()) { + continue; + } + qe.setVoided(true); + qe.setVoidReason(voidReason); + qe.setVoidedBy(voidingUser); + qe.setDateVoided(voidedDate); + queueEntryService.saveQueueEntry(qe); + voidedCount++; + log.trace("Voided queue entry {} on {}", qe, voidedDate); + } + if (voidedCount > 0) { + log.info("Voided {} queue entries of visit {} with reason: {}", voidedCount, visit.getVisitId(), voidReason); + } + } + finally { + Context.removeProxyPrivilege(PrivilegeConstants.GET_QUEUE_ENTRIES); + Context.removeProxyPrivilege(PrivilegeConstants.MANAGE_QUEUE_ENTRIES); + } + } +} diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java index 820f57e5..830e430c 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesSaveHandlerTest.java @@ -10,11 +10,8 @@ package org.openmrs.module.queue.api; import static org.hamcrest.Matchers.equalTo; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; import java.util.Arrays; import java.util.Date; @@ -25,7 +22,6 @@ import org.junit.Test; import org.openmrs.Visit; import org.openmrs.api.VisitService; -import org.openmrs.api.context.Context; import org.openmrs.module.queue.SpringTestConfiguration; import org.openmrs.module.queue.model.QueueEntry; import org.openmrs.test.BaseModuleContextSensitiveTest; @@ -86,78 +82,4 @@ public void shouldEndQueueEntriesIfVisitIsStopped() { assertThat(visit.getStopDatetime(), equalTo(stopDate)); assertThat(queueEntry.getEndedAt(), equalTo(stopDate)); } - - @Test - public void shouldNotVoidQueueEntriesIfVisitIsNotVoided() { - assertFalse(visit.getVoided()); - assertFalse(queueEntry.getVoided()); - visit = visitService.saveVisit(visit); - queueEntry = queueEntryService.getQueueEntryById(queueEntry.getId()).get(); - assertFalse(visit.getVoided()); - assertFalse(queueEntry.getVoided()); - } - - @Test - public void shouldVoidQueueEntriesIfVisitIsVoided() { - assertFalse(visit.getVoided()); - assertFalse(queueEntry.getVoided()); - visit.setVoided(true); - String voidReason = "for testing"; - visit.setVoidReason(voidReason); - visit = visitService.saveVisit(visit); - queueEntry = queueEntryService.getQueueEntryById(queueEntry.getId()).get(); - assertTrue(visit.getVoided()); - assertThat(visit.getVoidReason(), equalTo(voidReason)); - assertNotNull(visit.getDateVoided()); - assertNotNull(visit.getVoidedBy()); - assertTrue(queueEntry.getVoided()); - assertThat(queueEntry.getVoidReason(), equalTo(voidReason)); - assertThat(queueEntry.getDateVoided(), equalTo(visit.getDateVoided())); - assertThat(queueEntry.getVoidedBy(), equalTo(visit.getVoidedBy())); - } - - @Test - public void shouldVoidQueueEntriesWhenVisitIsVoidedThroughVoidVisit() { - // voidVisit is the path REST and patient deletion use. Whether this handler sees the visit as - // voided there depends on it running after core's BaseVoidHandler, so pin the outcome. - assertFalse(queueEntry.getVoided()); - String voidReason = "for testing"; - visit = visitService.voidVisit(visit, voidReason); - queueEntry = queueEntryService.getQueueEntryById(queueEntry.getId()).get(); - assertTrue(visit.getVoided()); - assertTrue(queueEntry.getVoided()); - assertThat(queueEntry.getVoidReason(), equalTo(voidReason)); - assertThat(queueEntry.getDateVoided(), equalTo(visit.getDateVoided())); - assertThat(queueEntry.getVoidedBy(), equalTo(visit.getVoidedBy())); - } - - @Test - public void shouldVoidQueueEntriesForUserWithoutQueuePrivileges() { - // "butch" from the standard test dataset has the Provider role and no privileges; proxy only the - // core visit privileges so the handler has to obtain the queue privileges itself - Context.becomeUser("3-4"); - Context.addProxyPrivilege(org.openmrs.util.PrivilegeConstants.DELETE_VISITS); - Context.addProxyPrivilege(org.openmrs.util.PrivilegeConstants.GET_ENCOUNTERS); - try { - assertFalse(Context.hasPrivilege(org.openmrs.module.queue.utils.PrivilegeConstants.GET_QUEUE_ENTRIES)); - visit = visitService.voidVisit(visit, "for testing"); - } - finally { - Context.removeProxyPrivilege(org.openmrs.util.PrivilegeConstants.DELETE_VISITS); - Context.removeProxyPrivilege(org.openmrs.util.PrivilegeConstants.GET_ENCOUNTERS); - authenticate(); - } - assertTrue(visit.getVoided()); - assertTrue(queueEntryService.getQueueEntryById(queueEntry.getId()).get().getVoided()); - } - - @Test - public void shouldVoidOverlappingQueueEntriesInTheSameQueueIfVisitIsVoided() { - executeDataSet("org/openmrs/module/queue/api/dao/QueueEntryDaoTest_overlappingEntriesInitialDataset.xml"); - visit.setVoided(true); - visit.setVoidReason("for testing"); - visitService.saveVisit(visit); - assertTrue(queueEntryService.getQueueEntryById(3).get().getVoided()); - assertTrue(queueEntryService.getQueueEntryById(12).get().getVoided()); - } } diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandlerTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandlerTest.java new file mode 100644 index 00000000..723e3140 --- /dev/null +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandlerTest.java @@ -0,0 +1,176 @@ +/* + * 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.module.queue.api; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; + +import java.util.Arrays; +import java.util.Date; +import java.util.List; + +import org.junit.Before; +import org.junit.Test; +import org.openmrs.Visit; +import org.openmrs.api.VisitService; +import org.openmrs.api.context.Context; +import org.openmrs.module.queue.SpringTestConfiguration; +import org.openmrs.module.queue.model.QueueEntry; +import org.openmrs.test.BaseModuleContextSensitiveTest; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.test.context.ContextConfiguration; + +@ContextConfiguration(classes = SpringTestConfiguration.class, inheritLocations = false) +public class VisitWithQueueEntriesVoidHandlerTest extends BaseModuleContextSensitiveTest { + + private static final List INITIAL_DATASET_XML = Arrays.asList( + "org/openmrs/module/queue/api/dao/QueueDaoTest_locationInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_conceptsInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_patientInitialDataset.xml", + "org/openmrs/module/queue/api/dao/VisitQueueEntryDaoTest_visitInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueDaoTest_initialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_initialDataset.xml", + "org/openmrs/module/queue/validators/QueueEntryValidatorTest_globalPropertyInitialDataset.xml"); + + // Visit 102 owns the single active queue entry 3 + private static final int VISIT_ID = 102; + + private static final int QUEUE_ENTRY_ID = 3; + + // Visit 101 owns queue entries 1 (ended), 2 (active) and 10 (voided on its own) + private static final int VISIT_WITH_VOIDED_ENTRY_ID = 101; + + private Visit visit; + + private QueueEntry queueEntry; + + @Autowired + @Qualifier("queue.QueueEntryService") + private QueueEntryService queueEntryService; + + @Autowired + private VisitWithQueueEntriesVoidHandler handler; + + @Autowired + private VisitService visitService; + + @Before + public void setup() { + INITIAL_DATASET_XML.forEach(this::executeDataSet); + queueEntry = queueEntryService.getQueueEntryById(QUEUE_ENTRY_ID).get(); + visit = queueEntry.getVisit(); + } + + @Test + public void shouldVoidQueueEntriesWhenVisitIsVoided() { + assertFalse(queueEntry.getVoided()); + String voidReason = "for testing"; + + visitService.voidVisit(visit, voidReason); + Context.flushSession(); + Context.clearSession(); + + Visit voidedVisit = visitService.getVisit(VISIT_ID); + QueueEntry voidedEntry = queueEntryService.getQueueEntryById(QUEUE_ENTRY_ID).get(); + assertTrue(voidedVisit.getVoided()); + assertTrue(voidedEntry.getVoided()); + assertThat(voidedEntry.getVoidReason(), equalTo(voidReason)); + assertThat(voidedEntry.getDateVoided().getTime(), equalTo(voidedVisit.getDateVoided().getTime())); + assertThat(voidedEntry.getVoidedBy(), equalTo(voidedVisit.getVoidedBy())); + } + + @Test + public void shouldVoidQueueEntriesBeforeTheVisitItselfIsFlaggedVoided() { + // This is the state the handler sees whenever it is ordered ahead of core's BaseVoidHandler, and + // nothing pins that order: the two carry the same @Handler order, HandlerUtil sorts stably, and + // the tie falls to the iteration order of the map ServiceContext.getRegisteredComponents builds, + // which moves with the number of VoidHandler beans a deployment registers. Taking the void state + // from the arguments rather than from the visit is what makes the cascade independent of that. + assertFalse(visit.getVoided()); + assertFalse(queueEntry.getVoided()); + // whole seconds: date_voided does not round-trip milliseconds + Date voidedDate = new Date(System.currentTimeMillis() / 1000 * 1000); + + handler.handle(visit, Context.getAuthenticatedUser(), voidedDate, "for testing"); + Context.flushSession(); + Context.clearSession(); + + QueueEntry voidedEntry = queueEntryService.getQueueEntryById(QUEUE_ENTRY_ID).get(); + assertFalse(visitService.getVisit(VISIT_ID).getVoided()); + assertTrue(voidedEntry.getVoided()); + assertThat(voidedEntry.getVoidReason(), equalTo("for testing")); + assertThat(voidedEntry.getDateVoided().getTime(), equalTo(voidedDate.getTime())); + assertThat(voidedEntry.getVoidedBy(), equalTo(Context.getAuthenticatedUser())); + } + + @Test + public void shouldVoidQueueEntriesForUserWithoutQueuePrivileges() { + // "butch" from the standard test dataset has the Provider role and no privileges; proxy only the + // core visit privileges so the handler has to obtain the queue privileges itself + Context.becomeUser("3-4"); + Context.addProxyPrivilege(org.openmrs.util.PrivilegeConstants.DELETE_VISITS); + Context.addProxyPrivilege(org.openmrs.util.PrivilegeConstants.GET_ENCOUNTERS); + try { + assertFalse(Context.hasPrivilege(org.openmrs.module.queue.utils.PrivilegeConstants.GET_QUEUE_ENTRIES)); + visitService.voidVisit(visit, "for testing"); + Context.flushSession(); + } + finally { + Context.removeProxyPrivilege(org.openmrs.util.PrivilegeConstants.DELETE_VISITS); + Context.removeProxyPrivilege(org.openmrs.util.PrivilegeConstants.GET_ENCOUNTERS); + authenticate(); + } + Context.clearSession(); + assertTrue(visitService.getVisit(VISIT_ID).getVoided()); + assertTrue(queueEntryService.getQueueEntryById(QUEUE_ENTRY_ID).get().getVoided()); + } + + @Test + public void shouldVoidOverlappingQueueEntriesInTheSameQueueIfVisitIsVoided() { + executeDataSet("org/openmrs/module/queue/api/dao/QueueEntryDaoTest_overlappingEntriesInitialDataset.xml"); + + visitService.voidVisit(visit, "for testing"); + Context.flushSession(); + Context.clearSession(); + + assertTrue(queueEntryService.getQueueEntryById(3).get().getVoided()); + assertTrue(queueEntryService.getQueueEntryById(12).get().getVoided()); + } + + @Test + public void shouldLeaveTheStampOfAnAlreadyVoidedQueueEntryAlone() { + // entry 10 was voided on its own, so an unvoid must be able to tell it apart from the entries + // that went down with the visit + QueueEntry alreadyVoided = queueEntryService.getQueueEntryById(10).get(); + assertTrue(alreadyVoided.getVoided()); + Date originalDateVoided = alreadyVoided.getDateVoided(); + String originalVoidReason = alreadyVoided.getVoidReason(); + assertNotNull(originalDateVoided); + + visitService.voidVisit(visitService.getVisit(VISIT_WITH_VOIDED_ENTRY_ID), "for testing"); + Context.flushSession(); + Context.clearSession(); + + Visit voidedVisit = visitService.getVisit(VISIT_WITH_VOIDED_ENTRY_ID); + alreadyVoided = queueEntryService.getQueueEntryById(10).get(); + assertThat(alreadyVoided.getDateVoided().getTime(), equalTo(originalDateVoided.getTime())); + assertThat(alreadyVoided.getVoidReason(), equalTo(originalVoidReason)); + assertThat(alreadyVoided.getDateVoided().getTime(), not(equalTo(voidedVisit.getDateVoided().getTime()))); + + QueueEntry cascaded = queueEntryService.getQueueEntryById(2).get(); + assertTrue(cascaded.getVoided()); + assertThat(cascaded.getDateVoided().getTime(), equalTo(voidedVisit.getDateVoided().getTime())); + } +} From 3c43c4f3236593900f2697aeb6e6d2d5873510b5 Mon Sep 17 00:00:00 2001 From: dkayiwa Date: Wed, 16 Sep 2026 17:56:16 +0300 Subject: [PATCH 07/12] O3-5459: Purge queue entries in the same transaction as the visit VisitWithQueueEntriesDeleteAdvice purged a visit's queue entries from a MethodBeforeAdvice, which commits that half before core has decided whether it will delete the visit at all. VisitServiceImpl.purgeVisit refuses a visit that still has encounters, and it does so after the advice has run, so the entries went and the visit stayed. Nothing rolled them back either. Advice registered through Context.addAdvice is appended to the proxy ServiceContext holds, and core's applicationContext-service.xml already auto-proxies every @Transactional service implementation, so that outer proxy sits one layer outside the one which begins the transaction. Make it a MethodInterceptor and open the transaction here, so the cascade and core's delete stand or fall together. VisitWithQueueEntriesDeleteAdviceTransactionTest covers it, in its own class because it has to run outside the usual test transaction: inside one the purged entries read as deleted either way, so a transactional test passes against the bug. Also close two gaps the same review found: - ModuleAdviceConfigTest walks the config.xml elements and does what ModuleFactory.loadAdvice does with them. Nothing else in the build reads them, and a bad point or class fails quietly: the module starts with the cascade simply absent. - shouldVoidQueueEntriesThatHadAlreadyEnded pins that the void cascade covers entries already ended by a queue transition, which is the shape a visit deleted after "serve patient" actually has. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012KvveaJ2rSEqEgxHrUPZU4 --- .../VisitWithQueueEntriesDeleteAdvice.java | 44 +++++-- ...eueEntriesDeleteAdviceTransactionTest.java | 115 ++++++++++++++++++ .../VisitWithQueueEntriesVoidHandlerTest.java | 20 +++ .../module/queue/ModuleAdviceConfigTest.java | 91 ++++++++++++++ 4 files changed, 263 insertions(+), 7 deletions(-) create mode 100644 integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java create mode 100644 omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java diff --git a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java index 66c95921..963742ec 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java +++ b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java @@ -9,9 +9,10 @@ */ package org.openmrs.module.queue.api; -import java.lang.reflect.Method; import java.util.List; +import org.aopalliance.intercept.MethodInterceptor; +import org.aopalliance.intercept.MethodInvocation; import org.openmrs.Visit; import org.openmrs.api.context.Context; import org.openmrs.module.queue.api.search.QueueEntrySearchCriteria; @@ -19,7 +20,9 @@ import org.openmrs.module.queue.utils.PrivilegeConstants; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.springframework.aop.MethodBeforeAdvice; +import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.TransactionStatus; +import org.springframework.transaction.support.DefaultTransactionDefinition; /** * Purges the queue entries of a visit before {@link org.openmrs.api.VisitService#purgeVisit} @@ -29,21 +32,48 @@ * Purging cannot be done from a handler: core runs save/void handlers through RequiredDataAdvice, * which is not consulted on a purge, so advice on the service is the only hook. Voiding is owned by * {@link VisitWithQueueEntriesVoidHandler}, which core does reach on the {@code voidVisit} path. + *

+ * The cascade and core's own delete are run inside one transaction opened here, because the entries + * must not go without the visit. {@code purgeVisit} refuses a visit that still has encounters, and + * it does so after this advice has run, so the two have to stand or fall together. A transaction is + * opened explicitly rather than relied upon: advice registered through {@code Context.addAdvice} is + * appended to the proxy that {@code ServiceContext} holds, and core's + * {@code applicationContext-service.xml} already auto-proxies every {@code @Transactional} service + * implementation, so that outer proxy sits one layer outside the one which begins the transaction + * and nothing here would otherwise be rolled back with the failed delete. */ -public class VisitWithQueueEntriesDeleteAdvice implements MethodBeforeAdvice { +public class VisitWithQueueEntriesDeleteAdvice implements MethodInterceptor { private static final Logger log = LoggerFactory.getLogger(VisitWithQueueEntriesDeleteAdvice.class); @Override - public void before(Method method, Object[] args, Object target) { - if (!"purgeVisit".equals(method.getName()) || args.length == 0 || !(args[0] instanceof Visit)) { - return; + public Object invoke(MethodInvocation invocation) throws Throwable { + Object[] args = invocation.getArguments(); + if (!"purgeVisit".equals(invocation.getMethod().getName()) || args.length == 0 || !(args[0] instanceof Visit)) { + return invocation.proceed(); } Visit visit = (Visit) args[0]; if (visit.getVisitId() == null) { - return; + return invocation.proceed(); } + PlatformTransactionManager transactionManager = Context.getRegisteredComponent("transactionManager", + PlatformTransactionManager.class); + TransactionStatus transaction = transactionManager.getTransaction(new DefaultTransactionDefinition()); + Object result; + try { + purgeQueueEntries(visit); + result = invocation.proceed(); + } + catch (Throwable t) { + transactionManager.rollback(transaction); + throw t; + } + transactionManager.commit(transaction); + return result; + } + + private void purgeQueueEntries(Visit visit) { // Purging a visit is driven by a core service whose callers need not hold queue privileges, // so grant them for the duration of this cascade, as the queue handlers do Context.addProxyPrivilege(PrivilegeConstants.GET_QUEUE_ENTRIES); diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java new file mode 100644 index 00000000..0f74a4bf --- /dev/null +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java @@ -0,0 +1,115 @@ +/* + * 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.module.queue.api; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +import java.util.Arrays; +import java.util.List; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.openmrs.Encounter; +import org.openmrs.Visit; +import org.openmrs.api.APIException; +import org.openmrs.api.EncounterService; +import org.openmrs.api.LocationService; +import org.openmrs.api.VisitService; +import org.openmrs.api.context.Context; +import org.openmrs.module.queue.SpringTestConfiguration; +import org.openmrs.test.BaseModuleContextSensitiveTest; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.transaction.annotation.Propagation; +import org.springframework.transaction.annotation.Transactional; + +/** + * Covers the one thing {@link VisitWithQueueEntriesDeleteAdviceTest} structurally cannot: that the + * queue entries the advice purges go only if the visit goes too. This needs its own class because + * the test has to run outside the transaction the other tests rely on. Inside one, the purged + * entries read as deleted whether or not the failed delete would have rolled them back, so a + * transactional test passes either way. + */ +@ContextConfiguration(classes = SpringTestConfiguration.class, inheritLocations = false) +public class VisitWithQueueEntriesDeleteAdviceTransactionTest extends BaseModuleContextSensitiveTest { + + private static final List INITIAL_DATASET_XML = Arrays.asList( + "org/openmrs/module/queue/api/dao/QueueDaoTest_locationInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_conceptsInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_patientInitialDataset.xml", + "org/openmrs/module/queue/api/dao/VisitQueueEntryDaoTest_visitInitialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueDaoTest_initialDataset.xml", + "org/openmrs/module/queue/api/dao/QueueEntryDaoTest_initialDataset.xml", + "org/openmrs/module/queue/validators/QueueEntryValidatorTest_globalPropertyInitialDataset.xml"); + + private static final int QUEUE_ENTRY_ID = 3; + + private final VisitWithQueueEntriesDeleteAdvice advice = new VisitWithQueueEntriesDeleteAdvice(); + + @Autowired + @Qualifier("queue.QueueEntryService") + private QueueEntryService queueEntryService; + + @Autowired + private VisitService visitService; + + @Autowired + private EncounterService encounterService; + + @Autowired + private LocationService locationService; + + @Before + public void setup() { + INITIAL_DATASET_XML.forEach(this::executeDataSet); + // config.xml is not read by module tests, so register it here to exercise the same + // interceptor chain that production purges go through + Context.addAdvice(VisitService.class, advice); + } + + @After + public void tearDown() { + Context.removeAdvice(VisitService.class, advice); + // nothing this test wrote was rolled back, so wipe it and let the next class re-seed + deleteAllData(); + } + + @Test + @Transactional(propagation = Propagation.NOT_SUPPORTED) + public void shouldKeepQueueEntriesWhenCoreRefusesToPurgeTheVisit() { + Visit visit = queueEntryService.getQueueEntryById(QUEUE_ENTRY_ID).get().getVisit(); + Integer visitId = visit.getVisitId(); + giveTheVisitAnEncounter(visit); + assertFalse("the visit needs an encounter for core to refuse the purge", + encounterService.getEncountersByVisit(visit, true).isEmpty()); + + // core checks for encounters only once this advice has already run + assertThrows(APIException.class, () -> visitService.purgeVisit(visit)); + Context.clearSession(); + + assertNotNull("the visit survived, so its queue entries must too", visitService.getVisit(visitId)); + assertTrue(queueEntryService.getQueueEntryById(QUEUE_ENTRY_ID).isPresent()); + } + + private void giveTheVisitAnEncounter(Visit visit) { + Encounter encounter = new Encounter(); + encounter.setPatient(visit.getPatient()); + encounter.setEncounterType(encounterService.getEncounterType(1)); + encounter.setEncounterDatetime(visit.getStartDatetime()); + encounter.setLocation(locationService.getLocation(1)); + encounter.setVisit(visit); + encounterService.saveEncounter(encounter); + } +} diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandlerTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandlerTest.java index 723e3140..5f9c07fc 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandlerTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesVoidHandlerTest.java @@ -173,4 +173,24 @@ public void shouldLeaveTheStampOfAnAlreadyVoidedQueueEntryAlone() { assertTrue(cascaded.getVoided()); assertThat(cascaded.getDateVoided().getTime(), equalTo(voidedVisit.getDateVoided().getTime())); } + + @Test + public void shouldVoidQueueEntriesThatHadAlreadyEnded() { + // An entry is ended rather than removed when the patient is served or moved on to another queue, + // so a visit deleted after that has ended entries on it and they belong to a visit that now never + // happened. Nothing else pins this: the cascade covers them only because + // QueueEntrySearchCriteria.isEnded defaults to null, and the sibling save handler sets it to false. + QueueEntry ended = queueEntryService.getQueueEntryById(1).get(); + assertNotNull(ended.getEndedAt()); + assertFalse(ended.getVoided()); + + visitService.voidVisit(visitService.getVisit(VISIT_WITH_VOIDED_ENTRY_ID), "for testing"); + Context.flushSession(); + Context.clearSession(); + + Visit voidedVisit = visitService.getVisit(VISIT_WITH_VOIDED_ENTRY_ID); + ended = queueEntryService.getQueueEntryById(1).get(); + assertTrue(ended.getVoided()); + assertThat(ended.getDateVoided().getTime(), equalTo(voidedVisit.getDateVoided().getTime())); + } } diff --git a/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java b/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java new file mode 100644 index 00000000..1ea5fffa --- /dev/null +++ b/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java @@ -0,0 +1,91 @@ +/* + * 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.module.queue; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import javax.xml.parsers.DocumentBuilder; +import javax.xml.parsers.DocumentBuilderFactory; + +import java.io.InputStream; +import java.net.URL; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +import org.aopalliance.aop.Advice; +import org.junit.Test; +import org.w3c.dom.Document; +import org.w3c.dom.Element; +import org.w3c.dom.NodeList; + +/** + * The {@code } elements in config.xml are the only thing that registers this module's AOP + * advice in a deployed server, and module tests register advice themselves, so nothing else in the + * build reads them. Getting one wrong fails quietly rather than loudly: + * {@code AdvicePoint.getClassInstance} catches the reflection failure and logs a warning, + * {@code ModuleFactory.loadAdvice} then logs at debug and carries on, and the module starts with + * the cascade simply absent. + */ +public class ModuleAdviceConfigTest { + + private static final String MODULE_PACKAGE = "org.openmrs.module.queue"; + + @Test + public void everyAdvicePointAndClassInConfigXmlShouldBeUsable() throws Exception { + NodeList adviceElements = queueConfigXml().getElementsByTagName("advice"); + assertTrue("config.xml should declare at least one advice", adviceElements.getLength() > 0); + + for (int i = 0; i < adviceElements.getLength(); i++) { + Element advice = (Element) adviceElements.item(i); + String point = childText(advice, "point"); + String adviceClassName = childText(advice, "class"); + assertNotNull("advice needs a point", point); + assertNotNull("advice needs a class", adviceClassName); + + // this mirrors what ModuleFactory.loadAdvice and AdvicePoint.getClassInstance do: load the + // point, load the advice class, call its public no-arg constructor, cast the result to Advice + Class.forName(point); + Class adviceClass = Class.forName(adviceClassName); + assertTrue(adviceClassName + " must implement " + Advice.class.getName() + " to be registered on " + point, + Advice.class.isAssignableFrom(adviceClass)); + assertNotNull(adviceClass.getConstructor().newInstance()); + } + } + + /** + * Reads this module's packaged config.xml, picked out by its {@code } rather than by + * taking the first {@code config.xml} on the classpath, because required modules ship one too. + */ + private Document queueConfigXml() throws Exception { + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); + factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + DocumentBuilder builder = factory.newDocumentBuilder(); + + List ours = new ArrayList<>(); + for (URL url : Collections.list(getClass().getClassLoader().getResources("config.xml"))) { + try (InputStream in = url.openStream()) { + Document document = builder.parse(in); + if (MODULE_PACKAGE.equals(childText(document.getDocumentElement(), "package"))) { + ours.add(document); + } + } + } + assertEquals("expected exactly one config.xml declaring package " + MODULE_PACKAGE, 1, ours.size()); + return ours.get(0); + } + + private static String childText(Element parent, String tagName) { + NodeList children = parent.getElementsByTagName(tagName); + return children.getLength() == 0 ? null : children.item(0).getTextContent().trim(); + } +} From 9328cd69ccde95304dc649c19fe4589a2eaf0063 Mon Sep 17 00:00:00 2001 From: dkayiwa Date: Wed, 16 Sep 2026 18:10:11 +0300 Subject: [PATCH 08/12] O3-5459: Tighten the purge cascade's tests and what its comments claim Phase 1 of a harden pass over the previous commit. - VisitWithQueueEntriesDeleteAdviceTest covers the advice's pass-through. It is an around interceptor now, so every VisitService call in a server with the module installed goes through it, and a no-arg method covers the empty argument array. Dropping the returned value reddens it. - The transaction test pins why the purge was refused. Without that it passed on any APIException, including one raised before the cascade ran at all: becoming a user without Purge Visits satisfied every other assertion in it. - ModuleAdviceConfigTest reaches for the advice class's no-arg constructor before casting to Advice, which is the order AdvicePoint uses and also the only order in which the constructor check is reachable. Pointed at a class with an @Autowired constructor it now reddens; behind the Advice cast it never could. It looks the constructor up rather than calling it, so an advice that touches Context on construction is not failed for that here. - Say what the proxy stack does rather than that core auto-proxies every @Transactional service: applicationContext-service.xml declares a DefaultAdvisorAutoProxyCreator beside a TransactionAttributeSourceAdvisor, which is checkable. - Record why the transaction manager is looked up per call. ModuleUtil and DispatcherServlet re-run ModuleFactory.loadAdvice after a Spring refresh and AdvicePoint hands them the same cached advice instance, so a field would outlive the context its bean came from. - "config.xml is not read by module tests" is no longer true now that ModuleAdviceConfigTest reads it; say that the harness does not register advice from it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012KvveaJ2rSEqEgxHrUPZU4 --- .../VisitWithQueueEntriesDeleteAdvice.java | 23 ++++++++++++------- ...VisitWithQueueEntriesDeleteAdviceTest.java | 16 +++++++++++-- ...eueEntriesDeleteAdviceTransactionTest.java | 22 +++++++++++------- .../module/queue/ModuleAdviceConfigTest.java | 22 ++++++++++++------ 4 files changed, 58 insertions(+), 25 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java index 963742ec..fd54c302 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java +++ b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java @@ -29,18 +29,22 @@ * deletes it. Core's purge cascade knows nothing about queue entries, so without this the delete * fails on the queue_entry foreign key to visit. *

- * Purging cannot be done from a handler: core runs save/void handlers through RequiredDataAdvice, - * which is not consulted on a purge, so advice on the service is the only hook. Voiding is owned by + * Purging cannot be done from a handler. RequiredDataAdvice dispatches handlers only for method + * names beginning save, create, void, unvoid, retire or unretire, so a purge reaches none of them, + * and the module hooks the service instead. Voiding is owned by * {@link VisitWithQueueEntriesVoidHandler}, which core does reach on the {@code voidVisit} path. *

* The cascade and core's own delete are run inside one transaction opened here, because the entries * must not go without the visit. {@code purgeVisit} refuses a visit that still has encounters, and - * it does so after this advice has run, so the two have to stand or fall together. A transaction is - * opened explicitly rather than relied upon: advice registered through {@code Context.addAdvice} is - * appended to the proxy that {@code ServiceContext} holds, and core's - * {@code applicationContext-service.xml} already auto-proxies every {@code @Transactional} service - * implementation, so that outer proxy sits one layer outside the one which begins the transaction - * and nothing here would otherwise be rolled back with the failed delete. + * it does so after this advice has run, so the two have to stand or fall together. + *

+ * The transaction is opened explicitly rather than inherited from the service. Advice registered + * through {@code Context.addAdvice} is appended to the proxy {@code ServiceContext} holds for + * {@code VisitService}, and core's {@code applicationContext-service.xml} declares a + * {@code DefaultAdvisorAutoProxyCreator} beside a {@code TransactionAttributeSourceAdvisor}, which + * leaves {@code visitServiceTarget} transaction-proxied before the proxy this advice is appended to + * wraps it. The transaction therefore begins a layer below anything registered here, and without + * this one nothing the cascade did would be rolled back with the refused delete. */ public class VisitWithQueueEntriesDeleteAdvice implements MethodInterceptor { @@ -57,6 +61,9 @@ public Object invoke(MethodInvocation invocation) throws Throwable { return invocation.proceed(); } + // Looked up per call, not held in a field: ModuleUtil and DispatcherServlet re-run + // ModuleFactory.loadAdvice after a Spring refresh, and AdvicePoint hands them this same cached + // instance, so a field would outlive the context the bean came from PlatformTransactionManager transactionManager = Context.getRegisteredComponent("transactionManager", PlatformTransactionManager.class); TransactionStatus transaction = transactionManager.getTransaction(new DefaultTransactionDefinition()); diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java index 418a2483..4f9c6952 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java @@ -10,6 +10,7 @@ package org.openmrs.module.queue.api; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -55,8 +56,9 @@ public class VisitWithQueueEntriesDeleteAdviceTest extends BaseModuleContextSens public void setup() { INITIAL_DATASET_XML.forEach(this::executeDataSet); visit = queueEntryService.getQueueEntryById(3).get().getVisit(); - // config.xml is not read by module tests, so register it here to exercise the same - // interceptor chain that production purges go through + // the module test harness does not register advice from config.xml, so register it here to + // exercise the same interceptor chain that production purges go through (ModuleAdviceConfigTest + // covers the config.xml declaration itself) Context.addAdvice(VisitService.class, advice); } @@ -112,4 +114,14 @@ public void shouldPurgeVoidedQueueEntriesOfAPurgedVisit() { assertNull(visitService.getVisit(101)); assertFalse(queueEntryService.getQueueEntryById(10).isPresent()); } + + @Test + public void shouldLeaveOtherVisitServiceCallsAlone() { + // This is an around interceptor, so every VisitService call in a server with the module + // installed passes through it, not just the purges. A no-arg method covers the empty argument + // array it has to tolerate on the way past. + assertFalse(visitService.getAllVisitTypes().isEmpty()); + assertNotNull(visitService.getVisit(visit.getVisitId())); + assertTrue(queueEntryService.getQueueEntryById(3).isPresent()); + } } diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java index 0f74a4bf..a0f0f5f4 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java @@ -9,8 +9,10 @@ */ package org.openmrs.module.queue.api; +import static org.hamcrest.Matchers.containsString; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThat; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; @@ -36,11 +38,11 @@ import org.springframework.transaction.annotation.Transactional; /** - * Covers the one thing {@link VisitWithQueueEntriesDeleteAdviceTest} structurally cannot: that the - * queue entries the advice purges go only if the visit goes too. This needs its own class because - * the test has to run outside the transaction the other tests rely on. Inside one, the purged - * entries read as deleted whether or not the failed delete would have rolled them back, so a - * transactional test passes either way. + * Covers what {@link VisitWithQueueEntriesDeleteAdviceTest} structurally cannot: that the queue + * entries the advice purges go only if the visit goes too. This needs its own class because the + * test has to run outside the transaction the other tests rely on. Inside one, the purged entries + * read as deleted whether or not the failed delete would have rolled them back, so a transactional + * test passes either way. */ @ContextConfiguration(classes = SpringTestConfiguration.class, inheritLocations = false) public class VisitWithQueueEntriesDeleteAdviceTransactionTest extends BaseModuleContextSensitiveTest { @@ -74,8 +76,9 @@ public class VisitWithQueueEntriesDeleteAdviceTransactionTest extends BaseModule @Before public void setup() { INITIAL_DATASET_XML.forEach(this::executeDataSet); - // config.xml is not read by module tests, so register it here to exercise the same - // interceptor chain that production purges go through + // the module test harness does not register advice from config.xml, so register it here to + // exercise the same interceptor chain that production purges go through (ModuleAdviceConfigTest + // covers the config.xml declaration itself) Context.addAdvice(VisitService.class, advice); } @@ -96,7 +99,10 @@ public void shouldKeepQueueEntriesWhenCoreRefusesToPurgeTheVisit() { encounterService.getEncountersByVisit(visit, true).isEmpty()); // core checks for encounters only once this advice has already run - assertThrows(APIException.class, () -> visitService.purgeVisit(visit)); + APIException refused = assertThrows(APIException.class, () -> visitService.purgeVisit(visit)); + // pin what refused it, so this cannot pass on an exception raised before the cascade ran at all + // (the wording is core's Visit.purge.inUse) + assertThat(refused.getMessage(), containsString("encounters")); Context.clearSession(); assertNotNull("the visit survived, so its queue entries must too", visitService.getVisit(visitId)); diff --git a/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java b/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java index 1ea5fffa..a127b687 100644 --- a/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java +++ b/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java @@ -29,10 +29,10 @@ import org.w3c.dom.NodeList; /** - * The {@code } elements in config.xml are the only thing that registers this module's AOP - * advice in a deployed server, and module tests register advice themselves, so nothing else in the - * build reads them. Getting one wrong fails quietly rather than loudly: - * {@code AdvicePoint.getClassInstance} catches the reflection failure and logs a warning, + * The {@code } elements in config.xml are what registers this module's AOP advice in a + * deployed server, and this test is the build's only check on them: module tests register advice + * themselves rather than going through config.xml. Getting one wrong fails quietly rather than + * loudly: {@code AdvicePoint.getClassInstance} catches the reflection failure and logs a warning, * {@code ModuleFactory.loadAdvice} then logs at debug and carries on, and the module starts with * the cascade simply absent. */ @@ -52,13 +52,21 @@ public void everyAdvicePointAndClassInConfigXmlShouldBeUsable() throws Exception assertNotNull("advice needs a point", point); assertNotNull("advice needs a class", adviceClassName); - // this mirrors what ModuleFactory.loadAdvice and AdvicePoint.getClassInstance do: load the - // point, load the advice class, call its public no-arg constructor, cast the result to Advice + // this mirrors what ModuleFactory.loadAdvice and AdvicePoint.getClassInstance do with them: + // load the point, load the advice class, reach for its public no-arg constructor, and cast + // the instance to Advice. The constructor is looked up rather than called, so an advice that + // legitimately touches Context on construction is not failed for it here. Class.forName(point); Class adviceClass = Class.forName(adviceClassName); + try { + adviceClass.getConstructor(); + } + catch (NoSuchMethodException e) { + throw new AssertionError( + adviceClassName + " needs a public no-arg constructor for AdvicePoint to" + " instantiate it", e); + } assertTrue(adviceClassName + " must implement " + Advice.class.getName() + " to be registered on " + point, Advice.class.isAssignableFrom(adviceClass)); - assertNotNull(adviceClass.getConstructor().newInstance()); } } From 9411e0a8084c419d6cc4744c147be0beb7f841fc Mon Sep 17 00:00:00 2001 From: dkayiwa Date: Wed, 16 Sep 2026 19:03:19 +0300 Subject: [PATCH 09/12] O3-5459: Accept an Advisor, keep the refusal reason, fix two comments Phase 2 of the harden pass. Two lenses independently found the first of these; the rest came one each. - ModuleAdviceConfigTest rejected a legal config.xml. ModuleFactory.loadAdvice registers the instance with Context.addAdvisor if it is an Advisor and Context.addAdvice otherwise, and org.springframework.aop.Advisor holds an Advice rather than being one, so the Advice-only assertion failed a DefaultPointcutAdvisor that core would have loaded. Both arms checked: a throwaway Advisor now passes, and a class that is neither, with a no-arg constructor, still fails. - A throwing rollback took the refusal reason with it. The caller saw only TransactionSystemException; the APIException saying the visit still has encounters was gone, not as a cause and not in any log. Attach it as a suppressed exception, which is what Spring's own TransactionAspectSupport does for this case. Forcing the rollback to throw now prints it. - The transaction test's javadoc had the mechanism backwards. It said a transactional test sees the entries as deleted, which would fail the assertion rather than pass it. Measured on the un-transacted advice inside a transaction: present=false before Context.clearSession() and present=true after, so the clear discards the unflushed deletes and the reading cannot tell a rollback from one. - deleteAllData() in that test's teardown is not what protects the next class; core's @AfterClass wipes after every class already. It is there for a future second method in this class, whose executeDataSet would otherwise meet this one's committed rows. - The advice javadoc now names the inert advisor. visitService really is a TransactionProxyFactoryBean, so a maintainer who greps for that and concludes the advice is already inside a transaction would simplify the explicit one away. Its transaction advisor does not match purgeVisit, because its target is the interface proxy the auto-proxy creator wrapped around visitServiceTarget. - The pass-through comment claimed the interface-wide interception was new. It is not: config.xml is byte-identical at the base sha and addAdvice has always registered against the whole point. What is new is that an around interceptor has to proceed and return the result itself. Also add a message to the assertion that fires on every mutation of this cascade, since it reported a bare AssertionError. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012KvveaJ2rSEqEgxHrUPZU4 --- .../VisitWithQueueEntriesDeleteAdvice.java | 33 +++++++++++++------ ...VisitWithQueueEntriesDeleteAdviceTest.java | 7 ++-- ...eueEntriesDeleteAdviceTransactionTest.java | 14 +++++--- .../module/queue/ModuleAdviceConfigTest.java | 15 +++++---- 4 files changed, 45 insertions(+), 24 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java index fd54c302..ae436cf1 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java +++ b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java @@ -34,17 +34,22 @@ * and the module hooks the service instead. Voiding is owned by * {@link VisitWithQueueEntriesVoidHandler}, which core does reach on the {@code voidVisit} path. *

- * The cascade and core's own delete are run inside one transaction opened here, because the entries + * The cascade and core's own delete run in one transaction, taken out here, because the entries * must not go without the visit. {@code purgeVisit} refuses a visit that still has encounters, and - * it does so after this advice has run, so the two have to stand or fall together. + * it does so after this advice has run, so the two have to stand or fall together. Propagation is + * the default, so this joins a caller's transaction where there is one and starts its own where + * there is not. *

- * The transaction is opened explicitly rather than inherited from the service. Advice registered - * through {@code Context.addAdvice} is appended to the proxy {@code ServiceContext} holds for - * {@code VisitService}, and core's {@code applicationContext-service.xml} declares a - * {@code DefaultAdvisorAutoProxyCreator} beside a {@code TransactionAttributeSourceAdvisor}, which - * leaves {@code visitServiceTarget} transaction-proxied before the proxy this advice is appended to - * wraps it. The transaction therefore begins a layer below anything registered here, and without - * this one nothing the cascade did would be rolled back with the refused delete. + * It has to be taken out here rather than inherited from the service, and the reason is easy to get + * backwards. {@code applicationContext-service.xml} does declare {@code visitService} as a + * {@code TransactionProxyFactoryBean} (line 515), but that proxy's own transaction advisor is inert + * for {@code purgeVisit}: its target is the proxy that the {@code DefaultAdvisorAutoProxyCreator} + * and {@code TransactionAttributeSourceAdvisor} pair (lines 46 and 52) has already wrapped around + * {@code visitServiceTarget}, and that target is a JDK proxy of the {@code VisitService} interface, + * which carries no {@code @Transactional} for the attribute source to find. The transaction + * therefore begins in the inner proxy, a layer below where {@code Context.addAdvice} appends this + * advice, and without the one taken out here nothing the cascade did would be rolled back with the + * refused delete. */ public class VisitWithQueueEntriesDeleteAdvice implements MethodInterceptor { @@ -73,7 +78,15 @@ public Object invoke(MethodInvocation invocation) throws Throwable { result = invocation.proceed(); } catch (Throwable t) { - transactionManager.rollback(transaction); + try { + transactionManager.rollback(transaction); + } + catch (RuntimeException | Error rollbackFailure) { + // keep hold of t: it is the only thing that says why the purge was refused, and the + // rollback failure is what the caller is about to see instead + rollbackFailure.addSuppressed(t); + throw rollbackFailure; + } throw t; } transactionManager.commit(transaction); diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java index 4f9c6952..1815b03d 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java @@ -117,9 +117,10 @@ public void shouldPurgeVoidedQueueEntriesOfAPurgedVisit() { @Test public void shouldLeaveOtherVisitServiceCallsAlone() { - // This is an around interceptor, so every VisitService call in a server with the module - // installed passes through it, not just the purges. A no-arg method covers the empty argument - // array it has to tolerate on the way past. + // Context.addAdvice registers against the whole point, so every VisitService call in a server + // with the module installed reaches this advice and not just the purges. What is new is that an + // around interceptor has to proceed and hand back the result itself, where a before-advice did + // not. A no-arg method also covers the empty argument array it tolerates on the way past. assertFalse(visitService.getAllVisitTypes().isEmpty()); assertNotNull(visitService.getVisit(visit.getVisitId())); assertTrue(queueEntryService.getQueueEntryById(3).isPresent()); diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java index a0f0f5f4..1807871b 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java @@ -40,9 +40,10 @@ /** * Covers what {@link VisitWithQueueEntriesDeleteAdviceTest} structurally cannot: that the queue * entries the advice purges go only if the visit goes too. This needs its own class because the - * test has to run outside the transaction the other tests rely on. Inside one, the purged entries - * read as deleted whether or not the failed delete would have rolled them back, so a transactional - * test passes either way. + * test has to run outside the transaction the other tests rely on. Inside one the cascade's deletes + * are still unflushed when the delete is refused and {@code Context.clearSession()} discards them, + * so the entry reads as present again whether or not anything was rolled back, and the assertion + * holds either way. */ @ContextConfiguration(classes = SpringTestConfiguration.class, inheritLocations = false) public class VisitWithQueueEntriesDeleteAdviceTransactionTest extends BaseModuleContextSensitiveTest { @@ -85,7 +86,9 @@ public void setup() { @After public void tearDown() { Context.removeAdvice(VisitService.class, advice); - // nothing this test wrote was rolled back, so wipe it and let the next class re-seed + // Nothing this test wrote was rolled back. Core wipes after the last method of a class anyway, + // so this is here for the second test method: without it that method's executeDataSet would + // meet the rows this one committed. deleteAllData(); } @@ -106,7 +109,8 @@ public void shouldKeepQueueEntriesWhenCoreRefusesToPurgeTheVisit() { Context.clearSession(); assertNotNull("the visit survived, so its queue entries must too", visitService.getVisit(visitId)); - assertTrue(queueEntryService.getQueueEntryById(QUEUE_ENTRY_ID).isPresent()); + assertTrue("the refused purge must not have taken the queue entry down with it", + queueEntryService.getQueueEntryById(QUEUE_ENTRY_ID).isPresent()); } private void giveTheVisitAnEncounter(Visit visit) { diff --git a/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java b/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java index a127b687..f31cc694 100644 --- a/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java +++ b/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java @@ -24,6 +24,7 @@ import org.aopalliance.aop.Advice; import org.junit.Test; +import org.springframework.aop.Advisor; import org.w3c.dom.Document; import org.w3c.dom.Element; import org.w3c.dom.NodeList; @@ -52,10 +53,10 @@ public void everyAdvicePointAndClassInConfigXmlShouldBeUsable() throws Exception assertNotNull("advice needs a point", point); assertNotNull("advice needs a class", adviceClassName); - // this mirrors what ModuleFactory.loadAdvice and AdvicePoint.getClassInstance do with them: - // load the point, load the advice class, reach for its public no-arg constructor, and cast - // the instance to Advice. The constructor is looked up rather than called, so an advice that - // legitimately touches Context on construction is not failed for it here. + // AdvicePoint.getClassInstance loads the class and calls its public no-arg constructor, and + // ModuleFactory.loadAdvice then registers the instance as an Advisor if it is one and as an + // Advice otherwise, so either type is legal here. The constructor is looked up rather than + // called, so an advice that legitimately touches Context on construction is not failed for it. Class.forName(point); Class adviceClass = Class.forName(adviceClassName); try { @@ -65,8 +66,10 @@ public void everyAdvicePointAndClassInConfigXmlShouldBeUsable() throws Exception throw new AssertionError( adviceClassName + " needs a public no-arg constructor for AdvicePoint to" + " instantiate it", e); } - assertTrue(adviceClassName + " must implement " + Advice.class.getName() + " to be registered on " + point, - Advice.class.isAssignableFrom(adviceClass)); + assertTrue( + adviceClassName + " must implement " + Advice.class.getName() + " or " + Advisor.class.getName() + + " to be registered on " + point, + Advice.class.isAssignableFrom(adviceClass) || Advisor.class.isAssignableFrom(adviceClass)); } } From d51817d7324a59a2373e5edbea65b38d0708a6be Mon Sep 17 00:00:00 2001 From: dkayiwa Date: Wed, 16 Sep 2026 19:34:36 +0300 Subject: [PATCH 10/12] O3-5459: Stop explaining the proxy stack, and read our own config.xml Phase 2 pass 2. Two lenses found the first of these independently, and one of them built the slice against core 2.8.7 to show it. The advice javadoc explained why the platform's own transaction does not cover advice added with Context.addAdvice, naming three beans and their line numbers in core's applicationContext-service.xml. That was exact for 2.7.4 and inverted for 2.8.x: core 2.8.0 removed DefaultAdvisorAutoProxyCreator and TransactionAttributeSourceAdvisor outright, and there the outer visitService proxy is transactional for purgeVisit, so a maintainer on 2.8 would have found no such beans, read that the transaction was unnecessary, and deleted it. The regression test would have agreed with them: with the transaction stripped it fails on 2.7.4 and passes on 2.8.7. This is the third try at that paragraph, so rather than write a fourth, the platform archaeology is gone. What is left is the invariant that holds on both: addAdvice gives no guarantee a transaction is open, default propagation joins one if there is, and the test is what decides whether the entries survive a refused delete. The same applies to the teardown comment, whose stated reason was refuted twice - dbunit REFRESH tolerates the committed dataset rows, so that was never the reason - and which now says only that the test commits and wipes after itself. Also from that pass: - ModuleAdviceConfigTest reads omod/src/main/resources/config.xml instead of scanning the classpath for it. The scan parsed every config.xml there, and a required module ships one with a DOCTYPE; a malformed or entity-bearing document of somebody else's decided whether this test passed, and external general entities were still resolved, which means a network call for a document we did not write. Controls both ways: a bad still reddens, and a missing file fails loudly rather than finding nothing. - Its javadoc no longer says a bad always fails quietly. That is true of a class that will not load or instantiate, but loadAdvice casts to Advice without testing and ModuleUtil.refreshApplicationContext has no catch, so a wrong type aborts the post-refresh loop for every started module. - TransactionDefinition.withDefaults() instead of allocating a mutable DefaultTransactionDefinition per purge, which also states in code what the javadoc otherwise has to assert in prose. - The addSuppressed comment said the caller sees the refusal reason. It reaches the log; error responses built from getCause() do not show it. - Neither advice test now names ModuleAdviceConfigTest: integration-tests does not depend on queue-omod, so that could never have been a {@link} and renaming the class would have left both comments pointing nowhere. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012KvveaJ2rSEqEgxHrUPZU4 --- .../VisitWithQueueEntriesDeleteAdvice.java | 30 +++++------ ...VisitWithQueueEntriesDeleteAdviceTest.java | 3 +- ...eueEntriesDeleteAdviceTransactionTest.java | 8 ++- .../module/queue/ModuleAdviceConfigTest.java | 51 +++++++++---------- 4 files changed, 41 insertions(+), 51 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java index ae436cf1..04eefe03 100644 --- a/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java +++ b/api/src/main/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdvice.java @@ -21,8 +21,8 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.transaction.PlatformTransactionManager; +import org.springframework.transaction.TransactionDefinition; import org.springframework.transaction.TransactionStatus; -import org.springframework.transaction.support.DefaultTransactionDefinition; /** * Purges the queue entries of a visit before {@link org.openmrs.api.VisitService#purgeVisit} @@ -36,20 +36,15 @@ *

* The cascade and core's own delete run in one transaction, taken out here, because the entries * must not go without the visit. {@code purgeVisit} refuses a visit that still has encounters, and - * it does so after this advice has run, so the two have to stand or fall together. Propagation is - * the default, so this joins a caller's transaction where there is one and starts its own where - * there is not. + * it does so after this advice has run, so the two have to stand or fall together. *

- * It has to be taken out here rather than inherited from the service, and the reason is easy to get - * backwards. {@code applicationContext-service.xml} does declare {@code visitService} as a - * {@code TransactionProxyFactoryBean} (line 515), but that proxy's own transaction advisor is inert - * for {@code purgeVisit}: its target is the proxy that the {@code DefaultAdvisorAutoProxyCreator} - * and {@code TransactionAttributeSourceAdvisor} pair (lines 46 and 52) has already wrapped around - * {@code visitServiceTarget}, and that target is a JDK proxy of the {@code VisitService} interface, - * which carries no {@code @Transactional} for the attribute source to find. The transaction - * therefore begins in the inner proxy, a layer below where {@code Context.addAdvice} appends this - * advice, and without the one taken out here nothing the cascade did would be rolled back with the - * refused delete. + * The transaction is taken out here rather than relied upon, because {@code Context.addAdvice} + * gives no guarantee that one is open by the time the advice runs. Whether one is depends on how + * the platform wraps {@code visitService}, which is not this module's to depend on and has already + * changed between supported versions. Propagation is the default, so this joins a transaction where + * the platform provides one and opens its own where it does not. Do not remove it on the strength + * of a platform that provides one: {@code VisitWithQueueEntriesDeleteAdviceTransactionTest} is what + * says whether the entries survive a refused delete, and it is green either way on such a platform. */ public class VisitWithQueueEntriesDeleteAdvice implements MethodInterceptor { @@ -71,7 +66,7 @@ public Object invoke(MethodInvocation invocation) throws Throwable { // instance, so a field would outlive the context the bean came from PlatformTransactionManager transactionManager = Context.getRegisteredComponent("transactionManager", PlatformTransactionManager.class); - TransactionStatus transaction = transactionManager.getTransaction(new DefaultTransactionDefinition()); + TransactionStatus transaction = transactionManager.getTransaction(TransactionDefinition.withDefaults()); Object result; try { purgeQueueEntries(visit); @@ -82,8 +77,9 @@ public Object invoke(MethodInvocation invocation) throws Throwable { transactionManager.rollback(transaction); } catch (RuntimeException | Error rollbackFailure) { - // keep hold of t: it is the only thing that says why the purge was refused, and the - // rollback failure is what the caller is about to see instead + // t is the only thing that says why the purge was refused, and the rollback failure is + // what propagates in its place. Suppressing it keeps it in the printed stack trace, so + // it reaches the server log; error responses built from getCause() will not show it. rollbackFailure.addSuppressed(t); throw rollbackFailure; } diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java index 1815b03d..3ae3e3e7 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTest.java @@ -57,8 +57,7 @@ public void setup() { INITIAL_DATASET_XML.forEach(this::executeDataSet); visit = queueEntryService.getQueueEntryById(3).get().getVisit(); // the module test harness does not register advice from config.xml, so register it here to - // exercise the same interceptor chain that production purges go through (ModuleAdviceConfigTest - // covers the config.xml declaration itself) + // exercise the same interceptor chain that production purges go through Context.addAdvice(VisitService.class, advice); } diff --git a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java index 1807871b..a658fd20 100644 --- a/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java +++ b/integration-tests/src/test/java/org/openmrs/module/queue/api/VisitWithQueueEntriesDeleteAdviceTransactionTest.java @@ -78,17 +78,15 @@ public class VisitWithQueueEntriesDeleteAdviceTransactionTest extends BaseModule public void setup() { INITIAL_DATASET_XML.forEach(this::executeDataSet); // the module test harness does not register advice from config.xml, so register it here to - // exercise the same interceptor chain that production purges go through (ModuleAdviceConfigTest - // covers the config.xml declaration itself) + // exercise the same interceptor chain that production purges go through Context.addAdvice(VisitService.class, advice); } @After public void tearDown() { Context.removeAdvice(VisitService.class, advice); - // Nothing this test wrote was rolled back. Core wipes after the last method of a class anyway, - // so this is here for the second test method: without it that method's executeDataSet would - // meet the rows this one committed. + // This test runs outside a transaction, so everything it wrote is committed, including an + // encounter that no dataset would replace. Wipe it rather than leave it to whatever runs next. deleteAllData(); } diff --git a/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java b/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java index f31cc694..b562f867 100644 --- a/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java +++ b/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java @@ -9,18 +9,13 @@ */ package org.openmrs.module.queue; -import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; -import java.io.InputStream; -import java.net.URL; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; +import java.io.File; import org.aopalliance.aop.Advice; import org.junit.Test; @@ -32,15 +27,19 @@ /** * The {@code } elements in config.xml are what registers this module's AOP advice in a * deployed server, and this test is the build's only check on them: module tests register advice - * themselves rather than going through config.xml. Getting one wrong fails quietly rather than - * loudly: {@code AdvicePoint.getClassInstance} catches the reflection failure and logs a warning, - * {@code ModuleFactory.loadAdvice} then logs at debug and carries on, and the module starts with - * the cascade simply absent. + * themselves rather than going through config.xml. + *

+ * Two of the three things checked here fail quietly in a server. A {@code } that will not + * load leaves {@code ModuleFactory.loadAdvice} logging a warning, and one that will not instantiate + * leaves {@code AdvicePoint.getClassInstance} returning null and {@code loadAdvice} logging at + * debug; either way the module starts with the cascade simply absent. The type check is the + * opposite: {@code loadAdvice} casts to {@code Advice} without testing, catching only + * {@code ClassNotFoundException} and {@code NoClassDefFoundError}, and its caller in + * {@code ModuleUtil.refreshApplicationContext} has no catch at all, so a wrong type aborts the + * post-refresh loop for every started module rather than just this one. */ public class ModuleAdviceConfigTest { - private static final String MODULE_PACKAGE = "org.openmrs.module.queue"; - @Test public void everyAdvicePointAndClassInConfigXmlShouldBeUsable() throws Exception { NodeList adviceElements = queueConfigXml().getElementsByTagName("advice"); @@ -64,7 +63,7 @@ public void everyAdvicePointAndClassInConfigXmlShouldBeUsable() throws Exception } catch (NoSuchMethodException e) { throw new AssertionError( - adviceClassName + " needs a public no-arg constructor for AdvicePoint to" + " instantiate it", e); + adviceClassName + " needs a public no-arg constructor for AdvicePoint to instantiate it", e); } assertTrue( adviceClassName + " must implement " + Advice.class.getName() + " or " + Advisor.class.getName() @@ -74,25 +73,23 @@ public void everyAdvicePointAndClassInConfigXmlShouldBeUsable() throws Exception } /** - * Reads this module's packaged config.xml, picked out by its {@code } rather than by - * taking the first {@code config.xml} on the classpath, because required modules ship one too. + * Reads this module's own config.xml from the source tree. Reading it off the classpath instead + * would mean parsing every {@code config.xml} there to find this one, and a required module ships + * one: a malformed or entity-bearing document of somebody else's then decides whether this test + * passes. Nothing in the {@code } elements is Maven-filtered, so the source copy and the + * packaged copy say the same thing. */ private Document queueConfigXml() throws Exception { + // surefire runs with the module directory as its working directory + File configXml = new File("src/main/resources/config.xml"); + assertTrue("expected to find " + configXml.getAbsolutePath(), configXml.isFile()); + DocumentBuilderFactory factory = DocumentBuilderFactory.newInstance(); factory.setFeature("http://apache.org/xml/features/nonvalidating/load-external-dtd", false); + factory.setFeature("http://xml.org/sax/features/external-general-entities", false); + factory.setExpandEntityReferences(false); DocumentBuilder builder = factory.newDocumentBuilder(); - - List ours = new ArrayList<>(); - for (URL url : Collections.list(getClass().getClassLoader().getResources("config.xml"))) { - try (InputStream in = url.openStream()) { - Document document = builder.parse(in); - if (MODULE_PACKAGE.equals(childText(document.getDocumentElement(), "package"))) { - ours.add(document); - } - } - } - assertEquals("expected exactly one config.xml declaring package " + MODULE_PACKAGE, 1, ours.size()); - return ours.get(0); + return builder.parse(configXml); } private static String childText(Element parent, String tagName) { From 2546dafa6911434ac3e685e3fd683280e880bbc3 Mon Sep 17 00:00:00 2001 From: dkayiwa Date: Wed, 16 Sep 2026 20:07:40 +0300 Subject: [PATCH 11/12] O3-5459: Stop attributing core's log lines, and state the filtering constraint Phase 2 pass 3. Three of four lenses found nothing; the fourth and a second one found the same thing, in a javadoc the previous pass had just introduced. That javadoc said a which will not load leaves loadAdvice logging a warning, and one which will not instantiate leaves getClassInstance returning null and loadAdvice logging at debug. Both halves are wrong and the distinction does not exist: AdvicePoint.getClassInstance wraps the class load and the newInstance in one catch, logs its own warning keyed on the rather than the class name, and returns null, so both failures take the same path. loadAdvice's warning is reachable only from Context.loadClass(advice.getPoint()) - a bad , which that sentence never mentioned. It also contradicted the file's own inline comment twenty lines below, which had it right all along. That is the second refuted attempt at describing what core logs when part of an is wrong, so the attribution is gone rather than corrected again. What stays is the part three lenses and a reading of core agree on: loadAdvice casts to Advice without testing and ModuleUtil.refreshApplicationContext has no catch, so a wrong type propagates out of the post-refresh loop. Narrowed from "every started module" to the ones behind this one, since those already iterated have their advice. Also: reading config.xml from the source tree requires the elements to stay free of the ${...} tokens the rest of the file uses, because Maven resolves those in the packaged copy and this test does not. The twelve lines above uses exactly that style, so say it is a constraint instead of assuming nobody follows the neighbour. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012KvveaJ2rSEqEgxHrUPZU4 --- .../module/queue/ModuleAdviceConfigTest.java | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java b/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java index b562f867..899c97da 100644 --- a/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java +++ b/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java @@ -29,14 +29,13 @@ * deployed server, and this test is the build's only check on them: module tests register advice * themselves rather than going through config.xml. *

- * Two of the three things checked here fail quietly in a server. A {@code } that will not - * load leaves {@code ModuleFactory.loadAdvice} logging a warning, and one that will not instantiate - * leaves {@code AdvicePoint.getClassInstance} returning null and {@code loadAdvice} logging at - * debug; either way the module starts with the cascade simply absent. The type check is the - * opposite: {@code loadAdvice} casts to {@code Advice} without testing, catching only - * {@code ClassNotFoundException} and {@code NoClassDefFoundError}, and its caller in - * {@code ModuleUtil.refreshApplicationContext} has no catch at all, so a wrong type aborts the - * post-refresh loop for every started module rather than just this one. + * The checks below mirror what {@code ModuleFactory.loadAdvice} and + * {@code AdvicePoint.getClassInstance} do with the two values, so that a config.xml this build + * accepts is one a server can load. The type check earns its place twice over: {@code loadAdvice} + * casts to {@code Advice} without testing, catching only {@code ClassNotFoundException} and + * {@code NoClassDefFoundError}, and its caller in {@code ModuleUtil.refreshApplicationContext} has + * no catch at all, so a wrong type propagates out of the post-refresh loop and the started modules + * behind this one never get their advice loaded. */ public class ModuleAdviceConfigTest { @@ -76,8 +75,9 @@ public void everyAdvicePointAndClassInConfigXmlShouldBeUsable() throws Exception * Reads this module's own config.xml from the source tree. Reading it off the classpath instead * would mean parsing every {@code config.xml} there to find this one, and a required module ships * one: a malformed or entity-bearing document of somebody else's then decides whether this test - * passes. Nothing in the {@code } elements is Maven-filtered, so the source copy and the - * packaged copy say the same thing. + * passes. The source copy is the one a typo lands in, and reading it requires the {@code } + * elements to stay free of the {@code ${...}} tokens the rest of the file uses, since Maven would + * resolve those in the packaged copy and this test would not. */ private Document queueConfigXml() throws Exception { // surefire runs with the module directory as its working directory From 57e8eec6934843bf0ff940fd30fe16f01097538d Mon Sep 17 00:00:00 2001 From: dkayiwa Date: Wed, 16 Sep 2026 21:35:38 +0300 Subject: [PATCH 12/12] O3-5459: Guard the advice point, not just that it loads Cycle 2. Three lenses found nothing; one found that ModuleAdviceConfigTest gave a single check - that the class loads - while giving three, and demonstrated what that lets past. With org.openmrs.api.impl.VisitServiceImpl and a correct , the test exits 0. Driving the real registration path, Context.addAdvice(VisitServiceImpl.class, advice) throws NullPointerException from ServiceContext.addAdvice, which does services.get(cls) with no null check on a map keyed by the registered service interface. That escapes loadAdvice's ClassNotFoundException | NoClassDefFoundError catch and the catch-less post-refresh loop in ModuleUtil, so naming the implementation has the same blast radius as a wrong advice type: the started modules behind this one never get their advice loaded. Interface-for-implementation is the typo this invites, and a deployed server is where it would surface. So the point must be an interface. That is necessary and not sufficient - only a running Context could say whether it is a registered service - and the comment says so rather than implying the check is complete. Also check the advice class is not abstract. AdvicePoint calls newInstance(), so an abstract class fails there and quietly leaves the cascade absent, but it passes both a getConstructor() lookup and the Advisor test: AbstractPointcutAdvisor is abstract, is an Advisor, and has a public no-arg constructor. Testing the modifier keeps the constructor looked up rather than called, so an advice that legitimately touches Context on construction is still not failed for it. Both controls redden on exactly those two classes, and the slice is green against core 2.8.7 as well as the 2.7.4 the pom defaults to. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012KvveaJ2rSEqEgxHrUPZU4 --- .../module/queue/ModuleAdviceConfigTest.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java b/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java index 899c97da..ab37cb71 100644 --- a/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java +++ b/omod/src/test/java/org/openmrs/module/queue/ModuleAdviceConfigTest.java @@ -9,6 +9,7 @@ */ package org.openmrs.module.queue; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; @@ -16,6 +17,7 @@ import javax.xml.parsers.DocumentBuilderFactory; import java.io.File; +import java.lang.reflect.Modifier; import org.aopalliance.aop.Advice; import org.junit.Test; @@ -51,12 +53,21 @@ public void everyAdvicePointAndClassInConfigXmlShouldBeUsable() throws Exception assertNotNull("advice needs a point", point); assertNotNull("advice needs a class", adviceClassName); + // The point is the service interface ServiceContext keys its map by. Context.addAdvice does + // services.get(cls) with no null check, so naming an implementation here throws out of the + // same post-refresh loop a wrong advice type does. Being an interface is necessary and not + // sufficient; only a running Context could say whether it is a registered service. + Class pointClass = Class.forName(point); + assertTrue(point + " must be the service interface, not an implementation class", pointClass.isInterface()); + // AdvicePoint.getClassInstance loads the class and calls its public no-arg constructor, and // ModuleFactory.loadAdvice then registers the instance as an Advisor if it is one and as an // Advice otherwise, so either type is legal here. The constructor is looked up rather than - // called, so an advice that legitimately touches Context on construction is not failed for it. - Class.forName(point); + // called, so an advice that legitimately touches Context on construction is not failed for it, + // which is why abstractness is checked separately instead of by trying to instantiate. Class adviceClass = Class.forName(adviceClassName); + assertFalse(adviceClassName + " must not be abstract for AdvicePoint to instantiate it", + Modifier.isAbstract(adviceClass.getModifiers())); try { adviceClass.getConstructor(); }