From 4ae46f539419ab95af623892d0e1d1f3f340df63 Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Sun, 5 Apr 2020 12:53:56 +0300 Subject: [PATCH 01/16] throw illegalargumentex instead of illegalstateex when updating to invalid state, wip --- .../internal/executor/WorkflowDispatcher.java | 2 +- .../executor/WorkflowStateProcessor.java | 14 ++++++------ .../workflow/WorkflowDefinitionScanner.java | 2 +- .../AbstractWorkflowDefinition.java | 22 ++++++++++--------- .../IllegalArgumentExceptionMapper.java | 18 +++++++++++++++ 5 files changed, 39 insertions(+), 19 deletions(-) create mode 100644 nflow-jetty/src/main/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapper.java diff --git a/nflow-engine/src/main/java/io/nflow/engine/internal/executor/WorkflowDispatcher.java b/nflow-engine/src/main/java/io/nflow/engine/internal/executor/WorkflowDispatcher.java index ebc27ff0f..28de9d6d6 100644 --- a/nflow-engine/src/main/java/io/nflow/engine/internal/executor/WorkflowDispatcher.java +++ b/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); } } diff --git a/nflow-engine/src/main/java/io/nflow/engine/internal/executor/WorkflowStateProcessor.java b/nflow-engine/src/main/java/io/nflow/engine/internal/executor/WorkflowStateProcessor.java index a2ca1e113..90a224df7 100644 --- a/nflow-engine/src/main/java/io/nflow/engine/internal/executor/WorkflowStateProcessor.java +++ b/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); } } } diff --git a/nflow-engine/src/main/java/io/nflow/engine/internal/workflow/WorkflowDefinitionScanner.java b/nflow-engine/src/main/java/io/nflow/engine/internal/workflow/WorkflowDefinitionScanner.java index a9c56d616..45aa023e8 100644 --- a/nflow-engine/src/main/java/io/nflow/engine/internal/workflow/WorkflowDefinitionScanner.java +++ b/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; diff --git a/nflow-engine/src/main/java/io/nflow/engine/workflow/definition/AbstractWorkflowDefinition.java b/nflow-engine/src/main/java/io/nflow/engine/workflow/definition/AbstractWorkflowDefinition.java index 899e7feb7..7613aff25 100644 --- a/nflow-engine/src/main/java/io/nflow/engine/workflow/definition/AbstractWorkflowDefinition.java +++ b/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; diff --git a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapper.java b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapper.java new file mode 100644 index 000000000..45c23c113 --- /dev/null +++ b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapper.java @@ -0,0 +1,18 @@ +package io.nflow.jetty.mapper; + +import static javax.ws.rs.core.Response.status; +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.rest.v1.msg.ErrorResponse; + +@Provider +public class IllegalArgumentExceptionMapper implements ExceptionMapper { + @Override + public Response toResponse(IllegalArgumentException e) { + return status(BAD_REQUEST.getStatusCode()).entity(new ErrorResponse(e.getMessage())).build(); + } +} From 9f46af9c2ab2c70f676c33be9791b4e9399c6672 Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Mon, 6 Apr 2020 23:01:28 +0300 Subject: [PATCH 02/16] move mappers, add test, cleanup jetty config, remove useless test --- .../jetty/config/NflowJettyConfiguration.java | 10 ++------ .../io/nflow/jetty/mapper/package-info.java | 4 ---- .../config/NflowJettyConfigurationTest.java | 22 ----------------- .../CustomValidationExceptionMapper.java | 2 +- .../IllegalArgumentExceptionMapper.java | 2 +- ...teVariableValueTooLongExceptionMapper.java | 2 +- .../mapper/WebApplicationExceptionMapper.java | 2 +- .../CustomValidationExceptionMapperTest.java | 10 ++++---- .../IllegalArgumentExceptionMapperTest.java | 24 +++++++++++++++++++ ...riableValueTooLongExceptionMapperTest.java | 3 ++- .../WebApplicationExceptionMapperTest.java | 3 ++- .../io/nflow/rest/mapper/package-info.java | 4 ++++ 12 files changed, 42 insertions(+), 46 deletions(-) delete mode 100644 nflow-jetty/src/main/java/io/nflow/jetty/mapper/package-info.java delete mode 100644 nflow-jetty/src/test/java/io/nflow/jetty/config/NflowJettyConfigurationTest.java rename {nflow-jetty/src/main/java/io/nflow/jetty => nflow-rest-api-jax-rs/src/main/java/io/nflow/rest}/mapper/CustomValidationExceptionMapper.java (97%) rename {nflow-jetty/src/main/java/io/nflow/jetty => nflow-rest-api-jax-rs/src/main/java/io/nflow/rest}/mapper/IllegalArgumentExceptionMapper.java (94%) rename {nflow-jetty/src/main/java/io/nflow/jetty => nflow-rest-api-jax-rs/src/main/java/io/nflow/rest}/mapper/StateVariableValueTooLongExceptionMapper.java (95%) rename {nflow-jetty/src/main/java/io/nflow/jetty => nflow-rest-api-jax-rs/src/main/java/io/nflow/rest}/mapper/WebApplicationExceptionMapper.java (94%) rename {nflow-jetty/src/test/java/io/nflow/jetty => nflow-rest-api-jax-rs/src/test/java/io/nflow/rest}/mapper/CustomValidationExceptionMapperTest.java (87%) create mode 100644 nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/IllegalArgumentExceptionMapperTest.java rename {nflow-jetty/src/test/java/io/nflow/jetty => nflow-rest-api-jax-rs/src/test/java/io/nflow/rest}/mapper/StateVariableValueTooLongExceptionMapperTest.java (89%) rename {nflow-jetty/src/test/java/io/nflow/jetty => nflow-rest-api-jax-rs/src/test/java/io/nflow/rest}/mapper/WebApplicationExceptionMapperTest.java (90%) create mode 100644 nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/package-info.java diff --git a/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java b/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java index 0eeb07c23..df2634b15 100644 --- a/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java +++ b/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java @@ -35,11 +35,11 @@ import com.fasterxml.jackson.jaxrs.json.JacksonJsonProvider; 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; +import io.nflow.rest.mapper.CustomValidationExceptionMapper; +import io.nflow.rest.mapper.StateVariableValueTooLongExceptionMapper; import io.nflow.rest.v1.jaxrs.MaintenanceResource; import io.nflow.rest.v1.jaxrs.StatisticsResource; import io.nflow.rest.v1.jaxrs.WorkflowDefinitionResource; @@ -74,7 +74,6 @@ public Server jaxRsServer(WorkflowInstanceResource workflowInstanceResource, } factory.setProviders(asList( jsonProvider(nflowRestObjectMapper), - validationExceptionMapper(), corsHeadersProvider(), new WebApplicationExceptionMapper(), new CustomValidationExceptionMapper(), @@ -113,11 +112,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(); diff --git a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/package-info.java b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/package-info.java deleted file mode 100644 index b00af635d..000000000 --- a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/package-info.java +++ /dev/null @@ -1,4 +0,0 @@ -/** - * JAX-RS exception mappers for nFlow REST API in Jetty. - */ -package io.nflow.jetty.mapper; diff --git a/nflow-jetty/src/test/java/io/nflow/jetty/config/NflowJettyConfigurationTest.java b/nflow-jetty/src/test/java/io/nflow/jetty/config/NflowJettyConfigurationTest.java deleted file mode 100644 index 7f8f114c5..000000000 --- a/nflow-jetty/src/test/java/io/nflow/jetty/config/NflowJettyConfigurationTest.java +++ /dev/null @@ -1,22 +0,0 @@ -package io.nflow.jetty.config; - -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.notNullValue; - -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.Mock; -import org.mockito.junit.jupiter.MockitoExtension; -import org.springframework.core.env.Environment; - -@ExtendWith(MockitoExtension.class) -public class NflowJettyConfigurationTest { - - @Mock - private Environment env; - - @Test - public void createsValidationExceptionMapper() { - assertThat(new NflowJettyConfiguration().validationExceptionMapper(), notNullValue()); - } -} diff --git a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/CustomValidationExceptionMapper.java b/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/CustomValidationExceptionMapper.java similarity index 97% rename from nflow-jetty/src/main/java/io/nflow/jetty/mapper/CustomValidationExceptionMapper.java rename to nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/CustomValidationExceptionMapper.java index 7234c812e..60916d985 100644 --- a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/CustomValidationExceptionMapper.java +++ b/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/CustomValidationExceptionMapper.java @@ -1,4 +1,4 @@ -package io.nflow.jetty.mapper; +package io.nflow.rest.mapper; import static java.util.stream.Collectors.joining; diff --git a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapper.java b/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/IllegalArgumentExceptionMapper.java similarity index 94% rename from nflow-jetty/src/main/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapper.java rename to nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/IllegalArgumentExceptionMapper.java index 45c23c113..4e7500c5a 100644 --- a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapper.java +++ b/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/IllegalArgumentExceptionMapper.java @@ -1,4 +1,4 @@ -package io.nflow.jetty.mapper; +package io.nflow.rest.mapper; import static javax.ws.rs.core.Response.status; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; diff --git a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapper.java b/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/StateVariableValueTooLongExceptionMapper.java similarity index 95% rename from nflow-jetty/src/main/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapper.java rename to nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/StateVariableValueTooLongExceptionMapper.java index e9189e871..8e9c93013 100644 --- a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapper.java +++ b/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/StateVariableValueTooLongExceptionMapper.java @@ -1,4 +1,4 @@ -package io.nflow.jetty.mapper; +package io.nflow.rest.mapper; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; diff --git a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/WebApplicationExceptionMapper.java b/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/WebApplicationExceptionMapper.java similarity index 94% rename from nflow-jetty/src/main/java/io/nflow/jetty/mapper/WebApplicationExceptionMapper.java rename to nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/WebApplicationExceptionMapper.java index 1469ad53d..cc21abd1f 100644 --- a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/WebApplicationExceptionMapper.java +++ b/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/WebApplicationExceptionMapper.java @@ -1,4 +1,4 @@ -package io.nflow.jetty.mapper; +package io.nflow.rest.mapper; import static javax.ws.rs.core.Response.status; diff --git a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/CustomValidationExceptionMapperTest.java b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/CustomValidationExceptionMapperTest.java similarity index 87% rename from nflow-jetty/src/test/java/io/nflow/jetty/mapper/CustomValidationExceptionMapperTest.java rename to nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/CustomValidationExceptionMapperTest.java index 7cfdbb97e..aff0b9db7 100644 --- a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/CustomValidationExceptionMapperTest.java +++ b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/CustomValidationExceptionMapperTest.java @@ -1,8 +1,6 @@ -package io.nflow.jetty.mapper; +package io.nflow.rest.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")); } diff --git a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/IllegalArgumentExceptionMapperTest.java b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/IllegalArgumentExceptionMapperTest.java new file mode 100644 index 000000000..d6866fd9f --- /dev/null +++ b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/IllegalArgumentExceptionMapperTest.java @@ -0,0 +1,24 @@ +package io.nflow.rest.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.rest.v1.msg.ErrorResponse; + +public class IllegalArgumentExceptionMapperTest { + IllegalArgumentExceptionMapper mapper = new IllegalArgumentExceptionMapper(); + + @Test + public void exceptionIsMappedToBadRequest() { + try (Response response = mapper.toResponse(new IllegalArgumentException("error"))) { + assertThat(response.getStatus(), is(BAD_REQUEST.getStatusCode())); + ErrorResponse error = (ErrorResponse) response.getEntity(); + assertThat(error.error, is("error")); + } + } +} diff --git a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapperTest.java b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/StateVariableValueTooLongExceptionMapperTest.java similarity index 89% rename from nflow-jetty/src/test/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapperTest.java rename to nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/StateVariableValueTooLongExceptionMapperTest.java index edc22a7cf..27db62313 100644 --- a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapperTest.java +++ b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/StateVariableValueTooLongExceptionMapperTest.java @@ -1,4 +1,4 @@ -package io.nflow.jetty.mapper; +package io.nflow.rest.mapper; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; import static org.hamcrest.CoreMatchers.is; @@ -9,6 +9,7 @@ import org.junit.jupiter.api.Test; import io.nflow.engine.workflow.executor.StateVariableValueTooLongException; +import io.nflow.rest.mapper.StateVariableValueTooLongExceptionMapper; import io.nflow.rest.v1.msg.ErrorResponse; public class StateVariableValueTooLongExceptionMapperTest { diff --git a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/WebApplicationExceptionMapperTest.java b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/WebApplicationExceptionMapperTest.java similarity index 90% rename from nflow-jetty/src/test/java/io/nflow/jetty/mapper/WebApplicationExceptionMapperTest.java rename to nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/WebApplicationExceptionMapperTest.java index e1f1cea8e..ba0561381 100644 --- a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/WebApplicationExceptionMapperTest.java +++ b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/WebApplicationExceptionMapperTest.java @@ -1,4 +1,4 @@ -package io.nflow.jetty.mapper; +package io.nflow.rest.mapper; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; import static org.hamcrest.CoreMatchers.is; @@ -9,6 +9,7 @@ import org.junit.jupiter.api.Test; +import io.nflow.rest.mapper.WebApplicationExceptionMapper; import io.nflow.rest.v1.msg.ErrorResponse; public class WebApplicationExceptionMapperTest { diff --git a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/package-info.java b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/package-info.java new file mode 100644 index 000000000..a9e9fbbfd --- /dev/null +++ b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/package-info.java @@ -0,0 +1,4 @@ +/** + * JAX-RS exception mappers. + */ +package io.nflow.rest.mapper; From e7a5aef885ac35603e1d55d0439bca0e24b5d93b Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Mon, 6 Apr 2020 23:19:41 +0300 Subject: [PATCH 03/16] register illegal argument exception mapper and add test --- .../jetty/config/NflowJettyConfiguration.java | 2 ++ .../tests/CreditApplicationWorkflowTest.java | 16 +++++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java b/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java index df2634b15..401eab205 100644 --- a/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java +++ b/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java @@ -39,6 +39,7 @@ import io.nflow.rest.config.jaxrs.CorsHeaderContainerResponseFilter; import io.nflow.rest.config.jaxrs.DateTimeParamConverterProvider; import io.nflow.rest.mapper.CustomValidationExceptionMapper; +import io.nflow.rest.mapper.IllegalArgumentExceptionMapper; import io.nflow.rest.mapper.StateVariableValueTooLongExceptionMapper; import io.nflow.rest.v1.jaxrs.MaintenanceResource; import io.nflow.rest.v1.jaxrs.StatisticsResource; @@ -78,6 +79,7 @@ public Server jaxRsServer(WorkflowInstanceResource workflowInstanceResource, new WebApplicationExceptionMapper(), new CustomValidationExceptionMapper(), new StateVariableValueTooLongExceptionMapper(), + new IllegalArgumentExceptionMapper(), new DateTimeParamConverterProvider() )); factory.setFeatures(asList(new LoggingFeature(), swaggerFeature())); diff --git a/nflow-tests/src/test/java/io/nflow/tests/CreditApplicationWorkflowTest.java b/nflow-tests/src/test/java/io/nflow/tests/CreditApplicationWorkflowTest.java index 9d90eec1f..a14c924ed 100644 --- a/nflow-tests/src/test/java/io/nflow/tests/CreditApplicationWorkflowTest.java +++ b/nflow-tests/src/test/java/io/nflow/tests/CreditApplicationWorkflowTest.java @@ -4,6 +4,7 @@ import static java.time.Duration.ofSeconds; import static java.util.Arrays.asList; import static org.apache.cxf.jaxrs.client.WebClient.fromClient; +import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.notNullValue; import static org.joda.time.DateTime.now; @@ -11,6 +12,8 @@ import java.math.BigDecimal; import java.util.UUID; +import javax.ws.rs.core.Response; + import org.junit.jupiter.api.MethodOrderer; import org.junit.jupiter.api.Order; import org.junit.jupiter.api.Test; @@ -67,12 +70,23 @@ public void moveToGrantLoanState() { @Test @Order(4) + public void moveToInvalidStateFails() { + UpdateWorkflowInstanceRequest ureq = new UpdateWorkflowInstanceRequest(); + ureq.nextActivationTime = now(); + ureq.state = "invalid"; + try (Response response = fromClient(workflowInstanceIdResource, true).path(resp.id).put(ureq)) { + assertThat(response.getStatus(), is(Response.Status.BAD_REQUEST.getStatusCode())); + } + } + + @Test + @Order(5) public void checkErrorStateReached() { getWorkflowInstanceWithTimeout(resp.id, "error", ofSeconds(5)); } @Test - @Order(5) + @Order(6) public void checkWorkflowInstanceActions() { int i = 1; assertWorkflowInstance(resp.id, actionHistoryValidator(asList( From eca0df09e922513b932a72e029be00d86d400376 Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Mon, 6 Apr 2020 23:29:59 +0300 Subject: [PATCH 04/16] update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d50e43a95..a2463e033 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ - `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. - Dependency updates: - spring 5.2.5 - jackson 2.10.3 @@ -27,6 +28,9 @@ - spotbugs 4.0.2 - hibernate 6.1.4 - commons-lang3 3.10 +- `nflow-rest-api-jax-rs` + - Move exception mappers from `nflow-jetty` / `io.nflow.jetty.mapper` to `nflow-rest-api-jax-rs` / `io.nflow.rest.mapper` + - Map `IllegalArgumentException`s to HTTP 400 Bad Request - `nflow-explorer` - Dependency updates: - swagger-ui 2.2.10 From 753ed35319b4dc3c45ee266b79816daf6088b773 Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Tue, 14 Apr 2020 11:44:44 +0300 Subject: [PATCH 05/16] move exception mappers back to nflow-jetty, convert IAE to bad request response in resources --- CHANGELOG.md | 5 +++-- .../io/nflow/jetty/config/NflowJettyConfiguration.java | 6 +++--- .../jetty}/mapper/CustomValidationExceptionMapper.java | 3 ++- .../jetty}/mapper/IllegalArgumentExceptionMapper.java | 2 +- .../mapper/StateVariableValueTooLongExceptionMapper.java | 2 +- .../jetty}/mapper/WebApplicationExceptionMapper.java | 2 +- .../mapper/CustomValidationExceptionMapperTest.java | 2 +- .../mapper/IllegalArgumentExceptionMapperTest.java | 3 ++- .../StateVariableValueTooLongExceptionMapperTest.java | 4 ++-- .../jetty}/mapper/WebApplicationExceptionMapperTest.java | 4 ++-- .../io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java | 9 +++++++-- .../rest/v1/springweb/WorkflowInstanceResource.java | 9 +++++++-- 12 files changed, 32 insertions(+), 19 deletions(-) rename {nflow-rest-api-jax-rs/src/main/java/io/nflow/rest => nflow-jetty/src/main/java/io/nflow/jetty}/mapper/CustomValidationExceptionMapper.java (97%) rename {nflow-rest-api-jax-rs/src/main/java/io/nflow/rest => nflow-jetty/src/main/java/io/nflow/jetty}/mapper/IllegalArgumentExceptionMapper.java (94%) rename {nflow-rest-api-jax-rs/src/main/java/io/nflow/rest => nflow-jetty/src/main/java/io/nflow/jetty}/mapper/StateVariableValueTooLongExceptionMapper.java (95%) rename {nflow-rest-api-jax-rs/src/main/java/io/nflow/rest => nflow-jetty/src/main/java/io/nflow/jetty}/mapper/WebApplicationExceptionMapper.java (94%) rename {nflow-rest-api-jax-rs/src/test/java/io/nflow/rest => nflow-jetty/src/test/java/io/nflow/jetty}/mapper/CustomValidationExceptionMapperTest.java (98%) rename {nflow-rest-api-jax-rs/src/test/java/io/nflow/rest => nflow-jetty/src/test/java/io/nflow/jetty}/mapper/IllegalArgumentExceptionMapperTest.java (89%) rename {nflow-rest-api-jax-rs/src/test/java/io/nflow/rest => nflow-jetty/src/test/java/io/nflow/jetty}/mapper/StateVariableValueTooLongExceptionMapperTest.java (89%) rename {nflow-rest-api-jax-rs/src/test/java/io/nflow/rest => nflow-jetty/src/test/java/io/nflow/jetty}/mapper/WebApplicationExceptionMapperTest.java (89%) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2463e033..8860a6a00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,8 +29,9 @@ - hibernate 6.1.4 - commons-lang3 3.10 - `nflow-rest-api-jax-rs` - - Move exception mappers from `nflow-jetty` / `io.nflow.jetty.mapper` to `nflow-rest-api-jax-rs` / `io.nflow.rest.mapper` - - Map `IllegalArgumentException`s to HTTP 400 Bad Request + - Convert IllegalArgumentException to HTTP Bad Request in `WorkflowInstanceResource.updateWorkflowInstance` +- `nflow-rest-api-spring-web` + - Convert IllegalArgumentException to HTTP Bad Request in `WorkflowInstanceResource.updateWorkflowInstance` - `nflow-explorer` - Dependency updates: - swagger-ui 2.2.10 diff --git a/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java b/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java index 401eab205..ce0861a24 100644 --- a/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java +++ b/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java @@ -35,12 +35,12 @@ import com.fasterxml.jackson.jaxrs.json.JacksonJsonProvider; import io.nflow.engine.config.NFlow; +import io.nflow.jetty.mapper.CustomValidationExceptionMapper; +import io.nflow.jetty.mapper.IllegalArgumentExceptionMapper; +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; -import io.nflow.rest.mapper.CustomValidationExceptionMapper; -import io.nflow.rest.mapper.IllegalArgumentExceptionMapper; -import io.nflow.rest.mapper.StateVariableValueTooLongExceptionMapper; import io.nflow.rest.v1.jaxrs.MaintenanceResource; import io.nflow.rest.v1.jaxrs.StatisticsResource; import io.nflow.rest.v1.jaxrs.WorkflowDefinitionResource; diff --git a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/CustomValidationExceptionMapper.java b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/CustomValidationExceptionMapper.java similarity index 97% rename from nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/CustomValidationExceptionMapper.java rename to nflow-jetty/src/main/java/io/nflow/jetty/mapper/CustomValidationExceptionMapper.java index 60916d985..6708aec9a 100644 --- a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/CustomValidationExceptionMapper.java +++ b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/CustomValidationExceptionMapper.java @@ -1,4 +1,4 @@ -package io.nflow.rest.mapper; +package io.nflow.jetty.mapper; import static java.util.stream.Collectors.joining; @@ -14,6 +14,7 @@ import io.nflow.rest.v1.msg.ErrorResponse; + @Provider public class CustomValidationExceptionMapper implements ExceptionMapper { diff --git a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/IllegalArgumentExceptionMapper.java b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapper.java similarity index 94% rename from nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/IllegalArgumentExceptionMapper.java rename to nflow-jetty/src/main/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapper.java index 4e7500c5a..45c23c113 100644 --- a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/IllegalArgumentExceptionMapper.java +++ b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapper.java @@ -1,4 +1,4 @@ -package io.nflow.rest.mapper; +package io.nflow.jetty.mapper; import static javax.ws.rs.core.Response.status; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; diff --git a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/StateVariableValueTooLongExceptionMapper.java b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapper.java similarity index 95% rename from nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/StateVariableValueTooLongExceptionMapper.java rename to nflow-jetty/src/main/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapper.java index 8e9c93013..e9189e871 100644 --- a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/StateVariableValueTooLongExceptionMapper.java +++ b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapper.java @@ -1,4 +1,4 @@ -package io.nflow.rest.mapper; +package io.nflow.jetty.mapper; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; diff --git a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/WebApplicationExceptionMapper.java b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/WebApplicationExceptionMapper.java similarity index 94% rename from nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/WebApplicationExceptionMapper.java rename to nflow-jetty/src/main/java/io/nflow/jetty/mapper/WebApplicationExceptionMapper.java index cc21abd1f..1469ad53d 100644 --- a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/mapper/WebApplicationExceptionMapper.java +++ b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/WebApplicationExceptionMapper.java @@ -1,4 +1,4 @@ -package io.nflow.rest.mapper; +package io.nflow.jetty.mapper; import static javax.ws.rs.core.Response.status; diff --git a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/CustomValidationExceptionMapperTest.java b/nflow-jetty/src/test/java/io/nflow/jetty/mapper/CustomValidationExceptionMapperTest.java similarity index 98% rename from nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/CustomValidationExceptionMapperTest.java rename to nflow-jetty/src/test/java/io/nflow/jetty/mapper/CustomValidationExceptionMapperTest.java index aff0b9db7..165d886ab 100644 --- a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/CustomValidationExceptionMapperTest.java +++ b/nflow-jetty/src/test/java/io/nflow/jetty/mapper/CustomValidationExceptionMapperTest.java @@ -1,4 +1,4 @@ -package io.nflow.rest.mapper; +package io.nflow.jetty.mapper; import static java.util.Arrays.asList; import static org.hamcrest.MatcherAssert.assertThat; diff --git a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/IllegalArgumentExceptionMapperTest.java b/nflow-jetty/src/test/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapperTest.java similarity index 89% rename from nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/IllegalArgumentExceptionMapperTest.java rename to nflow-jetty/src/test/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapperTest.java index d6866fd9f..708535db6 100644 --- a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/IllegalArgumentExceptionMapperTest.java +++ b/nflow-jetty/src/test/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapperTest.java @@ -1,4 +1,4 @@ -package io.nflow.rest.mapper; +package io.nflow.jetty.mapper; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; import static org.hamcrest.CoreMatchers.is; @@ -8,6 +8,7 @@ import org.junit.jupiter.api.Test; +import io.nflow.jetty.mapper.IllegalArgumentExceptionMapper; import io.nflow.rest.v1.msg.ErrorResponse; public class IllegalArgumentExceptionMapperTest { diff --git a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/StateVariableValueTooLongExceptionMapperTest.java b/nflow-jetty/src/test/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapperTest.java similarity index 89% rename from nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/StateVariableValueTooLongExceptionMapperTest.java rename to nflow-jetty/src/test/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapperTest.java index 27db62313..eaf428d88 100644 --- a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/StateVariableValueTooLongExceptionMapperTest.java +++ b/nflow-jetty/src/test/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapperTest.java @@ -1,4 +1,4 @@ -package io.nflow.rest.mapper; +package io.nflow.jetty.mapper; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; import static org.hamcrest.CoreMatchers.is; @@ -9,7 +9,7 @@ import org.junit.jupiter.api.Test; import io.nflow.engine.workflow.executor.StateVariableValueTooLongException; -import io.nflow.rest.mapper.StateVariableValueTooLongExceptionMapper; +import io.nflow.jetty.mapper.StateVariableValueTooLongExceptionMapper; import io.nflow.rest.v1.msg.ErrorResponse; public class StateVariableValueTooLongExceptionMapperTest { diff --git a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/WebApplicationExceptionMapperTest.java b/nflow-jetty/src/test/java/io/nflow/jetty/mapper/WebApplicationExceptionMapperTest.java similarity index 89% rename from nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/WebApplicationExceptionMapperTest.java rename to nflow-jetty/src/test/java/io/nflow/jetty/mapper/WebApplicationExceptionMapperTest.java index ba0561381..062c8de50 100644 --- a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/WebApplicationExceptionMapperTest.java +++ b/nflow-jetty/src/test/java/io/nflow/jetty/mapper/WebApplicationExceptionMapperTest.java @@ -1,4 +1,4 @@ -package io.nflow.rest.mapper; +package io.nflow.jetty.mapper; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; import static org.hamcrest.CoreMatchers.is; @@ -9,7 +9,7 @@ import org.junit.jupiter.api.Test; -import io.nflow.rest.mapper.WebApplicationExceptionMapper; +import io.nflow.jetty.mapper.WebApplicationExceptionMapper; import io.nflow.rest.v1.msg.ErrorResponse; public class WebApplicationExceptionMapperTest { diff --git a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java b/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java index ac6df10b8..22f3d8e1a 100644 --- a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java +++ b/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java @@ -9,6 +9,7 @@ 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 java.net.URI; @@ -110,8 +111,12 @@ 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.getStatusCode(), e.getMessage()).build(); + } } @GET diff --git a/nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java b/nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java index fe6c65f4a..96a207d45 100644 --- a/nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java +++ b/nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java @@ -3,6 +3,7 @@ import static io.nflow.rest.config.springweb.PathConstants.NFLOW_SPRING_WEB_PATH_PREFIX; import static io.nflow.rest.v1.ResourcePaths.NFLOW_WORKFLOW_INSTANCE_PATH; import static java.util.Optional.ofNullable; +import static org.springframework.http.HttpStatus.BAD_REQUEST; import static org.springframework.http.HttpStatus.CONFLICT; import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE; import static org.springframework.http.ResponseEntity.created; @@ -97,8 +98,12 @@ public ResponseEntity createWorkflowInstance( @ApiResponse(code = 409, message = "If workflow was executing and no update was done") }) public ResponseEntity updateWorkflowInstance(@ApiParam("Internal id for workflow instance") @PathVariable("id") long id, @RequestBody @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).body(e.getMessage()); + } } @GetMapping(path = "/id/{id}") From 3d205c625abab42c0fa02c3fb21c546a206c0818 Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Tue, 14 Apr 2020 11:45:37 +0300 Subject: [PATCH 06/16] remove exception mapper for IAE --- .../jetty/config/NflowJettyConfiguration.java | 2 -- .../mapper/IllegalArgumentExceptionMapper.java | 18 ------------------ 2 files changed, 20 deletions(-) delete mode 100644 nflow-jetty/src/main/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapper.java diff --git a/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java b/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java index ce0861a24..9c824ade9 100644 --- a/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java +++ b/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.IllegalArgumentExceptionMapper; import io.nflow.jetty.mapper.StateVariableValueTooLongExceptionMapper; import io.nflow.rest.config.RestConfiguration; import io.nflow.rest.config.jaxrs.CorsHeaderContainerResponseFilter; @@ -79,7 +78,6 @@ public Server jaxRsServer(WorkflowInstanceResource workflowInstanceResource, new WebApplicationExceptionMapper(), new CustomValidationExceptionMapper(), new StateVariableValueTooLongExceptionMapper(), - new IllegalArgumentExceptionMapper(), new DateTimeParamConverterProvider() )); factory.setFeatures(asList(new LoggingFeature(), swaggerFeature())); diff --git a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapper.java b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapper.java deleted file mode 100644 index 45c23c113..000000000 --- a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapper.java +++ /dev/null @@ -1,18 +0,0 @@ -package io.nflow.jetty.mapper; - -import static javax.ws.rs.core.Response.status; -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.rest.v1.msg.ErrorResponse; - -@Provider -public class IllegalArgumentExceptionMapper implements ExceptionMapper { - @Override - public Response toResponse(IllegalArgumentException e) { - return status(BAD_REQUEST.getStatusCode()).entity(new ErrorResponse(e.getMessage())).build(); - } -} From 884a474dc70c7a11a3703b5e5416d6dcb188958e Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Tue, 14 Apr 2020 11:46:24 +0300 Subject: [PATCH 07/16] revert whitespace change --- .../io/nflow/jetty/mapper/CustomValidationExceptionMapper.java | 1 - 1 file changed, 1 deletion(-) diff --git a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/CustomValidationExceptionMapper.java b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/CustomValidationExceptionMapper.java index 6708aec9a..7234c812e 100644 --- a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/CustomValidationExceptionMapper.java +++ b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/CustomValidationExceptionMapper.java @@ -14,7 +14,6 @@ import io.nflow.rest.v1.msg.ErrorResponse; - @Provider public class CustomValidationExceptionMapper implements ExceptionMapper { From 66cc4329a24b6da9736a898131a54a4f76355ae7 Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Tue, 14 Apr 2020 11:49:19 +0300 Subject: [PATCH 08/16] revert changes --- .../io/nflow/jetty/mapper/package-info.java | 4 +++ .../IllegalArgumentExceptionMapperTest.java | 25 ------------------- 2 files changed, 4 insertions(+), 25 deletions(-) create mode 100644 nflow-jetty/src/main/java/io/nflow/jetty/mapper/package-info.java delete mode 100644 nflow-jetty/src/test/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapperTest.java diff --git a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/package-info.java b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/package-info.java new file mode 100644 index 000000000..b00af635d --- /dev/null +++ b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/package-info.java @@ -0,0 +1,4 @@ +/** + * JAX-RS exception mappers for nFlow REST API in Jetty. + */ +package io.nflow.jetty.mapper; diff --git a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapperTest.java b/nflow-jetty/src/test/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapperTest.java deleted file mode 100644 index 708535db6..000000000 --- a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/IllegalArgumentExceptionMapperTest.java +++ /dev/null @@ -1,25 +0,0 @@ -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.jetty.mapper.IllegalArgumentExceptionMapper; -import io.nflow.rest.v1.msg.ErrorResponse; - -public class IllegalArgumentExceptionMapperTest { - IllegalArgumentExceptionMapper mapper = new IllegalArgumentExceptionMapper(); - - @Test - public void exceptionIsMappedToBadRequest() { - try (Response response = mapper.toResponse(new IllegalArgumentException("error"))) { - assertThat(response.getStatus(), is(BAD_REQUEST.getStatusCode())); - ErrorResponse error = (ErrorResponse) response.getEntity(); - assertThat(error.error, is("error")); - } - } -} From 28231f1ce9022e914b22d2dc1ecd053926a1e7ef Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Tue, 14 Apr 2020 11:51:43 +0300 Subject: [PATCH 09/16] revert changes --- .../mapper/StateVariableValueTooLongExceptionMapperTest.java | 1 - .../nflow/jetty/mapper/WebApplicationExceptionMapperTest.java | 1 - .../src/test/java/io/nflow/rest/mapper/package-info.java | 4 ---- 3 files changed, 6 deletions(-) delete mode 100644 nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/package-info.java diff --git a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapperTest.java b/nflow-jetty/src/test/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapperTest.java index eaf428d88..edc22a7cf 100644 --- a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapperTest.java +++ b/nflow-jetty/src/test/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapperTest.java @@ -9,7 +9,6 @@ import org.junit.jupiter.api.Test; import io.nflow.engine.workflow.executor.StateVariableValueTooLongException; -import io.nflow.jetty.mapper.StateVariableValueTooLongExceptionMapper; import io.nflow.rest.v1.msg.ErrorResponse; public class StateVariableValueTooLongExceptionMapperTest { diff --git a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/WebApplicationExceptionMapperTest.java b/nflow-jetty/src/test/java/io/nflow/jetty/mapper/WebApplicationExceptionMapperTest.java index 062c8de50..e1f1cea8e 100644 --- a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/WebApplicationExceptionMapperTest.java +++ b/nflow-jetty/src/test/java/io/nflow/jetty/mapper/WebApplicationExceptionMapperTest.java @@ -9,7 +9,6 @@ import org.junit.jupiter.api.Test; -import io.nflow.jetty.mapper.WebApplicationExceptionMapper; import io.nflow.rest.v1.msg.ErrorResponse; public class WebApplicationExceptionMapperTest { diff --git a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/package-info.java b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/package-info.java deleted file mode 100644 index a9e9fbbfd..000000000 --- a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/mapper/package-info.java +++ /dev/null @@ -1,4 +0,0 @@ -/** - * JAX-RS exception mappers. - */ -package io.nflow.rest.mapper; From cb66f8b4d414ca504574616bfd8a67331ce9da13 Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Sun, 19 Apr 2020 00:41:41 +0300 Subject: [PATCH 10/16] Remove StateVariableValueTooLongExceptionMapper, convert it to bad request response in resources instead --- .../WorkflowInstancePreProcessor.java | 4 +-- .../StateVariableValueTooLongException.java | 2 +- .../jetty/config/NflowJettyConfiguration.java | 2 -- ...teVariableValueTooLongExceptionMapper.java | 18 ------------- ...riableValueTooLongExceptionMapperTest.java | 25 ------------------- .../v1/jaxrs/WorkflowInstanceResource.java | 6 ++++- .../springweb/WorkflowInstanceResource.java | 12 ++++++--- 7 files changed, 16 insertions(+), 53 deletions(-) delete mode 100644 nflow-jetty/src/main/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapper.java delete mode 100644 nflow-jetty/src/test/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapperTest.java diff --git a/nflow-engine/src/main/java/io/nflow/engine/internal/workflow/WorkflowInstancePreProcessor.java b/nflow-engine/src/main/java/io/nflow/engine/internal/workflow/WorkflowInstancePreProcessor.java index c94c4ff01..6e2e0c5f8 100644 --- a/nflow-engine/src/main/java/io/nflow/engine/internal/workflow/WorkflowInstancePreProcessor.java +++ b/nflow-engine/src/main/java/io/nflow/engine/internal/workflow/WorkflowInstancePreProcessor.java @@ -31,14 +31,14 @@ public WorkflowInstancePreProcessor(WorkflowDefinitionService workflowDefinition 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)) { diff --git a/nflow-engine/src/main/java/io/nflow/engine/workflow/executor/StateVariableValueTooLongException.java b/nflow-engine/src/main/java/io/nflow/engine/workflow/executor/StateVariableValueTooLongException.java index a04182c47..3bbfa667f 100644 --- a/nflow-engine/src/main/java/io/nflow/engine/workflow/executor/StateVariableValueTooLongException.java +++ b/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; diff --git a/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java b/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java index 9c824ade9..12378fe5b 100644 --- a/nflow-jetty/src/main/java/io/nflow/jetty/config/NflowJettyConfiguration.java +++ b/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; @@ -77,7 +76,6 @@ public Server jaxRsServer(WorkflowInstanceResource workflowInstanceResource, corsHeadersProvider(), new WebApplicationExceptionMapper(), new CustomValidationExceptionMapper(), - new StateVariableValueTooLongExceptionMapper(), new DateTimeParamConverterProvider() )); factory.setFeatures(asList(new LoggingFeature(), swaggerFeature())); diff --git a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapper.java b/nflow-jetty/src/main/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapper.java deleted file mode 100644 index e9189e871..000000000 --- a/nflow-jetty/src/main/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapper.java +++ /dev/null @@ -1,18 +0,0 @@ -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 { - @Override - public Response toResponse(StateVariableValueTooLongException e) { - return Response.status(BAD_REQUEST).entity(new ErrorResponse(e.getMessage())).build(); - } -} diff --git a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapperTest.java b/nflow-jetty/src/test/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapperTest.java deleted file mode 100644 index edc22a7cf..000000000 --- a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/StateVariableValueTooLongExceptionMapperTest.java +++ /dev/null @@ -1,25 +0,0 @@ -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")); - } - } -} diff --git a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java b/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java index 22f3d8e1a..5054a2f49 100644 --- a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java +++ b/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java @@ -99,7 +99,11 @@ public Response createWorkflowInstance( 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 { + return created(URI.create(String.valueOf(id))).entity(createWorkflowConverter.convert(instance)).build(); + } catch (IllegalArgumentException e) { + return status(BAD_REQUEST.getStatusCode(), e.getMessage()).build(); + } } @PUT diff --git a/nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java b/nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java index 96a207d45..def0e4e6f 100644 --- a/nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java +++ b/nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java @@ -82,12 +82,16 @@ public WorkflowInstanceResource(WorkflowInstanceService workflowInstances, Creat @ApiOperation(value = "Submit new workflow instance") @ApiResponses({ @ApiResponse(code = 201, message = "Workflow was created", response = CreateWorkflowInstanceResponse.class), @ApiResponse(code = 400, message = "If instance could not be created, for example when state variable value was too long") }) - public ResponseEntity createWorkflowInstance( + public ResponseEntity createWorkflowInstance( @RequestBody @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))).body(createWorkflowConverter.convert(instance)); + try { + long id = workflowInstances.insertWorkflowInstance(instance); + 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()); + } } @PutMapping(path = "/id/{id}", consumes = APPLICATION_JSON_VALUE) From 2645f05e1d111c791f0a749364d4355cf8afb100 Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Sun, 19 Apr 2020 00:51:15 +0300 Subject: [PATCH 11/16] update changelog [ci skip] --- CHANGELOG.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8860a6a00..277071b38 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +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 `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 @@ -29,9 +31,9 @@ - hibernate 6.1.4 - commons-lang3 3.10 - `nflow-rest-api-jax-rs` - - Convert IllegalArgumentException to HTTP Bad Request in `WorkflowInstanceResource.updateWorkflowInstance` + - 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.updateWorkflowInstance` + - Convert `IllegalArgumentException` to HTTP Bad Request in `WorkflowInstanceResource.insertWorkflowInstance` and `WorkflowInstanceResource.updateWorkflowInstance`. - `nflow-explorer` - Dependency updates: - swagger-ui 2.2.10 From c01080be0182ccfc9cdea695dbdf094622f05567 Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Sun, 19 Apr 2020 00:56:22 +0300 Subject: [PATCH 12/16] remove todo [ci skip] --- .../engine/internal/workflow/WorkflowInstancePreProcessor.java | 1 - 1 file changed, 1 deletion(-) diff --git a/nflow-engine/src/main/java/io/nflow/engine/internal/workflow/WorkflowInstancePreProcessor.java b/nflow-engine/src/main/java/io/nflow/engine/internal/workflow/WorkflowInstancePreProcessor.java index 6e2e0c5f8..4584d9346 100644 --- a/nflow-engine/src/main/java/io/nflow/engine/internal/workflow/WorkflowInstancePreProcessor.java +++ b/nflow-engine/src/main/java/io/nflow/engine/internal/workflow/WorkflowInstancePreProcessor.java @@ -27,7 +27,6 @@ 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) { From 92ba2adc46fb3d23ca223252b786a50b08f06cf9 Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Sun, 19 Apr 2020 01:33:09 +0300 Subject: [PATCH 13/16] tune rest api responses and fix tests --- .../v1/jaxrs/WorkflowInstanceResource.java | 21 +++++++++---------- .../springweb/WorkflowInstanceResource.java | 9 ++++---- .../tests/CreditApplicationWorkflowTest.java | 2 ++ .../io/nflow/tests/StateVariablesTest.java | 5 ++--- 4 files changed, 19 insertions(+), 18 deletions(-) diff --git a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java b/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java index 5054a2f49..ac7915e25 100644 --- a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java +++ b/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java @@ -11,6 +11,7 @@ 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; @@ -22,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; @@ -31,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; @@ -87,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 @@ -97,12 +97,12 @@ 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); 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.getStatusCode(), e.getMessage()).build(); + return status(BAD_REQUEST).entity(e.getMessage()).build(); } } @@ -119,7 +119,7 @@ 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.getStatusCode(), e.getMessage()).build(); + return status(BAD_REQUEST).entity(e.getMessage()).build(); } } @@ -127,14 +127,13 @@ public Response updateWorkflowInstance(@ApiParam("Internal id for workflow insta @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(); } } diff --git a/nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java b/nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java index def0e4e6f..1085914d8 100644 --- a/nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java +++ b/nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java @@ -2,13 +2,14 @@ import static io.nflow.rest.config.springweb.PathConstants.NFLOW_SPRING_WEB_PATH_PREFIX; import static io.nflow.rest.v1.ResourcePaths.NFLOW_WORKFLOW_INSTANCE_PATH; +import static java.lang.String.format; import static java.util.Optional.ofNullable; import static org.springframework.http.HttpStatus.BAD_REQUEST; import static org.springframework.http.HttpStatus.CONFLICT; +import static org.springframework.http.HttpStatus.NOT_FOUND; import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE; import static org.springframework.http.ResponseEntity.created; import static org.springframework.http.ResponseEntity.noContent; -import static org.springframework.http.ResponseEntity.notFound; import static org.springframework.http.ResponseEntity.ok; import static org.springframework.http.ResponseEntity.status; @@ -21,7 +22,6 @@ import javax.inject.Inject; import javax.validation.Valid; -import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import org.springframework.dao.EmptyResultDataAccessException; import org.springframework.http.ResponseEntity; import org.springframework.stereotype.Component; @@ -33,6 +33,7 @@ import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RestController; +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; @@ -113,14 +114,14 @@ public ResponseEntity updateWorkflowInstance(@ApiParam("Internal id for workf @GetMapping(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 ResponseEntity fetchWorkflowInstance( + public ResponseEntity fetchWorkflowInstance( @ApiParam("Internal id for workflow instance") @PathVariable("id") long id, @RequestParam(value = "include", required = false) @ApiParam(value = INCLUDE_PARAM_DESC, allowableValues = INCLUDE_PARAM_VALUES, allowMultiple = true) String include, @RequestParam(value = "maxActions", required = false) @ApiParam("Maximum number of actions returned for each workflow instance") Long maxActions) { try { return ok().body(super.fetchWorkflowInstance(id, include, maxActions, this.workflowInstances, this.listWorkflowConverter)); } catch (@SuppressWarnings("unused") EmptyResultDataAccessException e) { - return notFound().build(); + return status(NOT_FOUND).body(format("Workflow instance %s not found", id)); } } diff --git a/nflow-tests/src/test/java/io/nflow/tests/CreditApplicationWorkflowTest.java b/nflow-tests/src/test/java/io/nflow/tests/CreditApplicationWorkflowTest.java index a14c924ed..8cc860312 100644 --- a/nflow-tests/src/test/java/io/nflow/tests/CreditApplicationWorkflowTest.java +++ b/nflow-tests/src/test/java/io/nflow/tests/CreditApplicationWorkflowTest.java @@ -7,6 +7,7 @@ import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.startsWith; import static org.joda.time.DateTime.now; import java.math.BigDecimal; @@ -76,6 +77,7 @@ 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'")); } } diff --git a/nflow-tests/src/test/java/io/nflow/tests/StateVariablesTest.java b/nflow-tests/src/test/java/io/nflow/tests/StateVariablesTest.java index 73a10dc8a..55218e7e5 100644 --- a/nflow-tests/src/test/java/io/nflow/tests/StateVariablesTest.java +++ b/nflow-tests/src/test/java/io/nflow/tests/StateVariablesTest.java @@ -32,7 +32,6 @@ 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.ListWorkflowInstanceResponse; import io.nflow.rest.v1.msg.UpdateWorkflowInstanceRequest; import io.nflow.tests.demo.workflow.StateWorkflow; @@ -109,7 +108,7 @@ public void updateWorkflowWithTooLongStateVariableValueReturnsBadRequest() { try (Response response = getInstanceIdResource(createResponse.id).put(req)) { assertThat(response.getStatus(), is(BAD_REQUEST.getStatusCode())); - assertThat(response.readEntity(ErrorResponse.class).error, startsWith("Too long value")); + assertThat(response.readEntity(String.class), startsWith("Too long value")); } } @@ -123,7 +122,7 @@ public void insertWorkflowWithTooLongStateVariableValueReturnsBadRequest() { try (Response response = fromClient(workflowInstanceResource, true).put(createRequest)) { assertThat(response.getStatus(), is(BAD_REQUEST.getStatusCode())); - assertThat(response.readEntity(ErrorResponse.class).error, startsWith("Too long value")); + assertThat(response.readEntity(String.class), startsWith("Too long value")); } } From 017f8b688351dab2f900778cf0be59a6090ca847 Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Sun, 19 Apr 2020 01:54:31 +0300 Subject: [PATCH 14/16] fix tests --- .../io/nflow/rest/v1/jaxrs/WorkflowInstanceResourceTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResourceTest.java b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResourceTest.java index 1ecd1eb24..9329858fa 100644 --- a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResourceTest.java +++ b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResourceTest.java @@ -235,7 +235,7 @@ public void fetchingExistingWorkflowWorks() { when(workflowInstances.getWorkflowInstance(42, emptySet(), null)).thenReturn(instance); ListWorkflowInstanceResponse resp = mock(ListWorkflowInstanceResponse.class); when(listWorkflowConverter.convert(eq(instance), any(Set.class))).thenReturn(resp); - ListWorkflowInstanceResponse result = resource.fetchWorkflowInstance(42, null, null); + ListWorkflowInstanceResponse result = resource.fetchWorkflowInstance(42, null, null).readEntity(ListWorkflowInstanceResponse.class); verify(workflowInstances).getWorkflowInstance(42, emptySet(), null); assertEquals(resp, result); } @@ -249,7 +249,7 @@ public void fetchingExistingWorkflowWorksWithAllIncludes() { ListWorkflowInstanceResponse resp = mock(ListWorkflowInstanceResponse.class); when(listWorkflowConverter.convert(eq(instance), any(Set.class))).thenReturn(resp); ListWorkflowInstanceResponse result = resource.fetchWorkflowInstance(42, - "actions,currentStateVariables,actionStateVariables,childWorkflows", 10L); + "actions,currentStateVariables,actionStateVariables,childWorkflows", 10L).readEntity(ListWorkflowInstanceResponse.class); verify(workflowInstances).getWorkflowInstance(42, includes, 10L); assertEquals(resp, result); } From 2da8fc34107c63893be2ed487fd379a8ffdc1384 Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Sun, 19 Apr 2020 01:59:10 +0300 Subject: [PATCH 15/16] fix test --- .../rest/v1/jaxrs/WorkflowInstanceResourceTest.java | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResourceTest.java b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResourceTest.java index 9329858fa..89d3ab2d2 100644 --- a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResourceTest.java +++ b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResourceTest.java @@ -5,13 +5,13 @@ import static java.util.Arrays.asList; import static java.util.Collections.emptySet; import static javax.ws.rs.core.Response.Status.CREATED; +import static javax.ws.rs.core.Response.Status.NOT_FOUND; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.allOf; import static org.hamcrest.Matchers.contains; 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.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.lenient; @@ -25,7 +25,6 @@ import java.util.Optional; import java.util.Set; -import javax.ws.rs.NotFoundException; import javax.ws.rs.core.Response; import org.joda.time.DateTime; @@ -222,10 +221,12 @@ public void listWorkflowInstancesWorksWithAllIncludes() { } @Test - public void fetchingNonExistingWorkflowThrowsNotFoundException() { - when(workflowInstances.getWorkflowInstance(42, emptySet(), null)) - .thenThrow(EmptyResultDataAccessException.class); - assertThrows(NotFoundException.class, () -> resource.fetchWorkflowInstance(42, null, null)); + 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"))); + } } @SuppressWarnings("unchecked") From 59042cb204d4e2eada10dfa7844480658f9d961b Mon Sep 17 00:00:00 2001 From: Edvard Fonsell Date: Sun, 19 Apr 2020 14:57:00 +0300 Subject: [PATCH 16/16] review fixes: make WorkflowInstanceResource error responses to return json body --- .../CustomValidationExceptionMapperTest.java | 8 +++-- .../nflow/rest/v1/msg/SetSignalResponse.java | 15 +++++++++ .../v1/jaxrs/WorkflowInstanceResource.java | 22 +++++++------ .../jaxrs/WorkflowInstanceResourceTest.java | 31 ++++++++++--------- .../springweb/WorkflowInstanceResource.java | 20 +++++++----- .../io/nflow/tests/AbstractNflowTest.java | 5 +-- .../tests/CreditApplicationWorkflowTest.java | 5 ++- .../io/nflow/tests/SignalWorkflowTest.java | 6 ++-- .../io/nflow/tests/StateVariablesTest.java | 8 +++-- 9 files changed, 78 insertions(+), 42 deletions(-) create mode 100644 nflow-rest-api-common/src/main/java/io/nflow/rest/v1/msg/SetSignalResponse.java diff --git a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/CustomValidationExceptionMapperTest.java b/nflow-jetty/src/test/java/io/nflow/jetty/mapper/CustomValidationExceptionMapperTest.java index 165d886ab..52b15fd38 100644 --- a/nflow-jetty/src/test/java/io/nflow/jetty/mapper/CustomValidationExceptionMapperTest.java +++ b/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")); } diff --git a/nflow-rest-api-common/src/main/java/io/nflow/rest/v1/msg/SetSignalResponse.java b/nflow-rest-api-common/src/main/java/io/nflow/rest/v1/msg/SetSignalResponse.java new file mode 100644 index 000000000..f77eee92d --- /dev/null +++ b/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; + +} diff --git a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java b/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java index ac7915e25..2abd657d6 100644 --- a/nflow-rest-api-jax-rs/src/main/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResource.java +++ b/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,13 +120,15 @@ 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, @@ -133,7 +136,7 @@ public Response fetchWorkflowInstance(@ApiParam("Internal id for workflow instan 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 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 diff --git a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResourceTest.java b/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResourceTest.java index 89d3ab2d2..8afa307c1 100644 --- a/nflow-rest-api-jax-rs/src/test/java/io/nflow/rest/v1/jaxrs/WorkflowInstanceResourceTest.java +++ b/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); + } } diff --git a/nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java b/nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java index 1085914d8..6cea1c7f4 100644 --- a/nflow-rest-api-spring-web/src/main/java/io/nflow/rest/v1/springweb/WorkflowInstanceResource.java +++ b/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 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) diff --git a/nflow-tests/src/test/java/io/nflow/tests/AbstractNflowTest.java b/nflow-tests/src/test/java/io/nflow/tests/AbstractNflowTest.java index d767fc0ef..c81785039 100644 --- a/nflow-tests/src/test/java/io/nflow/tests/AbstractNflowTest.java +++ b/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 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) { diff --git a/nflow-tests/src/test/java/io/nflow/tests/CreditApplicationWorkflowTest.java b/nflow-tests/src/test/java/io/nflow/tests/CreditApplicationWorkflowTest.java index 8cc860312..3534ac1ca 100644 --- a/nflow-tests/src/test/java/io/nflow/tests/CreditApplicationWorkflowTest.java +++ b/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'")); } } diff --git a/nflow-tests/src/test/java/io/nflow/tests/SignalWorkflowTest.java b/nflow-tests/src/test/java/io/nflow/tests/SignalWorkflowTest.java index b8c8835d7..4b4eea049 100644 --- a/nflow-tests/src/test/java/io/nflow/tests/SignalWorkflowTest.java +++ b/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 diff --git a/nflow-tests/src/test/java/io/nflow/tests/StateVariablesTest.java b/nflow-tests/src/test/java/io/nflow/tests/StateVariablesTest.java index 55218e7e5..ee0850156 100644 --- a/nflow-tests/src/test/java/io/nflow/tests/StateVariablesTest.java +++ b/nflow-tests/src/test/java/io/nflow/tests/StateVariablesTest.java @@ -2,6 +2,7 @@ import static java.time.Duration.ofSeconds; import static java.util.Collections.singletonMap; +import static javax.ws.rs.core.MediaType.APPLICATION_JSON_TYPE; import static javax.ws.rs.core.Response.Status.BAD_REQUEST; import static org.apache.commons.lang3.StringUtils.repeat; import static org.apache.cxf.jaxrs.client.WebClient.fromClient; @@ -32,6 +33,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.ListWorkflowInstanceResponse; import io.nflow.rest.v1.msg.UpdateWorkflowInstanceRequest; import io.nflow.tests.demo.workflow.StateWorkflow; @@ -108,7 +110,8 @@ public void updateWorkflowWithTooLongStateVariableValueReturnsBadRequest() { try (Response response = getInstanceIdResource(createResponse.id).put(req)) { assertThat(response.getStatus(), is(BAD_REQUEST.getStatusCode())); - assertThat(response.readEntity(String.class), startsWith("Too long value")); + assertThat(response.getMediaType(), is(APPLICATION_JSON_TYPE)); + assertThat(response.readEntity(ErrorResponse.class).error, startsWith("Too long value")); } } @@ -122,7 +125,8 @@ public void insertWorkflowWithTooLongStateVariableValueReturnsBadRequest() { try (Response response = fromClient(workflowInstanceResource, true).put(createRequest)) { assertThat(response.getStatus(), is(BAD_REQUEST.getStatusCode())); - assertThat(response.readEntity(String.class), startsWith("Too long value")); + assertThat(response.getMediaType(), is(APPLICATION_JSON_TYPE)); + assertThat(response.readEntity(ErrorResponse.class).error, startsWith("Too long value")); } }