KTOR-8352: Fix TRACE method handling in AndroidClientEngine #4899
Conversation
|
""" WalkthroughThis change updates HTTP method handling in the Ktor client and server. It replaces a hardcoded list of methods without request bodies with a property-based check, adds "TRACE" to the set of methods that do not allow bodies, introduces a new echo test endpoint, and adds corresponding client tests to verify support for all HTTP methods. Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HttpMethodTest.kt (2)
20-28: Consider adding error handling for network failures.The test doesn't handle potential network exceptions that could occur during HTTP requests. Consider wrapping the request in try-catch or using test framework utilities that handle failures gracefully.
Consider adding error handling:
test { client -> for (method in httpMethods) { if (!client.supportsMethod(method)) continue - - val response = client.request("$TEST_SERVER/echo/method") { this.method = method } - assertEquals(HttpStatusCode.OK, response.status) - assertEquals(method.value, response.headers["Http-Method"]) + + try { + val response = client.request("$TEST_SERVER/echo/method") { this.method = method } + assertEquals(HttpStatusCode.OK, response.status) + assertEquals(method.value, response.headers["Http-Method"]) + } catch (e: Exception) { + throw AssertionError("Failed to send $method request: ${e.message}", e) + } } }
32-38: Good design with proper documentation.The helper function properly handles engine-specific limitations with clear documentation. The JDK bug reference provides good context for the Android engine PATCH method exclusion.
However, consider these improvements:
- String-based engine identification could be fragile if class names change
- Verify the JDK bug is still relevant for current Android versions
Consider using type checking instead of string comparison:
-private fun HttpClient.supportsMethod(method: HttpMethod): Boolean { - return when(engine::class.simpleName) { - // https://bugs.openjdk.org/browse/JDK-7016595 - "AndroidClientEngine" -> method != HttpMethod.Patch - else -> true - } -} +private fun HttpClient.supportsMethod(method: HttpMethod): Boolean { + return when { + // https://bugs.openjdk.org/browse/JDK-7016595 + engine::class.qualifiedName?.contains("AndroidClientEngine") == true -> method != HttpMethod.Patch + else -> true + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ktor-client/ktor-client-android/jvm/src/io/ktor/client/engine/android/AndroidClientEngine.kt(1 hunks)ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HttpMethodTest.kt(1 hunks)ktor-http/common/src/io/ktor/http/HttpMethod.kt(1 hunks)ktor-test-server/src/main/kotlin/test/server/ClientTestServer.kt(1 hunks)ktor-test-server/src/main/kotlin/test/server/tests/Echo.kt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ktor-test-server/src/main/kotlin/test/server/ClientTestServer.kt (1)
ktor-test-server/src/main/kotlin/test/server/tests/Echo.kt (1)
echoTest(12-25)
🔇 Additional comments (7)
ktor-test-server/src/main/kotlin/test/server/ClientTestServer.kt (1)
34-34: LGTM: Clean integration of echo test endpoint.The addition of
echoTest()properly integrates the new echo endpoint for HTTP method testing. The positioning between existing test servers is appropriate and follows the established pattern.ktor-http/common/src/io/ktor/http/HttpMethod.kt (1)
62-62: LGTM: Correct addition of TRACE method to methods without body.Adding
HttpMethod("TRACE")toREQUESTS_WITHOUT_BODYis correct according to HTTP specification (RFC 7231). The TRACE method is used for diagnostic purposes and should not have a request body, which aligns with the PR objective to fix TRACE method handling.ktor-test-server/src/main/kotlin/test/server/tests/Echo.kt (1)
12-25: LGTM: Well-designed echo endpoint for HTTP method testing.The implementation is clean and appropriate for its purpose:
- Uses response header instead of body to accommodate methods that don't support bodies
- Clear route structure at
/echo/method- Proper extraction of HTTP method using
call.request.local.method.value- Returns 200 OK status which is appropriate for all HTTP methods
This endpoint will effectively support testing of all HTTP methods, including the newly supported TRACE method.
ktor-client/ktor-client-android/jvm/src/io/ktor/client/engine/android/AndroidClientEngine.kt (1)
65-65: LGTM: Core fix for TRACE method handling in Android client engine.This change from checking a hardcoded list to using
!data.method.supportsRequestBodyis the main fix for KTOR-8352. Benefits include:
- Consistency: Now uses the same property-based approach as the rest of the codebase
- Maintainability: No need to maintain separate lists in different places
- Correctness: Automatically handles TRACE and any future methods added to
REQUESTS_WITHOUT_BODYThe existing error handling logic is preserved, making this a safe and focused fix.
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HttpMethodTest.kt (3)
1-13: LGTM!The package declaration and imports are well-organized and include all necessary dependencies for HTTP client testing.
24-26:✅ Verification successful
Verify test server endpoint and header name consistency.
The test relies on the
/echo/methodendpoint returning the HTTP method in an "Http-Method" header. Ensure this endpoint exists and the header name matches the server implementation.
🏁 Script executed:
#!/bin/bash # Description: Verify the echo endpoint implementation and header name consistency # Expected: Find the echo endpoint implementation with correct header name # Search for echo endpoint implementation rg -A 10 -B 5 "echo.*method" --type kotlin # Search for Http-Method header usage rg "Http-Method" --type kotlinLength of output: 3126
Echo endpoint and header name are consistent
The
/echo/methodroute inktor-test-server/src/main/kotlin/test/server/tests/Echo.ktindeed callscall.response.header("Http-Method", method)which matches the test’s expectation (
response.headers["Http-Method"]). No changes required.
18-18:✅ Verification successful
Verify TRACE method handling consistency.
The TRACE method is manually added to the default methods list. While this aligns with the PR objective to fix TRACE handling, consider whether TRACE should be included in
HttpMethod.DefaultMethodsfor consistency.
🏁 Script executed:
#!/bin/bash # Description: Check if TRACE method is properly defined in HttpMethod and whether it should be part of DefaultMethods # Expected: Find TRACE method definition and verify its inclusion in DefaultMethods # Search for TRACE method definition rg -A 3 -B 3 "TRACE.*=" --type kotlin # Search for DefaultMethods definition rg -A 10 "DefaultMethods.*=" --type kotlinLength of output: 2619
No change needed: TRACE is intentionally excluded from DefaultMethods
The
HttpMethod.DefaultMethodslist inktor-http/common/src/io/ktor/http/HttpMethod.ktis defined as:public val DefaultMethods: List<HttpMethod> = listOf(Get, Post, Put, Patch, Delete, Head, Options)TRACE is handled separately (e.g. in
REQUESTS_WITHOUT_BODY), so manually appendingHttpMethod("TRACE")in the test ensures it’s covered without altering the core defaults.If the goal is to make TRACE a true “default” method everywhere, update the
DefaultMethodsdefinition above. Otherwise, the test’s approach is correct.
osipxd
force-pushed
the
osipxd/android-trace-method
branch
2 times, most recently
from
303880f to
0522790
Compare
osipxd
force-pushed
the
osipxd/android-trace-method
branch
from
0522790 to
17d86fd
Compare
Subsystem
Client, Android
Motivation
KTOR-8352 Android: "ProtocolException: TRACE does not support writing" when sending TRACE request
Solution
Added test checking that all clients are able to send requests with any default HTTP header.
Fixed the Android client by using recently added
!method.supportsRequestBodyinstead of checkingMETHODS_WITHOUT_BODYlist.