diff --git a/sdk-core/api/sdk-core.api b/sdk-core/api/sdk-core.api index 7ebcc312..49d0e595 100644 --- a/sdk-core/api/sdk-core.api +++ b/sdk-core/api/sdk-core.api @@ -1026,6 +1026,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 3e5ec734..ffece00b 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 @@ -114,6 +114,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 88af552b..727fd292 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 c4cc9dbb..01f6faf9 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)