[Enhancement] Enhance validation for create connector API

This PR addresses the first part of this enhancement "Validate if connector payload has all the required fields. If not provided, throw the illegal argument exception". Validation of fields description, parameters, credential, and request_body are missing. That validations are added in this fix. Added new test cases correspong to these validations and fixed all failing test cases because of these new validations. Partially Resolves #1382 Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>
opensearch-project · Dec 6, 2024 · db98d71 · db98d71
1 parent 1d30671
commit db98d71
Show file tree

Hide file tree

Showing 7 changed files with 148 additions and 11 deletions.
diff --git a/common/src/main/java/org/opensearch/ml/common/connector/ConnectorAction.java b/common/src/main/java/org/opensearch/ml/common/connector/ConnectorAction.java
@@ -63,6 +63,9 @@ public ConnectorAction(
         if (method == null) {
             throw new IllegalArgumentException("method can't null");
         }
+        if (requestBody == null) {
+            throw new IllegalArgumentException("request body can't null");
+        }
         this.actionType = actionType;
         this.method = method;
         this.url = url;

diff --git a/...on/src/main/java/org/opensearch/ml/common/transport/connector/MLCreateConnectorInput.java b/...on/src/main/java/org/opensearch/ml/common/transport/connector/MLCreateConnectorInput.java
@@ -93,6 +93,15 @@ public MLCreateConnectorInput(
             if (protocol == null) {
                 throw new IllegalArgumentException("Connector protocol is null");
             }
+            if (description == null) {
+                throw new IllegalArgumentException("Connector description is null");
+            }
+            if (parameters == null || parameters.isEmpty()) {
+                throw new IllegalArgumentException("Connector parameters is null or empty list");
+            }
+            if (credential == null || credential.isEmpty()) {
+                throw new IllegalArgumentException("Connector credential is null or empty list");
+            }
         }
         this.name = name;
         this.description = description;

diff --git a/common/src/test/java/org/opensearch/ml/common/connector/ConnectorActionTest.java b/common/src/test/java/org/opensearch/ml/common/connector/ConnectorActionTest.java
@@ -37,7 +37,8 @@ public void constructor_NullActionType() {
         ConnectorAction.ActionType actionType = null;
         String method = "post";
         String url = "https://test.com";
-        new ConnectorAction(actionType, method, url, null, null, null, null);
+        String requestBody = "{\"input\": \"${parameters.input}\"}";
+        new ConnectorAction(actionType, method, url, null, requestBody, null, null);
     }
 
     @Test
@@ -47,7 +48,8 @@ public void constructor_NullUrl() {
         ConnectorAction.ActionType actionType = ConnectorAction.ActionType.PREDICT;
         String method = "post";
         String url = null;
-        new ConnectorAction(actionType, method, url, null, null, null, null);
+        String requestBody = "{\"input\": \"${parameters.input}\"}";
+        new ConnectorAction(actionType, method, url, null, requestBody, null, null);
     }
 
     @Test
@@ -57,15 +59,28 @@ public void constructor_NullMethod() {
         ConnectorAction.ActionType actionType = ConnectorAction.ActionType.PREDICT;
         String method = null;
         String url = "https://test.com";
-        new ConnectorAction(actionType, method, url, null, null, null, null);
+        String requestBody = "{\"input\": \"${parameters.input}\"}";
+        new ConnectorAction(actionType, method, url, null, requestBody, null, null);
+    }
+
+    @Test
+    public void constructor_NullRequestBody() {
+        exceptionRule.expect(IllegalArgumentException.class);
+        exceptionRule.expectMessage("request body can't null");
+        ConnectorAction.ActionType actionType = ConnectorAction.ActionType.PREDICT;
+        String method = "post";
+        String url = "https://test.com";
+        String requestBody = null;
+        new ConnectorAction(actionType, method, url, null, requestBody, null, null);
     }
 
     @Test
     public void writeTo_NullValue() throws IOException {
         ConnectorAction.ActionType actionType = ConnectorAction.ActionType.PREDICT;
         String method = "http";
         String url = "https://test.com";
-        ConnectorAction action = new ConnectorAction(actionType, method, url, null, null, null, null);
+        String requestBody = "{\"input\": \"${parameters.input}\"}";
+        ConnectorAction action = new ConnectorAction(actionType, method, url, null, requestBody, null, null);
         BytesStreamOutput output = new BytesStreamOutput();
         action.writeTo(output);
         ConnectorAction action2 = new ConnectorAction(output.bytes().streamInput());
@@ -103,12 +118,18 @@ public void toXContent_NullValue() throws IOException {
         ConnectorAction.ActionType actionType = ConnectorAction.ActionType.PREDICT;
         String method = "http";
         String url = "https://test.com";
-        ConnectorAction action = new ConnectorAction(actionType, method, url, null, null, null, null);
+        String requestBody = "{\"input\": \"${parameters.input}\"}";
+        ConnectorAction action = new ConnectorAction(actionType, method, url, null, requestBody, null, null);
 
         XContentBuilder builder = XContentBuilder.builder(XContentType.JSON.xContent());
         action.toXContent(builder, ToXContent.EMPTY_PARAMS);
         String content = TestHelper.xContentBuilderToString(builder);
-        Assert.assertEquals("{\"action_type\":\"PREDICT\",\"method\":\"http\",\"url\":\"https://test.com\"}", content);
+        Assert
+            .assertEquals(
+                "{\"action_type\":\"PREDICT\",\"method\":\"http\",\"url\":\"https://test.com\","
+                    + "\"request_body\":\"{\\\"input\\\": \\\"${parameters.input}\\\"}\"}",
+                content
+            );
     }
 
     @Test

diff --git a/...c/test/java/org/opensearch/ml/common/transport/connector/MLCreateConnectorInputTests.java b/...c/test/java/org/opensearch/ml/common/transport/connector/MLCreateConnectorInputTests.java
@@ -157,6 +157,101 @@ public void constructorMLCreateConnectorInput_NullProtocol() {
             .build();
     }
 
+    @Test
+    public void constructorMLCreateConnectorInput_NullDescription() {
+        exceptionRule.expect(IllegalArgumentException.class);
+        exceptionRule.expectMessage("Connector description is null");
+        MLCreateConnectorInput
+            .builder()
+            .name("test_connector_name")
+            .description(null)
+            .version("1")
+            .protocol("http")
+            .parameters(Map.of("input", "test input value"))
+            .credential(Map.of("key", "test_key_value"))
+            .actions(List.of())
+            .access(AccessMode.PUBLIC)
+            .backendRoles(Arrays.asList("role1", "role2"))
+            .addAllBackendRoles(false)
+            .build();
+    }
+
+    @Test
+    public void constructorMLCreateConnectorInput_NullParameters() {
+        exceptionRule.expect(IllegalArgumentException.class);
+        exceptionRule.expectMessage("Connector parameters is null or empty list");
+        MLCreateConnectorInput
+            .builder()
+            .name("test_connector_name")
+            .description("this is a test connector")
+            .version("1")
+            .protocol("http")
+            .parameters(null)
+            .credential(Map.of("key", "test_key_value"))
+            .actions(List.of())
+            .access(AccessMode.PUBLIC)
+            .backendRoles(Arrays.asList("role1", "role2"))
+            .addAllBackendRoles(false)
+            .build();
+    }
+
+    @Test
+    public void constructorMLCreateConnectorInput_EmptyParameters() {
+        exceptionRule.expect(IllegalArgumentException.class);
+        exceptionRule.expectMessage("Connector parameters is null or empty list");
+        MLCreateConnectorInput
+            .builder()
+            .name("test_connector_name")
+            .description("this is a test connector")
+            .version("1")
+            .protocol("http")
+            .parameters(Map.of())
+            .credential(Map.of("key", "test_key_value"))
+            .actions(List.of())
+            .access(AccessMode.PUBLIC)
+            .backendRoles(Arrays.asList("role1", "role2"))
+            .addAllBackendRoles(false)
+            .build();
+    }
+
+    @Test
+    public void constructorMLCreateConnectorInput_NullCredential() {
+        exceptionRule.expect(IllegalArgumentException.class);
+        exceptionRule.expectMessage("Connector credential is null or empty list");
+        MLCreateConnectorInput
+            .builder()
+            .name("test_connector_name")
+            .description("this is a test connector")
+            .version("1")
+            .protocol("http")
+            .parameters(Map.of("input", "test input value"))
+            .credential(null)
+            .actions(List.of())
+            .access(AccessMode.PUBLIC)
+            .backendRoles(Arrays.asList("role1", "role2"))
+            .addAllBackendRoles(false)
+            .build();
+    }
+
+    @Test
+    public void constructorMLCreateConnectorInput_EmptyCredential() {
+        exceptionRule.expect(IllegalArgumentException.class);
+        exceptionRule.expectMessage("Connector credential is null or empty list");
+        MLCreateConnectorInput
+            .builder()
+            .name("test_connector_name")
+            .description("this is a test connector")
+            .version("1")
+            .protocol("http")
+            .parameters(Map.of("input", "test input value"))
+            .credential(Map.of())
+            .actions(List.of())
+            .access(AccessMode.PUBLIC)
+            .backendRoles(Arrays.asList("role1", "role2"))
+            .addAllBackendRoles(false)
+            .build();
+    }
+
     @Test
     public void testToXContent_FullFields() throws Exception {
         XContentBuilder builder = XContentFactory.jsonBuilder();
@@ -223,8 +318,11 @@ public void readInputStream_SuccessWithNullFields() throws IOException {
         MLCreateConnectorInput mlCreateMinimalConnectorInput = MLCreateConnectorInput
             .builder()
             .name("test_connector_name")
+            .description("this is a test connector")
             .version("1")
             .protocol("http")
+            .parameters(Map.of("input", "test input value"))
+            .credential(Map.of("key", "test_key_value"))
             .build();
         readInputStream(mlCreateMinimalConnectorInput, parsedInput -> {
             assertEquals(mlCreateMinimalConnectorInput.getName(), parsedInput.getName());

diff --git a/ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/ConnectorUtils.java b/ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/ConnectorUtils.java
@@ -336,7 +336,7 @@ public static ConnectorAction createConnectorAction(Connector connector, Connect
 
         // Initialize the default method and requestBody
         String method = "POST";
-        String requestBody = null;
+        String requestBody = "{}";
         String url = "";
 
         switch (getRemoteServerFromURL(predictEndpoint)) {

diff --git a/...gorithms/src/test/java/org/opensearch/ml/engine/algorithms/remote/ConnectorUtilsTest.java b/...gorithms/src/test/java/org/opensearch/ml/engine/algorithms/remote/ConnectorUtilsTest.java
@@ -344,7 +344,7 @@ public void testGetTask_createBatchStatusActionForOpenAI() {
         assertEquals(ConnectorAction.ActionType.BATCH_PREDICT_STATUS, result.getActionType());
         assertEquals("GET", result.getMethod());
         assertEquals("https://api.openai.com/v1/batches/${parameters.id}", result.getUrl());
-        assertNull(result.getRequestBody());
+        assertEquals("{}", result.getRequestBody());
         assertTrue(result.getHeaders().containsKey("Authorization"));
     }
 
@@ -355,6 +355,7 @@ public void testGetTask_createCancelBatchActionForBedrock() {
             .name("test")
             .protocol("http")
             .version("1")
+            .description("this is a test connector")
             .credential(Map.of("api_key", "credential_value"))
             .parameters(Map.of("param1", "value1"))
             .actions(
@@ -384,6 +385,6 @@ public void testGetTask_createCancelBatchActionForBedrock() {
             "https://bedrock.${parameters.region}.amazonaws.com/model-invocation-job/${parameters.processedJobArn}/stop",
             result.getUrl()
         );
-        assertNull(result.getRequestBody());
+        assertEquals("{}", result.getRequestBody());
     }
 }
diff --git a/...src/test/java/org/opensearch/ml/action/connector/TransportCreateConnectorActionTests.java b/...src/test/java/org/opensearch/ml/action/connector/TransportCreateConnectorActionTests.java
@@ -133,6 +133,7 @@ public void setup() {
                     .builder()
                     .actionType(ConnectorAction.ActionType.PREDICT)
                     .method("POST")
+                    .requestBody("{ \"inputText\": \"${parameters.inputText}\" }")
                     .url("https://${parameters.endpoint}/v1/completions")
                     .build()
             );
@@ -142,6 +143,7 @@ public void setup() {
         input = MLCreateConnectorInput
             .builder()
             .name("test_name")
+            .description("this is a test connector")
             .version("1")
             .actions(actions)
             .parameters(parameters)
@@ -447,21 +449,24 @@ public void test_execute_URL_notMatchingExpression_exception() {
                     .actionType(ConnectorAction.ActionType.PREDICT)
                     .method("POST")
                     .url("https://${parameters.endpoint}/v1/completions")
+                    .requestBody("{ \"inputText\": \"${parameters.inputText}\" }")
                     .build()
             );
 
+        Map<String, String> parameters = ImmutableMap.of("endpoint", "api.openai1.com");
+        Map<String, String> credential = ImmutableMap.of("access_key", "mockKey", "secret_key", "mockSecret");
         MLCreateConnectorInput mlCreateConnectorInput = MLCreateConnectorInput
             .builder()
             .name(randomAlphaOfLength(5))
             .description(randomAlphaOfLength(10))
             .version("1")
             .protocol(ConnectorProtocols.HTTP)
+            .parameters(parameters)
+            .credential(credential)
             .actions(actions)
             .build();
         MLCreateConnectorRequest request = new MLCreateConnectorRequest(mlCreateConnectorInput);
 
-        Map<String, String> parameters = ImmutableMap.of("endpoint", "api.openai1.com");
-        mlCreateConnectorInput.setParameters(parameters);
         TransportCreateConnectorAction action = new TransportCreateConnectorAction(
             transportService,
             actionFilters,