fix: http trace interceptor not keeping original HttpBody properties and LogMode not behaving as expected (#1035)

Author: aajtodd

.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"
+    ]
+}

.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"
+}

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,26 +112,35 @@ 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 { (key, values) ->
+            buffer.writeUtf8(values.joinToString(separator = ";", prefix = "$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
         }
     }
 
     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
+    }
+}

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 = assertNotNull(builder.body.readAll()?.decodeToString())
+        assertEquals(content, actualReplacedContent)
     }
 
     @Test

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"
     }
 

runtime/smithy-client/common/test/aws/smithy/kotlin/runtime/client/LogModeTest.kt

@@ -55,4 +55,15 @@ class LogModeTest {
     fun testUnsupportedCompositeLogMode() {
         assertFailsWith<ClientException> { 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))
+    }
 }