From 2c77f6a277a6158cbdf12ff0b41112356a55227d Mon Sep 17 00:00:00 2001 From: budaidev Date: Tue, 10 Jun 2025 11:01:32 +0200 Subject: [PATCH] FINERACT-2304: Avoid retrying after the command succeed --- .../RetryConfigurationAssembler.java | 11 ++ .../CommandResultPersistenceException.java | 26 +++ .../SynchronousCommandProcessingService.java | 101 ++++++++---- ...nchronousCommandProcessingServiceTest.java | 148 ++++++++++++++++-- 4 files changed, 237 insertions(+), 49 deletions(-) create mode 100644 fineract-core/src/main/java/org/apache/fineract/commands/exception/CommandResultPersistenceException.java diff --git a/fineract-core/src/main/java/org/apache/fineract/commands/configuration/RetryConfigurationAssembler.java b/fineract-core/src/main/java/org/apache/fineract/commands/configuration/RetryConfigurationAssembler.java index fb0fc05ba..b79223ee1 100644 --- a/fineract-core/src/main/java/org/apache/fineract/commands/configuration/RetryConfigurationAssembler.java +++ b/fineract-core/src/main/java/org/apache/fineract/commands/configuration/RetryConfigurationAssembler.java @@ -25,6 +25,7 @@ import io.github.resilience4j.retry.RetryRegistry; import java.util.Arrays; import lombok.AllArgsConstructor; import org.apache.fineract.batch.service.BatchExecutionException; +import org.apache.fineract.commands.exception.CommandResultPersistenceException; import org.apache.fineract.infrastructure.core.config.FineractProperties; import org.apache.fineract.infrastructure.core.domain.FineractRequestContextHolder; import org.springframework.stereotype.Service; @@ -35,6 +36,7 @@ public class RetryConfigurationAssembler { public static final String EXECUTE_COMMAND = "executeCommand"; public static final String BATCH_RETRY = "batchRetry"; + public static final String COMMAND_RESULT_PERSISTENCE = "commandResultPersistence"; private static final String LAST_EXECUTION_EXCEPTION_KEY = "LAST_EXECUTION_EXCEPTION"; private final RetryRegistry registry; private final FineractProperties fineractProperties; @@ -107,4 +109,13 @@ public class RetryConfigurationAssembler { return configBuilder; } + + public Retry getRetryConfigurationForCommandResultPersistence() { + RetryConfig.Builder configBuilder = buildCommonExecuteCommandConfiguration(); + + configBuilder.retryOnException(e -> e instanceof RuntimeException && !(e instanceof CommandResultPersistenceException)); + + RetryConfig config = configBuilder.build(); + return registry.retry(COMMAND_RESULT_PERSISTENCE, config); + } } diff --git a/fineract-core/src/main/java/org/apache/fineract/commands/exception/CommandResultPersistenceException.java b/fineract-core/src/main/java/org/apache/fineract/commands/exception/CommandResultPersistenceException.java new file mode 100644 index 000000000..a20b01807 --- /dev/null +++ b/fineract-core/src/main/java/org/apache/fineract/commands/exception/CommandResultPersistenceException.java @@ -0,0 +1,26 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.fineract.commands.exception; + +public class CommandResultPersistenceException extends RuntimeException { + + public CommandResultPersistenceException(String message, Throwable cause) { + super(message, cause); + } +} 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 656dfaa05..bef99cb4d 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 @@ -24,6 +24,7 @@ import static org.apache.http.HttpStatus.SC_OK; import com.google.gson.Gson; import com.google.gson.reflect.TypeToken; +import io.github.resilience4j.retry.Retry; import java.lang.reflect.Type; import java.time.Instant; import java.util.ArrayList; @@ -37,6 +38,7 @@ import org.apache.fineract.commands.configuration.RetryConfigurationAssembler; import org.apache.fineract.commands.domain.CommandProcessingResultType; import org.apache.fineract.commands.domain.CommandSource; import org.apache.fineract.commands.domain.CommandWrapper; +import org.apache.fineract.commands.exception.CommandResultPersistenceException; import org.apache.fineract.commands.exception.UnsupportedCommandException; import org.apache.fineract.commands.handler.NewCommandSourceHandler; import org.apache.fineract.commands.provider.CommandHandlerProvider; @@ -132,43 +134,74 @@ public class SynchronousCommandProcessingService implements CommandProcessingSer setIdempotencyKeyStoreFlag(true); - final CommandProcessingResult result; - try { - result = commandSourceService.processCommand(findCommandHandler(wrapper), command, commandSource, user, - isApprovedByChecker); - } catch (Throwable t) { // NOSONAR - RuntimeException mappable = ErrorHandler.getMappable(t); - ErrorInfo errorInfo = commandSourceService.generateErrorInfo(mappable); - Integer statusCode = errorInfo.getStatusCode(); - commandSource.setResultStatusCode(statusCode); - commandSource.setResult(errorInfo.getMessage()); - if (statusCode != SC_OK) { - commandSource.setStatus(ERROR); - } - if (!isEnclosingTransaction) { // TODO: temporary solution - commandSource = commandSourceService.saveResultNewTransaction(commandSource); - } - // must not throw any exception; must persist in new transaction as the current transaction was already - // marked as rollback - publishHookErrorEvent(wrapper, command, errorInfo); - throw mappable; - } - - commandSource.setResultStatusCode(SC_OK); - commandSource.updateForAudit(result); - commandSource.setResult(toApiResultJsonSerializer.serializeResult(result)); - commandSource.setStatus(PROCESSED); - commandSource = commandSourceService.saveResultSameTransaction(commandSource); - storeCommandIdInContext(commandSource); // Store command id as a request attribute - - result.setRollbackTransaction(null); - publishHookEvent(wrapper.entityName(), wrapper.actionName(), command, result); // TODO must be performed in - // a - // new transaction - return result; + return executeCommand(wrapper, command, isApprovedByChecker, commandSource, user, isEnclosingTransaction); }); } + private CommandProcessingResult executeCommand(final CommandWrapper wrapper, final JsonCommand command, + final boolean isApprovedByChecker, CommandSource commandSource, AppUser user, boolean isEnclosingTransaction) { + + final CommandProcessingResult result; + try { + result = commandSourceService.processCommand(findCommandHandler(wrapper), command, commandSource, user, isApprovedByChecker); + } catch (Throwable t) { // NOSONAR + RuntimeException mappable = ErrorHandler.getMappable(t); + ErrorInfo errorInfo = commandSourceService.generateErrorInfo(mappable); + Integer statusCode = errorInfo.getStatusCode(); + commandSource.setResultStatusCode(statusCode); + commandSource.setResult(errorInfo.getMessage()); + if (statusCode != SC_OK) { + commandSource.setStatus(ERROR); + } + if (!isEnclosingTransaction) { // TODO: temporary solution + commandSourceService.saveResultNewTransaction(commandSource); + } + // must not throw any exception; must persist in new transaction as the current transaction was already + // marked as rollback + publishHookErrorEvent(wrapper, command, errorInfo); + throw mappable; + } + + Retry persistenceRetry = retryConfigurationAssembler.getRetryConfigurationForCommandResultPersistence(); + + try { + CommandSource finalCommandSource = commandSource; + CommandSource savedCommandSource = persistenceRetry.executeSupplier(() -> { + // Get metrics for logging + long attemptNumber = persistenceRetry.getMetrics().getNumberOfFailedCallsWithRetryAttempt() + 1; + + // Critical: Refetch on retry attempts (not on first attempt) + CommandSource currentSource = finalCommandSource; + if (attemptNumber > 1) { + log.info("Retrying command result save - attempt {} for command ID {}", attemptNumber, finalCommandSource.getId()); + currentSource = commandSourceService.getCommandSource(finalCommandSource.getId()); + } + + // Update command source with results + currentSource.setResultStatusCode(SC_OK); + currentSource.updateForAudit(result); + currentSource.setResult(toApiResultJsonSerializer.serializeResult(result)); + currentSource.setStatus(PROCESSED); + + // Return saved command source + return commandSourceService.saveResultSameTransaction(currentSource); + }); + + // Command successfully saved + storeCommandIdInContext(savedCommandSource); + + } catch (Exception e) { + // After all retries have been exhausted + log.error("Failed to persist command result after multiple retries for command ID {}", commandSource.getId(), e); + throw new CommandResultPersistenceException("Failed to persist command result after multiple retries", e); + } + + result.setRollbackTransaction(null); + publishHookEvent(wrapper.entityName(), wrapper.actionName(), command, result); // TODO must be performed in a + // new transaction + return result; + } + private void storeCommandIdInContext(CommandSource savedCommandSource) { if (savedCommandSource.getId() == null) { throw new IllegalStateException("Command source not saved"); 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 af59e49da..31cefcbd7 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,8 +20,11 @@ package org.apache.fineract.commands.service; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.any; +import static org.mockito.Mockito.atLeast; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; @@ -35,11 +38,13 @@ import io.github.resilience4j.retry.RetryRegistry; import jakarta.servlet.http.HttpServletRequest; import java.time.Duration; import java.util.Map; +import java.util.concurrent.atomic.AtomicInteger; import org.apache.fineract.batch.exception.ErrorInfo; import org.apache.fineract.commands.configuration.RetryConfigurationAssembler; import org.apache.fineract.commands.domain.CommandProcessingResultType; import org.apache.fineract.commands.domain.CommandSource; import org.apache.fineract.commands.domain.CommandWrapper; +import org.apache.fineract.commands.exception.CommandResultPersistenceException; import org.apache.fineract.commands.handler.NewCommandSourceHandler; import org.apache.fineract.commands.provider.CommandHandlerProvider; import org.apache.fineract.infrastructure.configuration.domain.ConfigurationDomainService; @@ -52,6 +57,7 @@ import org.apache.fineract.infrastructure.core.exception.PlatformApiDataValidati import org.apache.fineract.infrastructure.core.serialization.ToApiJsonSerializer; import org.apache.fineract.infrastructure.security.service.PlatformSecurityContext; import org.apache.fineract.useradministration.domain.AppUser; +import org.jetbrains.annotations.NotNull; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -128,6 +134,9 @@ public class SynchronousCommandProcessingServiceTest { var impl = new RetryConfigurationAssembler(retryRegistry, fineractProperties, fineractRequestContextHolder); var retry = impl.getRetryConfigurationForExecuteCommand(); when(retryConfigurationAssembler.getRetryConfigurationForExecuteCommand()).thenReturn(retry); + + var persistenceRetry = impl.getRetryConfigurationForCommandResultPersistence(); + when(retryConfigurationAssembler.getRetryConfigurationForCommandResultPersistence()).thenReturn(persistenceRetry); } @AfterEach @@ -145,11 +154,7 @@ public class SynchronousCommandProcessingServiceTest { @Test public void testExecuteCommandSuccessAfter2Fails() { - CommandWrapper commandWrapper = Mockito.mock(CommandWrapper.class); - when(commandWrapper.isDatatableResource()).thenReturn(false); - when(commandWrapper.isNoteResource()).thenReturn(false); - when(commandWrapper.isSurveyResource()).thenReturn(false); - when(commandWrapper.isLoanDisburseDetailResource()).thenReturn(false); + CommandWrapper commandWrapper = getCommandWrapper(); long commandId = 1L; JsonCommand jsonCommand = Mockito.mock(JsonCommand.class); @@ -371,11 +376,7 @@ public class SynchronousCommandProcessingServiceTest { @Test public void testExecuteCommandSuccess() { - CommandWrapper commandWrapper = Mockito.mock(CommandWrapper.class); - when(commandWrapper.isDatatableResource()).thenReturn(false); - when(commandWrapper.isNoteResource()).thenReturn(false); - when(commandWrapper.isSurveyResource()).thenReturn(false); - when(commandWrapper.isLoanDisburseDetailResource()).thenReturn(false); + CommandWrapper commandWrapper = getCommandWrapper(); long commandId = 1L; JsonCommand jsonCommand = Mockito.mock(JsonCommand.class); @@ -415,11 +416,7 @@ public class SynchronousCommandProcessingServiceTest { @Test public void testExecuteCommandFails() { - CommandWrapper commandWrapper = Mockito.mock(CommandWrapper.class); - when(commandWrapper.isDatatableResource()).thenReturn(false); - when(commandWrapper.isNoteResource()).thenReturn(false); - when(commandWrapper.isSurveyResource()).thenReturn(false); - when(commandWrapper.isLoanDisburseDetailResource()).thenReturn(false); + CommandWrapper commandWrapper = getCommandWrapper(); JsonCommand jsonCommand = Mockito.mock(JsonCommand.class); Long commandId = jsonCommand.commandId(); @@ -457,6 +454,16 @@ public class SynchronousCommandProcessingServiceTest { verify(commandSourceService).generateErrorInfo(runtimeException); } + @NotNull + private static CommandWrapper getCommandWrapper() { + CommandWrapper commandWrapper = Mockito.mock(CommandWrapper.class); + when(commandWrapper.isDatatableResource()).thenReturn(false); + when(commandWrapper.isNoteResource()).thenReturn(false); + when(commandWrapper.isSurveyResource()).thenReturn(false); + when(commandWrapper.isLoanDisburseDetailResource()).thenReturn(false); + return commandWrapper; + } + @Test public void publishHookEventHandlesInvalidJson() { String entityName = "entity"; @@ -473,4 +480,115 @@ public class SynchronousCommandProcessingServiceTest { private static final class RetryException extends RuntimeException {} + @Test + public void testExecuteCommandWithRetry() { + CommandWrapper commandWrapper = getCommandWrapper(); + when(commandWrapper.isInterestPauseResource()).thenReturn(false); + + 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 idempotencyKey = "test-idempotency-key"; + when(idempotencyKeyResolver.resolve(commandWrapper)).thenReturn(idempotencyKey); + + CommandSource commandSource = Mockito.mock(CommandSource.class); + when(commandSource.getId()).thenReturn(commandId); + when(commandSourceService.findCommandSource(commandWrapper, idempotencyKey)).thenReturn(null); + + AppUser appUser = Mockito.mock(AppUser.class); + when(appUser.getId()).thenReturn(1L); + when(context.authenticatedUser(Mockito.any(CommandWrapper.class))).thenReturn(appUser); + + when(commandSourceService.saveInitialNewTransaction(commandWrapper, jsonCommand, appUser, idempotencyKey)) + .thenReturn(commandSource); + when(commandSourceService.processCommand(commandHandler, jsonCommand, commandSource, appUser, false)) + .thenReturn(commandProcessingResult); + + final AtomicInteger saveAttempts = new AtomicInteger(0); + + doAnswer(invocation -> { + int attempt = saveAttempts.incrementAndGet(); + if (attempt == 1) { + throw new RuntimeException("Database error on first attempt"); + } + return commandSource; + }).when(commandSourceService).saveResultSameTransaction(any(CommandSource.class)); + + // When fetching the command source after failure, return the same mock + when(commandSourceService.getCommandSource(commandId)).thenReturn(commandSource); + + // Execute the command + CommandProcessingResult result = underTest.executeCommand(commandWrapper, jsonCommand, false); + + assertEquals(2, saveAttempts.get(), "Expected 2 save attempts"); + + verify(commandSource, atLeast(1)).setResultStatusCode(200); + verify(commandSource, atLeast(1)).updateForAudit(commandProcessingResult); + verify(commandSource, atLeast(1)).setResult(any()); + verify(commandSource, atLeast(1)).setStatus(CommandProcessingResultType.PROCESSED); + + assertEquals(commandProcessingResult, result); + } + + @Test + public void testExecuteCommandWithMaxRetryFailure() { + CommandWrapper commandWrapper = getCommandWrapper(); + when(commandWrapper.isInterestPauseResource()).thenReturn(false); + + 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 idempotencyKey = "test-idempotency-key"; + when(idempotencyKeyResolver.resolve(commandWrapper)).thenReturn(idempotencyKey); + + CommandSource commandSource = Mockito.mock(CommandSource.class); + when(commandSource.getId()).thenReturn(commandId); + when(commandSourceService.findCommandSource(commandWrapper, idempotencyKey)).thenReturn(null); + + AppUser appUser = Mockito.mock(AppUser.class); + when(appUser.getId()).thenReturn(1L); + when(context.authenticatedUser(Mockito.any(CommandWrapper.class))).thenReturn(appUser); + + when(commandSourceService.saveInitialNewTransaction(commandWrapper, jsonCommand, appUser, idempotencyKey)) + .thenReturn(commandSource); + when(commandSourceService.processCommand(commandHandler, jsonCommand, commandSource, appUser, false)) + .thenReturn(commandProcessingResult); + + final AtomicInteger saveAttempts = new AtomicInteger(0); + + // Simulate persistent save failure - all retry attempts fail + RuntimeException persistentException = new RuntimeException("Database error persists"); + doAnswer(invocation -> { + // Count the number of attempts + saveAttempts.incrementAndGet(); + // Always throw the exception to trigger retry + throw persistentException; + }).when(commandSourceService).saveResultSameTransaction(any(CommandSource.class)); + + when(commandSourceService.getCommandSource(commandId)).thenReturn(commandSource); + + CommandResultPersistenceException exception = assertThrows(CommandResultPersistenceException.class, () -> { + underTest.executeCommand(commandWrapper, jsonCommand, false); + }); + + assertEquals(persistentException, exception.getCause()); + + assertTrue(saveAttempts.get() >= 3, "Expected at least 3 save attempts, but got: " + saveAttempts.get()); + } }