Skip to content

http/request: remove redundant toReplayable overrides and derive Method token from enum name #174

Description

@OmarAlJarrah

Related: #113 tracks the JDK transport buffering non-replayable bodies through the same toReplayable path — a separate behavior fix, not part of these cleanups.

While reading through http/request I noticed a small cluster of declarations that
restate behavior they already inherit. The bodies that report isReplayable() == true
each override toReplayable to return this, which is exactly what the base class already
does for any replayable body; and Method threads a wire-token string through its
constructor that is always identical to the enum constant's own name. None of this is
load-bearing — it's duplication that makes the types look like they're doing more than they
are. The three cleanups below are behavior-preserving.

sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/RequestBody.kt:228, 247, 277 — Drop the no-op toReplayable overrides on the replayable bodies

The base RequestBody.toReplayable short-circuits to this whenever isReplayable() is
true. The three private replayable bodies all return true from isReplayable() and then
re-declare an override that returns this — the exact behavior they'd inherit.

Old code:

// Base RequestBody.toReplayable already returns `this` for any replayable body (UNCHANGED — shown for context):
@JvmOverloads
public open fun toReplayable(provider: IoProvider = Io.provider): RequestBody {
    if (isReplayable()) return this
    val buffer = provider.buffer()
    writeTo(buffer)
    return BufferRequestBody(buffer, mediaType(), buffer.size)
}

// ...

/** Replayable body backed by an in-memory [Buffer]. Reads via non-consuming `peek()`. */
private class BufferRequestBody(
    private val buffer: Buffer,
    private val mediaType: MediaType?,
    private val length: Long,
) : RequestBody() {
    override fun mediaType(): MediaType? = mediaType

    override fun contentLength(): Long = length

    override fun isReplayable(): Boolean = true

    override fun toReplayable(provider: IoProvider): RequestBody = this

    @Throws(IOException::class)
    override fun writeTo(sink: BufferedSink) {
        sink.writeAll(buffer.peek())
    }
}

/** Replayable body backed by a byte array. */
private class ByteArrayRequestBody(
    private val bytes: ByteArray,
    private val mediaType: MediaType?,
) : RequestBody() {
    override fun mediaType(): MediaType? = mediaType

    override fun contentLength(): Long = bytes.size.toLong()

    override fun isReplayable(): Boolean = true

    override fun toReplayable(provider: IoProvider): RequestBody = this

    @Throws(IOException::class)
    override fun writeTo(sink: BufferedSink) {
        sink.write(bytes)
    }
}

/**
 * Replayable body backed by an [InputStream] that supports `mark/reset`. The stream is
 * marked at construction time; each [writeTo] after the first calls `reset()` to rewind
 * before reading.
 *
 * The `firstWrite` toggle is an [AtomicBoolean] so concurrent `writeTo` calls cannot both
 * skip (or both perform) the `reset()` — at most one transitions the flag, ensuring exactly
 * one reset between any two writes.
 */
private class ResettableInputStreamRequestBody(
    private val input: InputStream,
    private val length: Long,
    private val mediaType: MediaType?,
) : RequestBody() {
    private val hasWritten = AtomicBoolean(false)

    override fun mediaType(): MediaType? = mediaType

    override fun contentLength(): Long = length

    override fun isReplayable(): Boolean = true

    override fun toReplayable(provider: IoProvider): RequestBody = this

    @Throws(IOException::class)
    override fun writeTo(sink: BufferedSink) {
        if (hasWritten.getAndSet(true)) {
            input.reset()
        }
        copyExactly(input, sink, length)
    }
}

New code:

/** Replayable body backed by an in-memory [Buffer]. Reads via non-consuming `peek()`. */
private class BufferRequestBody(
    private val buffer: Buffer,
    private val mediaType: MediaType?,
    private val length: Long,
) : RequestBody() {
    override fun mediaType(): MediaType? = mediaType

    override fun contentLength(): Long = length

    override fun isReplayable(): Boolean = true

    @Throws(IOException::class)
    override fun writeTo(sink: BufferedSink) {
        sink.writeAll(buffer.peek())
    }
}

/** Replayable body backed by a byte array. */
private class ByteArrayRequestBody(
    private val bytes: ByteArray,
    private val mediaType: MediaType?,
) : RequestBody() {
    override fun mediaType(): MediaType? = mediaType

    override fun contentLength(): Long = bytes.size.toLong()

    override fun isReplayable(): Boolean = true

    @Throws(IOException::class)
    override fun writeTo(sink: BufferedSink) {
        sink.write(bytes)
    }
}

/**
 * Replayable body backed by an [InputStream] that supports `mark/reset`. The stream is
 * marked at construction time; each [writeTo] after the first calls `reset()` to rewind
 * before reading.
 *
 * The `firstWrite` toggle is an [AtomicBoolean] so concurrent `writeTo` calls cannot both
 * skip (or both perform) the `reset()` — at most one transitions the flag, ensuring exactly
 * one reset between any two writes.
 */
private class ResettableInputStreamRequestBody(
    private val input: InputStream,
    private val length: Long,
    private val mediaType: MediaType?,
) : RequestBody() {
    private val hasWritten = AtomicBoolean(false)

    override fun mediaType(): MediaType? = mediaType

    override fun contentLength(): Long = length

    override fun isReplayable(): Boolean = true

    @Throws(IOException::class)
    override fun writeTo(sink: BufferedSink) {
        if (hasWritten.getAndSet(true)) {
            input.reset()
        }
        copyExactly(input, sink, length)
    }
}

Usage — before → after:

// `body` is one of the three replayable bodies; isReplayable() == true
val replayable = body.toReplayable() // base short-circuit returns `this`
val replayable = body.toReplayable() // call sites unchanged — behavior-preserving

