From a1138df38ad92e5de0369a9f1d1bf99ab86f1b2f Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 7 Jul 2026 13:34:22 +0300 Subject: [PATCH 1/3] chore: remove an internal planning scratch file committed by mistake .superpowers/sdd/progress.md was a local planning note that should never have been tracked; drop it from the branch. --- .superpowers/sdd/progress.md | 67 ------------------------------------ 1 file changed, 67 deletions(-) delete mode 100644 .superpowers/sdd/progress.md diff --git a/.superpowers/sdd/progress.md b/.superpowers/sdd/progress.md deleted file mode 100644 index 9d58dc4..0000000 --- a/.superpowers/sdd/progress.md +++ /dev/null @@ -1,67 +0,0 @@ -# DX Improvements — Progress Ledger - -Branch: worktree-dx-improvements | Base: b015fd3 -Plan: docs/superpowers/plans/2026-07-03-dx-improvements.md - -## Work units (file-cohesive) -- [x] U1 Response family (T1 Status preds, T2 isSuccessful+string/bytes, T3 throwOnError+bodyAs, T9 deserialize) -- [x] U2 Request family + MediaType (T4, T5, T6, T7, T8-core) -- [x] U3 Jackson serde (T10 Tristate fix, T8 jsonBody) -- [x] U5 *Options builders + config + JDK timeout (T12, T13) -- [x] U6 ServiceLoader IoProvider (T15) -- [x] Pipeline steps (T16 throwOnHttpError, T17 standard, T22 pillar-fail, T25 HttpPipeline:HttpClient) -- [ ] U8 Async + transports (T14 NetworkException, T18 handleWith, T19 cancel, T23 proxyEnv) -- [x] U9 Serde TypeRef + pagination SAM (T20, T21) -- [x] U10 Value-class demotion (T24 ETag/HttpRange/RequestConditions) -- [x] U11 Per-request RequestOptions (T26) -- [x] U12 Recovery-stack rename (T27, T28) -- [x] U4 Docs (T11) — after ServiceLoader so samples are final -- [ ] FINAL apiDump + full gated build + final review + PR - -## Completed -- U1: complete (commits 43a101e..a51d5d1, gates green: 2002 tests, detekt, apiCheck, ktlint) -- U2: complete (commits 0ac3035..e1283ee, 36 tests, gates green) -- U3: complete (commits db870b5..c5b456d, 38 tests, gates green) - -## Wave-1 review findings (from wave1-review.md) -- FIX (Important) bodyAs: close body in finally (ResponseExtensions.kt ~60-68) -- FIX (Important) throwOnError: bound buffered body (cap ~1 MiB) + doc (ResponseExtensions.kt ~34-42) -- FIX (Important) T3 wording: "replayable" -> "buffered in memory, readable after throw" -- FIX (Minor) bodyAs: debug-log the swallowed exception (ref e) -- FIX (Minor) delete(): clear stale body for consistency with get()/head() -- DEFER to final: #6 jsonBody redundancy (plan-mandated, adjudicate), #8 create(value) nullability doc -- U5: complete (commits 3a1482f..275fc33, 11 tests, gates green) -- Wave-1 fixes: complete (commit c54e8cf; leak, OOM-cap, wording, log, delete-body — tests green) -- U6: complete (commit 7e66659, ServiceLoader fallback, full sdk-core suite green) -- U7a: complete (commits 8d54cee..c84cdc2; added Stage PRE_REDIRECT(50); NOTE: subagent briefly committed on MAIN then reverted it to b015fd3 — main verified clean, user kuri changes untouched) -- U7b: complete (commits b9196cb..c7a8cb5, pillar hard-fail + HttpPipeline:HttpClient; cwd guard held, main clean) -- U8a: complete (commits f3830bf..eba18e1, transports NetworkException + proxyFromEnvironment, 13 suites green) -- U8b: complete (commits e39ae69..0b22857, handleWith + unified cancel; 2nd main-slip reverted cleanly, main verified intact) -- U9: complete (commits 85f3307..7db7482, SAM extractors + TypeRef, gates green, path guard held) - -## Final-review items (defer) -- jsonHandlerOrThrow leaves LIVE error body on throw; throwOnError BUFFERS — reconcile for consistency (buffer both, bounded 1 MiB) -- jsonBody(serde,value) redundancy vs RequestBody.create(value,serde) — decide keep/drop -- docs/pipelines.md stale "replace emits warning" line (U7b) — fix in U4 docs -- run aggregate koverVerify (80% floor) at finalization -- U10: complete (commit fcbe657, ETag/HttpRange demoted + String overloads, de-mangling proven) -- U11: complete (commits 2f707ac..5f8e494, RequestOptions timeout+maxRetries wired, full gate green) -- final-review: U11 negative per-call maxRetries used as-is (no clamp) — decide if OK; PipelineNext.options now public -- U12: complete (commit 2d8df24, recovery stack -> RecoveryChain/RetryRecovery/ThrowOnHttpErrorRecovery, grep-clean, gates green) -- U4: complete (commit d36b3dd, README+samples, sdk-example test+run green) -- ALL 13 UNITS COMPLETE — starting finalization - -## Finalization -- apiDump reconciled (virtualthreads straggler committed) -- FULL GATED BUILD: BUILD SUCCESSFUL — all module tests, detekt, ktlint, allWarningsAsErrors, apiCheck, R8 shrink guard, kover 80% floor ALL GREEN -- Final whole-branch review: running -- TODO: triage final review + deferred items -> one fix wave -> PR -- Final review fix wave: complete (02bec41,5d95eea,e752dbc,cfce016,3ef4028) — all 5 findings fixed, gate green -- NOTE: intermediate commit cfce016 api-stale (same-file entanglement); branch HEAD green -- Running final full gated build before push - -## DONE -- Final full gated build: BUILD SUCCESSFUL (kover+R8+all gates) -- Pushed worktree-dx-improvements -> origin/dx-improvements -- PR #208 opened (base main), clean (no styleguide/.superpowers/plan), CI build running -- 33 commits, +6277/-761 across 124 files From ddbbdd7c6eac57ed1423e3d38740e110ae20e32a Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 7 Jul 2026 13:34:36 +0300 Subject: [PATCH 2/3] fix: correct cancellation, timeout classification, and error-handling edges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Futures: raise the interrupting flag before clearing the worker reference in InterruptibleFuture.cancel. The previous order let a worker finish its task, skip the unbind spin, return to its pool, and only then receive an interrupt aimed at the cancelled call — landing it on an unrelated task. - JDK transport: stop misclassifying a read timeout as a cancellation. SocketTimeoutException extends InterruptedIOException, so the sync catch set the caller's interrupt flag on a plain timeout and the async mapper returned it un-normalized. Both now surface it as a retryable NetworkException, matching the OkHttp transport and the retry steps' carve-out. - Jackson: JacksonSerde.from() no longer fails construction for an ObjectMapper subclass whose copy() is unsupported — it falls back to using the supplied mapper. - OperationRequestAssembler: catch IllegalArgumentException (what url(String) now throws) so a base URL that resolves to a malformed URL keeps its enriched base-URL/operation context instead of the builder's generic message. - RequestOptions.maxRetries: reject a negative override at build time rather than letting it be silently reinterpreted downstream as "use the configured default", mirroring timeout()'s validation. 0 still disables retries. - throwOnHttpError recovery: buffer the error body up to the shared 1 MiB bound instead of 4 KiB, so a large error payload is not truncated mid-value only on the recovery path. --- .../sdk/core/http/request/RequestOptions.kt | 15 ++++++-- .../operation/OperationRequestAssembler.kt | 24 +++++++------ .../pipeline/step/ThrowOnHttpErrorRecovery.kt | 16 +++++---- .../org/dexpace/sdk/core/util/Futures.kt | 18 +++++----- .../steps/DefaultAsyncRetryStepTest.kt | 14 ++++---- .../pipeline/steps/RetryPolicySupportTest.kt | 12 ++++--- .../core/http/pipeline/steps/RetryStepTest.kt | 15 ++++---- .../core/http/request/RequestOptionsTest.kt | 15 ++++++++ .../dexpace/sdk/serde/jackson/JacksonSerde.kt | 34 +++++++++++++------ .../sdk/transport/jdkhttp/JdkHttpTransport.kt | 11 ++++++ .../jdkhttp/internal/TransportFailures.kt | 6 ++++ 11 files changed, 120 insertions(+), 60 deletions(-) diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/RequestOptions.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/RequestOptions.kt index 9ac2a3e..52c8d44 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/RequestOptions.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/request/RequestOptions.kt @@ -45,7 +45,7 @@ import org.dexpace.sdk.core.generics.Builder as SdkBuilder * single-attempt bound. * @property tags Opaque per-call tags, keyed by string. Read-only; never `null`, may be empty. * @property maxRetries Per-call retry-count override, or `null` to keep the retry step's configured - * budget. `0` disables retries for this call. + * budget. `0` disables retries for this call; a negative value is rejected by the builder. */ public class RequestOptions private constructor( public val timeout: Duration?, @@ -123,10 +123,21 @@ public class RequestOptions private constructor( /** * Sets the per-call retry-count override, or clears it when [maxRetries] is `null` (fall - * back to the retry step's configured budget). `0` disables retries for the call. + * back to the retry step's configured budget). + * + * A non-`null` [maxRetries] must be non-negative: a negative value is rejected here rather + * than reaching the retry step, where it would be silently reinterpreted as "use the + * configured default" — the opposite of the caller's likely intent to suppress retries. + * Pass `0` to disable retries for the call, or `null` to use the transport / pipeline + * default. + * + * @throws IllegalArgumentException if [maxRetries] is negative. */ public fun maxRetries(maxRetries: Int?): Builder = apply { + require(maxRetries == null || maxRetries >= 0) { + "maxRetries must be non-negative (or null to use the transport default), was $maxRetries" + } this.maxRetries = maxRetries } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt index 036e47e..810a5aa 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt @@ -9,7 +9,6 @@ package org.dexpace.sdk.core.operation import org.dexpace.sdk.core.http.common.PercentEncoding import org.dexpace.sdk.core.http.request.Request -import java.net.MalformedURLException /** * Assembles a [Request] from an [OperationParams] projection and a base URL. Internal — callers @@ -36,21 +35,24 @@ internal object OperationRequestAssembler { val path = resolvePath(params.pathTemplate, params.pathParams()) val query = params.queryParams().encode() val url = buildUrl(baseUrl, path, query) - return try { - Request.builder() - .method(params.method) - .url(url) - .headers(params.headers()) - .body(params.body()) - .build() - } catch (e: MalformedURLException) { - // Surface a malformed base URL (e.g. missing scheme) as the SPI's documented - // IllegalArgumentException rather than leaking a checked IOException to callers. + val builder = Request.builder().method(params.method) + // Guard only the URL resolution: `url(String)` rejects a malformed base URL with an + // IllegalArgumentException whose generic "Invalid URL: …" message drops the base-URL / + // operation context. Re-throw it with that context restored, and keep the guard tight so + // an unrelated IllegalArgumentException from build() (e.g. a missing required field) is not + // mislabelled as a URL problem. + try { + builder.url(url) + } catch (e: IllegalArgumentException) { throw IllegalArgumentException( "Cannot assemble a request: base URL '$baseUrl' resolved to a malformed URL '$url'", e, ) } + return builder + .headers(params.headers()) + .body(params.body()) + .build() } private fun resolvePath( diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorRecovery.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorRecovery.kt index a7cd08b..25caaf0 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorRecovery.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorRecovery.kt @@ -8,6 +8,7 @@ package org.dexpace.sdk.core.pipeline.step import org.dexpace.sdk.core.http.context.DispatchContext +import org.dexpace.sdk.core.http.response.MAX_BUFFERED_ERROR_BODY_BYTES import org.dexpace.sdk.core.http.response.Response import org.dexpace.sdk.core.http.response.bufferErrorBody import org.dexpace.sdk.core.http.response.exception.HttpException @@ -45,17 +46,18 @@ import org.dexpace.sdk.core.http.response.exception.HttpExceptionFactory * which would also close the live response body. To keep the error body readable on the * resulting [org.dexpace.sdk.core.pipeline.ResponseOutcome.Failure] — for a log-line * [HttpException.bodySnapshot] or downstream typed-error deserialization — this step buffers the - * error body into a small, replayable in-memory body before constructing the exception, via the + * error body into a bounded, replayable in-memory body before constructing the exception, via the * shared [org.dexpace.sdk.core.http.response.bufferErrorBody] helper (the same routine that backs * [org.dexpace.sdk.core.http.response.throwOnError], so the two error-mapping paths share one * bounded, replayable implementation and cannot drift). The buffer is bounded at - * [HttpException.DEFAULT_SNAPSHOT_BYTES], consistent with the `bodySnapshot` cap (smaller than the - * standalone `throwOnError` cap, since this copy is primarily a log-line snapshot), so a - * multi-megabyte 5xx body cannot OOM the process. Mapping the buffered body into a typed error - * value is still left to the generated layer (see [HttpException.value]). + * [MAX_BUFFERED_ERROR_BODY_BYTES] — the same 1 MiB cap [org.dexpace.sdk.core.http.response.throwOnError] + * and the stage-layer error mapping use, so all error-body buffering shares one bound and a + * sub-megabyte error payload survives whole while a multi-megabyte 5xx body cannot OOM the process. + * Mapping the buffered body into a typed error value is still left to the generated layer (see + * [HttpException.value]). * * The cap is a hard truncation, not just an OOM guard: an error body larger than - * [HttpException.DEFAULT_SNAPSHOT_BYTES] is preserved only up to that many bytes, and the excess + * [MAX_BUFFERED_ERROR_BODY_BYTES] is preserved only up to that many bytes, and the excess * is discarded with no marker. This is harmless for a log-line snapshot, but a downstream * consumer that deserializes the buffered body (e.g. the generated layer parsing it into * [HttpException.value]) must tolerate a structurally incomplete payload — a JSON object cut @@ -81,7 +83,7 @@ public object ThrowOnHttpErrorRecovery : ResponsePipelineStep { return input } throw HttpExceptionFactory.fromResponse( - bufferErrorBody(input, HttpException.DEFAULT_SNAPSHOT_BYTES.toLong()), + bufferErrorBody(input, MAX_BUFFERED_ERROR_BODY_BYTES), ) } } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/util/Futures.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/util/Futures.kt index 0111093..e375c64 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/util/Futures.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/util/Futures.kt @@ -123,14 +123,16 @@ private class InterruptibleFuture : CompletableFuture() { override fun cancel(mayInterruptIfRunning: Boolean): Boolean { val cancelled = super.cancel(mayInterruptIfRunning) if (cancelled && mayInterruptIfRunning) { - val thread = worker.getAndSet(null) - if (thread != null) { - interrupting.set(true) - try { - thread.interrupt() - } finally { - interrupting.set(false) - } + // Raise the flag BEFORE reading the worker, mirroring FutureTask's transition to + // INTERRUPTING before it reads the runner thread. Ordering it the other way lets a + // worker finish, observe interrupting==false in unbindWorker(), return to its pool, and + // pick up an unrelated task — only to then receive this in-flight interrupt, poisoning a + // pooled thread mid-unrelated-work. + interrupting.set(true) + try { + worker.getAndSet(null)?.interrupt() + } finally { + interrupting.set(false) } } return cancelled diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncRetryStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncRetryStepTest.kt index 0fe2611..9724d8f 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncRetryStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncRetryStepTest.kt @@ -117,17 +117,17 @@ class DefaultAsyncRetryStepTest { } @Test - fun `negative per-call maxRetries falls back to the configured budget, not zero retries`() { - // A negative per-call override is clamped to "use default" — mirroring the configured-path - // clamp — so the configured budget of 3 stays in force rather than collapsing to 0 retries. - val client = QueueClient().enqueue(503).enqueue(503).enqueue(503).enqueue(200) - val options = RequestOptions.builder().maxRetries(-1).build() + fun `per-call maxRetries of zero performs exactly one attempt`() { + // 0 is honoured as a real budget: the 503 is returned without any retry, even though the + // configured budget is 3 — the per-call override takes effect. + val client = QueueClient().enqueue(503).enqueue(200) + val options = RequestOptions.builder().maxRetries(0).build() val future = pipeline(client, HttpRetryOptions.fixed(maxRetries = 3, delay = Duration.ofMillis(50))) .sendAsync(getRequest(), options) scheduler.runAll() - assertEquals(200, future.join().status.code) - assertEquals(4, client.callCount, "negative override must fall back to the configured budget of 3") + assertEquals(503, future.join().status.code) + assertEquals(1, client.callCount, "maxRetries(0) must suppress retries even under a configured budget of 3") } @Test diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryPolicySupportTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryPolicySupportTest.kt index c372c27..ca40189 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryPolicySupportTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryPolicySupportTest.kt @@ -11,6 +11,7 @@ import org.dexpace.sdk.core.http.request.RequestOptions import org.dexpace.sdk.core.instrumentation.ClientLogger import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith class RetryPolicySupportTest { private val logger = ClientLogger(RetryPolicySupportTest::class) @@ -28,11 +29,12 @@ class RetryPolicySupportTest { } @Test - fun `negative per-call override falls back to the configured budget, not zero retries`() { - // A negative per-call value means "use the configured default", mirroring how a negative - // CONFIGURED maxRetries is clamped — it must NOT collapse to 0 retries. - assertEquals(5, support(configuredMaxRetries = 5).effectiveMaxRetries(options(-1))) - assertEquals(5, support(configuredMaxRetries = 5).effectiveMaxRetries(options(-100))) + fun `negative per-call maxRetries is rejected by the builder`() { + // The builder fails fast on a negative override, so effectiveMaxRetries never sees one: a + // caller wanting to suppress retries must pass 0, not a negative sentinel that would once + // have been silently reinterpreted as "use the configured default". + assertFailsWith { options(-1) } + assertFailsWith { options(-100) } } @Test diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryStepTest.kt index f9c29c7..5dbd83f 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryStepTest.kt @@ -155,14 +155,11 @@ class RetryStepTest { } @Test - fun `negative per-call maxRetries falls back to the configured budget, not zero retries`() { - // A negative per-call override is clamped to "use default" — mirroring the configured-path - // clamp in RetryPolicySupport — so the configured budget of 3 stays in force rather than - // collapsing to 0 retries. + fun `per-call maxRetries of zero performs exactly one attempt`() { + // 0 is honoured as a real budget: the 503 is returned without any retry, even though the + // configured budget is 3 — the per-call override takes effect. val fake = FakeHttpClient() - .enqueue { status(503) } - .enqueue { status(503) } .enqueue { status(503) } .enqueue { status(200) } @@ -171,10 +168,10 @@ class RetryStepTest { .append(DefaultRetryStep(HttpRetryOptions(maxRetries = 3), zeroDelayClock())) .build() - val options = RequestOptions.builder().maxRetries(-1).build() + val options = RequestOptions.builder().maxRetries(0).build() val response = pipeline.send(getRequest(), options) - assertEquals(200, response.status.code) - assertEquals(4, fake.callCount, "negative override must fall back to the configured budget of 3") + assertEquals(503, response.status.code) + assertEquals(1, fake.callCount, "maxRetries(0) must suppress retries even under a configured budget of 3") } @Test diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestOptionsTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestOptionsTest.kt index 365247f..f9bd088 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestOptionsTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/request/RequestOptionsTest.kt @@ -55,6 +55,21 @@ class RequestOptionsTest { } } + @Test + fun `maxRetries rejects a negative override`() { + // Fail fast rather than let a negative value be silently reinterpreted downstream as + // "use the configured default" — the opposite of a caller's likely intent to suppress + // retries. Callers disable retries with 0, not a negative sentinel. + assertFailsWith { + RequestOptions.builder().maxRetries(-1) + } + } + + @Test + fun `maxRetries accepts zero to disable retries`() { + assertEquals(0, RequestOptions.builder().maxRetries(0).build().maxRetries) + } + @Test fun `timeout accepts null to clear the override`() { val options = RequestOptions.builder().timeout(Duration.ofSeconds(1)).timeout(null).build() diff --git a/sdk-serde-jackson/src/main/kotlin/org/dexpace/sdk/serde/jackson/JacksonSerde.kt b/sdk-serde-jackson/src/main/kotlin/org/dexpace/sdk/serde/jackson/JacksonSerde.kt index 1a4a106..e9264a3 100644 --- a/sdk-serde-jackson/src/main/kotlin/org/dexpace/sdk/serde/jackson/JacksonSerde.kt +++ b/sdk-serde-jackson/src/main/kotlin/org/dexpace/sdk/serde/jackson/JacksonSerde.kt @@ -106,10 +106,13 @@ public class JacksonSerde private constructor( * Build a [JacksonSerde] backed by a caller-supplied [mapper]. * * This method operates on a **defensive copy** of [mapper] — the caller's original instance - * is never mutated. Because the copy is taken via [ObjectMapper.copy], the supplied mapper - * must be a standard `ObjectMapper` / `JsonMapper` (or a subclass that overrides `copy()`): - * Jackson's own `copy()` throws `IllegalStateException` for a subclass that does not override - * it. Modules registered on [mapper] *after* this call are not reflected in the copy — register + * is never mutated — **for a standard `ObjectMapper` / `JsonMapper` (or a subclass that + * overrides `copy()`)**. The copy is taken via [ObjectMapper.copy], and Jackson's own + * `copy()` throws `IllegalStateException` for a subclass that does not override it. For such + * a subclass the SDK no longer fails construction: it falls back to using the supplied mapper + * directly (registering [TristateModule] on it when [registerTristate] is `true`) instead of + * throwing. In that fallback the caller's instance *is* mutated by the module registration. + * Modules registered on [mapper] *after* this call are not reflected in the copy — register * them before calling [from], or register them on the returned serde's [mapper]. * * ## Tristate PATCH semantics @@ -118,7 +121,8 @@ public class JacksonSerde private constructor( * registered on the underlying mapper. Without it, [org.dexpace.sdk.core.serde.Tristate.Absent] * and [org.dexpace.sdk.core.serde.Tristate.Null] are indistinguishable on the wire — both * serialize to a non-absent representation, silently corrupting `PATCH` payloads. By default, - * [from] registers [TristateModule] on the copy automatically. + * [from] registers [TristateModule] automatically — on the defensive copy, or on the supplied + * mapper itself in the subclass fallback described above. * * Pass `registerTristate = false` only when the caller's mapper already carries a * manually-registered [TristateModule] (or a compatible substitute) and a second @@ -130,9 +134,9 @@ public class JacksonSerde private constructor( * caller's [OutputStream]/[InputStream] is left open. * * @param mapper The mapper to copy and use as the serialization engine. - * @param registerTristate When `true` (the default), [TristateModule] is registered on - * the copy. Pass `false` when the caller has already registered the module or a - * compatible substitute. + * @param registerTristate When `true` (the default), [TristateModule] is registered on the + * engine ([mapper]'s defensive copy, or [mapper] itself in the subclass fallback). Pass + * `false` when the caller has already registered the module or a compatible substitute. */ @JvmStatic @JvmOverloads @@ -140,9 +144,17 @@ public class JacksonSerde private constructor( mapper: ObjectMapper, registerTristate: Boolean = true, ): JacksonSerde { - val copy = mapper.copy() - if (registerTristate) copy.registerModule(TristateModule()) - return JacksonSerde(copy) + val engine = + try { + mapper.copy() + } catch (e: IllegalStateException) { + // ObjectMapper.copy() throws for a subclass that does not override copy(). Rather than + // fail construction, fall back to the supplied mapper (which we then mutate by registering + // TristateModule), matching the behavior for such mappers before the defensive copy. + mapper + } + if (registerTristate) engine.registerModule(TristateModule()) + return JacksonSerde(engine) } } } diff --git a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt index 860e434..c55ae31 100644 --- a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt +++ b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/JdkHttpTransport.kt @@ -29,6 +29,7 @@ import java.net.PasswordAuthentication import java.net.Proxy import java.net.ProxySelector import java.net.SocketAddress +import java.net.SocketTimeoutException import java.net.URI import java.net.http.HttpResponse import java.time.Duration @@ -136,6 +137,16 @@ public class JdkHttpTransport private constructor( val wrapped = InterruptedIOException("interrupted while waiting for response") wrapped.initCause(e) throw wrapped + } catch (e: SocketTimeoutException) { + // `SocketTimeoutException` extends `InterruptedIOException`, but a response/read + // timeout is a retryable transport failure, not a thread interrupt. Letting it fall + // into the `InterruptedIOException` branch below would spuriously set the caller's + // interrupt flag. Classify it exactly like a generic transport IOException so it + // surfaces as a retryable `NetworkException`, matching the retry steps' carve-out + // (`is InterruptedIOException && !is SocketTimeoutException`, treating a socket + // timeout as a retryable timeout). This catch must precede the + // `InterruptedIOException` one to stay reachable, since it is a subtype. + throw NetworkException(e.message, e) } catch (e: InterruptedIOException) { Thread.currentThread().interrupt() throw e diff --git a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/TransportFailures.kt b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/TransportFailures.kt index 1c40782..19967ab 100644 --- a/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/TransportFailures.kt +++ b/sdk-transport-jdkhttp/src/main/kotlin/org/dexpace/sdk/transport/jdkhttp/internal/TransportFailures.kt @@ -11,6 +11,7 @@ import org.dexpace.sdk.core.http.response.exception.NetworkException import org.dexpace.sdk.core.util.Futures import java.io.IOException import java.io.InterruptedIOException +import java.net.SocketTimeoutException /** * Maps a JDK transport-level failure onto the SDK's [NetworkException] contract. @@ -21,6 +22,10 @@ import java.io.InterruptedIOException * strips those envelopes via [Futures.unwrap] (which is cycle-safe) and then classifies the root * cause: * + * - A read timeout ([SocketTimeoutException]) is wrapped in a [NetworkException] like any other + * transport failure. It is matched *before* the [InterruptedIOException] branch below (which it + * extends) because it is a timeout, not a cancellation — mirroring the sync path and the retry + * steps' carve-out, so both transports classify a timeout identically and it stays retryable. * - An interrupt-derived failure ([InterruptedIOException]) is returned untouched — as the * ORIGINAL [error] with any envelopes intact — so the caller keeps observing it as an * interrupt. @@ -32,6 +37,7 @@ import java.io.InterruptedIOException */ internal fun mapTransportFailure(error: Throwable): Throwable = when (val cause = Futures.unwrap(error)) { + is SocketTimeoutException -> NetworkException(cause.message, cause) is InterruptedIOException -> error is IOException -> NetworkException(cause.message, cause) else -> error From cf31290ade9c98fcbb7fd8cb41599368c9794e30 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 7 Jul 2026 13:34:46 +0300 Subject: [PATCH 3/3] refactor: de-duplicate retry, redaction, and content-type logic Consolidate copy-pasted logic that had drifted or could drift: - Single-source the idempotent HTTP method set as Method.IDEMPOTENT_METHODS; RetrySettings.DEFAULT_RETRYABLE_METHODS and RetryPolicySupport both derive from it instead of maintaining parallel literals. - Hoist the interrupt/timeout retry-classification predicate into RetryPolicySupport.isNonRetryableInterrupt, shared by the sync and async retry steps so they cannot classify cancellation differently. - Extract the URL-valued-header redaction into a single HeaderValueRedactor used by both the sync and async instrumentation steps. - Extract the OkHttp Content-Type resolution into resolveOkHttpContentType, shared by the two request-body adapters. - Drop the now-dead negative guard in RetryPolicySupport.effectiveMaxRetries, since the builder now rejects a negative per-call maxRetries. --- .../steps/DefaultAsyncInstrumentationStep.kt | 25 ++-------- .../pipeline/steps/DefaultAsyncRetryStep.kt | 5 +- .../steps/DefaultInstrumentationStep.kt | 27 ++--------- .../http/pipeline/steps/DefaultRetryStep.kt | 5 +- .../http/pipeline/steps/RetryPolicySupport.kt | 37 +++++++------- .../dexpace/sdk/core/http/request/Method.kt | 9 ++++ .../instrumentation/HeaderValueRedactor.kt | 48 +++++++++++++++++++ .../core/pipeline/step/retry/RetrySettings.kt | 4 +- .../pipeline/steps/InstrumentationStepTest.kt | 4 +- .../okhttp/internal/ContentTypeResolver.kt | 32 +++++++++++++ .../okhttp/internal/FileRequestBodyAdapter.kt | 8 +--- .../okhttp/internal/SdkRequestBodyAdapter.kt | 8 +--- 12 files changed, 127 insertions(+), 85 deletions(-) create mode 100644 sdk-core/src/main/kotlin/org/dexpace/sdk/core/instrumentation/HeaderValueRedactor.kt create mode 100644 sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/ContentTypeResolver.kt diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncInstrumentationStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncInstrumentationStep.kt index be913be..2b35a54 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncInstrumentationStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncInstrumentationStep.kt @@ -17,6 +17,7 @@ import org.dexpace.sdk.core.http.request.Request import org.dexpace.sdk.core.http.response.LoggableResponseBody import org.dexpace.sdk.core.http.response.Response import org.dexpace.sdk.core.instrumentation.ClientLogger +import org.dexpace.sdk.core.instrumentation.HeaderValueRedactor import org.dexpace.sdk.core.instrumentation.LoggingEvent import org.dexpace.sdk.core.instrumentation.MdcSnapshot import org.dexpace.sdk.core.instrumentation.Span @@ -365,7 +366,10 @@ public class DefaultAsyncInstrumentationStep val typed = HttpHeaderName.fromString(nameLower) when { options.allowedHeaderNames.contains(typed) -> - ev.field(prefix + nameLower, joinHeaderValues(typed, values)) + ev.field( + prefix + nameLower, + HeaderValueRedactor.render(typed, values, options.allowedQueryParamNames), + ) options.isRedactedHeaderNamesLoggingEnabled -> ev.field(prefix + nameLower, "REDACTED") // else: silently omit @@ -373,21 +377,6 @@ public class DefaultAsyncInstrumentationStep } } - private fun joinHeaderValues( - name: HttpHeaderName?, - values: List, - ): String { - // A URL-valued header (Location, Content-Location) can carry credentials in its query - // or fragment; redact it through the same UrlRedactor applied to url.full. - val rendered = - if (name in URL_VALUED_HEADERS) { - values.map { UrlRedactor.redactUrlValue(it, options.allowedQueryParamNames) } - } else { - values - } - return if (rendered.size == 1) rendered[0] else rendered.joinToString(", ") - } - private fun safeRedact(request: Request): String = try { UrlRedactor.redact(request.url, options.allowedQueryParamNames) @@ -451,9 +440,5 @@ public class DefaultAsyncInstrumentationStep // Nanoseconds in one millisecond, expressed as Double so the division returns // millisecond fractions (e.g. 1.234 ms) for high-resolution latency histograms. private const val NANOS_PER_MILLI_DOUBLE = 1_000_000.0 - - // Allowed headers whose value is a URL: redacted through UrlRedactor, not logged raw. - private val URL_VALUED_HEADERS: Set = - setOf(HttpHeaderName.LOCATION, HttpHeaderName.CONTENT_LOCATION) } } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncRetryStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncRetryStep.kt index 1e32f9d..63c8008 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncRetryStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncRetryStep.kt @@ -19,7 +19,6 @@ import org.dexpace.sdk.core.util.Clock import org.dexpace.sdk.core.util.Futures import java.io.IOException import java.io.InterruptedIOException -import java.net.SocketTimeoutException import java.time.Duration import java.util.concurrent.CompletableFuture import java.util.concurrent.ScheduledExecutorService @@ -293,9 +292,7 @@ public open class DefaultAsyncRetryStep // DefaultRetryStep. SocketTimeoutException extends InterruptedIOException but is a // retryable read timeout, not a cancellation, so it is excluded here and left to the // normal retry classification below. - if ((exception is InterruptedIOException && exception !is SocketTimeoutException) || - exception is InterruptedException - ) { + if (RetryPolicySupport.isNonRetryableInterrupt(exception)) { Thread.currentThread().interrupt() failTerminally(support.asInterruptedIo(exception)) return diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultInstrumentationStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultInstrumentationStep.kt index 4e38773..5735cad 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultInstrumentationStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultInstrumentationStep.kt @@ -15,6 +15,7 @@ import org.dexpace.sdk.core.http.request.Request import org.dexpace.sdk.core.http.response.LoggableResponseBody import org.dexpace.sdk.core.http.response.Response import org.dexpace.sdk.core.instrumentation.ClientLogger +import org.dexpace.sdk.core.instrumentation.HeaderValueRedactor import org.dexpace.sdk.core.instrumentation.LoggingEvent import org.dexpace.sdk.core.instrumentation.Span import org.dexpace.sdk.core.instrumentation.UrlRedactor @@ -269,7 +270,10 @@ public class DefaultInstrumentationStep val typed = HttpHeaderName.fromString(nameLower) when { options.allowedHeaderNames.contains(typed) -> - ev.field(prefix + nameLower, joinHeaderValues(typed, values)) + ev.field( + prefix + nameLower, + HeaderValueRedactor.render(typed, values, options.allowedQueryParamNames), + ) options.isRedactedHeaderNamesLoggingEnabled -> ev.field(prefix + nameLower, "REDACTED") // else: silently omit @@ -277,22 +281,6 @@ public class DefaultInstrumentationStep } } - private fun joinHeaderValues( - name: HttpHeaderName?, - values: List, - ): String { - // A URL-valued header (Location, Content-Location) can carry credentials in its query - // or fragment (OAuth code, pre-signed signature, implicit-flow token). Redact its value - // through the same UrlRedactor applied to url.full instead of logging it verbatim. - val rendered = - if (name in URL_VALUED_HEADERS) { - values.map { UrlRedactor.redactUrlValue(it, options.allowedQueryParamNames) } - } else { - values - } - return if (rendered.size == 1) rendered[0] else rendered.joinToString(", ") - } - private fun safeRedact(request: Request): String = try { UrlRedactor.redact(request.url, options.allowedQueryParamNames) @@ -356,10 +344,5 @@ public class DefaultInstrumentationStep // Nanoseconds in one millisecond, expressed as Double so the division returns // millisecond fractions (e.g. 1.234 ms) for high-resolution latency histograms. private const val NANOS_PER_MILLI_DOUBLE = 1_000_000.0 - - // Allowed headers whose value is a URL: their query/fragment can carry credentials, so - // they are redacted through UrlRedactor rather than logged verbatim. - private val URL_VALUED_HEADERS: Set = - setOf(HttpHeaderName.LOCATION, HttpHeaderName.CONTENT_LOCATION) } } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRetryStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRetryStep.kt index da99b39..2ec1170 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRetryStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRetryStep.kt @@ -18,7 +18,6 @@ import org.dexpace.sdk.core.pipeline.step.retry.RetrySettings import org.dexpace.sdk.core.util.Clock import java.io.IOException import java.io.InterruptedIOException -import java.net.SocketTimeoutException import java.time.Duration /** @@ -229,9 +228,7 @@ public open class DefaultRetryStep // the @Throws(IOException) contract holds, with prior failures attached. // SocketTimeoutException extends InterruptedIOException but is a read timeout, not a // cancellation, so it is excluded here and left to the normal retry classification. - if ((exception is InterruptedIOException && exception !is SocketTimeoutException) || - exception is InterruptedException - ) { + if (RetryPolicySupport.isNonRetryableInterrupt(exception)) { Thread.currentThread().interrupt() val ioe = support.asInterruptedIo(exception) suppressed?.forEach(ioe::addSuppressed) diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryPolicySupport.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryPolicySupport.kt index cc7c3ce..40c9131 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryPolicySupport.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryPolicySupport.kt @@ -7,13 +7,14 @@ package org.dexpace.sdk.core.http.pipeline.steps -import org.dexpace.sdk.core.http.request.Method +import org.dexpace.sdk.core.http.request.IDEMPOTENT_METHODS import org.dexpace.sdk.core.http.request.Request import org.dexpace.sdk.core.http.request.RequestOptions import org.dexpace.sdk.core.instrumentation.ClientLogger import org.dexpace.sdk.core.pipeline.step.retry.BackoffCalculator import org.dexpace.sdk.core.pipeline.step.retry.RetrySettings import java.io.InterruptedIOException +import java.net.SocketTimeoutException import java.time.Duration /** @@ -50,18 +51,14 @@ internal class RetryPolicySupport( .build() /** - * Resolves the retry budget for a single call from its [RequestOptions]. A per-call - * [RequestOptions.maxRetries] override wins only when it is non-negative; a `null` (no - * override) or a negative override falls back to the configured, already-clamped - * [options]`.maxRetries`. - * - * The negative-override fallback is deliberate: it mirrors [clampOptions]' handling of a - * negative *configured* `maxRetries`, so a negative per-call value means "use the configured - * default", NOT "0 retries" (which is what `maxRetries = 0` requests). Callers should read this - * once per call — the options are constant across retry re-drives. + * Resolves the retry budget for a single call from its [RequestOptions]. A non-`null` per-call + * [RequestOptions.maxRetries] override wins — the builder rejects a negative value, so a + * present override is always `>= 0` here — while a `null` override falls back to the configured, + * already-clamped [options]`.maxRetries`. `0` is a real value (one attempt, no retries), not + * "unset". Callers should read this once per call — the options are constant across retry + * re-drives. */ - fun effectiveMaxRetries(callOptions: RequestOptions): Int = - callOptions.maxRetries?.takeIf { it >= 0 } ?: options.maxRetries + fun effectiveMaxRetries(callOptions: RequestOptions): Int = callOptions.maxRetries ?: options.maxRetries /** * Returns `true` when [request] may be re-sent: a body-less request only when its method is @@ -127,10 +124,16 @@ internal class RetryPolicySupport( return opts.withMaxRetries(DefaultRetryStep.DEFAULT_MAX_RETRIES) } - private companion object { - // Methods safe to re-send regardless of body replayability (idempotent per RFC 9110). - // Mirrors RetrySettings.DEFAULT_RETRYABLE_METHODS. - private val IDEMPOTENT_METHODS: Set = - setOf(Method.GET, Method.HEAD, Method.OPTIONS, Method.PUT, Method.DELETE) + internal companion object { + /** + * Classifies [exception] as a non-retryable interrupt / cancellation, per the SDK-wide + * convention: an [InterruptedIOException] other than a [SocketTimeoutException] (a read + * timeout, which stays retryable), or a bare [InterruptedException]. Shared by + * [DefaultRetryStep] and [DefaultAsyncRetryStep] so the blocking and async stacks classify + * cancellation identically. + */ + internal fun isNonRetryableInterrupt(exception: Throwable): Boolean = + (exception is InterruptedIOException && exception !is SocketTimeoutException) || + exception is InterruptedException } } 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 8120e4b..ccf8f90 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 @@ -47,3 +47,12 @@ public enum class Method(public val permitsRequestBody: Boolean) { @Suppress("MemberNameEqualsClassName") public val method: String get() = name } + +/** + * HTTP methods that are idempotent per RFC 9110 §9.2.2 and therefore safe to replay without a + * replayable request body. Single canonical source both retry defaults derive from: + * `RetrySettings.DEFAULT_RETRYABLE_METHODS` (the configurable retry allow-list) and the inherent + * replay-safety gate in `RetryPolicySupport`. `linkedSetOf` fixes a stable iteration order. + */ +internal val IDEMPOTENT_METHODS: Set = + linkedSetOf(Method.GET, Method.HEAD, Method.OPTIONS, Method.PUT, Method.DELETE) diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/instrumentation/HeaderValueRedactor.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/instrumentation/HeaderValueRedactor.kt new file mode 100644 index 0000000..98d4847 --- /dev/null +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/instrumentation/HeaderValueRedactor.kt @@ -0,0 +1,48 @@ +/* + * Copyright (c) 2026 dexpace and Omar Aljarrah + * + * Licensed under the MIT License. See LICENSE in the project root. + * SPDX-License-Identifier: MIT + */ + +package org.dexpace.sdk.core.instrumentation + +import org.dexpace.sdk.core.http.common.HttpHeaderName + +/** + * Renders a header's values for structured logging, redacting URL-valued headers. + * + * Shared by the sync and async instrumentation steps so the redaction policy lives in one place + * — a change here (or a new URL-valued header) applies to both without drift, which is what keeps + * the credential-leak this closes from re-opening. + */ +internal object HeaderValueRedactor { + /** + * Allowed headers whose value is a URL: their query/fragment can carry credentials (an OAuth + * code, a pre-signed signature, an implicit-flow token), so they are redacted through + * [UrlRedactor] rather than logged verbatim. Kept to the plain-URL-valued headers only — + * headers like `Link`/`Refresh` have non-plain-URL syntax [UrlRedactor.redactUrlValue] is not + * built for. + */ + internal val URL_VALUED_HEADERS: Set = + setOf(HttpHeaderName.LOCATION, HttpHeaderName.CONTENT_LOCATION) + + /** + * Joins [values] into a single loggable string. When [name] is a URL-valued header, each value + * is redacted through [UrlRedactor.redactUrlValue] (using [allowedQueryParams] to gate which + * query parameters survive); otherwise values pass through verbatim. + */ + internal fun render( + name: HttpHeaderName?, + values: List, + allowedQueryParams: Set, + ): String { + val rendered = + if (name in URL_VALUED_HEADERS) { + values.map { UrlRedactor.redactUrlValue(it, allowedQueryParams) } + } else { + values + } + return if (rendered.size == 1) rendered[0] else rendered.joinToString(", ") + } +} diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetrySettings.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetrySettings.kt index 3847848..29a9ac5 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetrySettings.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetrySettings.kt @@ -9,6 +9,7 @@ package org.dexpace.sdk.core.pipeline.step.retry import org.dexpace.sdk.core.generics.Builder import org.dexpace.sdk.core.http.common.HttpHeaderName +import org.dexpace.sdk.core.http.request.IDEMPOTENT_METHODS import org.dexpace.sdk.core.http.request.Method import java.time.Duration import java.util.concurrent.ScheduledExecutorService @@ -304,8 +305,7 @@ public class RetrySettings * is replayable (the orthogonal axis checked by [RetryRecovery.canRetry]). */ @JvmField - public val DEFAULT_RETRYABLE_METHODS: Set = - linkedSetOf(Method.GET, Method.HEAD, Method.OPTIONS, Method.PUT, Method.DELETE) + public val DEFAULT_RETRYABLE_METHODS: Set = IDEMPOTENT_METHODS // Spelled-out status constants to satisfy detekt's MagicNumber rule. private const val SC_REQUEST_TIMEOUT = 408 diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/InstrumentationStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/InstrumentationStepTest.kt index 82eca05..e946016 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/InstrumentationStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/InstrumentationStepTest.kt @@ -367,8 +367,8 @@ class InstrumentationStepTest { @Test fun `multi-valued allowed header is joined by comma in the log event`() { - // Branch coverage on joinHeaderValues: a header with more than one value must take - // the joinToString(", ") path. The default allow-list contains Via — feed in two + // Branch coverage on HeaderValueRedactor.render: a header with more than one value must + // take the joinToString(", ") path. The default allow-list contains Via — feed in two // values to drive that path. val fake = FakeHttpClient().enqueue { status(200) } val pipeline = diff --git a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/ContentTypeResolver.kt b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/ContentTypeResolver.kt new file mode 100644 index 0000000..6caf56d --- /dev/null +++ b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/ContentTypeResolver.kt @@ -0,0 +1,32 @@ +/* + * Copyright (c) 2026 dexpace and Omar Aljarrah + * + * Licensed under the MIT License. See LICENSE in the project root. + * SPDX-License-Identifier: MIT + */ + +package org.dexpace.sdk.transport.okhttp.internal + +import okhttp3.MediaType.Companion.toMediaTypeOrNull +import org.dexpace.sdk.core.http.common.MediaType + +/** + * Resolves the `okhttp3.MediaType` reported by a request-body adapter's `contentType()`. + * + * The caller's explicit request `Content-Type` header ([explicitContentType]) wins when one was + * set; otherwise the SDK body's [MediaType] ([sdkMediaType]) is used. Either way the value is + * parsed via `toString()` → `okhttp3.MediaType`, and an unparseable string returns `null`. + * Reporting the explicit header here is what keeps it authoritative: OkHttp's `BridgeInterceptor` + * overwrites the request's `Content-Type` with `body.contentType()` when the latter is non-null, + * so a `null` return (no explicit header AND no body media type, or an unparseable explicit header) + * leaves the caller's own header untouched. + */ +internal fun resolveOkHttpContentType( + explicitContentType: String?, + sdkMediaType: MediaType?, +): okhttp3.MediaType? = + if (explicitContentType != null) { + explicitContentType.toMediaTypeOrNull() + } else { + sdkMediaType?.toString()?.toMediaTypeOrNull() + } diff --git a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/FileRequestBodyAdapter.kt b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/FileRequestBodyAdapter.kt index 6ac6cc4..6c3b272 100644 --- a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/FileRequestBodyAdapter.kt +++ b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/FileRequestBodyAdapter.kt @@ -7,7 +7,6 @@ package org.dexpace.sdk.transport.okhttp.internal -import okhttp3.MediaType.Companion.toMediaTypeOrNull import okhttp3.RequestBody import okio.BufferedSink import okio.FileSystem @@ -40,12 +39,7 @@ internal class FileRequestBodyAdapter( private val sdkBody: FileRequestBody, private val explicitContentType: String? = null, ) : RequestBody() { - override fun contentType(): okhttp3.MediaType? = - if (explicitContentType != null) { - explicitContentType.toMediaTypeOrNull() - } else { - sdkBody.mediaType()?.toString()?.toMediaTypeOrNull() - } + override fun contentType(): okhttp3.MediaType? = resolveOkHttpContentType(explicitContentType, sdkBody.mediaType()) override fun contentLength(): Long = sdkBody.count diff --git a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/SdkRequestBodyAdapter.kt b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/SdkRequestBodyAdapter.kt index e7d34fa..3fd8f86 100644 --- a/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/SdkRequestBodyAdapter.kt +++ b/sdk-transport-okhttp/src/main/kotlin/org/dexpace/sdk/transport/okhttp/internal/SdkRequestBodyAdapter.kt @@ -7,7 +7,6 @@ package org.dexpace.sdk.transport.okhttp.internal -import okhttp3.MediaType.Companion.toMediaTypeOrNull import okhttp3.RequestBody import okio.BufferedSink import org.dexpace.sdk.core.io.Io @@ -44,12 +43,7 @@ internal class SdkRequestBodyAdapter( private val sdkBody: SdkRequestBody, private val explicitContentType: String? = null, ) : RequestBody() { - override fun contentType(): okhttp3.MediaType? = - if (explicitContentType != null) { - explicitContentType.toMediaTypeOrNull() - } else { - sdkBody.mediaType()?.toString()?.toMediaTypeOrNull() - } + override fun contentType(): okhttp3.MediaType? = resolveOkHttpContentType(explicitContentType, sdkBody.mediaType()) override fun contentLength(): Long = sdkBody.contentLength()