Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Commit

Permalink
Merge pull request #727 from Netflix/refactor/improve_api_error_handling
Browse files Browse the repository at this point in the history 
Refactor/improve api error handling
  • Loading branch information
@falu2010-netflix
falu2010-netflix authored Aug 21, 2018
2 parents 0475f9b + f6aa9b6 commit e7431b4
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 13 deletions.
6 changes: 5 additions & 1 deletion core/src/main/java/com/netflix/conductor/service/ExecutionService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,12 @@ public Map<String, Integer> getTaskQueueSizes(List<String> 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);
}

Expand Down
25 changes: 16 additions & 9 deletions core/src/main/java/com/netflix/conductor/service/MetadataService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<WorkflowDef> wfs) {
for (WorkflowDef wf : wfs) {
metadataDAO.update(wf);
public void updateWorkflowDef(List<WorkflowDef> 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);
}
}

Expand Down Expand Up @@ -159,14 +164,16 @@ public List<WorkflowDef> 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);
}

/**
Expand Down
5 changes: 4 additions & 1 deletion core/src/main/java/com/netflix/conductor/service/TaskService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -179,6 +180,7 @@ public List<TaskExecLog> 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);
}

Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -227,6 +229,7 @@ public Map<String, Long> getAllQueueDetails() {
*/
public List<PollData> getPollData(String taskType) {
ServiceUtils.checkNotNullOrEmpty(taskType, "TaskType cannot be null or empty.");
//TODO: check if taskType is valid or not
return executionService.getPollData(taskType);
}

Expand Down
2 changes: 1 addition & 1 deletion jersey/src/main/java/com/netflix/conductor/server/resources/WorkflowResource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public List<Workflow> 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<String, List<Workflow>> getWorkflows(@PathParam("name") String name,
@QueryParam("includeClosed") @DefaultValue("false") boolean includeClosed,
@QueryParam("includeTasks") @DefaultValue("false") boolean includeTasks,
Expand Down
82 changes: 81 additions & 1 deletion test-harness/src/test/java/com/netflix/conductor/tests/integration/End2EndTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<WorkflowDef> 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();
Expand Down Expand Up @@ -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<WorkflowDef> 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(expected = Test.None.class /* no exception expected */)
public void testListworkflowsByCorrelationId() {
wc.getWorkflows("test", "test12", false, false);
}
}

0 comments on commit e7431b4

Please sign in to comment.