-
Notifications
You must be signed in to change notification settings - Fork 85
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
fix response payload and incorrectly parsing error response #66
Conversation
aaf200a
to
c82964a
Compare
c82964a
to
2234a50
Compare
@@ -594,7 +605,7 @@ private void generateOperationDeserializer( | |||
|
|||
// Write out the error deserialization dispatcher. | |||
Set<StructureShape> errorShapes = HttpProtocolGeneratorUtils.generateErrorDispatcher( | |||
context, operation, responseType, this::writeErrorCodeParser); | |||
context, operation, responseType, this::writeErrorCodeParser, this.isErrorCodeInBody); |
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.
References to this.isErrorCodeInBody
don't need this.
if there's not another variable named that in scope. That should be all cases except the constructor.
@@ -254,7 +264,7 @@ private void generateOperationDeserializer(GenerationContext context, OperationS | |||
|
|||
// Write out the error deserialization dispatcher. | |||
Set<StructureShape> errorShapes = HttpProtocolGeneratorUtils.generateErrorDispatcher( | |||
context, operation, responseType, this::writeErrorCodeParser); | |||
context, operation, responseType, this::writeErrorCodeParser, this.isErrorCodeInBody); |
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.
References to this.isErrorCodeInBody
don't need this.
if there's not another variable named that in scope. That should be all cases except the constructor.
...java/software/amazon/smithy/typescript/codegen/integration/HttpBindingProtocolGenerator.java
Outdated
Show resolved
Hide resolved
if (isBodyParsed) { | ||
// Body is already parsed in error dispatcher, simply assign body to data. | ||
writer.write("const data: any = output.body;"); | ||
return ListUtils.of(); |
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.
This will short circuit deserializing any modeled content in the document of an error response. There is no call to deserializeOutputDocument
and any shapes that may need to be deserialized won't be added to the deserializingDocumentShapes
set here.
...ain/java/software/amazon/smithy/typescript/codegen/integration/HttpRpcProtocolGenerator.java
Outdated
Show resolved
Hide resolved
@@ -621,7 +633,8 @@ private void generateErrorDeserializer(GenerationContext context, StructureShape | |||
}); | |||
|
|||
readHeaders(context, error, bindingIndex); | |||
List<HttpBinding> documentBindings = readResponseBody(context, error, bindingIndex); | |||
List<HttpBinding> documentBindings = readErrorResponseBody( |
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.
Nit: This doesn't need to fall to a new line.
/** | ||
* Creates a Http binding protocol generator. | ||
* | ||
* @param isErrorCodeInBody A boolean indicates whether error code is located in error response body. |
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.
A boolean that indicates if the error code for the implementing protocol is located in the error response body, meaning this generator will parse the body before attempting to load an error code.
// Prepare error response for parsing error code. If error code needs to be parsed from response body | ||
// 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", |
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.
This object name toggle is used in multiple places, can this logic exist once and be stored?
/** | ||
* Creates a Http RPC protocol generator. | ||
* | ||
* @param isErrorCodeInBody A boolean indicates whether error code is located in error response body. |
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.
See updated text in other file.
...ain/java/software/amazon/smithy/typescript/codegen/integration/HttpRpcProtocolGenerator.java
Outdated
Show resolved
Hide resolved
if (isErrorCodeInBody) { | ||
// Body is already parsed in error dispatcher, simply assign body to data. | ||
writer.write("const data: any = output.body;"); | ||
List<HttpBinding> responseBindings = bindingIndex.getResponseBindings(error).values() |
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.
This will contain all response bindings for the error
shape, use Location.DOCUMENT
to refine this.
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.
Will update this.
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.
3bb25b2
to
cd40b1e
Compare
* [aws#694](aws#694) * [smithy-typescript#66](smithy-lang/smithy-typescript#66) * [smithy-typescript#87](smithy-lang/smithy-typescript#87)
* [#694](#694) * [smithy-typescript#66](smithy-lang/smithy-typescript#66) * [smithy-typescript#87](smithy-lang/smithy-typescript#87)
* [aws#694](aws#694) * [smithy-typescript#66](smithy-lang/smithy-typescript#66) * [smithy-typescript#87](smithy-lang/smithy-typescript#87)
* [aws#694](aws#694) * [smithy-typescript#66](smithy-lang/smithy-typescript#66) * [smithy-typescript#87](smithy-lang/smithy-typescript#87)
* [aws#694](aws#694) * [smithy-typescript#66](smithy-lang/smithy-typescript#66) * [smithy-typescript#87](smithy-lang/smithy-typescript#87)
* [aws#694](aws#694) * [smithy-typescript#66](smithy-lang/smithy-typescript#66) * [smithy-typescript#87](smithy-lang/smithy-typescript#87)
Currently when response body is a payload member, the SDK either keeps reponse intact if it has a streaming trait, or parse the body to 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 String, we shouldn't parse it with XML or JSON parser; We should also treat Blob shape differently as we should encode the binaries into string; Only when shape is Union or Structure can we assume payload is parsable by 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 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.
* add logger middleware and logger class * update logger level setting
reopen #63
Two fixes:
Currently when response body is a payload member, the SDK either
keeps reponse intact if it has a
streaming
trait, 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 String, we shouldn't
parse it with XML or JSON parser; We should also treat
Blob
shapedifferently as we should encode the binaries into string; Only when
shape is
Union
orStructure
can 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 class member in Protocol generator--
isErrorCodeInBody
. When it is true, it means error type flagexists 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;
Example:
When it is false, it means
error type can be inferred without touching response body, then
error deser can access the error response intact.
/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.