From 01c25c972c0b70bdb3ed07084f2168a22f381051 Mon Sep 17 00:00:00 2001 From: Aravindan Ramkumar <1028385+aravindanr@users.noreply.github.com> Date: Thu, 14 Jul 2022 13:26:48 -0700 Subject: [PATCH] API to validate a workflow definition --- .../conductor/client/http/MetadataClient.java | 14 ++++- .../common/metadata/workflow/WorkflowDef.java | 2 +- .../conductor/service/MetadataService.java | 10 ++++ .../service/MetadataServiceImpl.java | 12 ++--- .../service/MetadataServiceTest.java | 54 +++++++++++++++++++ .../server/service/MetadataServiceImpl.java | 10 ++++ .../main/proto/grpc/metadata_service.proto | 9 ++++ .../rest/controllers/MetadataResource.java | 6 +++ .../controllers/MetadataResourceTest.java | 7 +++ 9 files changed, 114 insertions(+), 10 deletions(-) diff --git a/client/src/main/java/com/netflix/conductor/client/http/MetadataClient.java b/client/src/main/java/com/netflix/conductor/client/http/MetadataClient.java index b67a9f22a1..b141af6ac0 100644 --- a/client/src/main/java/com/netflix/conductor/client/http/MetadataClient.java +++ b/client/src/main/java/com/netflix/conductor/client/http/MetadataClient.java @@ -39,15 +39,25 @@ public MetadataClient( // Workflow Metadata Operations /** - * Register a workflow definition with the server + * Register a workflow definition with the server. * * @param workflowDef the workflow definition */ public void registerWorkflowDef(WorkflowDef workflowDef) { - Validate.notNull(workflowDef, "Worfklow definition cannot be null"); + Validate.notNull(workflowDef, "Workflow definition cannot be null"); post("metadata/workflow", workflowDef); } + /** + * Validates a workflow definition with the server. + * + * @param workflowDef the workflow definition + */ + public void validateWorkflowDef(WorkflowDef workflowDef) { + Validate.notNull(workflowDef, "Workflow definition cannot be null"); + post("metadata/workflow/validate", workflowDef); + } + /** * Updates a list of existing workflow definitions * diff --git a/common/src/main/java/com/netflix/conductor/common/metadata/workflow/WorkflowDef.java b/common/src/main/java/com/netflix/conductor/common/metadata/workflow/WorkflowDef.java index c91e276922..7ce775ee91 100644 --- a/common/src/main/java/com/netflix/conductor/common/metadata/workflow/WorkflowDef.java +++ b/common/src/main/java/com/netflix/conductor/common/metadata/workflow/WorkflowDef.java @@ -76,7 +76,7 @@ public enum TimeoutPolicy { @Max(value = 2, message = "workflowDef schemaVersion: {value} is only supported") private int schemaVersion = 2; - // By default a workflow is restartable + // By default, a workflow is restartable @ProtoField(id = 9) private boolean restartable = true; 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 9f3110cf94..9acadbb4b7 100644 --- a/core/src/main/java/com/netflix/conductor/service/MetadataService.java +++ b/core/src/main/java/com/netflix/conductor/service/MetadataService.java @@ -103,6 +103,16 @@ Optional getLatestWorkflow( void registerWorkflowDef( @NotNull(message = "WorkflowDef cannot be null") @Valid WorkflowDef workflowDef); + /** + * Validates a {@link WorkflowDef}. + * + * @param workflowDef The {@link WorkflowDef} object. + */ + default void validateWorkflowDef( + @NotNull(message = "WorkflowDef cannot be null") @Valid WorkflowDef workflowDef) { + // do nothing, WorkflowDef is annotated with @Valid and calling this method will validate it + } + /** * @param name Name of the workflow definition to be removed * @param version Version of the workflow definition to be removed diff --git a/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java b/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java index 363e4af4b7..84f6fcf822 100644 --- a/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java +++ b/core/src/main/java/com/netflix/conductor/service/MetadataServiceImpl.java @@ -64,6 +64,11 @@ public void registerTaskDef(List taskDefinitions) { } } + @Override + public void validateWorkflowDef(WorkflowDef workflowDef) { + // do nothing, WorkflowDef is annotated with @Valid and calling this method will validate it + } + /** * @param taskDefinition Task Definition to be updated */ @@ -153,13 +158,6 @@ public List getWorkflowDefs() { } public void registerWorkflowDef(WorkflowDef workflowDef) { - if (workflowDef.getName().contains(":")) { - throw new IllegalArgumentException( - "Workflow name cannot contain the following set of characters: ':'"); - } - if (workflowDef.getSchemaVersion() < 1 || workflowDef.getSchemaVersion() > 2) { - workflowDef.setSchemaVersion(2); - } workflowDef.setCreateTime(System.currentTimeMillis()); metadataDAO.createWorkflowDef(workflowDef); } diff --git a/core/src/test/java/com/netflix/conductor/service/MetadataServiceTest.java b/core/src/test/java/com/netflix/conductor/service/MetadataServiceTest.java index c6c71c6e81..8ea351ac78 100644 --- a/core/src/test/java/com/netflix/conductor/service/MetadataServiceTest.java +++ b/core/src/test/java/com/netflix/conductor/service/MetadataServiceTest.java @@ -298,6 +298,22 @@ public void testRegisterWorkflowDefNoName() { fail("metadataService.registerWorkflowDef did not throw ConstraintViolationException !"); } + @Test(expected = ConstraintViolationException.class) + public void testValidateWorkflowDefNoName() { + try { + WorkflowDef workflowDef = new WorkflowDef(); + metadataService.validateWorkflowDef(workflowDef); + } catch (ConstraintViolationException ex) { + assertEquals(3, ex.getConstraintViolations().size()); + Set messages = getConstraintViolationMessages(ex.getConstraintViolations()); + assertTrue(messages.contains("WorkflowDef name cannot be null or empty")); + assertTrue(messages.contains("WorkflowTask list cannot be empty")); + assertTrue(messages.contains("ownerEmail cannot be empty")); + throw ex; + } + fail("metadataService.validateWorkflowDef did not throw ConstraintViolationException !"); + } + @Test(expected = ConstraintViolationException.class) public void testRegisterWorkflowDefInvalidName() { try { @@ -318,6 +334,26 @@ public void testRegisterWorkflowDefInvalidName() { fail("metadataService.registerWorkflowDef did not throw ConstraintViolationException !"); } + @Test(expected = ConstraintViolationException.class) + public void testValidateWorkflowDefInvalidName() { + try { + WorkflowDef workflowDef = new WorkflowDef(); + workflowDef.setName("invalid:name"); + workflowDef.setOwnerEmail("inavlid-email"); + metadataService.validateWorkflowDef(workflowDef); + } catch (ConstraintViolationException ex) { + assertEquals(3, ex.getConstraintViolations().size()); + Set messages = getConstraintViolationMessages(ex.getConstraintViolations()); + assertTrue(messages.contains("WorkflowTask list cannot be empty")); + assertTrue( + messages.contains( + "Workflow name cannot contain the following set of characters: ':'")); + assertTrue(messages.contains("ownerEmail should be valid email address")); + throw ex; + } + fail("metadataService.validateWorkflowDef did not throw ConstraintViolationException !"); + } + @Test public void testRegisterWorkflowDef() { WorkflowDef workflowDef = new WorkflowDef(); @@ -336,6 +372,24 @@ public void testRegisterWorkflowDef() { assertEquals(2, workflowDef.getSchemaVersion()); } + @Test + public void testValidateWorkflowDef() { + WorkflowDef workflowDef = new WorkflowDef(); + workflowDef.setName("somename"); + workflowDef.setSchemaVersion(2); + workflowDef.setOwnerEmail("sample@test.com"); + List tasks = new ArrayList<>(); + WorkflowTask workflowTask = new WorkflowTask(); + workflowTask.setTaskReferenceName("hello"); + workflowTask.setName("hello"); + tasks.add(workflowTask); + workflowDef.setTasks(tasks); + when(metadataDAO.getTaskDef(any())).thenReturn(new TaskDef()); + metadataService.validateWorkflowDef(workflowDef); + verify(metadataDAO, times(1)).createWorkflowDef(workflowDef); + assertEquals(2, workflowDef.getSchemaVersion()); + } + @Test(expected = ConstraintViolationException.class) public void testUnregisterWorkflowDefNoName() { try { diff --git a/grpc-server/src/main/java/com/netflix/conductor/grpc/server/service/MetadataServiceImpl.java b/grpc-server/src/main/java/com/netflix/conductor/grpc/server/service/MetadataServiceImpl.java index 72ed91331a..369430d689 100644 --- a/grpc-server/src/main/java/com/netflix/conductor/grpc/server/service/MetadataServiceImpl.java +++ b/grpc-server/src/main/java/com/netflix/conductor/grpc/server/service/MetadataServiceImpl.java @@ -55,6 +55,16 @@ public void createWorkflow( response.onCompleted(); } + @Override + public void validateWorkflow( + MetadataServicePb.ValidateWorkflowRequest req, + StreamObserver response) { + WorkflowDef workflow = PROTO_MAPPER.fromProto(req.getWorkflow()); + service.validateWorkflowDef(workflow); + response.onNext(MetadataServicePb.ValidateWorkflowResponse.getDefaultInstance()); + response.onCompleted(); + } + @Override public void updateWorkflows( MetadataServicePb.UpdateWorkflowsRequest req, diff --git a/grpc/src/main/proto/grpc/metadata_service.proto b/grpc/src/main/proto/grpc/metadata_service.proto index 1716c6bbec..0ba4f7ad7d 100644 --- a/grpc/src/main/proto/grpc/metadata_service.proto +++ b/grpc/src/main/proto/grpc/metadata_service.proto @@ -12,6 +12,9 @@ service MetadataService { // POST /workflow rpc CreateWorkflow(CreateWorkflowRequest) returns (CreateWorkflowResponse); + // POST /workflow/validate + rpc ValidateWorkflow(ValidateWorkflowRequest) returns (ValidateWorkflowResponse); + // PUT /workflow rpc UpdateWorkflows(UpdateWorkflowsRequest) returns (UpdateWorkflowsResponse); @@ -37,6 +40,12 @@ message CreateWorkflowRequest { message CreateWorkflowResponse {} +message ValidateWorkflowRequest { + conductor.proto.WorkflowDef workflow = 1; +} + +message ValidateWorkflowResponse {} + message UpdateWorkflowsRequest { repeated conductor.proto.WorkflowDef defs = 1; } diff --git a/rest/src/main/java/com/netflix/conductor/rest/controllers/MetadataResource.java b/rest/src/main/java/com/netflix/conductor/rest/controllers/MetadataResource.java index 096c0a1206..0f9bb546bd 100644 --- a/rest/src/main/java/com/netflix/conductor/rest/controllers/MetadataResource.java +++ b/rest/src/main/java/com/netflix/conductor/rest/controllers/MetadataResource.java @@ -50,6 +50,12 @@ public void create(@RequestBody WorkflowDef workflowDef) { metadataService.registerWorkflowDef(workflowDef); } + @PostMapping("/workflow/validate") + @Operation(summary = "Validates a new workflow definition") + public void validate(@RequestBody WorkflowDef workflowDef) { + metadataService.validateWorkflowDef(workflowDef); + } + @PutMapping("/workflow") @Operation(summary = "Create or update workflow definition") public void update(@RequestBody List workflowDefs) { diff --git a/rest/src/test/java/com/netflix/conductor/rest/controllers/MetadataResourceTest.java b/rest/src/test/java/com/netflix/conductor/rest/controllers/MetadataResourceTest.java index 803933af11..eadf68e8bf 100644 --- a/rest/src/test/java/com/netflix/conductor/rest/controllers/MetadataResourceTest.java +++ b/rest/src/test/java/com/netflix/conductor/rest/controllers/MetadataResourceTest.java @@ -50,6 +50,13 @@ public void testCreateWorkflow() { verify(mockMetadataService, times(1)).registerWorkflowDef(any(WorkflowDef.class)); } + @Test + public void testValidateWorkflow() { + WorkflowDef workflowDef = new WorkflowDef(); + metadataResource.validate(workflowDef); + verify(mockMetadataService, times(1)).validateWorkflowDef(any(WorkflowDef.class)); + } + @Test public void testUpdateWorkflow() { WorkflowDef workflowDef = new WorkflowDef();