Skip to content

Commit

Permalink
Render only shape name in server error responses (#2866)
Browse files Browse the repository at this point in the history
This essentially reverts the behavior introduced in #1982.

The rationale for that change:

> [...] since some existing clients rely on it to deserialize the error
shape
> and fail if only the shape name is present

no longer holds. Since serializing only the shape name is a SHOULD in
the spec, it's best that we honor it.

## Checklist
<!--- If a checkbox below is not applicable, then please DELETE it
rather than leaving it unchecked -->
- [x] I have updated `CHANGELOG.next.toml` if I made changes to the
smithy-rs codegen or runtime crates

----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._
  • Loading branch information
david-perez authored Jul 25, 2023
1 parent 666c474 commit a5465b7
Show file tree
Hide file tree
Showing 5 changed files with 44 additions and 27 deletions.
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

0 comments on commit a5465b7

Please sign in to comment.