Addressing PR comments (Part 1). Moving common vlaues to CommonValue …

…class Signed-off-by: Joshua Palis <[email protected]>
opensearch-project · Oct 9, 2023 · 791f943 · 791f943
1 parent 527ddd6
commit 791f943
Show file tree

Hide file tree

Showing 9 changed files with 38 additions and 50 deletions.
diff --git a/src/main/java/org/opensearch/flowframework/FlowFrameworkPlugin.java b/src/main/java/org/opensearch/flowframework/FlowFrameworkPlugin.java
@@ -46,29 +46,14 @@
 import java.util.List;
 import java.util.function.Supplier;
 
+import static org.opensearch.flowframework.common.CommonValue.FLOW_FRAMEWORK_THREAD_POOL_PREFIX;
+import static org.opensearch.flowframework.common.CommonValue.PROVISION_THREAD_POOL;
+
 /**
  * An OpenSearch plugin that enables builders to innovate AI apps on OpenSearch.
  */
 public class FlowFrameworkPlugin extends Plugin implements ActionPlugin {
 
-    // TODO : Move names to common values class
-    /**
-     * The base URI for this plugin's rest actions
-     */
-    public static final String AI_FLOW_FRAMEWORK_BASE_URI = "/_plugins/_flow_framework";
-    /**
-     * The URI for this plugin's workflow rest actions
-     */
-    public static final String WORKFLOWS_URI = AI_FLOW_FRAMEWORK_BASE_URI + "/workflows";
-    /**
-     * Flow Framework plugin thread pool name prefix
-     */
-    public static final String FLOW_FRAMEWORK_THREAD_POOL_PREFIX = "thread_pool.flow_framework.";
-    /**
-     * The provision workflow thread pool name
-     */
-    public static final String PROVISION_THREAD_POOL = "opensearch_workflow_provision";
-
     /**
      * Instantiate this plugin.
      */

diff --git a/src/main/java/org/opensearch/flowframework/common/CommonValue.java b/src/main/java/org/opensearch/flowframework/common/CommonValue.java
@@ -19,4 +19,15 @@ public class CommonValue {
     public static final String GLOBAL_CONTEXT_INDEX = ".plugins-ai-global-context";
     public static final String GLOBAL_CONTEXT_INDEX_MAPPING = "mappings/global-context.json";
     public static final Integer GLOBAL_CONTEXT_INDEX_VERSION = 1;
+
+    /** The base URI for this plugin's rest actions */
+    public static final String AI_FLOW_FRAMEWORK_BASE_URI = "/_plugins/_flow_framework";
+    /** The URI for this plugin's workflow rest actions */
+    public static final String WORKFLOWS_URI = AI_FLOW_FRAMEWORK_BASE_URI + "/workflows";
+    /** Flow Framework plugin thread pool name prefix */
+    public static final String FLOW_FRAMEWORK_THREAD_POOL_PREFIX = "thread_pool.flow_framework.";
+    /** The provision workflow thread pool name */
+    public static final String PROVISION_THREAD_POOL = "opensearch_workflow_provision";
+    /** Field name for workflow Id, the document Id of the indexed use case template */
+    public static final String WORKFLOW_ID = "workflow_id";
 }
diff --git a/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java
@@ -10,7 +10,6 @@
 
 import com.google.common.collect.ImmutableList;
 import org.opensearch.client.node.NodeClient;
-import org.opensearch.flowframework.FlowFrameworkPlugin;
 import org.opensearch.flowframework.model.Template;
 import org.opensearch.flowframework.transport.CreateWorkflowAction;
 import org.opensearch.flowframework.transport.WorkflowRequest;
@@ -22,19 +21,16 @@
 import java.util.List;
 import java.util.Locale;
 
+import static org.opensearch.flowframework.common.CommonValue.WORKFLOWS_URI;
+import static org.opensearch.flowframework.common.CommonValue.WORKFLOW_ID;
+
 /**
  * Rest Action to facilitate requests to create and update a use case template
  */
 public class RestCreateWorkflowAction extends BaseRestHandler {
 
     private static final String CREATE_WORKFLOW_ACTION = "create_workflow_action";
 
-    // TODO : move to common values class, pending implementation
-    /**
-     * Field name for workflow Id, the document Id of the indexed use case template
-     */
-    public static final String WORKFLOW_ID = "workflow_id";
-
     /**
      * Intantiates a new RestCreateWorkflowAction
      */
@@ -51,9 +47,9 @@ public String getName() {
     public List<Route> routes() {
         return ImmutableList.of(
             // Create new workflow
-            new Route(RestRequest.Method.POST, String.format(Locale.ROOT, "%s", FlowFrameworkPlugin.WORKFLOWS_URI)),
-            // Update workflow
-            new Route(RestRequest.Method.PUT, String.format(Locale.ROOT, "%s/{%s}", FlowFrameworkPlugin.WORKFLOWS_URI, WORKFLOW_ID))
+            new Route(RestRequest.Method.POST, String.format(Locale.ROOT, "%s", WORKFLOWS_URI)),
+            // Update use case template
+            new Route(RestRequest.Method.PUT, String.format(Locale.ROOT, "%s/{%s}", WORKFLOWS_URI, WORKFLOW_ID))
         );
     }
 

diff --git a/src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java
@@ -10,7 +10,6 @@
 
 import com.google.common.collect.ImmutableList;
 import org.opensearch.client.node.NodeClient;
-import org.opensearch.flowframework.FlowFrameworkPlugin;
 import org.opensearch.flowframework.model.Template;
 import org.opensearch.flowframework.transport.ProvisionWorkflowAction;
 import org.opensearch.flowframework.transport.WorkflowRequest;
@@ -22,6 +21,9 @@
 import java.util.List;
 import java.util.Locale;
 
+import static org.opensearch.flowframework.common.CommonValue.WORKFLOWS_URI;
+import static org.opensearch.flowframework.common.CommonValue.WORKFLOW_ID;
+
 /**
  * Rest action to facilitate requests to provision a workflow from an inline defined or stored use case template
  */
@@ -51,12 +53,9 @@ public String getName() {
     public List<Route> routes() {
         return ImmutableList.of(
             // Provision workflow from inline use case template
-            new Route(RestRequest.Method.POST, String.format(Locale.ROOT, "%s/%s", FlowFrameworkPlugin.WORKFLOWS_URI, "_provision")),
+            new Route(RestRequest.Method.POST, String.format(Locale.ROOT, "%s/%s", WORKFLOWS_URI, "_provision")),
             // Provision workflow from indexed use case template
-            new Route(
-                RestRequest.Method.POST,
-                String.format(Locale.ROOT, "%s/{%s}/%s", FlowFrameworkPlugin.WORKFLOWS_URI, WORKFLOW_ID, "_provision")
-            )
+            new Route(RestRequest.Method.POST, String.format(Locale.ROOT, "%s/{%s}/%s", WORKFLOWS_URI, WORKFLOW_ID, "_provision"))
         );
     }
 

diff --git a/src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java b/src/main/java/org/opensearch/flowframework/transport/ProvisionWorkflowTransportAction.java
@@ -31,7 +31,7 @@
 import java.util.concurrent.CompletionException;
 import java.util.stream.Collectors;
 
-import static org.opensearch.flowframework.FlowFrameworkPlugin.PROVISION_THREAD_POOL;
+import static org.opensearch.flowframework.common.CommonValue.PROVISION_THREAD_POOL;
 
 /**
  * Transport Action to provision a workflow from a stored use case template

diff --git a/src/main/java/org/opensearch/flowframework/transport/WorkflowResponse.java b/src/main/java/org/opensearch/flowframework/transport/WorkflowResponse.java
@@ -13,10 +13,11 @@
 import org.opensearch.core.common.io.stream.StreamOutput;
 import org.opensearch.core.xcontent.ToXContentObject;
 import org.opensearch.core.xcontent.XContentBuilder;
-import org.opensearch.flowframework.rest.RestCreateWorkflowAction;
 
 import java.io.IOException;
 
+import static org.opensearch.flowframework.common.CommonValue.WORKFLOW_ID;
+
 /**
  * Transport Response from creating or provisioning a workflow
  */
@@ -61,7 +62,7 @@ public void writeTo(StreamOutput out) throws IOException {
     // TODO : Replace WORKFLOW_ID after string is moved to common values class
     @Override
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
-        return builder.startObject().field(RestCreateWorkflowAction.WORKFLOW_ID, this.workflowId).endObject();
+        return builder.startObject().field(WORKFLOW_ID, this.workflowId).endObject();
     }
 
 }
diff --git a/src/test/java/org/opensearch/flowframework/rest/RestCreateWorkflowActionTests.java b/src/test/java/org/opensearch/flowframework/rest/RestCreateWorkflowActionTests.java
@@ -11,7 +11,6 @@
 import org.opensearch.client.node.NodeClient;
 import org.opensearch.core.common.bytes.BytesArray;
 import org.opensearch.core.xcontent.MediaTypeRegistry;
-import org.opensearch.flowframework.FlowFrameworkPlugin;
 import org.opensearch.rest.RestHandler.Route;
 import org.opensearch.rest.RestRequest;
 import org.opensearch.test.OpenSearchTestCase;
@@ -21,6 +20,7 @@
 import java.util.List;
 import java.util.Locale;
 
+import static org.opensearch.flowframework.common.CommonValue.WORKFLOWS_URI;
 import static org.mockito.Mockito.mock;
 
 public class RestCreateWorkflowActionTests extends OpenSearchTestCase {
@@ -42,8 +42,8 @@ public void setUp() throws Exception {
             + "\"user_inputs\":{\"userKey\":\"userValue\",\"userMapKey\":{\"nestedKey\":\"nestedValue\"}},"
             + "\"workflows\":{\"workflow\":{\"user_params\":{\"key\":\"value\"},\"nodes\":[{\"id\":\"A\",\"type\":\"a-type\",\"inputs\":{\"foo\":\"bar\"}},{\"id\":\"B\",\"type\":\"b-type\",\"inputs\":{\"baz\":\"qux\"}}],\"edges\":[{\"source\":\"A\",\"dest\":\"B\"}]}}}";
         this.createWorkflowRestAction = new RestCreateWorkflowAction();
-        this.createWorkflowPath = String.format(Locale.ROOT, "%s", FlowFrameworkPlugin.WORKFLOWS_URI);
-        this.updateWorkflowPath = String.format(Locale.ROOT, "%s/{%s}", FlowFrameworkPlugin.WORKFLOWS_URI, "workflow_id");
+        this.createWorkflowPath = String.format(Locale.ROOT, "%s", WORKFLOWS_URI);
+        this.updateWorkflowPath = String.format(Locale.ROOT, "%s/{%s}", WORKFLOWS_URI, "workflow_id");
         this.nodeClient = mock(NodeClient.class);
     }
 

diff --git a/src/test/java/org/opensearch/flowframework/rest/RestProvisionWorkflowActionTests.java b/src/test/java/org/opensearch/flowframework/rest/RestProvisionWorkflowActionTests.java
@@ -11,7 +11,6 @@
 import org.opensearch.client.node.NodeClient;
 import org.opensearch.core.common.bytes.BytesArray;
 import org.opensearch.core.xcontent.MediaTypeRegistry;
-import org.opensearch.flowframework.FlowFrameworkPlugin;
 import org.opensearch.rest.RestHandler.Route;
 import org.opensearch.rest.RestRequest;
 import org.opensearch.test.OpenSearchTestCase;
@@ -21,6 +20,7 @@
 import java.util.List;
 import java.util.Locale;
 
+import static org.opensearch.flowframework.common.CommonValue.WORKFLOWS_URI;
 import static org.mockito.Mockito.mock;
 
 public class RestProvisionWorkflowActionTests extends OpenSearchTestCase {
@@ -42,14 +42,8 @@ public void setUp() throws Exception {
             + "\"user_inputs\":{\"userKey\":\"userValue\",\"userMapKey\":{\"nestedKey\":\"nestedValue\"}},"
             + "\"workflows\":{\"workflow\":{\"user_params\":{\"key\":\"value\"},\"nodes\":[{\"id\":\"A\",\"type\":\"a-type\",\"inputs\":{\"foo\":\"bar\"}},{\"id\":\"B\",\"type\":\"b-type\",\"inputs\":{\"baz\":\"qux\"}}],\"edges\":[{\"source\":\"A\",\"dest\":\"B\"}]}}}";
         this.provisionWorkflowRestAction = new RestProvisionWorkflowAction();
-        this.provisionInlineWorkflowPath = String.format(Locale.ROOT, "%s/%s", FlowFrameworkPlugin.WORKFLOWS_URI, "_provision");
-        this.provisionSavedWorkflowPath = String.format(
-            Locale.ROOT,
-            "%s/{%s}/%s",
-            FlowFrameworkPlugin.WORKFLOWS_URI,
-            "workflow_id",
-            "_provision"
-        );
+        this.provisionInlineWorkflowPath = String.format(Locale.ROOT, "%s/%s", WORKFLOWS_URI, "_provision");
+        this.provisionSavedWorkflowPath = String.format(Locale.ROOT, "%s/{%s}/%s", WORKFLOWS_URI, "workflow_id", "_provision");
         this.nodeClient = mock(NodeClient.class);
     }
 

diff --git a/src/test/java/org/opensearch/flowframework/transport/WorkflowRequestResponseTests.java b/src/test/java/org/opensearch/flowframework/transport/WorkflowRequestResponseTests.java
@@ -51,7 +51,9 @@ public void setUp() throws Exception {
             templateVersion,
             compatibilityVersions,
             Map.ofEntries(Map.entry("userKey", "userValue"), Map.entry("userMapKey", Map.of("nestedKey", "nestedValue"))),
-            Map.of("workflow", workflow)
+            Map.of("workflow", workflow),
+            Map.of("outputKey", "outputValue"),
+            Map.of("resourceKey", "resourceValue")
         );
     }