Better error respones from WorkflowInstanceResource endpoints

CHANGELOG.md

@@ -9,6 +9,9 @@
 - `nflow-engine`
   - When shutdown is requested, stop processing workflows immediately after the current state has been executed.
   - Add `WorkflowExecutorLister.handlePotentiallyStuck(Duration processingTime)` to support custom handling when nFlow engine thinks the workflow state processing may be stuck. If any registered listener implementation returns true from this method, nFlow will interrupt the processing thread. The default implementation returns false.
+  - Throw `IllegalArgumentException` instead of `IllegalStateException` when trying to update workflow instance state to an invalid value.
+  - Throw `IllegalArugmentException` instead of `RuntimeException` when trying to insert workflow instance with unknown type or with a state that is not a start state.
+  - Make `StateVariableTooLongException` extend `IllegalArgumentException` instead of `RuntimeException`.
   - Dependency updates:
     - spring 5.2.5
     - jackson 2.10.3
@@ -27,6 +30,10 @@
     - spotbugs 4.0.2
     - hibernate 6.1.4
     - commons-lang3 3.10
+- `nflow-rest-api-jax-rs`
+  - Convert `IllegalArgumentException` to HTTP Bad Request in `WorkflowInstanceResource.insertWorkflowInstance` and `WorkflowInstanceResource.updateWorkflowInstance`.
+- `nflow-rest-api-spring-web`
+  - Convert `IllegalArgumentException` to HTTP Bad Request in `WorkflowInstanceResource.insertWorkflowInstance` and `WorkflowInstanceResource.updateWorkflowInstance`.
 - `nflow-explorer`
   - Dependency updates:
     - swagger-ui 2.2.10

nflow-engine/src/main/java/io/nflow/engine/internal/executor/WorkflowDispatcher.java

@@ -93,7 +93,7 @@ public void run() {
             logger.warn(pex.getMessage());
           } catch (@SuppressWarnings("unused") InterruptedException dropThrough) {
           } catch (Exception e) {
-            logger.error("Exception in executing dispatcher - retrying after sleep period (" + e.getMessage() + ")", e);
+            logger.error("Exception in executing dispatcher - retrying after sleep period ({})", e.getMessage(), e);
             sleep(false);
           }
         }

nflow-engine/src/main/java/io/nflow/engine/internal/executor/WorkflowStateProcessor.java

@@ -152,7 +152,7 @@ private void runImpl() {
       WorkflowState state;
       try {
         state = definition.getState(instance.state);
-      } catch (@SuppressWarnings("unused") IllegalStateException e) {
+      } catch (@SuppressWarnings("unused") IllegalArgumentException e) {
         rescheduleUnknownWorkflowState(instance);
         return;
       }
@@ -365,7 +365,7 @@ private void optionallyCleanupWorkflowInstanceHistory(WorkflowSettings settings,
         maintenanceDao.deleteActionAndStateHistory(instanceId, olderThan);
       }
     } catch (Throwable t) {
-      logger.error("Failure in workflow instance " + instanceId + " history cleanup", t);
+      logger.error("Failure in workflow instance {} history cleanup", instanceId, t);
     }
   }
 
@@ -493,8 +493,8 @@ public NextAction processState() {
             }
           }
         } catch (InvalidNextActionException e) {
-          logger.error("State '" + instance.state
-              + "' handler method failed to return valid next action, proceeding to error state '" + errorState + "'", e);
+          logger.error("State '{}' handler method failed to return valid next action, proceeding to error state '{}'",
+              instance.state, errorState, e);
           nextAction = moveToState(errorState, e.getMessage());
           execution.setFailed(e);
         }
@@ -522,7 +522,7 @@ private void processBeforeListeners() {
       try {
         listener.beforeProcessing(listenerContext);
       } catch (Throwable t) {
-        logger.error("Error in " + listener.getClass().getName() + ".beforeProcessing (" + t.getMessage() + ")", t);
+        logger.error("Error in {}.beforeProcessing ({})", listener.getClass().getName(), t.getMessage(), t);
       }
     }
   }
