review fixes: make WorkflowInstanceResource error responses to return…

… json body

nflow-jetty/src/test/java/io/nflow/jetty/mapper/CustomValidationExceptionMapperTest.java

@@ -1,6 +1,8 @@
 package io.nflow.jetty.mapper;
 
 import static java.util.Arrays.asList;
+import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
+import static javax.ws.rs.core.Response.Status.INTERNAL_SERVER_ERROR;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
 import static org.mockito.Mockito.mock;
@@ -46,7 +48,7 @@ public void constraintViolationExceptionCausesBadRequest() {
     ConstraintViolationException exception = mock(ConstraintViolationException.class);
     when(exception.getConstraintViolations()).thenReturn(new LinkedHashSet(asList(violation)));
     try (Response response = exceptionMapper.toResponse(exception)) {
-      assertThat(response.getStatus(), is(Response.Status.BAD_REQUEST.getStatusCode()));
+      assertThat(response.getStatus(), is(BAD_REQUEST.getStatusCode()));
       ErrorResponse error = (ErrorResponse) response.getEntity();
       assertThat(error.error, is("violationPath: violationMessage"));
     }
@@ -57,7 +59,7 @@ public void responseConstraintViolationExceptionCausesInternalServerError() {
     ConstraintViolationException exception = mock(ResponseConstraintViolationException.class);
     when(exception.getMessage()).thenReturn("error");
     try (Response response = exceptionMapper.toResponse(exception)) {
-      assertThat(response.getStatus(), is(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode()));
+      assertThat(response.getStatus(), is(INTERNAL_SERVER_ERROR.getStatusCode()));
       ErrorResponse error = (ErrorResponse) response.getEntity();
       assertThat(error.error, is("error"));
     }
@@ -68,7 +70,7 @@ public void otherExceptionsCauseInternalServerException() {
     ValidationException exception = mock(ValidationException.class);
     when(exception.getMessage()).thenReturn("error");
     try (Response response = exceptionMapper.toResponse(exception)) {
-      assertThat(response.getStatus(), is(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode()));
+      assertThat(response.getStatus(), is(INTERNAL_SERVER_ERROR.getStatusCode()));
       ErrorResponse error = (ErrorResponse) response.getEntity();
       assertThat(error.error, is("error"));
     }

nflow-rest-api-common/src/main/java/io/nflow/rest/v1/msg/SetSignalResponse.java

@@ -0,0 +1,15 @@
+package io.nflow.rest.v1.msg;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+import io.nflow.engine.model.ModelObject;
+import io.swagger.annotations.ApiModel;
+import io.swagger.annotations.ApiModelProperty;
+
+@ApiModel(description = "Response to wake up request.")
+@SuppressFBWarnings(value = "URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD", justification = "jackson reads dto fields")
+public class SetSignalResponse extends ModelObject {
+
+  @ApiModelProperty("True if the signal was set, false otherwise.")
+  public boolean setSignalSuccess;
+
+}

nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java

@@ -48,8 +48,10 @@
 import io.nflow.rest.v1.converter.ListWorkflowInstanceConverter;
 import io.nflow.rest.v1.msg.CreateWorkflowInstanceRequest;
 import io.nflow.rest.v1.msg.CreateWorkflowInstanceResponse;
+import io.nflow.rest.v1.msg.ErrorResponse;
 import io.nflow.rest.v1.msg.ListWorkflowInstanceResponse;
 import io.nflow.rest.v1.msg.SetSignalRequest;
+import io.nflow.rest.v1.msg.SetSignalResponse;
 import io.nflow.rest.v1.msg.UpdateWorkflowInstanceRequest;
 import io.nflow.rest.v1.msg.WakeupRequest;
 import io.nflow.rest.v1.msg.WakeupResponse;
@@ -102,14 +104,13 @@ public Response createWorkflowInstance(
       instance = workflowInstances.getWorkflowInstance(id, EnumSet.of(WorkflowInstanceInclude.CURRENT_STATE_VARIABLES), null);
       return created(URI.create(String.valueOf(id))).entity(createWorkflowConverter.convert(instance)).build();
     } catch (IllegalArgumentException e) {
-      return status(BAD_REQUEST).entity(e.getMessage()).build();
+      return status(BAD_REQUEST).entity(new ErrorResponse(e.getMessage())).build();
     }
   }
 
   @PUT
   @Path("/id/{id}")
-  @ApiOperation(value = "Update workflow instance", notes = "The service is typically used in manual state "
-      + "transition via nFlow Explorer or a business UI.")
+  @ApiOperation(value = "Update workflow instance", notes = "The service is typically used in manual state transition via nFlow Explorer or a business UI.")
   @ApiResponses({ @ApiResponse(code = 204, message = "If update was successful"),
     @ApiResponse(code = 400, message = "If instance could not be updated, for example when state variable value was too long"),
     @ApiResponse(code = 409, message = "If workflow was executing and no update was done") })
@@ -119,21 +120,23 @@ public Response updateWorkflowInstance(@ApiParam("Internal id for workflow insta
       boolean updated = super.updateWorkflowInstance(id, req, workflowInstanceFactory, workflowInstances, workflowInstanceDao);
       return (updated ? noContent() : status(CONFLICT)).build();
     } catch (IllegalArgumentException e) {
-      return status(BAD_REQUEST).entity(e.getMessage()).build();
+      return status(BAD_REQUEST).entity(new ErrorResponse(e.getMessage())).build();
     }
   }
 
   @GET
   @Path("/id/{id}")
   @ApiOperation(value = "Fetch a workflow instance", notes = "Fetch full state and action history of a single workflow instance.")
+  @ApiResponses({ @ApiResponse(code = 200, response = ListWorkflowInstanceResponse.class, message = "If instance was found"),
+    @ApiResponse(code = 404, message = "If instance was not found") })
   @SuppressFBWarnings(value = "LEST_LOST_EXCEPTION_STACK_TRACE", justification = "The empty result exception contains no useful information")
   public Response fetchWorkflowInstance(@ApiParam("Internal id for workflow instance") @PathParam("id") long id,
       @QueryParam("include") @ApiParam(value = INCLUDE_PARAM_DESC, allowableValues = INCLUDE_PARAM_VALUES, allowMultiple = true) String include,
       @QueryParam("maxActions") @ApiParam("Maximum number of actions returned for each workflow instance") Long maxActions) {
     try {
       return ok(super.fetchWorkflowInstance(id, include, maxActions, workflowInstances, listWorkflowConverter)).build();
     } catch (@SuppressWarnings("unused") EmptyResultDataAccessException e) {
-      return status(NOT_FOUND).entity(format("Workflow instance %s not found", id)).build();
+      return status(NOT_FOUND).entity(new ErrorResponse(format("Workflow instance %s not found", id))).build();
     }
   }
 
@@ -159,11 +162,12 @@ public Iterator<ListWorkflowInstanceResponse> listWorkflowInstances(
   @PUT
   @Path("/{id}/signal")
   @ApiOperation(value = "Set workflow instance signal value", notes = "The service may be used for example to interrupt executing workflow instance.")
-  @ApiResponses({ @ApiResponse(code = 200, message = "When operation was successful") })
-  public Response setSignal(@ApiParam("Internal id for workflow instance") @PathParam("id") long id,
+  @ApiResponses({ @ApiResponse(code = 200, message = "When setting the signal was attempted") })
+  public SetSignalResponse setSignal(@ApiParam("Internal id for workflow instance") @PathParam("id") long id,
       @Valid @ApiParam("New signal value") SetSignalRequest req) {
-    boolean updated = workflowInstances.setSignal(id, ofNullable(req.signal), req.reason, WorkflowActionType.externalChange);
-    return (updated ? ok("Signal was set successfully") : ok("Signal was not set")).build();
+    SetSignalResponse response = new SetSignalResponse();
+    response.setSignalSuccess = workflowInstances.setSignal(id, ofNullable(req.signal), req.reason, WorkflowActionType.externalChange);
+    return response;
   }
 
   @PUT

nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResourceTest.java

@@ -12,6 +12,8 @@
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.lenient;
@@ -53,8 +55,10 @@
 import io.nflow.rest.v1.converter.CreateWorkflowConverter;
 import io.nflow.rest.v1.converter.ListWorkflowInstanceConverter;
 import io.nflow.rest.v1.msg.CreateWorkflowInstanceRequest;
+import io.nflow.rest.v1.msg.ErrorResponse;
 import io.nflow.rest.v1.msg.ListWorkflowInstanceResponse;
 import io.nflow.rest.v1.msg.SetSignalRequest;
+import io.nflow.rest.v1.msg.SetSignalResponse;
 import io.nflow.rest.v1.msg.UpdateWorkflowInstanceRequest;
 
 @ExtendWith(MockitoExtension.class)
@@ -145,7 +149,7 @@ public void whenUpdatingStateWithDescriptionUpdateWorkflowInstanceWorks() {
   @Test
   public void whenUpdatingNextActivationTimeUpdateWorkflowInstanceWorks() {
     UpdateWorkflowInstanceRequest req = new UpdateWorkflowInstanceRequest();
-    req.nextActivationTime = new DateTime(2014,11,12,17,55,0);
+    req.nextActivationTime = new DateTime(2014, 11, 12, 17, 55, 0);
     resource.updateWorkflowInstance(3, req);
     verify(workflowInstances).updateWorkflowInstance(
         (WorkflowInstance) argThat(allOf(hasField("state", equalTo(null)), hasField("status", equalTo(null)))),
@@ -225,7 +229,7 @@ public void fetchingNonExistingWorkflowReturnsNotFound() {
     when(workflowInstances.getWorkflowInstance(42, emptySet(), null)).thenThrow(EmptyResultDataAccessException.class);
     try (Response response = resource.fetchWorkflowInstance(42, null, null)) {
       assertThat(response.getStatus(), is(equalTo(NOT_FOUND.getStatusCode())));
-      assertThat(response.readEntity(String.class), is(equalTo("Workflow instance 42 not found")));
+      assertThat(response.readEntity(ErrorResponse.class).error, is(equalTo("Workflow instance 42 not found")));
     }
   }
 
@@ -256,31 +260,28 @@ public void fetchingExistingWorkflowWorksWithAllIncludes() {
   }
 
   @Test
-  public void setSignalWorks() {
+  public void setSignalSuccessIsTrueWhenSignalWasSet() {
     SetSignalRequest req = new SetSignalRequest();
     req.signal = 42;
     req.reason = "testing";
     when(workflowInstances.setSignal(99, Optional.of(42), "testing", WorkflowActionType.externalChange)).thenReturn(true);
 
-    try (Response response = resource.setSignal(99, req)) {
-      verify(workflowInstances).setSignal(99, Optional.of(42), "testing", WorkflowActionType.externalChange);
-      assertThat(response.getStatus(), is(Response.Status.OK.getStatusCode()));
-      assertThat(response.readEntity(String.class), is("Signal was set successfully"));
-    }
+    SetSignalResponse response = resource.setSignal(99, req);
+
+    verify(workflowInstances).setSignal(99, Optional.of(42), "testing", WorkflowActionType.externalChange);
+    assertTrue(response.setSignalSuccess);
   }
 
   @Test
-  public void setSignalReturnsOkWhenSignalIsNotUpdated() {
+  public void setSignalSuccessIsFalseWhenSignalWasNotSet() {
     SetSignalRequest req = new SetSignalRequest();
     req.signal = null;
     req.reason = "testing";
     when(workflowInstances.setSignal(99, Optional.empty(), "testing", WorkflowActionType.externalChange)).thenReturn(false);
 
-    try (Response response = resource.setSignal(99, req)) {
-      verify(workflowInstances).setSignal(99, Optional.empty(), "testing", WorkflowActionType.externalChange);
-      assertThat(response.getStatus(), is(Response.Status.OK.getStatusCode()));
-      assertThat(response.readEntity(String.class), is("Signal was not set"));
-    }
-  }
+    SetSignalResponse response = resource.setSignal(99, req);
 
+    verify(workflowInstances).setSignal(99, Optional.empty(), "testing", WorkflowActionType.externalChange);
+    assertFalse(response.setSignalSuccess);
+  }
 }

nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java

@@ -46,8 +46,10 @@
 import io.nflow.rest.v1.converter.ListWorkflowInstanceConverter;
 import io.nflow.rest.v1.msg.CreateWorkflowInstanceRequest;
 import io.nflow.rest.v1.msg.CreateWorkflowInstanceResponse;
+import io.nflow.rest.v1.msg.ErrorResponse;
 import io.nflow.rest.v1.msg.ListWorkflowInstanceResponse;
 import io.nflow.rest.v1.msg.SetSignalRequest;
+import io.nflow.rest.v1.msg.SetSignalResponse;
 import io.nflow.rest.v1.msg.UpdateWorkflowInstanceRequest;
 import io.nflow.rest.v1.msg.WakeupRequest;
 import io.nflow.rest.v1.msg.WakeupResponse;
@@ -91,7 +93,7 @@ public ResponseEntity<?> createWorkflowInstance(
       instance = workflowInstances.getWorkflowInstance(id, EnumSet.of(WorkflowInstanceInclude.CURRENT_STATE_VARIABLES), null);
       return created(URI.create(String.valueOf(id))).body(createWorkflowConverter.convert(instance));
     } catch (IllegalArgumentException e) {
-      return status(BAD_REQUEST).body(e.getMessage());
+      return status(BAD_REQUEST).body(new ErrorResponse((e.getMessage())));
     }
   }
 
@@ -107,12 +109,14 @@ public ResponseEntity<?> updateWorkflowInstance(@ApiParam("Internal id for workf
       boolean updated = super.updateWorkflowInstance(id, req, workflowInstanceFactory, workflowInstances, workflowInstanceDao);
       return (updated ? noContent() : status(CONFLICT)).build();
     } catch (IllegalArgumentException e) {
-      return status(BAD_REQUEST).body(e.getMessage());
+      return status(BAD_REQUEST).body(new ErrorResponse(e.getMessage()));
     }
   }
 
   @GetMapping(path = "/id/{id}")
   @ApiOperation(value = "Fetch a workflow instance", notes = "Fetch full state and action history of a single workflow instance.")
+  @ApiResponses({ @ApiResponse(code = 200, response = ListWorkflowInstanceResponse.class, message = "If instance was found"),
+      @ApiResponse(code = 404, message = "If instance was not found") })
   @SuppressFBWarnings(value = "LEST_LOST_EXCEPTION_STACK_TRACE", justification = "The empty result exception contains no useful information")
   public ResponseEntity<?> fetchWorkflowInstance(
       @ApiParam("Internal id for workflow instance") @PathVariable("id") long id,
@@ -121,7 +125,7 @@ public ResponseEntity<?> fetchWorkflowInstance(
     try {
       return ok().body(super.fetchWorkflowInstance(id, include, maxActions, this.workflowInstances, this.listWorkflowConverter));
     } catch (@SuppressWarnings("unused") EmptyResultDataAccessException e) {
-      return status(NOT_FOUND).body(format("Workflow instance %s not found", id));
+      return status(NOT_FOUND).body(new ErrorResponse(format("Workflow instance %s not found", id)));
     }
   }
 
@@ -145,11 +149,13 @@ public Iterator<ListWorkflowInstanceResponse> listWorkflowInstances(
 
   @PutMapping(path = "/{id}/signal", consumes = APPLICATION_JSON_VALUE)
   @ApiOperation(value = "Set workflow instance signal value", notes = "The service may be used for example to interrupt executing workflow instance.")
-  @ApiResponses({ @ApiResponse(code = 200, message = "When operation was successful") })
-  public ResponseEntity<?> setSignal(@ApiParam("Internal id for workflow instance") @PathVariable("id") long id,
+  @ApiResponses({ @ApiResponse(code = 200, message = "When setting the signal was attempted") })
+  public SetSignalResponse setSignal(@ApiParam("Internal id for workflow instance") @PathVariable("id") long id,
       @RequestBody @Valid @ApiParam("New signal value") SetSignalRequest req) {
-    boolean updated = workflowInstances.setSignal(id, ofNullable(req.signal), req.reason, WorkflowActionType.externalChange);
-    return (updated ? ok("Signal was set successfully") : ok("Signal was not set"));
+    SetSignalResponse response = new SetSignalResponse();
+    response.setSignalSuccess = workflowInstances.setSignal(id, ofNullable(req.signal), req.reason,
+        WorkflowActionType.externalChange);
+    return response;
   }
 
   @PutMapping(path = "/{id}/wakeup", consumes = APPLICATION_JSON_VALUE)

nflow-tests/src/test/java/io/nflow/tests/AbstractNflowTest.java

@@ -33,6 +33,7 @@
 import io.nflow.rest.v1.msg.MaintenanceRequest.MaintenanceRequestItem;
 import io.nflow.rest.v1.msg.MaintenanceResponse;
 import io.nflow.rest.v1.msg.SetSignalRequest;
+import io.nflow.rest.v1.msg.SetSignalResponse;
 import io.nflow.rest.v1.msg.StatisticsResponse;
 import io.nflow.rest.v1.msg.UpdateWorkflowInstanceRequest;
 import io.nflow.rest.v1.msg.WakeupRequest;
@@ -100,11 +101,11 @@ protected WakeupResponse wakeup(long instanceId, List<String> expectedStates) {
     return getInstanceResource(instanceId).path("wakeup").put(request, WakeupResponse.class);
   }
 
-  protected String setSignal(long instanceId, int signal, String reason) {
+  protected SetSignalResponse setSignal(long instanceId, int signal, String reason) {
     SetSignalRequest request = new SetSignalRequest();
     request.signal = signal;
     request.reason = reason;
-    return getInstanceResource(instanceId).path("signal").put(request, String.class);
+    return getInstanceResource(instanceId).path("signal").put(request, SetSignalResponse.class);
   }
 
   private WebClient getInstanceResource(long instanceId) {

nflow-tests/src/test/java/io/nflow/tests/CreditApplicationWorkflowTest.java

@@ -3,6 +3,7 @@
 import static io.nflow.engine.workflow.instance.WorkflowInstanceAction.WorkflowActionType.stateExecution;
 import static java.time.Duration.ofSeconds;
 import static java.util.Arrays.asList;
+import static javax.ws.rs.core.MediaType.APPLICATION_JSON_TYPE;
 import static org.apache.cxf.jaxrs.client.WebClient.fromClient;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -25,6 +26,7 @@
 import io.nflow.rest.v1.msg.Action;
 import io.nflow.rest.v1.msg.CreateWorkflowInstanceRequest;
 import io.nflow.rest.v1.msg.CreateWorkflowInstanceResponse;
+import io.nflow.rest.v1.msg.ErrorResponse;
 import io.nflow.rest.v1.msg.UpdateWorkflowInstanceRequest;
 import io.nflow.tests.demo.workflow.CreditApplicationWorkflow;
 import io.nflow.tests.extension.NflowServerConfig;
@@ -77,7 +79,8 @@ public void moveToInvalidStateFails() {
     ureq.state = "invalid";
     try (Response response = fromClient(workflowInstanceIdResource, true).path(resp.id).put(ureq)) {
       assertThat(response.getStatus(), is(Response.Status.BAD_REQUEST.getStatusCode()));
-      assertThat(response.readEntity(String.class), startsWith("No state 'invalid'"));
+      assertThat(response.getMediaType(), is(APPLICATION_JSON_TYPE));
+      assertThat(response.readEntity(ErrorResponse.class).error, startsWith("No state 'invalid'"));
     }
   }
 

nflow-tests/src/test/java/io/nflow/tests/SignalWorkflowTest.java

@@ -6,9 +6,9 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.notNullValue;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.fail;
 
-import io.nflow.tests.extension.NflowServerConfig;
 import org.junit.jupiter.api.MethodOrderer;
 import org.junit.jupiter.api.Order;
 import org.junit.jupiter.api.Test;
@@ -22,6 +22,7 @@
 import io.nflow.rest.v1.msg.ListWorkflowInstanceResponse;
 import io.nflow.tests.demo.workflow.DemoWorkflow;
 import io.nflow.tests.demo.workflow.SlowWorkflow;
+import io.nflow.tests.extension.NflowServerConfig;
 
 @TestMethodOrder(MethodOrderer.OrderAnnotation.class)
 public class SignalWorkflowTest extends AbstractNflowTest {
@@ -65,8 +66,7 @@ public void checkSlowWorkflowIsRunning() throws Exception {
   @Test
   @Order(3)
   public void interruptWorkflowWithSignal() {
-    assertThat(setSignal(resp.id, SlowWorkflow.SIGNAL_INTERRUPT, "Setting signal via REST API"),
-        is("Signal was set successfully"));
+    assertTrue(setSignal(resp.id, SlowWorkflow.SIGNAL_INTERRUPT, "Setting signal via REST API").setSignalSuccess);
   }
 
   @Test