Remove useCase and defaultParams field in WorkflowRequest (#952)

* Remove useCase and defaultParams field in WorkflowRequest Signed-off-by: Junwei Dai <[email protected]> * Remove useCase and defaultParams field in WorkflowRequest Signed-off-by: Junwei Dai <[email protected]> * Removed the constructor that has never been used and tested. Signed-off-by: Junwei Dai <[email protected]> --------- Signed-off-by: Junwei Dai <[email protected]> Co-authored-by: Junwei Dai <[email protected]>
opensearch-project · Nov 11, 2024 · 219aec5 · 219aec5
1 parent 9b9ebb2
commit 219aec5
Show file tree

Hide file tree

Showing 5 changed files with 13 additions and 194 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -18,6 +18,8 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
 ### Features
 ### Enhancements
 ### Bug Fixes
+- Remove useCase and defaultParams field in WorkflowRequest ([#758](https://github.com/opensearch-project/flow-framework/pull/758))
+
 ### Infrastructure
 ### Documentation
 ### Maintenance

diff --git 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
@@ -62,23 +62,13 @@ public class WorkflowRequest extends ActionRequest {
      */
     private Map<String, String> params;
 
-    /**
-     * use case flag
-     */
-    private String useCase;
-
-    /**
-     * Deafult params map from use case
-     */
-    private Map<String, String> 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<String, String> 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<String, String> 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<String, String> params,
-        String useCase,
-        Map<String, String> 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<String, String> 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<String, String> 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
@@ -252,7 +252,7 @@ public void testMaxWorkflow() {
 
         @SuppressWarnings("unchecked")
         ActionListener<WorkflowResponse> 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<SearchResponse> searchListener = invocation.getArgument(1);
@@ -289,16 +289,7 @@ public void onFailure(Exception e) {
     public void testFailedToCreateNewWorkflow() {
         @SuppressWarnings("unchecked")
         ActionListener<WorkflowResponse> 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<WorkflowResponse> 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<WorkflowResponse> 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<WorkflowResponse> 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<Exception> exceptionCaptor = ArgumentCaptor.forClass(Exception.class);
@@ -519,16 +483,7 @@ public void testFailedToCreateNewWorkflowWithNoBackendRoleUser() {
 
         ActionListener<WorkflowResponse> 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<Exception> exceptionCaptor = ArgumentCaptor.forClass(Exception.class);
@@ -542,16 +497,7 @@ public void testFailedToCreateNewWorkflowWithNoBackendRoleUser() {
     public void testUpdateWorkflowWithReprovision() throws IOException {
         @SuppressWarnings("unchecked")
         ActionListener<WorkflowResponse> 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<GetResponse> getListener = invocation.getArgument(1);
@@ -595,16 +541,7 @@ public void testUpdateWorkflowWithReprovision() throws IOException {
     public void testFailedToUpdateWorkflowWithReprovision() throws IOException {
         @SuppressWarnings("unchecked")
         ActionListener<WorkflowResponse> 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<GetResponse> 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
@@ -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());