code review fixes

nflow-engine/src/main/java/com/nitorcreations/nflow/engine/internal/dao/WorkflowDefinitionDao.java

@@ -1,21 +1,22 @@
 package com.nitorcreations.nflow.engine.internal.dao;
 
 import static java.util.Collections.sort;
+import static org.slf4j.LoggerFactory.getLogger;
 import static org.springframework.util.CollectionUtils.isEmpty;
 
 import java.io.IOException;
 import java.sql.ResultSet;
 import java.sql.SQLException;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Comparator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 
 import javax.inject.Inject;
 
+import org.slf4j.Logger;
 import org.springframework.dao.DataIntegrityViolationException;
 import org.springframework.jdbc.core.RowMapper;
 import org.springframework.jdbc.core.namedparam.MapSqlParameterSource;
@@ -26,13 +27,13 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.nitorcreations.nflow.engine.internal.config.NFlow;
 import com.nitorcreations.nflow.engine.internal.workflow.StoredWorkflowDefinition;
-import com.nitorcreations.nflow.engine.internal.workflow.StoredWorkflowDefinition.State;
 import com.nitorcreations.nflow.engine.workflow.definition.WorkflowDefinition;
 import com.nitorcreations.nflow.engine.workflow.definition.WorkflowState;
 
 @Component
 public class WorkflowDefinitionDao {
 
+  private static final Logger logger = getLogger(WorkflowDefinitionDao.class);
   private ExecutorDao executorInfo;
   private NamedParameterJdbcTemplate namedJdbc;
   private ObjectMapper nflowObjectMapper;
@@ -69,7 +70,7 @@ public void storeWorkflowDefinition(WorkflowDefinition<? extends WorkflowState>
       try {
         namedJdbc.update(sql, params);
       } catch (DataIntegrityViolationException dex) {
-        // ignore: some other executor already stored the definition
+        logger.debug("Another executor already stored the definition.", dex);
       }
     }
   }
@@ -109,22 +110,16 @@ StoredWorkflowDefinition convert(WorkflowDefinition<? extends WorkflowState> def
       StoredWorkflowDefinition.State state = states.get(entry.getKey());
       state.onFailure = entry.getValue().name();
     }
-    List<StoredWorkflowDefinition.State> sortedStates = new ArrayList<>(states.values());
-    sort(sortedStates, new Comparator<StoredWorkflowDefinition.State>() {
-      @Override
-      public int compare(State state, State otherState) {
-        return state.type.compareTo(otherState.type);
-      }
-    });
-    resp.states = sortedStates;
+    resp.states = new ArrayList<>(states.values());
+    sort(resp.states);
     return resp;
   }
 
   private String serializeDefinition(StoredWorkflowDefinition storedDefinition) {
     try {
       return nflowObjectMapper.writeValueAsString(storedDefinition);
     } catch (JsonProcessingException e) {
-      throw new RuntimeException("Failed to serialize workflow definition " + storedDefinition.type);
+      throw new RuntimeException("Failed to serialize workflow definition " + storedDefinition.type, e);
     }
   }
 

nflow-engine/src/main/java/com/nitorcreations/nflow/engine/internal/workflow/StoredWorkflowDefinition.java

