Skip to content

Commit ada301b

Browse files
committed
test: lock in the review-fix behaviors with regression tests
Cover the trickiest corrected behaviors so a future change cannot silently reintroduce them: RetryRecovery re-classifying re-sent responses (503,503,200 reaches 200; exhaustion yields the mapped exception; a configured retryable status widens the set); TypeRef rejecting a captured type variable; the Response status-range predicates; charset-aware ResponseBody.string(); the replayable throwOnError error body; SocketTimeoutException being retried rather than treated as cancellation; no spurious mixed-provider warning; atomic StagedSteps.reload and the flattening HttpPipelineBuilder(pipeline) constructor; the auth-challenge non-replayable-body path; Jackson rejecting a literal-null body and not closing a caller stream; and UrlRedactor.redactUrlValue.
1 parent cada299 commit ada301b

12 files changed

Lines changed: 490 additions & 0 deletions

File tree

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

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -750,6 +750,51 @@ class HttpPipelineTest {
750750
assertEquals(2, copy.steps.size)
751751
}
752752

753+
@Test
754+
fun `surgical insert that would duplicate a pillar throws and leaves the builder intact`() {
755+
// insertAfter re-buckets by reloading the flattened step list. A second distinct RETRY-stage
756+
// step would create a duplicate RETRY pillar; the reload is atomic, so the collision throws
757+
// and the builder's prior steps survive — build() still yields exactly the original pillar.
758+
val client = RecordingHttpClient()
759+
val originalRetry = DefaultRetryStep()
760+
val builder = HttpPipelineBuilder(client).append(originalRetry)
761+
762+
assertFailsWith<IllegalStateException> {
763+
builder.insertAfter<DefaultRetryStep>(DefaultRetryStep())
764+
}
765+
766+
val pipeline = builder.build()
767+
assertEquals(1, pipeline.steps.size, "the rejected insert must not leave a duplicate pillar behind")
768+
assertSame(originalRetry, pipeline.steps[0], "the original retry pillar must remain intact")
769+
}
770+
771+
@Test
772+
fun `HttpPipelineBuilder of an HttpPipeline flattens rather than nesting`() {
773+
// A statically-typed HttpPipeline argument resolves to the flattening constructor: the
774+
// source pipeline's steps are copied into the new builder (same count/order as from()), NOT
775+
// nested as an opaque transport — which would leave the outer builder with zero steps.
776+
val client = RecordingHttpClient()
777+
val order = mutableListOf<String>()
778+
val retryStep = TaggingStep(Stage.RETRY, "retry", order)
779+
val preAuthStep = TaggingStep(Stage.PRE_AUTH, "pre-auth", order)
780+
val base = HttpPipelineBuilder(client).append(retryStep).append(preAuthStep).build()
781+
782+
val viaConstructor = HttpPipelineBuilder(base).build()
783+
val viaFrom = HttpPipelineBuilder.from(base).build()
784+
785+
// Same step count/order as from() — proving flatten, not nest.
786+
assertEquals(viaFrom.steps.size, viaConstructor.steps.size)
787+
assertEquals(2, viaConstructor.steps.size)
788+
assertSame(retryStep, viaConstructor.steps[0])
789+
assertSame(preAuthStep, viaConstructor.steps[1])
790+
// The transport is the base's transport, not the base pipeline nested as a transport.
791+
assertSame(client, viaConstructor.httpClient)
792+
793+
// Both steps actually run when sent (a nested pipeline would run zero outer steps).
794+
viaConstructor.send(request())
795+
assertEquals(listOf("retry", "pre-auth"), order)
796+
}
797+
753798
@Test
754799
fun `appendAll with an empty list is a no-op`() {
755800
val client = RecordingHttpClient()

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,23 @@ class StagedStepsTest {
8787
assertTrue(staged.pillarAt(Stage.PRE_AUTH) == null)
8888
}
8989

90+
@Test
91+
fun `reload that hits a pillar collision leaves prior state intact`() {
92+
// reload commits atomically: it assembles the new state into scratch collections and only
93+
// reassigns the live storage once the whole rebuild succeeds. A mid-rebuild pillar collision
94+
// must therefore throw with the pre-reload state completely untouched — never half-reloaded.
95+
val staged = stagedSteps()
96+
staged.append(FakeStep(Stage.RETRY, "retry"))
97+
staged.append(FakeStep(Stage.PRE_AUTH, "pre"))
98+
99+
assertFailsWith<IllegalStateException> {
100+
staged.reload(listOf(FakeStep(Stage.RETRY, "retry-A"), FakeStep(Stage.RETRY, "retry-B")))
101+
}
102+
103+
assertEquals("retry", staged.pillarAt(Stage.RETRY)?.tag, "the original pillar must survive the failed reload")
104+
assertEquals(listOf("pre"), staged.nonPillarAt(Stage.PRE_AUTH).map { it.tag }, "non-pillar steps must survive")
105+
}
106+
90107
// -------- append / prepend pillar routing --------
91108

92109
@Test

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

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -589,6 +589,34 @@ class AuthStepTest {
589589
assertEquals("refreshed-key", fake.requests[1].headers.get(HttpHeaderName.AUTHORIZATION))
590590
}
591591

592+
@Test
593+
fun `401 challenge whose replacement carries a non-replayable body surfaces the real 401`() {
594+
// The challenge handler returns a replacement request whose body cannot be replayed. Driving
595+
// it through the chain would trip the body's consume-once guard and surface an
596+
// IOException("HTTP pipeline failure") that masks the real 401. The step must instead skip
597+
// the replay (like the retry/redirect steps' replayability gate) and return the original 401.
598+
val step =
599+
object : KeyCredentialAuthStep(KeyCredential("k")) {
600+
override fun authorizeRequestOnChallenge(
601+
request: Request,
602+
response: Response,
603+
): Request =
604+
request.newBuilder()
605+
.method(Method.POST)
606+
.body(NonReplayableRequestBody())
607+
.build()
608+
}
609+
val fake =
610+
FakeHttpClient()
611+
.enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") }
612+
613+
val pipeline = HttpPipelineBuilder(fake).append(step).build()
614+
val response = pipeline.send(getHttpsRequest())
615+
616+
assertEquals(401, response.status.code, "the real 401 must surface, not a masked IOException")
617+
assertEquals(1, fake.callCount, "a non-replayable replacement body must skip the challenge replay")
618+
}
619+
592620
@Test
593621
fun `challenge retry closes the 401 response before driving the retry`() {
594622
val closes = AtomicInteger(0)
@@ -976,6 +1004,20 @@ class AuthStepTest {
9761004
.url("https://api.example.com/x")
9771005
.build()
9781006

1007+
/** Single-use request body: isReplayable() is false and writeTo streams fixed bytes. */
1008+
private class NonReplayableRequestBody : org.dexpace.sdk.core.http.request.RequestBody() {
1009+
override fun mediaType(): org.dexpace.sdk.core.http.common.MediaType? =
1010+
org.dexpace.sdk.core.http.common.MediaType.parse("text/plain")
1011+
1012+
override fun contentLength(): Long = 5
1013+
1014+
override fun isReplayable(): Boolean = false
1015+
1016+
override fun writeTo(sink: org.dexpace.sdk.core.io.BufferedSink) {
1017+
sink.write("hello".toByteArray(Charsets.UTF_8))
1018+
}
1019+
}
1020+
9791021
/** ResponseBody that increments [closes] on close() and refuses source() reads. */
9801022
private class CloseTrackingResponseBody(
9811023
private val closes: AtomicInteger,

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import org.dexpace.sdk.core.util.Clock
2828
import org.dexpace.sdk.core.util.Futures
2929
import java.io.IOException
3030
import java.io.InterruptedIOException
31+
import java.net.SocketTimeoutException
3132
import java.time.Duration
3233
import java.time.Instant
3334
import java.util.concurrent.CompletableFuture
@@ -190,6 +191,22 @@ class DefaultAsyncRetryStepTest {
190191
assertEquals(3, client.callCount)
191192
}
192193

194+
@Test
195+
fun `SocketTimeoutException is retried as a transient failure not treated as cancellation`() {
196+
// SocketTimeoutException extends InterruptedIOException, but it is a read timeout, not a
197+
// cancellation — it must retry via the normal classifier, unlike the InterruptedIOException
198+
// and bare-InterruptedException cases which abort after one attempt and restore the
199+
// interrupt flag.
200+
val client = FailNTimesClient(failures = 2) { SocketTimeoutException("read timed out") }
201+
val future =
202+
pipeline(client, HttpRetryOptions.fixed(maxRetries = 3, delay = Duration.ZERO))
203+
.sendAsync(getRequest())
204+
scheduler.runAll()
205+
assertEquals(200, future.join().status.code)
206+
assertEquals(3, client.callCount, "SocketTimeoutException must be retried, not surfaced as cancellation")
207+
assertFalse(Thread.interrupted(), "a read timeout must not restore the interrupt flag")
208+
}
209+
193210
@Test
194211
fun `non-retryable exception is surfaced immediately`() {
195212
val client = FailNTimesClient(failures = 5) { IllegalArgumentException("nope") }

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import java.io.ByteArrayInputStream
3535
import java.io.IOException
3636
import java.io.InputStream
3737
import java.io.InterruptedIOException
38+
import java.net.SocketTimeoutException
3839
import java.time.Duration
3940
import java.time.Instant
4041
import java.util.concurrent.TimeoutException
@@ -504,6 +505,32 @@ class RetryStepTest {
504505
assertEquals(2, attempts.get())
505506
}
506507

508+
@Test
509+
fun `SocketTimeoutException is retried as a transient failure not treated as cancellation`() {
510+
// SocketTimeoutException extends InterruptedIOException, but it is a read timeout — NOT a
511+
// cancellation. It must be routed through the normal retry classifier (retried up to
512+
// maxRetries), NOT the interrupt path that rethrows immediately and re-sets the interrupt
513+
// flag (contrast `InterruptedIOException from the transport is rethrown immediately`).
514+
val attempts = AtomicInteger(0)
515+
val client =
516+
object : HttpClient {
517+
override fun execute(request: Request): Response {
518+
val n = attempts.incrementAndGet()
519+
if (n < 3) throw SocketTimeoutException("read timed out")
520+
return okResponse(request)
521+
}
522+
}
523+
val pipeline =
524+
HttpPipelineBuilder(client)
525+
.append(DefaultRetryStep(HttpRetryOptions(maxRetries = 3), zeroDelayClock()))
526+
.build()
527+
528+
val response = pipeline.send(getRequest())
529+
assertEquals(200, response.status.code)
530+
assertEquals(3, attempts.get(), "SocketTimeoutException must be retried, not surfaced as cancellation")
531+
assertTrue(!Thread.currentThread().isInterrupted, "a read timeout must not set the interrupt flag")
532+
}
533+
507534
@Test
508535
fun `nested cause chain RuntimeException-of-IOException is retried`() {
509536
val attempts = AtomicInteger(0)

‎sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/ResponseExtensionsTest.kt‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,24 @@ class ResponseExtensionsTest {
133133
assertEquals(bodyText, bodyString)
134134
}
135135

136+
@Test
137+
fun `throwOnError buffered body is replayable and readable more than once`() {
138+
// bufferErrorBody produces a replayable in-memory body: each source() call re-reads the full
139+
// bytes. So the caught exception's body can be decoded via bodyAs AND then read again via
140+
// string(), both seeing the complete buffered payload.
141+
val bodyText = """{"error":"boom","detail":"buffered and replayable"}"""
142+
val r = response(Status.INTERNAL_SERVER_ERROR, bodyText)
143+
val ex = assertFailsWith<HttpException> { r.throwOnError() }
144+
145+
// First read: decode via bodyAs using a deserializer that consumes the whole stream.
146+
val firstRead = ex.bodyAs(streamReadingDeserializer(), String::class.java)
147+
assertEquals(bodyText, firstRead)
148+
149+
// Second read: the buffered body is replayable, so string() re-reads the full bytes.
150+
val secondRead = assertNotNull(ex.body).string()
151+
assertEquals(bodyText, secondRead)
152+
}
153+
136154
@Test
137155
fun `throwOnError exception status matches original response status`() {
138156
val r = response(Status.NOT_FOUND, "nope")
@@ -324,6 +342,26 @@ class ResponseExtensionsTest {
324342
)
325343
}
326344

345+
/** Deserializer that reads the whole input stream and returns it as a UTF-8 String. */
346+
@Suppress("UNCHECKED_CAST")
347+
private fun streamReadingDeserializer(): Deserializer =
348+
object : Deserializer {
349+
override fun <R> deserialize(
350+
input: String,
351+
type: Class<R>,
352+
): R = throw UnsupportedOperationException()
353+
354+
override fun <R> deserialize(
355+
input: ByteArray,
356+
type: Class<R>,
357+
): R = throw UnsupportedOperationException()
358+
359+
override fun <R> deserialize(
360+
inputStream: InputStream,
361+
type: Class<R>,
362+
): R = inputStream.readBytes().toString(Charsets.UTF_8) as R
363+
}
364+
327365
private fun fakeSerde(deserializer: Deserializer): Serde =
328366
object : Serde {
329367
override val serializer: Serializer get() = throw UnsupportedOperationException()

‎sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/ResponseTest.kt‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -403,6 +403,52 @@ class ResponseTest {
403403
.build()
404404
assertFalse(resp.isSuccessful)
405405
}
406+
407+
// ---- status-class delegation (isInformational / isRedirect / isClientError / …) ---------
408+
409+
private fun responseWith(status: Status): Response =
410+
Response.builder()
411+
.request(request())
412+
.protocol(Protocol.HTTP_1_1)
413+
.status(status)
414+
.build()
415+
416+
@Test
417+
fun `status-class predicates delegate to Status for representative codes`() {
418+
// Each predicate must mirror Status's own classification. Representative codes:
419+
// 100 informational, 301 redirect, 404 client error, 503 server error, 200 success.
420+
val informational = responseWith(Status.fromCode(100))
421+
assertTrue(informational.isInformational)
422+
assertFalse(informational.isRedirect)
423+
assertFalse(informational.isClientError)
424+
assertFalse(informational.isServerError)
425+
assertFalse(informational.isError)
426+
427+
val redirect = responseWith(Status.fromCode(301))
428+
assertFalse(redirect.isInformational)
429+
assertTrue(redirect.isRedirect)
430+
assertFalse(redirect.isClientError)
431+
assertFalse(redirect.isServerError)
432+
assertFalse(redirect.isError)
433+
434+
val clientError = responseWith(Status.fromCode(404))
435+
assertFalse(clientError.isRedirect)
436+
assertTrue(clientError.isClientError)
437+
assertFalse(clientError.isServerError)
438+
assertTrue(clientError.isError)
439+
440+
val serverError = responseWith(Status.fromCode(503))
441+
assertFalse(serverError.isClientError)
442+
assertTrue(serverError.isServerError)
443+
assertTrue(serverError.isError)
444+
445+
val success = responseWith(Status.fromCode(200))
446+
assertFalse(success.isInformational)
447+
assertFalse(success.isRedirect)
448+
assertFalse(success.isClientError)
449+
assertFalse(success.isServerError)
450+
assertFalse(success.isError)
451+
}
406452
}
407453

408454
class ResponseBodyTest {
@@ -522,4 +568,30 @@ class ResponseBodyTest {
522568
val body = ResponseBody.create(source)
523569
assertContentEquals(ByteArray(0), body.bytes())
524570
}
571+
572+
// ---- string() default charset derives from the media type ----------------------------
573+
574+
@Test
575+
fun `no-arg string() decodes using the charset declared in the media type`() {
576+
// 0xE9 is 'é' in ISO-8859-1 but an invalid lone byte in UTF-8. With the media type
577+
// declaring charset=ISO-8859-1, the no-arg string() must latin-1-decode it to "é",
578+
// not fall back to UTF-8.
579+
val bytes = byteArrayOf(0xE9.toByte())
580+
val mediaType = MediaType.parse("text/plain; charset=ISO-8859-1")
581+
val source = Io.provider.source(bytes)
582+
val body = ResponseBody.create(source, mediaType, bytes.size.toLong())
583+
assertEquals("é", body.string())
584+
}
585+
586+
@Test
587+
fun `explicit charset argument overrides the media type charset`() {
588+
// Bytes are the UTF-8 encoding of "é" (0xC3 0xA9). Even though the media type declares
589+
// ISO-8859-1 (under which those bytes decode to "é"), an explicit string(UTF_8) must use
590+
// UTF-8 and yield "é" — proving the argument takes precedence over the media type charset.
591+
val bytes = "é".toByteArray(Charsets.UTF_8)
592+
val mediaType = MediaType.parse("text/plain; charset=ISO-8859-1")
593+
val source = Io.provider.source(bytes)
594+
val body = ResponseBody.create(source, mediaType, bytes.size.toLong())
595+
assertEquals("é", body.string(Charsets.UTF_8))
596+
}
525597
}

‎sdk-core/src/test/kotlin/org/dexpace/sdk/core/instrumentation/UrlRedactorTest.kt‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,4 +181,37 @@ class UrlRedactorTest {
181181
// The trailing '&' is intentionally dropped — the output must NOT end with '&'.
182182
assertTrue(!out.endsWith("&"), "trailing '&' must be dropped; got: $out")
183183
}
184+
185+
// ---- redactUrlValue: absolute vs relative header-value redaction ---------------------
186+
187+
@Test
188+
fun `redactUrlValue redacts a disallowed query credential on an absolute URL`() {
189+
// An OAuth authorization code in the query of an absolute (e.g. Location) header value
190+
// must be redacted just like redact(URL) does.
191+
val out = UrlRedactor.redactUrlValue("https://cb/?code=SECRET")
192+
assertTrue(out.contains("code="), "the query key should be kept: $out")
193+
assertTrue(out.contains("***"), "the credential value should be redacted: $out")
194+
assertTrue(!out.contains("SECRET"), "raw authorization code leaked: $out")
195+
}
196+
197+
@Test
198+
fun `redactUrlValue redacts an implicit-flow access token in the fragment`() {
199+
// Implicit-flow tokens ride in the URL fragment as key=value tokens; they must be redacted
200+
// under the same allow-list as query parameters.
201+
val out = UrlRedactor.redactUrlValue("https://cb/#access_token=SECRET")
202+
assertTrue(out.contains("#"), "fragment marker should be present: $out")
203+
assertTrue(out.contains("access_token="), "fragment key should be kept: $out")
204+
assertTrue(out.contains("***"), "fragment token value should be redacted: $out")
205+
assertTrue(!out.contains("SECRET"), "raw fragment token leaked: $out")
206+
}
207+
208+
@Test
209+
fun `redactUrlValue keeps the path but drops the query for a relative value`() {
210+
// A relative Location value is not a parseable absolute URL, so the whole query (which may
211+
// carry an OAuth code / pre-signed signature) is dropped while the path stays visible.
212+
val out = UrlRedactor.redactUrlValue("/cb?code=SECRET")
213+
assertEquals("/cb?***", out)
214+
assertTrue(out.startsWith("/cb"), "the path must be preserved: $out")
215+
assertTrue(!out.contains("SECRET"), "raw query credential leaked from a relative value: $out")
216+
}
184217
}

0 commit comments

Comments
 (0)