diff --git a/CHANGELOG.md b/CHANGELOG.md index 07935fc7b..6a4108ce2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) - Incrementally remove resources from workflow state during deprovisioning ([#898](https://github.com/opensearch-project/flow-framework/pull/898)) ### Bug Fixes +- Remove useCase and defaultParams field in WorkflowRequest ([#758](https://github.com/opensearch-project/flow-framework/pull/758)) - Fixed Template Update Location and Improved Logger Statements in ReprovisionWorkflowTransportAction ([#918](https://github.com/opensearch-project/flow-framework/pull/918)) ### Infrastructure diff --git a/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java index 8acfab16a..4abedc365 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java @@ -226,8 +226,6 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli validation, provision || updateFields, params, - useCase, - useCaseDefaultsMap, reprovision ); diff --git a/src/main/java/org/opensearch/flowframework/transport/WorkflowRequest.java b/src/main/java/org/opensearch/flowframework/transport/WorkflowRequest.java index a258e6e10..97f032e31 100644 --- a/src/main/java/org/opensearch/flowframework/transport/WorkflowRequest.java +++ b/src/main/java/org/opensearch/flowframework/transport/WorkflowRequest.java @@ -62,23 +62,13 @@ public class WorkflowRequest extends ActionRequest { */ private Map params; - /** - * use case flag - */ - private String useCase; - - /** - * Deafult params map from use case - */ - private Map defaultParams; - /** * Instantiates a new WorkflowRequest, set validation to all, no provisioning * @param workflowId the documentId of the workflow * @param template the use case template which describes the workflow */ public WorkflowRequest(@Nullable String workflowId, @Nullable Template template) { - this(workflowId, template, new String[] { "all" }, false, Collections.emptyMap(), null, Collections.emptyMap(), false); + this(workflowId, template, new String[] { "all" }, false, Collections.emptyMap(), false); } /** @@ -88,28 +78,7 @@ public WorkflowRequest(@Nullable String workflowId, @Nullable Template template) * @param params The parameters from the REST path */ public WorkflowRequest(@Nullable String workflowId, @Nullable Template template, Map params) { - this(workflowId, template, new String[] { "all" }, true, params, null, Collections.emptyMap(), false); - } - - /** - * Instantiates a new WorkflowRequest with params map, set validation to all, provisioning to true - * @param workflowId the documentId of the workflow - * @param template the use case template which describes the workflow - * @param useCase the default use case give by user - * @param defaultParams The parameters from the REST body when a use case is given - */ - public WorkflowRequest(@Nullable String workflowId, @Nullable Template template, String useCase, Map defaultParams) { - this(workflowId, template, new String[] { "all" }, false, Collections.emptyMap(), useCase, defaultParams, false); - } - - /** - * Instantiates a new WorkflowRequest, set validation to all, sets reprovision flag - * @param workflowId the documentId of the workflow - * @param template the updated template - * @param reprovision the reprovision flag - */ - public WorkflowRequest(String workflowId, Template template, boolean reprovision) { - this(workflowId, template, new String[] { "all" }, false, Collections.emptyMap(), null, Collections.emptyMap(), reprovision); + this(workflowId, template, new String[] { "all" }, true, params, false); } /** @@ -119,8 +88,6 @@ public WorkflowRequest(String workflowId, Template template, boolean reprovision * @param validation flag to indicate if validation is necessary * @param provisionOrUpdate provision or updateFields flag. Only one may be true, the presence of update_fields key in map indicates if updating fields, otherwise true means it's provisioning. * @param params map of REST path params. If provisionOrUpdate is false, must be an empty map. If update_fields key is present, must be only key. - * @param useCase default use case given - * @param defaultParams the params to be used in the substitution based on the default use case. * @param reprovision flag to indicate if request is to reprovision */ public WorkflowRequest( @@ -129,8 +96,6 @@ public WorkflowRequest( String[] validation, boolean provisionOrUpdate, Map params, - String useCase, - Map defaultParams, boolean reprovision ) { this.workflowId = workflowId; @@ -142,8 +107,6 @@ public WorkflowRequest( throw new IllegalArgumentException("Params may only be included when provisioning."); } this.params = this.updateFields ? Collections.emptyMap() : params; - this.useCase = useCase; - this.defaultParams = defaultParams; this.reprovision = reprovision; } @@ -222,22 +185,6 @@ public Map getParams() { return Map.copyOf(this.params); } - /** - * Gets the use case - * @return the use case - */ - public String getUseCase() { - return this.useCase; - } - - /** - * Gets the params map - * @return the params map - */ - public Map getDefaultParams() { - return Map.copyOf(this.defaultParams); - } - /** * Gets the reprovision flag * @return the reprovision boolean diff --git a/src/test/java/org/opensearch/flowframework/transport/CreateWorkflowTransportActionTests.java b/src/test/java/org/opensearch/flowframework/transport/CreateWorkflowTransportActionTests.java index 52238871e..ba76bc833 100644 --- a/src/test/java/org/opensearch/flowframework/transport/CreateWorkflowTransportActionTests.java +++ b/src/test/java/org/opensearch/flowframework/transport/CreateWorkflowTransportActionTests.java @@ -252,7 +252,7 @@ public void testMaxWorkflow() { @SuppressWarnings("unchecked") ActionListener listener = mock(ActionListener.class); - WorkflowRequest workflowRequest = new WorkflowRequest(null, template, new String[] { "off" }, false, Collections.emptyMap(), null, Collections.emptyMap(), false); + WorkflowRequest workflowRequest = new WorkflowRequest(null, template, new String[] { "off" }, false, Collections.emptyMap(), false); doAnswer(invocation -> { ActionListener searchListener = invocation.getArgument(1); @@ -289,16 +289,7 @@ public void onFailure(Exception e) { public void testFailedToCreateNewWorkflow() { @SuppressWarnings("unchecked") ActionListener listener = mock(ActionListener.class); - WorkflowRequest workflowRequest = new WorkflowRequest( - null, - template, - new String[] { "off" }, - false, - Collections.emptyMap(), - null, - Collections.emptyMap(), - false - ); + WorkflowRequest workflowRequest = new WorkflowRequest(null, template, new String[] { "off" }, false, Collections.emptyMap(), false); // Bypass checkMaxWorkflows and force onResponse doAnswer(invocation -> { @@ -329,16 +320,7 @@ public void testFailedToCreateNewWorkflow() { public void testCreateNewWorkflow() { @SuppressWarnings("unchecked") ActionListener listener = mock(ActionListener.class); - WorkflowRequest workflowRequest = new WorkflowRequest( - null, - template, - new String[] { "off" }, - false, - Collections.emptyMap(), - null, - Collections.emptyMap(), - false - ); + WorkflowRequest workflowRequest = new WorkflowRequest(null, template, new String[] { "off" }, false, Collections.emptyMap(), false); // Bypass checkMaxWorkflows and force onResponse doAnswer(invocation -> { @@ -402,16 +384,7 @@ public void testCreateWithUserAndFilterOn() { ); ActionListener listener = mock(ActionListener.class); - WorkflowRequest workflowRequest = new WorkflowRequest( - null, - template, - new String[] { "off" }, - false, - Collections.emptyMap(), - null, - Collections.emptyMap(), - false - ); + WorkflowRequest workflowRequest = new WorkflowRequest(null, template, new String[] { "off" }, false, Collections.emptyMap(), false); // Bypass checkMaxWorkflows and force onResponse doAnswer(invocation -> { @@ -475,16 +448,7 @@ public void testFailedToCreateNewWorkflowWithNullUser() { ActionListener listener = mock(ActionListener.class); - WorkflowRequest workflowRequest = new WorkflowRequest( - null, - template, - new String[] { "off" }, - false, - Collections.emptyMap(), - null, - Collections.emptyMap(), - false - ); + WorkflowRequest workflowRequest = new WorkflowRequest(null, template, new String[] { "off" }, false, Collections.emptyMap(), false); createWorkflowTransportAction1.doExecute(mock(Task.class), workflowRequest, listener); ArgumentCaptor exceptionCaptor = ArgumentCaptor.forClass(Exception.class); @@ -519,16 +483,7 @@ public void testFailedToCreateNewWorkflowWithNoBackendRoleUser() { ActionListener listener = mock(ActionListener.class); - WorkflowRequest workflowRequest = new WorkflowRequest( - null, - template, - new String[] { "off" }, - false, - Collections.emptyMap(), - null, - Collections.emptyMap(), - false - ); + WorkflowRequest workflowRequest = new WorkflowRequest(null, template, new String[] { "off" }, false, Collections.emptyMap(), false); createWorkflowTransportAction1.doExecute(mock(Task.class), workflowRequest, listener); ArgumentCaptor exceptionCaptor = ArgumentCaptor.forClass(Exception.class); @@ -542,16 +497,7 @@ public void testFailedToCreateNewWorkflowWithNoBackendRoleUser() { public void testUpdateWorkflowWithReprovision() throws IOException { @SuppressWarnings("unchecked") ActionListener listener = mock(ActionListener.class); - WorkflowRequest workflowRequest = new WorkflowRequest( - "1", - template, - new String[] { "off" }, - false, - Collections.emptyMap(), - null, - Collections.emptyMap(), - true - ); + WorkflowRequest workflowRequest = new WorkflowRequest("1", template, new String[] { "off" }, false, Collections.emptyMap(), true); doAnswer(invocation -> { ActionListener getListener = invocation.getArgument(1); @@ -595,16 +541,7 @@ public void testUpdateWorkflowWithReprovision() throws IOException { public void testFailedToUpdateWorkflowWithReprovision() throws IOException { @SuppressWarnings("unchecked") ActionListener listener = mock(ActionListener.class); - WorkflowRequest workflowRequest = new WorkflowRequest( - "1", - template, - new String[] { "off" }, - false, - Collections.emptyMap(), - null, - Collections.emptyMap(), - true - ); + WorkflowRequest workflowRequest = new WorkflowRequest("1", template, new String[] { "off" }, false, Collections.emptyMap(), true); doAnswer(invocation -> { ActionListener getListener = invocation.getArgument(1); @@ -904,8 +841,6 @@ public void testCreateWorkflow_withValidation_withProvision_Success() throws Exc new String[] { "all" }, true, Collections.emptyMap(), - null, - Collections.emptyMap(), false ); @@ -966,8 +901,6 @@ public void testCreateWorkflow_withValidation_withProvision_FailedProvisioning() new String[] { "all" }, true, Collections.emptyMap(), - null, - Collections.emptyMap(), false ); diff --git a/src/test/java/org/opensearch/flowframework/transport/WorkflowRequestResponseTests.java b/src/test/java/org/opensearch/flowframework/transport/WorkflowRequestResponseTests.java index c06ae2e36..e92255e0f 100644 --- a/src/test/java/org/opensearch/flowframework/transport/WorkflowRequestResponseTests.java +++ b/src/test/java/org/opensearch/flowframework/transport/WorkflowRequestResponseTests.java @@ -153,69 +153,10 @@ public void testWorkflowRequestWithParams() throws IOException { assertEquals("bar", streamInputRequest.getParams().get("foo")); } - public void testWorkflowRequestWithUseCase() throws IOException { - WorkflowRequest workflowRequest = new WorkflowRequest("123", template, "cohere-embedding_model_deploy", Collections.emptyMap()); - assertNotNull(workflowRequest.getWorkflowId()); - assertEquals(template, workflowRequest.getTemplate()); - assertNull(workflowRequest.validate()); - assertFalse(workflowRequest.isProvision()); - assertFalse(workflowRequest.isUpdateFields()); - assertTrue(workflowRequest.getDefaultParams().isEmpty()); - assertEquals(workflowRequest.getUseCase(), "cohere-embedding_model_deploy"); - - BytesStreamOutput out = new BytesStreamOutput(); - workflowRequest.writeTo(out); - BytesStreamInput in = new BytesStreamInput(BytesReference.toBytes(out.bytes())); - - WorkflowRequest streamInputRequest = new WorkflowRequest(in); - - assertEquals(workflowRequest.getWorkflowId(), streamInputRequest.getWorkflowId()); - assertEquals(workflowRequest.getTemplate().toString(), streamInputRequest.getTemplate().toString()); - assertNull(streamInputRequest.validate()); - assertFalse(streamInputRequest.isProvision()); - assertFalse(streamInputRequest.isUpdateFields()); - // THESE TESTS FAIL - // assertTrue(streamInputRequest.getDefaultParams().isEmpty()); - // assertEquals(streamInputRequest.getUseCase(), "cohere-embedding_model_deploy"); - } - - public void testWorkflowRequestWithUseCaseAndParamsInBody() throws IOException { - WorkflowRequest workflowRequest = new WorkflowRequest("123", template, "cohere-embedding_model_deploy", Map.of("step", "model")); - assertNotNull(workflowRequest.getWorkflowId()); - assertEquals(template, workflowRequest.getTemplate()); - assertNull(workflowRequest.validate()); - assertFalse(workflowRequest.isProvision()); - assertFalse(workflowRequest.isUpdateFields()); - assertEquals(workflowRequest.getDefaultParams().get("step"), "model"); - - BytesStreamOutput out = new BytesStreamOutput(); - workflowRequest.writeTo(out); - BytesStreamInput in = new BytesStreamInput(BytesReference.toBytes(out.bytes())); - - WorkflowRequest streamInputRequest = new WorkflowRequest(in); - - assertEquals(workflowRequest.getWorkflowId(), streamInputRequest.getWorkflowId()); - assertEquals(workflowRequest.getTemplate().toString(), streamInputRequest.getTemplate().toString()); - assertNull(streamInputRequest.validate()); - assertFalse(streamInputRequest.isProvision()); - assertFalse(streamInputRequest.isUpdateFields()); - // THIS TEST FAILS - // assertEquals(streamInputRequest.getDefaultParams().get("step"), "model"); - } - public void testWorkflowRequestWithParamsNoProvision() throws IOException { IllegalArgumentException ex = assertThrows( IllegalArgumentException.class, - () -> new WorkflowRequest( - "123", - template, - new String[] { "all" }, - false, - Map.of("foo", "bar"), - null, - Collections.emptyMap(), - false - ) + () -> new WorkflowRequest("123", template, new String[] { "all" }, false, Map.of("foo", "bar"), false) ); assertEquals("Params may only be included when provisioning.", ex.getMessage()); } @@ -227,8 +168,6 @@ public void testWorkflowRequestWithOnlyUpdateParamNoProvision() throws IOExcepti new String[] { "all" }, true, Map.of(UPDATE_WORKFLOW_FIELDS, "true"), - null, - Collections.emptyMap(), false ); assertNotNull(workflowRequest.getWorkflowId());