From 52f75f604ba90f4f306b75b5d1816919bd3f02f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alberto=20Brigand=C3=AC?= Date: Wed, 20 Jun 2018 16:26:46 +0200 Subject: [PATCH] Return 409 HTTP code instead of 500 for Optimistic Lock Exceptions Fixes #265 --- .../reply/orchestrator/dto/common/Error.java | 6 +- .../GlobalControllerExceptionHandler.java | 40 ++++++++++-- .../controller/DeploymentControllerTest.java | 64 +++++++++++++++++-- 3 files changed, 96 insertions(+), 14 deletions(-) diff --git a/src/main/java/it/reply/orchestrator/dto/common/Error.java b/src/main/java/it/reply/orchestrator/dto/common/Error.java index d20ed5cafc..11fef20384 100644 --- a/src/main/java/it/reply/orchestrator/dto/common/Error.java +++ b/src/main/java/it/reply/orchestrator/dto/common/Error.java @@ -46,9 +46,11 @@ public class Error implements Serializable { /** * Generate an Error from an exception and a {@link HttpStatus}. - * - * @param ex + * + * @param exception * the exception + * @param message + * the message * @param status * the HttpStatus */ diff --git a/src/main/java/it/reply/orchestrator/exception/GlobalControllerExceptionHandler.java b/src/main/java/it/reply/orchestrator/exception/GlobalControllerExceptionHandler.java index e748d08008..6c555501e6 100644 --- a/src/main/java/it/reply/orchestrator/exception/GlobalControllerExceptionHandler.java +++ b/src/main/java/it/reply/orchestrator/exception/GlobalControllerExceptionHandler.java @@ -22,6 +22,8 @@ import lombok.extern.slf4j.Slf4j; +import org.flowable.engine.common.api.FlowableOptimisticLockingException; +import org.springframework.dao.TransientDataAccessException; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; @@ -38,7 +40,7 @@ public class GlobalControllerExceptionHandler extends ResponseEntityExceptionHan /** * {@link OrchestratorApiException} handler. - * + * * @param ex * the exception * @return a {@code ResponseEntity} instance @@ -51,7 +53,7 @@ public ResponseEntity handleException(OrchestratorApiException ex, WebRe /** * OAuth2Exception exception handler. This handler will just re-throw the exception and to let * the {@link AbstractOAuth2SecurityExceptionHandler} handle it. - * + * * @param ex * the exception * @return a {@code ResponseEntity} instance @@ -63,9 +65,30 @@ public ResponseEntity handleException(OAuth2Exception ex, WebRequest req throw ex; } + /** + * {@link TransientDataAccessException} and {@link FlowableOptimisticLockingException} handler. + * + * @param ex + * the exception + * @return a {@code ResponseEntity} instance + */ + @ExceptionHandler({TransientDataAccessException.class, FlowableOptimisticLockingException.class}) + public ResponseEntity handleTransientDataException(Exception ex, WebRequest request) { + HttpHeaders headers = new HttpHeaders(); + headers.set(HttpHeaders.RETRY_AFTER, "0"); + Error bodyToWrite = Error + .builder() + .message("The request couldn't be fulfilled because of a concurrent update." + + " Please retry later") + .exception(ex) + .status(HttpStatus.CONFLICT) + .build(); + return handleExceptionInternal(ex, bodyToWrite, headers, HttpStatus.CONFLICT, request); + } + /** * Server Error exception handler. - * + * * @param ex * the exception * @return a {@code ResponseEntity} instance @@ -82,6 +105,9 @@ public ResponseEntity handleGenericException(Exception ex, WebRequest re @Override protected ResponseEntity handleExceptionInternal(Exception ex, Object body, HttpHeaders headers, HttpStatus status, WebRequest request) { + if (status != HttpStatus.NOT_FOUND) { + LOG.error("Error handling request {}", request, ex); + } final HttpHeaders headersToWrite = CommonUtils.notNullOrDefaultValue(headers, HttpHeaders::new); final Object bodyToWrite = CommonUtils.notNullOrDefaultValue(body, () -> CommonUtils.checkNotNull(Error.builder().exception(ex).status(status).build())); @@ -90,10 +116,12 @@ protected ResponseEntity handleExceptionInternal(Exception ex, Object bo protected ResponseEntity handleExceptionInternal(Exception ex, HttpStatus status, WebRequest request) { - if (status != HttpStatus.NOT_FOUND && status != HttpStatus.CONFLICT) { - LOG.error("Error handling request {}", request, ex); - } return handleExceptionInternal(ex, null, null, status, request); } + protected ResponseEntity handleExceptionInternal(Exception ex, HttpStatus status, + HttpHeaders headers, WebRequest request) { + return handleExceptionInternal(ex, null, headers, status, request); + } + } diff --git a/src/test/java/it/reply/orchestrator/controller/DeploymentControllerTest.java b/src/test/java/it/reply/orchestrator/controller/DeploymentControllerTest.java index 9b84f037dd..adc8e63dcb 100644 --- a/src/test/java/it/reply/orchestrator/controller/DeploymentControllerTest.java +++ b/src/test/java/it/reply/orchestrator/controller/DeploymentControllerTest.java @@ -36,6 +36,7 @@ import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.put; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; +import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.header; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; @@ -49,14 +50,21 @@ import it.reply.orchestrator.exception.http.NotFoundException; import it.reply.orchestrator.resource.DeploymentResourceAssembler; import it.reply.orchestrator.service.DeploymentService; - import it.reply.orchestrator.utils.JsonUtils; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.UUID; + +import junitparams.JUnitParamsRunner; +import junitparams.Parameters; + import org.hamcrest.Matchers; import org.junit.ClassRule; import org.junit.Rule; import org.junit.Test; -import org.mockito.Answers; +import org.junit.runner.RunWith; import org.mockito.Mockito; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.test.autoconfigure.restdocs.AutoConfigureRestDocs; @@ -76,12 +84,9 @@ import org.springframework.test.context.junit4.rules.SpringClassRule; import org.springframework.test.context.junit4.rules.SpringMethodRule; import org.springframework.test.web.servlet.MockMvc; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.UUID; @WebMvcTest(controllers = DeploymentController.class, secure = false) +@RunWith(JUnitParamsRunner.class) @AutoConfigureRestDocs("target/generated-snippets") @Import(HateoasAwareSpringDataWebConfiguration.class) public class DeploymentControllerTest { @@ -420,6 +425,53 @@ public void updateDeploymentDeleteInProgress() throws Exception { jsonPath("$.message", is("Cannot update a deployment in DELETE_IN_PROGRESS state"))); } + @Test + @Parameters({"org.springframework.dao.TransientDataAccessException", + "org.flowable.engine.common.api.FlowableOptimisticLockingException"}) + public void updateDeploymentConcurrentTransientException(Class clazz) + throws Exception { + DeploymentRequest request = DeploymentRequest + .builder() + .template("template") + .build(); + + String deploymentId = "mmd34483-d937-4578-bfdb-ebe196bf82dd"; + Mockito.doThrow(Mockito.mock(clazz)) + .when(deploymentService) + .updateDeployment(deploymentId, request); + + mockMvc + .perform(put("/deployments/" + deploymentId).contentType(MediaType.APPLICATION_JSON) + .content(JsonUtils.serialize(request))) + .andExpect(header().string(HttpHeaders.RETRY_AFTER, "0")) + .andExpect(jsonPath("$.code", is(409))) + .andExpect(jsonPath("$.title", is("Conflict"))) + .andExpect( + jsonPath("$.message", + is("The request couldn't be fulfilled because of a concurrent update. Please retry later"))); + } + + @Test + @Parameters({"org.springframework.dao.TransientDataAccessException", + "org.flowable.engine.common.api.FlowableOptimisticLockingException"}) + public void deleteDeploymentConcurrentTransientException(Class clazz) + throws Exception { + + String deploymentId = "mmd34483-d937-4578-bfdb-ebe196bf82dd"; + Mockito.doThrow(Mockito.mock(clazz)) + .when(deploymentService) + .deleteDeployment(deploymentId); + + mockMvc + .perform(delete("/deployments/" + deploymentId)) + .andExpect(header().string(HttpHeaders.RETRY_AFTER, "0")) + .andExpect(jsonPath("$.code", is(409))) + .andExpect(jsonPath("$.title", is("Conflict"))) + .andExpect( + jsonPath("$.message", + is("The request couldn't be fulfilled because of a concurrent update. Please retry later"))); + } + @Test public void updateDeploymentSuccessfully() throws Exception {