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,8 @@
 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 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;
@@ -48,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(BAD_REQUEST_400));
+      assertThat(response.getStatus(), is(BAD_REQUEST.getStatusCode()));
       ErrorResponse error = (ErrorResponse) response.getEntity();
       assertThat(error.error, is("violationPath: violationMessage"));
     }
@@ -59,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(INTERNAL_SERVER_ERROR_500));
+      assertThat(response.getStatus(), is(INTERNAL_SERVER_ERROR.getStatusCode()));
       ErrorResponse error = (ErrorResponse) response.getEntity();
       assertThat(error.error, is("error"));
     }
@@ -70,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(INTERNAL_SERVER_ERROR_500));
+      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;
+
+}