Skip to content

fix: address various failing protocol tests #1223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Jan 23, 2025
Merged

fix: address various failing protocol tests #1223

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
95e1480
Allow serializing empty headers
lauzadis Jan 17, 2025
272c1c4
Add explanations for other failing protocol tests
lauzadis Jan 21, 2025
0b506f3
Add `@required` back
lauzadis Jan 21, 2025
6db0e69
Fill in correct default values
lauzadis Jan 21, 2025
b78964b
Revert "Fill in correct default values"
lauzadis Jan 21, 2025
48350f2
Revert "Add `@required` back"
lauzadis Jan 21, 2025
ed41af7
Add explanation to why `@required` is removed
lauzadis Jan 21, 2025
1333c24
Upgrade to Smithy v1.54.0
lauzadis Jan 21, 2025
3333049
Base64-decode default blob values
lauzadis Jan 21, 2025
0bab197
Fix test
lauzadis Jan 21, 2025
9f0ffe6
Add support for unpadded Base64 inputs
lauzadis Jan 21, 2025
514455c
git push
lauzadis Jan 21, 2025
81dc2a1
Re-enable `RpcV2CborClientPopulatesDefaultValuesInInput` test
lauzadis Jan 21, 2025
3149027
Revert `@required` changes
lauzadis Jan 21, 2025
04a33e9
`Symbol.Builder.addReferences`
lauzadis Jan 22, 2025
189edc7
Revert `@required` removal
lauzadis Jan 22, 2025
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
12 changes: 6 additions & 6 deletions codegen/protocol-tests/model/error-correction-tests.smithy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,9 @@ operation SayHelloXml { output: TestOutput, errors: [Error] }

