Skip to content

Commit cf31290

Browse files
OmarAlJarrahOmarAlJarrah
committed
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.
1 parent ddbbdd7 commit cf31290

12 files changed

Lines changed: 127 additions & 85 deletions

File tree

‎sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncInstrumentationStep.kt‎

Lines changed: 5 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import org.dexpace.sdk.core.http.request.Request
1717
import org.dexpace.sdk.core.http.response.LoggableResponseBody
1818
import org.dexpace.sdk.core.http.response.Response
1919
import org.dexpace.sdk.core.instrumentation.ClientLogger
20+
import org.dexpace.sdk.core.instrumentation.HeaderValueRedactor
2021
import org.dexpace.sdk.core.instrumentation.LoggingEvent
2122
import org.dexpace.sdk.core.instrumentation.MdcSnapshot
2223
import org.dexpace.sdk.core.instrumentation.Span
@@ -365,29 +366,17 @@ public class DefaultAsyncInstrumentationStep
365366
val typed = HttpHeaderName.fromString(nameLower)
366367
when {
367368
options.allowedHeaderNames.contains(typed) ->
368-
ev.field(prefix + nameLower, joinHeaderValues(typed, values))
369+
ev.field(
370+
prefix + nameLower,
371+
HeaderValueRedactor.render(typed, values, options.allowedQueryParamNames),
372+
)
369373
options.isRedactedHeaderNamesLoggingEnabled ->
370374
ev.field(prefix + nameLower, "REDACTED")
371375
// else: silently omit
372376
}
373377
}
374378
}
375379

376-
private fun joinHeaderValues(
377-
name: HttpHeaderName?,
378-
values: List<String>,
379-
): String {
380-
// A URL-valued header (Location, Content-Location) can carry credentials in its query
381-
// or fragment; redact it through the same UrlRedactor applied to url.full.
382-
val rendered =
383-
if (name in URL_VALUED_HEADERS) {
384-
values.map { UrlRedactor.redactUrlValue(it, options.allowedQueryParamNames) }
385-
} else {
386-
values
387-
}
388-
return if (rendered.size == 1) rendered[0] else rendered.joinToString(", ")
389-
}
390-
391380
private fun safeRedact(request: Request): String =
392381
try {
393382
UrlRedactor.redact(request.url, options.allowedQueryParamNames)
@@ -451,9 +440,5 @@ public class DefaultAsyncInstrumentationStep
451440
// Nanoseconds in one millisecond, expressed as Double so the division returns
452441
// millisecond fractions (e.g. 1.234 ms) for high-resolution latency histograms.
453442
private const val NANOS_PER_MILLI_DOUBLE = 1_000_000.0
454-
455-
// Allowed headers whose value is a URL: redacted through UrlRedactor, not logged raw.
456-
private val URL_VALUED_HEADERS: Set<HttpHeaderName> =
457-
setOf(HttpHeaderName.LOCATION, HttpHeaderName.CONTENT_LOCATION)
458443
}
459444
}

‎sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncRetryStep.kt‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import org.dexpace.sdk.core.util.Clock
1919
import org.dexpace.sdk.core.util.Futures
2020
import java.io.IOException
2121
import java.io.InterruptedIOException
22-
import java.net.SocketTimeoutException
2322
import java.time.Duration
2423
import java.util.concurrent.CompletableFuture
2524
import java.util.concurrent.ScheduledExecutorService
@@ -293,9 +292,7 @@ public open class DefaultAsyncRetryStep
293292
// DefaultRetryStep. SocketTimeoutException extends InterruptedIOException but is a
294293
// retryable read timeout, not a cancellation, so it is excluded here and left to the
295294
// normal retry classification below.
296-
if ((exception is InterruptedIOException && exception !is SocketTimeoutException) ||
297-
exception is InterruptedException
298-
) {
295+
if (RetryPolicySupport.isNonRetryableInterrupt(exception)) {
299296
Thread.currentThread().interrupt()
300297
failTerminally(support.asInterruptedIo(exception))
301298
return

‎sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultInstrumentationStep.kt‎

Lines changed: 5 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import org.dexpace.sdk.core.http.request.Request
1515
import org.dexpace.sdk.core.http.response.LoggableResponseBody
1616
import org.dexpace.sdk.core.http.response.Response
1717
import org.dexpace.sdk.core.instrumentation.ClientLogger
18+
import org.dexpace.sdk.core.instrumentation.HeaderValueRedactor
1819
import org.dexpace.sdk.core.instrumentation.LoggingEvent
1920
import org.dexpace.sdk.core.instrumentation.Span
2021
import org.dexpace.sdk.core.instrumentation.UrlRedactor
@@ -269,30 +270,17 @@ public class DefaultInstrumentationStep
269270
val typed = HttpHeaderName.fromString(nameLower)
270271
when {
271272
options.allowedHeaderNames.contains(typed) ->
272-
ev.field(prefix + nameLower, joinHeaderValues(typed, values))
273+
ev.field(
274+
prefix + nameLower,
275+
HeaderValueRedactor.render(typed, values, options.allowedQueryParamNames),
276+
)
273277
options.isRedactedHeaderNamesLoggingEnabled ->
274278
ev.field(prefix + nameLower, "REDACTED")
275279
// else: silently omit
276280
}
277281
}
278282
}
279283

