Skip to content

Commit

Permalink
Render full shape IDs in server error responses
Browse files Browse the repository at this point in the history
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
smithy-lang/smithy#1493.

See smithy-lang/smithy#1494 too.
  • Loading branch information
david-perez committed Nov 14, 2022
1 parent 7477cfb commit 131aff3
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 5 deletions.
8 changes: 6 additions & 2 deletions codegen-core/common-test-models/rest-json-extras.smithy
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,16 @@ service RestJsonExtras {
appliesTo: "client",
},
{
documentation: "Upper case error modeled lower case",
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.""",
id: "ServiceLevelErrorServer",
protocol: "aws.protocols#restJson1",
code: 500,
body: "{}",
headers: { "X-Amzn-Errortype": "ExtraError" },
headers: { "X-Amzn-Errortype": "aws.protocoltests.restjson#ExtraError" },
params: {},
appliesTo: "server",
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,15 @@ 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.
*/
override fun additionalErrorResponseHeaders(errorShape: StructureShape): List<Pair<String, String>> =
listOf("x-amzn-errortype" to errorShape.id.name)
listOf("x-amzn-errortype" to errorShape.id.toString())

override fun structuredDataParser(operationShape: OperationShape): StructuredDataParserGenerator =
JsonParserGenerator(codegenContext, httpBindingResolver, ::restJsonFieldName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1194,6 +1194,13 @@ class ServerProtocolTestGenerator(
): HttpRequestTestCase =
testCase.toBuilder().putHeader("x-amz-target", "JsonProtocol.${operationShape.id.name}").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()

// These are tests whose definitions in the `awslabs/smithy` repository are wrong.
// This is because they have not been written from a server perspective, and as such the expected `params` field is incomplete.
// TODO(https://github.com/awslabs/smithy-rs/issues/1288): Contribute a PR to fix them upstream.
Expand Down Expand Up @@ -1270,6 +1277,11 @@ class ServerProtocolTestGenerator(
)

private val BrokenResponseTests: Map<Pair<String, String>, KFunction1<HttpResponseTestCase, HttpResponseTestCase>> =
mapOf()
// TODO(https://github.com/awslabs/smithy/issues/1494)
mapOf(
Pair(RestJson, "RestJsonInvalidGreetingError") to ::fixRestJsonInvalidGreetingError,
Pair(RestJson, "RestJsonEmptyComplexErrorWithNoMessage") to ::fixRestJsonEmptyComplexErrorWithNoMessage,
Pair(RestJson, "RestJsonComplexErrorWithNoMessage") to ::fixRestJsonComplexErrorWithNoMessage,
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import software.amazon.smithy.rust.codegen.core.rustlang.RustWriter
import software.amazon.smithy.rust.codegen.core.rustlang.Writable
import software.amazon.smithy.rust.codegen.core.rustlang.asType
import software.amazon.smithy.rust.codegen.core.rustlang.conditionalBlock
import software.amazon.smithy.rust.codegen.core.rustlang.escape
import software.amazon.smithy.rust.codegen.core.rustlang.render
import software.amazon.smithy.rust.codegen.core.rustlang.rust
import software.amazon.smithy.rust.codegen.core.rustlang.rustBlock
Expand Down Expand Up @@ -635,7 +636,7 @@ private class ServerHttpBoundProtocolTraitImplGenerator(
builder = #{header_util}::set_response_header_if_absent(
builder,
http::header::HeaderName::from_static("$headerName"),
"$headerValue"
"${escape(headerValue)}"
);
""",
*codegenScope,
Expand Down

0 comments on commit 131aff3

Please sign in to comment.