Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Enhancement] Enhance validation for create connector API #3579

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+391 −13
Open

[Enhancement] Enhance validation for create connector API #3579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7df638e
[Enhancement] Enhance validation for create connector API
akolarkunnu Mar 11, 2025
236fda2
[Enhancement] Enhance validation for create connector API
akolarkunnu Mar 12, 2025
953f16e
Merge branch 'opensearch-project:main' into prepostfuncvalidation
akolarkunnu Mar 13, 2025
4e5455c
[Enhancement] Enhance validation for create connector API
akolarkunnu Mar 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 97 additions & 0 deletions common/src/main/java/org/opensearch/ml/common/connector/ConnectorAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,19 @@
package org.opensearch.ml.common.connector;

import static org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken;
import static org.opensearch.ml.common.connector.MLPostProcessFunction.DEFAULT_EMBEDDING;
import static org.opensearch.ml.common.connector.MLPostProcessFunction.DEFAULT_RERANK;
import static org.opensearch.ml.common.connector.MLPreProcessFunction.TEXT_DOCS_TO_DEFAULT_EMBEDDING_INPUT;
import static org.opensearch.ml.common.connector.MLPreProcessFunction.TEXT_SIMILARITY_TO_DEFAULT_INPUT;

import java.io.IOException;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;

import org.apache.commons.text.StringSubstitutor;
import org.opensearch.core.common.io.stream.StreamInput;
import org.opensearch.core.common.io.stream.StreamOutput;
import org.opensearch.core.common.io.stream.Writeable;
Expand All @@ -35,6 +41,13 @@ public class ConnectorAction implements ToXContentObject, Writeable {
public static final String REQUEST_BODY_FIELD = "request_body";
public static final String ACTION_PRE_PROCESS_FUNCTION = "pre_process_function";
public static final String ACTION_POST_PROCESS_FUNCTION = "post_process_function";
public static final String OPENAI = "openai";
public static final String COHERE = "cohere";
public static final String BEDROCK = "bedrock";
public static final String SAGEMAKER = "sagemaker";
public static final List<String> SUPPORTED_REMOTE_SERVERS_FOR_DEFAULT_ACTION_TYPES = List.of(SAGEMAKER, OPENAI, BEDROCK, COHERE);

private static final String INBUILT_FUNC_PREFIX = "connector.";

private ActionType actionType;
private String method;
Expand Down Expand Up @@ -185,6 +198,90 @@ public static ConnectorAction parse(XContentParser parser) throws IOException {
.build();
}

public void validatePrePostProcessFunctions(Map<String, String> parameters) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please add java doc with more detailed explanations.

StringSubstitutor substitutor = new StringSubstitutor(parameters, "${parameters.", "}");
String endPoint = substitutor.replace(url);
String remoteServer = getRemoteServerFromURL(endPoint);
validatePreProcessFunctions(remoteServer);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (isInBuiltProcessFunction(preProcessFunction)) then we can invoke validatePreProcessFunctions?

validatePostProcessFunctions(remoteServer);
}

private void validatePreProcessFunctions(String remoteServer) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's organize the private methods in the order it was being invoked.getRemoteServerFromURL and then validatePreProcessFunctions. Its easy to read for reviewers.

