Fix response code when trying to update a published workflow. (#224)

The error message returned when trying to update a workflw that has an already published version was confusing at it says that the workflow does not exist. THis has been fixed in this PR by firstly checking if the workflos exists, then returning a more detailed exception when a version is already published.
finos · Feb 14, 2023 · 98d543b · 98d543b
1 parent e0859cd
commit 98d543b
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 29 deletions.
diff --git a/...low-bot-app/src/main/java/com/symphony/bdk/workflow/exception/GlobalExceptionHandler.java b/...low-bot-app/src/main/java/com/symphony/bdk/workflow/exception/GlobalExceptionHandler.java
@@ -2,16 +2,12 @@
 package com.symphony.bdk.workflow.exception;
 
 import com.symphony.bdk.workflow.api.v1.dto.ErrorResponse;
-
 import lombok.extern.slf4j.Slf4j;
-import org.springframework.http.HttpHeaders;
 import org.springframework.http.HttpStatus;
 import org.springframework.http.ResponseEntity;
-import org.springframework.lang.Nullable;
 import org.springframework.stereotype.Component;
 import org.springframework.web.bind.annotation.ControllerAdvice;
 import org.springframework.web.bind.annotation.ExceptionHandler;
-import org.springframework.web.context.request.WebRequest;
 import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler;
 
 import javax.persistence.OptimisticLockException;
@@ -57,6 +53,13 @@ public ResponseEntity<ErrorResponse> handle(IllegalArgumentException exception)
     return handle(exception.getMessage(), HttpStatus.BAD_REQUEST);
   }
 
