From 3dedf66cbe261a5cc28d78ccdf4e101c4fcc32d8 Mon Sep 17 00:00:00 2001 From: Aravindan Ramkumar <1028385+aravindanr@users.noreply.github.com> Date: Thu, 3 Dec 2020 11:39:13 -0800 Subject: [PATCH] Added 204 "No Content" responses to fix tests and for backwards compatibility with 2.x clients --- .../rest/controllers/TaskResource.java | 69 ++++++++++--------- .../rest/controllers/WorkflowResource.java | 18 +++-- .../rest/controllers/TaskResourceTest.java | 10 ++- 3 files changed, 58 insertions(+), 39 deletions(-) diff --git a/rest/src/main/java/com/netflix/conductor/rest/controllers/TaskResource.java b/rest/src/main/java/com/netflix/conductor/rest/controllers/TaskResource.java index 44fb753188..f91977642e 100644 --- a/rest/src/main/java/com/netflix/conductor/rest/controllers/TaskResource.java +++ b/rest/src/main/java/com/netflix/conductor/rest/controllers/TaskResource.java @@ -12,10 +12,6 @@ */ package com.netflix.conductor.rest.controllers; -import static com.netflix.conductor.rest.config.RequestMappingConstants.TASKS; -import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE; -import static org.springframework.http.MediaType.TEXT_PLAIN_VALUE; - import com.netflix.conductor.common.metadata.tasks.PollData; import com.netflix.conductor.common.metadata.tasks.Task; import com.netflix.conductor.common.metadata.tasks.TaskExecLog; @@ -25,8 +21,7 @@ import com.netflix.conductor.common.run.TaskSummary; import com.netflix.conductor.service.TaskService; import io.swagger.v3.oas.annotations.Operation; -import java.util.List; -import java.util.Map; +import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.DeleteMapping; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; @@ -36,8 +31,16 @@ import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RestController; +import java.util.List; +import java.util.Map; +import java.util.Optional; + +import static com.netflix.conductor.rest.config.RequestMappingConstants.TASKS; +import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE; +import static org.springframework.http.MediaType.TEXT_PLAIN_VALUE; + @RestController -@RequestMapping(TASKS) +@RequestMapping(value = TASKS, produces = APPLICATION_JSON_VALUE) public class TaskResource { private final TaskService taskService; @@ -49,34 +52,37 @@ public TaskResource(TaskService taskService) { @GetMapping("/poll/{tasktype}") @Operation(summary = "Poll for a task of a certain type") public Task poll(@PathVariable("tasktype") String taskType, - @RequestParam(value = "workerid", required = false) String workerId, - @RequestParam(value = "domain", required = false) String domain) { + @RequestParam(value = "workerid", required = false) String workerId, + @RequestParam(value = "domain", required = false) String domain) { return taskService.poll(taskType, workerId, domain); } @GetMapping("/poll/batch/{tasktype}") @Operation(summary = "Batch poll for a task of a certain type") public List batchPoll(@PathVariable("tasktype") String taskType, - @RequestParam(value = "workerid", required = false) String workerId, - @RequestParam(value = "domain", required = false) String domain, - @RequestParam(value = "count", defaultValue = "1") int count, - @RequestParam(value = "timeout", defaultValue = "100") int timeout) { + @RequestParam(value = "workerid", required = false) String workerId, + @RequestParam(value = "domain", required = false) String domain, + @RequestParam(value = "count", defaultValue = "1") int count, + @RequestParam(value = "timeout", defaultValue = "100") int timeout) { return taskService.batchPoll(taskType, workerId, domain, count, timeout); } @GetMapping("/in_progress/{tasktype}") @Operation(summary = "Get in progress tasks. The results are paginated.") public List getTasks(@PathVariable("tasktype") String taskType, - @RequestParam(value = "startKey", required = false) String startKey, - @RequestParam(value = "count", defaultValue = "100", required = false) int count) { + @RequestParam(value = "startKey", required = false) String startKey, + @RequestParam(value = "count", defaultValue = "100", required = false) int count) { return taskService.getTasks(taskType, startKey, count); } - @GetMapping("/in_progress/{workflowId}/{taskRefName}") + @GetMapping(value = "/in_progress/{workflowId}/{taskRefName}") @Operation(summary = "Get in progress task for a given workflow id.") - public Task getPendingTaskForWorkflow(@PathVariable("workflowId") String workflowId, - @PathVariable("taskRefName") String taskReferenceName) { - return taskService.getPendingTaskForWorkflow(workflowId, taskReferenceName); + public ResponseEntity getPendingTaskForWorkflow(@PathVariable("workflowId") String workflowId, + @PathVariable("taskRefName") String taskReferenceName) { + // for backwards compatibility with 2.x client which expects a 204 when no Task is found + return Optional.ofNullable(taskService.getPendingTaskForWorkflow(workflowId, taskReferenceName)) + .map(ResponseEntity::ok) + .orElse(ResponseEntity.noContent().build()); } @PostMapping(produces = {TEXT_PLAIN_VALUE, APPLICATION_JSON_VALUE}) @@ -88,7 +94,7 @@ public String updateTask(@RequestBody TaskResult taskResult) { @PostMapping("/{taskId}/ack") @Operation(summary = "Ack Task is received") public String ack(@PathVariable("taskId") String taskId, - @RequestParam(value = "workerid", required = false) String workerId) { + @RequestParam(value = "workerid", required = false) String workerId) { return taskService.ackTaskReceived(taskId, workerId); } @@ -106,14 +112,15 @@ public List getTaskLogs(@PathVariable("taskId") String taskId) { @GetMapping("/{taskId}") @Operation(summary = "Get task by Id") - public Task getTask(@PathVariable("taskId") String taskId) { - return taskService.getTask(taskId); + public ResponseEntity getTask(@PathVariable("taskId") String taskId) { + // for backwards compatibility with 2.x client which expects a 204 when no Task is found + return Optional.ofNullable(taskService.getTask(taskId)).map(ResponseEntity::ok).orElse(ResponseEntity.noContent().build()); } @DeleteMapping("/queue/{taskType}/{taskId}") @Operation(summary = "Remove Task from a Task type queue") public void removeTaskFromQueue(@PathVariable("taskType") String taskType, - @PathVariable("taskId") String taskId) { + @PathVariable("taskId") String taskId) { taskService.removeTaskFromQueue(taskType, taskId); } @@ -154,22 +161,22 @@ public String requeuePendingTask(@PathVariable("taskType") String taskType) { } @Operation(summary = "Search for tasks based in payload and other parameters", - description = "use sort options as sort=:ASC|DESC e.g. sort=name&sort=workflowId:DESC." + - " If order is not specified, defaults to ASC") + description = "use sort options as sort=:ASC|DESC e.g. sort=name&sort=workflowId:DESC." + + " If order is not specified, defaults to ASC") @GetMapping(value = "/search", produces = APPLICATION_JSON_VALUE) public SearchResult search( - @RequestParam(value = "start", defaultValue = "0", required = false) int start, - @RequestParam(value = "size", defaultValue = "100", required = false) int size, - @RequestParam(value = "sort", required = false) String sort, - @RequestParam(value = "freeText", defaultValue = "*", required = false) String freeText, - @RequestParam(value = "query", required = false) String query) { + @RequestParam(value = "start", defaultValue = "0", required = false) int start, + @RequestParam(value = "size", defaultValue = "100", required = false) int size, + @RequestParam(value = "sort", required = false) String sort, + @RequestParam(value = "freeText", defaultValue = "*", required = false) String freeText, + @RequestParam(value = "query", required = false) String query) { return taskService.search(start, size, sort, freeText, query); } @Operation(summary = "Get the external uri where the task payload is to be stored") @GetMapping("/externalstoragelocation") public ExternalStorageLocation getExternalStorageLocation(@RequestParam("path") String path, - @RequestParam("operation") String operation, @RequestParam("payloadType") String payloadType) { + @RequestParam("operation") String operation, @RequestParam("payloadType") String payloadType) { return taskService.getExternalStorageLocation(path, operation, payloadType); } } diff --git a/rest/src/main/java/com/netflix/conductor/rest/controllers/WorkflowResource.java b/rest/src/main/java/com/netflix/conductor/rest/controllers/WorkflowResource.java index 4dc9cdacf6..2dd7a8ad19 100644 --- a/rest/src/main/java/com/netflix/conductor/rest/controllers/WorkflowResource.java +++ b/rest/src/main/java/com/netflix/conductor/rest/controllers/WorkflowResource.java @@ -12,10 +12,6 @@ */ package com.netflix.conductor.rest.controllers; -import static com.netflix.conductor.rest.config.RequestMappingConstants.WORKFLOW; -import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE; -import static org.springframework.http.MediaType.TEXT_PLAIN_VALUE; - import com.netflix.conductor.common.metadata.workflow.RerunWorkflowRequest; import com.netflix.conductor.common.metadata.workflow.SkipTaskRequest; import com.netflix.conductor.common.metadata.workflow.StartWorkflowRequest; @@ -25,8 +21,7 @@ import com.netflix.conductor.common.run.WorkflowSummary; import com.netflix.conductor.service.WorkflowService; import io.swagger.v3.oas.annotations.Operation; -import java.util.List; -import java.util.Map; +import org.springframework.http.HttpStatus; import org.springframework.web.bind.annotation.DeleteMapping; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; @@ -35,8 +30,16 @@ import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.ResponseStatus; import org.springframework.web.bind.annotation.RestController; +import java.util.List; +import java.util.Map; + +import static com.netflix.conductor.rest.config.RequestMappingConstants.WORKFLOW; +import static org.springframework.http.MediaType.APPLICATION_JSON_VALUE; +import static org.springframework.http.MediaType.TEXT_PLAIN_VALUE; + @RestController @RequestMapping(WORKFLOW) public class WorkflowResource { @@ -140,6 +143,7 @@ public String rerun(@PathVariable("workflowId") String workflowId, @PostMapping("/{workflowId}/restart") @Operation(summary = "Restarts a completed workflow") + @ResponseStatus(value = HttpStatus.NO_CONTENT) // for backwards compatibility with 2.x client which expects a 204 for this request public void restart(@PathVariable("workflowId") String workflowId, @RequestParam(value = "useLatestDefinitions", defaultValue = "false", required = false) boolean useLatestDefinitions) { workflowService.restartWorkflow(workflowId, useLatestDefinitions); @@ -147,12 +151,14 @@ public void restart(@PathVariable("workflowId") String workflowId, @PostMapping("/{workflowId}/retry") @Operation(summary = "Retries the last failed task") + @ResponseStatus(value = HttpStatus.NO_CONTENT) // for backwards compatibility with 2.x client which expects a 204 for this request public void retry(@PathVariable("workflowId") String workflowId) { workflowService.retryWorkflow(workflowId); } @PostMapping("/{workflowId}/resetcallbacks") @Operation(summary = "Resets callback times of all non-terminal SIMPLE tasks to 0") + @ResponseStatus(value = HttpStatus.NO_CONTENT) // for backwards compatibility with 2.x client which expects a 204 for this request public void resetWorkflow(@PathVariable("workflowId") String workflowId) { workflowService.resetWorkflow(workflowId); } diff --git a/rest/src/test/java/com/netflix/conductor/rest/controllers/TaskResourceTest.java b/rest/src/test/java/com/netflix/conductor/rest/controllers/TaskResourceTest.java index d337104ef2..68e5dcc85a 100644 --- a/rest/src/test/java/com/netflix/conductor/rest/controllers/TaskResourceTest.java +++ b/rest/src/test/java/com/netflix/conductor/rest/controllers/TaskResourceTest.java @@ -21,6 +21,7 @@ import com.netflix.conductor.service.TaskService; import org.junit.Before; import org.junit.Test; +import org.springframework.http.ResponseEntity; import java.util.ArrayList; import java.util.HashMap; @@ -28,6 +29,7 @@ import java.util.Map; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyList; @@ -97,7 +99,9 @@ public void testGetPendingTaskForWorkflow() { task.setDomain("test"); task.setStatus(Task.Status.IN_PROGRESS); when(mockTaskService.getPendingTaskForWorkflow(anyString(), anyString())).thenReturn(task); - assertEquals(task, taskResource.getPendingTaskForWorkflow("SIMPLE", "123")); + ResponseEntity entity = taskResource.getPendingTaskForWorkflow("SIMPLE", "123"); + assertNotNull(entity); + assertEquals(task, entity.getBody()); } @Test @@ -138,7 +142,9 @@ public void testGetTask() { task.setDomain("test"); task.setStatus(Task.Status.IN_PROGRESS); when(mockTaskService.getTask(anyString())).thenReturn(task); - assertEquals(task, taskResource.getTask("123")); + ResponseEntity entity = taskResource.getTask("123"); + assertNotNull(entity); + assertEquals(task, entity.getBody()); } @Test