From 2878a3ee3b316b6e20bc8738542372c5dbee0eab Mon Sep 17 00:00:00 2001 From: Jack Schofield Date: Fri, 5 Aug 2022 10:05:51 +0100 Subject: [PATCH 1/2] add task id naming validation Signed-off-by: Jack Schofield --- .../Controllers/WorkflowsController.cs | 8 +++++--- .../Validators/WorkflowValidator.cs | 11 +++++++++++ .../Controllers/WorkflowsControllerTests.cs | 14 +++++++++++--- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/WorkflowManager/Controllers/WorkflowsController.cs b/src/WorkflowManager/Controllers/WorkflowsController.cs index 95aeafd5b..3b4013eda 100644 --- a/src/WorkflowManager/Controllers/WorkflowsController.cs +++ b/src/WorkflowManager/Controllers/WorkflowsController.cs @@ -26,8 +26,8 @@ using Monai.Deploy.WorkflowManager.Contracts.Models; using Monai.Deploy.WorkflowManager.Contracts.Responses; using Monai.Deploy.WorkflowManager.Filter; -using Monai.Deploy.WorkflowManager.PayloadListener.Extensions; using Monai.Deploy.WorkflowManager.Services; +using Monai.Deploy.WorkflowManager.Validators; namespace Monai.Deploy.WorkflowManager.Controllers { @@ -133,8 +133,9 @@ public async Task GetAsync([FromRoute] string id) [HttpPost] public async Task CreateAsync([FromBody] Workflow workflow) { - if (!workflow.IsValid(out var validationErrors)) + if (WorkflowValidator.ValidateWorkflow(workflow, out var results)) { + var validationErrors = string.Join(", ", results.Errors); _logger.LogDebug($"{nameof(CreateAsync)} - Failed to validate {nameof(workflow)}: {validationErrors}"); return Problem($"Failed to validate {nameof(workflow)}: {string.Join(", ", validationErrors)}", $"/workflows", BadRequest); @@ -171,8 +172,9 @@ public async Task UpdateAsync([FromBody] Workflow workflow, [From return Problem($"Failed to validate {nameof(id)}, not a valid guid", $"/workflows/{id}", BadRequest); } - if (!workflow.IsValid(out var validationErrors)) + if (WorkflowValidator.ValidateWorkflow(workflow, out var results)) { + var validationErrors = string.Join(", ", results.Errors); _logger.LogDebug($"{nameof(UpdateAsync)} - Failed to validate {nameof(workflow)}: {validationErrors}"); return Problem($"Failed to validate {nameof(workflow)}: {string.Join(", ", validationErrors)}", $"/workflows/{id}", BadRequest); diff --git a/src/WorkflowManager/Validators/WorkflowValidator.cs b/src/WorkflowManager/Validators/WorkflowValidator.cs index 5d69861ae..fd61c2a23 100644 --- a/src/WorkflowManager/Validators/WorkflowValidator.cs +++ b/src/WorkflowManager/Validators/WorkflowValidator.cs @@ -16,6 +16,7 @@ using System.Collections.Generic; using System.Linq; +using System.Text.RegularExpressions; using Monai.Deploy.WorkflowManager.Contracts.Models; using Monai.Deploy.WorkflowManager.PayloadListener.Extensions; @@ -147,6 +148,16 @@ private static void ValidateWorkflowSpec(Workflow workflow) { Errors.Add("Missing Workflow Tasks."); } + + var taskIds = workflow.Tasks.Select(t => t.Id); + var pattern = new Regex(@"^[a-zA-Z0-9-_]+$"); + foreach (var taskId in taskIds) + { + if (pattern.IsMatch(taskId) is false) + { + Errors.Add($"TaskId: {taskId} Contains Invalid Characters."); + } + } } private static void ValidateTask(TaskObject[] tasks, TaskObject currentTask, int iterationCount, List paths = null) diff --git a/tests/UnitTests/WorkflowManager.Tests/Controllers/WorkflowsControllerTests.cs b/tests/UnitTests/WorkflowManager.Tests/Controllers/WorkflowsControllerTests.cs index cdaf99b05..55f348d73 100644 --- a/tests/UnitTests/WorkflowManager.Tests/Controllers/WorkflowsControllerTests.cs +++ b/tests/UnitTests/WorkflowManager.Tests/Controllers/WorkflowsControllerTests.cs @@ -484,7 +484,7 @@ public async Task DeleteAsync_WorkflowsGivenInvalidId_ShouldBadRequest() [Fact] public void ValidateWorkflow_ValidatesAWorkflow_ReturnsTrueAndHasCorrectValidationResultsAsync() { - for (int i = 0; i < 15; i++) + for (var i = 0; i < 15; i++) { var workflow = new WorkflowRevision @@ -626,6 +626,11 @@ public void ValidateWorkflow_ValidatesAWorkflow_ReturnsTrueAndHasCorrectValidati Id = "taskdesc3", Type = "type", Description = "taskdesc", + }, + new TaskObject { + Id = "task_de.sc3?", + Type = "type", + Description = "invalidid", } } } @@ -635,7 +640,7 @@ public void ValidateWorkflow_ValidatesAWorkflow_ReturnsTrueAndHasCorrectValidati Assert.True(workflowHasErrors); - Assert.Equal(24, results.Errors.Count); + Assert.Equal(26, results.Errors.Count); var successPath = "rootTask => taskSucessdesc1 => taskSucessdesc2"; Assert.Contains(successPath, results.SuccessfulPaths); @@ -643,7 +648,7 @@ public void ValidateWorkflow_ValidatesAWorkflow_ReturnsTrueAndHasCorrectValidati var expectedConvergenceError = "Detected task convergence on path: rootTask => taskdesc1 => taskdesc2 => ∞"; Assert.Contains(expectedConvergenceError, results.Errors); - var unreferencedTaskError = "Found Task(s) without any task destinations to it: taskdesc3"; + var unreferencedTaskError = "Found Task(s) without any task destinations to it: taskdesc3,task_de.sc3?"; Assert.Contains(unreferencedTaskError, results.Errors); var loopingTasksError = "Detected task convergence on path: rootTask => taskLoopdesc1 => taskLoopdesc2 => taskLoopdesc3 => taskLoopdesc4 => ∞"; @@ -652,6 +657,9 @@ public void ValidateWorkflow_ValidatesAWorkflow_ReturnsTrueAndHasCorrectValidati var missingDestinationError = "Missing destination DoesNotExistDestination in task taskLoopdesc4"; Assert.Contains(missingDestinationError, results.Errors); + var invalidTaskId = "TaskId: task_de.sc3? Contains Invalid Characters."; + Assert.Contains(invalidTaskId, results.Errors); + WorkflowValidator.Reset(); } } From db6dedea5b117d918bfe7544188e0b44ad979040 Mon Sep 17 00:00:00 2001 From: Jack Schofield Date: Fri, 5 Aug 2022 10:17:11 +0100 Subject: [PATCH 2/2] update docs Signed-off-by: Jack Schofield --- guidelines/mwm-workflow-spec.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/guidelines/mwm-workflow-spec.md b/guidelines/mwm-workflow-spec.md index ec897e1a0..4ad9f8e81 100644 --- a/guidelines/mwm-workflow-spec.md +++ b/guidelines/mwm-workflow-spec.md @@ -122,7 +122,7 @@ All task objects can have these attributes:- | Property | Type | Description | |------|------|------| -|id|str (50)|The id for this task. This should be unique within the current workflow.| +|id|str (50)|The id for this task. This should be unique within the current workflow. Supported chars: Alphanumeric _ - | |description|str (2000)|A human readable task description| |type|str (2000)|The task type - this determines the plugin that will be used to execute the task. See [task types](#task-types) for supported tasks.| |timeout_minutes|number|How long the task is allowed to run before it's canceled|