io + instrumentation: simplify TeeSink staging and logging/redaction helpers

[OmarAlJarrah]

While reading through the io and instrumentation packages I noticed a handful of small, self-contained simplifications. They're all behavior-preserving and independent — they can land together or one at a time. None of them touch the zero-copy tap/drain semantics or the tapLimit accounting; they just trim duplication and a couple of redundant locals.

sdk-core/src/main/kotlin/org/dexpace/sdk/core/io/TeeSink.kt:107-110 — use minOf for the tap allowance clamp

Old code:

    private fun tapAllowance(requested: Long): Long {
        val remaining = (tapLimit - mirrored).coerceAtLeast(0L)
        return if (requested < remaining) requested else remaining
    }

New code:

    private fun tapAllowance(requested: Long): Long =
        minOf(requested, (tapLimit - mirrored).coerceAtLeast(0L))

Usage — before → after:

// mirrorPrefix(...) — copy the leading bytes up to the remaining tap budget
val copy = tapAllowance(byteCount)

// mirrorPrefix(...) — copy the leading bytes up to the remaining tap budget
val copy = tapAllowance(byteCount)

call sites unchanged — behavior-preserving.

Why: minOf(remaining, requested) is the standard clamp idiom and reads clearer than a hand-rolled if/else min, with an identical result. The remaining-budget clamp via coerceAtLeast(0L) is preserved exactly.

API / Build: minOf(Long, Long) is the primitive, non-boxing stdlib overload and compiles cleanly to Java 8 bytecode; tapAllowance is private, so there is no public-API change.

sdk-core/src/main/kotlin/org/dexpace/sdk/core/io/TeeSink.kt:122-185 — extract a staged helper for the typed-write overrides

Old code:

    @Throws(IOException::class)
    override fun write(source: ByteArray): BufferedSink {
        scratch.write(source)
        drainScratch()
        return this
    }

    @Throws(IOException::class)
    override fun write(
        source: ByteArray,
        offset: Int,
        byteCount: Int,
    ): BufferedSink {
        scratch.write(source, offset, byteCount)
        drainScratch()
        return this
    }

    @Throws(IOException::class)
    override fun writeAll(source: Source): Long {
        var total = 0L
        while (true) {
            val read = source.read(scratch, SCRATCH_BYTES)
            when {
                read == -1L -> break // EOF — normal termination
                read == 0L -> throw IOException(
                    "Source returned 0 for byteCount=$SCRATCH_BYTES which violates the Source.read contract",
                )
                else -> {
                    drainScratch()
                    total += read
                }
            }
        }
        return total
    }

    @Throws(IOException::class)
    override fun writeUtf8(string: String): BufferedSink {
        scratch.writeUtf8(string)
        drainScratch()
        return this
    }

    @Throws(IOException::class)
    override fun writeUtf8(
        string: String,
        beginIndex: Int,
        endIndex: Int,
    ): BufferedSink {
        scratch.writeUtf8(string, beginIndex, endIndex)
        drainScratch()
        return this
    }

    @Throws(IOException::class)
    override fun writeString(
        string: String,
        charset: Charset,
    ): BufferedSink {
        scratch.writeString(string, charset)
        drainScratch()
        return this
    }

New code:

    /**
     * Stages one typed write into [scratch] then tees it into the tap and drains it into the
     * primary in a single pass, returning `this` for chaining. [encode] receives [scratch]
     * explicitly as its `it` argument so the staged write always targets the staging [Buffer] —
     * never one of this `TeeSink`'s own `write*` overrides, which a `Buffer.()` receiver lambda
     * could silently rebind to (and self-recurse) if a `Buffer` overload were ever added.
     */
    private inline fun staged(encode: (Buffer) -> Unit): BufferedSink {
        encode(scratch)
        drainScratch()
        return this
    }

    @Throws(IOException::class)
    override fun write(source: ByteArray): BufferedSink =
        staged { it.write(source) }

    @Throws(IOException::class)
    override fun write(
        source: ByteArray,
        offset: Int,
        byteCount: Int,
    ): BufferedSink = staged { it.write(source, offset, byteCount) }

    @Throws(IOException::class)
    override fun writeAll(source: Source): Long {
        var total = 0L
        while (true) {
            val read = source.read(scratch, SCRATCH_BYTES)
            when {
                read == -1L -> break // EOF — normal termination
                read == 0L -> throw IOException(
                    "Source returned 0 for byteCount=$SCRATCH_BYTES which violates the Source.read contract",
                )
                else -> {
                    drainScratch()
                    total += read
                }
            }
        }
        return total
    }

    @Throws(IOException::class)
    override fun writeUtf8(string: String): BufferedSink =
        staged { it.writeUtf8(string) }

    @Throws(IOException::class)
    override fun writeUtf8(
        string: String,
        beginIndex: Int,
        endIndex: Int,
    ): BufferedSink = staged { it.writeUtf8(string, beginIndex, endIndex) }

    @Throws(IOException::class)
    override fun writeString(
        string: String,
        charset: Charset,
    ): BufferedSink = staged { it.writeString(string, charset) }