structure TestOutputDocument with [TestStruct] {
innerField: Nested,
// FIXME: This trait fails smithy validator
// @required

// Note: This shape _should_ be @required, but causes Smithy httpResponseTests validation to fail.
// We expect `nested` to be deserialized as `null` and enforce @required using a runtime check, but Smithy validator doesn't recognize / allow this.
document: Document
}
structure TestOutput with [TestStruct] { innerField: Nested }
Expand All @@ -65,8 +66,8 @@ structure TestStruct {
@required
nestedListValue: NestedList

// FIXME: This trait fails smithy validator
// @required
// Note: This shape _should_ be @required, but causes Smithy httpResponseTests validation to fail.
// We expect `nested` to be deserialized as `null` and enforce @required using a runtime check, but Smithy validator doesn't recognize / allow this.
nested: Nested

@required
Expand Down Expand Up @@ -97,8 +98,7 @@ union MyUnion {
}

structure Nested {
// FIXME: This trait fails smithy validator
// @required
@required
a: String
}

Expand Down
6 changes: 1 addition & 5 deletions ...ftware/amazon/smithy/kotlin/codegen/aws/protocols/core/AwsHttpBindingProtocolGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,7 @@ abstract class AwsHttpBindingProtocolGenerator : HttpBindingProtocolGenerator()

val ignoredTests = TestMemberDelta(
setOf(
"AwsJson10ClientErrorCorrectsWithDefaultValuesWhenServerFailsToSerializeRequiredValues",
"RestJsonNullAndEmptyHeaders",
"NullAndEmptyHeaders",
"RpcV2CborClientPopulatesDefaultsValuesWhenMissingInResponse",
"RpcV2CborClientPopulatesDefaultValuesInInput",
"RpcV2CborClientPopulatesDefaultValuesInInput", // FIXME Bug in protocol test, malformed Base64 body. Fix will be in the next Smithy release: https://github.com/smithy-lang/smithy/pull/2502
),
)

Expand Down
2 changes: 1 addition & 1 deletion ...odegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/core/KotlinSymbolProvider.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ class KotlinSymbolProvider(private val model: Model, private val settings: Kotli
.takeUnless { it.isEmpty() }
?.let { "ByteStream.fromString(${it.dq()})" }

targetShape.isBlobShape -> "${node.toString().dq()}.encodeToByteArray()"
targetShape.isBlobShape -> "${node.toString().dq()}.decodeBase64().encodeToByteArray()"
Copy link
Contributor

@ianbotsf ianbotsf Jan 21, 2025

Choose a reason for hiding this comment

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

Question: Don't we need this for streaming targets too? (i.e., the when branch above this one)


targetShape.isDocumentShape -> getDefaultValueForDocument(node)
targetShape.isTimestampShape -> getDefaultValueForTimestamp(node.asNumberNode().get())
Expand Down
5 changes: 5 additions & 0 deletions ...gen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/StructureGenerator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,11 @@ class StructureGenerator(
} else {
memberSymbol
}

if (builderMemberSymbol.shape is BlobShape && builderMemberSymbol.isNotNullable) {
writer.addImport(RuntimeTypes.Core.Text.Encoding.decodeBase64)
}
Copy link
Contributor Author

@lauzadis lauzadis Jan 21, 2025

Choose a reason for hiding this comment

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

decodeBase64 is used in KotlinSymbolProvider, but we don't have access to the KotlinWriter there, so the import gets added here if necessary. Any other ideas to work around this?

Copy link
Contributor

@ianbotsf ianbotsf Jan 21, 2025

Choose a reason for hiding this comment

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

Correct, we don't have a KotlinWriter (nor should we) in KotlinSymbolProvider. But we're still providing symbols and symbols can declare additional imports. We should be setting up those references before returning from the symbol provider.

This probably means a bit of a light refactor in the symbol provider since it's mostly slinging text strings around and wrapping it into a symbol right before returning. But we probably need better awareness of non-textual symbol info in places like getDefaultValue.

Copy link
Contributor Author

@lauzadis lauzadis Jan 22, 2025

Choose a reason for hiding this comment

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

I did a minimal refactor to give getDefaultValue access to the Symbol being built, calling addReferences where appropriate


write("public var #L: #E", memberName, builderMemberSymbol)
}
write("")
Expand Down
19 changes: 3 additions & 16 deletions ...software/amazon/smithy/kotlin/codegen/rendering/protocol/HttpStringValuesMapSerializer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,9 +157,8 @@ class HttpStringValuesMapSerializer(
val paramName = binding.locationName
// addAll collection parameter 2
val param2 = if (mapFnContents.isEmpty()) "input.$memberName" else "input.$memberName.map { $mapFnContents }"
val nullCheck = if (memberSymbol.isNullable) "?" else ""
writer.write(
"if (input.#L$nullCheck.isNotEmpty() == true) #L(#S, #L)",
"if (input.#L != null) #L(#S, #L)",
memberName,
binding.location.addAllFnName,
paramName,
Expand All @@ -174,8 +173,7 @@ class HttpStringValuesMapSerializer(
val paramName = binding.locationName
val memberSymbol = symbolProvider.toSymbol(binding.member)

// NOTE: query parameters are allowed to be empty, whereas headers should omit empty string
// values from serde
// NOTE: query parameters are allowed to be empty
if ((location == HttpBinding.Location.QUERY || location == HttpBinding.Location.HEADER) && binding.member.hasTrait<IdempotencyTokenTrait>()) {
// Call the idempotency token function if no supplied value.
writer.addImport(RuntimeTypes.SmithyClient.IdempotencyTokenProviderExt)
Expand All @@ -185,18 +183,7 @@ class HttpStringValuesMapSerializer(
paramName,
)
} else {
val nullCheck =
if (location == HttpBinding.Location.QUERY ||
memberTarget.hasTrait<
@Suppress("DEPRECATION")
software.amazon.smithy.model.traits.EnumTrait,
>()
) {
if (memberSymbol.isNullable) "input.$memberName != null" else ""
} else {
val nullCheck = if (memberSymbol.isNullable) "?" else ""
"input.$memberName$nullCheck.isNotEmpty() == true"
}
val nullCheck = if (memberSymbol.isNullable) "input.$memberName != null" else ""

val cond = defaultCheck(binding.member) ?: nullCheck

Expand Down
2 changes: 1 addition & 1 deletion ...-codegen/src/test/kotlin/software/amazon/smithy/kotlin/codegen/core/SymbolProviderTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ class SymbolProviderTest {
"double,2.71828,2.71828",
"byte,10,10.toByte()",
"string,\"hello\",\"hello\"",
"blob,\"abcdefg\",\"abcdefg\".encodeToByteArray()",
"blob,\"abcdefg\",\"abcdefg\".decodeBase64().encodeToByteArray()",
"boolean,true,true",
"bigInteger,5,5",
"bigDecimal,9.0123456789,9.0123456789",
Expand Down
8 changes: 4 additions & 4 deletions ...tware/amazon/smithy/kotlin/codegen/rendering/protocol/HttpBindingProtocolGeneratorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ internal class SmokeTestOperationSerializer: HttpSerializer.NonStreaming<SmokeTe
}

builder.headers {
if (input.header1?.isNotEmpty() == true) append("X-Header1", input.header1)
if (input.header2?.isNotEmpty() == true) append("X-Header2", input.header2)
if (input.header1 != null) append("X-Header1", input.header1)
if (input.header2 != null) append("X-Header2", input.header2)
}

val payload = serializeSmokeTestOperationBody(context, input)
Expand Down Expand Up @@ -264,7 +264,7 @@ internal class TimestampInputOperationSerializer: HttpSerializer.NonStreaming<Ti
}
parameters.decodedParameters(PercentEncoding.SmithyLabel) {
if (input.queryTimestamp != null) add("qtime", input.queryTimestamp.format(TimestampFormat.ISO_8601))
if (input.queryTimestampList?.isNotEmpty() == true) addAll("qtimeList", input.queryTimestampList.map { it.format(TimestampFormat.ISO_8601) })
if (input.queryTimestampList != null) addAll("qtimeList", input.queryTimestampList.map { it.format(TimestampFormat.ISO_8601) })
}
}

Expand Down Expand Up @@ -304,7 +304,7 @@ internal class BlobInputOperationSerializer: HttpSerializer.NonStreaming<BlobInp
}

builder.headers {
if (input.headerMediaType?.isNotEmpty() == true) append("X-Blob", input.headerMediaType.encodeBase64())
if (input.headerMediaType != null) append("X-Blob", input.headerMediaType.encodeBase64())
}

val payload = serializeBlobInputOperationBody(context, input)
Expand Down
16 changes: 8 additions & 8 deletions ...ware/amazon/smithy/kotlin/codegen/rendering/protocol/HttpStringValuesMapSerializerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,8 @@ class HttpStringValuesMapSerializerTest {
contents.assertBalancedBracesAndParens()

val expectedContents = """
if (input.header1?.isNotEmpty() == true) append("X-Header1", input.header1)
if (input.header2?.isNotEmpty() == true) append("X-Header2", input.header2)
if (input.header1 != null) append("X-Header1", input.header1)
if (input.header2 != null) append("X-Header2", input.header2)
""".trimIndent()
contents.shouldContainOnlyOnceWithDiff(expectedContents)
}
Expand All @@ -157,7 +157,7 @@ class HttpStringValuesMapSerializerTest {
contents.assertBalancedBracesAndParens()

val expectedContents = """
if (input.headerMediaType?.isNotEmpty() == true) append("X-Blob", input.headerMediaType.encodeBase64())
if (input.headerMediaType != null) append("X-Blob", input.headerMediaType.encodeBase64())
""".trimIndent()
contents.shouldContainOnlyOnceWithDiff(expectedContents)
}
Expand All @@ -168,10 +168,10 @@ class HttpStringValuesMapSerializerTest {
contents.assertBalancedBracesAndParens()

val expectedContents = """
if (input.enumList?.isNotEmpty() == true) appendAll("x-enumList", input.enumList.map { quoteHeaderValue(it.value) })
if (input.intList?.isNotEmpty() == true) appendAll("x-intList", input.intList.map { it.toString() })
if (input.strList?.isNotEmpty() == true) appendAll("x-strList", input.strList.map { quoteHeaderValue(it) })
if (input.tsList?.isNotEmpty() == true) appendAll("x-tsList", input.tsList.map { it.format(TimestampFormat.RFC_5322) })
if (input.enumList != null) appendAll("x-enumList", input.enumList.map { quoteHeaderValue(it.value) })
if (input.intList != null) appendAll("x-intList", input.intList.map { it.toString() })
if (input.strList != null) appendAll("x-strList", input.strList.map { quoteHeaderValue(it) })
if (input.tsList != null) appendAll("x-tsList", input.tsList.map { it.format(TimestampFormat.RFC_5322) })
""".trimIndent()
contents.shouldContainOnlyOnceWithDiff(expectedContents)
}
Expand All @@ -190,7 +190,7 @@ class HttpStringValuesMapSerializerTest {
val queryContents = getTestContents(defaultModel, "com.test#TimestampInput", HttpBinding.Location.QUERY)
val expectedQueryContents = """
if (input.queryTimestamp != null) add("qtime", input.queryTimestamp.format(TimestampFormat.ISO_8601))
if (input.queryTimestampList?.isNotEmpty() == true) addAll("qtimeList", input.queryTimestampList.map { it.format(TimestampFormat.ISO_8601) })
if (input.queryTimestampList != null) addAll("qtimeList", input.queryTimestampList.map { it.format(TimestampFormat.ISO_8601) })
""".trimIndent()
queryContents.shouldContainOnlyOnceWithDiff(expectedQueryContents)
}
Expand Down
2 changes: 1 addition & 1 deletion gradle/libs.versions.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ micrometer-version = "1.14.2"
binary-compatibility-validator-version = "0.16.3"

# codegen
smithy-version = "1.53.0"
smithy-version = "1.54.0"
smithy-gradle-version = "0.9.0"

# testing
Expand Down
Loading