fix response payload and incorrectly parsing error response #63
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For some protocols, error type flag exists in error response body, then we need to collect response stream to JS object and parse the error type; For other protocols, error type flag doesn't exist in error response body, then we don't need to collect the response stream in error dispatcher. Instead, we can treat the error like normal response. So that error shape supports the same traits as normal responses like streaming, payload etc. This is done by add a new flag in Protocol generator-- isErrorCodeInBody. When it return true, it means error type flag exists in error response body, then body is parsed in errors dispatcher, and each error deser only need to deal with parsed response body in JS object format. When it returns false, it means error type can be inferred without touching response body, then error deser can access the error response intact.
The http payload trait can only be bound to strings, blobs, structures, and unions |
JordonPhillips
requested changes
Dec 24, 2019
| } else if (target instanceof BlobShape) { | ||
| // If payload is blob, only need to collect stream to binary data(Uint8Array). | ||
| writer.write("const data: any = await collectBody(output.body, context);"); | ||
| } else if (target instanceof CollectionShape || target instanceof StructureShape) { |
There was a problem hiding this comment.
This will never be a collection, but it could be a union
| @@ -108,6 +108,30 @@ static void generateMetadataDeserializer(GenerationContext context, SymbolRefere | |||
| writer.write(""); | |||
| } | |||
|
|
|||
| static void generateCollectBody(GenerationContext context) { | |||
There was a problem hiding this comment.
Can you add doc strings to these?
There was a problem hiding this comment.
To be clear I meant method-level javadocs as all the other methods in this class have.
| // then we collect body and parse it to JS object, otherwise leave the response body as is. | ||
| writer.openBlock( | ||
| "const $L: any = {", "};", shouldParseErrorBody ? "parsedOutput" : "errorOutput", () -> { | ||
| writer.write("...output,"); |
There was a problem hiding this comment.
indentation is off here
AllanZhengYP
force-pushed
the
fix-error-streaming
branch
from
82ad5a1
to
6297831
Compare
JordonPhillips
requested changes
Dec 26, 2019
AllanZhengYP
force-pushed
the
fix-error-streaming
branch
from
d03854c
to
6f4f986
Compare
If incoming error response is unknow error, we only make best effort to infer the error type. This change puts error response body to error message so that users have the information they need. This change is necessary for Node because when error response is unknown, the body stream will never be consumed, which would use a lot of resources before stream is flushed.
JordonPhillips
approved these changes
Dec 27, 2019
|
Can this get merged? (I don't have permission) |
kstich
added a commit
that referenced
this pull request
Dec 27, 2019
)" This reverts commit 44a72fe.
kstich
added a commit
that referenced
this pull request
Dec 27, 2019
)" This reverts commit 44a72fe.
srchase
pushed a commit
to srchase/smithy-typescript
that referenced
this pull request
Mar 17, 2023
…ang#63) * fix when response payload is not structured string * add support for parsing body in either dispatcher or deser For some protocols, error type flag exists in error response body, then we need to collect response stream to JS object and parse the error type; For other protocols, error type flag doesn't exist in error response body, then we don't need to collect the response stream in error dispatcher. Instead, we can treat the error like normal response. So that error shape supports the same traits as normal responses like streaming, payload etc. This is done by add a new flag in Protocol generator-- isErrorCodeInBody. When it return true, it means error type flag exists in error response body, then body is parsed in errors dispatcher, and each error deser only need to deal with parsed response body in JS object format. When it returns false, it means error type can be inferred without touching response body, then error deser can access the error response intact. * address feedbacks: fix indentation; readResponseBody; add docs * add response body string to unknown error message If incoming error response is unknow error, we only make best effort to infer the error type. This change puts error response body to error message so that users have the information they need. This change is necessary for Node because when error response is unknown, the body stream will never be consumed, which would use a lot of resources before stream is flushed.
srchase
pushed a commit
to srchase/smithy-typescript
that referenced
this pull request
Mar 17, 2023
…mithy-lang#63)" This reverts commit 44a72fe.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TwoThree fixes:Currently when response body is a payload member, the SDK either
keeps reponse intact if it has a
streamingtrait, or parse the bodyto JS object.
Actually when response payload is not streaming, it is
NOT always parsable(not a valid JSON or XML string). Specificly,
when payload is scalar(Number, String, BigInteger, etc.), we shouldn't
parse it with XML or JSON parser; We should also treat
Blobshapedifferently as we should encode the binaries into string; Only when
shape is
Set,MaporStructurecan we assume payload is parsableby JSON or XML parser.
For some protocols, error type flag exists in error response body,
then we need to collect response stream to JS object and parse the
error type; For other protocols, error type flag doesn't exist in
error response body, then we don't need to collect the response
stream in error dispatcher. Instead, we can treat the error like
normal response. So that error shape supports the same traits as
normal responses like streaming, payload etc.
This is done by adding a new flag in Protocol generator--
isErrorCodeInBody. When it return true, it means error type flag
exists in error response body, then body is parsed in errors
dispatcher, and each error deser only need to deal with parsed
response body in JS object format. When it returns false, it means
error type can be inferred without touching response body, then
error deser can access the error response intact.
UPDATED: If incoming error response is unknow error, we only
make best effort to infer the error type. This change puts error response
body string to error message so that users can have more information.
This change is necessary for Node because when error response is
unknown, the body stream will never be consumed, which would stay in
memory for a long time before it resolves automaticly.
This PR is based on presumptions:
DOCUMENT, the body CANNOT bestreaming, and can be parsed to JS object.
/cc @kstich
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.