Skip to content

Commit db65d74

Browse files
0marperez0marperez
committed
PR feedback
1 parent 4edabfb commit db65d74

File tree

7 files changed

+119
-56
lines changed

7 files changed

+119
-56
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,7 +345,7 @@ public final class aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksums
345345
public final class aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsResponseInterceptor : aws/smithy/kotlin/runtime/client/Interceptor {
346346
public static final field Companion Laws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsResponseInterceptor$Companion;
347347
public fun <init> (Lkotlin/jvm/functions/Function1;)V
348-
public fun <init> (ZLaws/smithy/kotlin/runtime/client/config/HttpChecksumConfigOption;Z)V
348+
public fun <init> (ZLaws/smithy/kotlin/runtime/client/config/HttpChecksumConfigOption;)V
349349
public fun modifyBeforeAttemptCompletion-gIAlu-s (Laws/smithy/kotlin/runtime/client/ResponseInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
350350
public fun modifyBeforeCompletion-gIAlu-s (Laws/smithy/kotlin/runtime/client/ResponseInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
351351
public fun modifyBeforeDeserialization (Laws/smithy/kotlin/runtime/client/ProtocolResponseInterceptorContext;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;

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

Lines changed: 16 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ import kotlinx.coroutines.job
2626
import kotlin.coroutines.coroutineContext
2727

2828
/**
29-
* Handles request checksums.
29+
* Handles request checksums for operations with the [HttpChecksumTrait] applied.
3030
*
3131
* If a user supplies a checksum via an HTTP header no calculation will be done. The exception is MD5, if a user
3232
* supplies an MD5 checksum header it will be ignored.
@@ -78,17 +78,11 @@ public class FlexibleChecksumsRequestInterceptor<I>(
7878
append("x-amz-checksum-")
7979
append(requestChecksumAlgorithm?.lowercase() ?: "crc32")
8080
}
81-
private val checksumAlgorithm = requestChecksumAlgorithm?.let {
82-
val hashFunction = requestChecksumAlgorithm.toHashFunction()
83-
if (hashFunction == null || !hashFunction.isSupported) {
84-
throw ClientException("Checksum algorithm '$requestChecksumAlgorithm' is not supported for flexible checksums")
85-
}
86-
hashFunction
87-
} ?: if (requestChecksumRequired || requestChecksumCalculation == HttpChecksumConfigOption.WHEN_SUPPORTED) {
88-
Crc32()
89-
} else {
90-
null
91-
}
81+
82+
private val checksumAlgorithm = requestChecksumAlgorithm
83+
?.toHashFunctionOrThrow()
84+
?.takeIf { it.isSupported }
85+
?: (Crc32().takeIf { requestChecksumRequired || requestChecksumCalculation == HttpChecksumConfigOption.WHEN_SUPPORTED })
9286

9387
// TODO: Remove in next minor version bump
9488
@Deprecated("readAfterSerialization is no longer used but can't be removed due to backwards incompatibility")
@@ -118,13 +112,8 @@ public class FlexibleChecksumsRequestInterceptor<I>(
118112
val deferredChecksum = CompletableDeferred<String>(context.executionContext.coroutineContext.job)
119113

120114
request.body = request.body
121-
.toHashingBody(
122-
checksumAlgorithm,
123-
request.body.contentLength,
124-
)
125-
.toCompletingBody(
126-
deferredChecksum,
127-
)
115+
.toHashingBody(checksumAlgorithm, request.body.contentLength)
116+
.toCompletingBody(deferredChecksum)
128117

129118
request.headers.append("x-amz-trailer", checksumHeader)
130119
request.trailingHeaders.append(checksumHeader, deferredChecksum)
@@ -258,21 +247,15 @@ public class FlexibleChecksumsRequestInterceptor<I>(
258247
* The header must start with "x-amz-checksum-" followed by the checksum algorithm's name.
259248
* MD5 is not considered a supported checksum algorithm.
260249
*/
261-
private fun HttpRequest.userProvidedChecksumHeader(logger: Logger): String? {
262-
this.headers.entries().forEach { header ->
263-
val headerName = header.key.lowercase()
264-
if (headerName.startsWith("x-amz-checksum-")) {
265-
if (headerName == "x-amz-checksum-md5") {
266-
logger.debug {
267-
"MD5 checksum was supplied via header, MD5 is not a supported algorithm, ignoring header"
268-
}
269-
} else {
270-
return headerName
271-
}
272-
}
250+
private fun HttpRequest.userProvidedChecksumHeader(logger: Logger) = headers
251+
.names()
252+
.filter { it.startsWith("x-amz-checksum-", ignoreCase = true) }
253+
.filterNot { key ->
254+
key
255+
.equals("x-amz-checksum-md5", ignoreCase = true)
256+
.also { if (it) logger.debug { "MD5 checksum was supplied via header, MD5 is not a supported algorithm, ignoring header" } }
273257
}
274-
return null
275-
}
258+
.firstOrNull()
276259

277260
/**
278261
* Maps supported hash functions to business metrics.

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

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,6 @@ internal val CHECKSUM_HEADER_VALIDATION_PRIORITY_LIST: List<String> = listOf(
4646
public class FlexibleChecksumsResponseInterceptor<I>(
4747
private val responseValidationRequired: Boolean,
4848
private val responseChecksumValidation: HttpChecksumConfigOption?,
49-
private val serviceUsesCompositeChecksums: Boolean,
5049
) : HttpInterceptor {
5150

5251
// FIXME: Remove in next minor version bump
@@ -56,7 +55,6 @@ public class FlexibleChecksumsResponseInterceptor<I>(
5655
) : this(
5756
false,
5857
HttpChecksumConfigOption.WHEN_REQUIRED,
59-
false,
6058
)
6159

6260
@InternalApi
@@ -78,11 +76,6 @@ public class FlexibleChecksumsResponseInterceptor<I>(
7876
}
7977
val serviceChecksumValue = context.protocolResponse.headers[checksumHeader]!!
8078

81-
if (serviceUsesCompositeChecksums && serviceChecksumValue.isCompositeChecksum()) {
82-
logger.debug { "Service returned composite checksum. Skipping validation." }
83-
return context.protocolResponse
84-
}
85-
8679
context.executionContext[ChecksumHeaderValidated] = checksumHeader
8780

8881
val checksumAlgorithm = checksumHeader
@@ -148,13 +141,3 @@ private fun validateAndThrow(expected: String, actual: String) {
148141
throw ChecksumMismatchException("Checksum mismatch. Expected $expected but was $actual")
149142
}
150143
}
151-
152-
/**
153-
* Verifies if a checksum is composite.
154-
* Composite checksums are used for multipart uploads.
155-
*/
156-
private fun String.isCompositeChecksum(): Boolean {
157-
// Ends with "-#" where "#" is a number
158-
val regex = Regex("-(\\d)+$")
159-
return regex.containsMatchIn(this)
160-
}

‎runtime/protocol/http-client/common/test/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsRequestInterceptorTest.kt‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,5 +202,50 @@ class FlexibleChecksumsRequestInterceptorTest {
202202
assertEquals(precalculatedChecksumValue, call.request.headers["x-amz-checksum-sha256"])
203203
}
204204

205+
@Test
206+
fun testDefaultChecksumConfiguration() = runTest {
207+
setOf(
208+
DefaultChecksumTest(true, HttpChecksumConfigOption.WHEN_SUPPORTED, true),
209+
DefaultChecksumTest(true, HttpChecksumConfigOption.WHEN_REQUIRED, true),
210+
DefaultChecksumTest(false, HttpChecksumConfigOption.WHEN_SUPPORTED, true),
211+
DefaultChecksumTest(false, HttpChecksumConfigOption.WHEN_REQUIRED, false),
212+
).forEach { runDefaultChecksumTest(it) }
213+
}
214+
205215
private fun Headers.getNumChecksumHeaders(): Int = entries().count { (name, _) -> name.startsWith("x-amz-checksum-") }
216+
217+
private data class DefaultChecksumTest(
218+
val requestChecksumRequired: Boolean,
219+
val requestChecksumCalculation: HttpChecksumConfigOption,
220+
val defaultChecksumExpected: Boolean,
221+
)
222+
223+
private fun runDefaultChecksumTest(
224+
testCase: DefaultChecksumTest,
225+
) = runTest {
226+
val defaultChecksumAlgorithmName = "crc32"
227+
val expectedChecksumValue = "WdqXHQ=="
228+
229+
val req = HttpRequestBuilder().apply {
230+
body = HttpBody.fromBytes("<Foo>bar</Foo>".encodeToByteArray())
231+
}
232+
233+
val op = newTestOperation<Unit, Unit>(req, Unit)
234+
235+
op.interceptors.add(
236+
FlexibleChecksumsRequestInterceptor<Unit>(
237+
requestChecksumAlgorithm = null, // See if default checksum is applied
238+
requestChecksumRequired = testCase.requestChecksumRequired,
239+
requestChecksumCalculation = testCase.requestChecksumCalculation,
240+
),
241+
)
242+
243+
op.roundTrip(client, Unit)
244+
val call = op.context.attributes[HttpOperationContext.HttpCallList].first()
245+
246+
when (testCase.defaultChecksumExpected) {
247+
true -> assertEquals(expectedChecksumValue, call.request.headers["x-amz-checksum-$defaultChecksumAlgorithmName"])
248+
false -> assertFalse { call.request.headers.contains("x-amz-checksum-$defaultChecksumAlgorithmName") }
249+
}
250+
}
206251
}

‎runtime/protocol/http-client/common/test/aws/smithy/kotlin/runtime/http/interceptors/FlexibleChecksumsResponseInterceptorTest.kt‎

Lines changed: 48 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@ class FlexibleChecksumsResponseInterceptorTest {
7777
FlexibleChecksumsResponseInterceptor<TestInput>(
7878
responseValidationRequired = true,
7979
responseChecksumValidation = HttpChecksumConfigOption.WHEN_SUPPORTED,
80-
serviceUsesCompositeChecksums = false,
8180
),
8281
)
8382

@@ -105,7 +104,6 @@ class FlexibleChecksumsResponseInterceptorTest {
105104
FlexibleChecksumsResponseInterceptor<TestInput>(
106105
responseValidationRequired = true,
107106
responseChecksumValidation = HttpChecksumConfigOption.WHEN_SUPPORTED,
108-
serviceUsesCompositeChecksums = false,
109107
),
110108
)
111109

@@ -134,7 +132,6 @@ class FlexibleChecksumsResponseInterceptorTest {
134132
FlexibleChecksumsResponseInterceptor<TestInput>(
135133
responseValidationRequired = true,
136134
responseChecksumValidation = HttpChecksumConfigOption.WHEN_SUPPORTED,
137-
serviceUsesCompositeChecksums = false,
138135
),
139136
)
140137

@@ -160,7 +157,6 @@ class FlexibleChecksumsResponseInterceptorTest {
160157
FlexibleChecksumsResponseInterceptor<TestInput>(
161158
responseValidationRequired = true,
162159
responseChecksumValidation = HttpChecksumConfigOption.WHEN_SUPPORTED,
163-
serviceUsesCompositeChecksums = false,
164160
),
165161
)
166162

@@ -182,7 +178,6 @@ class FlexibleChecksumsResponseInterceptorTest {
182178
FlexibleChecksumsResponseInterceptor<TestInput>(
183179
responseValidationRequired = false,
184180
responseChecksumValidation = HttpChecksumConfigOption.WHEN_REQUIRED,
185-
serviceUsesCompositeChecksums = false,
186181
),
187182
)
188183

@@ -199,4 +194,52 @@ class FlexibleChecksumsResponseInterceptorTest {
199194

200195
assertNull(op.context.getOrNull(ChecksumHeaderValidated))
201196
}
197+
198+
@Test
199+
fun testResponseValidationConfiguration() = runTest {
200+
setOf(
201+
ResponseChecksumValidationTest(true, HttpChecksumConfigOption.WHEN_SUPPORTED, true),
202+
ResponseChecksumValidationTest(true, HttpChecksumConfigOption.WHEN_REQUIRED, true),
203+
ResponseChecksumValidationTest(false, HttpChecksumConfigOption.WHEN_SUPPORTED, true),
204+
ResponseChecksumValidationTest(false, HttpChecksumConfigOption.WHEN_REQUIRED, false),
205+
).forEach { runResponseChecksumValidationTest(it) }
206+
}
207+
208+
private data class ResponseChecksumValidationTest(
209+
val responseValidationRequired: Boolean,
210+
val responseChecksumValidation: HttpChecksumConfigOption,
211+
val checksumValidationExpected: Boolean,
212+
)
213+
214+
private fun runResponseChecksumValidationTest(
215+
testCase: ResponseChecksumValidationTest,
216+
) = runTest {
217+
checksums.forEach { (checksumAlgorithmName, expectedChecksum) ->
218+
val req = HttpRequestBuilder()
219+
val op = newTestOperation<TestInput>(req)
220+
221+
op.interceptors.add(
222+
FlexibleChecksumsResponseInterceptor<TestInput>(
223+
responseValidationRequired = testCase.responseValidationRequired,
224+
responseChecksumValidation = testCase.responseChecksumValidation,
225+
),
226+
)
227+
228+
val responseChecksumHeaderName = "x-amz-checksum-$checksumAlgorithmName"
229+
230+
val responseHeaders = Headers {
231+
append(responseChecksumHeaderName, expectedChecksum)
232+
}
233+
234+
val client = getMockClient(response, responseHeaders)
235+
236+
val output = op.roundTrip(client, TestInput("input"))
237+
output.body.readAll()
238+
239+
when (testCase.checksumValidationExpected) {
240+
true -> assertEquals(responseChecksumHeaderName, op.context[ChecksumHeaderValidated])
241+
false -> assertNull(op.context.getOrNull(ChecksumHeaderValidated))
242+
}
243+
}
244+
}
202245
}

‎runtime/runtime-core/api/runtime-core.api‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -696,6 +696,7 @@ public final class aws/smithy/kotlin/runtime/hashing/HashFunctionKt {
696696
public static final fun hash ([BLaws/smithy/kotlin/runtime/hashing/HashFunction;)[B
697697
public static final fun hash ([BLkotlin/jvm/functions/Function0;)[B
698698
public static final fun toHashFunction (Ljava/lang/String;)Laws/smithy/kotlin/runtime/hashing/HashFunction;
699+
public static final fun toHashFunctionOrThrow (Ljava/lang/String;)Laws/smithy/kotlin/runtime/hashing/HashFunction;
699700
}
700701

701702
public final class aws/smithy/kotlin/runtime/hashing/HmacKt {

‎runtime/runtime-core/common/src/aws/smithy/kotlin/runtime/hashing/HashFunction.kt‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
*/
55
package aws.smithy.kotlin.runtime.hashing
66

7+
import aws.smithy.kotlin.runtime.ClientException
78
import aws.smithy.kotlin.runtime.InternalApi
89

910
/**
@@ -70,3 +71,10 @@ public fun String.toHashFunction(): HashFunction? = when (this.lowercase()) {
7071
"md5" -> Md5()
7172
else -> null
7273
}
74+
75+
/**
76+
* Return the [HashFunction] which is represented by this string, or an exception if none match.
77+
*/
78+
@InternalApi
79+
public fun String.toHashFunctionOrThrow(): HashFunction =
80+
toHashFunction() ?: throw ClientException("Checksum algorithm '$this' is not supported")

0 commit comments

Comments
 (0)