Skip to content

Commit

Permalink
Fix server code generation of @httpPayload-bound constrained shapes (
Browse files Browse the repository at this point in the history
…#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
<!--- 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._
  • Loading branch information
david-perez authored and rcoh committed Apr 24, 2023
1 parent 705d71c commit aef9d79
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 5 deletions.
6 changes: 6 additions & 0 deletions CHANGELOG.next.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
15 changes: 15 additions & 0 deletions codegen-core/common-test-models/constraints.smithy
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -311,6 +320,12 @@ structure ConstrainedHttpBoundShapesOperationInputOutput {
enumStringListQuery: ListOfEnumString,
}

structure ConstrainedHttpPayloadBoundShapeOperationInputOutput {
@required
@httpPayload
httpPayloadBoundConstrainedShape: ConA
}

structure QueryParamsTargetingMapOfPatternStringOperationInputOutput {
@httpQueryParams
mapOfPatternString: MapOfPatternString
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit aef9d79

Please sign in to comment.