Why: Each class already returns true from isReplayable(), so the inherited
toReplayable hits its if (isReplayable()) return this short-circuit and hands back the
same instance. The overrides only restate that, so removing them changes nothing.

API / Build: All three classes are private and never appear in the .api snapshot, so
no apiDump is needed. The Io / IoProvider imports stay referenced by the base
toReplayable, so nothing becomes an unused import.

sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/FileRequestBody.kt:103 — Drop the no-op toReplayable override and its now-unused import

Same story as above, but this body is public. The base returns this for a replayable body,
and this is still the FileRequestBody, so the transport-side is FileRequestBody
zero-copy / sendfile(2) dispatch keeps matching. Removing the override leaves
import org.dexpace.sdk.core.io.IoProvider referenced nowhere else in the file.

Old code:

import org.dexpace.sdk.core.http.common.MediaType
import org.dexpace.sdk.core.io.BufferedSink
import org.dexpace.sdk.core.io.IoProvider
import java.io.IOException
import java.nio.ByteBuffer
import java.nio.channels.Channels
import java.nio.channels.FileChannel
import java.nio.file.Files
import java.nio.file.NoSuchFileException
import java.nio.file.Path
import java.nio.file.StandardOpenOption

// ...

        override fun mediaType(): MediaType? = mediaType

        override fun contentLength(): Long = count

        override fun isReplayable(): Boolean = true

        override fun toReplayable(provider: IoProvider): RequestBody = this

New code:

import org.dexpace.sdk.core.http.common.MediaType
import org.dexpace.sdk.core.io.BufferedSink
import java.io.IOException
import java.nio.ByteBuffer
import java.nio.channels.Channels
import java.nio.channels.FileChannel
import java.nio.file.Files
import java.nio.file.NoSuchFileException
import java.nio.file.Path
import java.nio.file.StandardOpenOption

// ...

        override fun mediaType(): MediaType? = mediaType

        override fun contentLength(): Long = count

        override fun isReplayable(): Boolean = true

Usage — before → after:

val replayable = body.toReplayable()
check(replayable is FileRequestBody) // true — base toReplayable() returns `this`
val replayable = body.toReplayable()
check(replayable is FileRequestBody) // call sites unchanged — behavior-preserving

Why: FileRequestBody.isReplayable() is true, so the inherited toReplayable returns
this — the same FileRequestBody — which is exactly what the override does. Transports that
type-check is FileRequestBody for the sendfile(2) fast path keep matching, so the override
is dead weight.

API: FileRequestBody is public, so the override is in the snapshot
(sdk-core/api/sdk-core.api:
public fun toReplayable (Lorg/dexpace/sdk/core/io/IoProvider;)Lorg/dexpace/sdk/core/http/request/RequestBody;).
Removing it drops that line — run ./gradlew apiDump and commit the regenerated
sdk-core/api/sdk-core.api. The inherited RequestBody.toReplayable stays public and callable.

Build: with the override gone, import org.dexpace.sdk.core.io.IoProvider is referenced
nowhere in the file; ktlint's no-unused-imports (and allWarningsAsErrors) fails the build
unless it is deleted in the same change.

sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/Method.kt:29-44 — Derive the wire token from the enum name instead of a duplicated constructor arg

Every constant passes a string literal that is identical to its own enum name
(GET("GET", …), POST("POST", …), …), and toString() just returns that string — which is
also what the default enum toString() already does. The token can be a computed property over
name, dropping the constructor parameter, the nine duplicated literals, and the toString()
override.

Old code:

/**
 * HTTP request methods recognized by the SDK. Each constant carries the canonical token used
 * on the wire; [toString] returns that same token so the enum can be written directly into a
 * request line without translation.
 *
 * @property method Canonical uppercase method token sent in the request line.
 * @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(
    public val method: String,
    public val permitsRequestBody: Boolean,
) {
    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
}

New code:

/**
 * HTTP request methods recognized by the SDK. Each constant's name is the canonical token used
 * on the wire, so the enum's default `toString()` returns that token and the constant can be
 * written directly into a request line without translation.
 *
 * @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(public val permitsRequestBody: Boolean) {
    GET(permitsRequestBody = false),
    POST(permitsRequestBody = true),
    PUT(permitsRequestBody = true),
    DELETE(permitsRequestBody = true),
    PATCH(permitsRequestBody = true),
    HEAD(permitsRequestBody = false),
    OPTIONS(permitsRequestBody = true),
    TRACE(permitsRequestBody = false),
    CONNECT(permitsRequestBody = false),
    ;

    /** Canonical uppercase method token sent in the request line; identical to the enum [name]. */
    public val method: String get() = name
}

Usage — before → after:

val token = request.method.method   // "POST"
val rendered = Method.POST.toString() // "POST"
val token = request.method.method   // "POST" — `method` now returns `name`
val rendered = Method.POST.toString() // "POST" — default enum toString() returns `name`

Why: The token literal in every constant is just the constant's own name, and the
overridden toString() returns the same value the default enum toString() already produces.
Computing method from name removes nine duplicated literals plus the override while keeping
Method.POST.method and Method.POST.toString() equal to "POST".

API: getMethod() is unchanged (the property is still public, now computed), but the
toString() override leaves the snapshot — sdk-core/api/sdk-core.api currently lists
public fun toString ()Ljava/lang/String; under Method. Run ./gradlew apiDump and commit
the regenerated sdk-core/api/sdk-core.api. (The enum constructor is implicitly private and
not part of the snapshot, so dropping the parameter is not itself an API change.)

Metadata

Metadata

Assignees

No one assigned

    Labels

    sdk-coresdk-core toolkittech-debtsimplification / cleanup

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions