From 477e45f224ab4a2d4d3b82feda867be9b95af731 Mon Sep 17 00:00:00 2001 From: fjhaveri Date: Mon, 20 Aug 2018 18:35:26 -0700 Subject: [PATCH 1/4] fix(api): added missing null checks --- .../conductor/service/ExecutionService.java | 6 ++++- .../conductor/service/MetadataService.java | 25 ++++++++++++------- .../conductor/service/TaskService.java | 5 +++- 3 files changed, 25 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/com/netflix/conductor/service/ExecutionService.java b/core/src/main/java/com/netflix/conductor/service/ExecutionService.java index 848bd43f16..a5572ca01b 100644 --- a/core/src/main/java/com/netflix/conductor/service/ExecutionService.java +++ b/core/src/main/java/com/netflix/conductor/service/ExecutionService.java @@ -223,8 +223,12 @@ public Map getTaskQueueSizes(List taskDefNames) { return sizes; } - public void removeTaskfromQueue(String taskType, String taskId) { + public void removeTaskfromQueue(String taskId) { Task task = executionDAO.getTask(taskId); + if (task == null) { + throw new ApplicationException(ApplicationException.Code.NOT_FOUND, + String.format("No such task found by taskId: %s", taskId)); + } queueDAO.remove(QueueUtils.getQueueName(task), taskId); } diff --git a/core/src/main/java/com/netflix/conductor/service/MetadataService.java b/core/src/main/java/com/netflix/conductor/service/MetadataService.java index 6faae594ff..15b2550f3c 100644 --- a/core/src/main/java/com/netflix/conductor/service/MetadataService.java +++ b/core/src/main/java/com/netflix/conductor/service/MetadataService.java @@ -18,6 +18,7 @@ */ package com.netflix.conductor.service; +import com.google.common.base.Preconditions; import com.netflix.conductor.annotations.Trace; import com.netflix.conductor.common.metadata.events.EventHandler; import com.netflix.conductor.common.metadata.tasks.TaskDef; @@ -117,11 +118,15 @@ public void updateWorkflowDef(WorkflowDef def) { /** * - * @param wfs Workflow definitions to be updated. + * @param workflowDefList Workflow definitions to be updated. */ - public void updateWorkflowDef(List wfs) { - for (WorkflowDef wf : wfs) { - metadataDAO.update(wf); + public void updateWorkflowDef(List workflowDefList) { + ServiceUtils.checkNotNullOrEmpty(workflowDefList, "WorkflowDef list name cannot be null or empty"); + for (WorkflowDef workflowDef : workflowDefList) { + //TODO: revisit this error handling + ServiceUtils.checkNotNull(workflowDef, "WorkflowDef cannot be null"); + ServiceUtils.checkNotNullOrEmpty(workflowDef.getName(), "WorkflowDef name cannot be null"); + metadataDAO.update(workflowDef); } } @@ -159,14 +164,16 @@ public List getWorkflowDefs() { return metadataDAO.getAll(); } - public void registerWorkflowDef(WorkflowDef def) { - if (def.getName().contains(":")) { + public void registerWorkflowDef(WorkflowDef workflowDef) { + ServiceUtils.checkNotNull(workflowDef, "WorkflowDef cannot be null"); + ServiceUtils.checkNotNullOrEmpty(workflowDef.getName(), "Workflow name cannot be null or empty"); + if (workflowDef.getName().contains(":")) { throw new ApplicationException(Code.INVALID_INPUT, "Workflow name cannot contain the following set of characters: ':'"); } - if (def.getSchemaVersion() < 1 || def.getSchemaVersion() > 2) { - def.setSchemaVersion(2); + if (workflowDef.getSchemaVersion() < 1 || workflowDef.getSchemaVersion() > 2) { + workflowDef.setSchemaVersion(2); } - metadataDAO.create(def); + metadataDAO.create(workflowDef); } /** diff --git a/core/src/main/java/com/netflix/conductor/service/TaskService.java b/core/src/main/java/com/netflix/conductor/service/TaskService.java index 39ee71552a..6f0524a7fa 100644 --- a/core/src/main/java/com/netflix/conductor/service/TaskService.java +++ b/core/src/main/java/com/netflix/conductor/service/TaskService.java @@ -35,6 +35,7 @@ import com.netflix.conductor.common.run.SearchResult; import com.netflix.conductor.common.run.TaskSummary; +import com.netflix.conductor.core.execution.ApplicationException; import com.netflix.conductor.dao.QueueDAO; import com.netflix.conductor.metrics.Monitors; @@ -179,6 +180,7 @@ public List getTaskLogs(String taskId) { */ public Task getTask(String taskId) { ServiceUtils.checkNotNullOrEmpty(taskId, "TaskId cannot be null or empty."); + //TODO: add check if return task is null or not return executionService.getTask(taskId); } @@ -190,7 +192,7 @@ public Task getTask(String taskId) { public void removeTaskFromQueue(String taskType, String taskId) { ServiceUtils.checkNotNullOrEmpty(taskType, "TaskType cannot be null or empty."); ServiceUtils.checkNotNullOrEmpty(taskId, "TaskId cannot be null or empty."); - executionService.removeTaskfromQueue(taskType, taskId); + executionService.removeTaskfromQueue(taskId); } /** @@ -227,6 +229,7 @@ public Map getAllQueueDetails() { */ public List getPollData(String taskType) { ServiceUtils.checkNotNullOrEmpty(taskType, "TaskType cannot be null or empty."); + //TODO: check if taskType is valid or not return executionService.getPollData(taskType); } From 907de072a7de3c6144adadee1694d175eb68add3 Mon Sep 17 00:00:00 2001 From: fjhaveri Date: Mon, 20 Aug 2018 18:44:50 -0700 Subject: [PATCH 2/4] fix(api): change the return to APPLICATION_JSON for /workflow/{name}/correlated resource --- .../netflix/conductor/server/resources/WorkflowResource.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jersey/src/main/java/com/netflix/conductor/server/resources/WorkflowResource.java b/jersey/src/main/java/com/netflix/conductor/server/resources/WorkflowResource.java index 5b6f30918a..31dfd92b12 100644 --- a/jersey/src/main/java/com/netflix/conductor/server/resources/WorkflowResource.java +++ b/jersey/src/main/java/com/netflix/conductor/server/resources/WorkflowResource.java @@ -97,7 +97,7 @@ public List getWorkflows(@PathParam("name") String name, @POST @Path("/{name}/correlated") @ApiOperation("Lists workflows for the given correlation id list") - @Consumes(MediaType.WILDCARD) + @Consumes(MediaType.APPLICATION_JSON) public Map> getWorkflows(@PathParam("name") String name, @QueryParam("includeClosed") @DefaultValue("false") boolean includeClosed, @QueryParam("includeTasks") @DefaultValue("false") boolean includeTasks, From 3f0c0875e2275403aa15ee9c8b2897f84324fb46 Mon Sep 17 00:00:00 2001 From: fjhaveri Date: Mon, 20 Aug 2018 18:45:49 -0700 Subject: [PATCH 3/4] test(api): added new test cases for integration tests --- .../tests/integration/End2EndTests.java | 82 ++++++++++++++++++- 1 file changed, 81 insertions(+), 1 deletion(-) diff --git a/test-harness/src/test/java/com/netflix/conductor/tests/integration/End2EndTests.java b/test-harness/src/test/java/com/netflix/conductor/tests/integration/End2EndTests.java index 9d56171fe8..95be64e7ea 100644 --- a/test-harness/src/test/java/com/netflix/conductor/tests/integration/End2EndTests.java +++ b/test-harness/src/test/java/com/netflix/conductor/tests/integration/End2EndTests.java @@ -40,6 +40,7 @@ import org.junit.BeforeClass; import org.junit.Test; +import java.util.ArrayList; import java.util.HashMap; import java.util.LinkedList; import java.util.List; @@ -267,6 +268,31 @@ public void testInvalidResource() { } } + @Test + public void testUpdateWorkflow() { + WorkflowDef def = new WorkflowDef(); + def.setName("testWorkflowDel"); + def.setVersion(1); + mc.registerWorkflowDef(def); + def.setVersion(2); + List workflowList = new ArrayList<>(); + workflowList.add(def); + mc.updateWorkflowDefs(workflowList); + WorkflowDef def1 = mc.getWorkflowDef(def.getName(), 2); + assertNotNull(def1); + + try{ + mc.getTaskDef("test"); + } catch (ConductorClientException e) { + int statuCode = e.getStatus(); + assertEquals(404, statuCode); + assertEquals("No such taskType found by name: test", e.getMessage()); + assertFalse(e.isRetryable()); + } + } + + + @Test public void testStartWorkflow() { StartWorkflowRequest startWorkflowRequest = new StartWorkflowRequest(); @@ -303,5 +329,59 @@ public void testGetWorfklowNotFound() { assertFalse(e.isRetryable()); } } - + + @Test + public void testEmptyCreateWorkflowDef() { + try{ + WorkflowDef workflowDef = new WorkflowDef(); + mc.registerWorkflowDef(workflowDef); + } catch (ConductorClientException e){ + assertEquals(400, e.getStatus()); + assertEquals("Workflow name cannot be null or empty", e.getMessage()); + assertFalse(e.isRetryable()); + } + } + + @Test + public void testUpdateWorkflowDef() { + try{ + WorkflowDef workflowDef = new WorkflowDef(); + List workflowDefList = new ArrayList<>(); + workflowDefList.add(workflowDef); + mc.updateWorkflowDefs(workflowDefList); + } catch (ConductorClientException e){ + assertEquals(400, e.getStatus()); + assertEquals("WorkflowDef name cannot be null", e.getMessage()); + assertFalse(e.isRetryable()); + } + } + + @Test(expected = Test.None.class /* no exception expected */) + public void testGetTaskInProgress() { + tc.getPendingTaskForWorkflow("test", "t1"); + } + + @Test + public void testRemoveTaskFromTaskQueue() { + try { + tc.removeTaskFromQueue("test", "fakeQueue"); + } catch (ConductorClientException e) { + assertEquals(404, e.getStatus()); + } + } + + @Test + public void testTaskByTaskId() { + try { + tc.getTaskDetails("test123"); + } catch (ConductorClientException e) { + assertEquals(404, e.getStatus()); + assertEquals("No such task found by taskId: test123", e.getMessage()); + } + } + + @Test + public void testListworkflowsByCorrelationId() { + wc.getWorkflows("test", "test12", false, false); + } } From f6aa9b6adcd3d991e5f35d22909d965a1720bb1b Mon Sep 17 00:00:00 2001 From: fjhaveri Date: Tue, 21 Aug 2018 10:50:09 -0700 Subject: [PATCH 4/4] test(api): update the test case --- .../com/netflix/conductor/tests/integration/End2EndTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test-harness/src/test/java/com/netflix/conductor/tests/integration/End2EndTests.java b/test-harness/src/test/java/com/netflix/conductor/tests/integration/End2EndTests.java index 95be64e7ea..8dc4b60564 100644 --- a/test-harness/src/test/java/com/netflix/conductor/tests/integration/End2EndTests.java +++ b/test-harness/src/test/java/com/netflix/conductor/tests/integration/End2EndTests.java @@ -380,7 +380,7 @@ public void testTaskByTaskId() { } } - @Test + @Test(expected = Test.None.class /* no exception expected */) public void testListworkflowsByCorrelationId() { wc.getWorkflows("test", "test12", false, false); }