280-
private fun joinHeaderValues(
281-
name: HttpHeaderName?,
282-
values: List<String>,
283-
): String {
284-
// A URL-valued header (Location, Content-Location) can carry credentials in its query
285-
// or fragment (OAuth code, pre-signed signature, implicit-flow token). Redact its value
286-
// through the same UrlRedactor applied to url.full instead of logging it verbatim.
287-
val rendered =
288-
if (name in URL_VALUED_HEADERS) {
289-
values.map { UrlRedactor.redactUrlValue(it, options.allowedQueryParamNames) }
290-
} else {
291-
values
292-
}
293-
return if (rendered.size == 1) rendered[0] else rendered.joinToString(", ")
294-
}
295-
296284
private fun safeRedact(request: Request): String =
297285
try {
298286
UrlRedactor.redact(request.url, options.allowedQueryParamNames)
@@ -356,10 +344,5 @@ public class DefaultInstrumentationStep
356344
// Nanoseconds in one millisecond, expressed as Double so the division returns
357345
// millisecond fractions (e.g. 1.234 ms) for high-resolution latency histograms.
358346
private const val NANOS_PER_MILLI_DOUBLE = 1_000_000.0
359-
360-
// Allowed headers whose value is a URL: their query/fragment can carry credentials, so
361-
// they are redacted through UrlRedactor rather than logged verbatim.
362-
private val URL_VALUED_HEADERS: Set<HttpHeaderName> =
363-
setOf(HttpHeaderName.LOCATION, HttpHeaderName.CONTENT_LOCATION)
364347
}
365348
}

‎sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/DefaultRetryStep.kt‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import org.dexpace.sdk.core.pipeline.step.retry.RetrySettings
1818
import org.dexpace.sdk.core.util.Clock
1919
import java.io.IOException
2020
import java.io.InterruptedIOException
21-
import java.net.SocketTimeoutException
2221
import java.time.Duration
2322

2423
/**
@@ -229,9 +228,7 @@ public open class DefaultRetryStep
229228
// the @Throws(IOException) contract holds, with prior failures attached.
230229
// SocketTimeoutException extends InterruptedIOException but is a read timeout, not a
231230
// cancellation, so it is excluded here and left to the normal retry classification.
232-
if ((exception is InterruptedIOException && exception !is SocketTimeoutException) ||
233-
exception is InterruptedException
234-
) {
231+
if (RetryPolicySupport.isNonRetryableInterrupt(exception)) {
235232
Thread.currentThread().interrupt()
236233
val ioe = support.asInterruptedIo(exception)
237234
suppressed?.forEach(ioe::addSuppressed)

‎sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/RetryPolicySupport.kt‎

Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,14 @@
77

88
package org.dexpace.sdk.core.http.pipeline.steps
99

10-
import org.dexpace.sdk.core.http.request.Method
10+
import org.dexpace.sdk.core.http.request.IDEMPOTENT_METHODS
1111
import org.dexpace.sdk.core.http.request.Request
1212
import org.dexpace.sdk.core.http.request.RequestOptions
1313
import org.dexpace.sdk.core.instrumentation.ClientLogger
1414
import org.dexpace.sdk.core.pipeline.step.retry.BackoffCalculator
1515
import org.dexpace.sdk.core.pipeline.step.retry.RetrySettings
1616
import java.io.InterruptedIOException
17+
import java.net.SocketTimeoutException
1718
import java.time.Duration
1819

1920
/**
@@ -50,18 +51,14 @@ internal class RetryPolicySupport(
5051
.build()
5152

5253
/**
53-
* Resolves the retry budget for a single call from its [RequestOptions]. A per-call
54-
* [RequestOptions.maxRetries] override wins only when it is non-negative; a `null` (no
55-
* override) or a negative override falls back to the configured, already-clamped
56-
* [options]`.maxRetries`.
57-
*
58-
* The negative-override fallback is deliberate: it mirrors [clampOptions]' handling of a
59-
* negative *configured* `maxRetries`, so a negative per-call value means "use the configured
60-
* default", NOT "0 retries" (which is what `maxRetries = 0` requests). Callers should read this
61-
* once per call — the options are constant across retry re-drives.
54+
* Resolves the retry budget for a single call from its [RequestOptions]. A non-`null` per-call
55+
* [RequestOptions.maxRetries] override wins — the builder rejects a negative value, so a
56+
* present override is always `>= 0` here — while a `null` override falls back to the configured,
57+
* already-clamped [options]`.maxRetries`. `0` is a real value (one attempt, no retries), not
58+
* "unset". Callers should read this once per call — the options are constant across retry
59+
* re-drives.
6260
*/
63-
fun effectiveMaxRetries(callOptions: RequestOptions): Int =
64-
callOptions.maxRetries?.takeIf { it >= 0 } ?: options.maxRetries
61+
fun effectiveMaxRetries(callOptions: RequestOptions): Int = callOptions.maxRetries ?: options.maxRetries
6562