if (!isInBuiltProcessFunction(preProcessFunction)) {
return;
}
switch (remoteServer) {
case OPENAI:
if (!preProcessFunction.contains(OPENAI)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit confused with the latest change, so now the only validation is if the preProcessFunction contains "openai" as opposed to "connector.post_process.default.embedding"? so this in theory a more lenient validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I did that change based on your previous comment - "thinking if for these constants we can create the string using the INBUILT_FUNC_PREFIX? and maybe even have preprocessprefix and postprocessprefix? this can avoid any accidental changes "
I can see only that's the only(eg: openai, bedrock, cohere) unique text in the function names which we can defer for different llm services.
Or you meant something differently ? Creating different constant arrays for each llm service post and pre Process Functions ? So in this case we will have 8 arrays, 4 (openai, bedrock, cohere, sagemaker) for pre process functions and 4 for post process functions ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there has been a confusion: #3579 (comment)

In this comment, I meant that we should create the constants as such:
MLPostProcessFunction.OPENAI_EMBEDDING = INBUILT_FUNC_PREFIX + "openai.embedding" + ACTION_POST_PROCESS_FUNCTION*

*just an example may not be fully code accurate

This way if either of these constants change, we don't have to change the code. With respect to whether the check should be lenient or not we can wait on more comments.

throw new IllegalArgumentException(invalidProcessFuncExcText(OPENAI, "PreProcessFunction"));
}
break;
case COHERE:
if (!preProcessFunction.contains(COHERE)) {
throw new IllegalArgumentException(invalidProcessFuncExcText(COHERE, "PreProcessFunction"));
}
break;
case BEDROCK:
if (!preProcessFunction.contains(BEDROCK)) {
throw new IllegalArgumentException(invalidProcessFuncExcText(BEDROCK, "PreProcessFunction"));
}
break;
case SAGEMAKER:
if (!(TEXT_DOCS_TO_DEFAULT_EMBEDDING_INPUT.equals(preProcessFunction)
|| TEXT_SIMILARITY_TO_DEFAULT_INPUT.equals(preProcessFunction))) {
throw new IllegalArgumentException(
"LLM service is "
+ SAGEMAKER
+ ", so PreProcessFunction should be "
+ TEXT_DOCS_TO_DEFAULT_EMBEDDING_INPUT
+ " or "
+ TEXT_SIMILARITY_TO_DEFAULT_INPUT
);
}
}
}

private void validatePostProcessFunctions(String remoteServer) {
if (!isInBuiltProcessFunction(postProcessFunction)) {
return;
}
switch (remoteServer) {
case OPENAI:
if (!postProcessFunction.contains(OPENAI)) {
throw new IllegalArgumentException(invalidProcessFuncExcText(OPENAI, "PostProcessFunction"));
}
break;
case COHERE:
if (!postProcessFunction.contains(COHERE)) {
throw new IllegalArgumentException(invalidProcessFuncExcText(COHERE, "PostProcessFunction"));
}
break;
case BEDROCK:
if (!postProcessFunction.contains(BEDROCK)) {
throw new IllegalArgumentException(invalidProcessFuncExcText(BEDROCK, "PostProcessFunction"));
}
break;
case SAGEMAKER:
if (!(DEFAULT_EMBEDDING.equals(postProcessFunction) || DEFAULT_RERANK.equals(postProcessFunction))) {
throw new IllegalArgumentException(
"LLM service is " + SAGEMAKER + ", so PostProcessFunction should be " + DEFAULT_EMBEDDING + " or " + DEFAULT_RERANK
);
}
}
}

private String invalidProcessFuncExcText(String remoteServer, String func) {
return "LLM service is " + remoteServer + ", so " + func + " should be " + remoteServer + " " + func;
}

private boolean isInBuiltProcessFunction(String processFunction) {
return (processFunction != null && processFunction.startsWith(INBUILT_FUNC_PREFIX));
}

public static String getRemoteServerFromURL(String url) {
return SUPPORTED_REMOTE_SERVERS_FOR_DEFAULT_ACTION_TYPES.stream().filter(url::contains).findFirst().orElse("");
}

public enum ActionType {
PREDICT,
EXECUTE,
Expand Down
5 changes: 5 additions & 0 deletions common/src/main/java/org/opensearch/ml/common/connector/HttpConnector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ public HttpConnector(
String tenantId
) {
validateProtocol(protocol);
if (actions != null) {
for (ConnectorAction action : actions) {
action.validatePrePostProcessFunctions(parameters);
}
}
this.name = name;
this.description = description;
this.version = version;
Expand Down
5 changes: 5 additions & 0 deletions ...on/src/main/java/org/opensearch/ml/common/transport/connector/MLCreateConnectorInput.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,11 @@ public MLCreateConnectorInput(
if (credential == null || credential.isEmpty()) {
throw new IllegalArgumentException("Connector credential is null or empty list");
}
if (actions != null) {
for (ConnectorAction action : actions) {
action.validatePrePostProcessFunctions(parameters);
}
}
}
this.name = name;
this.description = description;
Expand Down
Loading
Loading