From aef9d7987949e829f35c80802e11e0f991ca508c Mon Sep 17 00:00:00 2001 From: david-perez Date: Wed, 19 Apr 2023 11:48:34 +0200 Subject: [PATCH] Fix server code generation of `@httpPayload`-bound constrained shapes (#2584) The issue is we're not changing the return type of the payload deserializing function to be the unconstrained type (e.g. the builder in case of an `@httpPayload`-bound structure shape) when the shape is constrained. Yet another example of why code-generating `constraints.smithy` (see #2101) is important. Closes #2583. ## Testing The added integration test operation fails to compile without this patch. ## Checklist - [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._ --- CHANGELOG.next.toml | 6 ++++++ .../common-test-models/constraints.smithy | 15 +++++++++++++++ .../generators/http/HttpBindingGenerator.kt | 2 +- .../HttpBoundProtocolPayloadGenerator.kt | 6 +++++- .../smithy/protocols/parse/JsonParserGenerator.kt | 7 ++++--- 5 files changed, 31 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index a428a3186e..7992dd6e5c 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -122,3 +122,9 @@ message = "Fix generation of constrained shapes reaching `@sensitive` shapes" references = ["smithy-rs#2582", "smithy-rs#2585"] meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "server" } author = "david-perez" + +[[smithy-rs]] +message = "Fix server code generation bug affecting constrained shapes bound with `@httpPayload`" +references = ["smithy-rs#2583", "smithy-rs#2584"] +meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "server" } +author = "david-perez" diff --git a/codegen-core/common-test-models/constraints.smithy b/codegen-core/common-test-models/constraints.smithy index bac10a0c15..a2add6c86a 100644 --- a/codegen-core/common-test-models/constraints.smithy +++ b/codegen-core/common-test-models/constraints.smithy @@ -12,7 +12,9 @@ service ConstraintsService { operations: [ ConstrainedShapesOperation, ConstrainedHttpBoundShapesOperation, + ConstrainedHttpPayloadBoundShapeOperation, ConstrainedRecursiveShapesOperation, + // `httpQueryParams` and `httpPrefixHeaders` are structurually // exclusive, so we need one operation per target shape type // combination. @@ -59,6 +61,13 @@ operation ConstrainedHttpBoundShapesOperation { errors: [ValidationException] } +@http(uri: "/constrained-http-payload-bound-shape-operation", method: "POST") +operation ConstrainedHttpPayloadBoundShapeOperation { + input: ConstrainedHttpPayloadBoundShapeOperationInputOutput, + output: ConstrainedHttpPayloadBoundShapeOperationInputOutput, + errors: [ValidationException] +} + @http(uri: "/constrained-recursive-shapes-operation", method: "POST") operation ConstrainedRecursiveShapesOperation { input: ConstrainedRecursiveShapesOperationInputOutput, @@ -311,6 +320,12 @@ structure ConstrainedHttpBoundShapesOperationInputOutput { enumStringListQuery: ListOfEnumString, } +structure ConstrainedHttpPayloadBoundShapeOperationInputOutput { + @required + @httpPayload + httpPayloadBoundConstrainedShape: ConA +} + structure QueryParamsTargetingMapOfPatternStringOperationInputOutput { @httpQueryParams mapOfPatternString: MapOfPatternString diff --git a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/http/HttpBindingGenerator.kt b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/http/HttpBindingGenerator.kt index 881d0f77af..7268dfd12a 100644 --- a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/http/HttpBindingGenerator.kt +++ b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/generators/http/HttpBindingGenerator.kt @@ -235,7 +235,7 @@ class HttpBindingGenerator( // The output needs to be Optional when deserializing the payload body or the caller signature // will not match. val outputT = symbolProvider.toSymbol(binding.member).makeOptional() - rustBlock("pub fn $fnName(body: &[u8]) -> std::result::Result<#T, #T>", outputT, errorSymbol) { + rustBlock("pub(crate) fn $fnName(body: &[u8]) -> std::result::Result<#T, #T>", outputT, errorSymbol) { deserializePayloadBody( binding, errorSymbol, diff --git a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/HttpBoundProtocolPayloadGenerator.kt b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/HttpBoundProtocolPayloadGenerator.kt index 10d4b83cec..6d4e7bf850 100644 --- a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/HttpBoundProtocolPayloadGenerator.kt +++ b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/HttpBoundProtocolPayloadGenerator.kt @@ -239,7 +239,11 @@ class HttpBoundProtocolPayloadGenerator( ) { val ref = if (payloadMetadata.takesOwnership) "" else "&" val serializer = protocolFunctions.serializeFn(member, fnNameSuffix = "http_payload") { fnName -> - val outputT = if (member.isStreaming(model)) symbolProvider.toSymbol(member) else RuntimeType.ByteSlab.toSymbol() + val outputT = if (member.isStreaming(model)) { + symbolProvider.toSymbol(member) + } else { + RuntimeType.ByteSlab.toSymbol() + } rustBlockTemplate( "pub fn $fnName(payload: $ref#{Member}) -> Result<#{outputT}, #{BuildError}>", "Member" to symbolProvider.toSymbol(member), diff --git a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGenerator.kt b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGenerator.kt index 1566d84bf3..01c2b64e6a 100644 --- a/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGenerator.kt +++ b/codegen-core/src/main/kotlin/software/amazon/smithy/rust/codegen/core/smithy/protocols/parse/JsonParserGenerator.kt @@ -154,14 +154,15 @@ class JsonParserGenerator( override fun payloadParser(member: MemberShape): RuntimeType { val shape = model.expectShape(member.target) + val returnSymbolToParse = returnSymbolToParse(shape) check(shape is UnionShape || shape is StructureShape || shape is DocumentShape) { - "payload parser should only be used on structures & unions" + "Payload parser should only be used on structure shapes, union shapes, and document shapes." } return protocolFunctions.deserializeFn(shape, fnNameSuffix = "payload") { fnName -> rustBlockTemplate( - "pub fn $fnName(input: &[u8]) -> Result<#{Shape}, #{Error}>", + "pub(crate) fn $fnName(input: &[u8]) -> Result<#{ReturnType}, #{Error}>", *codegenScope, - "Shape" to symbolProvider.toSymbol(shape), + "ReturnType" to returnSymbolToParse.symbol, ) { val input = if (shape is DocumentShape) { "input"