@@ -9,10 +9,10 @@ public class StoredWorkflowDefinition {
   public String onError;
   public List<State> states;
 
-  public static class State {
+  public static class State implements Comparable<State> {
 
     public State() {
-      //
+      // default constructor is required by Jackson deserializer
     }
 
     public State(String id, String type, String description) {
@@ -26,5 +26,10 @@ public State(String id, String type, String description) {
     public String description;
     public List<String> transitions = new ArrayList<>();
     public String onFailure;
+
+    @Override
+    public int compareTo(State state) {
+      return type.compareTo(state.type);
+    }
   }
 }

nflow-engine/src/main/resources/scripts/db/h2.create.ddl.sql

@@ -59,4 +59,4 @@ create table if not exists nflow_workflow_definition (
   executor_group varchar(64) not null
 );
 
-create unique index if not exists nflow_workflow_definition_uniq on nflow_workflow_definition (type, executor_group);
+create unique index if not exists nflow_workflow_definition_uniq on nflow_workflow_definition (type, executor_group);

nflow-engine/src/main/resources/scripts/db/mysql.create.ddl.sql

@@ -55,4 +55,4 @@ create table if not exists nflow_workflow_definition (
   modified_by int not null,
   executor_group varchar(64) not null,
   constraint nflow_workflow_definition_uniq unique (type, executor_group)
-);
+);

nflow-engine/src/main/resources/scripts/db/mysql.legacy.create.ddl.sql

@@ -65,4 +65,4 @@ create table if not exists nflow_workflow_definition (
 drop trigger if exists nflow_workflow_definition_insert;
 
 create trigger nflow_workflow_definition_insert before insert on `nflow_workflow_definition`
-  for each row set new.created = now();
+  for each row set new.created = now();

nflow-engine/src/main/resources/scripts/db/postgresql.create.ddl.sql

@@ -62,7 +62,7 @@ create table if not exists nflow_executor (
 create table if not exists nflow_workflow_definition (
   id serial primary key,
   type varchar(64) not null,
-  definition text not null,
+  definition json not null,
   created timestamptz not null default current_timestamp,
   modified timestamptz not null default current_timestamp,
   modified_by int not null,
@@ -71,4 +71,4 @@ create table if not exists nflow_workflow_definition (
 );
 
 drop trigger if exists update_nflow_definition_modified on nflow_workflow_definition;
-create trigger update_nflow_definition_modified before update on nflow_workflow_definition for each row execute procedure update_modified();
+create trigger update_nflow_definition_modified before update on nflow_workflow_definition for each row execute procedure update_modified();

nflow-engine/src/test/java/com/nitorcreations/nflow/engine/internal/dao/WorkflowDefinitionDaoTest.java

@@ -26,17 +26,17 @@ public class WorkflowDefinitionDaoTest extends BaseDaoTest {
   private final DummyTestWorkflow original = new DummyTestWorkflow();
 
   @Test
-  public void roundTripTest() {
+  public void storeAndLoadRequestedDefinitionFromDatabaseWorks() {
     roundTrip(asList(original.getType()));
   }
 
   @Test
-  public void roundTripTestWithoutArguments() {
+  public void storeAndLoadAllDefinitionsFromDatabaseWorks() {
     roundTrip(new ArrayList<String>());
   }
 
   @Test
-  public void roundTripTestDefinitionUpdated() {
+  public void storeAndUpdateDefinitionWorks() {
     roundTrip(asList(original.getType()));
     roundTrip(asList(original.getType()));
   }
@@ -50,6 +50,7 @@ private void roundTrip(List<String> typeQueryParameters) {
     assertThat(stored.type, is(convertedOriginal.type));
     assertThat(stored.onError, is(convertedOriginal.onError));
     assertThat(stored.description, is(convertedOriginal.description));
+    assertThat(stored.states.size(), is(convertedOriginal.states.size()));
     for (int i = 0; i < convertedOriginal.states.size(); i++) {
       assertThat(stored.states.get(i), reflectEquals(convertedOriginal.states.get(i)));
     }
@@ -76,9 +77,10 @@ public void convertStoredDefinitionWorks() {
             assertThat(convertedState.transitions.size(), is(0));
           }
           foundMatchingState = true;
+          break;
         }
       }
       assertThat("Not found match for state " + convertedState.id, foundMatchingState, is(true));
     }
   }
-}
+}

nflow-engine/src/test/java/com/nitorcreations/nflow/engine/service/DummyTestWorkflow.java

@@ -37,6 +37,8 @@ public String getDescription() {
 
   public DummyTestWorkflow() {
     super("dummy", DummyTestState.start, DummyTestState.end);
+    permit(DummyTestState.start, DummyTestState.end, DummyTestState.end);
+    permit(DummyTestState.alternativeStart, DummyTestState.end);
   }
 
   public NextAction start(StateExecution execution) {

nflow-rest-api/src/main/java/com/nitorcreations/nflow/rest/v1/WorkflowDefinitionResource.java

@@ -1,10 +1,10 @@
 package com.nitorcreations.nflow.rest.v1;
 
 import static java.util.Arrays.asList;
+import static java.util.Collections.sort;
 import static javax.ws.rs.core.MediaType.APPLICATION_JSON;
 
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -56,11 +56,11 @@ public WorkflowDefinitionResource(WorkflowDefinitionService workflowDefinitions,
 
   @GET
   @ApiOperation(value = "List workflow definitions", response = ListWorkflowDefinitionResponse.class, responseContainer = "List")
-  public Collection<ListWorkflowDefinitionResponse> listWorkflowInstances(@QueryParam("type") String[] types) {
+  public List<ListWorkflowDefinitionResponse> listWorkflowDefinitions(@QueryParam("type") String[] types) {
     List<WorkflowDefinition<? extends WorkflowState>> definitions = workflowDefinitions.getWorkflowDefinitions();
     Set<String> reqTypes = new LinkedHashSet<>(asList(types));
     Set<String> foundTypes = new HashSet<>();
-    Collection<ListWorkflowDefinitionResponse> response = new ArrayList<>();
+    List<ListWorkflowDefinitionResponse> response = new ArrayList<>();
     for (WorkflowDefinition<? extends WorkflowState> definition : definitions) {
       if (!reqTypes.isEmpty() && !reqTypes.contains(definition.getType())) {
         continue;
@@ -84,6 +84,7 @@ public Collection<ListWorkflowDefinitionResponse> listWorkflowInstances(@QueryPa
         response.add(converter.convert(storedDefinition));
       }
     }
+    sort(response);
     return response;
   }
 

nflow-rest-api/src/main/java/com/nitorcreations/nflow/rest/v1/msg/ListWorkflowDefinitionResponse.java

@@ -7,7 +7,7 @@
 
 @ApiModel(value = "Basic information of workflow definition")
 @SuppressFBWarnings(value="URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD", justification="jackson reads dto fields")
-public class ListWorkflowDefinitionResponse {
+public class ListWorkflowDefinitionResponse implements Comparable<ListWorkflowDefinitionResponse> {
 
   @ApiModelProperty(value = "Type of the workflow definition", required=true)
   public String type;
@@ -57,4 +57,9 @@ public static class TransitionDelays {
 
   }
 
+  @Override
+  public int compareTo(ListWorkflowDefinitionResponse response) {
+    return type.compareTo(response.type);
+  }
+
 }

nflow-rest-api/src/test/java/com/nitorcreations/nflow/rest/v1/WorkflowDefinitionResourceTest.java

@@ -63,36 +63,36 @@ public void setup() {
   }
 
   @Test
-  public void listWorkflowInstancesFindsExistingDefinition() {
-    Collection<ListWorkflowDefinitionResponse> ret = resource.listWorkflowInstances(new String[] { "dummy" } );
+  public void listWorkflowDefinitionsFindsExistingDefinition() {
+    Collection<ListWorkflowDefinitionResponse> ret = resource.listWorkflowDefinitions(new String[] { "dummy" });
     assertThat(ret.size(), is(1));
   }
 
   @Test
-  public void listWorkflowInstancesNotFindsNonExistentDefinition() {
-    Collection<ListWorkflowDefinitionResponse> ret = resource.listWorkflowInstances(new String[] { "nonexistent" } );
+  public void listWorkflowDefinitionsDoesNotFindNonExistentDefinition() {
+    Collection<ListWorkflowDefinitionResponse> ret = resource.listWorkflowDefinitions(new String[] { "nonexistent" });
     assertThat(ret.size(), is(0));
     verify(workflowDefinitionDao).queryStoredWorkflowDefinitions(stringList.capture());
     assertThat(stringList.getValue(), containsElements(asList("nonexistent")));
   }
 
   @Test
-  public void listWorkflowInstancesFindsExistingDefinitionWithoutArguments() {
-    Collection<ListWorkflowDefinitionResponse> ret = resource.listWorkflowInstances(new String[] {});
+  public void listWorkflowDefinitionsFindsExistingDefinitionWithoutArguments() {
+    Collection<ListWorkflowDefinitionResponse> ret = resource.listWorkflowDefinitions(new String[] {});
     assertThat(ret.size(), is(1));
     verify(workflowDefinitionDao).queryStoredWorkflowDefinitions(stringList.capture());
     assertThat(stringList.getValue().size(), is(0));
   }
 
   @Test
-  public void listWorkflowInstancesFindsExistingAndStoredDefinitionsWithoutArguments() {
+  public void listWorkflowDefinitionsFindsExistingAndStoredDefinitionsWithoutArguments() {
     ArrayList<StoredWorkflowDefinition> storedDefinitions = new ArrayList<>();
     StoredWorkflowDefinition storedDefinition = mock(StoredWorkflowDefinition.class);
     storedDefinitions.add(storedDefinition);
     when(workflowDefinitionDao.queryStoredWorkflowDefinitions(anyCollectionOf(String.class))).thenReturn(storedDefinitions);
     ListWorkflowDefinitionResponse storedDefinitionResponse = mock(ListWorkflowDefinitionResponse.class);
     when(converter.convert(storedDefinition)).thenReturn(storedDefinitionResponse);
-    Collection<ListWorkflowDefinitionResponse> ret = resource.listWorkflowInstances(new String[] {});
+    Collection<ListWorkflowDefinitionResponse> ret = resource.listWorkflowDefinitions(new String[] {});
     assertThat(ret.size(), is(2));
     verify(workflowDefinitionDao).queryStoredWorkflowDefinitions(stringList.capture());
     assertThat(stringList.getValue().size(), is(0));