From 637dfd88b6b75c50c7a9d4f677f1b01476ff708c Mon Sep 17 00:00:00 2001 From: Joshua Palis Date: Wed, 10 Apr 2024 19:21:59 +0000 Subject: [PATCH 1/8] Adding guardrails to default use case params Signed-off-by: Joshua Palis --- .../flowframework/common/CommonValue.java | 14 +++ .../flowframework/common/DefaultUseCases.java | 87 +++++++++++++++---- .../rest/RestCreateWorkflowAction.java | 28 ++++-- 3 files changed, 107 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/opensearch/flowframework/common/CommonValue.java b/src/main/java/org/opensearch/flowframework/common/CommonValue.java index 8df5613d4..ac40e5f8e 100644 --- a/src/main/java/org/opensearch/flowframework/common/CommonValue.java +++ b/src/main/java/org/opensearch/flowframework/common/CommonValue.java @@ -202,4 +202,18 @@ private CommonValue() {} public static final String RESOURCE_ID = "resource_id"; /** The field name for the opensearch-ml plugin */ public static final String OPENSEARCH_ML = "opensearch-ml"; + + /* + * Constants assoicated with substitution / default templates + */ + /** The field name for connector credential key substitution */ + public static final String CREATE_CONNECTOR_CREDENTIAL_KEY = "create_connector.credential.key"; + /** The field name for connector credential access key substitution */ + public static final String CREATE_CONNECTOR_CREDENTIAL_ACCESS_KEY = "create_connector.credential.access_key"; + /** The field name for connector credential secret key substitution */ + public static final String CREATE_CONNECTOR_CREDENTIAL_SECRET_KEY = "create_connector.credential.secret_key"; + /** The field name for connector credential session token substitution */ + public static final String CREATE_CONNECTOR_CREDENTIAL_SESSION_TOKEN = "create_connector.credential.session_token"; + /** The field name for ingest pipeline model ID substitution */ + public static final String CREATE_INGEST_PIPELINE_MODEL_ID = "create_ingest_pipeline.model_id"; } diff --git a/src/main/java/org/opensearch/flowframework/common/DefaultUseCases.java b/src/main/java/org/opensearch/flowframework/common/DefaultUseCases.java index 265409562..d3ed98859 100644 --- a/src/main/java/org/opensearch/flowframework/common/DefaultUseCases.java +++ b/src/main/java/org/opensearch/flowframework/common/DefaultUseCases.java @@ -13,6 +13,16 @@ import org.opensearch.core.rest.RestStatus; import org.opensearch.flowframework.exception.FlowFrameworkException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +import static org.opensearch.flowframework.common.CommonValue.CREATE_CONNECTOR_CREDENTIAL_ACCESS_KEY; +import static org.opensearch.flowframework.common.CommonValue.CREATE_CONNECTOR_CREDENTIAL_KEY; +import static org.opensearch.flowframework.common.CommonValue.CREATE_CONNECTOR_CREDENTIAL_SECRET_KEY; +import static org.opensearch.flowframework.common.CommonValue.CREATE_CONNECTOR_CREDENTIAL_SESSION_TOKEN; +import static org.opensearch.flowframework.common.CommonValue.CREATE_INGEST_PIPELINE_MODEL_ID; + /** * Enum encapsulating the different default use cases and templates we have stored */ @@ -22,94 +32,119 @@ public enum DefaultUseCases { OPEN_AI_EMBEDDING_MODEL_DEPLOY( "open_ai_embedding_model_deploy", "defaults/openai-embedding-defaults.json", - "substitutionTemplates/deploy-remote-model-template.json" + "substitutionTemplates/deploy-remote-model-template.json", + List.of(CREATE_CONNECTOR_CREDENTIAL_KEY) ), /** defaults file and substitution ready template for Cohere embedding model */ COHERE_EMBEDDING_MODEL_DEPLOY( "cohere_embedding_model_deploy", "defaults/cohere-embedding-defaults.json", - "substitutionTemplates/deploy-remote-model-extra-params-template.json" + "substitutionTemplates/deploy-remote-model-extra-params-template.json", + List.of(CREATE_CONNECTOR_CREDENTIAL_KEY) ), /** defaults file and substitution ready template for Bedrock Titan embedding model */ BEDROCK_TITAN_EMBEDDING_MODEL_DEPLOY( "bedrock_titan_embedding_model_deploy", "defaults/bedrock-titan-embedding-defaults.json", - "substitutionTemplates/deploy-remote-bedrock-model-template.json" + "substitutionTemplates/deploy-remote-bedrock-model-template.json", + List.of(CREATE_CONNECTOR_CREDENTIAL_ACCESS_KEY, CREATE_CONNECTOR_CREDENTIAL_SECRET_KEY, CREATE_CONNECTOR_CREDENTIAL_SESSION_TOKEN) ), /** defaults file and substitution ready template for Bedrock Titan multimodal embedding model */ BEDROCK_TITAN_MULTIMODAL_MODEL_DEPLOY( "bedrock_titan_multimodal_model_deploy", "defaults/bedrock-titan-multimodal-defaults.json", - "substitutionTemplates/deploy-remote-bedrock-model-template.json" + "substitutionTemplates/deploy-remote-bedrock-model-template.json", + List.of(CREATE_CONNECTOR_CREDENTIAL_ACCESS_KEY, CREATE_CONNECTOR_CREDENTIAL_SECRET_KEY, CREATE_CONNECTOR_CREDENTIAL_SESSION_TOKEN) ), /** defaults file and substitution ready template for Cohere chat model */ COHERE_CHAT_MODEL_DEPLOY( "cohere_chat_model_deploy", "defaults/cohere-chat-defaults.json", - "substitutionTemplates/deploy-remote-model-chat-template.json" + "substitutionTemplates/deploy-remote-model-chat-template.json", + List.of(CREATE_CONNECTOR_CREDENTIAL_KEY) ), /** defaults file and substitution ready template for OpenAI chat model */ OPENAI_CHAT_MODEL_DEPLOY( "openai_chat_model_deploy", "defaults/openai-chat-defaults.json", - "substitutionTemplates/deploy-remote-model-chat-template.json" + "substitutionTemplates/deploy-remote-model-chat-template.json", + List.of(CREATE_CONNECTOR_CREDENTIAL_KEY) ), /** defaults file and substitution ready template for local neural sparse model and ingest pipeline*/ LOCAL_NEURAL_SPARSE_SEARCH_BI_ENCODER( "local_neural_sparse_search_bi_encoder", "defaults/local-sparse-search-biencoder-defaults.json", - "substitutionTemplates/neural-sparse-local-biencoder-template.json" + "substitutionTemplates/neural-sparse-local-biencoder-template.json", + Collections.emptyList() ), /** defaults file and substitution ready template for semantic search, no model creation*/ - SEMANTIC_SEARCH("semantic_search", "defaults/semantic-search-defaults.json", "substitutionTemplates/semantic-search-template.json"), + SEMANTIC_SEARCH( + "semantic_search", + "defaults/semantic-search-defaults.json", + "substitutionTemplates/semantic-search-template.json", + List.of(CREATE_INGEST_PIPELINE_MODEL_ID) + ), /** defaults file and substitution ready template for multimodal search, no model creation*/ MULTI_MODAL_SEARCH( "multimodal_search", "defaults/multi-modal-search-defaults.json", - "substitutionTemplates/multi-modal-search-template.json" + "substitutionTemplates/multi-modal-search-template.json", + List.of(CREATE_INGEST_PIPELINE_MODEL_ID) ), /** defaults file and substitution ready template for multimodal search, no model creation*/ MULTI_MODAL_SEARCH_WITH_BEDROCK_TITAN( "multimodal_search_with_bedrock_titan", "defaults/multimodal-search-bedrock-titan-defaults.json", - "substitutionTemplates/multi-modal-search-with-bedrock-titan-template.json" + "substitutionTemplates/multi-modal-search-with-bedrock-titan-template.json", + List.of(CREATE_CONNECTOR_CREDENTIAL_ACCESS_KEY, CREATE_CONNECTOR_CREDENTIAL_SECRET_KEY, CREATE_CONNECTOR_CREDENTIAL_SESSION_TOKEN) ), /** defaults file and substitution ready template for semantic search with query enricher processor attached, no model creation*/ SEMANTIC_SEARCH_WITH_QUERY_ENRICHER( "semantic_search_with_query_enricher", "defaults/semantic-search-query-enricher-defaults.json", - "substitutionTemplates/semantic-search-with-query-enricher-template.json" + "substitutionTemplates/semantic-search-with-query-enricher-template.json", + List.of(CREATE_INGEST_PIPELINE_MODEL_ID) ), /** defaults file and substitution ready template for semantic search with cohere embedding model*/ SEMANTIC_SEARCH_WITH_COHERE_EMBEDDING( "semantic_search_with_cohere_embedding", "defaults/cohere-embedding-semantic-search-defaults.json", - "substitutionTemplates/semantic-search-with-model-template.json" + "substitutionTemplates/semantic-search-with-model-template.json", + List.of(CREATE_CONNECTOR_CREDENTIAL_KEY) ), /** defaults file and substitution ready template for semantic search with query enricher processor attached and cohere embedding model*/ SEMANTIC_SEARCH_WITH_COHERE_EMBEDDING_AND_QUERY_ENRICHER( "semantic_search_with_cohere_embedding_query_enricher", "defaults/cohere-embedding-semantic-search-with-query-enricher-defaults.json", - "substitutionTemplates/semantic-search-with-model-and-query-enricher-template.json" + "substitutionTemplates/semantic-search-with-model-and-query-enricher-template.json", + List.of(CREATE_CONNECTOR_CREDENTIAL_KEY) ), /** defaults file and substitution ready template for hybrid search, no model creation*/ - HYBRID_SEARCH("hybrid_search", "defaults/hybrid-search-defaults.json", "substitutionTemplates/hybrid-search-template.json"), + HYBRID_SEARCH( + "hybrid_search", + "defaults/hybrid-search-defaults.json", + "substitutionTemplates/hybrid-search-template.json", + List.of(CREATE_INGEST_PIPELINE_MODEL_ID) + ), /** defaults file and substitution ready template for conversational search with cohere chat model*/ CONVERSATIONAL_SEARCH_WITH_COHERE_DEPLOY( "conversational_search_with_llm_deploy", "defaults/conversational-search-defaults.json", - "substitutionTemplates/conversational-search-with-cohere-model-template.json" + "substitutionTemplates/conversational-search-with-cohere-model-template.json", + List.of(CREATE_CONNECTOR_CREDENTIAL_KEY) ); private final String useCaseName; private final String defaultsFile; private final String substitutionReadyFile; + private final List requiredParams; private static final Logger logger = LogManager.getLogger(DefaultUseCases.class); - DefaultUseCases(String useCaseName, String defaultsFile, String substitutionReadyFile) { + DefaultUseCases(String useCaseName, String defaultsFile, String substitutionReadyFile, List requiredParams) { this.useCaseName = useCaseName; this.defaultsFile = defaultsFile; this.substitutionReadyFile = substitutionReadyFile; + this.requiredParams = requiredParams; } /** @@ -136,6 +171,14 @@ public String getSubstitutionReadyFile() { return substitutionReadyFile; } + /** + * Returns the required params for the given enum Constant + * @return the required params of the given useCase + */ + public List getRequiredParams() { + return requiredParams; + } + /** * Gets the defaultsFile based on the given use case. * @param useCaseName name of the given use case @@ -171,4 +214,16 @@ public static String getSubstitutionReadyFileByUseCaseName(String useCaseName) t logger.error("Unable to find substitution ready file for use case: {}", useCaseName); throw new FlowFrameworkException("Unable to find substitution ready file for use case: " + useCaseName, RestStatus.BAD_REQUEST); } + + public static List getRequiredParamsByUseCaseName(String useCaseName) throws FlowFrameworkException { + if (useCaseName != null && !useCaseName.isEmpty()) { + for (DefaultUseCases useCase : values()) { + if (useCase.getUseCaseName().equals(useCaseName)) { + return new ArrayList(useCase.getRequiredParams()); + } + } + } + logger.error("Unable to find required parameters for use case: {}", useCaseName); + throw new FlowFrameworkException("Unable to find required parameters for use case: " + useCaseName, RestStatus.BAD_REQUEST); + } } diff --git a/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java index 59c8a3b59..e4e30eb1b 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java @@ -33,6 +33,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; @@ -132,6 +133,18 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli XContentParser parser = request.contentParser(); ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser); Map userDefaults = ParseUtils.parseStringToObjectMap(parser); + + // Validate user defaults key set + Set userDefaultKeys = userDefaults.keySet(); + List requiredParams = DefaultUseCases.getRequiredParamsByUseCaseName(useCase); + if (!userDefaultKeys.containsAll(requiredParams)) { + requiredParams.removeAll(userDefaultKeys); + throw new FlowFrameworkException( + "Missing the following required parameters for use case [" + useCase + "] : " + requiredParams.toString(), + RestStatus.BAD_REQUEST + ); + } + // updates the default params with anything user has given that matches for (Map.Entry userDefaultsEntry : userDefaults.entrySet()) { String key = userDefaultsEntry.getKey(); @@ -141,13 +154,16 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli } } } catch (Exception ex) { - RestStatus status = ex instanceof IOException ? RestStatus.BAD_REQUEST : ExceptionsHelper.status(ex); - String errorMessage = - "failure parsing request body when a use case is given, make sure to provide a map with values that are either Strings, Arrays, or Map of Strings to Strings"; - logger.error(errorMessage, ex); - throw new FlowFrameworkException(errorMessage, status); + if (ex instanceof FlowFrameworkException) { + throw (FlowFrameworkException) ex; + } else { + RestStatus status = ex instanceof IOException ? RestStatus.BAD_REQUEST : ExceptionsHelper.status(ex); + String errorMessage = + "failure parsing request body when a use case is given, make sure to provide a map with values that are either Strings, Arrays, or Map of Strings to Strings"; + logger.error(errorMessage, ex); + throw new FlowFrameworkException(errorMessage, status); + } } - } useCaseTemplateFileInStringFormat = (String) ParseUtils.conditionallySubstitute( From 18f98948d7a1e32e074f2ad4ecea302b4b32f202 Mon Sep 17 00:00:00 2001 From: Joshua Palis Date: Wed, 10 Apr 2024 19:29:21 +0000 Subject: [PATCH 2/8] Updating changelog and adding javadocs Signed-off-by: Joshua Palis --- CHANGELOG.md | 1 + .../opensearch/flowframework/common/DefaultUseCases.java | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b302d51e6..cba8b5e40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) ## [Unreleased 2.x](https://github.com/opensearch-project/flow-framework/compare/2.13...2.x) ### Features ### Enhancements +- Adding guardrails to default use case params ([#658](https://github.com/opensearch-project/flow-framework/pull/658)) ### Bug Fixes - Reset workflow state to initial state after successful deprovision ([#635](https://github.com/opensearch-project/flow-framework/pull/635)) - Silently ignore content on APIs that don't require it ([#639](https://github.com/opensearch-project/flow-framework/pull/639)) diff --git a/src/main/java/org/opensearch/flowframework/common/DefaultUseCases.java b/src/main/java/org/opensearch/flowframework/common/DefaultUseCases.java index d3ed98859..1023cd415 100644 --- a/src/main/java/org/opensearch/flowframework/common/DefaultUseCases.java +++ b/src/main/java/org/opensearch/flowframework/common/DefaultUseCases.java @@ -215,6 +215,12 @@ public static String getSubstitutionReadyFileByUseCaseName(String useCaseName) t throw new FlowFrameworkException("Unable to find substitution ready file for use case: " + useCaseName, RestStatus.BAD_REQUEST); } + /** + * Gets the required parameters based on the given use case + * @param useCaseName name of the given use case + * @return the list of required params + * @throws FlowFrameworkException if the use case doesn't exist in enum + */ public static List getRequiredParamsByUseCaseName(String useCaseName) throws FlowFrameworkException { if (useCaseName != null && !useCaseName.isEmpty()) { for (DefaultUseCases useCase : values()) { From d4c5560aa9b589a0fbc92eef41c331d14bff026d Mon Sep 17 00:00:00 2001 From: Joshua Palis Date: Wed, 10 Apr 2024 19:41:48 +0000 Subject: [PATCH 3/8] Fixing test Signed-off-by: Joshua Palis --- .../flowframework/rest/RestCreateWorkflowActionTests.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/opensearch/flowframework/rest/RestCreateWorkflowActionTests.java b/src/test/java/org/opensearch/flowframework/rest/RestCreateWorkflowActionTests.java index 3381bbbec..1d8151ea0 100644 --- a/src/test/java/org/opensearch/flowframework/rest/RestCreateWorkflowActionTests.java +++ b/src/test/java/org/opensearch/flowframework/rest/RestCreateWorkflowActionTests.java @@ -34,6 +34,7 @@ import java.util.Locale; import java.util.Map; +import static org.opensearch.flowframework.common.CommonValue.CREATE_CONNECTOR_CREDENTIAL_KEY; import static org.opensearch.flowframework.common.CommonValue.PROVISION_WORKFLOW; import static org.opensearch.flowframework.common.CommonValue.USE_CASE; import static org.opensearch.flowframework.common.CommonValue.WORKFLOW_URI; @@ -157,7 +158,7 @@ public void testCreateWorkflowRequestWithUseCaseAndContent() throws Exception { RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.POST) .withPath(this.createWorkflowPath) .withParams(Map.of(USE_CASE, DefaultUseCases.COHERE_EMBEDDING_MODEL_DEPLOY.getUseCaseName())) - .withContent(new BytesArray("{\"key\":\"step\"}"), MediaTypeRegistry.JSON) + .withContent(new BytesArray("{\"" + CREATE_CONNECTOR_CREDENTIAL_KEY + "\":\"step\"}"), MediaTypeRegistry.JSON) .build(); FakeRestChannel channel = new FakeRestChannel(request, false, 1); doAnswer(invocation -> { From 7239512196647fa867bdef57a07125b6998c5599 Mon Sep 17 00:00:00 2001 From: Joshua Palis Date: Wed, 10 Apr 2024 21:31:07 +0000 Subject: [PATCH 4/8] Fixing integration tests, covering case in which no content is passed at all for default cases Signed-off-by: Joshua Palis --- .../rest/RestCreateWorkflowAction.java | 11 ++++++++--- .../FlowFrameworkRestTestCase.java | 12 ++++++++++-- .../rest/FlowFrameworkRestApiIT.java | 18 ++++++++++++++---- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java index e4e30eb1b..d749fea92 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java @@ -127,8 +127,14 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli ); String defaultsFilePath = DefaultUseCases.getDefaultsFileByUseCaseName(useCase); useCaseDefaultsMap = ParseUtils.parseJsonFileToStringToStringMap("/" + defaultsFilePath); - - if (request.hasContent()) { + List requiredParams = DefaultUseCases.getRequiredParamsByUseCaseName(useCase); + + if (request.hasContent() == false && requiredParams.size() != 0) { + throw new FlowFrameworkException( + "Missing the following required parameters for use case [" + useCase + "] : " + requiredParams.toString(), + RestStatus.BAD_REQUEST + ); + } else { try { XContentParser parser = request.contentParser(); ensureExpectedToken(XContentParser.Token.START_OBJECT, parser.nextToken(), parser); @@ -136,7 +142,6 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli // Validate user defaults key set Set userDefaultKeys = userDefaults.keySet(); - List requiredParams = DefaultUseCases.getRequiredParamsByUseCaseName(useCase); if (!userDefaultKeys.containsAll(requiredParams)) { requiredParams.removeAll(userDefaultKeys); throw new FlowFrameworkException( diff --git a/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java b/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java index 8cbabc95f..635126be7 100644 --- a/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java +++ b/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java @@ -345,16 +345,24 @@ protected Response createWorkflow(RestClient client, Template template) throws E * Helper method to invoke the Create Workflow Rest Action without validation * @param client the rest client * @param useCase the usecase to create + * @param the required params * @throws Exception if the request fails * @return a rest response */ - protected Response createWorkflowWithUseCase(RestClient client, String useCase) throws Exception { + protected Response createWorkflowWithUseCase(RestClient client, String useCase, List params) throws Exception { + + StringBuilder sb = new StringBuilder(); + for (String param : params) { + sb.append("\"" + param + "\" : \"\"").append(","); + } + if (params.size() != 0) sb.deleteCharAt(sb.length() - 1); + return TestHelpers.makeRequest( client, "POST", WORKFLOW_URI + "?validation=off&use_case=" + useCase, Collections.emptyMap(), - "{}", + "{" + sb.toString() + "}", null ); } diff --git a/src/test/java/org/opensearch/flowframework/rest/FlowFrameworkRestApiIT.java b/src/test/java/org/opensearch/flowframework/rest/FlowFrameworkRestApiIT.java index ada8d5513..53d2b21a6 100644 --- a/src/test/java/org/opensearch/flowframework/rest/FlowFrameworkRestApiIT.java +++ b/src/test/java/org/opensearch/flowframework/rest/FlowFrameworkRestApiIT.java @@ -43,6 +43,8 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.stream.Collectors; +import static org.opensearch.flowframework.common.CommonValue.CREATE_CONNECTOR_CREDENTIAL_KEY; +import static org.opensearch.flowframework.common.CommonValue.CREATE_INGEST_PIPELINE_MODEL_ID; import static org.opensearch.flowframework.common.CommonValue.PROVISION_WORKFLOW; import static org.opensearch.flowframework.common.CommonValue.WORKFLOW_ID; @@ -406,7 +408,7 @@ public void testCreateAndProvisionIngestAndSearchPipeline() throws Exception { public void testDefaultCohereUseCase() throws Exception { // Hit Create Workflow API with original template - Response response = createWorkflowWithUseCase(client(), "cohere_embedding_model_deploy"); + Response response = createWorkflowWithUseCase(client(), "cohere_embedding_model_deploy", List.of(CREATE_CONNECTOR_CREDENTIAL_KEY)); assertEquals(RestStatus.CREATED, TestHelpers.restStatus(response)); Map responseMap = entityAsMap(response); @@ -442,8 +444,12 @@ public void testDefaultCohereUseCase() throws Exception { } public void testDefaultSemanticSearchUseCaseWithFailureExpected() throws Exception { - // Hit Create Workflow API with original template - Response response = createWorkflowWithUseCase(client(), "semantic_search"); + // Hit Create Workflow API with original template without required params + Response response = createWorkflowWithUseCase(client(), "semantic_search", Collections.emptyList()); + assertEquals(RestStatus.BAD_REQUEST, TestHelpers.restStatus(response)); + + // Pass in required params + response = createWorkflowWithUseCase(client(), "semantic_search", List.of(CREATE_INGEST_PIPELINE_MODEL_ID)); assertEquals(RestStatus.CREATED, TestHelpers.restStatus(response)); Map responseMap = entityAsMap(response); @@ -483,7 +489,11 @@ public void testAllDefaultUseCasesCreation() throws Exception { .collect(Collectors.toSet()); for (String useCaseName : allUseCaseNames) { - Response response = createWorkflowWithUseCase(client(), useCaseName); + Response response = createWorkflowWithUseCase( + client(), + useCaseName, + DefaultUseCases.getRequiredParamsByUseCaseName(useCaseName) + ); assertEquals(RestStatus.CREATED, TestHelpers.restStatus(response)); Map responseMap = entityAsMap(response); From f5f2fd997c7da6d433e46d3e0fcb2361eabc5ec3 Mon Sep 17 00:00:00 2001 From: Joshua Palis Date: Wed, 10 Apr 2024 21:52:38 +0000 Subject: [PATCH 5/8] Fixing tests Signed-off-by: Joshua Palis --- .../flowframework/rest/FlowFrameworkRestApiIT.java | 12 +++++++++--- .../rest/RestCreateWorkflowActionTests.java | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/test/java/org/opensearch/flowframework/rest/FlowFrameworkRestApiIT.java b/src/test/java/org/opensearch/flowframework/rest/FlowFrameworkRestApiIT.java index 53d2b21a6..894ee3010 100644 --- a/src/test/java/org/opensearch/flowframework/rest/FlowFrameworkRestApiIT.java +++ b/src/test/java/org/opensearch/flowframework/rest/FlowFrameworkRestApiIT.java @@ -445,11 +445,17 @@ public void testDefaultCohereUseCase() throws Exception { public void testDefaultSemanticSearchUseCaseWithFailureExpected() throws Exception { // Hit Create Workflow API with original template without required params - Response response = createWorkflowWithUseCase(client(), "semantic_search", Collections.emptyList()); - assertEquals(RestStatus.BAD_REQUEST, TestHelpers.restStatus(response)); + ResponseException exception = expectThrows( + ResponseException.class, + () -> createWorkflowWithUseCase(client(), "semantic_search", Collections.emptyList()) + ); + assertTrue( + exception.getMessage() + .contains("Missing the following required parameters for use case [semantic_search] : [create_ingest_pipeline.model_id]") + ); // Pass in required params - response = createWorkflowWithUseCase(client(), "semantic_search", List.of(CREATE_INGEST_PIPELINE_MODEL_ID)); + Response response = createWorkflowWithUseCase(client(), "semantic_search", List.of(CREATE_INGEST_PIPELINE_MODEL_ID)); assertEquals(RestStatus.CREATED, TestHelpers.restStatus(response)); Map responseMap = entityAsMap(response); diff --git a/src/test/java/org/opensearch/flowframework/rest/RestCreateWorkflowActionTests.java b/src/test/java/org/opensearch/flowframework/rest/RestCreateWorkflowActionTests.java index 1d8151ea0..b55d6b1f2 100644 --- a/src/test/java/org/opensearch/flowframework/rest/RestCreateWorkflowActionTests.java +++ b/src/test/java/org/opensearch/flowframework/rest/RestCreateWorkflowActionTests.java @@ -141,7 +141,7 @@ public void testCreateWorkflowRequestWithUseCaseButNoProvision() throws Exceptio RestRequest request = new FakeRestRequest.Builder(xContentRegistry()).withMethod(RestRequest.Method.POST) .withPath(this.createWorkflowPath) .withParams(Map.of(USE_CASE, DefaultUseCases.COHERE_EMBEDDING_MODEL_DEPLOY.getUseCaseName())) - .withContent(new BytesArray(""), MediaTypeRegistry.JSON) + .withContent(new BytesArray("{\"" + CREATE_CONNECTOR_CREDENTIAL_KEY + "\":\"\"}"), MediaTypeRegistry.JSON) .build(); FakeRestChannel channel = new FakeRestChannel(request, false, 1); doAnswer(invocation -> { From 855bd83136e1f65117b452c7343ab29c040cb15c Mon Sep 17 00:00:00 2001 From: Joshua Palis Date: Thu, 11 Apr 2024 20:23:16 +0000 Subject: [PATCH 6/8] Fixing rest create workflow action Signed-off-by: Joshua Palis --- .../flowframework/rest/RestCreateWorkflowAction.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java index d749fea92..ec6c38174 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java @@ -129,11 +129,13 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli useCaseDefaultsMap = ParseUtils.parseJsonFileToStringToStringMap("/" + defaultsFilePath); List requiredParams = DefaultUseCases.getRequiredParamsByUseCaseName(useCase); - if (request.hasContent() == false && requiredParams.size() != 0) { - throw new FlowFrameworkException( - "Missing the following required parameters for use case [" + useCase + "] : " + requiredParams.toString(), - RestStatus.BAD_REQUEST - ); + if (request.hasContent() == false) { + if (requiredParams.size() != 0) { + throw new FlowFrameworkException( + "Missing the following required parameters for use case [" + useCase + "] : " + requiredParams.toString(), + RestStatus.BAD_REQUEST + ); + } } else { try { XContentParser parser = request.contentParser(); From 079a3366b1bef803fbb743582940503e2d22f54d Mon Sep 17 00:00:00 2001 From: Joshua Palis Date: Thu, 11 Apr 2024 21:46:05 +0000 Subject: [PATCH 7/8] addressing PR comments Signed-off-by: Joshua Palis --- CHANGELOG.md | 2 +- .../opensearch/flowframework/common/DefaultUseCases.java | 7 +++---- .../flowframework/rest/RestCreateWorkflowAction.java | 6 +++--- .../flowframework/FlowFrameworkRestTestCase.java | 6 ++++-- 4 files changed, 11 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cba8b5e40..a66add559 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) ## [Unreleased 2.x](https://github.com/opensearch-project/flow-framework/compare/2.13...2.x) ### Features ### Enhancements -- Adding guardrails to default use case params ([#658](https://github.com/opensearch-project/flow-framework/pull/658)) +- Add guardrails to default use case params ([#658](https://github.com/opensearch-project/flow-framework/pull/658)) ### Bug Fixes - Reset workflow state to initial state after successful deprovision ([#635](https://github.com/opensearch-project/flow-framework/pull/635)) - Silently ignore content on APIs that don't require it ([#639](https://github.com/opensearch-project/flow-framework/pull/639)) diff --git a/src/main/java/org/opensearch/flowframework/common/DefaultUseCases.java b/src/main/java/org/opensearch/flowframework/common/DefaultUseCases.java index 1023cd415..bc88f2b4d 100644 --- a/src/main/java/org/opensearch/flowframework/common/DefaultUseCases.java +++ b/src/main/java/org/opensearch/flowframework/common/DefaultUseCases.java @@ -219,9 +219,8 @@ public static String getSubstitutionReadyFileByUseCaseName(String useCaseName) t * Gets the required parameters based on the given use case * @param useCaseName name of the given use case * @return the list of required params - * @throws FlowFrameworkException if the use case doesn't exist in enum */ - public static List getRequiredParamsByUseCaseName(String useCaseName) throws FlowFrameworkException { + public static List getRequiredParamsByUseCaseName(String useCaseName) { if (useCaseName != null && !useCaseName.isEmpty()) { for (DefaultUseCases useCase : values()) { if (useCase.getUseCaseName().equals(useCaseName)) { @@ -229,7 +228,7 @@ public static List getRequiredParamsByUseCaseName(String useCaseName) th } } } - logger.error("Unable to find required parameters for use case: {}", useCaseName); - throw new FlowFrameworkException("Unable to find required parameters for use case: " + useCaseName, RestStatus.BAD_REQUEST); + logger.error("Default use case [" + useCaseName + "] does not exist"); + throw new FlowFrameworkException("Default use case [" + useCaseName + "] does not exist", RestStatus.BAD_REQUEST); } } diff --git a/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java index ec6c38174..5db17d2b7 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestCreateWorkflowAction.java @@ -129,8 +129,8 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli useCaseDefaultsMap = ParseUtils.parseJsonFileToStringToStringMap("/" + defaultsFilePath); List requiredParams = DefaultUseCases.getRequiredParamsByUseCaseName(useCase); - if (request.hasContent() == false) { - if (requiredParams.size() != 0) { + if (!request.hasContent()) { + if (!requiredParams.isEmpty()) { throw new FlowFrameworkException( "Missing the following required parameters for use case [" + useCase + "] : " + requiredParams.toString(), RestStatus.BAD_REQUEST @@ -162,7 +162,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli } } catch (Exception ex) { if (ex instanceof FlowFrameworkException) { - throw (FlowFrameworkException) ex; + throw ex; } else { RestStatus status = ex instanceof IOException ? RestStatus.BAD_REQUEST : ExceptionsHelper.status(ex); String errorMessage = diff --git a/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java b/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java index 635126be7..dbdffb144 100644 --- a/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java +++ b/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java @@ -353,9 +353,11 @@ protected Response createWorkflowWithUseCase(RestClient client, String useCase, StringBuilder sb = new StringBuilder(); for (String param : params) { - sb.append("\"" + param + "\" : \"\"").append(","); + sb.append('"').append(param).append("\" : \"\","); + } + if (params.isEmpty()) { + sb.deleteCharAt(sb.length() - 1); } - if (params.size() != 0) sb.deleteCharAt(sb.length() - 1); return TestHelpers.makeRequest( client, From 613a83dfe9efca380cd833b52c9b472b12efdc43 Mon Sep 17 00:00:00 2001 From: Joshua Palis Date: Thu, 11 Apr 2024 22:15:18 +0000 Subject: [PATCH 8/8] fixing test Signed-off-by: Joshua Palis --- .../org/opensearch/flowframework/FlowFrameworkRestTestCase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java b/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java index dbdffb144..2eaff69b4 100644 --- a/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java +++ b/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java @@ -355,7 +355,7 @@ protected Response createWorkflowWithUseCase(RestClient client, String useCase, for (String param : params) { sb.append('"').append(param).append("\" : \"\","); } - if (params.isEmpty()) { + if (!params.isEmpty()) { sb.deleteCharAt(sb.length() - 1); }