From 5c51e573a994aea7b19a763a9dddebde9e35456a Mon Sep 17 00:00:00 2001 From: Aaron J Todd Date: Mon, 12 Feb 2024 10:34:27 -0500 Subject: [PATCH] fix: http trace interceptor not keeping original HttpBody properties and LogMode not behaving as expected --- .../00960dac-40d8-4b6d-a3a7-12eb944d5753.json | 8 ++++++ .../4ecf93e9-b11e-43b8-9c7c-ed7753916c64.json | 5 ++++ .../http/request/HttpRequestBuilder.kt | 26 ++++++++++++------- .../runtime/http/HttpRequestBuilderTest.kt | 19 ++++++++------ .../smithy/kotlin/runtime/client/LogMode.kt | 4 +-- .../kotlin/runtime/client/LogModeTest.kt | 11 ++++++++ 6 files changed, 54 insertions(+), 19 deletions(-) create mode 100644 .changes/00960dac-40d8-4b6d-a3a7-12eb944d5753.json create mode 100644 .changes/4ecf93e9-b11e-43b8-9c7c-ed7753916c64.json diff --git a/.changes/00960dac-40d8-4b6d-a3a7-12eb944d5753.json b/.changes/00960dac-40d8-4b6d-a3a7-12eb944d5753.json new file mode 100644 index 000000000..5de40b0d5 --- /dev/null +++ b/.changes/00960dac-40d8-4b6d-a3a7-12eb944d5753.json @@ -0,0 +1,8 @@ +{ + "id": "00960dac-40d8-4b6d-a3a7-12eb944d5753", + "type": "bugfix", + "description": "Fix `PutObject` request when `LogMode` is set to `LogRequestWithBody`", + "issues": [ + "awslabs/aws-sdk-kotlin#1198" + ] +} \ No newline at end of file diff --git a/.changes/4ecf93e9-b11e-43b8-9c7c-ed7753916c64.json b/.changes/4ecf93e9-b11e-43b8-9c7c-ed7753916c64.json new file mode 100644 index 000000000..873ee0a39 --- /dev/null +++ b/.changes/4ecf93e9-b11e-43b8-9c7c-ed7753916c64.json @@ -0,0 +1,5 @@ +{ + "id": "4ecf93e9-b11e-43b8-9c7c-ed7753916c64", + "type": "bugfix", + "description": "Fix `LogRequestWithBody` and `LogResponseWithBody` imply `LogRequest` and `LogResponse` respectively" +} \ No newline at end of file diff --git a/runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/request/HttpRequestBuilder.kt b/runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/request/HttpRequestBuilder.kt index 7d0750ea8..29a868875 100644 --- a/runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/request/HttpRequestBuilder.kt +++ b/runtime/protocol/http/common/src/aws/smithy/kotlin/runtime/http/request/HttpRequestBuilder.kt @@ -6,7 +6,6 @@ package aws.smithy.kotlin.runtime.http.request import aws.smithy.kotlin.runtime.InternalApi import aws.smithy.kotlin.runtime.http.* -import aws.smithy.kotlin.runtime.http.content.ByteArrayContent import aws.smithy.kotlin.runtime.io.* import aws.smithy.kotlin.runtime.net.url.Url import aws.smithy.kotlin.runtime.util.CanDeepCopy @@ -113,22 +112,17 @@ public suspend fun dumpRequest(request: HttpRequestBuilder, dumpBody: Boolean): val skip = setOf("Host", "Content-Length") request.headers.entries() .filterNot { it.key in skip } - .forEach { - buffer.writeUtf8(it.value.joinToString(separator = ";", prefix = "${it.key}: ", postfix = "\r\n")) + .forEach { entry -> + buffer.writeUtf8(entry.value.joinToString(separator = ";", prefix = "${entry.key}: ", postfix = "\r\n")) } buffer.writeUtf8("\r\n") - if (dumpBody) { when (val body = request.body) { is HttpBody.Bytes -> buffer.write(body.bytes()) is HttpBody.ChannelContent, is HttpBody.SourceContent -> { // consume the stream and replace the body - val content = body.readAll() - if (content != null) { - buffer.write(content) - request.body = ByteArrayContent(content) - } + request.body = copyHttpBody(request.body, buffer) } is HttpBody.Empty -> { } // nothing to dump } @@ -136,3 +130,17 @@ public suspend fun dumpRequest(request: HttpRequestBuilder, dumpBody: Boolean): return buffer.readUtf8() } + +private suspend fun copyHttpBody(original: HttpBody, buffer: SdkBuffer): HttpBody { + val content = original.readAll() ?: return HttpBody.Empty + buffer.write(content) + return object : HttpBody.SourceContent() { + override fun readFrom(): SdkSource = + content.source() + + // even though we know the content length we preserve the original in case it was chunked encoding + override val contentLength: Long? = original.contentLength + override val isOneShot: Boolean = original.isOneShot + override val isDuplex: Boolean = original.isDuplex + } +} diff --git a/runtime/protocol/http/common/test/aws/smithy/kotlin/runtime/http/HttpRequestBuilderTest.kt b/runtime/protocol/http/common/test/aws/smithy/kotlin/runtime/http/HttpRequestBuilderTest.kt index a57e6c53d..b7199fed9 100644 --- a/runtime/protocol/http/common/test/aws/smithy/kotlin/runtime/http/HttpRequestBuilderTest.kt +++ b/runtime/protocol/http/common/test/aws/smithy/kotlin/runtime/http/HttpRequestBuilderTest.kt @@ -18,9 +18,7 @@ import aws.smithy.kotlin.runtime.net.Host import aws.smithy.kotlin.runtime.net.Scheme import aws.smithy.kotlin.runtime.net.url.Url import kotlinx.coroutines.test.runTest -import kotlin.test.Test -import kotlin.test.assertEquals -import kotlin.test.assertTrue +import kotlin.test.* class HttpRequestBuilderTest { @Test @@ -67,13 +65,18 @@ class HttpRequestBuilderTest { } assertTrue(builder.body is HttpBody.ChannelContent) - dumpRequest(builder, false) + val actualNoContent = dumpRequest(builder, false) + val expectedNoContent = "GET /debug/test?foo=bar\r\nHost: test.amazon.com\r\nContent-Length: ${content.length}\r\nx-baz: quux;qux\r\n\r\n" assertTrue(builder.body is HttpBody.ChannelContent) + assertEquals(expectedNoContent, actualNoContent) - val actual = dumpRequest(builder, true) - assertTrue(builder.body is HttpBody.Bytes) - val expected = "GET /debug/test?foo=bar\r\nHost: test.amazon.com\r\nContent-Length: ${content.length}\r\nx-baz: quux;qux\r\n\r\n$content" - assertEquals(expected, actual) + val actualWithContent = dumpRequest(builder, true) + assertTrue(builder.body is HttpBody.SourceContent) + val expectedWithContent = "$expectedNoContent$content" + assertEquals(expectedWithContent, actualWithContent) + + val actualReplacedContent = builder.body.readAll()?.decodeToString() ?: fail("expected content") + assertEquals(content, actualReplacedContent) } @Test diff --git a/runtime/smithy-client/common/src/aws/smithy/kotlin/runtime/client/LogMode.kt b/runtime/smithy-client/common/src/aws/smithy/kotlin/runtime/client/LogMode.kt index 9d26202c5..a940f6549 100644 --- a/runtime/smithy-client/common/src/aws/smithy/kotlin/runtime/client/LogMode.kt +++ b/runtime/smithy-client/common/src/aws/smithy/kotlin/runtime/client/LogMode.kt @@ -34,7 +34,7 @@ public sealed class LogMode(private val mask: Int) { /** * Log the request details as well as the body if possible */ - public object LogRequestWithBody : LogMode(0x02) { + public object LogRequestWithBody : LogMode(0x03) { override fun toString(): String = "LogRequestWithBody" } @@ -48,7 +48,7 @@ public sealed class LogMode(private val mask: Int) { /** * Log the response details as well as the body if possible */ - public object LogResponseWithBody : LogMode(0x08) { + public object LogResponseWithBody : LogMode(0x0C) { override fun toString(): String = "LogResponseWithBody" } diff --git a/runtime/smithy-client/common/test/aws/smithy/kotlin/runtime/client/LogModeTest.kt b/runtime/smithy-client/common/test/aws/smithy/kotlin/runtime/client/LogModeTest.kt index 8762d1979..484b9a0b3 100644 --- a/runtime/smithy-client/common/test/aws/smithy/kotlin/runtime/client/LogModeTest.kt +++ b/runtime/smithy-client/common/test/aws/smithy/kotlin/runtime/client/LogModeTest.kt @@ -55,4 +55,15 @@ class LogModeTest { fun testUnsupportedCompositeLogMode() { assertFailsWith { LogMode.fromString("LogRequest|UnsupportedLogMode") } } + + @Test + fun testWithBodyImpliesWithout() { + // LogRequestWithBody implies LogRequest + assertTrue(LogMode.LogRequestWithBody.isEnabled(LogMode.LogRequest)) + assertFalse(LogMode.LogRequestWithBody.isEnabled(LogMode.LogResponse)) + + // LogResponseWithBody implies LogResponse + assertTrue(LogMode.LogResponseWithBody.isEnabled(LogMode.LogResponse)) + assertFalse(LogMode.LogResponseWithBody.isEnabled(LogMode.LogRequest)) + } }