Return 409 HTTP code instead of 500 for Optimistic Lock Exceptions

Fixes #265
indigo-dc · Jun 20, 2018 · 52f75f6 · 52f75f6
1 parent 1079419
commit 52f75f6
Show file tree

Hide file tree

Showing 3 changed files with 96 additions and 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
@@ -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
@@ -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<Object> 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<Object> 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<Object> 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<Object> handleGenericException(Exception ex, WebRequest re
   @Override
   protected ResponseEntity<Object> 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<Object> handleExceptionInternal(Exception ex, Object bo
 
   protected ResponseEntity<Object> 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<Object> 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
@@ -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<Exception> 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<Exception> 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 {