Copilot commented on code in PR #5881:
URL: https://github.com/apache/fineract/pull/5881#discussion_r3295242166
##########
fineract-provider/src/main/java/org/apache/fineract/accounting/journalentry/service/AccountingProcessorHelper.java:
##########
@@ -261,13 +261,14 @@ public SavingsDTO populateSavingsDtoFromMap(final
Map<String, Object> accounting
}
}
- if (!isAccountTransfer) {
- isAccountTransfer =
this.accountTransfersReadPlatformService.isAccountTransfer(Long.parseLong(transactionId),
+ boolean localIsAccountTransfer = isAccountTransfer;
+ if (!localIsAccountTransfer) {
+ localIsAccountTransfer =
this.accountTransfersReadPlatformService.isAccountTransfer(Long.parseLong(transactionId),
PortfolioAccountType.SAVINGS);
}
final SavingsTransactionDTO transaction = new
SavingsTransactionDTO(transactionOfficeId, paymentTypeId, transactionId,
- transactionDate, transactionType, amount, reversed,
feePayments, penaltyPayments, overdraftAmount, isAccountTransfer,
- taxPayments);
+ transactionDate, transactionType, amount, reversed,
feePayments, penaltyPayments, overdraftAmount,
+ localIsAccountTransfer, taxPayments);
Review Comment:
This change stops propagating the computed `isAccountTransfer` value back to
the outer `isAccountTransfer` variable. If this method iterates over multiple
transactions and `isAccountTransfer` was intentionally used as a memoized flag
to avoid repeated `accountTransfersReadPlatformService.isAccountTransfer(...)`
calls, that optimization is lost and may cause repeated service lookups. If
caching is intended, assign the computed value back (e.g., after the lookup) or
keep a dedicated memoized variable outside the per-transaction block.
##########
fineract-e2e-tests-runner/src/test/java/org/apache/fineract/test/initializer/global/LoanProductGlobalInitializerStep.java:
##########
@@ -5352,11 +5352,39 @@ private static List<PaymentAllocationOrder>
getPaymentAllocationOrder(
private PostLoanProductsResponse
createLoanProductIdempotent(PostLoanProductsRequest loanProductRequest) {
String productName = loanProductRequest.getName();
log.debug("Attempting to create loan product: {}", productName);
+
+ // First, check if product already exists
+ PostLoanProductsResponse existingResponse =
findExistingProduct(productName);
+ if (existingResponse != null) {
+ return existingResponse;
+ }
+
+ // Try to create
+ log.debug("Creating new loan product: {}", productName);
+ try {
+ PostLoanProductsResponse response = ok(() ->
fineractClient.loanProducts().createLoanProduct(loanProductRequest, Map.of()));
+ log.debug("Successfully created loan product '{}' with ID: {}",
productName, response.getResourceId());
+ return response;
+ } catch (Exception e) {
+ // On failure (e.g., race condition with parallel thread or
shortName collision from previous run),
+ // re-check if the product now exists before propagating the error
+ log.warn("Failed to create loan product '{}', re-checking if it
was created concurrently", productName, e);
+ PostLoanProductsResponse retryResponse =
findExistingProduct(productName);
+ if (retryResponse != null) {
+ log.info("Loan product '{}' found on retry with ID: {}",
productName, retryResponse.getResourceId());
+ return retryResponse;
+ }
+ // If still not found, propagate the original error
+ log.error("FAILED to create loan product '{}' and it doesn't exist
in database", productName);
Review Comment:
The final error log drops the exception details (`e`), which makes debugging
harder when the failure is rethrown. Include the exception in the
`log.error(...)` call so the stack trace is preserved at error level as well.
##########
fineract-e2e-tests-runner/src/test/java/org/apache/fineract/test/initializer/global/LoanProductGlobalInitializerStep.java:
##########
@@ -5352,11 +5352,39 @@ private static List<PaymentAllocationOrder>
getPaymentAllocationOrder(
private PostLoanProductsResponse
createLoanProductIdempotent(PostLoanProductsRequest loanProductRequest) {
String productName = loanProductRequest.getName();
log.debug("Attempting to create loan product: {}", productName);
+
+ // First, check if product already exists
+ PostLoanProductsResponse existingResponse =
findExistingProduct(productName);
+ if (existingResponse != null) {
+ return existingResponse;
+ }
+
+ // Try to create
+ log.debug("Creating new loan product: {}", productName);
+ try {
+ PostLoanProductsResponse response = ok(() ->
fineractClient.loanProducts().createLoanProduct(loanProductRequest, Map.of()));
+ log.debug("Successfully created loan product '{}' with ID: {}",
productName, response.getResourceId());
+ return response;
+ } catch (Exception e) {
+ // On failure (e.g., race condition with parallel thread or
shortName collision from previous run),
+ // re-check if the product now exists before propagating the error
+ log.warn("Failed to create loan product '{}', re-checking if it
was created concurrently", productName, e);
+ PostLoanProductsResponse retryResponse =
findExistingProduct(productName);
+ if (retryResponse != null) {
+ log.info("Loan product '{}' found on retry with ID: {}",
productName, retryResponse.getResourceId());
+ return retryResponse;
+ }
+ // If still not found, propagate the original error
+ log.error("FAILED to create loan product '{}' and it doesn't exist
in database", productName);
+ throw e;
+ }
+ }
+
+ private PostLoanProductsResponse findExistingProduct(String productName) {
try {
List<GetLoanProductsResponse> existingProducts =
fineractClient.loanProducts().retrieveAllLoanProducts(Map.of());
GetLoanProductsResponse existingProduct =
existingProducts.stream().filter(p ->
productName.equals(p.getName())).findFirst()
.orElse(null);
Review Comment:
Fetching all loan products and then filtering client-side can become
expensive as the dataset grows, and this method may run frequently during E2E
initialization (now potentially twice per create attempt due to retry logic).
If the API/client supports server-side filtering (e.g., by name) or a direct
'get by name' endpoint, prefer that; otherwise consider caching retrieved
products within the initializer run to avoid repeated full-list calls.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
