Skip to content

Commit f6eeecb

Browse files
aajtoddaajtodd
authored
chore: upgrade smithy to 1.17.0 (#587)
* chore: upgrade smithy to 1.17.0 * refactor(codegen): allow JSON serde to ignore jsonName trait * fix: handle escaped and quoted HTTP header values * fix(rt): add test for missing end quote * fix(rt): add tests for whitespace and other chars between quoted values
1 parent 50da2cb commit f6eeecb

File tree

11 files changed

+201
-21
lines changed

11 files changed

+201
-21
lines changed

‎gradle.properties‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ atomicFuVersion=0.17.0
1818
kotlinxSerializationVersion=1.3.0
1919

2020
# codegen
21-
smithyVersion=1.13.1
21+
smithyVersion=1.17.0
2222
smithyGradleVersion=0.5.3
2323

2424
# testing/utility

‎runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/util/HeaderLists.kt‎

Lines changed: 89 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,17 +5,89 @@
55
package aws.smithy.kotlin.runtime.http.util
66

77
import aws.smithy.kotlin.runtime.ClientException
8+
import aws.smithy.kotlin.runtime.util.InternalApi
89

910
/**
10-
* Attempt to split an HTTP header [value] by commas and returns the resulting list
11+
* Attempt to split an HTTP header [value] by commas and returns the resulting list.
12+
* This parsing implements [RFC-7230's](https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6)
13+
* specification for header values.
1114
*/
12-
fun splitHeaderListValues(value: String): List<String> = value.split(",").map { it.trim() }
15+
@InternalApi
16+
fun splitHeaderListValues(value: String): List<String> {
17+
val results = mutableListOf<String>()
18+
var currIdx = 0
19+
while (currIdx < value.length) {
20+
val next = when (value[currIdx]) {
21+
// skip ws
22+
' ', '\t' -> {
23+
currIdx++
24+
continue
25+
}
26+
'"' -> value.readNextQuoted(currIdx)
27+
else -> value.readNextUnquoted(currIdx)
28+
}
29+
currIdx = next.first
30+
results.add(next.second)
31+
}
32+
33+
return results
34+
}
35+
36+
private fun String.readNextQuoted(startIdx: Int, delim: Char = ','): Pair<Int, String> {
37+
// startIdx is start of the quoted value, there must be at least an ending quotation mark
38+
check(startIdx + 1 < length) { "unbalanced quoted header value" }
39+
40+
// find first non-escaped quote or end of string
41+
var endIdx = startIdx + 1
42+
while (endIdx < length) {
43+
when (this[endIdx]) {
44+
'\\' -> endIdx++ // skip escaped chars
45+
'"' -> break
46+
}
47+
endIdx++
48+
}
49+
50+
val next = substring(startIdx + 1, endIdx)
51+
52+
// consume trailing quote
53+
check(endIdx < length && this[endIdx] == '"') { "missing end quote around quoted header value: `$next`" }
54+
endIdx++
55+
56+
// consume delim
57+
while (endIdx < length) {
58+
when (this[endIdx]) {
59+
' ', '\t' -> endIdx++
60+
delim -> {
61+
endIdx++
62+
break
63+
}
64+
else -> error("Unexpected char `${this[endIdx]}` between header values. Previous header: `$next`")
65+
}
66+
}
67+
68+
val unescaped = next.replace("\\\"", "\"")
69+
.replace("\\\\", "\\")
70+
71+
return Pair(endIdx, unescaped)
72+
}
73+
74+
private fun String.readNextUnquoted(startIdx: Int, delim: Char = ','): Pair<Int, String> {
75+
check(startIdx < this.length)
76+
var endIdx = startIdx
77+
while (endIdx < length && this[endIdx] != delim) endIdx++
78+
79+
val next = substring(startIdx, endIdx)
80+
if (endIdx < length && this[endIdx] == delim) endIdx++
81+
82+
return Pair(endIdx, next.trim())
83+
}
1384

1485
/**
1586
* Attempt to split an HTTP header [value] as if it contains a list of HTTP-Date timestamp
1687
* values separated by commas. The split is aware of the HTTP-Date format and will skip
1788
* a comma within the timestamp value.
1889
*/
90+
@InternalApi
1991
fun splitHttpDateHeaderListValues(value: String): List<String> {
2092
val n = value.count { it == ',' }
2193
if (n <= 1) {
@@ -45,3 +117,18 @@ fun splitHttpDateHeaderListValues(value: String): List<String> {
45117

46118
return splits
47119
}
120+
121+
// chars in an HTTP header value that require quotations
122+
private const val QUOTABLE_HEADER_VALUE_CHARS = "\",()"
123+
124+
/**
125+
* Conditionally quotes and escapes a header value if the header value contains a comma or quote
126+
*/
127+
@InternalApi
128+
fun quoteHeaderValue(value: String): String =
129+
if (value.trim().length != value.length || QUOTABLE_HEADER_VALUE_CHARS.any { value.contains(it) }) {
130+
val formatted = value.replace("\\", "\\\\").replace("\"", "\\\"")
131+
"\"$formatted\""
132+
} else {
133+
value
134+
}

‎runtime/protocol/http/common/test/aws/smithy/kotlin/runtime/http/util/HeaderListsTest.kt‎

Lines changed: 69 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,67 @@ import io.kotest.matchers.string.shouldContain
88
import kotlin.test.Test
99
import kotlin.test.assertEquals
1010
import kotlin.test.assertFails
11+
import kotlin.test.assertFailsWith
1112

1213
class HeaderListsTest {
14+
1315
@Test
14-
fun itSplitsOnComma() {
16+
fun testSplitStringList() {
17+
assertEquals(listOf("foo"), splitHeaderListValues("foo"))
18+
19+
// trailing space
20+
assertEquals(listOf("fooTrailing"), splitHeaderListValues("fooTrailing "))
21+
22+
// leading and trailing space
23+
assertEquals(listOf(" foo "), splitHeaderListValues("\" foo \""))
24+
25+
// ignore spaces between values
26+
assertEquals(listOf("foo", "bar"), splitHeaderListValues("foo , bar"))
27+
assertEquals(listOf("foo", "bar"), splitHeaderListValues("\"foo\" , \"bar\""))
28+
29+
// comma in quotes
30+
assertEquals(listOf("foo,bar", "baz"), splitHeaderListValues("\"foo,bar\",baz"))
31+
32+
// comm in quotes w/trailing space
33+
assertEquals(listOf("foo,bar", "baz"), splitHeaderListValues("\"foo,bar\",baz "))
34+
35+
// quote in quotes
36+
assertEquals(listOf("foo\",bar", "\"asdf\"", "baz"), splitHeaderListValues("\"foo\\\",bar\",\"\\\"asdf\\\"\",baz"))
37+
38+
// quote in quote w/spaces
39+
assertEquals(listOf("foo\",bar", "\"asdf \"", "baz"), splitHeaderListValues("\"foo\\\",bar\", \"\\\"asdf \\\"\", baz"))
40+
41+
// empty quotes
42+
assertEquals(listOf("", "baz"), splitHeaderListValues("\"\",baz"))
43+
44+
// escaped slashes
45+
assertEquals(listOf("foo", "(foo\\bar)"), splitHeaderListValues("foo, \"(foo\\\\bar)\""))
46+
47+
// empty
48+
assertEquals(listOf("", "1"), splitHeaderListValues(",1"))
49+
50+
assertFailsWith<IllegalStateException> {
51+
splitHeaderListValues("foo, bar, \"baz")
52+
}.message.shouldContain("missing end quote around quoted header value: `baz`")
53+
54+
assertFailsWith<IllegalStateException> {
55+
splitHeaderListValues("foo , \"bar\" \tf,baz")
56+
}.message.shouldContain("Unexpected char `f` between header values. Previous header: `bar`")
57+
}
58+
59+
@Test
60+
fun testSplitIntList() {
1561
assertEquals(listOf("1"), splitHeaderListValues("1"))
1662
assertEquals(listOf("1", "2", "3"), splitHeaderListValues("1,2,3"))
1763
assertEquals(listOf("1", "2", "3"), splitHeaderListValues("1, 2, 3"))
18-
assertEquals(listOf("", "1"), splitHeaderListValues(",1"))
64+
65+
// quoted
66+
assertEquals(listOf("1", "2", "3", "-4", "5"), splitHeaderListValues("1,\"2\",3,\"-4\",5"))
67+
}
68+
69+
@Test
70+
fun testSplitBoolList() {
71+
assertEquals(listOf("true", "false", "true", "true"), splitHeaderListValues("true,\"false\",true,\"true\""))
1972
}
2073

2174
@Test
@@ -42,4 +95,18 @@ class HeaderListsTest {
4295
}
4396
ex.message!!.shouldContain("invalid timestamp HttpDate header comma separations: `Mon")
4497
}
98+
99+
@Test
100+
fun itQuotesHeaderValues() {
101+
assertEquals("", quoteHeaderValue(""))
102+
assertEquals("foo", quoteHeaderValue("foo"))
103+
assertEquals("\" foo\"", quoteHeaderValue(" foo"))
104+
assertEquals("foo bar", quoteHeaderValue("foo bar"))
105+
assertEquals("\"foo,bar\"", quoteHeaderValue("foo,bar"))
106+
assertEquals("\",\"", quoteHeaderValue(","))
107+
assertEquals("\"\\\"foo\\\"\"", quoteHeaderValue("\"foo\""))
108+
assertEquals("\"\\\"f\\\\oo\\\"\"", quoteHeaderValue("\"f\\oo\""))
109+
assertEquals("\"(\"", quoteHeaderValue("("))
110+
assertEquals("\")\"", quoteHeaderValue(")"))
111+
}
45112
}

‎smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/core/RuntimeTypes.kt‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,18 @@ object RuntimeTypes {
2525
val QueryParameters = runtimeSymbol("QueryParameters", KotlinDependency.HTTP)
2626
val QueryParametersBuilder = runtimeSymbol("QueryParametersBuilder", KotlinDependency.HTTP)
2727
val toQueryParameters = runtimeSymbol("toQueryParameters", KotlinDependency.HTTP)
28-
val encodeLabel = runtimeSymbol("encodeLabel", KotlinDependency.HTTP, "util")
2928
val readAll = runtimeSymbol("readAll", KotlinDependency.HTTP)
3029
val parameters = runtimeSymbol("parameters", KotlinDependency.HTTP)
3130
val toByteStream = runtimeSymbol("toByteStream", KotlinDependency.HTTP)
3231
val toHttpBody = runtimeSymbol("toHttpBody", KotlinDependency.HTTP)
3332
val isSuccess = runtimeSymbol("isSuccess", KotlinDependency.HTTP)
3433
val StatusCode = runtimeSymbol("HttpStatusCode", KotlinDependency.HTTP)
35-
val splitAsQueryParameters = runtimeSymbol("splitAsQueryParameters", KotlinDependency.HTTP, "util")
34+
35+
object Util {
36+
val encodeLabel = runtimeSymbol("encodeLabel", KotlinDependency.HTTP, "util")
37+
val splitAsQueryParameters = runtimeSymbol("splitAsQueryParameters", KotlinDependency.HTTP, "util")
38+
val quoteHeaderValue = runtimeSymbol("quoteHeaderValue", KotlinDependency.HTTP, "util")
39+
}
3640

3741
object Request {
3842
val HttpRequest = runtimeSymbol("HttpRequest", KotlinDependency.HTTP, "request")

‎smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/protocol/HttpBindingProtocolGenerator.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,7 @@ abstract class HttpBindingProtocolGenerator : ProtocolGenerator {
285285
"input.${binding.member.defaultName()}"
286286
}
287287

288-
val encodeSymbol = RuntimeTypes.Http.encodeLabel
288+
val encodeSymbol = RuntimeTypes.Http.Util.encodeLabel
289289
writer.addImport(encodeSymbol)
290290
val encodeFn = if (segment.isGreedyLabel) {
291291
writer.format("#T(greedy = true)", encodeSymbol)

‎smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/protocol/HttpStringValuesMapSerializer.kt‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -101,12 +101,18 @@ class HttpStringValuesMapSerializer(
101101
formatInstant("it", tsFormat, forceString = true)
102102
}
103103
ShapeType.STRING -> {
104-
if (collectionMemberTarget.isEnum) {
105-
// collections of enums should be mapped to the raw values
106-
"it.value"
107-
} else {
108-
// collections of string doesn't need mapped to anything
109-
""
104+
when (binding.location) {
105+
HttpBinding.Location.QUERY -> {
106+
if (collectionMemberTarget.isEnum) "it.value" else ""
107+
}
108+
else -> {
109+
// collections of enums should be mapped to the raw values
110+
val inner = if (collectionMemberTarget.isEnum) "it.value" else "it"
111+
// ensure header values targeting lists are quoted appropriately
112+
val quoteHeaderValue = RuntimeTypes.Http.Util.quoteHeaderValue
113+
writer.addImport(quoteHeaderValue)
114+
"${quoteHeaderValue.name}($inner)"
115+
}
110116
}
111117
}
112118
// default to "toString"

‎smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/serde/JsonParserGenerator.kt‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@ import software.amazon.smithy.model.traits.TimestampFormatTrait
2020

2121
open class JsonParserGenerator(
2222
// FIXME - we shouldn't need this, it's only required by JsonSerdeDescriptorGenerator because of toRenderingContext
23-
private val protocolGenerator: ProtocolGenerator
23+
private val protocolGenerator: ProtocolGenerator,
24+
private val supportsJsonNameTrait: Boolean = true
2425
) : StructuredDataParserGenerator {
2526

2627
open val defaultTimestampFormat: TimestampFormatTrait.Format = TimestampFormatTrait.Format.EPOCH_SECONDS
@@ -105,7 +106,7 @@ open class JsonParserGenerator(
105106
members: List<MemberShape>,
106107
writer: KotlinWriter,
107108
) {
108-
JsonSerdeDescriptorGenerator(ctx.toRenderingContext(protocolGenerator, shape, writer), members).render()
109+
JsonSerdeDescriptorGenerator(ctx.toRenderingContext(protocolGenerator, shape, writer), members, supportsJsonNameTrait).render()
109110
if (shape.isUnionShape) {
110111
val name = ctx.symbolProvider.toSymbol(shape).name
111112
DeserializeUnionGenerator(ctx, name, members, writer, defaultTimestampFormat).render()

‎smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/serde/JsonSerdeDescriptorGenerator.kt‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,12 @@ import software.amazon.smithy.model.traits.JsonNameTrait
1616

1717
/**
1818
* Field descriptor generator that processes the [jsonName trait](https://awslabs.github.io/smithy/1.0/spec/core/protocol-traits.html#jsonname-trait)
19+
* @param supportsJsonNameTrait Flag indicating if the jsonName trait should be used or not, when `false` the member name is used.
1920
*/
2021
open class JsonSerdeDescriptorGenerator(
2122
ctx: RenderingContext<Shape>,
22-
memberShapes: List<MemberShape>? = null
23+
memberShapes: List<MemberShape>? = null,
24+
private val supportsJsonNameTrait: Boolean = true
2325
) : AbstractSerdeDescriptorGenerator(ctx, memberShapes) {
2426

2527
override fun getFieldDescriptorTraits(
@@ -34,7 +36,11 @@ open class JsonSerdeDescriptorGenerator(
3436
RuntimeTypes.Serde.SerdeJson.JsonSerialName,
3537
)
3638

37-
val serialName = member.getTrait<JsonNameTrait>()?.value ?: member.memberName
39+
val serialName = if (supportsJsonNameTrait) {
40+
member.getTrait<JsonNameTrait>()?.value ?: member.memberName
41+
} else {
42+
member.memberName
43+
}
3844
return listOf(SdkFieldDescriptorTrait(RuntimeTypes.Serde.SerdeJson.JsonSerialName, serialName.dq()))
3945
}
4046
}

‎smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/rendering/serde/JsonSerializerGenerator.kt‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,8 @@ import software.amazon.smithy.model.traits.TimestampFormatTrait
2020

2121
open class JsonSerializerGenerator(
2222
// FIXME - we shouldn't need this, it's only required by JsonSerdeDescriptorGenerator because of toRenderingContext
23-
private val protocolGenerator: HttpBindingProtocolGenerator
23+
private val protocolGenerator: HttpBindingProtocolGenerator,
24+
private val supportsJsonNameTrait: Boolean = true
2425
) : StructuredDataSerializerGenerator {
2526

2627
open val defaultTimestampFormat: TimestampFormatTrait.Format = TimestampFormatTrait.Format.EPOCH_SECONDS
@@ -86,7 +87,7 @@ open class JsonSerializerGenerator(
8687
writer: KotlinWriter,
8788
) {
8889
// render the serde descriptors
89-
JsonSerdeDescriptorGenerator(ctx.toRenderingContext(protocolGenerator, shape, writer), members).render()
90+
JsonSerdeDescriptorGenerator(ctx.toRenderingContext(protocolGenerator, shape, writer), members, supportsJsonNameTrait).render()
9091
if (shape.isUnionShape) {
9192
SerializeUnionGenerator(ctx, members, writer, defaultTimestampFormat).render()
9293
} else {

‎smithy-kotlin-codegen/src/test/kotlin/software/amazon/smithy/kotlin/codegen/rendering/protocol/HttpStringValuesMapSerializerTest.kt‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,9 @@ class HttpStringValuesMapSerializerTest {
100100
contents.assertBalancedBracesAndParens()
101101

102102
val expectedContents = """
103-
if (input.enumList?.isNotEmpty() == true) appendAll("x-enumList", input.enumList.map { it.value })
103+
if (input.enumList?.isNotEmpty() == true) appendAll("x-enumList", input.enumList.map { quoteHeaderValue(it.value) })
104104
if (input.intList?.isNotEmpty() == true) appendAll("x-intList", input.intList.map { "${'$'}it" })
105+
if (input.strList?.isNotEmpty() == true) appendAll("x-strList", input.strList.map { quoteHeaderValue(it) })
105106
if (input.tsList?.isNotEmpty() == true) appendAll("x-tsList", input.tsList.map { it.format(TimestampFormat.RFC_5322) })
106107
""".trimIndent()
107108
contents.shouldContainOnlyOnceWithDiff(expectedContents)

0 commit comments

Comments
 (0)