From 789e841d60ddbd07df7c020496a0cd889a6d3038 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Wed, 17 Jun 2026 04:47:18 +0300 Subject: [PATCH 1/3] fix: reject a request body on a body-forbidden method at build time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `Request` could be built with a body on a method that forbids one (GET, HEAD, TRACE). The two transports then handled it inconsistently and neither was correct: - OkHttp's `Request.Builder.method("GET", body)` throws `IllegalArgumentException("method GET must not have a request body.")`. That unchecked exception escaped the transport's `@Throws(IOException)` contract (and the retry pipeline). - The JDK transport built a `BodyPublisher` from the body and then called `builder.GET()`, which ignores it. The body was silently dropped — and for a small body the eager publisher path had already drained a consume-once `writeTo` into a byte array that was then discarded. Pick one consistent policy and reject at construction. `Method` now carries a `permitsRequestBody` flag (`false` for GET/HEAD/TRACE/CONNECT), and `Request.RequestBuilder.build()` throws `IllegalArgumentException` when a body is set on a method that forbids one. Failing fast at the model boundary keeps both transports from diverging: OkHttp can no longer receive a body it would crash on, and the JDK transport no longer consumes a body it never sends. The JDK adapter now sends `noBody()` unconditionally for GET/HEAD/TRACE instead of adapting (and discarding) the publisher. Adds symmetric coverage in both transport suites plus core tests for the new validation and the `permitsRequestBody` flag. The new `Method.permitsRequestBody` getter is the only public-API addition (api snapshot regenerated). Closes #117 --- sdk-core/api/sdk-core.api | 1 + .../dexpace/sdk/core/http/request/Method.kt | 25 +++++--- .../dexpace/sdk/core/http/request/Request.kt | 24 +++++-- .../sdk/core/http/request/MethodTest.kt | 14 +++++ .../sdk/core/http/request/RequestTest.kt | 63 +++++++++++++++++++ .../jdkhttp/internal/RequestAdapter.kt | 21 ++++--- .../transport/jdkhttp/JdkHttpTransportTest.kt | 37 +++++++++++ .../okhttp/internal/RequestAdapter.kt | 5 +- .../transport/okhttp/OkHttpTransportTest.kt | 58 ++++++++++++++--- 9 files changed, 214 insertions(+), 34 deletions(-) diff --git a/sdk-core/api/sdk-core.api b/sdk-core/api/sdk-core.api index 98288a8c..ab043876 100644 --- a/sdk-core/api/sdk-core.api +++ b/sdk-core/api/sdk-core.api @@ -1033,6 +1033,7 @@ public final class org/dexpace/sdk/core/http/request/Method : java/lang/Enum { public static final field TRACE Lorg/dexpace/sdk/core/http/request/Method; public static fun getEntries ()Lkotlin/enums/EnumEntries; public final fun getMethod ()Ljava/lang/String; + public final fun getPermitsRequestBody ()Z public fun toString ()Ljava/lang/String; public static fun valueOf (Ljava/lang/String;)Lorg/dexpace/sdk/core/http/request/Method; public static fun values ()[Lorg/dexpace/sdk/core/http/request/Method; diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Method.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Method.kt index f73f507e..e9f35c93 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Method.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Method.kt @@ -15,20 +15,27 @@ package org.dexpace.sdk.core.http.request * request line without translation. * * @property method Canonical uppercase method token sent in the request line. + * @property permitsRequestBody Whether HTTP allows this method to carry a request body. `false` + * for `GET`, `HEAD`, and `TRACE`: GET/HEAD have no defined payload semantics (RFC 9110 + * §9.3.1/§9.3.2) and a TRACE request "MUST NOT send content" (§9.3.8). Transports disagree on + * how to handle a body on these methods — OkHttp throws `IllegalArgumentException`, the JDK + * builder silently ignores it — so a body on a body-forbidden method is rejected at request + * construction (see `Request.RequestBuilder.build`) rather than left to diverge per transport. */ @Suppress("unused") public enum class Method( public val method: String, + public val permitsRequestBody: Boolean, ) { - GET("GET"), - POST("POST"), - PUT("PUT"), - DELETE("DELETE"), - PATCH("PATCH"), - HEAD("HEAD"), - OPTIONS("OPTIONS"), - TRACE("TRACE"), - CONNECT("CONNECT"), + GET("GET", permitsRequestBody = false), + POST("POST", permitsRequestBody = true), + PUT("PUT", permitsRequestBody = true), + DELETE("DELETE", permitsRequestBody = true), + PATCH("PATCH", permitsRequestBody = true), + HEAD("HEAD", permitsRequestBody = false), + OPTIONS("OPTIONS", permitsRequestBody = true), + TRACE("TRACE", permitsRequestBody = false), + CONNECT("CONNECT", permitsRequestBody = false), ; override fun toString(): String = method diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt index 0bc6011e..cea031ed 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt @@ -37,7 +37,9 @@ import java.net.URL * @property method HTTP method on the wire. * @property url Fully-resolved target URL. * @property headers Request headers; may be empty but never `null`. - * @property body Request body, or `null` for methods without a payload (typical for GET/HEAD). + * @property body Request body, or `null` for methods without a payload. A non-`null` body is + * only valid on a method that permits one ([Method.permitsRequestBody]); building a `GET`, + * `HEAD`, or `TRACE` request with a body throws `IllegalArgumentException`. */ @ConsistentCopyVisibility public data class Request private constructor( @@ -247,16 +249,30 @@ public data class Request private constructor( /** * Builds the [Request]. * + * A body set on a method that forbids one ([Method.permitsRequestBody] is `false` — + * `GET`, `HEAD`, `TRACE`) is rejected here rather than passed to a transport: the two + * reference transports disagree on the case (OkHttp throws, the JDK builder drops the + * body and may consume a single-use stream for nothing), so the SDK fails fast at + * construction with one consistent error instead. + * * @return The built request. * @throws IllegalStateException If a required field is missing. + * @throws IllegalArgumentException If a body is set on a method that forbids one + * ([Method.GET], [Method.HEAD], or [Method.TRACE]). */ - override fun build(): Request = - Request( - method = checkRequired("method", method), + override fun build(): Request { + val resolvedMethod = checkRequired("method", method) + require(body == null || resolvedMethod.permitsRequestBody) { + "$resolvedMethod must not carry a request body; remove the body or use a " + + "method that permits one (e.g. POST/PUT/PATCH)." + } + return Request( + method = resolvedMethod, url = checkRequired("url", url), headers = headersBuilder.build(), body = body, ) + } } public companion object { diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/MethodTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/MethodTest.kt index c17e1dd6..f9a90833 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/MethodTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/MethodTest.kt @@ -49,4 +49,18 @@ class MethodTest { fun `entries enumerates exactly nine methods`() { assertEquals(9, Method.entries.size) } + + @Test + fun `permitsRequestBody is false only for GET HEAD TRACE and CONNECT`() { + assertEquals(false, Method.GET.permitsRequestBody) + assertEquals(false, Method.HEAD.permitsRequestBody) + assertEquals(false, Method.TRACE.permitsRequestBody) + assertEquals(false, Method.CONNECT.permitsRequestBody) + + assertEquals(true, Method.POST.permitsRequestBody) + assertEquals(true, Method.PUT.permitsRequestBody) + assertEquals(true, Method.PATCH.permitsRequestBody) + assertEquals(true, Method.DELETE.permitsRequestBody) + assertEquals(true, Method.OPTIONS.permitsRequestBody) + } } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestTest.kt index 735e6209..bd14f2ba 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestTest.kt @@ -219,6 +219,69 @@ class RequestTest { assertEquals("url is required", ex.message) } + // --------------------------------------------------------------------- + // Body / method compatibility — a body on a body-forbidden method is + // rejected at build time so transports never have to disagree on it. + // --------------------------------------------------------------------- + + @Test + fun `build rejects a body on a body-forbidden method`() { + for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) { + val ex = + assertFailsWith("expected rejection for $method") { + Request.builder() + .url("https://example.test") + .method(method) + .body(RequestBody.create("x", null)) + .build() + } + assertTrue( + ex.message!!.contains("$method must not carry a request body"), + "unexpected message for $method: ${ex.message}", + ) + } + } + + @Test + fun `build allows a body on a body-permitting method`() { + for (method in listOf(Method.POST, Method.PUT, Method.PATCH, Method.DELETE, Method.OPTIONS)) { + val payload = RequestBody.create("x", null) + val req = + Request.builder() + .url("https://example.test") + .method(method) + .body(payload) + .build() + assertEquals(payload, req.body, "body should be retained for $method") + } + } + + @Test + fun `build allows a body-less body-forbidden method`() { + for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) { + val req = + Request.builder() + .url("https://example.test") + .method(method) + .build() + assertNull(req.body, "body should stay null for $method") + } + } + + @Test + fun `newBuilder switching to a body-forbidden method while a body is set is rejected`() { + val post = + Request.builder() + .url("https://example.test") + .method(Method.POST) + .body(RequestBody.create("x", null)) + .build() + + assertFailsWith { + post.newBuilder().method(Method.GET).build() + } + } + // --------------------------------------------------------------------- // Equality — compares url by external form, never resolving DNS. // --------------------------------------------------------------------- diff --git a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt index 032886d2..77cfee61 100644 --- a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt +++ b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt @@ -61,10 +61,12 @@ internal class RequestAdapter( * that take a [HttpRequest.BodyPublisher] and those that force [HttpRequest.BodyPublishers.noBody] * reflects the HTTP semantics enforced by the JDK builder: * - * - `GET` / `HEAD` / `TRACE` — no body. The JDK throws if a non-noBody publisher is supplied - * on `HEAD`, so the adapter sends `noBody()` for these methods regardless of what the SDK - * request carried. Callers passing a body to a GET/HEAD/TRACE request will see the body - * silently dropped here. + * - `GET` / `HEAD` / `TRACE` — no body. [Method.permitsRequestBody] is `false` for these, so + * `Request.RequestBuilder.build` rejects a body on them at construction; an SDK request can + * never carry one here. The adapter sends `noBody()` unconditionally rather than adapting + * `request.body` — there is nothing to adapt, and `noBody()` consumes nothing (the previous + * code adapted then discarded the publisher, which for a small body drained a consume-once + * `writeTo` for nothing). * - `POST` / `PUT` / `PATCH` / `DELETE` / `OPTIONS` — body publisher passed through. `DELETE` * and `OPTIONS` with a body are unusual but permitted by HTTP and the JDK builder. * - `CONNECT` — rejected; see [adapt]'s KDoc. @@ -73,16 +75,15 @@ internal class RequestAdapter( builder: HttpRequest.Builder, request: SdkRequest, ) { - val publisher = BodyPublishers.adaptBody(request.body) when (request.method) { Method.GET -> builder.GET() Method.HEAD -> builder.method("HEAD", HttpRequest.BodyPublishers.noBody()) Method.TRACE -> builder.method("TRACE", HttpRequest.BodyPublishers.noBody()) - Method.POST -> builder.POST(publisher) - Method.PUT -> builder.PUT(publisher) - Method.DELETE -> builder.method("DELETE", publisher) - Method.PATCH -> builder.method("PATCH", publisher) - Method.OPTIONS -> builder.method("OPTIONS", publisher) + Method.POST -> builder.POST(BodyPublishers.adaptBody(request.body)) + Method.PUT -> builder.PUT(BodyPublishers.adaptBody(request.body)) + Method.DELETE -> builder.method("DELETE", BodyPublishers.adaptBody(request.body)) + Method.PATCH -> builder.method("PATCH", BodyPublishers.adaptBody(request.body)) + Method.OPTIONS -> builder.method("OPTIONS", BodyPublishers.adaptBody(request.body)) Method.CONNECT -> throw IllegalArgumentException( "java.net.http.HttpClient does not support user-issued CONNECT requests. " + "Configure a proxy on the underlying HttpClient instead.", diff --git a/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt b/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt index b78be3d0..5818652b 100644 --- a/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt +++ b/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt @@ -123,6 +123,43 @@ class JdkHttpTransportTest { assertEquals("/echo", recorded.url.encodedPath) } + // -------- body on a body-forbidden method (GET/HEAD/TRACE) -------- + + @Test + fun `bodyOnForbiddenMethodIsRejectedBeforeDispatch`() { + // The JDK builder ignores a body on GET/HEAD/TRACE, silently dropping it — and the eager + // BodyPublishers path would already have drained a small consume-once body into a byte + // array that is then discarded. The SDK rejects the body at request construction + // (Request.RequestBuilder.build), so such a request is never built and never reaches the + // transport, and no body is consumed for nothing. + for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) { + assertFailsWith("expected rejection for $method") { + Request.builder() + .method(method) + .url(server.url("/forbidden-body").toUrl()) + .body(RequestBody.create("x", CommonMediaTypes.TEXT_PLAIN)) + .build() + } + } + } + + @Test + fun `bodylessGetDispatchesWithNoBody`() { + server.enqueue(MockResponse.Builder().code(200).build()) + val request = + Request.builder() + .method(Method.GET) + .url(server.url("/bodyless-get").toUrl()) + .build() + transport.execute(request).use { response -> + assertEquals(200, response.status.code) + } + val recorded = server.takeRequest() + assertEquals("GET", recorded.method) + assertEquals("", recorded.body?.utf8() ?: "") + assertEquals("/bodyless-get", recorded.url.encodedPath) + } + // -------- async golden paths -------- @Test diff --git a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt index f83d7059..74ce9584 100644 --- a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt +++ b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt @@ -32,7 +32,10 @@ import org.dexpace.sdk.core.http.request.RequestBody as SdkRequestBody * makes a body optional for every method, so a body-less POST is a valid request; to keep * it from crashing with `IllegalArgumentException`, the adapter substitutes a zero-length * body for those methods (see [requiresRequestBody]). GET/HEAD/OPTIONS/TRACE/DELETE keep - * their `null` body. + * their `null` body. The inverse case — a body on a body-forbidden method, which OkHttp's + * `Request.Builder.method` rejects with `IllegalArgumentException("method GET must not have a + * request body.")` — cannot reach here: `Request.RequestBuilder.build` rejects a body on + * GET/HEAD/TRACE at construction, so `request.body` is always `null` for those methods. * * The adapter is stateless — the [logger] is the only field it carries so the DEBUG log * naming dropped headers attributes to the transport. diff --git a/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt b/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt index 77b88bd3..0cf399ec 100644 --- a/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt +++ b/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt @@ -159,6 +159,43 @@ class OkHttpTransportTest { assertEquals("0", recorded.headers["Content-Length"], "empty body must report zero length") } + // -------- body on a body-forbidden method (GET/HEAD/TRACE) -------- + + @Test + fun bodyOnForbiddenMethodIsRejectedBeforeDispatch() { + // OkHttp's Request.Builder.method throws IllegalArgumentException("method GET must not + // have a request body.") for a GET carrying a body. That unchecked exception would escape + // execute's @Throws(IOException) contract. The SDK rejects the body at request + // construction (Request.RequestBuilder.build), so such a request is never built and never + // reaches the transport. + for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) { + assertFailsWith("expected rejection for $method") { + Request.builder() + .method(method) + .url(server.url("/forbidden-body").toUrl()) + .body(RequestBody.create("x", null)) + .build() + } + } + } + + @Test + fun bodylessGetDispatchesWithNoBody() { + server.enqueue(MockResponse.Builder().code(200).build()) + val request = + Request.builder() + .method(Method.GET) + .url(server.url("/bodyless-get").toUrl()) + .build() + transport.execute(request).use { response -> + assertEquals(200, response.status.code) + } + val recorded = server.takeRequest() + assertEquals("GET", recorded.method) + assertEquals("", recorded.body?.utf8() ?: "") + assertEquals("/bodyless-get", recorded.url.encodedPath) + } + // -------- async golden paths -------- @Test @@ -195,15 +232,16 @@ class OkHttpTransportTest { @Test fun executeAsyncDeliversAdaptationFailureThroughFuture() { - // A body on a method OkHttp forbids one for (GET) makes request adaptation throw - // synchronously inside executeAsync. The contract is that executeAsync completes - // exceptionally on error, so the failure must arrive through the returned future — a - // future-composing caller's .exceptionally/.handle would never observe a synchronous throw. + // A non-http(s) URL scheme makes request adaptation throw synchronously inside + // executeAsync: java.net.URL accepts an ftp:// URL, but OkHttp's Request.Builder.url + // rejects any scheme other than http/https with IllegalArgumentException. The contract is + // that executeAsync completes exceptionally on error, so the failure must arrive through + // the returned future — a future-composing caller's .exceptionally/.handle would never + // observe a synchronous throw. val request = Request.builder() .method(Method.GET) - .url(server.url("/async-adapt-fail").toUrl()) - .body(RequestBody.create("x".toByteArray())) + .url(URL("ftp://example.test/async-adapt-fail")) .build() // Must return a future rather than throwing on the caller's thread. val future = transport.executeAsync(request) @@ -219,11 +257,11 @@ class OkHttpTransportTest { "adaptation failure must surface as the future's cause, was: ${ex.cause?.let { it::class }}", ) // Assert the message so an unrelated IllegalArgumentException cannot satisfy the test. - // OkHttp's Request.Builder.method rejects a body on GET with " must not have a - // request body." + // OkHttp's Request.Builder.url rejects a non-http(s) scheme with "Expected URL scheme + // 'http' or 'https' but was ''". assertTrue( - ex.cause?.message?.contains("must not have a request body") == true, - "expected OkHttp's body-on-GET rejection message, was: ${ex.cause?.message}", + ex.cause?.message?.contains("scheme") == true, + "expected OkHttp's URL-scheme rejection message, was: ${ex.cause?.message}", ) } From 8712cb33f2313bcc9cdc34c81a564f368ab9e8e0 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 22 Jun 2026 04:16:27 +0300 Subject: [PATCH 2/3] refactor: let RequestBuilder.body(null) clear a body and document CONNECT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RequestBuilder.body now accepts null, so a body-carrying request can be downgraded to a body-forbidden method (GET/HEAD/TRACE/CONNECT) via newBuilder by clearing the body first — previously there was no way to drop it. build() also runs the required-field checks before the body/method compatibility check, so a missing method or url is reported before a body conflict. Document that CONNECT, like GET/HEAD/TRACE, is rejected a request body at build time — across Method, Request, and both transport adapters — and correct the RFC 9110 wording: only TRACE carries a hard "MUST NOT send content" (§9.3.8), while GET/HEAD/CONNECT payloads merely have no generally defined semantics. Adds tests for the body(null) clear, the newBuilder downgrade recovery path, and CONNECT coverage in the core and transport suites. --- .../dexpace/sdk/core/http/request/Method.kt | 15 +++++--- .../dexpace/sdk/core/http/request/Request.kt | 26 ++++++++----- .../sdk/core/http/request/RequestTest.kt | 37 ++++++++++++++++++- .../jdkhttp/internal/RequestAdapter.kt | 3 +- .../transport/jdkhttp/JdkHttpTransportTest.kt | 4 +- .../okhttp/internal/RequestAdapter.kt | 6 +-- .../transport/okhttp/OkHttpTransportTest.kt | 4 +- 7 files changed, 69 insertions(+), 26 deletions(-) diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Method.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Method.kt index e9f35c93..107bff4f 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Method.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Method.kt @@ -15,12 +15,15 @@ package org.dexpace.sdk.core.http.request * request line without translation. * * @property method Canonical uppercase method token sent in the request line. - * @property permitsRequestBody Whether HTTP allows this method to carry a request body. `false` - * for `GET`, `HEAD`, and `TRACE`: GET/HEAD have no defined payload semantics (RFC 9110 - * §9.3.1/§9.3.2) and a TRACE request "MUST NOT send content" (§9.3.8). Transports disagree on - * how to handle a body on these methods — OkHttp throws `IllegalArgumentException`, the JDK - * builder silently ignores it — so a body on a body-forbidden method is rejected at request - * construction (see `Request.RequestBuilder.build`) rather than left to diverge per transport. + * @property permitsRequestBody Whether this SDK permits the method to carry a request body. + * `false` for `GET`, `HEAD`, `TRACE`, and `CONNECT`. Of these only `TRACE` is forbidden a body + * outright by HTTP — a TRACE request "MUST NOT send content" (RFC 9110 §9.3.8); for `GET`/`HEAD` + * (§9.3.1/§9.3.2) and `CONNECT` (§9.3.6) a request payload has no generally defined semantics + * and is discouraged rather than illegal. The SDK rejects a body on all four as one consistent + * policy, because the reference transports otherwise diverge on the case — OkHttp throws + * `IllegalArgumentException`, the JDK builder silently ignores the body — so it is rejected once + * at request construction (see `Request.RequestBuilder.build`) rather than left to behave + * differently per transport. */ @Suppress("unused") public enum class Method( diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt index cea031ed..a29cb547 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt @@ -39,7 +39,7 @@ import java.net.URL * @property headers Request headers; may be empty but never `null`. * @property body Request body, or `null` for methods without a payload. A non-`null` body is * only valid on a method that permits one ([Method.permitsRequestBody]); building a `GET`, - * `HEAD`, or `TRACE` request with a body throws `IllegalArgumentException`. + * `HEAD`, `TRACE`, or `CONNECT` request with a body throws `IllegalArgumentException`. */ @ConsistentCopyVisibility public data class Request private constructor( @@ -119,12 +119,16 @@ public data class Request private constructor( } /** - * Sets the request body. + * Sets the request body, or clears it when [body] is `null`. * - * @param body The request body. + * Passing `null` is the way to drop a body carried over by [newBuilder] — for example + * when downgrading a body-carrying request to `GET`, `HEAD`, `TRACE`, or `CONNECT`, whose + * [build] rejects a non-`null` body ([Method.permitsRequestBody] is `false` for them). + * + * @param body The request body, or `null` to clear any previously-set body. * @return This builder. */ - public fun body(body: RequestBody): RequestBuilder = + public fun body(body: RequestBody?): RequestBuilder = apply { this.body = body } @@ -250,25 +254,27 @@ public data class Request private constructor( * Builds the [Request]. * * A body set on a method that forbids one ([Method.permitsRequestBody] is `false` — - * `GET`, `HEAD`, `TRACE`) is rejected here rather than passed to a transport: the two - * reference transports disagree on the case (OkHttp throws, the JDK builder drops the - * body and may consume a single-use stream for nothing), so the SDK fails fast at - * construction with one consistent error instead. + * `GET`, `HEAD`, `TRACE`, `CONNECT`) is rejected here rather than passed to a transport: + * the two reference transports disagree on the case (OkHttp throws, the JDK builder drops + * the body and may consume a single-use stream for nothing), so the SDK fails fast at + * construction with one consistent error instead. To downgrade a body-carrying request to + * one of these methods, clear the body first with `body(null)`. * * @return The built request. * @throws IllegalStateException If a required field is missing. * @throws IllegalArgumentException If a body is set on a method that forbids one - * ([Method.GET], [Method.HEAD], or [Method.TRACE]). + * ([Method.GET], [Method.HEAD], [Method.TRACE], or [Method.CONNECT]). */ override fun build(): Request { val resolvedMethod = checkRequired("method", method) + val resolvedUrl = checkRequired("url", url) require(body == null || resolvedMethod.permitsRequestBody) { "$resolvedMethod must not carry a request body; remove the body or use a " + "method that permits one (e.g. POST/PUT/PATCH)." } return Request( method = resolvedMethod, - url = checkRequired("url", url), + url = resolvedUrl, headers = headersBuilder.build(), body = body, ) diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestTest.kt index bd14f2ba..aa4d12bd 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestTest.kt @@ -226,7 +226,7 @@ class RequestTest { @Test fun `build rejects a body on a body-forbidden method`() { - for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) { + for (method in listOf(Method.GET, Method.HEAD, Method.TRACE, Method.CONNECT)) { val ex = assertFailsWith("expected rejection for $method") { Request.builder() @@ -258,7 +258,7 @@ class RequestTest { @Test fun `build allows a body-less body-forbidden method`() { - for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) { + for (method in listOf(Method.GET, Method.HEAD, Method.TRACE, Method.CONNECT)) { val req = Request.builder() .url("https://example.test") @@ -282,6 +282,39 @@ class RequestTest { } } + @Test + fun `body null clears a previously-set body`() { + val req = + Request.builder() + .url("https://example.test") + .method(Method.POST) + .body(RequestBody.create("x", null)) + .body(null) + .build() + assertNull(req.body, "body(null) should clear the previously-set body") + } + + @Test + fun `newBuilder downgrade to a body-forbidden method succeeds after clearing the body`() { + val post = + Request.builder() + .url("https://example.test") + .method(Method.POST) + .body(RequestBody.create("x", null)) + .build() + + // Clearing the body with body(null) is the supported way to downgrade a body-carrying + // request to a method that forbids one. + val get = + post.newBuilder() + .method(Method.GET) + .body(null) + .build() + + assertEquals(Method.GET, get.method) + assertNull(get.body, "downgraded GET must not retain the original body") + } + // --------------------------------------------------------------------- // Equality — compares url by external form, never resolving DNS. // --------------------------------------------------------------------- diff --git a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt index 77cfee61..8e3b1382 100644 --- a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt +++ b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt @@ -61,7 +61,8 @@ internal class RequestAdapter( * that take a [HttpRequest.BodyPublisher] and those that force [HttpRequest.BodyPublishers.noBody] * reflects the HTTP semantics enforced by the JDK builder: * - * - `GET` / `HEAD` / `TRACE` — no body. [Method.permitsRequestBody] is `false` for these, so + * - `GET` / `HEAD` / `TRACE` — no body. [Method.permitsRequestBody] is `false` for these (and + * for `CONNECT`, which is rejected outright in its own branch below), so * `Request.RequestBuilder.build` rejects a body on them at construction; an SDK request can * never carry one here. The adapter sends `noBody()` unconditionally rather than adapting * `request.body` — there is nothing to adapt, and `noBody()` consumes nothing (the previous diff --git a/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt b/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt index 5818652b..9e9278d8 100644 --- a/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt +++ b/sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt @@ -123,7 +123,7 @@ class JdkHttpTransportTest { assertEquals("/echo", recorded.url.encodedPath) } - // -------- body on a body-forbidden method (GET/HEAD/TRACE) -------- + // -------- body on a body-forbidden method (GET/HEAD/TRACE/CONNECT) -------- @Test fun `bodyOnForbiddenMethodIsRejectedBeforeDispatch`() { @@ -132,7 +132,7 @@ class JdkHttpTransportTest { // array that is then discarded. The SDK rejects the body at request construction // (Request.RequestBuilder.build), so such a request is never built and never reaches the // transport, and no body is consumed for nothing. - for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) { + for (method in listOf(Method.GET, Method.HEAD, Method.TRACE, Method.CONNECT)) { assertFailsWith("expected rejection for $method") { Request.builder() .method(method) diff --git a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt index 74ce9584..032e2cf0 100644 --- a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt +++ b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt @@ -31,11 +31,11 @@ import org.dexpace.sdk.core.http.request.RequestBody as SdkRequestBody * body for POST/PUT/PATCH (the methods OkHttp treats as requiring one). The SDK model * makes a body optional for every method, so a body-less POST is a valid request; to keep * it from crashing with `IllegalArgumentException`, the adapter substitutes a zero-length - * body for those methods (see [requiresRequestBody]). GET/HEAD/OPTIONS/TRACE/DELETE keep - * their `null` body. The inverse case — a body on a body-forbidden method, which OkHttp's + * body for those methods (see [requiresRequestBody]). GET/HEAD/OPTIONS/TRACE/DELETE/CONNECT + * keep their `null` body. The inverse case — a body on a body-forbidden method, which OkHttp's * `Request.Builder.method` rejects with `IllegalArgumentException("method GET must not have a * request body.")` — cannot reach here: `Request.RequestBuilder.build` rejects a body on - * GET/HEAD/TRACE at construction, so `request.body` is always `null` for those methods. + * GET/HEAD/TRACE/CONNECT at construction, so `request.body` is always `null` for those methods. * * The adapter is stateless — the [logger] is the only field it carries so the DEBUG log * naming dropped headers attributes to the transport. diff --git a/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt b/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt index 0cf399ec..2d8d0ad4 100644 --- a/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt +++ b/sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt @@ -159,7 +159,7 @@ class OkHttpTransportTest { assertEquals("0", recorded.headers["Content-Length"], "empty body must report zero length") } - // -------- body on a body-forbidden method (GET/HEAD/TRACE) -------- + // -------- body on a body-forbidden method (GET/HEAD/TRACE/CONNECT) -------- @Test fun bodyOnForbiddenMethodIsRejectedBeforeDispatch() { @@ -168,7 +168,7 @@ class OkHttpTransportTest { // execute's @Throws(IOException) contract. The SDK rejects the body at request // construction (Request.RequestBuilder.build), so such a request is never built and never // reaches the transport. - for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) { + for (method in listOf(Method.GET, Method.HEAD, Method.TRACE, Method.CONNECT)) { assertFailsWith("expected rejection for $method") { Request.builder() .method(method) From 438de6aecf37598b62ce0d7b9add1a5782cd4ed2 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Mon, 22 Jun 2026 05:26:03 +0300 Subject: [PATCH 3/3] chore: name all body-permitting methods in the body-rejection error The build() rejection message listed only POST/PUT/PATCH as the body-permitting alternatives, which read as if DELETE and OPTIONS were also forbidden a body. List the full permitted set so the message matches Method.permitsRequestBody. --- .../main/kotlin/org/dexpace/sdk/core/http/request/Request.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt index a29cb547..7bf3834e 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt @@ -270,7 +270,7 @@ public data class Request private constructor( val resolvedUrl = checkRequired("url", url) require(body == null || resolvedMethod.permitsRequestBody) { "$resolvedMethod must not carry a request body; remove the body or use a " + - "method that permits one (e.g. POST/PUT/PATCH)." + "method that permits one (POST, PUT, PATCH, DELETE, or OPTIONS)." } return Request( method = resolvedMethod,