Skip to content

Commit 69b7765

Browse files
0marperez0marperez
committed
fix: pr feedback v2 (get rid of caching non bytes http bodies)
1 parent aa714d9 commit 69b7765

File tree

6 files changed

+122
-70
lines changed

6 files changed

+122
-70
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -255,7 +255,7 @@ public final class aws/smithy/kotlin/runtime/http/engine/internal/ManagedHttpCli
255255
public static final fun manage (Laws/smithy/kotlin/runtime/http/engine/HttpClientEngine;)Laws/smithy/kotlin/runtime/http/engine/HttpClientEngine;
256256
}
257257

258-
public abstract class aws/smithy/kotlin/runtime/http/interceptors/AbstractChecksumInterceptor : aws/smithy/kotlin/runtime/client/Interceptor {
258+
public abstract class aws/smithy/kotlin/runtime/http/interceptors/CachingChecksumInterceptor : aws/smithy/kotlin/runtime/client/Interceptor {
259259
public fun <init> ()V
260260
public abstract fun applyChecksum (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;Ljava/lang/String;)Laws/smithy/kotlin/runtime/http/request/HttpRequest;
261261
public abstract fun calculateChecksum (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
@@ -331,7 +331,7 @@ public final class aws/smithy/kotlin/runtime/http/interceptors/DiscoveredEndpoin
331331
public fun readBeforeTransmit (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;)V
332332
}
333333

334-
public final class aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptor : aws/smithy/kotlin/runtime/http/interceptors/AbstractChecksumInterceptor {
334+
public final class aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptor : aws/smithy/kotlin/runtime/http/interceptors/CachingChecksumInterceptor {
335335
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;
@@ -367,7 +367,7 @@ public final class aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksums
367367
public final fun getChecksumHeaderValidated ()Laws/smithy/kotlin/runtime/collections/AttributeKey;
368368
}
369369

370-
public final class aws/smithy/kotlin/runtime/http/interceptors/HttpChecksumRequiredInterceptor : aws/smithy/kotlin/runtime/http/interceptors/AbstractChecksumInterceptor {
370+
public final class aws/smithy/kotlin/runtime/http/interceptors/HttpChecksumRequiredInterceptor : aws/smithy/kotlin/runtime/http/interceptors/CachingChecksumInterceptor {
371371
public fun <init> ()V
372372
public fun applyChecksum (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;Ljava/lang/String;)Laws/smithy/kotlin/runtime/http/request/HttpRequest;
373373
public fun calculateChecksum (Laws/smithy/kotlin/runtime/client/ProtocolRequestInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;

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

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,13 @@ package aws.smithy.kotlin.runtime.http.interceptors
77

88
import aws.smithy.kotlin.runtime.InternalApi
99
import aws.smithy.kotlin.runtime.client.ProtocolRequestInterceptorContext
10-
import aws.smithy.kotlin.runtime.http.operation.HttpOperationContext
1110
import aws.smithy.kotlin.runtime.http.request.HttpRequest
1211

1312
/**
14-
* Handles checksum calculation so that checksums will be cached during retry loop
13+
* Enables inheriting [HttpInterceptor]s to use checksums caching
1514
*/
1615
@InternalApi
17-
public abstract class AbstractChecksumInterceptor : HttpInterceptor {
16+
public abstract class CachingChecksumInterceptor : HttpInterceptor {
1817
private var cachedChecksum: String? = null
1918

2019
override suspend fun modifyBeforeSigning(context: ProtocolRequestInterceptorContext<Any, HttpRequest>): HttpRequest {
@@ -31,9 +30,3 @@ public abstract class AbstractChecksumInterceptor : HttpInterceptor {
3130

3231
public abstract fun applyChecksum(context: ProtocolRequestInterceptorContext<Any, HttpRequest>, checksum: String): HttpRequest
3332
}
34-
35-
/**
36-
* @return The default checksum algorithm name, null if default checksums are disabled.
37-
*/
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/ChecksumInterceptorUtils.kt‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
package aws.smithy.kotlin.runtime.http.interceptors
2+
3+
import aws.smithy.kotlin.runtime.businessmetrics.emitBusinessMetric
4+
import aws.smithy.kotlin.runtime.client.ProtocolRequestInterceptorContext
5+
import aws.smithy.kotlin.runtime.hashing.HashFunction
6+
import aws.smithy.kotlin.runtime.hashing.resolveChecksumAlgorithmHeaderName
7+
import aws.smithy.kotlin.runtime.hashing.toBusinessMetric
8+
import aws.smithy.kotlin.runtime.http.operation.HttpOperationContext
9+
import aws.smithy.kotlin.runtime.http.request.HttpRequest
10+
import aws.smithy.kotlin.runtime.http.request.toBuilder
11+
import aws.smithy.kotlin.runtime.http.toCompletingBody
12+
import aws.smithy.kotlin.runtime.http.toHashingBody
13+
import kotlinx.coroutines.CompletableDeferred
14+
import kotlinx.coroutines.job
15+
16+
/**
17+
* Configures [HttpRequest] with AWS chunked streaming to calculate checksum during transmission
18+
* @return [HttpRequest]
19+
*/
20+
internal fun calculateAwsChunkedStreamingChecksum(
21+
context: ProtocolRequestInterceptorContext<Any, HttpRequest>,
22+
checksumAlgorithm: HashFunction,
23+
): HttpRequest {
24+
val request = context.protocolRequest.toBuilder()
25+
val deferredChecksum = CompletableDeferred<String>(context.executionContext.coroutineContext.job)
26+
val checksumHeader = checksumAlgorithm.resolveChecksumAlgorithmHeaderName()
27+
28+
request.body = request.body
29+
.toHashingBody(checksumAlgorithm, request.body.contentLength)
30+
.toCompletingBody(deferredChecksum)
31+
32+
request.headers.append("x-amz-trailer", checksumHeader)
33+
request.trailingHeaders.append(checksumHeader, deferredChecksum)
34+
35+
context.executionContext.emitBusinessMetric(checksumAlgorithm.toBusinessMetric())
36+
37+
return request.build()
38+
}
39+
40+
/**
41+
* @return The default checksum algorithm name in the execution context, null if default checksums are disabled.
42+
*/
43+
internal val ProtocolRequestInterceptorContext<Any, HttpRequest>.defaultChecksumAlgorithmName: String?
44+
get() = executionContext.getOrNull(HttpOperationContext.DefaultChecksumAlgorithm)

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

Lines changed: 29 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,6 @@ import aws.smithy.kotlin.runtime.io.*
1717
import aws.smithy.kotlin.runtime.telemetry.logging.Logger
1818
import aws.smithy.kotlin.runtime.telemetry.logging.logger
1919
import aws.smithy.kotlin.runtime.text.encoding.encodeBase64String
20-
import kotlinx.coroutines.CompletableDeferred
21-
import kotlinx.coroutines.job
2220
import kotlin.coroutines.coroutineContext
2321

2422
/**
@@ -50,7 +48,7 @@ public class FlexibleChecksumsRequestInterceptor(
5048
private val requestChecksumRequired: Boolean,
5149
private val requestChecksumCalculation: RequestHttpChecksumConfig?,
5250
private val requestChecksumAlgorithm: String?,
53-
) : AbstractChecksumInterceptor() {
51+
) : CachingChecksumInterceptor() {
5452
override suspend fun modifyBeforeSigning(context: ProtocolRequestInterceptorContext<Any, HttpRequest>): HttpRequest {
5553
val logger = coroutineContext.logger<FlexibleChecksumsRequestInterceptor>()
5654

@@ -68,25 +66,17 @@ public class FlexibleChecksumsRequestInterceptor(
6866
requestChecksumAlgorithm,
6967
context,
7068
)?.let { checksumAlgorithm ->
71-
if (context.protocolRequest.body.isEligibleForAwsChunkedStreaming) { // Handle checksum calculation here
69+
return if (context.protocolRequest.body.isEligibleForAwsChunkedStreaming) {
7270
logger.debug { "Calculating checksum during transmission using: ${checksumAlgorithm::class.simpleName}" }
73-
74-
val request = context.protocolRequest.toBuilder()
75-
val deferredChecksum = CompletableDeferred<String>(context.executionContext.coroutineContext.job)
76-
val checksumHeader = checksumAlgorithm.resolveChecksumAlgorithmHeaderName()
77-
78-
request.body = request.body
79-
.toHashingBody(checksumAlgorithm, request.body.contentLength)
80-
.toCompletingBody(deferredChecksum)
81-
82-
request.headers.append("x-amz-trailer", checksumHeader)
83-
request.trailingHeaders.append(checksumHeader, deferredChecksum)
84-
85-
context.executionContext.emitBusinessMetric(checksumAlgorithm.toBusinessMetric())
86-
87-
return request.build()
88-
} else { // Delegate checksum calculation to super class, calculateChecksum, and applyChecksum
89-
return super.modifyBeforeSigning(context)
71+
calculateAwsChunkedStreamingChecksum(context, checksumAlgorithm)
72+
} else {
73+
if (context.protocolRequest.body is HttpBody.Bytes) {
74+
// Cache checksum
75+
super.modifyBeforeSigning(context)
76+
} else {
77+
val checksum = calculateFlexibleChecksumsChecksum(context)
78+
applyFlexibleChecksumsChecksum(context, checksum)
79+
}
9080
}
9181
}
9282

@@ -113,8 +103,12 @@ public class FlexibleChecksumsRequestInterceptor(
113103
it.isSupportedForFlexibleChecksums
114104
}
115105

116-
// Handles calculating checksum for non-aws-chunked requests
117-
override suspend fun calculateChecksum(context: ProtocolRequestInterceptorContext<Any, HttpRequest>): String? {
106+
/**
107+
* Calculates a checksum based on the requirements and limitations of [FlexibleChecksumsRequestInterceptor]
108+
*/
109+
private suspend fun calculateFlexibleChecksumsChecksum(
110+
context: ProtocolRequestInterceptorContext<Any, HttpRequest>,
111+
): String {
118112
val req = context.protocolRequest.toBuilder()
119113
val checksumAlgorithm = resolveChecksumAlgorithm(
120114
requestChecksumRequired,
@@ -136,8 +130,13 @@ public class FlexibleChecksumsRequestInterceptor(
136130
}
137131
}
138132

139-
// Handles applying checksum for non-aws-chunked requests
140-
override fun applyChecksum(
133+
override suspend fun calculateChecksum(context: ProtocolRequestInterceptorContext<Any, HttpRequest>): String? =
134+
calculateFlexibleChecksumsChecksum(context)
135+
136+
/**
137+
* Applies a checksum based on the requirements and limitations of [FlexibleChecksumsRequestInterceptor]
138+
*/
139+
private fun applyFlexibleChecksumsChecksum(
141140
context: ProtocolRequestInterceptorContext<Any, HttpRequest>,
142141
checksum: String,
143142
): HttpRequest {
@@ -157,6 +156,11 @@ public class FlexibleChecksumsRequestInterceptor(
157156
return request.build()
158157
}
159158

159+
override fun applyChecksum(
160+
context: ProtocolRequestInterceptorContext<Any, HttpRequest>,
161+
checksum: String,
162+
): HttpRequest = applyFlexibleChecksumsChecksum(context, checksum)
163+
160164
/**
161165
* Checks if a user provided a checksum for a request via an HTTP header.
162166
* The header must start with "x-amz-checksum-" followed by the checksum algorithm's name.

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

Lines changed: 38 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
package aws.smithy.kotlin.runtime.http.interceptors
77

88
import aws.smithy.kotlin.runtime.InternalApi
9-
import aws.smithy.kotlin.runtime.businessmetrics.emitBusinessMetric
109
import aws.smithy.kotlin.runtime.client.ProtocolRequestInterceptorContext
1110
import aws.smithy.kotlin.runtime.hashing.*
1211
import aws.smithy.kotlin.runtime.http.*
@@ -16,15 +15,13 @@ import aws.smithy.kotlin.runtime.http.request.toBuilder
1615
import aws.smithy.kotlin.runtime.io.rollingHash
1716
import aws.smithy.kotlin.runtime.telemetry.logging.logger
1817
import aws.smithy.kotlin.runtime.text.encoding.encodeBase64String
19-
import kotlinx.coroutines.CompletableDeferred
20-
import kotlinx.coroutines.job
2118
import kotlin.coroutines.coroutineContext
2219

2320
/**
2421
* Handles checksum request calculation from the `httpChecksumRequired` trait.
2522
*/
2623
@InternalApi
27-
public class HttpChecksumRequiredInterceptor : AbstractChecksumInterceptor() {
24+
public class HttpChecksumRequiredInterceptor : CachingChecksumInterceptor() {
2825
override suspend fun modifyBeforeSigning(context: ProtocolRequestInterceptorContext<Any, HttpRequest>): HttpRequest {
2926
if (context.defaultChecksumAlgorithmName == null) {
3027
// Don't calculate checksum
@@ -34,31 +31,28 @@ public class HttpChecksumRequiredInterceptor : AbstractChecksumInterceptor() {
3431
val checksumAlgorithmName = context.defaultChecksumAlgorithmName!!
3532
val checksumAlgorithm = checksumAlgorithmName.toHashFunctionOrThrow()
3633

37-
val logger = coroutineContext.logger<HttpChecksumRequiredInterceptor>()
38-
39-
if (context.protocolRequest.body.isEligibleForAwsChunkedStreaming) { // Handle checksum calculation here
40-
logger.debug { "Calculating checksum during transmission using: ${checksumAlgorithm::class.simpleName}" }
41-
42-
val request = context.protocolRequest.toBuilder()
43-
val deferredChecksum = CompletableDeferred<String>(context.executionContext.coroutineContext.job)
44-
val checksumHeader = checksumAlgorithm.resolveChecksumAlgorithmHeaderName()
45-
46-
request.body = request.body
47-
.toHashingBody(checksumAlgorithm, request.body.contentLength)
48-
.toCompletingBody(deferredChecksum)
49-
50-
request.headers.append("x-amz-trailer", checksumHeader)
51-
request.trailingHeaders.append(checksumHeader, deferredChecksum)
52-
53-
context.executionContext.emitBusinessMetric(checksumAlgorithm.toBusinessMetric())
54-
55-
return request.build()
56-
} else { // Delegate checksum calculation to super class, calculateChecksum, and applyChecksum
57-
return super.modifyBeforeSigning(context)
34+
return if (context.protocolRequest.body.isEligibleForAwsChunkedStreaming) {
35+
coroutineContext.logger<HttpChecksumRequiredInterceptor>().debug {
36+
"Calculating checksum during transmission using: ${checksumAlgorithm::class.simpleName}"
37+
}
38+
calculateAwsChunkedStreamingChecksum(context, checksumAlgorithm)
39+
} else {
40+
if (context.protocolRequest.body is HttpBody.Bytes) {
41+
// Cache checksum
42+
super.modifyBeforeSigning(context)
43+
} else {
44+
val checksum = calculateHttpChecksumRequiredChecksum(context)
45+
applyHttpChecksumRequiredChecksum(context, checksum)
46+
}
5847
}
5948
}
6049

61-
public override suspend fun calculateChecksum(context: ProtocolRequestInterceptorContext<Any, HttpRequest>): String? {
50+
/**
51+
* Calculates a checksum based on the requirements and limitations of [HttpChecksumRequiredInterceptor]
52+
*/
53+
private suspend fun calculateHttpChecksumRequiredChecksum(
54+
context: ProtocolRequestInterceptorContext<Any, HttpRequest>,
55+
): String {
6256
val req = context.protocolRequest.toBuilder()
6357
val checksumAlgorithmName = context.defaultChecksumAlgorithmName!!
6458
val checksumAlgorithm = checksumAlgorithmName.toHashFunctionOrThrow()
@@ -76,12 +70,29 @@ public class HttpChecksumRequiredInterceptor : AbstractChecksumInterceptor() {
7670
}
7771
}
7872

79-
public override fun applyChecksum(context: ProtocolRequestInterceptorContext<Any, HttpRequest>, checksum: String): HttpRequest {
73+
public override suspend fun calculateChecksum(
74+
context: ProtocolRequestInterceptorContext<Any, HttpRequest>,
75+
): String? =
76+
calculateHttpChecksumRequiredChecksum(context)
77+
78+
/**
79+
* Applies a checksum based on the requirements and limitations of [HttpChecksumRequiredInterceptor]
80+
*/
81+
private fun applyHttpChecksumRequiredChecksum(
82+
context: ProtocolRequestInterceptorContext<Any, HttpRequest>,
83+
checksum: String,
84+
): HttpRequest {
8085
val checksumAlgorithmName = context.defaultChecksumAlgorithmName!!
8186
val checksumHeader = checksumAlgorithmName.resolveChecksumAlgorithmHeaderName()
8287
val request = context.protocolRequest.toBuilder()
8388

8489
request.header(checksumHeader, checksum)
8590
return request.build()
8691
}
92+
93+
public override fun applyChecksum(
94+
context: ProtocolRequestInterceptorContext<Any, HttpRequest>,
95+
checksum: String,
96+
): HttpRequest =
97+
applyHttpChecksumRequiredChecksum(context, checksum)
8798
}

‎runtime/protocol/http-client/common/test/aws/smithy/kotlin/runtime/http/interceptors/AbstractChecksumInterceptorTest.kt‎ renamed to ‎runtime/protocol/http-client/common/test/aws/smithy/kotlin/runtime/http/interceptors/CachingChecksumInterceptorTest.kt‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import aws.smithy.kotlin.runtime.client.ProtocolRequestInterceptorContext
66
import aws.smithy.kotlin.runtime.collections.get
77
import aws.smithy.kotlin.runtime.http.HttpBody
88
import aws.smithy.kotlin.runtime.http.SdkHttpClient
9-
import aws.smithy.kotlin.runtime.http.interceptors.AbstractChecksumInterceptor
9+
import aws.smithy.kotlin.runtime.http.interceptors.CachingChecksumInterceptor
1010
import aws.smithy.kotlin.runtime.http.operation.HttpOperationContext
1111
import aws.smithy.kotlin.runtime.http.operation.newTestOperation
1212
import aws.smithy.kotlin.runtime.http.operation.roundTrip
@@ -21,7 +21,7 @@ import kotlin.test.assertEquals
2121

2222
private val CHECKSUM_TEST_HEADER = "x-amz-kotlin-sdk-test-checksum-header"
2323

24-
class AbstractChecksumInterceptorTest {
24+
class CachingChecksumInterceptorTest {
2525
private val client = SdkHttpClient(TestEngine())
2626

2727
@Test
@@ -33,7 +33,7 @@ class AbstractChecksumInterceptorTest {
3333

3434
val op = newTestOperation<Unit, Unit>(req, Unit)
3535

36-
op.interceptors.add(TestAbstractChecksumInterceptor(expectedChecksumValue))
36+
op.interceptors.add(TestCachingChecksumInterceptor(expectedChecksumValue))
3737

3838
op.roundTrip(client, Unit)
3939
val call = op.context.attributes[HttpOperationContext.HttpCallList].first()
@@ -49,16 +49,16 @@ class AbstractChecksumInterceptorTest {
4949

5050
val op = newTestOperation<Unit, Unit>(req, Unit)
5151

52-
op.interceptors.add(TestAbstractChecksumInterceptor(expectedChecksumValue))
52+
op.interceptors.add(TestCachingChecksumInterceptor(expectedChecksumValue))
5353

5454
// the TestAbstractChecksumInterceptor will throw an exception if calculateChecksum is called more than once.
5555
op.roundTrip(client, Unit)
5656
op.roundTrip(client, Unit)
5757
}
5858

59-
inner class TestAbstractChecksumInterceptor(
59+
inner class TestCachingChecksumInterceptor(
6060
private val expectedChecksum: String?,
61-
) : AbstractChecksumInterceptor() {
61+
) : CachingChecksumInterceptor() {
6262
private var alreadyCalculatedChecksum = false
6363

6464
override suspend fun calculateChecksum(context: ProtocolRequestInterceptorContext<Any, HttpRequest>): String? {

0 commit comments

Comments
 (0)