From c187e3d5e063aa41828071761ffc2ecb69758197 Mon Sep 17 00:00:00 2001 From: david-perez Date: Thu, 17 Nov 2022 17:51:51 +0100 Subject: [PATCH] Render full shape IDs in server error responses (#1982) 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. --- CHANGELOG.next.toml | 26 +++++++++++++++++++ .../rest-json-extras.smithy | 8 ++++-- .../codegen/core/smithy/protocols/RestJson.kt | 8 +++++- .../protocol/ServerProtocolTestGenerator.kt | 14 +++++++++- .../ServerHttpBoundProtocolGenerator.kt | 4 ++- 5 files changed, 55 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index ab05df0124..ccd59ef313 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -431,3 +431,29 @@ message = "`Endpoint::immutable` now takes `impl AsRef` instead of `Uri`. F references = ["smithy-rs#1984", "smithy-rs#1496"] meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "client" } author = "jdisanti" + +[[smithy-rs]] +message = """ +[RestJson1](https://awslabs.github.io/smithy/2.0/aws/protocols/aws-restjson1-protocol.html#operation-error-serialization) server SDKs now serialize the [full shape ID](https://smithy.io/2.0/spec/model.html#shape-id) (including namespace) in operation error responses. + +Example server error response before: + +``` +HTTP/1.1 400 Bad Request +content-type: application/json +x-amzn-errortype: InvalidRequestException +... +``` + +Example server error response now: + +``` +HTTP/1.1 400 Bad Request +content-type: application/json +x-amzn-errortype: com.example.service#InvalidRequestException +... +``` +""" +references = ["smithy-rs#1982"] +meta = { "breaking" = true, "tada" = false, "bug" = false, "target" = "server" } +author = "david-perez" diff --git a/codegen-core/common-test-models/rest-json-extras.smithy b/codegen-core/common-test-models/rest-json-extras.smithy index 65b7fcc8f1..b9946baa81 100644 --- a/codegen-core/common-test-models/rest-json-extras.smithy +++ b/codegen-core/common-test-models/rest-json-extras.smithy @@ -81,12 +81,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", } diff --git a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/RestJson.kt b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/RestJson.kt index aa8e0f84b9..0a7234f548 100644 --- a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/RestJson.kt +++ b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/RestJson.kt @@ -83,9 +83,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> = - listOf("x-amzn-errortype" to errorShape.id.name) + listOf("x-amzn-errortype" to errorShape.id.toString()) override fun structuredDataParser(operationShape: OperationShape): StructuredDataParserGenerator { fun builderSymbol(shape: StructureShape): Symbol = diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt index 93ae58ce98..a865f27220 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt @@ -1170,6 +1170,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. @@ -1246,6 +1253,11 @@ class ServerProtocolTestGenerator( ) private val BrokenResponseTests: Map, KFunction1> = - 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, + ) } } diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt index fa45a73ffd..a8a6e1608f 100644 --- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt +++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt @@ -33,6 +33,8 @@ 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 import software.amazon.smithy.rust.codegen.core.rustlang.rustBlockTemplate @@ -636,7 +638,7 @@ private class ServerHttpBoundProtocolTraitImplGenerator( builder = #{header_util}::set_response_header_if_absent( builder, http::header::HeaderName::from_static("$headerName"), - "$headerValue" + "${escape(headerValue)}" ); """, *codegenScope,