From 131aff316a8307baf64e7af03684f4fdb583498c Mon Sep 17 00:00:00 2001 From: david-perez Date: Mon, 14 Nov 2022 16:57:50 +0100 Subject: [PATCH] Render full shape IDs in server error responses 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. --- .../common-test-models/rest-json-extras.smithy | 8 ++++++-- .../rust/codegen/core/smithy/protocols/RestJson.kt | 8 +++++++- .../protocol/ServerProtocolTestGenerator.kt | 14 +++++++++++++- .../protocols/ServerHttpBoundProtocolGenerator.kt | 3 ++- 4 files changed, 28 insertions(+), 5 deletions(-) diff --git a/codegen-core/common-test-models/rest-json-extras.smithy b/codegen-core/common-test-models/rest-json-extras.smithy index 10224b0c7cf..70472b4ca97 100644 --- a/codegen-core/common-test-models/rest-json-extras.smithy +++ b/codegen-core/common-test-models/rest-json-extras.smithy @@ -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", } 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 7a25fabfdb6..60029cdaf0e 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 @@ -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> = - 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) 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 01120f4fc9b..a83a8a3c6fb 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 @@ -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. @@ -1270,6 +1277,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 2cf86be5eb1..d0ed29357c2 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 @@ -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 @@ -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,