diff --git a/fineract-core/src/main/java/org/apache/fineract/commands/service/SynchronousCommandProcessingService.java b/fineract-core/src/main/java/org/apache/fineract/commands/service/SynchronousCommandProcessingService.java index a13835b6266..db6517ba8f9 100644 --- a/fineract-core/src/main/java/org/apache/fineract/commands/service/SynchronousCommandProcessingService.java +++ b/fineract-core/src/main/java/org/apache/fineract/commands/service/SynchronousCommandProcessingService.java @@ -62,6 +62,8 @@ import org.apache.fineract.useradministration.domain.AppUser; import org.springframework.context.ApplicationContext; import org.springframework.stereotype.Service; +import org.springframework.transaction.support.TransactionSynchronization; +import org.springframework.transaction.support.TransactionSynchronizationManager; @Service @Slf4j @@ -197,8 +199,23 @@ private CommandProcessingResult executeCommand(final CommandWrapper wrapper, fin } result.setRollbackTransaction(null); - publishHookEvent(wrapper.entityName(), wrapper.actionName(), command, result); // TODO must be performed in a - // new transaction + + // When running inside an enclosing batch transaction, defer hook publication + // until after the transaction commits. This prevents webhooks from firing for + // commands that are subsequently rolled back when a later command in the batch + // fails (e.g. a withdrawal succeeds but its fee charge fails, rolling back both). + if (isEnclosingTransaction && TransactionSynchronizationManager.isSynchronizationActive()) { + TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() { + + @Override + public void afterCommit() { + publishHookEvent(wrapper.entityName(), wrapper.actionName(), command, result); + } + }); + } else { + publishHookEvent(wrapper.entityName(), wrapper.actionName(), command, result); + } + return result; } diff --git a/fineract-provider/src/test/java/org/apache/fineract/commands/service/SynchronousCommandProcessingServiceTest.java b/fineract-provider/src/test/java/org/apache/fineract/commands/service/SynchronousCommandProcessingServiceTest.java index 7bcb2b7d4f2..fa67f353840 100644 --- a/fineract-provider/src/test/java/org/apache/fineract/commands/service/SynchronousCommandProcessingServiceTest.java +++ b/fineract-provider/src/test/java/org/apache/fineract/commands/service/SynchronousCommandProcessingServiceTest.java @@ -20,6 +20,7 @@ import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.anyString; @@ -27,6 +28,7 @@ import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -52,6 +54,7 @@ import org.apache.fineract.infrastructure.core.api.JsonCommand; import org.apache.fineract.infrastructure.core.config.FineractProperties; import org.apache.fineract.infrastructure.core.data.CommandProcessingResult; +import org.apache.fineract.infrastructure.core.domain.BatchRequestContextHolder; import org.apache.fineract.infrastructure.core.domain.FineractRequestContextHolder; import org.apache.fineract.infrastructure.core.exception.IdempotentCommandProcessUnderProcessingException; import org.apache.fineract.infrastructure.core.serialization.ToApiJsonSerializer; @@ -67,6 +70,7 @@ import org.mockito.Spy; import org.springframework.context.ApplicationContext; import org.springframework.lang.NonNull; +import org.springframework.transaction.support.TransactionSynchronizationManager; import org.springframework.web.context.request.RequestContextHolder; import org.springframework.web.context.request.ServletRequestAttributes; @@ -598,4 +602,103 @@ public void testExecuteCommandWithMaxRetryFailure() { underTest.executeCommand(commandWrapper, jsonCommand, false); verify(commandSourceService, times(4)).getCommandSource(commandId); } + + /** + * Test that when running inside an enclosing batch transaction, hook events are NOT published immediately but + * deferred to afterCommit. This prevents webhooks (e.g. SMS notifications) from firing for commands that are + * subsequently rolled back when a later command in the batch fails. + */ + @Test + public void testHookEventDeferredInEnclosingTransaction() { + CommandWrapper commandWrapper = getCommandWrapper(); + + long commandId = 1L; + JsonCommand jsonCommand = Mockito.mock(JsonCommand.class); + when(jsonCommand.commandId()).thenReturn(commandId); + + NewCommandSourceHandler commandHandler = Mockito.mock(NewCommandSourceHandler.class); + CommandProcessingResult commandProcessingResult = Mockito.mock(CommandProcessingResult.class); + when(commandProcessingResult.isRollbackTransaction()).thenReturn(false); + when(commandHandler.processCommand(jsonCommand)).thenReturn(commandProcessingResult); + when(commandHandlerProvider.getHandler(Mockito.any(), Mockito.any())).thenReturn(commandHandler); + + when(configurationDomainService.isMakerCheckerEnabledForTask(Mockito.any())).thenReturn(false); + String idk = "idk"; + when(idempotencyKeyResolver.resolve(commandWrapper)).thenReturn(idk); + CommandSource commandSource = Mockito.mock(CommandSource.class); + when(commandSource.getId()).thenReturn(commandId); + when(commandSourceService.findCommandSource(commandWrapper, idk)).thenReturn(null); + when(commandSourceService.getCommandSource(commandId)).thenReturn(commandSource); + + AppUser appUser = Mockito.mock(AppUser.class); + when(context.authenticatedUser(Mockito.any(CommandWrapper.class))).thenReturn(appUser); + when(commandSourceService.saveInitialNewTransaction(commandWrapper, jsonCommand, appUser, idk)).thenReturn(commandSource); + when(commandSourceService.saveResultSameTransaction(commandSource)).thenReturn(commandSource); + when(commandSource.getStatus()).thenReturn(CommandProcessingResultType.PROCESSED.getValue()); + + when(commandSourceService.processCommand(commandHandler, jsonCommand, commandSource, appUser, false)) + .thenReturn(commandProcessingResult); + + // Simulate enclosing batch transaction with active synchronization + BatchRequestContextHolder.setIsEnclosingTransaction(true); + TransactionSynchronizationManager.initSynchronization(); + try { + underTest.executeCommand(commandWrapper, jsonCommand, false); + + // Hook event should NOT have been published immediately + verify(applicationContext, never()).publishEvent(any()); + + // It should be registered as an afterCommit synchronization + assertEquals(1, TransactionSynchronizationManager.getSynchronizations().size()); + } finally { + TransactionSynchronizationManager.clearSynchronization(); + BatchRequestContextHolder.resetIsEnclosingTransaction(); + } + } + + /** + * Test that when NOT in an enclosing transaction, no TransactionSynchronization is registered. The code takes the + * immediate path (not the deferred path), preserving existing behaviour for non-batch single-request commands. + */ + @Test + public void testHookEventNotDeferredOutsideEnclosingTransaction() { + CommandWrapper commandWrapper = getCommandWrapper(); + + long commandId = 1L; + JsonCommand jsonCommand = Mockito.mock(JsonCommand.class); + when(jsonCommand.commandId()).thenReturn(commandId); + when(jsonCommand.json()).thenReturn(null); // null causes publishHookEvent to exit early; avoids mocking the + // full hook-serialisation chain + + NewCommandSourceHandler commandHandler = Mockito.mock(NewCommandSourceHandler.class); + CommandProcessingResult commandProcessingResult = Mockito.mock(CommandProcessingResult.class); + when(commandProcessingResult.isRollbackTransaction()).thenReturn(false); + when(commandHandler.processCommand(jsonCommand)).thenReturn(commandProcessingResult); + when(commandHandlerProvider.getHandler(Mockito.any(), Mockito.any())).thenReturn(commandHandler); + + when(configurationDomainService.isMakerCheckerEnabledForTask(Mockito.any())).thenReturn(false); + String idk = "idk"; + when(idempotencyKeyResolver.resolve(commandWrapper)).thenReturn(idk); + CommandSource commandSource = Mockito.mock(CommandSource.class); + when(commandSource.getId()).thenReturn(commandId); + when(commandSourceService.findCommandSource(commandWrapper, idk)).thenReturn(null); + when(commandSourceService.getCommandSource(commandId)).thenReturn(commandSource); + + AppUser appUser = Mockito.mock(AppUser.class); + when(context.authenticatedUser(Mockito.any(CommandWrapper.class))).thenReturn(appUser); + when(commandSourceService.saveInitialNewTransaction(commandWrapper, jsonCommand, appUser, idk)).thenReturn(commandSource); + when(commandSourceService.saveResultSameTransaction(commandSource)).thenReturn(commandSource); + when(commandSource.getStatus()).thenReturn(CommandProcessingResultType.PROCESSED.getValue()); + + when(commandSourceService.processCommand(commandHandler, jsonCommand, commandSource, appUser, false)) + .thenReturn(commandProcessingResult); + + // Not in enclosing transaction (default) + BatchRequestContextHolder.resetIsEnclosingTransaction(); + + underTest.executeCommand(commandWrapper, jsonCommand, false); + + // The deferred path was not taken: no TransactionSynchronization should have been registered. + assertFalse(TransactionSynchronizationManager.isSynchronizationActive()); + } }