6663
/**
6764
* Returns `true` when [request] may be re-sent: a body-less request only when its method is
@@ -127,10 +124,16 @@ internal class RetryPolicySupport(
127124
return opts.withMaxRetries(DefaultRetryStep.DEFAULT_MAX_RETRIES)
128125
}
129126

130-
private companion object {
131-
// Methods safe to re-send regardless of body replayability (idempotent per RFC 9110).
132-
// Mirrors RetrySettings.DEFAULT_RETRYABLE_METHODS.
133-
private val IDEMPOTENT_METHODS: Set<Method> =
134-
setOf(Method.GET, Method.HEAD, Method.OPTIONS, Method.PUT, Method.DELETE)
127+
internal companion object {
128+
/**
129+
* Classifies [exception] as a non-retryable interrupt / cancellation, per the SDK-wide
130+
* convention: an [InterruptedIOException] other than a [SocketTimeoutException] (a read
131+
* timeout, which stays retryable), or a bare [InterruptedException]. Shared by
132+
* [DefaultRetryStep] and [DefaultAsyncRetryStep] so the blocking and async stacks classify
133+
* cancellation identically.
134+
*/
135+
internal fun isNonRetryableInterrupt(exception: Throwable): Boolean =
136+
(exception is InterruptedIOException && exception !is SocketTimeoutException) ||
137+
exception is InterruptedException
135138
}
136139
}

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,3 +47,12 @@ public enum class Method(public val permitsRequestBody: Boolean) {
4747
@Suppress("MemberNameEqualsClassName")
4848
public val method: String get() = name
4949
}
50+
51+
/**
52+
* HTTP methods that are idempotent per RFC 9110 §9.2.2 and therefore safe to replay without a
53+
* replayable request body. Single canonical source both retry defaults derive from:
54+
* `RetrySettings.DEFAULT_RETRYABLE_METHODS` (the configurable retry allow-list) and the inherent
55+
* replay-safety gate in `RetryPolicySupport`. `linkedSetOf` fixes a stable iteration order.
56+
*/
57+
internal val IDEMPOTENT_METHODS: Set<Method> =
58+
linkedSetOf(Method.GET, Method.HEAD, Method.OPTIONS, Method.PUT, Method.DELETE)

‎sdk-core/src/main/kotlin/org/dexpace/sdk/core/instrumentation/HeaderValueRedactor.kt‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
/*
2+
* Copyright (c) 2026 dexpace and Omar Aljarrah
3+
*
4+
* Licensed under the MIT License. See LICENSE in the project root.
5+
* SPDX-License-Identifier: MIT
6+
*/
7+
8+
package org.dexpace.sdk.core.instrumentation
9+
10+
import org.dexpace.sdk.core.http.common.HttpHeaderName
11+
12+
/**
13+
* Renders a header's values for structured logging, redacting URL-valued headers.
14+
*
15+
* Shared by the sync and async instrumentation steps so the redaction policy lives in one place
16+
* — a change here (or a new URL-valued header) applies to both without drift, which is what keeps
17+
* the credential-leak this closes from re-opening.
18+
*/
19+
internal object HeaderValueRedactor {
20+
/**
21+
* Allowed headers whose value is a URL: their query/fragment can carry credentials (an OAuth
22+
* code, a pre-signed signature, an implicit-flow token), so they are redacted through
23+
* [UrlRedactor] rather than logged verbatim. Kept to the plain-URL-valued headers only —
24+
* headers like `Link`/`Refresh` have non-plain-URL syntax [UrlRedactor.redactUrlValue] is not
25+
* built for.
26+
*/
27+
internal val URL_VALUED_HEADERS: Set<HttpHeaderName> =
28+
setOf(HttpHeaderName.LOCATION, HttpHeaderName.CONTENT_LOCATION)
29+
30+
/**
31+
* Joins [values] into a single loggable string. When [name] is a URL-valued header, each value
32+
* is redacted through [UrlRedactor.redactUrlValue] (using [allowedQueryParams] to gate which
33+
* query parameters survive); otherwise values pass through verbatim.
34+
*/
35+
internal fun render(
36+
name: HttpHeaderName?,
37+
values: List<String>,
38+
allowedQueryParams: Set<String>,
39+
): String {
40+
val rendered =
41+
if (name in URL_VALUED_HEADERS) {
42+
values.map { UrlRedactor.redactUrlValue(it, allowedQueryParams) }
43+
} else {
44+
values
45+
}
46+
return if (rendered.size == 1) rendered[0] else rendered.joinToString(", ")
47+
}
48+
}

