This is an automated email from the ASF dual-hosted git repository.
adamsaghy pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/fineract.git
The following commit(s) were added to refs/heads/develop by this push:
new e27a0bb53 FINERACT-2000: Clean-up retryable error codes
e27a0bb53 is described below
commit e27a0bb53f846b417c3a18133e1845d900a3c67d
Author: jmarta <[email protected]>
AuthorDate: Fri Nov 17 16:41:32 2023 +0100
FINERACT-2000: Clean-up retryable error codes
---
.../core/data/ApiGlobalErrorResponse.java | 5 +--
.../ConcurrencyFailureExceptionMapper.java | 6 +--
.../IdempotentCommandExceptionMapper.java | 13 ++++---
.../OptimisticLockExceptionMapper.java | 8 ++--
...etailWritePlatformServiceJpaRepositoryImpl.java | 2 +-
.../SavingsAccountTransactionTest.java | 45 +++++++++++++---------
6 files changed, 44 insertions(+), 35 deletions(-)
diff --git
a/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/data/ApiGlobalErrorResponse.java
b/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/data/ApiGlobalErrorResponse.java
index f744f1686..b71d49826 100644
---
a/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/data/ApiGlobalErrorResponse.java
+++
b/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/data/ApiGlobalErrorResponse.java
@@ -23,7 +23,6 @@ import static org.apache.http.HttpStatus.SC_BAD_REQUEST;
import static org.apache.http.HttpStatus.SC_CONFLICT;
import static org.apache.http.HttpStatus.SC_FORBIDDEN;
import static org.apache.http.HttpStatus.SC_INTERNAL_SERVER_ERROR;
-import static org.apache.http.HttpStatus.SC_LOCKED;
import static org.apache.http.HttpStatus.SC_METHOD_NOT_ALLOWED;
import static org.apache.http.HttpStatus.SC_NOT_FOUND;
import static org.apache.http.HttpStatus.SC_SERVICE_UNAVAILABLE;
@@ -107,7 +106,7 @@ public class ApiGlobalErrorResponse {
return create(SC_CONFLICT, "error.msg.loan.locked", msg, msg);
}
- public static ApiGlobalErrorResponse locked(String type, String
identifier) {
+ public static ApiGlobalErrorResponse conflict(String type, String
identifier) {
String details = "";
if (type == null) {
type = "unknown";
@@ -119,7 +118,7 @@ public class ApiGlobalErrorResponse {
}
String msg = "The server is currently unable to handle the request due
to concurrent modification " + details
+ ", please try again";
- return create(SC_LOCKED, "error.msg.platform.service." + type +
".conflict", msg, msg);
+ return create(SC_CONFLICT, "error.msg.platform.service." + type +
".conflict", msg, msg);
}
public static ApiGlobalErrorResponse unAuthorized(final String
defaultUserMessage) {
diff --git
a/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/exceptionmapper/ConcurrencyFailureExceptionMapper.java
b/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/exceptionmapper/ConcurrencyFailureExceptionMapper.java
index 902fb9e92..d3d8f9710 100644
---
a/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/exceptionmapper/ConcurrencyFailureExceptionMapper.java
+++
b/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/exceptionmapper/ConcurrencyFailureExceptionMapper.java
@@ -18,7 +18,7 @@
*/
package org.apache.fineract.infrastructure.core.exceptionmapper;
-import static org.apache.http.HttpStatus.SC_LOCKED;
+import static org.apache.http.HttpStatus.SC_CONFLICT;
import jakarta.ws.rs.core.MediaType;
import jakarta.ws.rs.core.Response;
@@ -53,8 +53,8 @@ public class ConcurrencyFailureExceptionMapper implements
FineractExceptionMappe
type = "lock";
identifier = null;
}
- final ApiGlobalErrorResponse dataIntegrityError =
ApiGlobalErrorResponse.locked(type, identifier);
- return
Response.status(SC_LOCKED).entity(dataIntegrityError).type(MediaType.APPLICATION_JSON).build();
+ final ApiGlobalErrorResponse dataIntegrityError =
ApiGlobalErrorResponse.conflict(type, identifier);
+ return
Response.status(SC_CONFLICT).entity(dataIntegrityError).type(MediaType.APPLICATION_JSON).build();
}
@Override
diff --git
a/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/exceptionmapper/IdempotentCommandExceptionMapper.java
b/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/exceptionmapper/IdempotentCommandExceptionMapper.java
index 4facc0976..c37625041 100644
---
a/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/exceptionmapper/IdempotentCommandExceptionMapper.java
+++
b/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/exceptionmapper/IdempotentCommandExceptionMapper.java
@@ -19,10 +19,11 @@
package org.apache.fineract.infrastructure.core.exceptionmapper;
import static
org.apache.fineract.infrastructure.core.exception.AbstractIdempotentCommandException.IDEMPOTENT_CACHE_HEADER;
+import static org.apache.http.HttpStatus.SC_INTERNAL_SERVER_ERROR;
+import static org.apache.http.HttpStatus.SC_OK;
import jakarta.ws.rs.core.MediaType;
import jakarta.ws.rs.core.Response;
-import jakarta.ws.rs.core.Response.Status;
import jakarta.ws.rs.ext.ExceptionMapper;
import jakarta.ws.rs.ext.Provider;
import lombok.extern.slf4j.Slf4j;
@@ -46,18 +47,18 @@ public class IdempotentCommandExceptionMapper implements
FineractExceptionMapper
@Override
public Response toResponse(final AbstractIdempotentCommandException
exception) {
log.warn("Processing {} request: {}", exception.getClass().getName(),
exception.getMessage());
- Status status = null;
+ Integer status = null;
if (exception instanceof IdempotentCommandProcessSucceedException pse)
{
Integer statusCode = pse.getStatusCode();
- status = statusCode == null ? Status.OK :
Status.fromStatusCode(statusCode);
+ status = statusCode == null ? SC_OK : statusCode;
}
if (exception instanceof
IdempotentCommandProcessUnderProcessingException) {
- status = Status.CONFLICT;
+ status = 425;
} else if (exception instanceof
IdempotentCommandProcessFailedException pfe) {
- status = Status.fromStatusCode(pfe.getStatusCode());
+ status = pfe.getStatusCode();
}
if (status == null) {
- status = Status.INTERNAL_SERVER_ERROR;
+ status = SC_INTERNAL_SERVER_ERROR;
}
return
Response.status(status).entity(exception.getResponse()).header(IDEMPOTENT_CACHE_HEADER,
"true")
.type(MediaType.APPLICATION_JSON).build();
diff --git
a/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/exceptionmapper/OptimisticLockExceptionMapper.java
b/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/exceptionmapper/OptimisticLockExceptionMapper.java
index b4af9e3ff..3e293d19b 100644
---
a/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/exceptionmapper/OptimisticLockExceptionMapper.java
+++
b/fineract-core/src/main/java/org/apache/fineract/infrastructure/core/exceptionmapper/OptimisticLockExceptionMapper.java
@@ -18,7 +18,7 @@
*/
package org.apache.fineract.infrastructure.core.exceptionmapper;
-import static org.apache.http.HttpStatus.SC_LOCKED;
+import static org.apache.http.HttpStatus.SC_CONFLICT;
import jakarta.ws.rs.core.MediaType;
import jakarta.ws.rs.core.Response;
@@ -46,12 +46,12 @@ public class OptimisticLockExceptionMapper implements
FineractExceptionMapper, E
log.warn("Exception: {}, Message: {}", exception.getClass().getName(),
exception.getMessage());
String type = exception.getQuery() == null ? "unknown" : "query";
String identifier = "unknown";
- final ApiGlobalErrorResponse dataIntegrityError =
ApiGlobalErrorResponse.locked(type, identifier);
- return
Response.status(SC_LOCKED).entity(dataIntegrityError).type(MediaType.APPLICATION_JSON).build();
+ final ApiGlobalErrorResponse dataIntegrityError =
ApiGlobalErrorResponse.conflict(type, identifier);
+ return
Response.status(SC_CONFLICT).entity(dataIntegrityError).type(MediaType.APPLICATION_JSON).build();
}
@Override
public int errorCode() {
- return 4009;
+ return 4019;
}
}
diff --git
a/fineract-provider/src/main/java/org/apache/fineract/portfolio/paymentdetail/service/PaymentDetailWritePlatformServiceJpaRepositoryImpl.java
b/fineract-provider/src/main/java/org/apache/fineract/portfolio/paymentdetail/service/PaymentDetailWritePlatformServiceJpaRepositoryImpl.java
index d2d498fdb..52e3db711 100644
---
a/fineract-provider/src/main/java/org/apache/fineract/portfolio/paymentdetail/service/PaymentDetailWritePlatformServiceJpaRepositoryImpl.java
+++
b/fineract-provider/src/main/java/org/apache/fineract/portfolio/paymentdetail/service/PaymentDetailWritePlatformServiceJpaRepositoryImpl.java
@@ -51,7 +51,7 @@ public class
PaymentDetailWritePlatformServiceJpaRepositoryImpl implements Payme
@Override
@Transactional
public PaymentDetail persistPaymentDetail(final PaymentDetail
paymentDetail) {
- return this.paymentDetailRepository.save(paymentDetail);
+ return this.paymentDetailRepository.saveAndFlush(paymentDetail);
}
@Override
diff --git
a/integration-tests/src/test/java/org/apache/fineract/integrationtests/SavingsAccountTransactionTest.java
b/integration-tests/src/test/java/org/apache/fineract/integrationtests/SavingsAccountTransactionTest.java
index b161ab001..ea5f63b9a 100644
---
a/integration-tests/src/test/java/org/apache/fineract/integrationtests/SavingsAccountTransactionTest.java
+++
b/integration-tests/src/test/java/org/apache/fineract/integrationtests/SavingsAccountTransactionTest.java
@@ -20,8 +20,8 @@ package org.apache.fineract.integrationtests;
import static
org.apache.fineract.integrationtests.common.savings.SavingsAccountHelper.PAYMENT_TYPE_ID;
import static
org.apache.fineract.integrationtests.common.system.DatatableHelper.addDatatableColumn;
+import static org.apache.http.HttpStatus.SC_CONFLICT;
import static org.apache.http.HttpStatus.SC_FORBIDDEN;
-import static org.apache.http.HttpStatus.SC_LOCKED;
import static org.apache.http.HttpStatus.SC_OK;
import static org.hamcrest.Matchers.anyOf;
import static org.hamcrest.Matchers.is;
@@ -31,6 +31,7 @@ import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import com.fasterxml.jackson.core.JsonProcessingException;
+import com.google.common.base.Strings;
import com.google.gson.Gson;
import io.restassured.builder.RequestSpecBuilder;
import io.restassured.builder.ResponseSpecBuilder;
@@ -85,6 +86,7 @@ public class SavingsAccountTransactionTest {
private ResponseSpecification responseSpec;
private ResponseSpecification concurrentResponseSpec;
+ private ResponseSpecification deadlockResponseSpec;
private RequestSpecification requestSpec;
private SavingsProductHelper savingsProductHelper;
private SavingsAccountHelper savingsAccountHelper;
@@ -96,7 +98,8 @@ public class SavingsAccountTransactionTest {
this.requestSpec = new
RequestSpecBuilder().setContentType(ContentType.JSON).build();
this.requestSpec.header("Authorization", "Basic " +
Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
this.responseSpec = new
ResponseSpecBuilder().expectStatusCode(SC_OK).build();
- this.concurrentResponseSpec = new
ResponseSpecBuilder().expectStatusCode(anyOf(is(SC_OK), is(SC_LOCKED))).build();
+ this.concurrentResponseSpec = new
ResponseSpecBuilder().expectStatusCode(anyOf(is(SC_OK),
is(SC_CONFLICT))).build();
+ this.deadlockResponseSpec = new
ResponseSpecBuilder().expectStatusCode(anyOf(is(SC_OK), is(SC_CONFLICT),
is(SC_FORBIDDEN))).build();
this.savingsAccountHelper = new SavingsAccountHelper(this.requestSpec,
this.responseSpec);
this.savingsProductHelper = new SavingsProductHelper();
this.datatableHelper = new DatatableHelper(this.requestSpec,
this.responseSpec);
@@ -190,7 +193,7 @@ public class SavingsAccountTransactionTest {
SavingsAccountHelper batchWithTransactionHelper = new
SavingsAccountHelper(requestSpec, concurrentResponseSpec);
SavingsAccountHelper batchWithoutTransactionHelper = new
SavingsAccountHelper(requestSpec,
- new ResponseSpecBuilder().expectStatusCode(anyOf(is(SC_OK),
is(SC_LOCKED), is(SC_FORBIDDEN))).build());
+ new ResponseSpecBuilder().expectStatusCode(anyOf(is(SC_OK),
is(SC_CONFLICT), is(SC_FORBIDDEN))).build());
String transactionDate = SavingsAccountHelper.TRANSACTION_DATE;
String transactionAmount = "10";
ExecutorService executor = Executors.newFixedThreadPool(30);
@@ -223,7 +226,7 @@ public class SavingsAccountTransactionTest {
log.info("\nFinished all threads");
}
- // @Test
+ @Test
public void testDeadlockSavingsBatchTransactions() {
final Integer clientID = ClientHelper.createClient(requestSpec,
responseSpec);
ClientHelper.verifyClientCreatedOnServer(requestSpec, responseSpec,
clientID);
@@ -239,7 +242,7 @@ public class SavingsAccountTransactionTest {
savingsAccountHelper.approveSavings(savingsId2);
savingsAccountHelper.activateSavings(savingsId2);
- SavingsAccountHelper batchWithTransactionHelper = new
SavingsAccountHelper(requestSpec, concurrentResponseSpec);
+ SavingsAccountHelper batchWithTransactionHelper = new
SavingsAccountHelper(requestSpec, deadlockResponseSpec);
String transactionDate = SavingsAccountHelper.TRANSACTION_DATE;
String transactionAmount = "10";
@@ -293,11 +296,12 @@ public class SavingsAccountTransactionTest {
assertEquals(transactionId, (Integer) transaction.get("id"), "Check
Savings " + transactionType + " Transaction");
LocalDate transactionDateFromResponse = extractLocalDate(transaction,
"date");
- assertTrue(DateUtils.isEqual(transactionDate,
transactionDateFromResponse),
- "Transaction Date check for Savings " + transactionType + "
Transaction");
- LocalDate submittedOnDate = extractLocalDate(transaction,
"submittedOnDate");
- assertTrue(DateUtils.isEqual(submittedOnDate,
Utils.getLocalDateOfTenant()),
- "Submitted On Date check for Savings " + transactionType + "
Transaction");
+ assertTrue(DateUtils.isEqual(transactionDate,
transactionDateFromResponse), "Transaction Date check for Savings " +
transactionType
+ + " Transaction. Expected: " + transactionDate + ", current: "
+ transactionDateFromResponse);
+ LocalDate submittedOnDate = Utils.getLocalDateOfTenant();
+ LocalDate submittedOnDateFromResponse = extractLocalDate(transaction,
"submittedOnDate");
+ assertTrue(DateUtils.isEqual(submittedOnDate,
submittedOnDateFromResponse), "Submitted On Date check for Savings " +
transactionType
+ + " Transaction. Expected: " + submittedOnDate + ", current: "
+ submittedOnDateFromResponse);
}
private LocalDate extractLocalDate(HashMap transactionMap, String
fieldName) {
@@ -382,7 +386,7 @@ public class SavingsAccountTransactionTest {
if (enclosingTransaction) {
Integer statusCode1 = responses.get(0).getStatusCode();
assertNotNull(statusCode1);
- assertTrue(SC_OK == statusCode1 || SC_LOCKED ==
statusCode1);
+ assertTrue(SC_OK == statusCode1 || SC_CONFLICT ==
statusCode1, "Status code: " + statusCode1);
if (SC_OK == statusCode1) {
assertEquals(4, responses.size());
Integer statusCode4 = responses.get(3).getStatusCode();
@@ -395,11 +399,11 @@ public class SavingsAccountTransactionTest {
assertEquals(4, responses.size());
Integer statusCode1 = responses.get(0).getStatusCode();
assertNotNull(statusCode1);
- assertTrue(SC_OK == statusCode1 || SC_LOCKED ==
statusCode1);
+ assertTrue(SC_OK == statusCode1 || SC_CONFLICT ==
statusCode1, "Status code: " + statusCode1);
Integer statusCode4 = responses.get(3).getStatusCode();
assertNotNull(statusCode4);
- assertTrue(SC_OK == statusCode1 ? (SC_OK == statusCode4 ||
SC_LOCKED == statusCode4)
- : (SC_FORBIDDEN == statusCode4 || SC_LOCKED ==
statusCode4));
+ assertTrue(SC_OK == statusCode1 ? (SC_OK == statusCode4 ||
SC_CONFLICT == statusCode4)
+ : (SC_FORBIDDEN == statusCode4 || SC_CONFLICT ==
statusCode4), "Status code: " + statusCode4);
}
} else {
String json = transactionData.getJson();
@@ -415,12 +419,12 @@ public class SavingsAccountTransactionTest {
private static boolean checkConcurrentResponse(String response) {
assertNotNull(response);
JsonPath res = JsonPath.from(response);
- String statusCode = (String) res.get("httpStatusCode");
+ String statusCode = res.get("httpStatusCode");
if (statusCode == null) {
assertNotNull(res.get(CommonConstants.RESPONSE_RESOURCE_ID));
return true;
}
- assertEquals(String.valueOf(SC_LOCKED), statusCode);
+ assertEquals(String.valueOf(SC_CONFLICT), statusCode);
return false;
}
}
@@ -436,9 +440,14 @@ public class SavingsAccountTransactionTest {
ResponseSpecification responseSpec = savingsHelper.getResponseSpec();
final List<BatchResponse> responses =
BatchHelper.postBatchRequestsWithEnclosingTransaction(requestSpec,
responseSpec, json);
assertNotNull(responses);
- Integer statusCode = responses.get(0).getStatusCode();
+ BatchResponse response1 = responses.get(0);
+ Integer statusCode = response1.getStatusCode();
+ String msg = Strings.nullToEmpty(response1.getBody());
assertNotNull(statusCode);
- assertTrue(SC_OK == statusCode || SC_LOCKED == statusCode);
+ assertTrue(
+ SC_OK == statusCode || SC_CONFLICT == statusCode
+ || (SC_FORBIDDEN == statusCode && msg.contains("Cannot
add or update a child row")),
+ "Status code: " + statusCode + ", message: " + msg);
if (SC_OK == statusCode) {
assertEquals(4, responses.size());
Integer statusCode4 = responses.get(3).getStatusCode();
