-
Notifications
You must be signed in to change notification settings - Fork 36
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
Updates exception handling with FlowFrameworkException and fix Global Context mapping #106
Conversation
… attempting to parse from source Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
…exception handling, fixing affected tests Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Joshua Palis <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #106 +/- ##
============================================
- Coverage 81.46% 79.04% -2.42%
+ Complexity 285 263 -22
============================================
Files 30 30
Lines 1122 1026 -96
Branches 126 103 -23
============================================
- Hits 914 811 -103
- Misses 162 179 +17
+ Partials 46 36 -10
|
…ng all invocations of toDocumentSource/parseFromDocumentSource and using toXContent/parse instead Signed-off-by: Joshua Palis <[email protected]>
…nt requests Signed-off-by: Joshua Palis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I'm okay with 400 (Bad Request) but technically it means bad syntax. From RFC 2068:
The request could not be understood by the server due to malformed
syntax. The client SHOULD NOT repeat the request without
modifications.
Consider using 422 (Unprocessable Entity) for the cases where it's valid JSON but a bad value (equivalent of IllegalArgumetnException). From RFC 2518:
The 422 (Unprocessable Entity) status code means the server
understands the content type of the request entity (hence a
415(Unsupported Media Type) status code is inappropriate), and the
syntax of the request entity is correct (thus a 400 (Bad Request)
status code is inappropriate) but was unable to process the contained
instructions. For example, this error condition may occur if an XML
request body contains well-formed (i.e., syntactically correct), but
semantically erroneous, XML instructions.
For this PR I'd suggest 422 for the cases where you parse a valid edge source/destination but it doesn't match a node, for example (but based on ExceptionsHelper
I'm still OK with 400.)
Closing this PR in favor of #137 |
Description
This PR updates the workflow parsing, workflow execution and rest action exception handling to throw
FlowFrameworkException
s with the correct REST Status code.Example invalid create workflow request with missing name :
Note : Opening this PR up for second opinions regarding the various rest status codes for each exception. I've opted to return 400 for parsing, rest request format errors. Client exception rest status codes are determined via the OpenSearch ExceptionsHelper::status method
Additionally this PR modified the global context mapping so that
user_outputs
,workflows
, andresourcesCreated
are of typeobject
instead oftext
. This allows us to useTemplate::toXContent
andTemplate::parse
directly when reading and writing to the global context indexIssues Resolved
Part of #88
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.