‎sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetrySettings.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ package org.dexpace.sdk.core.pipeline.step.retry
99

1010
import org.dexpace.sdk.core.generics.Builder
1111
import org.dexpace.sdk.core.http.common.HttpHeaderName
12+
import org.dexpace.sdk.core.http.request.IDEMPOTENT_METHODS
1213
import org.dexpace.sdk.core.http.request.Method
1314
import java.time.Duration
1415
import java.util.concurrent.ScheduledExecutorService
@@ -304,8 +305,7 @@ public class RetrySettings
304305
* is replayable (the orthogonal axis checked by [RetryRecovery.canRetry]).
305306
*/
306307
@JvmField
307-
public val DEFAULT_RETRYABLE_METHODS: Set<Method> =
308-
linkedSetOf(Method.GET, Method.HEAD, Method.OPTIONS, Method.PUT, Method.DELETE)
308+
public val DEFAULT_RETRYABLE_METHODS: Set<Method> = IDEMPOTENT_METHODS
309309

310310
// Spelled-out status constants to satisfy detekt's MagicNumber rule.
311311
private const val SC_REQUEST_TIMEOUT = 408

‎sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/InstrumentationStepTest.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -367,8 +367,8 @@ class InstrumentationStepTest {
367367

368368
@Test
369369
fun `multi-valued allowed header is joined by comma in the log event`() {
370-
// Branch coverage on joinHeaderValues: a header with more than one value must take
371-
// the joinToString(", ") path. The default allow-list contains Via — feed in two
370+
// Branch coverage on HeaderValueRedactor.render: a header with more than one value must
371+
// take the joinToString(", ") path. The default allow-list contains Via — feed in two
372372
// values to drive that path.
373373
val fake = FakeHttpClient().enqueue { status(200) }
374374
val pipeline =

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

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
/*
2+
* Copyright (c) 2026 dexpace and Omar Aljarrah
3+
*
4+
* Licensed under the MIT License. See LICENSE in the project root.
5+
* SPDX-License-Identifier: MIT
6+
*/
7+
8+
package org.dexpace.sdk.transport.okhttp.internal
9+
10+
import okhttp3.MediaType.Companion.toMediaTypeOrNull
11+
import org.dexpace.sdk.core.http.common.MediaType
12+
13+
/**
14+
* Resolves the `okhttp3.MediaType` reported by a request-body adapter's `contentType()`.
15+
*
16+
* The caller's explicit request `Content-Type` header ([explicitContentType]) wins when one was
17+
* set; otherwise the SDK body's [MediaType] ([sdkMediaType]) is used. Either way the value is
18+
* parsed via `toString()` → `okhttp3.MediaType`, and an unparseable string returns `null`.
19+
* Reporting the explicit header here is what keeps it authoritative: OkHttp's `BridgeInterceptor`
20+
* overwrites the request's `Content-Type` with `body.contentType()` when the latter is non-null,
21+
* so a `null` return (no explicit header AND no body media type, or an unparseable explicit header)
22+
* leaves the caller's own header untouched.
23+
*/
24+
internal fun resolveOkHttpContentType(
25+
explicitContentType: String?,
26+
sdkMediaType: MediaType?,
27+
): okhttp3.MediaType? =
28+
if (explicitContentType != null) {
29+
explicitContentType.toMediaTypeOrNull()
30+
} else {
31+
sdkMediaType?.toString()?.toMediaTypeOrNull()
32+
}

0 commit comments

Comments
 (0)