@@ -532,7 +532,7 @@ private void processAfterListeners() {
       try {
         listener.afterProcessing(listenerContext);
       } catch (Throwable t) {
-        logger.error("Error in " + listener.getClass().getName() + ".afterProcessing (" + t.getMessage() + ")", t);
+        logger.error("Error in {}.afterProcessing ({})", listener.getClass().getName(), t.getMessage(), t);
       }
     }
   }
@@ -542,7 +542,7 @@ private void processAfterFailureListeners(Throwable ex) {
       try {
         listener.afterFailure(listenerContext, ex);
       } catch (Throwable t) {
-        logger.error("Error in " + listener.getClass().getName() + ".afterFailure (" + t.getMessage() + ")", t);
+        logger.error("Error in {}.afterFailure ({})", listener.getClass().getName(), t.getMessage(), t);
       }
     }
   }

nflow-engine/src/main/java/io/nflow/engine/internal/workflow/WorkflowDefinitionScanner.java

@@ -108,7 +108,7 @@ Object defaultValue(StateVar stateInfo, Class<?> clazz) {
         ctr.newInstance();
         return ctr;
       } catch (NoSuchMethodException | SecurityException | InstantiationException | IllegalAccessException | IllegalArgumentException | InvocationTargetException e) {
-        logger.warn("Could not instantiate " + clazz + " using empty constructor", e);
+        logger.warn("Could not instantiate {} using empty constructor", clazz, e);
       }
     }
     return null;

nflow-engine/src/main/java/io/nflow/engine/internal/workflow/WorkflowInstancePreProcessor.java

@@ -27,18 +27,17 @@ public WorkflowInstancePreProcessor(WorkflowDefinitionService workflowDefinition
     this.workflowInstanceDao = workflowInstanceDao;
   }
 