+  @ExceptionHandler(UnsupportedOperationException.class)
+  public ResponseEntity<ErrorResponse> handle(UnsupportedOperationException exception) {
+    log.error("Unsupported operation exception: [{}]", exception.getMessage());
+    log.debug("", exception);
+    return handle(exception.getMessage(), HttpStatus.UNPROCESSABLE_ENTITY);
+  }
+
   @ExceptionHandler(OptimisticLockException.class)
   public ResponseEntity<ErrorResponse> handle(OptimisticLockException exception) {
     log.error("Optimistic locking exception: [{}]", exception.getMessage());

diff --git a/...bot-app/src/main/java/com/symphony/bdk/workflow/management/WorkflowManagementService.java b/...bot-app/src/main/java/com/symphony/bdk/workflow/management/WorkflowManagementService.java
@@ -27,6 +27,7 @@
 @Slf4j
 public class WorkflowManagementService {
   private static final String WORKFLOW_NOT_EXIST_EXCEPTION_MSG = "Workflow %s does not exist.";
+  private static final String WORKFLOW_UPDATE_FORBIDDEN_EXCEPTION_MSG = "Update on a published Workflow is forbidden.";
   private final WorkflowEngine<CamundaTranslatedWorkflowContext> workflowEngine;
   private final VersionedWorkflowRepository versionRepository;
   private final ObjectConverter objectConverter;
@@ -104,14 +105,17 @@ public Optional<VersionedWorkflowView> get(String id, Long version) {
   }
 
   private VersionedWorkflow readAndValidate(Workflow workflow) {
-    Optional<VersionedWorkflow> activeVersion = versionRepository.findByWorkflowIdAndPublishedFalse(workflow.getId());
+    List<VersionedWorkflow> activeVersion = versionRepository.findByWorkflowId(workflow.getId());
     if (activeVersion.isEmpty()) {
       throw new NotFoundException(String.format(WORKFLOW_NOT_EXIST_EXCEPTION_MSG, workflow.getId()));
     }
-    if (activeVersion.get().getPublished()) {
-      throw new IllegalArgumentException("Update on a published Workflow is forbidden.");
+
+    if (activeVersion.stream().anyMatch(VersionedWorkflow::getPublished)) {
+      throw new UnsupportedOperationException(WORKFLOW_UPDATE_FORBIDDEN_EXCEPTION_MSG);
     }
-    return activeVersion.get();
+
+    // The list can only contain 1 draft version item
+    return activeVersion.get(0);
   }
 
   public void delete(String id) {

diff --git a/...bot-app/src/test/java/com/symphony/bdk/workflow/exception/GlobalExceptionHandlerTest.java b/...bot-app/src/test/java/com/symphony/bdk/workflow/exception/GlobalExceptionHandlerTest.java
@@ -1,13 +1,12 @@
 package com.symphony.bdk.workflow.exception;
 
-import static org.assertj.core.api.Assertions.assertThat;
-
 import com.symphony.bdk.workflow.api.v1.dto.ErrorResponse;
-
 import org.junit.jupiter.api.Test;
 import org.springframework.http.HttpStatus;
 import org.springframework.http.ResponseEntity;
 
+import static org.assertj.core.api.Assertions.assertThat;
+
 class GlobalExceptionHandlerTest {
 
   private final GlobalExceptionHandler globalExceptionHandler = new GlobalExceptionHandler();
@@ -32,6 +31,16 @@ void testIllegalArgumentException() {
     assertThat(response.getBody()).isEqualTo(expectedErrorResponse);
   }
 
+  @Test
+  void testUnsupportedOperationException() {
+    ErrorResponse expectedErrorResponse = new ErrorResponse("Unsupported operation exception's message");
+    ResponseEntity<ErrorResponse> response = globalExceptionHandler.handle(
+            new UnsupportedOperationException("Unsupported operation exception's message"));
+
+    assertThat(response.getStatusCode()).isEqualTo(HttpStatus.UNPROCESSABLE_ENTITY);
+    assertThat(response.getBody()).isEqualTo(expectedErrorResponse);
+  }
+
   @Test
   void testNotFoundException() {
     ErrorResponse expectedErrorResponse = new ErrorResponse("NotFound exception's message");

diff --git a/...app/src/test/java/com/symphony/bdk/workflow/management/WorkflowManagementServiceTest.java b/...app/src/test/java/com/symphony/bdk/workflow/management/WorkflowManagementServiceTest.java
@@ -1,18 +1,6 @@
 package com.symphony.bdk.workflow.management;
 
-import static org.assertj.core.api.Assertions.assertThatThrownBy;
-import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
-import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType;
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.anyLong;
-import static org.mockito.ArgumentMatchers.anyString;
-import static org.mockito.ArgumentMatchers.eq;
-import static org.mockito.Mockito.doNothing;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
-
+import com.github.fge.jsonschema.core.exceptions.ProcessingException;
 import com.symphony.bdk.workflow.api.v1.dto.SwadlView;
 import com.symphony.bdk.workflow.converter.ObjectConverter;
 import com.symphony.bdk.workflow.engine.WorkflowEngine;
@@ -23,8 +11,6 @@
 import com.symphony.bdk.workflow.swadl.v1.Workflow;
 import com.symphony.bdk.workflow.versioning.model.VersionedWorkflow;
 import com.symphony.bdk.workflow.versioning.repository.VersionedWorkflowRepository;
-
-import com.github.fge.jsonschema.core.exceptions.ProcessingException;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.ArgumentCaptor;
@@ -33,8 +19,22 @@
 import org.mockito.junit.jupiter.MockitoExtension;
 
 import java.io.IOException;
+import java.util.Collections;
 import java.util.Optional;
 
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatExceptionOfType;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
 @ExtendWith(MockitoExtension.class)
 public class WorkflowManagementServiceTest {
 
@@ -144,7 +144,7 @@ void testUpdate_latestNoPublishedVersion_publishSucceed() {
     when(conveter.convert(anyString(), eq(Workflow.class))).thenReturn(workflow);
     VersionedWorkflow noPublishedVersion = new VersionedWorkflow();
     noPublishedVersion.setPublished(false);
-    when(versionRepository.findByWorkflowIdAndPublishedFalse(anyString())).thenReturn(Optional.of(noPublishedVersion));
+    when(versionRepository.findByWorkflowId(anyString())).thenReturn(Collections.singletonList(noPublishedVersion));
     CamundaTranslatedWorkflowContext context = mock(CamundaTranslatedWorkflowContext.class);
     when(camundaEngine.translate(any(Workflow.class))).thenReturn(context);
     when(camundaEngine.deploy(any(CamundaTranslatedWorkflowContext.class))).thenReturn("id");
@@ -167,7 +167,7 @@ void testUpdate_latestNoPublishedVersion_updateOnlySucceed() {
     VersionedWorkflow noPublishedVersion = new VersionedWorkflow();
     noPublishedVersion.setPublished(false);
     noPublishedVersion.setActive(false);
-    when(versionRepository.findByWorkflowIdAndPublishedFalse(anyString())).thenReturn(Optional.of(noPublishedVersion));
+    when(versionRepository.findByWorkflowId(anyString())).thenReturn(Collections.singletonList(noPublishedVersion));
     CamundaTranslatedWorkflowContext context = mock(CamundaTranslatedWorkflowContext.class);
     when(camundaEngine.translate(any(Workflow.class))).thenReturn(context);
     when(versionRepository.save(any(VersionedWorkflow.class))).thenReturn(noPublishedVersion);
@@ -179,10 +179,25 @@ void testUpdate_latestNoPublishedVersion_updateOnlySucceed() {
     verify(camundaEngine, never()).deploy(any(CamundaTranslatedWorkflowContext.class));
   }
 
+  @Test
+  void testUpdate_updatePublishedWorkflow() {
+    Properties properties = new Properties();
+    properties.setPublish(true);
+    workflow.setProperties(properties);
+    when(conveter.convert(anyString(), eq(Workflow.class))).thenReturn(workflow);
+    VersionedWorkflow publishedVersion = new VersionedWorkflow();
+    publishedVersion.setPublished(true);
+    when(versionRepository.findByWorkflowId(anyString())).thenReturn(Collections.singletonList(publishedVersion));
+
+    assertThatThrownBy(() -> workflowManagementService.update(swadlView))
+            .isInstanceOf(UnsupportedOperationException.class)
+            .hasMessage("Update on a published Workflow is forbidden.");
+  }
+
   @Test
   void testUpdate_noActiveVersion_notFoundException() {
     when(conveter.convert(anyString(), eq(Workflow.class))).thenReturn(workflow);
-    when(versionRepository.findByWorkflowIdAndPublishedFalse(anyString())).thenReturn(Optional.empty());
+    when(versionRepository.findByWorkflowId(anyString())).thenReturn(Collections.emptyList());
 
     assertThatThrownBy(() -> workflowManagementService.update(swadlView)).isInstanceOf(NotFoundException.class);
   }