Skip to content

Commit 7116221

Browse files
0marperez0marperez
committed
PR feedback
1 parent 3ef1c20 commit 7116221

File tree

20 files changed

+220
-239
lines changed

20 files changed

+220
-239
lines changed

‎codegen/protocol-tests/build.gradle.kts‎

Lines changed: 6 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -24,27 +24,17 @@ data class ProtocolTest(val projectionName: String, val serviceShapeId: String,
2424
// for the configured protocols in [enabledProtocols].
2525
val enabledProtocols = listOf(
2626
ProtocolTest("aws-ec2-query", "aws.protocoltests.ec2#AwsEc2"),
27-
28-
// FIXME: Re-enable. This test is broken after a smithy update: https://github.com/smithy-lang/smithy/pull/2467
29-
// ProtocolTest("aws-json-10", "aws.protocoltests.json10#JsonRpc10"),
30-
27+
ProtocolTest("aws-json-10", "aws.protocoltests.json10#JsonRpc10"),
3128
ProtocolTest("aws-json-11", "aws.protocoltests.json#JsonProtocol"),
32-
33-
// FIXME: Re-enable. These tests are broken after a smithy update: https://github.com/smithy-lang/smithy/pull/2403
34-
// ProtocolTest("aws-restjson", "aws.protocoltests.restjson#RestJson"),
35-
// ProtocolTest("aws-restxml", "aws.protocoltests.restxml#RestXml"),
36-
29+
ProtocolTest("aws-restjson", "aws.protocoltests.restjson#RestJson"),
30+
ProtocolTest("aws-restxml", "aws.protocoltests.restxml#RestXml"),
3731
ProtocolTest("aws-restxml-xmlns", "aws.protocoltests.restxml.xmlns#RestXmlWithNamespace"),
3832
ProtocolTest("aws-query", "aws.protocoltests.query#AwsQuery"),
39-
40-
// FIXME: Re-enable. This test is broken after a smithy update: https://github.com/smithy-lang/smithy/pull/2467
41-
// ProtocolTest("smithy-rpcv2-cbor", "smithy.protocoltests.rpcv2Cbor#RpcV2Protocol"),
33+
ProtocolTest("smithy-rpcv2-cbor", "smithy.protocoltests.rpcv2Cbor#RpcV2Protocol"),
4234

4335
// Custom hand written tests
44-
// FIXME: Re-enable. These tests were relying on a smithy bug that has since been fixed.
45-
// https://github.com/smithy-lang/smithy/pull/2393
46-
// ProtocolTest("error-correction-json", "aws.protocoltests.errorcorrection#RequiredValueJson"),
47-
// ProtocolTest("error-correction-xml", "aws.protocoltests.errorcorrection#RequiredValueXml"),
36+
ProtocolTest("error-correction-json", "aws.protocoltests.errorcorrection#RequiredValueJson"),
37+
ProtocolTest("error-correction-xml", "aws.protocoltests.errorcorrection#RequiredValueXml"),
4838
)
4939

5040
smithyBuild {

‎codegen/protocol-tests/model/error-correction-tests.smithy‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ operation SayHelloXml { output: TestOutput, errors: [Error] }
3939

4040
structure TestOutputDocument with [TestStruct] {
4141
innerField: Nested,
42+
// FIXME: Enable trait
4243
// @required
4344
document: Document
4445
}
@@ -64,6 +65,7 @@ structure TestStruct {
6465
@required
6566
nestedListValue: NestedList
6667

68+
// FIXME: Enable trait
6769
// @required
6870
nested: Nested
6971

@@ -95,6 +97,7 @@ union MyUnion {
9597
}
9698

9799
structure Nested {
100+
// FIXME: Enable trait
98101
// @required
99102
a: String
100103
}

‎codegen/smithy-aws-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/aws/protocols/core/AwsHttpBindingProtocolGenerator.kt‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,15 @@ abstract class AwsHttpBindingProtocolGenerator : HttpBindingProtocolGenerator()
3939
// The following can be used to generate only a specific test by name.
4040
// val targetedTest = TestMemberDelta(setOf("RestJsonComplexErrorWithNoMessage"), TestContainmentMode.RUN_TESTS)
4141

42-
val ignoredTests = TestMemberDelta(setOf())
42+
val ignoredTests = TestMemberDelta(
43+
setOf(
44+
"AwsJson10ClientErrorCorrectsWithDefaultValuesWhenServerFailsToSerializeRequiredValues",
45+
"RestJsonNullAndEmptyHeaders",
46+
"NullAndEmptyHeaders",
47+
"RpcV2CborClientPopulatesDefaultsValuesWhenMissingInResponse",
48+
"RpcV2CborClientPopulatesDefaultValuesInInput",
49+
),
50+
)
4351

4452
val requestTestBuilder = HttpProtocolUnitTestRequestGenerator.Builder()
4553
val responseTestBuilder = HttpProtocolUnitTestResponseGenerator.Builder()

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,6 @@ object RuntimeTypes {
201201
val toNumber = symbol("toNumber")
202202
val type = symbol("type")
203203
val PlatformProvider = symbol("PlatformProvider")
204-
val runBlocking = symbol("runBlocking")
205204
}
206205

207206
object Net : RuntimeTypePackage(KotlinDependency.CORE, "net") {
@@ -234,8 +233,9 @@ object RuntimeTypes {
234233
object Config : RuntimeTypePackage(KotlinDependency.SMITHY_CLIENT, "config") {
235234
val RequestCompressionConfig = symbol("RequestCompressionConfig")
236235
val CompressionClientConfig = symbol("CompressionClientConfig")
237-
val HttpChecksumClientConfig = symbol("HttpChecksumClientConfig")
238-
val HttpChecksumConfigOption = symbol("HttpChecksumConfigOption")
236+
val HttpChecksumConfig = symbol("HttpChecksumConfig")
237+
val RequestHttpChecksumConfig = symbol("RequestHttpChecksumConfig")
238+
val ResponseHttpChecksumConfig = symbol("ResponseHttpChecksumConfig")
239239
}
240240

241241
object Endpoints : RuntimeTypePackage(KotlinDependency.SMITHY_CLIENT, "endpoints") {
@@ -415,6 +415,7 @@ object RuntimeTypes {
415415

416416
val CompletableDeferred = "kotlinx.coroutines.CompletableDeferred".toSymbol()
417417
val job = "kotlinx.coroutines.job".toSymbol()
418+
val runBlocking = "kotlinx.coroutines.runBlocking".toSymbol()
418419

419420
object Flow {
420421
// NOTE: smithy-kotlin core has an API dependency on this already

‎codegen/smithy-kotlin-codegen/src/main/kotlin/software/amazon/smithy/kotlin/codegen/model/ShapeExt.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import software.amazon.smithy.rulesengine.language.EndpointRuleSet
2121
import software.amazon.smithy.rulesengine.traits.EndpointRuleSetTrait
2222
import software.amazon.smithy.rulesengine.traits.EndpointTestCase
2323
import software.amazon.smithy.rulesengine.traits.EndpointTestsTrait
24-
import kotlin.streams.toList as kotlinToList // Gave this import a unique name because the Java one was being preferred
24+
import java.util.stream.Collectors
2525

2626
/**
2727
* Get all shapes of a particular type from the model.
@@ -33,7 +33,7 @@ import kotlin.streams.toList as kotlinToList // Gave this import a unique name b
3333
* shape's closure for example)
3434
*/
3535
@Suppress("EXTENSION_SHADOWED_BY_MEMBER")
36-
inline fun <reified T : Shape> Model.shapes(): List<T> = shapes(T::class.java).kotlinToList()
36+
inline fun <reified T : Shape> Model.shapes(): List<T> = shapes(T::class.java).collect(Collectors.toList())
3737

3838
/**
3939
* Extension function to return a shape of expected type.

‎runtime/protocol/http-client/api/http-client.api‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -332,15 +332,15 @@ public final class aws/smithy/kotlin/runtime/http/interceptors/DiscoveredEndpoin
332332
}
333333

334334
public final class aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptor : aws/smithy/kotlin/runtime/http/interceptors/AbstractChecksumInterceptor {
335-
public fun <init> (ZLaws/smithy/kotlin/runtime/client/config/HttpChecksumConfigOption;Ljava/lang/String;)V
335+
public fun <init> (ZLaws/smithy/kotlin/runtime/client/config/RequestHttpChecksumConfig;Ljava/lang/String;)V
336336
public fun applyChecksum (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;Ljava/lang/String;)Laws/smithy/kotlin/runtime/http/request/HttpRequest;
337337
public fun calculateChecksum (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
338338
public fun modifyBeforeSigning (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
339339
}
340340

341341
public class aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsResponseInterceptor : aws/smithy/kotlin/runtime/client/Interceptor {
342342
public static final field Companion Laws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsResponseInterceptor$Companion;
343-
public fun <init> (ZLaws/smithy/kotlin/runtime/client/config/HttpChecksumConfigOption;)V
343+
public fun <init> (ZLaws/smithy/kotlin/runtime/client/config/ResponseHttpChecksumConfig;)V
344344
public fun ignoreChecksum (Ljava/lang/String;)Z
345345
public fun modifyBeforeAttemptCompletion-gIAlu-s (Laws/smithy/kotlin/runtime/client/ResponseInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
346346
public fun modifyBeforeCompletion-gIAlu-s (Laws/smithy/kotlin/runtime/client/ResponseInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;

‎runtime/protocol/http-client/common/src/aws/smithy/kotlin/runtime/http/interceptors/AbstractChecksumInterceptor.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,5 +35,5 @@ public abstract class AbstractChecksumInterceptor : HttpInterceptor {
3535
/**
3636
* @return The default checksum algorithm name, null if default checksums are disabled.
3737
*/
38-
internal fun defaultChecksumAlgorithmName(context: ProtocolRequestInterceptorContext<Any, HttpRequest>): String? =
39-
context.executionContext.getOrNull(HttpOperationContext.DefaultChecksumAlgorithm)
38+
internal val ProtocolRequestInterceptorContext<Any, HttpRequest>.defaultChecksumAlgorithmName: String?
39+
get() = executionContext.getOrNull(HttpOperationContext.DefaultChecksumAlgorithm)

‎runtime/protocol/http-client/common/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptor.kt‎

Lines changed: 59 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,13 @@
55

66
package aws.smithy.kotlin.runtime.http.interceptors
77

8+
import aws.smithy.kotlin.runtime.ClientException
89
import aws.smithy.kotlin.runtime.InternalApi
910
import aws.smithy.kotlin.runtime.businessmetrics.BusinessMetric
1011
import aws.smithy.kotlin.runtime.businessmetrics.SmithyBusinessMetric
1112
import aws.smithy.kotlin.runtime.businessmetrics.emitBusinessMetric
1213
import aws.smithy.kotlin.runtime.client.ProtocolRequestInterceptorContext
13-
import aws.smithy.kotlin.runtime.client.config.HttpChecksumConfigOption
14+
import aws.smithy.kotlin.runtime.client.config.RequestHttpChecksumConfig
1415
import aws.smithy.kotlin.runtime.hashing.*
1516
import aws.smithy.kotlin.runtime.http.*
1617
import aws.smithy.kotlin.runtime.http.request.HttpRequest
@@ -50,7 +51,7 @@ import kotlin.coroutines.coroutineContext
5051
@InternalApi
5152
public class FlexibleChecksumsRequestInterceptor(
5253
private val requestChecksumRequired: Boolean,
53-
private val requestChecksumCalculation: HttpChecksumConfigOption?,
54+
private val requestChecksumCalculation: RequestHttpChecksumConfig?,
5455
private val requestChecksumAlgorithm: String?,
5556
) : AbstractChecksumInterceptor() {
5657
override suspend fun modifyBeforeSigning(context: ProtocolRequestInterceptorContext<Any, HttpRequest>): HttpRequest {
@@ -64,7 +65,7 @@ public class FlexibleChecksumsRequestInterceptor(
6465
return context.protocolRequest
6566
}
6667

67-
checksumAlgorithm(
68+
resolveChecksumAlgorithm(
6869
requestChecksumRequired,
6970
requestChecksumCalculation,
7071
requestChecksumAlgorithm,
@@ -75,7 +76,7 @@ public class FlexibleChecksumsRequestInterceptor(
7576

7677
val request = context.protocolRequest.toBuilder()
7778
val deferredChecksum = CompletableDeferred<String>(context.executionContext.coroutineContext.job)
78-
val checksumHeader = checksumAlgorithmHeader(checksumAlgorithm)
79+
val checksumHeader = checksumAlgorithm.resolveChecksumAlgorithmHeaderName()
7980

8081
request.body = request.body
8182
.toHashingBody(checksumAlgorithm, request.body.contentLength)
@@ -84,7 +85,7 @@ public class FlexibleChecksumsRequestInterceptor(
8485
request.headers.append("x-amz-trailer", checksumHeader)
8586
request.trailingHeaders.append(checksumHeader, deferredChecksum)
8687
context.executionContext.emitBusinessMetric(checksumAlgorithm.toBusinessMetric())
87-
} else { // Delegate checksum calculation to super class
88+
} else { // Delegate checksum calculation to super class, calculateChecksum, and applyChecksum
8889
return super.modifyBeforeSigning(context)
8990
}
9091
}
@@ -96,26 +97,26 @@ public class FlexibleChecksumsRequestInterceptor(
9697
/**
9798
* Determines what checksum algorithm to use, null if none is required
9899
*/
99-
private fun checksumAlgorithm(
100+
private fun resolveChecksumAlgorithm(
100101
requestChecksumRequired: Boolean,
101-
requestChecksumCalculation: HttpChecksumConfigOption?,
102+
requestChecksumCalculation: RequestHttpChecksumConfig?,
102103
requestChecksumAlgorithm: String?,
103104
context: ProtocolRequestInterceptorContext<Any, HttpRequest>,
104105
): HashFunction? =
105106
requestChecksumAlgorithm
106107
?.toHashFunctionOrThrow()
107108
?.takeIf { it.isSupportedForFlexibleChecksums }
108-
?: defaultChecksumAlgorithmName(context)
109+
?: context.defaultChecksumAlgorithmName
109110
?.toHashFunctionOrThrow()
110111
?.takeIf {
111-
(requestChecksumRequired || requestChecksumCalculation == HttpChecksumConfigOption.WHEN_SUPPORTED) &&
112+
(requestChecksumRequired || requestChecksumCalculation == RequestHttpChecksumConfig.WHEN_SUPPORTED) &&
112113
it.isSupportedForFlexibleChecksums
113114
}
114115

115116
// Handles calculating checksum for non-aws-chunked requests
116117
override suspend fun calculateChecksum(context: ProtocolRequestInterceptorContext<Any, HttpRequest>): String? {
117118
val req = context.protocolRequest.toBuilder()
118-
val checksumAlgorithm = checksumAlgorithm(
119+
val checksumAlgorithm = resolveChecksumAlgorithm(
119120
requestChecksumRequired,
120121
requestChecksumCalculation,
121122
requestChecksumAlgorithm,
@@ -141,13 +142,13 @@ public class FlexibleChecksumsRequestInterceptor(
141142
checksum: String,
142143
): HttpRequest {
143144
val request = context.protocolRequest.toBuilder()
144-
val checksumAlgorithm = checksumAlgorithm(
145+
val checksumAlgorithm = resolveChecksumAlgorithm(
145146
requestChecksumRequired,
146147
requestChecksumCalculation,
147148
requestChecksumAlgorithm,
148149
context,
149150
)!!
150-
val checksumHeader = checksumAlgorithmHeader(checksumAlgorithm)
151+
val checksumHeader = checksumAlgorithm.resolveChecksumAlgorithmHeaderName()
151152

152153
request.headers[checksumHeader] = checksum
153154
request.headers.removeAllChecksumHeadersExcept(checksumHeader)
@@ -184,8 +185,8 @@ public class FlexibleChecksumsRequestInterceptor(
184185
.names()
185186
.firstOrNull {
186187
it.startsWith("x-amz-checksum-", ignoreCase = true) &&
187-
!it.equals("x-amz-checksum-md5", ignoreCase = true).also { itEqualsMd5 ->
188-
if (itEqualsMd5) {
188+
!it.equals("x-amz-checksum-md5", ignoreCase = true).also { isMd5 ->
189+
if (isMd5) {
189190
logger.debug { "MD5 checksum was supplied via header, MD5 is not a supported algorithm, ignoring header" }
190191
}
191192
}
@@ -203,10 +204,51 @@ public class FlexibleChecksumsRequestInterceptor(
203204
}
204205

205206
/**
206-
* Removes all checksum headers except specified header
207+
* Removes all checksum headers except [headerName]
208+
* @param headerName the checksum header name to keep
207209
*/
208-
private fun HeadersBuilder.removeAllChecksumHeadersExcept(checksumHeader: String) =
210+
private fun HeadersBuilder.removeAllChecksumHeadersExcept(headerName: String) =
209211
names()
210-
.filter { it.startsWith("x-amz-checksum-", ignoreCase = true) && !it.equals(checksumHeader, ignoreCase = true) }
212+
.filter { it.startsWith("x-amz-checksum-", ignoreCase = true) && !it.equals(headerName, ignoreCase = true) }
211213
.forEach { remove(it) }
214+
215+
/**
216+
* Convert an [HttpBody] with an underlying [HashingSource] or [HashingByteReadChannel]
217+
* to a [CompletingSource] or [CompletingByteReadChannel], respectively.
218+
*/
219+
private fun HttpBody.toCompletingBody(deferred: CompletableDeferred<String>): HttpBody = when (this) {
220+
is HttpBody.SourceContent -> CompletingSource(deferred, (readFrom() as HashingSource)).toHttpBody(contentLength)
221+
is HttpBody.ChannelContent -> CompletingByteReadChannel(deferred, (readFrom() as HashingByteReadChannel)).toHttpBody(contentLength)
222+
else -> throw ClientException("HttpBody type is not supported")
223+
}
224+
225+
/**
226+
* An [SdkSource] which uses the underlying [hashingSource]'s checksum to complete a [CompletableDeferred] value.
227+
*/
228+
internal class CompletingSource(
229+
private val deferred: CompletableDeferred<String>,
230+
private val hashingSource: HashingSource,
231+
) : SdkSource by hashingSource {
232+
override fun read(sink: SdkBuffer, limit: Long): Long = hashingSource.read(sink, limit)
233+
.also {
234+
if (it == -1L) {
235+
deferred.complete(hashingSource.digest().encodeBase64String())
236+
}
237+
}
238+
}
239+
240+
/**
241+
* An [SdkByteReadChannel] which uses the underlying [hashingChannel]'s checksum to complete a [CompletableDeferred] value.
242+
*/
243+
internal class CompletingByteReadChannel(
244+
private val deferred: CompletableDeferred<String>,
245+
private val hashingChannel: HashingByteReadChannel,
246+
) : SdkByteReadChannel by hashingChannel {
247+
override suspend fun read(sink: SdkBuffer, limit: Long): Long = hashingChannel.read(sink, limit)
248+
.also {
249+
if (it == -1L) {
250+
deferred.complete(hashingChannel.digest().encodeBase64String())
251+
}
252+
}
253+
}
212254
}

‎runtime/protocol/http-client/common/src/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsResponseInterceptor.kt‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ package aws.smithy.kotlin.runtime.http.interceptors
88
import aws.smithy.kotlin.runtime.ClientException
99
import aws.smithy.kotlin.runtime.InternalApi
1010
import aws.smithy.kotlin.runtime.client.ProtocolResponseInterceptorContext
11-
import aws.smithy.kotlin.runtime.client.config.HttpChecksumConfigOption
11+
import aws.smithy.kotlin.runtime.client.config.ResponseHttpChecksumConfig
1212
import aws.smithy.kotlin.runtime.collections.AttributeKey
1313
import aws.smithy.kotlin.runtime.hashing.toHashFunctionOrThrow
1414
import aws.smithy.kotlin.runtime.http.HttpBody
@@ -46,7 +46,7 @@ internal val CHECKSUM_HEADER_VALIDATION_PRIORITY_LIST: List<String> = listOf(
4646
@InternalApi
4747
public open class FlexibleChecksumsResponseInterceptor(
4848
private val responseValidationRequired: Boolean,
49-
private val responseChecksumValidation: HttpChecksumConfigOption?,
49+
private val responseChecksumValidation: ResponseHttpChecksumConfig?,
5050
) : HttpInterceptor {
5151
@InternalApi
5252
public companion object {
@@ -57,7 +57,7 @@ public open class FlexibleChecksumsResponseInterceptor(
5757
override suspend fun modifyBeforeDeserialization(context: ProtocolResponseInterceptorContext<Any, HttpRequest, HttpResponse>): HttpResponse {
5858
val logger = coroutineContext.logger<FlexibleChecksumsResponseInterceptor>()
5959

60-
val configuredToVerifyChecksum = responseValidationRequired || responseChecksumValidation == HttpChecksumConfigOption.WHEN_SUPPORTED
60+
val configuredToVerifyChecksum = responseValidationRequired || responseChecksumValidation == ResponseHttpChecksumConfig.WHEN_SUPPORTED
6161
if (!configuredToVerifyChecksum) return context.protocolResponse
6262

6363
val checksumHeader = CHECKSUM_HEADER_VALIDATION_PRIORITY_LIST
@@ -68,7 +68,7 @@ public open class FlexibleChecksumsResponseInterceptor(
6868

6969
val serviceChecksumValue = context.protocolResponse.headers[checksumHeader]!!
7070
if (ignoreChecksum(serviceChecksumValue)) {
71-
logger.warn { "Checksum detected but validation was skipped." }
71+
logger.info { "Checksum detected but validation was skipped." }
7272
return context.protocolResponse
7373
}
7474

@@ -95,7 +95,7 @@ public open class FlexibleChecksumsResponseInterceptor(
9595
return context.protocolResponse
9696
}
9797
is HttpBody.SourceContent, is HttpBody.ChannelContent -> {
98-
logger.debug { "Validating checksum during deserialization from $checksumHeader" }
98+
logger.debug { "Validating checksum after deserialization from $checksumHeader" }
9999

100100
return context.protocolResponse.copy(
101101
body = context.protocolResponse.body

0 commit comments

Comments
 (0)