-  // TODO should this set next_activation for child workflows?
   public WorkflowInstance process(WorkflowInstance instance) {
     AbstractWorkflowDefinition<?> def = workflowDefinitionService.getWorkflowDefinition(instance.type);
     if (def == null) {
-      throw new RuntimeException("No workflow definition found for type [" + instance.type + "]");
+      throw new IllegalArgumentException("No workflow definition found for type [" + instance.type + "]");
     }
     WorkflowInstance.Builder builder = new WorkflowInstance.Builder(instance);
     if (instance.state == null) {
       builder.setState(def.getInitialState().name());
     } else {
       if (!def.isStartState(instance.state)) {
-        throw new RuntimeException("Specified state [" + instance.state + "] is not a start state.");
+        throw new IllegalArgumentException("Specified state [" + instance.state + "] is not a start state.");
       }
     }
     if (isEmpty(instance.externalId)) {

nflow-engine/src/main/java/io/nflow/engine/workflow/definition/AbstractWorkflowDefinition.java

@@ -217,24 +217,26 @@ public WorkflowStateMethod getMethod(String stateName) {
 
   /**
    * Returns the workflow state for the given state name.
-   * @param state The name of the workflow state.
+   *
+   * @param state
+   *          The name of the workflow state.
    * @return The workflos state matching the state name.
-   * @throws IllegalStateException when a matching state can not be found.
+   * @throws IllegalArgumentException
+   *           when a matching state can not be found.
    */
   public WorkflowState getState(String state) {
-    for (WorkflowState s : getStates()) {
-      if (Objects.equals(s.name(), state)) {
-        return s;
-      }
-    }
-    throw new IllegalStateException("No state '" + state + "' in workflow definiton " + getType());
+    return getStates().stream().filter(s -> Objects.equals(s.name(), state)).findFirst()
+        .orElseThrow(() -> new IllegalArgumentException("No state '" + state + "' in workflow definiton " + getType()));
   }
 
   /**
    * Check if the given state is a valid start state.
-   * @param state The name of the workflow state.
+   *
+   * @param state
+   *          The name of the workflow state.
    * @return True if the given state is a valid start date, false otherwise.
-   * @throws IllegalStateException if the given state name does not match any state.
+   * @throws IllegalArgumentException
+   *           if the given state name does not match any state.
    */
   public boolean isStartState(String state) {
     return getState(state).getType() == WorkflowStateType.start;

nflow-engine/src/main/java/io/nflow/engine/workflow/executor/StateVariableValueTooLongException.java

@@ -1,6 +1,6 @@
 package io.nflow.engine.workflow.executor;
 
-public class StateVariableValueTooLongException extends RuntimeException {
+public class StateVariableValueTooLongException extends IllegalArgumentException {
 
   private static final long serialVersionUID = 1L;
 

nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java

@@ -36,7 +36,6 @@
 
 import io.nflow.engine.config.NFlow;
 import io.nflow.jetty.mapper.CustomValidationExceptionMapper;
-import io.nflow.jetty.mapper.StateVariableValueTooLongExceptionMapper;
 import io.nflow.rest.config.RestConfiguration;
 import io.nflow.rest.config.jaxrs.CorsHeaderContainerResponseFilter;
 import io.nflow.rest.config.jaxrs.DateTimeParamConverterProvider;
@@ -74,11 +73,9 @@ public Server jaxRsServer(WorkflowInstanceResource workflowInstanceResource,
     }
     factory.setProviders(asList(
         jsonProvider(nflowRestObjectMapper),
-        validationExceptionMapper(),
         corsHeadersProvider(),
         new WebApplicationExceptionMapper(),
         new CustomValidationExceptionMapper(),
-        new StateVariableValueTooLongExceptionMapper(),
         new DateTimeParamConverterProvider()
         ));
     factory.setFeatures(asList(new LoggingFeature(), swaggerFeature()));
@@ -113,11 +110,6 @@ public JacksonJsonProvider jsonProvider(@Named(REST_OBJECT_MAPPER) ObjectMapper
     return new JacksonJsonProvider(nflowRestObjectMapper);
   }
 
-  @Bean
-  public CustomValidationExceptionMapper validationExceptionMapper() {
-    return new CustomValidationExceptionMapper();
-  }
-
   @Bean(destroyMethod = "shutdown")
   public SpringBus cxf() {
     return new SpringBus();

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

@@ -1,8 +1,6 @@
 package io.nflow.jetty.mapper;
 
 import static java.util.Arrays.asList;
-import static org.eclipse.jetty.http.HttpStatus.BAD_REQUEST_400;
-import static org.eclipse.jetty.http.HttpStatus.INTERNAL_SERVER_ERROR_500;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
 import static org.mockito.Mockito.mock;
@@ -48,7 +46,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(BAD_REQUEST_400));
+      assertThat(response.getStatus(), is(Response.Status.BAD_REQUEST.getStatusCode()));
       ErrorResponse error = (ErrorResponse) response.getEntity();
       assertThat(error.error, is("violationPath: violationMessage"));
     }
@@ -59,7 +57,7 @@ public void responseConstraintViolationExceptionCausesInternalServerError() {
     ConstraintViolationException exception = mock(ResponseConstraintViolationException.class);
     when(exception.getMessage()).thenReturn("error");
     try (Response response = exceptionMapper.toResponse(exception)) {
-      assertThat(response.getStatus(), is(INTERNAL_SERVER_ERROR_500));
+      assertThat(response.getStatus(), is(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode()));
       ErrorResponse error = (ErrorResponse) response.getEntity();
       assertThat(error.error, is("error"));
     }
@@ -70,7 +68,7 @@ public void otherExceptionsCauseInternalServerException() {
     ValidationException exception = mock(ValidationException.class);
     when(exception.getMessage()).thenReturn("error");
     try (Response response = exceptionMapper.toResponse(exception)) {
-      assertThat(response.getStatus(), is(INTERNAL_SERVER_ERROR_500));
+      assertThat(response.getStatus(), is(Response.Status.INTERNAL_SERVER_ERROR.getStatusCode()));
       ErrorResponse error = (ErrorResponse) response.getEntity();
       assertThat(error.error, is("error"));
     }

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

@@ -9,7 +9,9 @@
 import static javax.ws.rs.core.Response.noContent;
 import static javax.ws.rs.core.Response.ok;
 import static javax.ws.rs.core.Response.status;
+import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
 import static javax.ws.rs.core.Response.Status.CONFLICT;
+import static javax.ws.rs.core.Response.Status.NOT_FOUND;
 
 import java.net.URI;
 import java.util.Collections;
@@ -21,7 +23,6 @@
 import javax.validation.Valid;
 import javax.ws.rs.Consumes;
 import javax.ws.rs.GET;
-import javax.ws.rs.NotFoundException;
 import javax.ws.rs.OPTIONS;
 import javax.ws.rs.PUT;
 import javax.ws.rs.Path;
@@ -30,10 +31,10 @@
 import javax.ws.rs.QueryParam;
 import javax.ws.rs.core.Response;
 
-import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import org.springframework.dao.EmptyResultDataAccessException;
 import org.springframework.stereotype.Component;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import io.nflow.engine.internal.dao.WorkflowInstanceDao;
 import io.nflow.engine.service.WorkflowInstanceInclude;
 import io.nflow.engine.service.WorkflowInstanceService;
@@ -86,7 +87,7 @@ public WorkflowInstanceResource(WorkflowInstanceService workflowInstances, Creat
   @ApiOperation(value = "CORS preflight handling")
   @Consumes(WILDCARD)
   public Response corsPreflight() {
-    return Response.ok().build();
+    return ok().build();
   }
 
   @PUT
@@ -96,9 +97,13 @@ public Response corsPreflight() {
   public Response createWorkflowInstance(
       @Valid @ApiParam(value = "Submitted workflow instance information", required = true) CreateWorkflowInstanceRequest req) {
     WorkflowInstance instance = createWorkflowConverter.convert(req);
-    long id = workflowInstances.insertWorkflowInstance(instance);
-    instance = workflowInstances.getWorkflowInstance(id, EnumSet.of(WorkflowInstanceInclude.CURRENT_STATE_VARIABLES), null);
-    return created(URI.create(String.valueOf(id))).entity(createWorkflowConverter.convert(instance)).build();
+    try {
+      long id = workflowInstances.insertWorkflowInstance(instance);
+      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();
+    }
   }
 
   @PUT
@@ -110,22 +115,25 @@ public Response createWorkflowInstance(
     @ApiResponse(code = 409, message = "If workflow was executing and no update was done") })
   public Response updateWorkflowInstance(@ApiParam("Internal id for workflow instance") @PathParam("id") long id,
       @ApiParam("Submitted workflow instance information") UpdateWorkflowInstanceRequest req) {
-    boolean updated = super.updateWorkflowInstance(id, req, workflowInstanceFactory, workflowInstances, workflowInstanceDao);
-    return (updated ? noContent() : status(CONFLICT)).build();
+    try {
+      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();
+    }
   }
 
   @GET
   @Path("/id/{id}")
   @ApiOperation(value = "Fetch a workflow instance", notes = "Fetch full state and action history of a single workflow instance.")
   @SuppressFBWarnings(value = "LEST_LOST_EXCEPTION_STACK_TRACE", justification = "The empty result exception contains no useful information")
-  public ListWorkflowInstanceResponse fetchWorkflowInstance(
-      @ApiParam("Internal id for workflow instance") @PathParam("id") long id,
+  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 super.fetchWorkflowInstance(id, include, maxActions, workflowInstances, listWorkflowConverter);
+      return ok(super.fetchWorkflowInstance(id, include, maxActions, workflowInstances, listWorkflowConverter)).build();
     } catch (@SuppressWarnings("unused") EmptyResultDataAccessException e) {
-      throw new NotFoundException(format("Workflow instance %s not found", id));
+      return status(NOT_FOUND).entity(format("Workflow instance %s not found", id)).build();
     }
   }