Return HTTP 400 Bad Request if trying to insert or update state variables with too long value via REST API

CHANGELOG.md

@@ -6,7 +6,8 @@
 
 **Details**
 - `nflow-engine`
-  - Throw `StateVariableValueTooLongException` if a state variable value that does not fit into the database column is detected. Checked in `StateExecution.setVariable`, `StateExecution.addWorkflows`, `StateExecution.addChildWorkflows`, `WorkflowInstanceService.insertWorkflowInstance` and when creating a new instance via REST API. If the exception is thrown during state processing and not handled by the state implementation, nFlow engine will catch the exception and retry state processing after delay configured by property `nflow.executor.stateVariableValueTooLongRetryDelay.minutes` (default is 60).
+  - Throw `StateVariableValueTooLongException` if a state variable value that does not fit into the database column is detected. Checked in `StateExecution.setVariable`, `StateExecution.addWorkflows`, `StateExecution.addChildWorkflows`, `WorkflowInstanceService.insertWorkflowInstance` and when creating a new or updating an existing instance via REST API.
+    If the exception is thrown during state processing and not handled by the state implementation, nFlow engine will catch the exception and retry state processing after delay configured by property `nflow.executor.stateVariableValueTooLongRetryDelay.minutes` (default is 60).
   - Fix honoring of `includeCurrentStateVariables` flag in `WorkflowInstanceService.listWorkflowInstances`. This caused major slowness when using bulk workflows.
     To preserve the existing (incorrect) default behaviour in backwards compatible way the default value in `QueryWorkflowInstances.Builder` is changed to `true`. The REST API is unaffected.
     Especially in workflows with many children that use the `StateExecution.getAllChildWorkflows` method the performance impact can be high. Before 7.0.0 release, it is recommended to use `StateExecution.queryChildWorkflows(new QueryWorkflowInstances.Builder().setIncludeCurrentStateVariables(false).build())` if state variables are not needed.
@@ -17,6 +18,9 @@
     - spotbugs 4.0.0-beta4
     - spotbugs-annotations 4.0.0-beta4
     - swagger 1.6.0
+- `nflow-rest-api`
+  - REST API returns HTTP 400 in case the state variable value is too long when inserting a new or updating an existing workflow instance
+  - Exception mappers return JSON objects instead of raw strings
 - `nflow-explorer`
   - Dependency updates:
     - angular 1.7.9

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

@@ -18,6 +18,7 @@
 import org.apache.cxf.feature.Feature;
 import org.apache.cxf.interceptor.Interceptor;
 import org.apache.cxf.jaxrs.JAXRSServerFactoryBean;
+import org.apache.cxf.jaxrs.impl.WebApplicationExceptionMapper;
 import org.apache.cxf.jaxrs.swagger.Swagger2Feature;
 import org.apache.cxf.jaxrs.validation.JAXRSBeanValidationInInterceptor;
 import org.apache.cxf.jaxrs.validation.JAXRSBeanValidationOutInterceptor;
@@ -35,9 +36,8 @@
 import com.fasterxml.jackson.jaxrs.json.JacksonJsonProvider;
 
 import io.nflow.engine.config.NFlow;
-import io.nflow.jetty.mapper.BadRequestExceptionMapper;
 import io.nflow.jetty.mapper.CustomValidationExceptionMapper;
-import io.nflow.jetty.mapper.NotFoundExceptionMapper;
+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;
@@ -77,8 +77,9 @@ public Server jaxRsServer(WorkflowInstanceResource workflowInstanceResource,
         jsonProvider(nflowRestObjectMapper),
         validationExceptionMapper(),
         corsHeadersProvider(),
-        notFoundExceptionMapper(),
-        new BadRequestExceptionMapper(),
+        new WebApplicationExceptionMapper(),
+        new CustomValidationExceptionMapper(),
+        new StateVariableValueTooLongExceptionMapper(),
         new DateTimeParamConverterProvider()
         ));
     factory.setFeatures(asList(new LoggingFeature(), swaggerFeature()));