Usage — before → after:

// e.g. LoggableRequestBody writing the request body through the tee sink
sink.writeUtf8(jsonPayload)

// e.g. LoggableRequestBody writing the request body through the tee sink
sink.writeUtf8(jsonPayload)

call sites unchanged — behavior-preserving.

Why: Five typed-write overrides repeated the same stage → drainScratch() → return this triple; the helper states that pattern once. Passing the staging buffer explicitly as it (rather than via a Buffer.() receiver) keeps an unqualified write(...) from ever resolving back to this, which would self-recurse.

API / Build: TeeSink is internal and staged is private inline, so there is no public-API change and no apiDump. The helper has a functional parameter, so inlining is meaningful and it won't trip the "insignificant inlining" warning under allWarningsAsErrors; each override keeps its @Throws(IOException::class) and BufferedSink return type. writeAll (its per-chunk pump loop) and write(Buffer, Long) are intentionally left untouched.

sdk-core/src/main/kotlin/org/dexpace/sdk/core/instrumentation/ClientLogger.kt:144-186 — drop the pass-through toSlf4j

Only canLog, slf4jLevel, and toSlf4j change here; the warnDroppedEventFieldOnce guard that sits between them is shown unchanged for context.

Old code:

    public fun canLog(level: LogLevel): Boolean = slf4j.isEnabledForLevel(toSlf4j(level))

    internal fun slf4jLevel(level: LogLevel): Level = toSlf4j(level)

    /**
     * One-shot guard for the [warnDroppedEventFieldOnce] diagnostic. The misuse it flags — a
     * per-event `field("event", …)` colliding with the authoritative `event(name)` tag — is a
     * static call-site error, so a single line per logger surfaces it. State lives here rather
     * than on the single-shot [LoggingEvent] so a hot loop emitting the same misuse can't flood
     * DEBUG.
     */
    private val droppedEventFieldWarned: AtomicBoolean = AtomicBoolean(false)

    /**
     * Emits — at most once per logger — the DEBUG hint that a per-event field named
     * [LoggingEvent.EVENT_KEY] was dropped because `event(name)` owns that key. The
     * [eventNameTag] of the first observed collision is recorded as an example; the fix
     * ("rename the field") is the same regardless of the name.
     *
     * The `isDebugEnabled` check precedes the one-shot CAS so the guard is spent only on an
     * actual emission: if DEBUG is off at the first collision and is enabled later (SLF4J levels
     * can change at runtime), the warning still fires once. When DEBUG is off the call is a single
     * cheap boolean check, which keeps it off the hot path.
     */
    internal fun warnDroppedEventFieldOnce(eventNameTag: String) {
        if (!slf4j.isDebugEnabled) return
        if (!droppedEventFieldWarned.compareAndSet(false, true)) return
        slf4j.debug(
            "LoggingEvent: dropped a field named \"{}\" because event(\"{}\") owns that key; " +
                "rename the field to keep its value. (logged once per logger)",
            LoggingEvent.EVENT_KEY,
            eventNameTag,
        )
    }

    private fun toSlf4j(level: LogLevel): Level =
        when (level) {
            LogLevel.ERROR -> Level.ERROR
            LogLevel.WARNING -> Level.WARN
            LogLevel.INFO -> Level.INFO
            // SLF4J has no VERBOSE; map to DEBUG (the closest convention).
            LogLevel.VERBOSE -> Level.DEBUG
        }

