Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this only affect clients?

Copy link
Contributor Author

@landonxjames landonxjames Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated tests for this only apply to clients: https://github.com/smithy-lang/smithy/pull/2403/files

There are similar tests for the server, but they still indicate that empty headers should not be serialized by the server: https://github.com/smithy-lang/smithy/blob/main/smithy-aws-protocol-tests/model/restJson1/http-headers.smithy#L455

I asked internally about this discrepancy and there didn't seem to be much of a reason for it, so it might be updated in the future to align the two.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a PR to smithy to update the server tests to align: smithy-lang/smithy#2433

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,
Comment on lines +632 to +636
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

) {
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