@@ -118,11 +119,6 @@ public CustomValidationExceptionMapper validationExceptionMapper() {
     return new CustomValidationExceptionMapper();
   }
 
-  @Bean
-  public NotFoundExceptionMapper notFoundExceptionMapper() {
-    return new NotFoundExceptionMapper();
-  }
-
   @Bean(destroyMethod = "shutdown")
   public SpringBus cxf() {
     return new SpringBus();

nflow-jetty/src/main/java/io/nflow/jetty/mapper/CustomValidationExceptionMapper.java

@@ -1,6 +1,7 @@
 package io.nflow.jetty.mapper;
 
-import javax.validation.ConstraintViolation;
+import static java.util.stream.Collectors.joining;
+
 import javax.validation.ConstraintViolationException;
 import javax.validation.ValidationException;
 import javax.ws.rs.core.Response;
@@ -11,28 +12,25 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import io.nflow.rest.v1.msg.ErrorResponse;
+
 @Provider
 public class CustomValidationExceptionMapper implements ExceptionMapper<ValidationException> {
 
-  private static final Logger logger = LoggerFactory.getLogger(CustomValidationExceptionMapper.class);
+  static final Logger logger = LoggerFactory.getLogger(CustomValidationExceptionMapper.class);
 
   @Override
   public Response toResponse(ValidationException exception) {
-    if (exception instanceof ConstraintViolationException) {
-      final ConstraintViolationException constraint = (ConstraintViolationException) exception;
-      final boolean isResponseException = constraint instanceof ResponseConstraintViolationException;
-      StringBuilder sb = new StringBuilder();
-      for (final ConstraintViolation<?> violation: constraint.getConstraintViolations()) {
-        logger.warn("{}.{}: {}",violation.getRootBeanClass().getSimpleName(), violation.getPropertyPath(), violation.getMessage());
-        sb.append(violation.getPropertyPath()).append(": ").append(violation.getMessage()).append(", ");
-      }
-      sb.setLength(sb.length() - 2);
-      if (isResponseException) {
-        return Response.status(Response.Status.INTERNAL_SERVER_ERROR).build();
-      }
-      return Response.status(Response.Status.BAD_REQUEST).entity(sb).build();
+    if (exception instanceof ConstraintViolationException && !(exception instanceof ResponseConstraintViolationException)) {
+      ConstraintViolationException constraint = (ConstraintViolationException) exception;
+      String error = constraint.getConstraintViolations().stream().map(violation -> {
+        logger.warn("{}.{}: {}", violation.getRootBeanClass().getSimpleName(), violation.getPropertyPath(),
+            violation.getMessage());
+        return violation.getPropertyPath() + ": " + violation.getMessage();
+      }).collect(joining(", "));
+      return Response.status(Response.Status.BAD_REQUEST).entity(new ErrorResponse(error)).build();
     }
-    return Response.status(Response.Status.INTERNAL_SERVER_ERROR).build();
+    return Response.status(Response.Status.INTERNAL_SERVER_ERROR).entity(new ErrorResponse(exception.getMessage())).build();
   }
 
 }

nflow-jetty/src/main/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapper.java

@@ -0,0 +1,18 @@
+package io.nflow.jetty.mapper;
+
+import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
+
+import javax.ws.rs.core.Response;
+import javax.ws.rs.ext.ExceptionMapper;
+import javax.ws.rs.ext.Provider;
+
+import io.nflow.engine.workflow.executor.StateVariableValueTooLongException;
+import io.nflow.rest.v1.msg.ErrorResponse;
+
+@Provider
+public class StateVariableValueTooLongExceptionMapper implements ExceptionMapper<StateVariableValueTooLongException> {
+  @Override
+  public Response toResponse(StateVariableValueTooLongException e) {
+    return Response.status(BAD_REQUEST).entity(new ErrorResponse(e.getMessage())).build();
+  }
+}

nflow-jetty/src/main/java/io/nflow/jetty/mapper/WebApplicationExceptionMapper.java

@@ -0,0 +1,18 @@
+package io.nflow.jetty.mapper;
+
+import static javax.ws.rs.core.Response.status;
+
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.Response;
+import javax.ws.rs.ext.ExceptionMapper;
+import javax.ws.rs.ext.Provider;
+
+import io.nflow.rest.v1.msg.ErrorResponse;
+
+@Provider
+public class WebApplicationExceptionMapper implements ExceptionMapper<WebApplicationException> {
+  @Override
+  public Response toResponse(WebApplicationException e) {
+    return status(e.getResponse().getStatus()).entity(new ErrorResponse(e.getMessage())).build();
+  }
+}

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

@@ -17,12 +17,13 @@
 import javax.ws.rs.core.Response;
 
 import org.apache.cxf.validation.ResponseConstraintViolationException;
-
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.junit.jupiter.MockitoExtension;
 
+import io.nflow.rest.v1.msg.ErrorResponse;
+
 @ExtendWith(MockitoExtension.class)
 public class CustomValidationExceptionMapperTest {
 
@@ -44,37 +45,34 @@ public void constraintViolationExceptionCausesBadRequest() {
     when(violation.getPropertyPath()).thenReturn(violationPath);
     when(violation.getMessage()).thenReturn("violationMessage");
 
-    ConstraintViolationException cex = mock(ConstraintViolationException.class);
-    when(cex.getConstraintViolations()).thenReturn(new LinkedHashSet(asList(violation)));
-    try (Response resp = exceptionMapper.toResponse(cex)) {
-      assertThat(resp.getStatus(), is(BAD_REQUEST_400));
-      assertThat(resp.getEntity().toString(), is("violationPath: violationMessage"));
+    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));
+      ErrorResponse error = (ErrorResponse) response.getEntity();
+      assertThat(error.error, is("violationPath: violationMessage"));
     }
   }
 
-  @SuppressWarnings({ "unchecked", "rawtypes" })
   @Test
   public void responseConstraintViolationExceptionCausesInternalServerError() {
-    Path violationPath = mock(Path.class);
-    when(violationPath.toString()).thenReturn("violationPath");
-
-    ConstraintViolation violation = mock(ConstraintViolation.class);
-    when(violation.getRootBeanClass()).thenReturn(CustomValidationExceptionMapperTest.class);
-    when(violation.getPropertyPath()).thenReturn(violationPath);
-    when(violation.getMessage()).thenReturn("violationMessage");
-
-    ConstraintViolationException cex = mock(ResponseConstraintViolationException.class);
-    when(cex.getConstraintViolations()).thenReturn(new LinkedHashSet(asList(violation)));
-    try (Response resp = exceptionMapper.toResponse(cex)) {
-      assertThat(resp.getStatus(), is(INTERNAL_SERVER_ERROR_500));
+    ConstraintViolationException exception = mock(ResponseConstraintViolationException.class);
+    when(exception.getMessage()).thenReturn("error");
+    try (Response response = exceptionMapper.toResponse(exception)) {
+      assertThat(response.getStatus(), is(INTERNAL_SERVER_ERROR_500));
+      ErrorResponse error = (ErrorResponse) response.getEntity();
+      assertThat(error.error, is("error"));
     }
   }
 
   @Test
   public void otherExceptionsCauseInternalServerException() {
-    ValidationException cex = mock(ValidationException.class);
-    try (Response resp = exceptionMapper.toResponse(cex)) {
-      assertThat(resp.getStatus(), is(INTERNAL_SERVER_ERROR_500));
+    ValidationException exception = mock(ValidationException.class);
+    when(exception.getMessage()).thenReturn("error");
+    try (Response response = exceptionMapper.toResponse(exception)) {
+      assertThat(response.getStatus(), is(INTERNAL_SERVER_ERROR_500));
+      ErrorResponse error = (ErrorResponse) response.getEntity();
+      assertThat(error.error, is("error"));
     }
   }
 }

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

@@ -0,0 +1,25 @@
+package io.nflow.jetty.mapper;
+
+import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import javax.ws.rs.core.Response;
+
+import org.junit.jupiter.api.Test;
+
+import io.nflow.engine.workflow.executor.StateVariableValueTooLongException;
+import io.nflow.rest.v1.msg.ErrorResponse;
+
+public class StateVariableValueTooLongExceptionMapperTest {
+  StateVariableValueTooLongExceptionMapper mapper = new StateVariableValueTooLongExceptionMapper();
+
+  @Test
+  public void exceptionIsMappedToBadRequest() {
+    try (Response response = mapper.toResponse(new StateVariableValueTooLongException("error"))) {
+      assertThat(response.getStatus(), is(BAD_REQUEST.getStatusCode()));
+      ErrorResponse error = (ErrorResponse) response.getEntity();
+      assertThat(error.error, is("error"));
+    }
+  }
+}

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

@@ -0,0 +1,25 @@
+package io.nflow.jetty.mapper;
+
+import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import javax.ws.rs.WebApplicationException;
+import javax.ws.rs.core.Response;
+
+import org.junit.jupiter.api.Test;
+
+import io.nflow.rest.v1.msg.ErrorResponse;
+
+public class WebApplicationExceptionMapperTest {
+  WebApplicationExceptionMapper mapper = new WebApplicationExceptionMapper();
+
+  @Test
+  public void exceptionStatusAndMessageAreUsedInResponse() {
+    try (Response response = mapper.toResponse(new WebApplicationException("error", BAD_REQUEST))) {
+      assertThat(response.getStatus(), is(BAD_REQUEST.getStatusCode()));
+      ErrorResponse error = (ErrorResponse) response.getEntity();
+      assertThat(error.error, is("error"));
+    }
+  }
+}

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

@@ -27,6 +27,7 @@
 import org.springframework.dao.EmptyResultDataAccessException;
 
 import io.nflow.engine.internal.dao.WorkflowDefinitionDao;
+import io.nflow.engine.internal.dao.WorkflowInstanceDao;
 import io.nflow.engine.internal.workflow.StoredWorkflowDefinition;
 import io.nflow.engine.service.WorkflowDefinitionService;
 import io.nflow.engine.service.WorkflowInstanceInclude;
@@ -93,7 +94,7 @@ public List<ListWorkflowDefinitionResponse> listWorkflowDefinitions(final List<S
 
   public boolean updateWorkflowInstance(final long id,
       final UpdateWorkflowInstanceRequest req, final WorkflowInstanceFactory workflowInstanceFactory,
-      final WorkflowInstanceService workflowInstances) {
+      final WorkflowInstanceService workflowInstances, WorkflowInstanceDao workflowInstanceDao) {
     WorkflowInstance.Builder builder = workflowInstanceFactory.newWorkflowInstanceBuilder().setId(id)
         .setNextActivation(req.nextActivationTime);
     String msg = defaultIfBlank(req.actionDescription, "");
@@ -123,6 +124,7 @@ public boolean updateWorkflowInstance(final long id,
       return true;
     }
     WorkflowInstance instance = builder.setStateText(msg).build();
+    instance.getChangedStateVariables().forEach(workflowInstanceDao::checkStateVariableValueLength);
     WorkflowInstanceAction action = new WorkflowInstanceAction.Builder(instance).setType(externalChange)
         .setStateText(trimToNull(msg)).setExecutionEnd(now()).build();
     return workflowInstances.updateWorkflowInstance(instance, action);

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

@@ -0,0 +1,17 @@
+package io.nflow.rest.v1.msg;
+
+import io.nflow.engine.model.ModelObject;
+
+public class ErrorResponse extends ModelObject {
+
+  public String error;
+
+  public ErrorResponse() {
+    // empty constructor is required by jersey object mapping
+  }
+
+  public ErrorResponse(String error) {
+    this.error = error;
+  }
+
+}