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.

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

Adding guardrails to default use case params #658

Merged
merged 8 commits into from
Apr 11, 2024

Conversation

joshpalis
Copy link
Member

Description

Adds a requiredParams field to the DefaultUseCases enum which is used validate the given request body of default use case request

Examples :

curl -i -XPOST "http://localhost:9200/_plugins/_flow_framework/workflow?use_case=semantic_search_with_cohere_embedding_query_enricher&provision=true" -H "Content-Type:application/json" -d '{"test_value" : "QFaXLdVUFfk7XYxqSOtweCvVxXS2l3Jsw1DeKhuR"}'
HTTP/1.1 400 Bad Request
X-OpenSearch-Version: OpenSearch/3.0.0-SNAPSHOT (opensearch)
content-type: application/json; charset=UTF-8
content-length: 157

{"error":"Missing the following required parameters for use case [semantic_search_with_cohere_embedding_query_enricher] : [create_connector.credential.key]"}

curl -i -XPOST "http://localhost:9200/_plugins/_flow_framework/workflow?use_case=multimodal_search_with_bedrock_titan&provision=true&?pretty" -H "Content-Type:application/json" -d '{"test_value" : "QFaXLdVUFfk7XYxqSOtweCvVxXS2l3Jsw1DeKhuR"}'
HTTP/1.1 400 Bad Request
X-OpenSearch-Version: OpenSearch/3.0.0-SNAPSHOT (opensearch)
content-type: application/json; charset=UTF-8
content-length: 231

{"error":"Missing the following required parameters for use case [multimodal_search_with_bedrock_titan] : [create_connector.credential.access_key, create_connector.credential.secret_key, create_connector.credential.session_token]"}

Issues Resolved

#655

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Joshua Palis <[email protected]>
Copy link

codecov bot commented Apr 10, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 72.20%. Comparing base (3c67cff) to head (613a83d).

Files Patch % Lines
...h/flowframework/rest/RestCreateWorkflowAction.java 12.50% 12 Missing and 2 partials ⚠️
...ensearch/flowframework/common/DefaultUseCases.java 84.61% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #658      +/-   ##
============================================
- Coverage     72.22%   72.20%   -0.02%     
- Complexity      680      682       +2     
============================================
  Files            82       82              
  Lines          3528     3562      +34     
  Branches        285      290       +5     
============================================
+ Hits           2548     2572      +24     
- Misses          854      859       +5     
- Partials        126      131       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

… at all for default cases

Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

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

LG overall with few comments

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

LGTM generally. Lots of nits for your consideration. I'm traveling and unlikely to review again promptly so please don't wait on me to review again.

Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

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

LGTM

@joshpalis joshpalis merged commit 9d28045 into opensearch-project:main Apr 11, 2024
31 of 32 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 11, 2024
* Adding guardrails to default use case params

Signed-off-by: Joshua Palis <[email protected]>

* Updating changelog and adding javadocs

Signed-off-by: Joshua Palis <[email protected]>

* Fixing test

Signed-off-by: Joshua Palis <[email protected]>

* Fixing integration tests, covering case in which no content is passed at all for default cases

Signed-off-by: Joshua Palis <[email protected]>

* Fixing tests

Signed-off-by: Joshua Palis <[email protected]>

* Fixing rest create workflow action

Signed-off-by: Joshua Palis <[email protected]>

* addressing PR comments

Signed-off-by: Joshua Palis <[email protected]>

* fixing test

Signed-off-by: Joshua Palis <[email protected]>

---------

Signed-off-by: Joshua Palis <[email protected]>
(cherry picked from commit 9d28045)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
joshpalis pushed a commit that referenced this pull request Apr 11, 2024
Adding guardrails to default use case params (#658)

* Adding guardrails to default use case params



* Updating changelog and adding javadocs



* Fixing test



* Fixing integration tests, covering case in which no content is passed at all for default cases



* Fixing tests



* Fixing rest create workflow action



* addressing PR comments



* fixing test



---------


(cherry picked from commit 9d28045)

Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport PRs to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants