Skip to content

Commit 7a5ac3d

Browse files
committed
refactor: let RequestBuilder.body(null) clear a body and document CONNECT
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.
1 parent d691cd6 commit 7a5ac3d

7 files changed

Lines changed: 69 additions & 26 deletions

File tree

sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Method.kt

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,15 @@ package org.dexpace.sdk.core.http.request
1515
* request line without translation.
1616
*
1717
* @property method Canonical uppercase method token sent in the request line.
18-
* @property permitsRequestBody Whether HTTP allows this method to carry a request body. `false`
19-
* for `GET`, `HEAD`, and `TRACE`: GET/HEAD have no defined payload semantics (RFC 9110
20-
* §9.3.1/§9.3.2) and a TRACE request "MUST NOT send content" (§9.3.8). Transports disagree on
21-
* how to handle a body on these methods — OkHttp throws `IllegalArgumentException`, the JDK
22-
* builder silently ignores it — so a body on a body-forbidden method is rejected at request
23-
* construction (see `Request.RequestBuilder.build`) rather than left to diverge per transport.
18+
* @property permitsRequestBody Whether this SDK permits the method to carry a request body.
19+
* `false` for `GET`, `HEAD`, `TRACE`, and `CONNECT`. Of these only `TRACE` is forbidden a body
20+
* outright by HTTP — a TRACE request "MUST NOT send content" (RFC 9110 §9.3.8); for `GET`/`HEAD`
21+
* (§9.3.1/§9.3.2) and `CONNECT` (§9.3.6) a request payload has no generally defined semantics
22+
* and is discouraged rather than illegal. The SDK rejects a body on all four as one consistent
23+
* policy, because the reference transports otherwise diverge on the case — OkHttp throws
24+
* `IllegalArgumentException`, the JDK builder silently ignores the body — so it is rejected once
25+
* at request construction (see `Request.RequestBuilder.build`) rather than left to behave
26+
* differently per transport.
2427
*/
2528
@Suppress("unused")
2629
public enum class Method(

sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Request.kt

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ import java.net.URL
3939
* @property headers Request headers; may be empty but never `null`.
4040
* @property body Request body, or `null` for methods without a payload. A non-`null` body is
4141
* only valid on a method that permits one ([Method.permitsRequestBody]); building a `GET`,
42-
* `HEAD`, or `TRACE` request with a body throws `IllegalArgumentException`.
42+
* `HEAD`, `TRACE`, or `CONNECT` request with a body throws `IllegalArgumentException`.
4343
*/
4444
@ConsistentCopyVisibility
4545
public data class Request private constructor(
@@ -119,12 +119,16 @@ public data class Request private constructor(
119119
}
120120

121121
/**
122-
* Sets the request body.
122+
* Sets the request body, or clears it when [body] is `null`.
123123
*
124-
* @param body The request body.
124+
* Passing `null` is the way to drop a body carried over by [newBuilder] — for example
125+
* when downgrading a body-carrying request to `GET`, `HEAD`, `TRACE`, or `CONNECT`, whose
126+
* [build] rejects a non-`null` body ([Method.permitsRequestBody] is `false` for them).
127+
*
128+
* @param body The request body, or `null` to clear any previously-set body.
125129
* @return This builder.
126130
*/
127-
public fun body(body: RequestBody): RequestBuilder =
131+
public fun body(body: RequestBody?): RequestBuilder =
128132
apply {
129133
this.body = body
130134
}
@@ -250,25 +254,27 @@ public data class Request private constructor(
250254
* Builds the [Request].
251255
*
252256
* A body set on a method that forbids one ([Method.permitsRequestBody] is `false` —
253-
* `GET`, `HEAD`, `TRACE`) is rejected here rather than passed to a transport: the two
254-
* reference transports disagree on the case (OkHttp throws, the JDK builder drops the
255-
* body and may consume a single-use stream for nothing), so the SDK fails fast at
256-
* construction with one consistent error instead.
257+
* `GET`, `HEAD`, `TRACE`, `CONNECT`) is rejected here rather than passed to a transport:
258+
* the two reference transports disagree on the case (OkHttp throws, the JDK builder drops
259+
* the body and may consume a single-use stream for nothing), so the SDK fails fast at
260+
* construction with one consistent error instead. To downgrade a body-carrying request to
261+
* one of these methods, clear the body first with `body(null)`.
257262
*
258263
* @return The built request.
259264
* @throws IllegalStateException If a required field is missing.
260265
* @throws IllegalArgumentException If a body is set on a method that forbids one
261-
* ([Method.GET], [Method.HEAD], or [Method.TRACE]).
266+
* ([Method.GET], [Method.HEAD], [Method.TRACE], or [Method.CONNECT]).
262267
*/
263268
override fun build(): Request {
264269
val resolvedMethod = checkRequired("method", method)
270+
val resolvedUrl = checkRequired("url", url)
265271
require(body == null || resolvedMethod.permitsRequestBody) {
266272
"$resolvedMethod must not carry a request body; remove the body or use a " +
267273
"method that permits one (e.g. POST/PUT/PATCH)."
268274
}
269275
return Request(
270276
method = resolvedMethod,
271-
url = checkRequired("url", url),
277+
url = resolvedUrl,
272278
headers = headersBuilder.build(),
273279
body = body,
274280
)

sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestTest.kt

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ class RequestTest {
226226

227227
@Test
228228
fun `build rejects a body on a body-forbidden method`() {
229-
for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) {
229+
for (method in listOf(Method.GET, Method.HEAD, Method.TRACE, Method.CONNECT)) {
230230
val ex =
231231
assertFailsWith<IllegalArgumentException>("expected rejection for $method") {
232232
Request.builder()
@@ -258,7 +258,7 @@ class RequestTest {
258258

259259
@Test
260260
fun `build allows a body-less body-forbidden method`() {
261-
for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) {
261+
for (method in listOf(Method.GET, Method.HEAD, Method.TRACE, Method.CONNECT)) {
262262
val req =
263263
Request.builder()
264264
.url("https://example.test")
@@ -282,6 +282,39 @@ class RequestTest {
282282
}
283283
}
284284

285+
@Test
286+
fun `body null clears a previously-set body`() {
287+
val req =
288+
Request.builder()
289+
.url("https://example.test")
290+
.method(Method.POST)
291+
.body(RequestBody.create("x", null))
292+
.body(null)
293+
.build()
294+
assertNull(req.body, "body(null) should clear the previously-set body")
295+
}
296+
297+
@Test
298+
fun `newBuilder downgrade to a body-forbidden method succeeds after clearing the body`() {
299+
val post =
300+
Request.builder()
301+
.url("https://example.test")
302+
.method(Method.POST)
303+
.body(RequestBody.create("x", null))
304+
.build()
305+
306+
// Clearing the body with body(null) is the supported way to downgrade a body-carrying
307+
// request to a method that forbids one.
308+
val get =
309+
post.newBuilder()
310+
.method(Method.GET)
311+
.body(null)
312+
.build()
313+
314+
assertEquals(Method.GET, get.method)
315+
assertNull(get.body, "downgraded GET must not retain the original body")
316+
}
317+
285318
// ---------------------------------------------------------------------
286319
// Equality — compares url by external form, never resolving DNS.
287320
// ---------------------------------------------------------------------

sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/RequestAdapter.kt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,8 @@ internal class RequestAdapter(
6161
* that take a [HttpRequest.BodyPublisher] and those that force [HttpRequest.BodyPublishers.noBody]
6262
* reflects the HTTP semantics enforced by the JDK builder:
6363
*
64-
* - `GET` / `HEAD` / `TRACE` — no body. [Method.permitsRequestBody] is `false` for these, so
64+
* - `GET` / `HEAD` / `TRACE` — no body. [Method.permitsRequestBody] is `false` for these (and
65+
* for `CONNECT`, which is rejected outright in its own branch below), so
6566
* `Request.RequestBuilder.build` rejects a body on them at construction; an SDK request can
6667
* never carry one here. The adapter sends `noBody()` unconditionally rather than adapting
6768
* `request.body` — there is nothing to adapt, and `noBody()` consumes nothing (the previous

sdk-transport-jdkhttp/src/test/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransportTest.kt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ class JdkHttpTransportTest {
114114
assertEquals("/echo", recorded.url.encodedPath)
115115
}
116116

117-
// -------- body on a body-forbidden method (GET/HEAD/TRACE) --------
117+
// -------- body on a body-forbidden method (GET/HEAD/TRACE/CONNECT) --------
118118

119119
@Test
120120
fun `bodyOnForbiddenMethodIsRejectedBeforeDispatch`() {
@@ -123,7 +123,7 @@ class JdkHttpTransportTest {
123123
// array that is then discarded. The SDK rejects the body at request construction
124124
// (Request.RequestBuilder.build), so such a request is never built and never reaches the
125125
// transport, and no body is consumed for nothing.
126-
for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) {
126+
for (method in listOf(Method.GET, Method.HEAD, Method.TRACE, Method.CONNECT)) {
127127
assertFailsWith<IllegalArgumentException>("expected rejection for $method") {
128128
Request.builder()
129129
.method(method)

sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/RequestAdapter.kt

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,11 +31,11 @@ import org.dexpace.sdk.core.http.request.RequestBody as SdkRequestBody
3131
* body for POST/PUT/PATCH (the methods OkHttp treats as requiring one). The SDK model
3232
* makes a body optional for every method, so a body-less POST is a valid request; to keep
3333
* it from crashing with `IllegalArgumentException`, the adapter substitutes a zero-length
34-
* body for those methods (see [requiresRequestBody]). GET/HEAD/OPTIONS/TRACE/DELETE keep
35-
* their `null` body. The inverse case — a body on a body-forbidden method, which OkHttp's
34+
* body for those methods (see [requiresRequestBody]). GET/HEAD/OPTIONS/TRACE/DELETE/CONNECT
35+
* keep their `null` body. The inverse case — a body on a body-forbidden method, which OkHttp's
3636
* `Request.Builder.method` rejects with `IllegalArgumentException("method GET must not have a
3737
* request body.")` — cannot reach here: `Request.RequestBuilder.build` rejects a body on
38-
* GET/HEAD/TRACE at construction, so `request.body` is always `null` for those methods.
38+
* GET/HEAD/TRACE/CONNECT at construction, so `request.body` is always `null` for those methods.
3939
*
4040
* The adapter is stateless — the [logger] is the only field it carries so the DEBUG log
4141
* naming dropped headers attributes to the transport.

sdk-transport-okhttp/src/test/kotlin/org/dexpace/sdk/transport/okhttp/OkHttpTransportTest.kt

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ class OkHttpTransportTest {
159159
assertEquals("0", recorded.headers["Content-Length"], "empty body must report zero length")
160160
}
161161

162-
// -------- body on a body-forbidden method (GET/HEAD/TRACE) --------
162+
// -------- body on a body-forbidden method (GET/HEAD/TRACE/CONNECT) --------
163163

164164
@Test
165165
fun bodyOnForbiddenMethodIsRejectedBeforeDispatch() {
@@ -168,7 +168,7 @@ class OkHttpTransportTest {
168168
// execute's @Throws(IOException) contract. The SDK rejects the body at request
169169
// construction (Request.RequestBuilder.build), so such a request is never built and never
170170
// reaches the transport.
171-
for (method in listOf(Method.GET, Method.HEAD, Method.TRACE)) {
171+
for (method in listOf(Method.GET, Method.HEAD, Method.TRACE, Method.CONNECT)) {
172172
assertFailsWith<IllegalArgumentException>("expected rejection for $method") {
173173
Request.builder()
174174
.method(method)

0 commit comments

Comments
 (0)