Skip to content

FINERACT-2304 Fix auditing of failed batch request while enclosing transaction got enabled #4791

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@
import org.apache.fineract.batch.exception.ErrorInfo;
import org.apache.fineract.batch.service.ResolutionHelper.BatchRequestNode;
import org.apache.fineract.commands.configuration.RetryConfigurationAssembler;
import org.apache.fineract.commands.domain.CommandSource;
import org.apache.fineract.commands.service.CommandSourceService;
import org.apache.fineract.infrastructure.core.domain.BatchRequestContextHolder;
import org.apache.fineract.infrastructure.core.exception.ErrorHandler;
import org.apache.fineract.infrastructure.core.filters.BatchCallHandler;
Expand Down Expand Up @@ -94,6 +96,8 @@ public class BatchApiServiceImpl implements BatchApiService {

private final RetryConfigurationAssembler retryConfigurationAssembler;

private final CommandSourceService commandSourceService;

private EntityManager entityManager;

/**
Expand Down Expand Up @@ -166,9 +170,11 @@ private List<BatchResponse> callInTransaction(Consumer<TransactionTemplate> tran
try {
return retryingBatch.get();
} catch (TransactionException | NonTransientDataAccessException ex) {
saveFailedCommandSourceEntries(ex);
return buildErrorResponses(ex, responseList);
} catch (BatchExecutionException ex) {
log.error("Exception during the batch request processing", ex);
saveFailedCommandSourceEntries(ex.getCause());
responseList.add(buildErrorResponse(ex.getCause(), ex.getRequest()));
return responseList;
}
Expand Down Expand Up @@ -395,4 +401,17 @@ private BatchResponse buildErrorResponse(Long requestId, Integer statusCode, Str
public void setEntityManager(EntityManager entityManager) {
this.entityManager = entityManager;
}

private void saveFailedCommandSourceEntries(final Throwable ex) {
try {
final List<CommandSource> commandSources = BatchRequestContextHolder.getCommandSources();
if (!commandSources.isEmpty()) {
final String errorMessage = ex != null ? ex.getMessage() : "Batch processing failed";
log.info("Saving {} failed entries for batch audit with error: {}", commandSources.size(), errorMessage);
commandSourceService.saveFailedCommandSourcesNewTransaction(commandSources, errorMessage);
}
} catch (Exception e) {
log.error("Failed to save failed CommandSource entries for batch audit", e);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ public enum CommandProcessingResultType {
AWAITING_APPROVAL(2, "commandProcessingResultType.awaiting.approval"), //
REJECTED(3, "commandProcessingResultType.rejected"), //
UNDER_PROCESSING(4, "commandProcessingResultType.underProcessing"), //
ERROR(5, "commandProcessingResultType.error");
ERROR(5, "commandProcessingResultType.error"), //
ROLLBACK(6, "commandProcessingResultType.rollback");

private static final Map<Integer, CommandProcessingResultType> BY_ID = Arrays.stream(values())
.collect(Collectors.toMap(CommandProcessingResultType::getValue, v -> v));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@

import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
import java.util.List;
import java.util.Set;
import lombok.RequiredArgsConstructor;
import org.apache.fineract.batch.exception.ErrorInfo;
import org.apache.fineract.commands.domain.CommandProcessingResultType;
import org.apache.fineract.commands.domain.CommandSource;
import org.apache.fineract.commands.domain.CommandSourceRepository;
import org.apache.fineract.commands.domain.CommandWrapper;
Expand Down Expand Up @@ -69,12 +71,6 @@ public CommandSource saveInitialNewTransaction(CommandWrapper wrapper, JsonComma
return saveInitial(wrapper, jsonCommand, maker, idempotencyKey);
}

@NonNull
@Transactional(propagation = Propagation.REQUIRED)
public CommandSource saveInitialSameTransaction(CommandWrapper wrapper, JsonCommand jsonCommand, AppUser maker, String idempotencyKey) {
return saveInitial(wrapper, jsonCommand, maker, idempotencyKey);
}

@NonNull
private CommandSource saveInitial(CommandWrapper wrapper, JsonCommand jsonCommand, AppUser maker, String idempotencyKey) {
try {
Expand All @@ -90,8 +86,8 @@ private CommandSource saveInitial(CommandWrapper wrapper, JsonCommand jsonComman
}

@Transactional(propagation = Propagation.REQUIRES_NEW, isolation = Isolation.REPEATABLE_READ)
public CommandSource saveResultNewTransaction(@NonNull CommandSource commandSource) {
return saveResult(commandSource);
public void saveResultNewTransaction(@NonNull CommandSource commandSource) {
saveResult(commandSource);
}

@Transactional(propagation = Propagation.REQUIRED)
Expand All @@ -108,6 +104,16 @@ public ErrorInfo generateErrorInfo(Throwable t) {
return errorHandler.handle(ErrorHandler.getMappable(t));
}

@Transactional(propagation = Propagation.REQUIRES_NEW)
public void saveFailedCommandSourcesNewTransaction(final List<CommandSource> commandSources, final String errorMessage) {
commandSources.forEach(commandSource -> {
commandSource.setStatus(CommandProcessingResultType.ROLLBACK);
commandSource.setResult(errorMessage);
commandSource.setResultStatusCode(500);
commandSourceRepository.saveAndFlush(commandSource);
});
}

@Transactional(propagation = Propagation.REQUIRES_NEW)
public CommandSource getCommandSource(Long commandSourceId) {
return commandSourceRepository.findById(commandSourceId).orElseThrow(() -> new CommandNotFoundException(commandSourceId));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,10 @@ public CommandProcessingResult executeCommand(final CommandWrapper wrapper, fina
storeCommandIdInContext(commandSource); // Store command id as a request attribute
}

if (isEnclosingTransaction) {
BatchRequestContextHolder.addCommandSource(commandSource);
}

setIdempotencyKeyStoreFlag(true);

return executeCommand(wrapper, command, isApprovedByChecker, commandSource, user, isEnclosingTransaction);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@
*/
package org.apache.fineract.infrastructure.core.domain;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import org.apache.fineract.commands.domain.CommandSource;
import org.springframework.transaction.TransactionStatus;

public final class BatchRequestContextHolder {
Expand All @@ -32,6 +35,8 @@ private BatchRequestContextHolder() {}

private static final ThreadLocal<Boolean> isEnclosingTransaction = new ThreadLocal<>();

private static final ThreadLocal<List<CommandSource>> commandSources = ThreadLocal.withInitial(ArrayList::new);

/**
* True if the batch attributes are set
*
Expand Down Expand Up @@ -87,6 +92,7 @@ public static void setIsEnclosingTransaction(boolean isEnclosingTransaction) {

public static void resetIsEnclosingTransaction() {
isEnclosingTransaction.remove();
commandSources.get().clear();
}

/**
Expand Down Expand Up @@ -130,4 +136,15 @@ public static void setEnclosingTransaction(TransactionStatus enclosingTransactio
public static void resetTransaction() {
batchTransaction.set(Optional.empty());
}

public static void addCommandSource(final CommandSource commandSource) {
if (isEnclosingTransaction() && commandSource != null) {
commandSources.get().add(commandSource);
}
}

public static List<CommandSource> getCommandSources() {
return new ArrayList<>(commandSources.get());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.fineract.batch.service;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
Expand All @@ -41,7 +42,10 @@
import org.apache.fineract.batch.domain.BatchResponse;
import org.apache.fineract.batch.exception.ErrorInfo;
import org.apache.fineract.commands.configuration.RetryConfigurationAssembler;
import org.apache.fineract.commands.domain.CommandSource;
import org.apache.fineract.commands.service.CommandSourceService;
import org.apache.fineract.infrastructure.core.config.FineractProperties;
import org.apache.fineract.infrastructure.core.domain.BatchRequestContextHolder;
import org.apache.fineract.infrastructure.core.domain.FineractRequestContextHolder;
import org.apache.fineract.infrastructure.core.exception.ErrorHandler;
import org.apache.fineract.infrastructure.core.filters.BatchRequestPreprocessor;
Expand Down Expand Up @@ -74,6 +78,9 @@ class BatchApiServiceImplTest {
@Mock
private ErrorHandler errorHandler;

@Mock
private CommandSourceService commandSourceService;

@Mock
private RetryRegistry registry;

Expand All @@ -97,7 +104,7 @@ class BatchApiServiceImplTest {
@BeforeEach
void setUp() {
batchApiService = new BatchApiServiceImpl(strategyProvider, resolutionHelper, transactionManager, errorHandler, List.of(),
batchPreprocessors, retryConfigurationAssembler);
batchPreprocessors, retryConfigurationAssembler, commandSourceService);
batchApiService.setEntityManager(entityManager);
request = new BatchRequest();
request.setRequestId(1L);
Expand Down Expand Up @@ -227,6 +234,32 @@ void testHandleBatchRequestsWithEnclosingTransactionReadOnly() {
Mockito.verifyNoInteractions(entityManager);
}

@Test
void testFailedCommandSourceEntriesAreSavedOnBatchFailure() {
final List<BatchRequest> requestList = List.of(request);
when(strategyProvider.getCommandStrategy(any())).thenReturn(commandStrategy);
when(commandStrategy.execute(any(), any())).thenThrow(new RuntimeException("Test failure"));

final ErrorInfo errorInfo = mock(ErrorInfo.class);
when(errorInfo.getMessage()).thenReturn("Test failure");
when(errorInfo.getStatusCode()).thenReturn(500);
when(errorHandler.handle(any())).thenReturn(errorInfo);

when(transactionManager.getTransaction(any()))
.thenReturn(new DefaultTransactionStatus("txn_name", null, true, true, false, false, false, null));

final CommandSource mockCommandSource = mock(CommandSource.class);
BatchRequestContextHolder.setIsEnclosingTransaction(true);
BatchRequestContextHolder.addCommandSource(mockCommandSource);

final BatchResponse result = batchApiService.handleBatchRequestsWithEnclosingTransaction(requestList, uriInfo).getFirst();
assertNotNull(result);
assertEquals(500, result.getStatusCode());
assertTrue(result.getBody().contains("Test failure"));

verify(commandSourceService, times(1)).saveFailedCommandSourcesNewTransaction(any(), anyString());
}

private static final class RetryException extends RuntimeException {}

}
Loading