Skip to content

Commit

Permalink
Bump smithy version to 1.52 (#3887)
Browse files Browse the repository at this point in the history
## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here -->
Bumping smithy to 1.52 for an `httpChecksum` trait bugfix

A few fixes had to be made to protocol tests for this release:
* Update document to initialize as empty `{}` instead of null
* Update our header serialization to serialize present but empty headers
to `""` (Note that this change does not apply to server header
serialization)
* Add `accept: application/cbor` header to add rpcV2Cbor requests
* Skip two broken tests. These tests were fixed in
smithy-lang/smithy#2423 and that change was
included in the 1.52 release notes, but the actual change did not make
it into the build artifact. The changes should make it into the 1.52.1
release in the next few days:
smithy-lang/smithy#2428

_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
landonxjames authored Oct 24, 2024
1 parent 2e53910 commit 8d8e7ab
Show file tree
Hide file tree
Showing 6 changed files with 94 additions and 34 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ apply SayHello @httpResponseTests([{
listValue: [],
mapValue: {},
doubleListValue: []
document: null
document: {}
nested: { a: "" }
},
code: 200,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ class RequestBindingGenerator(
*/
fun renderUpdateHttpBuilder(implBlockWriter: RustWriter) {
uriBase(implBlockWriter)
val addHeadersFn = httpBindingGenerator.generateAddHeadersFn(operationShape)
val addHeadersFn = httpBindingGenerator.generateAddHeadersFn(operationShape, serializeEmptyHeaders = true)
val hasQuery = uriQuery(implBlockWriter)
Attribute.AllowClippyUnnecessaryWraps.render(implBlockWriter)
implBlockWriter.rustBlockTemplate(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

package software.amazon.smithy.rust.codegen.client.smithy.generators.protocol

import software.amazon.smithy.model.node.NumberNode
import software.amazon.smithy.model.shapes.DoubleShape
import software.amazon.smithy.model.shapes.FloatShape
import software.amazon.smithy.model.shapes.OperationShape
Expand All @@ -28,6 +27,7 @@ import software.amazon.smithy.rust.codegen.core.smithy.generators.protocol.Broke
import software.amazon.smithy.rust.codegen.core.smithy.generators.protocol.FailingTest
import software.amazon.smithy.rust.codegen.core.smithy.generators.protocol.ProtocolSupport
import software.amazon.smithy.rust.codegen.core.smithy.generators.protocol.ProtocolTestGenerator
import software.amazon.smithy.rust.codegen.core.smithy.generators.protocol.ServiceShapeId
import software.amazon.smithy.rust.codegen.core.smithy.generators.protocol.ServiceShapeId.AWS_JSON_10
import software.amazon.smithy.rust.codegen.core.smithy.generators.protocol.ServiceShapeId.REST_JSON
import software.amazon.smithy.rust.codegen.core.smithy.generators.protocol.ServiceShapeId.RPC_V2_CBOR
Expand Down Expand Up @@ -83,20 +83,54 @@ class ClientProtocolTestGenerator(
)

private val BrokenTests:
Set<BrokenTest> = setOf()
Set<BrokenTest> =
// The two tests below were fixed in "https://github.com/smithy-lang/smithy/pull/2423", but the fixes didn't make
// it into the build artifact for 1.52
setOf(
BrokenTest.ResponseTest(
ServiceShapeId.REST_XML,
"NestedXmlMapWithXmlNameDeserializes",
howToFixItFn = ::fixRestXMLInvalidRootNodeResponse,
inAtLeast = setOf("1.52.0"),
trackedIn =
setOf(
"https://github.com/smithy-lang/smithy/pull/2423",
),
),
BrokenTest.RequestTest(
ServiceShapeId.REST_XML,
"NestedXmlMapWithXmlNameSerializes",
howToFixItFn = ::fixRestXMLInvalidRootNodeRequest,
inAtLeast = setOf("1.52.0"),
trackedIn =
setOf(
"https://github.com/smithy-lang/smithy/pull/2423",
),
),
)

private fun fixRestJsonClientIgnoresDefaultValuesIfMemberValuesArePresentInResponse(
testCase: TestCase.ResponseTest,
): TestCase.ResponseTest {
val fixedParams =
testCase.testCase.params.toBuilder().withMember("defaultTimestamp", NumberNode.from(2)).build()
private fun fixRestXMLInvalidRootNodeResponse(testCase: TestCase.ResponseTest): TestCase.ResponseTest {
val fixedBody =
testCase.testCase.body.get()
.replace("NestedXmlMapWithXmlNameResponse", "NestedXmlMapWithXmlNameInputOutput")
return TestCase.ResponseTest(
testCase.testCase.toBuilder()
.params(fixedParams)
.body(fixedBody)
.build(),
testCase.targetShape,
)
}

private fun fixRestXMLInvalidRootNodeRequest(testCase: TestCase.RequestTest): TestCase.RequestTest {
val fixedBody =
testCase.testCase.body.get()
.replace("NestedXmlMapWithXmlNameRequest", "NestedXmlMapWithXmlNameInputOutput")
return TestCase.RequestTest(
testCase.testCase.toBuilder()
.body(fixedBody)
.build(),
)
}
}

override val appliesTo: AppliesTo
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import software.amazon.smithy.rust.codegen.core.rustlang.RustType
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.asOptional
import software.amazon.smithy.rust.codegen.core.rustlang.conditionalBlock
import software.amazon.smithy.rust.codegen.core.rustlang.qualifiedName
import software.amazon.smithy.rust.codegen.core.rustlang.render
import software.amazon.smithy.rust.codegen.core.rustlang.rust
Expand Down Expand Up @@ -491,6 +492,7 @@ class HttpBindingGenerator(
fun generateAddHeadersFn(
shape: Shape,
httpMessageType: HttpMessageType = HttpMessageType.REQUEST,
serializeEmptyHeaders: Boolean = false,
): RuntimeType? {
val (headerBindings, prefixHeaderBinding) =
when (httpMessageType) {
Expand Down Expand Up @@ -538,7 +540,7 @@ class HttpBindingGenerator(
""",
*codegenScope,
) {
headerBindings.forEach { httpBinding -> renderHeaders(httpBinding) }
headerBindings.forEach { httpBinding -> renderHeaders(httpBinding, serializeEmptyHeaders) }
if (prefixHeaderBinding != null) {
renderPrefixHeader(prefixHeaderBinding)
}
Expand All @@ -547,7 +549,10 @@ class HttpBindingGenerator(
}
}

private fun RustWriter.renderHeaders(httpBinding: HttpBinding) {
private fun RustWriter.renderHeaders(
httpBinding: HttpBinding,
serializeEmptyHeaders: Boolean,
) {
check(httpBinding.location == HttpLocation.HEADER)
val memberShape = httpBinding.member
val targetShape = model.expectShape(memberShape.target)
Expand Down Expand Up @@ -585,6 +590,7 @@ class HttpBindingGenerator(
targetShape,
timestampFormat,
renderErrorMessage,
serializeEmptyHeaders,
)
} else {
renderHeaderValue(
Expand All @@ -596,6 +602,7 @@ class HttpBindingGenerator(
renderErrorMessage,
serializeIfDefault = memberSymbol.isOptional(),
memberShape,
serializeEmptyHeaders,
)
}
}
Expand All @@ -608,6 +615,7 @@ class HttpBindingGenerator(
shape: CollectionShape,
timestampFormat: TimestampFormatTrait.Format,
renderErrorMessage: (String) -> Writable,
serializeEmptyHeaders: Boolean,
) {
val loopVariable = ValueExpression.Reference(safeName("inner"))
val context = HeaderValueSerializationContext(value, shape)
Expand All @@ -617,17 +625,29 @@ class HttpBindingGenerator(
)(this)
}

rustBlock("for ${loopVariable.name} in ${context.valueExpression.asRef()}") {
this.renderHeaderValue(
headerName,
loopVariable,
model.expectShape(shape.member.target),
isMultiValuedHeader = true,
timestampFormat,
renderErrorMessage,
serializeIfDefault = true,
shape.member,
)
// Conditionally wrap the header generation in a block that handles empty header values if
// `serializeEmptyHeaders` is true
conditionalBlock(
"""
// Empty vec in header is serialized as an empty string
if ${context.valueExpression.name}.is_empty() {
builder = builder.header("$headerName", "");
} else {""",
"}", conditional = serializeEmptyHeaders,
) {
rustBlock("for ${loopVariable.name} in ${context.valueExpression.asRef()}") {
this.renderHeaderValue(
headerName,
loopVariable,
model.expectShape(shape.member.target),
isMultiValuedHeader = true,
timestampFormat,
renderErrorMessage,
serializeIfDefault = true,
shape.member,
serializeEmptyHeaders,
)
}
}
}

Expand All @@ -647,6 +667,7 @@ class HttpBindingGenerator(
renderErrorMessage: (String) -> Writable,
serializeIfDefault: Boolean,
memberShape: MemberShape,
serializeEmptyHeaders: Boolean,
) {
val context = HeaderValueSerializationContext(value, shape)
for (customization in customizations) {
Expand All @@ -669,20 +690,24 @@ class HttpBindingGenerator(
isMultiValuedHeader = isMultiValuedHeader,
)
val safeName = safeName("formatted")
rustTemplate(
"""
let $safeName = $formatted;
if !$safeName.is_empty() {

// If `serializeEmptyHeaders` is false we wrap header serialization in a `!foo.is_empty()` check and skip
// serialization if the header value is empty
rust("let $safeName = $formatted;")
conditionalBlock("if !$safeName.is_empty() {", "}", conditional = !serializeEmptyHeaders) {
rustTemplate(
"""
let header_value = $safeName;
let header_value: #{HeaderValue} = header_value.parse().map_err(|err| {
#{invalid_field_error:W}
})?;
builder = builder.header("$headerName", header_value);
}
""",
"HeaderValue" to RuntimeType.Http.resolve("HeaderValue"),
"invalid_field_error" to renderErrorMessage("header_value"),
)
""",
"HeaderValue" to RuntimeType.Http.resolve("HeaderValue"),
"invalid_field_error" to renderErrorMessage("header_value"),
)
}
}
if (serializeIfDefault) {
block(context.valueExpression)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,9 @@ open class RpcV2Cbor(val codegenContext: CodegenContext) : Protocol {
// using floating point seconds from the epoch.
override val defaultTimestampFormat: TimestampFormatTrait.Format = TimestampFormatTrait.Format.EPOCH_SECONDS

// The accept header is required by the spec (and by all of the protocol tests)
override fun additionalRequestHeaders(operationShape: OperationShape): List<Pair<String, String>> =
listOf("smithy-protocol" to "rpc-v2-cbor")
listOf("smithy-protocol" to "rpc-v2-cbor", "accept" to "application/cbor")

override fun additionalResponseHeaders(operationShape: OperationShape): List<Pair<String, String>> =
listOf("smithy-protocol" to "rpc-v2-cbor")
Expand Down
2 changes: 1 addition & 1 deletion gradle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ smithy.rs.runtime.crate.unstable.version=0.60.6
kotlin.code.style=official
# codegen
smithyGradlePluginVersion=0.9.0
smithyVersion=1.51.0
smithyVersion=1.52.0
allowLocalDeps=false
# kotlin
kotlinVersion=1.9.20
Expand Down

0 comments on commit 8d8e7ab

Please sign in to comment.