feat: Multipart request support#1548
feat: Multipart request support#1548ranjeetchouhan wants to merge 2 commits intoChuckerTeam:mainfrom
Conversation
| val body = "This is a body".toRequestBody("text/plain".toMediaType()) | ||
| multipart("Value 1", body).enqueue(noOpCallback) |
There was a problem hiding this comment.
| val body = "This is a body".toRequestBody("text/plain".toMediaType()) | |
| multipart("Value 1", body).enqueue(noOpCallback) | |
| val value2RequestBody = "This is a body".toRequestBody("text/plain".toMediaType()) | |
| multipart(value1 = "value1", value2 = value2RequestBody).enqueue(noOpCallback) |
| testObject.insertTransaction(transactionThree) | ||
| testObject.insertTransaction(transactionFour) | ||
| testObject.getFilteredTransactionTuples(code = "", path = "").observeForever { result -> | ||
| assertTuples(listOf(transactionFour, transactionThree, transactionOne, transactionTwo), result) |
There was a problem hiding this comment.
Can you send this + all the other related changes as a separate PR please?
|
|
||
| val transaction = chuckerInterceptor.expectTransaction() | ||
|
|
||
| // This assertion is what we WANT to see after the fix. |
| override fun onOptionsItemSelected(item: MenuItem) = | ||
| when (item.itemId) { | ||
| R.id.share_text -> | ||
| R.id.share_text -> { |
There was a problem hiding this comment.
please send those changes in a separate PR
| @ColumnInfo(name = "error") var error: String?, | ||
| @ColumnInfo(name = "graphQlDetected") var graphQlDetected: Boolean = false, | ||
| @ColumnInfo(name = "graphQlOperationName") var graphQlOperationName: String?, | ||
| @ColumnInfo(name = "requestContentType") var requestContentType: String?, |
There was a problem hiding this comment.
we need to bump the DB version because of this
| return buildString { | ||
| body.parts.forEach { part -> | ||
| part.headers?.forEach { header -> | ||
| append(header.first + ": " + header.second + "\n") |
There was a problem hiding this comment.
can you string interpolate here?
There was a problem hiding this comment.
Pull request overview
This PR adds multipart request body support to Chucker, allowing proper inspection and formatting of multipart/form-data requests. It also addresses deprecation warnings and various lint issues.
Key changes:
- Adds multipart request body processing with intelligent plain text vs. binary detection
- Extends transaction filtering to support searching by request content type
- Updates test utilities and adds comprehensive multipart test coverage
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| library/src/main/kotlin/com/chuckerteam/chucker/internal/support/RequestProcessor.kt | Adds processMultipartPayload method to format multipart bodies with headers and content, includes isPlainText helper |
| library/src/main/kotlin/com/chuckerteam/chucker/internal/data/room/HttpTransactionDao.kt | Updates database queries to include requestContentType field in tuple selection and filtering |
| library/src/main/kotlin/com/chuckerteam/chucker/internal/data/repository/HttpTransactionDatabaseRepository.kt | Passes contentTypeQuery parameter to enable filtering by request content type |
| library/src/main/kotlin/com/chuckerteam/chucker/internal/data/entity/HttpTransactionTuple.kt | Adds requestContentType field to tuple for efficient filtering |
| library/src/test/kotlin/com/chuckerteam/chucker/api/ChuckerInterceptorMultipartTest.kt | New test file validating multipart body formatting with text and binary parts |
| library/src/test/kotlin/com/chuckerteam/chucker/internal/data/repository/HttpTransactionDatabaseRepositoryTest.kt | Adds test for filtering transactions by requestContentType, fixes formatting |
| library/src/test/kotlin/com/chuckerteam/chucker/internal/data/entity/TransactionTestUtils.kt | Updates assertion helper to verify requestContentType field |
| library/src/test/kotlin/com/chuckerteam/chucker/internal/data/entity/HttpTransactionTupleTest.kt | Adds requestContentType parameter to test helper method |
| library/src/main/kotlin/com/chuckerteam/chucker/internal/ui/transaction/TransactionActivity.kt | Adds braces to when expression branches for lint compliance |
| sample/src/main/kotlin/com/chuckerteam/chucker/sample/HttpBinHttpTask.kt | Adds multipart endpoint demonstration using non-deprecated toRequestBody extension |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @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 | ||
| } | ||
| } |
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 companion object { | ||
| const val MAX_PREFIX_LENGTH = 64L | ||
| const val MAX_CODEPOINTS_TO_CHECK = 16 | ||
| } |
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.
| } | ||
|
|
||
| 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).
| partBody.writeTo(buffer) | |
| try { | |
| partBody.writeTo(buffer) | |
| } catch (e: IOException) { | |
| Logger.error("Failed to read multipart request payload", e) | |
| append(context.getString(R.string.chucker_body_content_truncated)) | |
| return@buildString | |
| } |
| @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)") | ||
| } |
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.
| private fun processMultipartPayload(body: MultipartBody): String { | ||
| return buildString { | ||
| body.parts.forEach { part -> | ||
| part.headers?.forEach { header -> | ||
| append(header.first + ": " + header.second + "\n") | ||
| } | ||
| 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) | ||
|
|
||
| if (isPlainText(buffer)) { | ||
| append("\n") | ||
| append(buffer.readUtf8()) | ||
| } else { | ||
| append("\n(binary: ${partBody.contentLength()} bytes)") | ||
| } | ||
| append("\n\n") |
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.
Uh oh!
There was an error while loading. Please reload this page.