feat: Multipart request support #1548
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ import com.chuckerteam.chucker.R | |||||||||||||||||
| import com.chuckerteam.chucker.api.BodyDecoder | ||||||||||||||||||
| import com.chuckerteam.chucker.api.ChuckerCollector | ||||||||||||||||||
| import com.chuckerteam.chucker.internal.data.entity.HttpTransaction | ||||||||||||||||||
| import okhttp3.MultipartBody | ||||||||||||||||||
| import okhttp3.Request | ||||||||||||||||||
| import okio.Buffer | ||||||||||||||||||
| import okio.ByteString | ||||||||||||||||||
|
|
@@ -17,6 +18,11 @@ internal class RequestProcessor( | |||||||||||||||||
| private val headersToRedact: Set<String>, | ||||||||||||||||||
| private val bodyDecoders: List<BodyDecoder>, | ||||||||||||||||||
| ) { | ||||||||||||||||||
| private companion object { | ||||||||||||||||||
| const val MAX_PREFIX_LENGTH = 64L | ||||||||||||||||||
| const val MAX_CODEPOINTS_TO_CHECK = 16 | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+21
to
+24
There was a problem hiding this comment. The constants MAX_PREFIX_LENGTH and MAX_CODEPOINTS_TO_CHECK differ from the existing implementation in OkioUtils.kt (which uses MAX_PREFIX_SIZE = 64L and CODE_POINT_SIZE = 16). While the values are the same, using different constant names can cause confusion. If you switch to using Buffer.isProbablyPlainText as suggested, this inconsistency would be resolved. |
||||||||||||||||||
|
|
||||||||||||||||||
| fun process( | ||||||||||||||||||
| request: Request, | ||||||||||||||||||
| transaction: HttpTransaction, | ||||||||||||||||||
|
|
@@ -60,14 +66,22 @@ internal class RequestProcessor( | |||||||||||||||||
| return | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| if (body is MultipartBody) { | ||||||||||||||||||
| val content = processMultipartPayload(body) | ||||||||||||||||||
| transaction.requestBody = content | ||||||||||||||||||
| transaction.isRequestBodyEncoded = false | ||||||||||||||||||
| return | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| val requestSource = | ||||||||||||||||||
| try { | ||||||||||||||||||
| Buffer().apply { body.writeTo(this) } | ||||||||||||||||||
| } catch (e: IOException) { | ||||||||||||||||||
| Logger.error("Failed to read request payload", e) | ||||||||||||||||||
| return | ||||||||||||||||||
| } | ||||||||||||||||||
| val limitingSource = | ||||||||||||||||||
| LimitingSource(requestSource.uncompress(request.headers), maxContentLength) | ||||||||||||||||||
|
|
||||||||||||||||||
| val contentBuffer = Buffer().apply { limitingSource.use { writeAll(it) } } | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -79,6 +93,60 @@ internal class RequestProcessor( | |||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| private fun processMultipartPayload(body: MultipartBody): String { | ||||||||||||||||||
| return buildString { | ||||||||||||||||||
| body.parts.forEach { part -> | ||||||||||||||||||
| part.headers?.forEach { header -> | ||||||||||||||||||
| append(header.first + ": " + header.second + "\n") | ||||||||||||||||||
|
Member
There was a problem hiding this comment. can you string interpolate here? |
||||||||||||||||||
| } | ||||||||||||||||||
| val partBody = part.body | ||||||||||||||||||
| if (partBody.contentType() != null) { | ||||||||||||||||||
| append("Content-Type: ${partBody.contentType()}\n") | ||||||||||||||||||
| } | ||||||||||||||||||
| if (partBody.contentLength() != -1L) { | ||||||||||||||||||
| append("Content-Length: ${partBody.contentLength()}\n") | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| val buffer = Buffer() | ||||||||||||||||||
| partBody.writeTo(buffer) | ||||||||||||||||||
|
There was a problem hiding this comment. The call to partBody.writeTo(buffer) could throw an IOException but is not wrapped in a try-catch block. If this fails, the entire multipart processing would fail and potentially crash. Consider adding error handling similar to how it's done in processPayload for regular request bodies (lines 76-82).
Suggested change
|
||||||||||||||||||
|
|
||||||||||||||||||
| if (isPlainText(buffer)) { | ||||||||||||||||||
| append("\n") | ||||||||||||||||||
| append(buffer.readUtf8()) | ||||||||||||||||||
| } else { | ||||||||||||||||||
| append("\n(binary: ${partBody.contentLength()} bytes)") | ||||||||||||||||||
| } | ||||||||||||||||||
| append("\n\n") | ||||||||||||||||||
|
Comment on lines
+96
to
+119
There was a problem hiding this comment. Each multipart part's body is fully read into memory before checking against maxContentLength. For very large plain text parts, this could cause memory issues. Consider checking the accumulated length before reading each part, or implementing a streaming approach similar to how regular request bodies are handled with LimitingSource. |
||||||||||||||||||
|
|
||||||||||||||||||
| if (length >= maxContentLength) { | ||||||||||||||||||
| append(context.getString(R.string.chucker_body_content_truncated)) | ||||||||||||||||||
| return@buildString | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| @Suppress("TooGenericExceptionCaught", "SwallowedException") | ||||||||||||||||||
| private fun isPlainText(buffer: Buffer): Boolean { | ||||||||||||||||||
| try { | ||||||||||||||||||
| val prefix = Buffer() | ||||||||||||||||||
| val byteCount = if (buffer.size < MAX_PREFIX_LENGTH) buffer.size else MAX_PREFIX_LENGTH | ||||||||||||||||||
| buffer.copyTo(prefix, 0, byteCount) | ||||||||||||||||||
| repeat(MAX_CODEPOINTS_TO_CHECK) { | ||||||||||||||||||
| if (prefix.exhausted()) { | ||||||||||||||||||
| return@repeat | ||||||||||||||||||
| } | ||||||||||||||||||
| val codePoint = prefix.readUtf8CodePoint() | ||||||||||||||||||
| if (Character.isISOControl(codePoint) && !Character.isWhitespace(codePoint)) { | ||||||||||||||||||
| return false | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| return true | ||||||||||||||||||
| } catch (e: Exception) { | ||||||||||||||||||
| return false | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+129
to
+148
There was a problem hiding this comment. The isPlainText method duplicates existing functionality. The codebase already has a Buffer.isProbablyPlainText extension property in OkioUtils.kt that does the same thing. Consider using the existing utility instead of creating a duplicate implementation. This would reduce code duplication and ensure consistent behavior across the codebase. |
||||||||||||||||||
|
|
||||||||||||||||||
| private fun decodePayload( | ||||||||||||||||||
| request: Request, | ||||||||||||||||||
| body: ByteString, | ||||||||||||||||||
|
|
||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| package com.chuckerteam.chucker.api | ||
|
|
||
| import com.chuckerteam.chucker.util.ChuckerInterceptorDelegate | ||
| import com.chuckerteam.chucker.util.ClientFactory | ||
| import com.chuckerteam.chucker.util.NoLoggerRule | ||
| import com.chuckerteam.chucker.util.readByteStringBody | ||
| import com.google.common.truth.Truth.assertThat | ||
| import okhttp3.MediaType.Companion.toMediaType | ||
| import okhttp3.MultipartBody | ||
| import okhttp3.Request | ||
| import okhttp3.RequestBody.Companion.toRequestBody | ||
| import okhttp3.mockwebserver.MockResponse | ||
| import okhttp3.mockwebserver.MockWebServer | ||
| import org.junit.Rule | ||
| import org.junit.jupiter.api.extension.ExtendWith | ||
| import org.junit.jupiter.api.io.TempDir | ||
| import org.junit.jupiter.params.ParameterizedTest | ||
| import org.junit.jupiter.params.provider.EnumSource | ||
| import java.io.File | ||
|
|
||
| @ExtendWith(NoLoggerRule::class) | ||
| internal class ChuckerInterceptorMultipartTest { | ||
| @get:Rule | ||
| val server = MockWebServer() | ||
|
|
||
| private val serverUrl = server.url("/") | ||
|
|
||
| @TempDir | ||
| lateinit var tempDir: File | ||
|
|
||
| @ParameterizedTest | ||
| @EnumSource(value = ClientFactory::class) | ||
| fun `multipart body is formatted correctly`(factory: ClientFactory) { | ||
| val chuckerInterceptor = | ||
| ChuckerInterceptorDelegate( | ||
| cacheDirectoryProvider = { tempDir }, | ||
| ) | ||
| val client = factory.create(chuckerInterceptor) | ||
|
|
||
| val binaryData = byteArrayOf(0x00, 0x01, 0x02, 0x03) | ||
| val multipartBody = | ||
| MultipartBody | ||
| .Builder() | ||
| .setType(MultipartBody.FORM) | ||
| .addFormDataPart("title", "Square Logo") | ||
| .addFormDataPart( | ||
| "image", | ||
| "logo.png", | ||
| binaryData.toRequestBody("image/png".toMediaType()), | ||
| ).build() | ||
|
|
||
| val request = | ||
| Request | ||
| .Builder() | ||
| .url(serverUrl) | ||
| .post(multipartBody) | ||
| .build() | ||
| server.enqueue(MockResponse().setBody("OK")) | ||
|
|
||
| client.newCall(request).execute().readByteStringBody() | ||
|
|
||
| val transaction = chuckerInterceptor.expectTransaction() | ||
|
|
||
| // This assertion is what we WANT to see after the fix. | ||
|
|
||
| // Current behavior will likely fail this. | ||
| assertThat(transaction.requestBody).contains("Content-Disposition: form-data; name=\"title\"") | ||
| assertThat(transaction.requestBody).contains("Square Logo") | ||
| assertThat(transaction.requestBody).contains("Content-Disposition: form-data; name=\"image\"") | ||
| assertThat(transaction.requestBody).contains("filename=\"logo.png\"") | ||
| assertThat(transaction.requestBody).contains("Content-Type: image/png") | ||
| // Binary content should be replaced with placeholder | ||
| assertThat(transaction.requestBody).contains("(binary: 4 bytes)") | ||
| } | ||
|
Comment on lines
+31
to
+73
There was a problem hiding this comment. The test doesn't cover the case where multipart content exceeds maxContentLength. Consider adding a test case that validates the truncation behavior (line 121-123 in RequestProcessor.kt) works correctly for multipart bodies, similar to how it's tested for regular request bodies in ChuckerInterceptorTest. |
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to bump the DB version because of this