New code:

    public fun canLog(level: LogLevel): Boolean = slf4j.isEnabledForLevel(slf4jLevel(level))

    internal fun slf4jLevel(level: LogLevel): Level =
        when (level) {
            LogLevel.ERROR -> Level.ERROR
            LogLevel.WARNING -> Level.WARN
            LogLevel.INFO -> Level.INFO
            // SLF4J has no VERBOSE; map to DEBUG (the closest convention).
            LogLevel.VERBOSE -> Level.DEBUG
        }

    /**
     * One-shot guard for the [warnDroppedEventFieldOnce] diagnostic. The misuse it flags — a
     * per-event `field("event", …)` colliding with the authoritative `event(name)` tag — is a
     * static call-site error, so a single line per logger surfaces it. State lives here rather
     * than on the single-shot [LoggingEvent] so a hot loop emitting the same misuse can't flood
     * DEBUG.
     */
    private val droppedEventFieldWarned: AtomicBoolean = AtomicBoolean(false)

    /**
     * Emits — at most once per logger — the DEBUG hint that a per-event field named
     * [LoggingEvent.EVENT_KEY] was dropped because `event(name)` owns that key. The
     * [eventNameTag] of the first observed collision is recorded as an example; the fix
     * ("rename the field") is the same regardless of the name.
     *
     * The `isDebugEnabled` check precedes the one-shot CAS so the guard is spent only on an
     * actual emission: if DEBUG is off at the first collision and is enabled later (SLF4J levels
     * can change at runtime), the warning still fires once. When DEBUG is off the call is a single
     * cheap boolean check, which keeps it off the hot path.
     */
    internal fun warnDroppedEventFieldOnce(eventNameTag: String) {
        if (!slf4j.isDebugEnabled) return
        if (!droppedEventFieldWarned.compareAndSet(false, true)) return
        slf4j.debug(
            "LoggingEvent: dropped a field named \"{}\" because event(\"{}\") owns that key; " +
                "rename the field to keep its value. (logged once per logger)",
            LoggingEvent.EVENT_KEY,
            eventNameTag,
        )
    }

Usage — before → after:

// LoggingEvent maps the SDK level onto the SLF4J fluent builder
val builder = logger.slf4j.atLevel(logger.slf4jLevel(level))

// LoggingEvent maps the SDK level onto the SLF4J fluent builder
val builder = logger.slf4j.atLevel(logger.slf4jLevel(level))

call sites unchanged — behavior-preserving.

Why: toSlf4j was a pure forwarding function whose only callers were slf4jLevel and canLog; folding the when into slf4jLevel and pointing canLog at it removes one redundant hop. slf4jLevel stays because LoggingEvent calls it.

API / Build: toSlf4j and slf4jLevel are non-public and canLog's signature is unchanged, so there is no apiCheck impact and no apiDump.

sdk-core/src/main/kotlin/org/dexpace/sdk/core/instrumentation/UrlRedactor.kt:165-196 — collapse the conditional-assignment block in appendRedactedPair

Old code:

    private fun appendRedactedPair(
        out: StringBuilder,
        pair: String,
        allowedLower: Set<String>,
    ) {
        if (pair.isEmpty()) return
        val eq = pair.indexOf('=')
        val encodedName: String
        val hasValue: Boolean
        val encodedValue: String
        if (eq < 0) {
            encodedName = pair
            hasValue = false
            encodedValue = ""
        } else {
            encodedName = pair.substring(0, eq)
            hasValue = true
            encodedValue = pair.substring(eq + 1)
        }

        val allowed = allowedLower.contains(safeDecode(encodedName).lowercase())

        out.append(encodedName)
        if (hasValue) {
            out.append('=')
            if (allowed) {
                out.append(encodedValue)
            } else {
                out.append(REDACTED_ENCODED)
            }
        }
    }

New code:

    private fun appendRedactedPair(
        out: StringBuilder,
        pair: String,
        allowedLower: Set<String>,
    ) {
        if (pair.isEmpty()) return
        val eq = pair.indexOf('=')
        val encodedName = if (eq < 0) pair else pair.substring(0, eq)
        out.append(encodedName)
        if (eq < 0) return

        out.append('=')
        if (allowedLower.contains(safeDecode(encodedName).lowercase())) {
            out.append(pair.substring(eq + 1))
        } else {
            out.append(REDACTED_ENCODED)
        }
    }

Usage — before → after:

// appendRedactedQuery splits the query on '&' and redacts each pair
appendRedactedPair(out, pair, allowedLower)

// appendRedactedQuery splits the query on '&' and redacts each pair
appendRedactedPair(out, pair, allowedLower)

call sites unchanged — behavior-preserving.

Why: The three vals (encodedName / hasValue / encodedValue) plus an if/else collapse to a single encodedName and an early return for the bare-name case. The value substring now happens only on the allowed branch, so the redacted path no longer computes and discards pair.substring(eq + 1); the eq < 0 path also returns before the now-unused allow-list decode. Output is byte-for-byte identical.

API / Build: appendRedactedPair is a private helper — no API or build-gate impact.