Unverified Commit a5465b79 authored by david-perez's avatar david-perez Committed by GitHub
Browse files

Render only shape name in server error responses (#2866) 

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._
parent 666c474f

CHANGELOG.next.toml

+22 −0
Original line number Diff line number Diff line
@@ -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"

codegen-core/common-test-models/rest-json-extras.smithy

+2 −6
Original line number Diff line number Diff line
@@ -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",

    }

codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/RestJson.kt

+11 −6
Original line number Diff line number Diff line
@@ -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)

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt

+1 −15
Original line number Diff line number Diff line
@@ -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
@@ -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)

codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerAwsJson.kt

+8 −0
Original line number Diff line number Diff line
@@ -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) {