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

Render only shape name in server error responses #2866

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -664,3 +664,25 @@ message = "The `alb_health_check` module has been moved out of the `plugin` modu
references = ["smithy-rs#2865"]
meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "server" }
author = "david-perez"

[[smithy-rs]]
message = """
[RestJson1](https://awslabs.github.io/smithy/2.0/aws/protocols/aws-restjson1-protocol.html#operation-error-serialization) server SDKs now serialize only the [shape name](https://smithy.io/2.0/spec/model.html#shape-id) in operation error responses. Previously (from versions 0.52.0 to 0.55.4), the full shape ID was rendered.
Example server error response by a smithy-rs server version 0.52.0 until 0.55.4:
```
HTTP/1.1 400 Bad Request
content-type: application/json
x-amzn-errortype: com.example.service#InvalidRequestException
...
```
Example server error response now:
```
HTTP/1.1 400 Bad Request
content-type: application/json
x-amzn-errortype: InvalidRequestException
...
```
"""
references = ["smithy-rs#2866"]
meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "server" }
author = "david-perez"
8 changes: 2 additions & 6 deletions codegen-core/common-test-models/rest-json-extras.smithy
Original file line number Diff line number Diff line change
Expand Up @@ -81,16 +81,12 @@ service RestJsonExtras {
appliesTo: "client",
},
{
documentation: """
Upper case error modeled lower case.
Servers render the full shape ID (including namespace), since some
existing clients rely on it to deserialize the error shape and fail
if only the shape name is present.""",
documentation: "Upper case error modeled lower case.",
id: "ServiceLevelErrorServer",
protocol: "aws.protocols#restJson1",
code: 500,
body: "{}",
headers: { "X-Amzn-Errortype": "aws.protocoltests.restjson#ExtraError" },
headers: { "X-Amzn-Errortype": "ExtraError" },
params: {},
appliesTo: "server",
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,19 @@ open class RestJson(val codegenContext: CodegenContext) : Protocol {
* RestJson1 implementations can denote errors in responses in several ways.
* New server-side protocol implementations MUST use a header field named `X-Amzn-Errortype`.
*
* Note that the spec says that implementations SHOULD strip the error shape ID's namespace.
* However, our server implementation renders the full shape ID (including namespace), since some
* existing clients rely on it to deserialize the error shape and fail if only the shape name is present.
* This is compliant with the spec, see https://github.com/awslabs/smithy/pull/1493.
* See https://github.com/awslabs/smithy/issues/1494 too.
* Note that the spec says that implementations SHOULD strip the error shape ID's namespace
* (see https://smithy.io/2.0/aws/protocols/aws-restjson1-protocol.html#operation-error-serialization):
*
* > The value of this component SHOULD contain only the shape name of the error's Shape ID.
*
* But it's a SHOULD; we could strip the namespace if we wanted to. In fact, we did so in smithy-rs versions
* 0.52.0 to 0.55.4; see:
* - https://github.com/awslabs/smithy-rs/pull/1982
* - https://github.com/awslabs/smithy/pull/1493
* - https://github.com/awslabs/smithy/issues/1494
*/
override fun additionalErrorResponseHeaders(errorShape: StructureShape): List<Pair<String, String>> =
listOf("x-amzn-errortype" to errorShape.id.toString())
listOf("x-amzn-errortype" to errorShape.id.name)

override fun structuredDataParser(): StructuredDataParserGenerator =
JsonParserGenerator(codegenContext, httpBindingResolver, ::restJsonFieldName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -921,15 +921,6 @@ class ServerProtocolTestGenerator(
).asObjectNode().get(),
).build()

private fun fixRestJsonInvalidGreetingError(testCase: HttpResponseTestCase): HttpResponseTestCase =
testCase.toBuilder().putHeader("X-Amzn-Errortype", "aws.protocoltests.restjson#InvalidGreeting").build()

private fun fixRestJsonEmptyComplexErrorWithNoMessage(testCase: HttpResponseTestCase): HttpResponseTestCase =
testCase.toBuilder().putHeader("X-Amzn-Errortype", "aws.protocoltests.restjson#ComplexError").build()

private fun fixRestJsonComplexErrorWithNoMessage(testCase: HttpResponseTestCase): HttpResponseTestCase =
testCase.toBuilder().putHeader("X-Amzn-Errortype", "aws.protocoltests.restjson#ComplexError").build()

// TODO(https://github.com/awslabs/smithy/issues/1506)
private fun fixRestJsonMalformedPatternReDOSString(testCase: HttpMalformedRequestTestCase): HttpMalformedRequestTestCase {
val brokenResponse = testCase.response
Expand Down Expand Up @@ -962,12 +953,7 @@ class ServerProtocolTestGenerator(
)

private val BrokenResponseTests: Map<Pair<String, String>, KFunction1<HttpResponseTestCase, HttpResponseTestCase>> =
// TODO(https://github.com/awslabs/smithy/issues/1494)
mapOf(
Pair(RestJson, "RestJsonInvalidGreetingError") to ::fixRestJsonInvalidGreetingError,
Pair(RestJson, "RestJsonEmptyComplexErrorWithNoMessage") to ::fixRestJsonEmptyComplexErrorWithNoMessage,
Pair(RestJson, "RestJsonComplexErrorWithNoMessage") to ::fixRestJsonComplexErrorWithNoMessage,
)
mapOf()

private val BrokenMalformedRequestTests: Map<Pair<String, String>, KFunction1<HttpMalformedRequestTestCase, HttpMalformedRequestTestCase>> =
// TODO(https://github.com/awslabs/smithy/issues/1506)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,14 @@ class ServerAwsJsonFactory(
/**
* AwsJson requires errors to be serialized in server responses with an additional `__type` field. This
* customization writes the right field depending on the version of the AwsJson protocol.
*
* From the specs:
* - https://smithy.io/2.0/aws/protocols/aws-json-1_0-protocol.html#operation-error-serialization
* - https://smithy.io/2.0/aws/protocols/aws-json-1_1-protocol.html#operation-error-serialization
*
* > Error responses in the <awsJson1_x> protocol are serialized identically to standard responses with one additional
* > component to distinguish which error is contained. New server-side protocol implementations SHOULD use a body
* > field named __type
*/
class ServerAwsJsonError(private val awsJsonVersion: AwsJsonVersion) : JsonSerializerCustomization() {
override fun section(section: JsonSerializerSection): Writable = when (section) {
Expand Down