From ff4464070c5bd1e617f4bd783b2bcde36d229fc6 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 16 Jun 2026 16:19:47 +0300 Subject: [PATCH 1/3] refactor!: unify the HTTP error path on a single exception base and a Retryable interface The response-carrying exception hierarchy had two overlapping bases and the factory that produces the typed family was never wired into the error path. - Collapse `HttpResponseException` (an `IOException` carrying a `Response` + optional error value) into `HttpException`. `HttpException` becomes the single response-carrying base; it gains an optional `value` slot for a deserialized error payload so the generated layer can stamp a typed body later. Transport failures keep using the `NetworkException` (`IOException`) sibling, so existing `catch (IOException)` sites for transport errors are unaffected. - Introduce a `Retryable` interface (`val isRetryable: Boolean`) and implement it on both `HttpException` and `NetworkException`. The two types previously disagreed on the accessor name (`retryable` vs `isRetryable`); they now expose the same member through the interface. Retry classification in `RetryStep` keys off `Retryable` instead of matching concrete types, so a new retryable exception type participates automatically. The set of retryable statuses is unchanged (408/429 and 5xx except 501/505, still derived from `RetryUtils`). - Wire `HttpExceptionFactory.fromResponse` into the live error path via a new `ThrowOnHttpErrorStep` response step: on a 4xx/5xx response it throws the factory's typed exception, which `ResponsePipeline` turns into a failure outcome that flows through the recovery chain (including retry) like a transport failure. Previously the factory had no callers and 4xx/5xx responses never produced the typed subclass family. This is a binary-incompatible change to the public exception API: the `HttpResponseException` type is removed, the `retryable` getter on `HttpException` and `NetworkException` is renamed to `isRetryable`, and `HttpException` gains a `value` property and a corresponding constructor parameter. API snapshot regenerated; docs updated. --- README.md | 4 +- docs/architecture.md | 8 +- docs/http.md | 36 ++-- docs/pipelines.md | 8 +- docs/refs-comparison.md | 2 +- sdk-core/api/sdk-core.api | 32 ++-- .../http/response/HttpResponseException.kt | 60 ------- .../http/response/exception/HttpException.kt | 29 +++- .../http/response/exception/HttpExceptions.kt | 2 +- .../response/exception/NetworkException.kt | 17 +- .../core/http/response/exception/Retryable.kt | 32 ++++ .../pipeline/step/ThrowOnHttpErrorStep.kt | 66 +++++++ .../sdk/core/pipeline/step/retry/RetryStep.kt | 34 ++-- .../org/dexpace/sdk/core/util/RetryUtils.kt | 7 +- .../response/HttpResponseExceptionTest.kt | 163 ------------------ .../exception/HttpExceptionFactoryTest.kt | 96 +++++++---- .../pipeline/step/ThrowOnHttpErrorStepTest.kt | 143 +++++++++++++++ 17 files changed, 415 insertions(+), 324 deletions(-) delete mode 100644 sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/HttpResponseException.kt create mode 100644 sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/Retryable.kt create mode 100644 sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStep.kt delete mode 100644 sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/HttpResponseExceptionTest.kt create mode 100644 sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStepTest.kt diff --git a/README.md b/README.md index 56a7de8e..a5ce807a 100644 --- a/README.md +++ b/README.md @@ -246,8 +246,8 @@ See [docs/pipelines.md](docs/pipelines.md) for the step-author walkthrough. |---|---| | `client` | `HttpClient`, `AsyncHttpClient` — the two transport SPIs (sync and async). | | `http.request` | `Request`, `RequestBody`, `FileRequestBody`, `LoggableRequestBody`, `Method`. | -| `http.response` | `Response`, `ResponseBody`, `LoggableResponseBody`, `Status` (a value-carrying class with a total `fromCode`), `HttpResponseException`. | -| `http.response.exception` | Typed `HttpException` hierarchy (`BadRequestException`, `RequestTimeoutException`, `TooManyRequestsException`, `ServiceUnavailableException`, …) with `retryable` derived from `RetryUtils.isRetryable`, plus `NetworkException` and `HttpExceptionFactory`. | +| `http.response` | `Response`, `ResponseBody`, `LoggableResponseBody`, `Status` (a value-carrying class with a total `fromCode`). | +| `http.response.exception` | Typed `HttpException` hierarchy (`BadRequestException`, `RequestTimeoutException`, `TooManyRequestsException`, `ServiceUnavailableException`, …) with `isRetryable` derived from `RetryUtils.isRetryable` and exposed via the `Retryable` interface, plus `NetworkException` and `HttpExceptionFactory`. | | `http.common` | `Headers`, `HttpHeaderName` (interned), `MediaType`, `Protocol`, `HttpRange`, `ETag`, `RequestConditions`. | | `http.context` | `CallContext` → `DispatchContext` → `RequestContext` → `ExchangeContext` chain, `ContextStore`. | | `http.pipeline` | Sync (`HttpStep` / `HttpPipeline` / `HttpPipelineBuilder` / `PipelineNext` / `Stage`) and async (`AsyncHttpStep` / `AsyncHttpPipeline` / `AsyncHttpPipelineBuilder` / `AsyncPipelineNext`) pipeline machinery, plus `AsyncPipelineBridges`. | diff --git a/docs/architecture.md b/docs/architecture.md index 32c8a5e3..4001889a 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -575,8 +575,8 @@ responds in a uniform way: subsequent blocking call also surfaces it. 3. Throws `InterruptedIOException` (or the operation's natural failure exception with `InterruptedException` added as a suppressed cause). -4. Classifies the interruption as **non-retryable** — `HttpResponseException.isRetryable` - returns `false` for an interrupt-driven failure. +4. Classifies the interruption as **non-retryable** — an interrupt-driven failure is not a + `Retryable` condition, so the retry step never re-issues it. Loops bounded by user input (retry attempts, paged iteration, server-sent-event consumption, drain loops in body logging) check `Thread.currentThread().isInterrupted` at @@ -653,8 +653,8 @@ they should construct a fresh one. |----------------------|-------------------------------------------------------------------------------------------------| | `io` | Source, Sink, BufferedSource, BufferedSink, Buffer, IoProvider, Io, TeeSink (internal) | | `http.request` | Request, RequestBody, FileRequestBody, LoggableRequestBody, Method | -| `http.response` | Response, ResponseBody, LoggableResponseBody, Status, HttpResponseException | -| `http.response.exception` | HttpException, HttpExceptionFactory, RequestTimeoutException (and siblings), NetworkException | +| `http.response` | Response, ResponseBody, LoggableResponseBody, Status | +| `http.response.exception` | HttpException, HttpExceptionFactory, Retryable, RequestTimeoutException (and siblings), NetworkException | | `http.common` | Headers, MediaType, CommonMediaTypes, Protocol, ETag, HttpRange, RequestConditions | | `http.auth` | Credential, KeyCredential, BearerToken, ChallengeHandler, Basic/Digest/CompositeChallengeHandler, AuthChallengeParser | | `http.context` | CallContext, DispatchContext, RequestContext, ExchangeContext, ContextStore | diff --git a/docs/http.md b/docs/http.md index 6c0c933d..769ef512 100644 --- a/docs/http.md +++ b/docs/http.md @@ -286,8 +286,9 @@ on whether a code is one the SDK recognizes. The SDK ships a typed exception hierarchy under `org.dexpace.sdk.core.http.response.exception`. The shape mirrors `gax`'s `ApiException` taxonomy translated to HTTP terms: one base class plus one concrete subclass per canonical status code. The base class derives its retryable flag -from a single source of truth, so a downstream retry policy can read `exception.retryable` -instead of maintaining a parallel predicate map. +from a single source of truth and exposes it through the `Retryable` interface, so a downstream +retry policy can read `(t as? Retryable)?.isRetryable` instead of maintaining a parallel +predicate map or matching concrete exception types. ### HttpException Hierarchy @@ -302,16 +303,21 @@ abstract class HttpException( val body: ResponseBody?, // lazy — NOT eagerly buffered message: String? = null, cause: Throwable? = null, -) : RuntimeException(message, cause) { - val retryable: Boolean = RetryUtils.isRetryable(status.code) + val value: Any? = null, // slot for a deserialized error payload (generated layer) +) : RuntimeException(message, cause), Retryable { + override val isRetryable: Boolean = RetryUtils.isRetryable(status.code) fun bodySnapshot(maxBytes: Int = DEFAULT_SNAPSHOT_BYTES): ByteArray? } ``` -`retryable` is a `val` **derived** at construction from `RetryUtils.isRetryable(status.code)`, +`isRetryable` is a `val` **derived** at construction from `RetryUtils.isRetryable(status.code)`, not hardcoded per subclass and not a constructor parameter. This guarantees the baked flag can never disagree with the live retry policy: 408 / 429 and the 5xx range except 501 and 505 -are retryable, everything else is not. +are retryable, everything else is not. It satisfies the `Retryable` interface (shared with +`NetworkException`), which is what the retry step keys off. + +`value` is a slot for a deserialized error payload, left `null` here — `sdk-core` does not +parse bodies. The generated service layer populates it on a per-operation typed subclass. `bodySnapshot()` returns a non-consuming preview of the body bytes — it reads from a fresh `source().peek()` view so the primary read path is undisturbed — capped at `maxBytes` @@ -355,19 +361,21 @@ subclass: it extends `java.io.IOException` so existing `catch (IOException)` cal working, and it carries no status/headers/body because none arrived. ```kotlin -open class NetworkException(message: String? = null, cause: Throwable? = null) : IOException(message, cause) { - val retryable: Boolean = true // always retryable at the SDK level +open class NetworkException(message: String? = null, cause: Throwable? = null) : + IOException(message, cause), Retryable { + override val isRetryable: Boolean = true // always retryable at the SDK level } ``` -The `retryable` flag is always `true`: nothing reached the server, so the SDK can safely +The `isRetryable` flag is always `true`: nothing reached the server, so the SDK can safely attempt the request again. Whether the request itself is *safe* to retry (HTTP method idempotency, replayable body) is the retry policy's call, not this class's. ### HttpExceptionFactory -`HttpExceptionFactory.fromResponse(response)` maps a non-2xx `Response` to the right -subclass: +`HttpExceptionFactory.fromResponse(response)` maps a 4xx/5xx `Response` to the right subclass. +It is the one seam that turns an error response into a typed exception, so the family is +produced consistently rather than re-derived at each call site: ```kotlin val response = httpClient.execute(request) @@ -379,6 +387,12 @@ if (!response.status.isSuccess) { The factory throws `IllegalArgumentException` if called with a status outside 400..599 — 1xx/2xx/3xx outcomes are not exceptions and should not be funneled through this path. +In the recovery-aware pipeline this is wired through `ThrowOnHttpErrorStep`, a +`ResponsePipelineStep` that calls `fromResponse` on a 4xx/5xx response and throws the result. +`ResponsePipeline` converts that throw into a `ResponseOutcome.Failure`, which then flows +through the recovery chain (e.g. `RetryStep`) exactly like a transport failure — and because +the thrown `HttpException` is `Retryable`, retry classification keys off it uniformly. + --- ## Common Types diff --git a/docs/pipelines.md b/docs/pipelines.md index 4130dc23..e062db1b 100644 --- a/docs/pipelines.md +++ b/docs/pipelines.md @@ -355,11 +355,13 @@ public class RetryStep( ) ``` -It retries only when the outcome is a `Failure` whose throwable is classified retryable: +It retries only when the outcome is a `Failure` whose throwable is classified retryable. The +classifier keys off the `Retryable` interface, not concrete exception types: -- An `HttpException` with `retryable == true` whose status code is in +- An `HttpException` (which is `Retryable`) with `isRetryable == true` whose status code is in `RetrySettings.retryableStatuses`. -- A `NetworkException` (a transport failure with no response on the wire — always retryable). +- A `NetworkException` (also `Retryable`, always `true` — a transport failure with no response + on the wire). Idempotency is enforced independently of classification: a request is eligible only when its method is in `RetrySettings.retryableMethods` **or** its body is replayable. Non-idempotent diff --git a/docs/refs-comparison.md b/docs/refs-comparison.md index d33df5ad..aea2b3b7 100644 --- a/docs/refs-comparison.md +++ b/docs/refs-comparison.md @@ -223,7 +223,7 @@ below records where each scheme's design was sourced from: **What shipped, and what's left:** 1. `HttpException` is the base, with status-code-keyed subclasses (`NotFoundException`, `UnauthorizedException`, `TooManyRequestsException`, `InternalServerErrorException`, etc.) plus `ClientErrorException`/`ServerErrorException` fallbacks for unmapped 4xx/5xx. `NetworkException` is a sibling for transport failures. The set mirrors gax's taxonomy scaled to HTTP statuses. -2. `retryable: Boolean` is a `val` derived once at construction from `RetryUtils.isRetryable(status.code)` — not a per-subclass constant. This is the single source of truth, so it can never disagree with the live retry policy (408 retryable; 501/505 not). A retry predicate is just `(t as? HttpException)?.retryable == true`. +2. `isRetryable: Boolean` (from the `Retryable` interface) is a `val` derived once at construction from `RetryUtils.isRetryable(status.code)` — not a per-subclass constant. This is the single source of truth, so it can never disagree with the live retry policy (408 retryable; 501/505 not). `NetworkException` implements the same interface (always `true`), so a retry predicate keys off the interface: `(t as? Retryable)?.isRetryable == true`. 3. The base exposes `status` + `headers` + a **lazy** `body: ResponseBody?` (not eagerly buffered), plus a non-consuming `bodySnapshot()` that reads from a `peek()` view so the primary read path is undisturbed — large 5xx bodies don't OOM. Per-operation per-status subclasses carrying typed bodies (Expedia pattern) are still codegen's job. 4. Tolerant error-body parsing (Square's `SquareApiException.parseErrors`) remains future work — never throw inside an exception constructor; pass through the raw body on parse failure. diff --git a/sdk-core/api/sdk-core.api b/sdk-core/api/sdk-core.api index de3a5d14..b855b5f7 100644 --- a/sdk-core/api/sdk-core.api +++ b/sdk-core/api/sdk-core.api @@ -1100,16 +1100,6 @@ public final class org/dexpace/sdk/core/http/request/RequestBody$Companion { public static synthetic fun create$default (Lorg/dexpace/sdk/core/http/request/RequestBody$Companion;[BLorg/dexpace/sdk/core/http/common/MediaType;ILjava/lang/Object;)Lorg/dexpace/sdk/core/http/request/RequestBody; } -public class org/dexpace/sdk/core/http/response/HttpResponseException : java/io/IOException { - public fun (Ljava/lang/String;Lorg/dexpace/sdk/core/http/response/Response;)V - public fun (Ljava/lang/String;Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/Object;)V - public fun (Ljava/lang/String;Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/Object;Ljava/lang/Throwable;)V - public synthetic fun (Ljava/lang/String;Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/Object;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V - public final fun getResponse ()Lorg/dexpace/sdk/core/http/response/Response; - public final fun getValue ()Ljava/lang/Object; - public final fun isRetryable ()Z -} - public final class org/dexpace/sdk/core/http/response/LoggableResponseBody : org/dexpace/sdk/core/http/response/ResponseBody { public fun (Lorg/dexpace/sdk/core/http/response/ResponseBody;)V public fun (Lorg/dexpace/sdk/core/http/response/ResponseBody;Lorg/dexpace/sdk/core/io/IoProvider;)V @@ -1319,20 +1309,22 @@ public class org/dexpace/sdk/core/http/response/exception/GoneException : org/de public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } -public abstract class org/dexpace/sdk/core/http/response/exception/HttpException : java/lang/RuntimeException { +public abstract class org/dexpace/sdk/core/http/response/exception/HttpException : java/lang/RuntimeException, org/dexpace/sdk/core/http/response/exception/Retryable { public static final field Companion Lorg/dexpace/sdk/core/http/response/exception/HttpException$Companion; public static final field DEFAULT_SNAPSHOT_BYTES I public fun (Lorg/dexpace/sdk/core/http/response/Status;Lorg/dexpace/sdk/core/http/common/Headers;Lorg/dexpace/sdk/core/http/response/ResponseBody;)V public fun (Lorg/dexpace/sdk/core/http/response/Status;Lorg/dexpace/sdk/core/http/common/Headers;Lorg/dexpace/sdk/core/http/response/ResponseBody;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Status;Lorg/dexpace/sdk/core/http/common/Headers;Lorg/dexpace/sdk/core/http/response/ResponseBody;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Status;Lorg/dexpace/sdk/core/http/common/Headers;Lorg/dexpace/sdk/core/http/response/ResponseBody;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Status;Lorg/dexpace/sdk/core/http/common/Headers;Lorg/dexpace/sdk/core/http/response/ResponseBody;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Status;Lorg/dexpace/sdk/core/http/common/Headers;Lorg/dexpace/sdk/core/http/response/ResponseBody;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V public final fun bodySnapshot ()[B public final fun bodySnapshot (I)[B public static synthetic fun bodySnapshot$default (Lorg/dexpace/sdk/core/http/response/exception/HttpException;IILjava/lang/Object;)[B public final fun getBody ()Lorg/dexpace/sdk/core/http/response/ResponseBody; public final fun getHeaders ()Lorg/dexpace/sdk/core/http/common/Headers; - public final fun getRetryable ()Z public final fun getStatus ()Lorg/dexpace/sdk/core/http/response/Status; + public final fun getValue ()Ljava/lang/Object; + public fun isRetryable ()Z } public final class org/dexpace/sdk/core/http/response/exception/HttpException$Companion { @@ -1357,12 +1349,12 @@ public class org/dexpace/sdk/core/http/response/exception/MethodNotAllowedExcept public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } -public class org/dexpace/sdk/core/http/response/exception/NetworkException : java/io/IOException { +public class org/dexpace/sdk/core/http/response/exception/NetworkException : java/io/IOException, org/dexpace/sdk/core/http/response/exception/Retryable { public fun ()V public fun (Ljava/lang/String;)V public fun (Ljava/lang/String;Ljava/lang/Throwable;)V public synthetic fun (Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V - public final fun getRetryable ()Z + public fun isRetryable ()Z } public class org/dexpace/sdk/core/http/response/exception/NotFoundException : org/dexpace/sdk/core/http/response/exception/HttpException { @@ -1386,6 +1378,10 @@ public class org/dexpace/sdk/core/http/response/exception/RequestTimeoutExceptio public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } +public abstract interface class org/dexpace/sdk/core/http/response/exception/Retryable { + public abstract fun isRetryable ()Z +} + public class org/dexpace/sdk/core/http/response/exception/ServerErrorException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V @@ -2091,6 +2087,12 @@ public abstract interface class org/dexpace/sdk/core/pipeline/step/ResponseRecov public abstract fun invoke (Lorg/dexpace/sdk/core/pipeline/ResponseOutcome;)Lorg/dexpace/sdk/core/pipeline/ResponseOutcome; } +public final class org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStep : org/dexpace/sdk/core/pipeline/step/ResponsePipelineStep { + public static final field INSTANCE Lorg/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStep; + public synthetic fun execute (Ljava/lang/Object;Lorg/dexpace/sdk/core/http/context/DispatchContext;)Ljava/lang/Object; + public fun execute (Lorg/dexpace/sdk/core/http/response/Response;Lorg/dexpace/sdk/core/http/context/DispatchContext;)Lorg/dexpace/sdk/core/http/response/Response; +} + public final class org/dexpace/sdk/core/pipeline/step/retry/BackoffCalculator { public static final field INSTANCE Lorg/dexpace/sdk/core/pipeline/step/retry/BackoffCalculator; public static final fun computeDelay (ILorg/dexpace/sdk/core/pipeline/step/retry/RetrySettings;)Ljava/time/Duration; diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/HttpResponseException.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/HttpResponseException.kt deleted file mode 100644 index 7e4d7102..00000000 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/HttpResponseException.kt +++ /dev/null @@ -1,60 +0,0 @@ -/* - * 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.http.response - -import org.dexpace.sdk.core.util.RetryUtils -import java.io.IOException - -/** - * An [IOException] that carries an HTTP [Response] (when available) and an optional - * deserialized error [value]. The exception classifies its own retryability at - * construction time so the retry policy can query a precomputed field rather than - * maintaining a parallel predicate. - * - * Retryability rules (see [RetryUtils]): - * - If [response] is non-null, [isRetryable] reflects [RetryUtils.isRetryable] for the - * response status code: 408, 429, or 5xx excluding 501 and 505. - * - Otherwise, if [cause] is non-null, [isRetryable] is `true` when the cause chain - * contains an [IOException] or [java.util.concurrent.TimeoutException]. - * - If both are null, [isRetryable] is `false`. - * - * Extends [IOException] so it propagates cleanly through APIs declaring - * `@Throws(IOException::class)`. Marked `open` so service clients can introduce typed - * subclasses; the [isRetryable] field is `final` and cannot be overridden. - * - * @param message Human-readable error description. - * @property response Response that triggered the error, or `null` when the failure happened - * before a response was received. - * @property value Optional deserialized payload describing the error (e.g. a JSON error body). - * @param cause Underlying cause to chain into the [IOException], or `null`. - */ -public open class HttpResponseException - @JvmOverloads - constructor( - message: String, - public val response: Response?, - public val value: Any? = null, - cause: Throwable? = null, - ) : IOException(message, cause) { - /** - * Whether this exception represents a retryable condition. Computed once at - * construction; querying is free. - */ - public val isRetryable: Boolean = computeIsRetryable(response, cause) - - private companion object { - private fun computeIsRetryable( - response: Response?, - cause: Throwable?, - ): Boolean { - response?.let { return RetryUtils.isRetryable(it.status.code) } - cause?.let { return RetryUtils.isRetryable(it) } - return false - } - } - } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpException.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpException.kt index afea41aa..121976a1 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpException.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpException.kt @@ -25,14 +25,18 @@ import java.io.IOException * so service-client codegen can introduce per-operation typed subclasses by deriving from a * concrete one and stamping a typed deserialized payload. * + * This is the single response-carrying exception base. It is a [RuntimeException], so callers + * are not forced to declare `throws` for protocol-level failures; transport-level failures use + * [NetworkException] (an `IOException` sibling) instead. + * * ## Retryability * - * [retryable] is a `val` derived at construction time from [RetryUtils.isRetryable] applied - * to [status]'s code — it is *not* hardcoded per subclass. This guarantees the baked flag can - * never disagree with the live retry policy: 408 / 429 and the 5xx range (except 501 and 505) - * are retryable, every other status is not. A downstream retry policy is expected to query - * this field rather than maintaining a parallel predicate map. Transport-level - * [NetworkException] is a sibling type and always retryable. + * [HttpException] implements [Retryable]. [isRetryable] is a `val` derived at construction time + * from [RetryUtils.isRetryable] applied to [status]'s code — it is *not* hardcoded per subclass. + * This guarantees the baked flag can never disagree with the live retry policy: 408 / 429 and + * the 5xx range (except 501 and 505) are retryable, every other status is not. A downstream + * retry policy queries the [Retryable] interface rather than maintaining a parallel predicate + * map. Transport-level [NetworkException] is a sibling type and always retryable. * * ## Body access * @@ -41,6 +45,10 @@ import java.io.IOException * non-consuming preview suitable for log lines. The snapshot uses * [org.dexpace.sdk.core.io.BufferedSource.peek] so the primary read path is undisturbed. * + * [value] is a slot for a deserialized error payload (e.g. a parsed JSON error object). It is + * `null` here; the generated service layer is expected to populate it on a per-operation typed + * subclass once it has deserialized [body]. + * * ## Why `RuntimeException` and not `IOException` * * A successful transport produced a response — the failure is at the protocol level, not at @@ -51,7 +59,9 @@ import java.io.IOException * @property status Parsed HTTP status from the originating response. * @property headers Response headers as received on the wire. * @property body Lazy response body, or `null` when the response carries no payload. - * @property retryable Whether the exception represents a retryable condition. Derived once at + * @property value Optional deserialized payload describing the error, or `null` when none was + * parsed. Reserved for the generated layer to stamp a typed error object. + * @property isRetryable Whether the exception represents a retryable condition. Derived once at * construction from [RetryUtils.isRetryable] over [status]'s code, so it always mirrors the * live retry policy rather than a per-subclass constant. */ @@ -63,13 +73,14 @@ public abstract class HttpException public val body: ResponseBody?, message: String? = null, cause: Throwable? = null, - ) : RuntimeException(message ?: defaultMessage(status), cause) { + public val value: Any? = null, + ) : RuntimeException(message ?: defaultMessage(status), cause), Retryable { /** * Whether this exception represents a retryable condition. Derived from * [RetryUtils.isRetryable] over [status]'s code so it can never disagree with the * live retry policy. */ - public val retryable: Boolean = RetryUtils.isRetryable(status.code) + override val isRetryable: Boolean = RetryUtils.isRetryable(status.code) /** * Returns a non-consuming preview of the body bytes, capped at [maxBytes]. diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptions.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptions.kt index 7587ee1f..06c2bec4 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptions.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptions.kt @@ -18,7 +18,7 @@ import org.dexpace.sdk.core.http.response.Response // that stamps a deserialized error payload — see `docs/refs-comparison.md` Error Model §, // action item 3. // -// None of these subclasses sets its own retryable flag: `HttpException.retryable` is derived +// None of these subclasses sets its own retryable flag: `HttpException.isRetryable` is derived // from `RetryUtils.isRetryable(status.code)` (the single source of truth), so the baked flag // always mirrors the live retry policy — 408 / 429 and the 5xx range except 501 / 505 are // retryable, everything else is not. Fallback subclasses (`ClientErrorException`, diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/NetworkException.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/NetworkException.kt index 3fc8182a..b1a259e0 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/NetworkException.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/NetworkException.kt @@ -23,24 +23,25 @@ import java.io.IOException * * ## Retryability * - * Always [retryable] = `true`. A transport-level failure with no response on the wire is by - * definition transient from the SDK's standpoint: nothing was processed by the server, so a - * retry can attempt the request from scratch. Whether the operation is *safe* to retry - * still depends on idempotency (HTTP method + replayable body) — that decision lives at the - * retry step, not here. + * Implements [Retryable] with [isRetryable] always `true`. A transport-level failure with no + * response on the wire is by definition transient from the SDK's standpoint: nothing was + * processed by the server, so a retry can attempt the request from scratch. Whether the + * operation is *safe* to retry still depends on idempotency (HTTP method + replayable body) — + * that decision lives at the retry step, not here. * - * @property retryable Always `true`; exposed as a `val` for API symmetry with [HttpException]. + * @property isRetryable Always `true`; satisfies [Retryable] so the retry classifier treats + * transport failures uniformly with retryable [HttpException]s. */ public open class NetworkException @JvmOverloads constructor( message: String? = null, cause: Throwable? = null, - ) : IOException(message, cause) { + ) : IOException(message, cause), Retryable { /** * Whether this exception represents a retryable condition. Always `true` for * `NetworkException` — transport failures with no response are by definition * transient at the SDK level. */ - public val retryable: Boolean = true + override val isRetryable: Boolean = true } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/Retryable.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/Retryable.kt new file mode 100644 index 00000000..d1811d34 --- /dev/null +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/Retryable.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.core.http.response.exception + +/** + * Marks an exception that knows whether the condition it represents is worth retrying. + * + * The retry machinery keys off this interface rather than concrete exception types: a + * [org.dexpace.sdk.core.pipeline.step.retry.RetryStep] asks `error is Retryable` and reads + * [isRetryable] instead of matching every subclass it might encounter. New retryable + * exception types participate automatically by implementing this interface — no change to the + * classifier is required. + * + * Two SDK exceptions implement it today: + * - [HttpException] — a response was received; [isRetryable] is derived once at construction + * from the status code via [org.dexpace.sdk.core.util.RetryUtils.isRetryable]. + * - [NetworkException] — the transport failed before any response arrived; [isRetryable] is + * always `true` (nothing reached the server, so a fresh attempt is safe at the SDK level). + * + * A `true` flag means the *condition* is transient. Whether a specific request is *safe* to + * replay (HTTP-method idempotency, replayable body) is an orthogonal decision owned by the + * retry step, not by this flag. + */ +public interface Retryable { + /** Whether the condition this exception represents is a retryable one. */ + public val isRetryable: Boolean +} diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStep.kt new file mode 100644 index 00000000..eb5a0669 --- /dev/null +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStep.kt @@ -0,0 +1,66 @@ +/* + * 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.pipeline.step + +import org.dexpace.sdk.core.http.context.DispatchContext +import org.dexpace.sdk.core.http.response.Response +import org.dexpace.sdk.core.http.response.exception.HttpException +import org.dexpace.sdk.core.http.response.exception.HttpExceptionFactory + +/** + * Response step that turns an HTTP error response into the matching typed [HttpException]. + * + * This is the error path for the recovery-aware pipeline: a transport hands back a `Response` + * for every completed exchange — including 4xx and 5xx — so a 4xx/5xx is a [Response], not a + * thrown exception, until something maps it. Placed in a + * [org.dexpace.sdk.core.pipeline.ResponsePipeline.responseSteps] list, this step performs that + * mapping via [HttpExceptionFactory.fromResponse], so the typed [HttpException] family is + * produced consistently from one seam rather than re-derived ad hoc at each call site. + * + * ## Behaviour + * + * - **2xx (and 1xx/3xx) response** — returned unchanged; the success chain continues. + * - **4xx / 5xx response** — [HttpExceptionFactory.fromResponse] is invoked and the resulting + * [HttpException] is thrown. [org.dexpace.sdk.core.pipeline.ResponsePipeline] catches it, + * closes the in-hand response, and wraps it into a + * [org.dexpace.sdk.core.pipeline.ResponseOutcome.Failure] — which then flows through the + * recovery chain (e.g. [org.dexpace.sdk.core.pipeline.step.retry.RetryStep]) exactly like a + * transport failure. Because the thrown [HttpException] implements + * [org.dexpace.sdk.core.http.response.exception.Retryable], retry classification keys off it + * uniformly. + * + * Range classification matches [HttpExceptionFactory]: only 400..599 maps to an exception, so + * the success / informational / redirection ranges fall through untouched. Mapping the body + * into a typed error value is left to the generated layer (see [HttpException.value]); this + * step intentionally does not consume [Response.body]. + * + * ## Thread-safety + * + * Stateless; safe to share across concurrent requests and reuse as a singleton. + */ +public object ThrowOnHttpErrorStep : ResponsePipelineStep { + private const val ERROR_RANGE_MIN = 400 + private const val ERROR_RANGE_MAX = 599 + + /** + * Returns [input] unchanged for a non-error status; throws the + * [HttpExceptionFactory.fromResponse] mapping for a 4xx / 5xx status. + * + * @throws HttpException when `input.status.code` is in 400..599. + */ + override fun execute( + input: Response, + context: DispatchContext, + ): Response { + val code = input.status.code + if (code in ERROR_RANGE_MIN..ERROR_RANGE_MAX) { + throw HttpExceptionFactory.fromResponse(input) + } + return input + } +} diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStep.kt index 8d50c8ec..fc2ac125 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/retry/RetryStep.kt @@ -12,6 +12,7 @@ import org.dexpace.sdk.core.http.request.Request import org.dexpace.sdk.core.http.response.Response import org.dexpace.sdk.core.http.response.exception.HttpException import org.dexpace.sdk.core.http.response.exception.NetworkException +import org.dexpace.sdk.core.http.response.exception.Retryable import org.dexpace.sdk.core.pipeline.ResponseOutcome import org.dexpace.sdk.core.pipeline.step.ResponseRecoveryStep import java.io.InterruptedIOException @@ -31,14 +32,16 @@ import java.util.concurrent.TimeUnit * * ## Semantics * - * - On a [ResponseOutcome.Failure] whose throwable is an [HttpException] with `retryable = + * Retry eligibility is classified off the [Retryable] interface, not concrete exception types: + * - On a [ResponseOutcome.Failure] whose throwable is an [HttpException] with `isRetryable = * true` AND whose status is in [RetrySettings.retryableStatuses], the step schedules a * retry — provided the request is idempotency-safe (see [canRetry]). - * - On a [ResponseOutcome.Failure] whose throwable is a [NetworkException], the step always - * schedules a retry (network-level failures had no response, so the server never saw the - * request — replay is safe modulo the body-replayability check). - * - On any other outcome (other throwable types or a [ResponseOutcome.Success]), the step - * is a pass-through. + * - On a [ResponseOutcome.Failure] whose throwable is a [NetworkException] (always + * [Retryable.isRetryable]), the step always schedules a retry (network-level failures had no + * response, so the server never saw the request — replay is safe modulo the body- + * replayability check). + * - On any other outcome (a non-[Retryable] throwable, a [Retryable] whose flag is `false`, or + * a [ResponseOutcome.Success]), the step is a pass-through. * * ## Idempotency * @@ -332,15 +335,18 @@ public class RetryStep } /** - * Returns true when [error] is an SDK-classified retryable condition AND, for - * [HttpException], the status is in [RetrySettings.retryableStatuses]. + * Returns true when [error] is an SDK-classified retryable condition. Classification + * keys off the [Retryable] interface rather than concrete exception types: a non- + * [Retryable] throwable is never retried, and a [Retryable] is retried only when its + * [Retryable.isRetryable] flag is set. An [HttpException] carries an additional gate — + * its status must also be in [RetrySettings.retryableStatuses] — so the caller can + * narrow which retryable statuses this step actually re-issues. A [NetworkException] + * (no response, hence no status) passes once its flag is set. */ - private fun isClassifiedRetryable(error: Throwable): Boolean = - when (error) { - is NetworkException -> error.retryable - is HttpException -> error.retryable && settings.retryableStatuses.contains(error.status.code) - else -> false - } + private fun isClassifiedRetryable(error: Throwable): Boolean { + if (error !is Retryable || !error.isRetryable) return false + return error !is HttpException || settings.retryableStatuses.contains(error.status.code) + } /** * Returns true when [request] is safe to retry. A body-less request is retry-safe only diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/util/RetryUtils.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/util/RetryUtils.kt index 51bfdf0e..b31008e5 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/util/RetryUtils.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/util/RetryUtils.kt @@ -11,9 +11,10 @@ import java.io.IOException import java.util.concurrent.TimeoutException /** - * Shared retryability classifier used by both [org.dexpace.sdk.core.http.response.HttpResponseException] - * (to precompute its `isRetryable` flag at construction time) and the default retry - * policy (Rank 4). + * Shared retryability classifier used by both + * [org.dexpace.sdk.core.http.response.exception.HttpException] (to derive its + * [org.dexpace.sdk.core.http.response.exception.Retryable.isRetryable] flag at construction + * time) and the default retry policy (Rank 4). * * Retryable conditions: * - **Status codes**: 408 (Request Timeout), 429 (Too Many Requests), all 5xx except diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/HttpResponseExceptionTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/HttpResponseExceptionTest.kt deleted file mode 100644 index 32377a48..00000000 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/HttpResponseExceptionTest.kt +++ /dev/null @@ -1,163 +0,0 @@ -/* - * 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.http.response - -import org.dexpace.sdk.core.http.common.Protocol -import org.dexpace.sdk.core.http.request.Method -import org.dexpace.sdk.core.http.request.Request -import java.io.IOException -import kotlin.test.Test -import kotlin.test.assertEquals -import kotlin.test.assertFalse -import kotlin.test.assertNull -import kotlin.test.assertSame -import kotlin.test.assertTrue - -class HttpResponseExceptionTest { - @Test - fun `null response and null cause means not retryable`() { - val ex = HttpResponseException("boom", response = null) - assertFalse(ex.isRetryable) - assertNull(ex.response) - assertNull(ex.cause) - assertNull(ex.value) - } - - @Test - fun `200 OK response is not retryable`() { - val ex = HttpResponseException("ok-but-thrown", response = response(Status.OK)) - assertFalse(ex.isRetryable) - } - - @Test - fun `503 Service Unavailable response is retryable`() { - val ex = HttpResponseException("unavailable", response = response(Status.SERVICE_UNAVAILABLE)) - assertTrue(ex.isRetryable) - } - - @Test - fun `501 Not Implemented response is NOT retryable`() { - // 501 is explicitly excluded from the retryable 5xx range. - val ex = HttpResponseException("not impl", response = response(Status.NOT_IMPLEMENTED)) - assertFalse(ex.isRetryable) - } - - @Test - fun `505 HTTP Version Not Supported response is NOT retryable`() { - val ex = HttpResponseException("bad version", response = response(Status.HTTP_VERSION_NOT_SUPPORTED)) - assertFalse(ex.isRetryable) - } - - @Test - fun `cause is IOException with null response is retryable`() { - val cause = IOException("network") - val ex = HttpResponseException("io failed", response = null, cause = cause) - assertTrue(ex.isRetryable) - assertSame(cause, ex.cause) - } - - @Test - fun `cause chain wraps IOException is retryable`() { - val root = IOException("root") - val middle = RuntimeException("middle", root) - val outer = IllegalStateException("outer", middle) - val ex = HttpResponseException("nested", response = null, cause = outer) - assertTrue(ex.isRetryable) - } - - @Test - fun `response takes precedence over cause when response status is non-retryable`() { - // Response is 200 (not retryable) but cause is IOException (would be retryable). - // Response wins because it is non-null — matches the spec's response-first rule. - val ex = - HttpResponseException( - "ok with io cause", - response = response(Status.OK), - cause = IOException("io"), - ) - assertFalse(ex.isRetryable) - } - - @Test - fun `self-referential cause chain does not infinite-loop`() { - val selfCausing = SelfReferentialException("loop") - selfCausing.setSelfCause() - // Construction must terminate; isRetryable must be false (no IOException in the chain). - val ex = HttpResponseException("self loop", response = null, cause = selfCausing) - assertFalse(ex.isRetryable) - } - - @Test - fun `value parameter is preserved`() { - val payload = mapOf("error" to "rate-limited") - val ex = - HttpResponseException( - "payload", - response = response(Status.TOO_MANY_REQUESTS), - value = payload, - ) - assertSame(payload, ex.value) - assertTrue(ex.isRetryable) // 429 is retryable. - } - - @Test - fun `extends IOException`() { - val ex = HttpResponseException("io", response = null) - // Catchable by IOException — the whole point of inheriting from it. - val asIo: IOException = ex - assertEquals("io", asIo.message) - } - - @Test - fun `message and cause are propagated to IOException constructor`() { - val cause = RuntimeException("root") - val ex = HttpResponseException("oops", response = null, cause = cause) - assertEquals("oops", ex.message) - assertSame(cause, ex.cause) - } - - @Test - fun `subclasses can be defined and inherit isRetryable behavior`() { - class TypedHttpResponseException(message: String, response: Response?, cause: Throwable?) : - HttpResponseException(message, response, value = null, cause = cause) - - val ex = - TypedHttpResponseException( - "typed", - response = response(Status.SERVICE_UNAVAILABLE), - cause = null, - ) - assertTrue(ex.isRetryable) - } - - // ---- Helpers ----------------------------------------------------------------------- - - private fun response(status: Status): Response { - val req = - Request.builder() - .method(Method.GET) - .url("https://api.example.com/") - .build() - return Response.builder() - .request(req) - .protocol(Protocol.HTTP_1_1) - .status(status) - .build() - } - - private class SelfReferentialException(message: String) : RuntimeException(message) { - private var selfCause: Boolean = false - - fun setSelfCause() { - selfCause = true - } - - override val cause: Throwable? - get() = if (selfCause) this else null - } -} diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptionFactoryTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptionFactoryTest.kt index 2e5dfd1b..eb18d209 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptionFactoryTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptionFactoryTest.kt @@ -51,7 +51,7 @@ class HttpExceptionFactoryTest { ex.javaClass, "factory must dispatch status ${status.code} (${status.statusName}) to ${expectedClass.simpleName}", ) - assertEquals(expectedRetryable, ex.retryable, "${status.code} retryable flag mismatch") + assertEquals(expectedRetryable, ex.isRetryable, "${status.code} retryable flag mismatch") assertEquals(status, ex.status) } } @@ -64,11 +64,11 @@ class HttpExceptionFactoryTest { // should fall through to the 4xx generic. Same for the auth-handshake-style 407. val teapot = HttpExceptionFactory.fromResponse(response(Status.IM_A_TEAPOT)) assertTrue(teapot is ClientErrorException, "418 should be ClientErrorException") - assertEquals(false, teapot.retryable) + assertEquals(false, teapot.isRetryable) val proxyAuth = HttpExceptionFactory.fromResponse(response(Status.PROXY_AUTHENTICATION_REQUIRED)) assertTrue(proxyAuth is ClientErrorException, "407 should be ClientErrorException") - assertEquals(false, proxyAuth.retryable) + assertEquals(false, proxyAuth.isRetryable) } @Test @@ -77,21 +77,21 @@ class HttpExceptionFactoryTest { // RetryUtils (which excludes 501/505) — NOT the old blanket "5xx is retryable" rule. val notImpl = HttpExceptionFactory.fromResponse(response(Status.NOT_IMPLEMENTED)) assertTrue(notImpl is ServerErrorException, "501 should be ServerErrorException") - assertEquals(false, notImpl.retryable, "501 must not be retryable (RetryUtils excludes it)") - assertEquals(RetryUtils.isRetryable(Status.NOT_IMPLEMENTED.code), notImpl.retryable) + assertEquals(false, notImpl.isRetryable, "501 must not be retryable (RetryUtils excludes it)") + assertEquals(RetryUtils.isRetryable(Status.NOT_IMPLEMENTED.code), notImpl.isRetryable) val versionBad = HttpExceptionFactory.fromResponse(response(Status.HTTP_VERSION_NOT_SUPPORTED)) assertTrue(versionBad is ServerErrorException, "505 should be ServerErrorException") - assertEquals(false, versionBad.retryable, "505 must not be retryable (RetryUtils excludes it)") - assertEquals(RetryUtils.isRetryable(Status.HTTP_VERSION_NOT_SUPPORTED.code), versionBad.retryable) + assertEquals(false, versionBad.isRetryable, "505 must not be retryable (RetryUtils excludes it)") + assertEquals(RetryUtils.isRetryable(Status.HTTP_VERSION_NOT_SUPPORTED.code), versionBad.isRetryable) } @Test fun `408 maps to RequestTimeoutException and is retryable`() { val ex = HttpExceptionFactory.fromResponse(response(Status.REQUEST_TIMEOUT)) assertTrue(ex is RequestTimeoutException, "408 should map to RequestTimeoutException, not the 4xx fallback") - assertEquals(true, ex.retryable, "408 must be retryable per RetryUtils") - assertEquals(RetryUtils.isRetryable(Status.REQUEST_TIMEOUT.code), ex.retryable) + assertEquals(true, ex.isRetryable, "408 must be retryable per RetryUtils") + assertEquals(RetryUtils.isRetryable(Status.REQUEST_TIMEOUT.code), ex.isRetryable) assertEquals(Status.REQUEST_TIMEOUT, ex.status) } @@ -130,7 +130,7 @@ class HttpExceptionFactoryTest { val ex = HttpExceptionFactory.fromResponse(response(status)) assertEquals( RetryUtils.isRetryable(status.code), - ex.retryable, + ex.isRetryable, "baked retryable for ${status.code} (${status.statusName}) must equal RetryUtils.isRetryable", ) } @@ -160,36 +160,72 @@ class HttpExceptionFactoryTest { @Test fun `retryable flag matches the canonical retryable status set`() { // 4xx subclasses except 429 — not retryable. - assertEquals(false, BadRequestException(response(Status.BAD_REQUEST)).retryable) - assertEquals(false, UnauthorizedException(response(Status.UNAUTHORIZED)).retryable) - assertEquals(false, ForbiddenException(response(Status.FORBIDDEN)).retryable) - assertEquals(false, NotFoundException(response(Status.NOT_FOUND)).retryable) - assertEquals(false, MethodNotAllowedException(response(Status.METHOD_NOT_ALLOWED)).retryable) - assertEquals(false, ConflictException(response(Status.CONFLICT)).retryable) - assertEquals(false, GoneException(response(Status.GONE)).retryable) - assertEquals(false, PayloadTooLargeException(response(Status.PAYLOAD_TOO_LARGE)).retryable) - assertEquals(false, UnsupportedMediaTypeException(response(Status.UNSUPPORTED_MEDIA_TYPE)).retryable) - assertEquals(false, UnprocessableEntityException(response(Status.UNPROCESSABLE_ENTITY)).retryable) - assertEquals(false, ClientErrorException(response(Status.IM_A_TEAPOT)).retryable) + assertEquals(false, BadRequestException(response(Status.BAD_REQUEST)).isRetryable) + assertEquals(false, UnauthorizedException(response(Status.UNAUTHORIZED)).isRetryable) + assertEquals(false, ForbiddenException(response(Status.FORBIDDEN)).isRetryable) + assertEquals(false, NotFoundException(response(Status.NOT_FOUND)).isRetryable) + assertEquals(false, MethodNotAllowedException(response(Status.METHOD_NOT_ALLOWED)).isRetryable) + assertEquals(false, ConflictException(response(Status.CONFLICT)).isRetryable) + assertEquals(false, GoneException(response(Status.GONE)).isRetryable) + assertEquals(false, PayloadTooLargeException(response(Status.PAYLOAD_TOO_LARGE)).isRetryable) + assertEquals(false, UnsupportedMediaTypeException(response(Status.UNSUPPORTED_MEDIA_TYPE)).isRetryable) + assertEquals(false, UnprocessableEntityException(response(Status.UNPROCESSABLE_ENTITY)).isRetryable) + assertEquals(false, ClientErrorException(response(Status.IM_A_TEAPOT)).isRetryable) // 408 + 429 + the retryable 5xx codes — retryable. - assertEquals(true, RequestTimeoutException(response(Status.REQUEST_TIMEOUT)).retryable) - assertEquals(true, TooManyRequestsException(response(Status.TOO_MANY_REQUESTS)).retryable) - assertEquals(true, InternalServerErrorException(response(Status.INTERNAL_SERVER_ERROR)).retryable) - assertEquals(true, BadGatewayException(response(Status.BAD_GATEWAY)).retryable) - assertEquals(true, ServiceUnavailableException(response(Status.SERVICE_UNAVAILABLE)).retryable) - assertEquals(true, GatewayTimeoutException(response(Status.GATEWAY_TIMEOUT)).retryable) + assertEquals(true, RequestTimeoutException(response(Status.REQUEST_TIMEOUT)).isRetryable) + assertEquals(true, TooManyRequestsException(response(Status.TOO_MANY_REQUESTS)).isRetryable) + assertEquals(true, InternalServerErrorException(response(Status.INTERNAL_SERVER_ERROR)).isRetryable) + assertEquals(true, BadGatewayException(response(Status.BAD_GATEWAY)).isRetryable) + assertEquals(true, ServiceUnavailableException(response(Status.SERVICE_UNAVAILABLE)).isRetryable) + assertEquals(true, GatewayTimeoutException(response(Status.GATEWAY_TIMEOUT)).isRetryable) // 501 routes through the 5xx fallback but is NOT retryable — the baked flag now // mirrors RetryUtils instead of the old blanket 5xx rule. - assertEquals(false, ServerErrorException(response(Status.NOT_IMPLEMENTED)).retryable) - assertEquals(false, ServerErrorException(response(Status.HTTP_VERSION_NOT_SUPPORTED)).retryable) + assertEquals(false, ServerErrorException(response(Status.NOT_IMPLEMENTED)).isRetryable) + assertEquals(false, ServerErrorException(response(Status.HTTP_VERSION_NOT_SUPPORTED)).isRetryable) } @Test fun `NetworkException is always retryable`() { val ex = NetworkException("connect refused") - assertEquals(true, ex.retryable) + assertEquals(true, ex.isRetryable) + } + + // ---- 4b. Retryable interface + error-body value slot -------------------------------- + + @Test + fun `HttpException and NetworkException both implement Retryable`() { + val http: Throwable = HttpExceptionFactory.fromResponse(response(Status.SERVICE_UNAVAILABLE)) + val net: Throwable = NetworkException("connect failed") + // Both surface their retry classification through the shared Retryable seam, so the + // retry classifier can key off the interface instead of matching concrete types. + assertTrue(Retryable::class.java.isInstance(http), "HttpException must implement Retryable") + assertTrue(Retryable::class.java.isInstance(net), "NetworkException must implement Retryable") + assertEquals(true, (http as Retryable).isRetryable) + assertEquals(true, (net as Retryable).isRetryable) + } + + @Test + fun `factory-produced exception carries a null value slot by default`() { + // The deserialized error-body slot exists for the generated layer to populate; the + // factory leaves it null because sdk-core does not parse bodies. + val ex = HttpExceptionFactory.fromResponse(response(Status.BAD_REQUEST)) + assertNull(ex.value, "value slot must default to null") + } + + @Test + fun `value slot round-trips a deserialized error payload`() { + val payload = mapOf("code" to "rate_limited", "message" to "slow down") + val ex = + object : HttpException( + status = Status.TOO_MANY_REQUESTS, + headers = Headers.Builder().build(), + body = null, + value = payload, + ) {} + assertEquals(payload, ex.value) + assertEquals(true, ex.isRetryable, "429 is retryable") } // ---- 5. bodySnapshot() behavior ----------------------------------------------------- diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStepTest.kt new file mode 100644 index 00000000..7e4d31e7 --- /dev/null +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStepTest.kt @@ -0,0 +1,143 @@ +/* + * 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.pipeline.step + +import org.dexpace.sdk.core.http.common.Headers +import org.dexpace.sdk.core.http.common.Protocol +import org.dexpace.sdk.core.http.context.DispatchContext +import org.dexpace.sdk.core.http.request.Method +import org.dexpace.sdk.core.http.request.Request +import org.dexpace.sdk.core.http.response.Response +import org.dexpace.sdk.core.http.response.Status +import org.dexpace.sdk.core.http.response.exception.HttpException +import org.dexpace.sdk.core.http.response.exception.NotFoundException +import org.dexpace.sdk.core.http.response.exception.Retryable +import org.dexpace.sdk.core.http.response.exception.ServiceUnavailableException +import org.dexpace.sdk.core.http.response.exception.TooManyRequestsException +import org.dexpace.sdk.core.pipeline.ResponseOutcome +import org.dexpace.sdk.core.pipeline.ResponsePipeline +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertIs +import kotlin.test.assertSame +import kotlin.test.assertTrue + +/** + * Verifies that [ThrowOnHttpErrorStep] is the live error path: an error response is routed + * through [org.dexpace.sdk.core.http.response.exception.HttpExceptionFactory.fromResponse] and + * surfaces as the matching typed [HttpException], while a success response passes through. + */ +class ThrowOnHttpErrorStepTest { + // ---- the step in isolation ---------------------------------------------------------- + + @Test + fun `2xx response passes through unchanged`() { + val ok = response(Status.OK) + val out = ThrowOnHttpErrorStep.execute(ok, ctx()) + assertSame(ok, out, "a success response must pass through untouched") + } + + @Test + fun `3xx response passes through unchanged`() { + // The factory rejects non-4xx/5xx; the step must not even call it for a 3xx. + val redirect = response(Status.MOVED_PERMANENTLY) + assertSame(redirect, ThrowOnHttpErrorStep.execute(redirect, ctx())) + } + + @Test + fun `404 maps to NotFoundException via the factory`() { + val ex = + assertFailsWith { + ThrowOnHttpErrorStep.execute(response(Status.NOT_FOUND), ctx()) + } + assertIs(ex, "404 must produce the factory's NotFoundException") + assertEquals(Status.NOT_FOUND, ex.status) + } + + @Test + fun `503 maps to a retryable ServiceUnavailableException`() { + val ex = + assertFailsWith { + ThrowOnHttpErrorStep.execute(response(Status.SERVICE_UNAVAILABLE), ctx()) + } + assertIs(ex) + assertTrue(ex.isRetryable, "503 must surface as retryable") + } + + @Test + fun `error exception carries the response headers and status`() { + val headers = Headers.Builder().add("Retry-After", "7").build() + val ex = + assertFailsWith { + ThrowOnHttpErrorStep.execute(response(Status.TOO_MANY_REQUESTS, headers), ctx()) + } + assertIs(ex) + assertEquals("7", ex.headers.get("Retry-After")) + assertEquals(Status.TOO_MANY_REQUESTS, ex.status) + } + + // ---- wired through the recovery-aware ResponsePipeline ------------------------------ + + @Test + fun `pipeline turns a 500 success-outcome into a Failure carrying the factory exception`() { + val pipeline = ResponsePipeline(responseSteps = listOf(ThrowOnHttpErrorStep)) + val outcome = ResponseOutcome.Success(response(Status.INTERNAL_SERVER_ERROR)) + + val result = pipeline.apply(outcome, ctx()) + + val failure = assertIs(result, "a 5xx must become a Failure") + val error = assertIs(failure.error) + assertEquals(Status.INTERNAL_SERVER_ERROR, error.status) + } + + @Test + fun `pipeline leaves a 2xx success-outcome as a Success`() { + val pipeline = ResponsePipeline(responseSteps = listOf(ThrowOnHttpErrorStep)) + val ok = response(Status.OK) + + val result = pipeline.apply(ResponseOutcome.Success(ok), ctx()) + + val success = assertIs(result) + assertSame(ok, success.response) + } + + @Test + fun `the mapped failure is Retryable so the retry classifier can key off it`() { + // End-to-end check of the #23 / #37 seam: the error path emits an HttpException, and + // that exception is Retryable, which is exactly what RetryStep.isClassifiedRetryable + // matches on. + val pipeline = ResponsePipeline(responseSteps = listOf(ThrowOnHttpErrorStep)) + val result = pipeline.apply(ResponseOutcome.Success(response(Status.BAD_GATEWAY)), ctx()) + + val failure = assertIs(result) + val retryable = assertIs(failure.error) + assertTrue(retryable.isRetryable, "502 must classify as retryable through the interface") + } + + // ---- helpers ------------------------------------------------------------------------ + + private fun ctx(): DispatchContext = DispatchContext.default() + + private fun request(): Request = + Request.builder() + .url("https://api.example.com/resource") + .method(Method.GET) + .build() + + private fun response( + status: Status, + headers: Headers = Headers.Builder().build(), + ): Response = + Response.builder() + .request(request()) + .protocol(Protocol.HTTP_1_1) + .status(status) + .headers(headers) + .build() +} From cf7e4989976bc9bb45214ec7678b8fe70e796379 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 16 Jun 2026 22:32:08 +0300 Subject: [PATCH 2/3] fix: keep HTTP error bodies readable and forward the value slot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ThrowOnHttpErrorStep built the exception from the live response, but the pipeline closes that response (and its body) as soon as the step throws, so the error body was unreadable on the resulting Failure — contradicting the bodySnapshot and stream-on-demand contract. Buffer the error body into a bounded, replayable in-memory body before constructing the exception, capped at HttpException.DEFAULT_SNAPSHOT_BYTES so a large 5xx body cannot exhaust memory. Add a test that the failure's body and bodySnapshot are still readable after the pipeline closes the original. Forward the value slot through the concrete HttpException subclasses so a codegen subclass derived from a concrete type (as the base KDoc describes) can stamp a typed payload; previously only the abstract base accepted it. Single-source the 400..599 error boundary by adding isErrorStatus and fromResponseOrNull to HttpExceptionFactory and having the step use them, instead of redeclaring the range. Tighten the value round-trip test to assertSame, reword a test comment off tracker IDs, and soften the docs so they describe ThrowOnHttpErrorStep as a building block rather than implying a default pipeline wires it. Regenerate the API snapshot. --- docs/http.md | 12 +-- sdk-core/api/sdk-core.api | 56 ++++++++---- .../exception/HttpExceptionFactory.kt | 20 +++++ .../http/response/exception/HttpExceptions.kt | 36 ++++++++ .../pipeline/step/ThrowOnHttpErrorStep.kt | 85 ++++++++++++++++--- .../exception/HttpExceptionFactoryTest.kt | 3 +- .../pipeline/step/ThrowOnHttpErrorStepTest.kt | 84 +++++++++++++++--- 7 files changed, 251 insertions(+), 45 deletions(-) diff --git a/docs/http.md b/docs/http.md index 769ef512..18ae9fe9 100644 --- a/docs/http.md +++ b/docs/http.md @@ -387,11 +387,13 @@ if (!response.status.isSuccess) { The factory throws `IllegalArgumentException` if called with a status outside 400..599 — 1xx/2xx/3xx outcomes are not exceptions and should not be funneled through this path. -In the recovery-aware pipeline this is wired through `ThrowOnHttpErrorStep`, a -`ResponsePipelineStep` that calls `fromResponse` on a 4xx/5xx response and throws the result. -`ResponsePipeline` converts that throw into a `ResponseOutcome.Failure`, which then flows -through the recovery chain (e.g. `RetryStep`) exactly like a transport failure — and because -the thrown `HttpException` is `Retryable`, retry classification keys off it uniformly. +For the recovery-aware pipeline, `ThrowOnHttpErrorStep` packages this mapping as a +`ResponsePipelineStep`: drop it into a `ResponsePipeline.responseSteps` list and it calls +`fromResponse` on a 4xx/5xx response and throws the result. `ResponsePipeline` converts that +throw into a `ResponseOutcome.Failure`, which then flows through the recovery chain (e.g. +`RetryStep`) exactly like a transport failure — and because the thrown `HttpException` is +`Retryable`, retry classification keys off it uniformly. The step is a building block; no +default pipeline in `sdk-core` assembles it for you. --- diff --git a/sdk-core/api/sdk-core.api b/sdk-core/api/sdk-core.api index b855b5f7..9da3b00e 100644 --- a/sdk-core/api/sdk-core.api +++ b/sdk-core/api/sdk-core.api @@ -1264,49 +1264,56 @@ public class org/dexpace/sdk/core/http/response/exception/BadGatewayException : public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public class org/dexpace/sdk/core/http/response/exception/BadRequestException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public class org/dexpace/sdk/core/http/response/exception/ClientErrorException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public class org/dexpace/sdk/core/http/response/exception/ConflictException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public class org/dexpace/sdk/core/http/response/exception/ForbiddenException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public class org/dexpace/sdk/core/http/response/exception/GatewayTimeoutException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public class org/dexpace/sdk/core/http/response/exception/GoneException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public abstract class org/dexpace/sdk/core/http/response/exception/HttpException : java/lang/RuntimeException, org/dexpace/sdk/core/http/response/exception/Retryable { @@ -1333,20 +1340,24 @@ public final class org/dexpace/sdk/core/http/response/exception/HttpException$Co public final class org/dexpace/sdk/core/http/response/exception/HttpExceptionFactory { public static final field INSTANCE Lorg/dexpace/sdk/core/http/response/exception/HttpExceptionFactory; public static final fun fromResponse (Lorg/dexpace/sdk/core/http/response/Response;)Lorg/dexpace/sdk/core/http/response/exception/HttpException; + public static final fun fromResponseOrNull (Lorg/dexpace/sdk/core/http/response/Response;)Lorg/dexpace/sdk/core/http/response/exception/HttpException; + public static final fun isErrorStatus (I)Z } public class org/dexpace/sdk/core/http/response/exception/InternalServerErrorException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public class org/dexpace/sdk/core/http/response/exception/MethodNotAllowedException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public class org/dexpace/sdk/core/http/response/exception/NetworkException : java/io/IOException, org/dexpace/sdk/core/http/response/exception/Retryable { @@ -1361,21 +1372,24 @@ public class org/dexpace/sdk/core/http/response/exception/NotFoundException : or public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public class org/dexpace/sdk/core/http/response/exception/PayloadTooLargeException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public class org/dexpace/sdk/core/http/response/exception/RequestTimeoutException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public abstract interface class org/dexpace/sdk/core/http/response/exception/Retryable { @@ -1386,42 +1400,48 @@ public class org/dexpace/sdk/core/http/response/exception/ServerErrorException : public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public class org/dexpace/sdk/core/http/response/exception/ServiceUnavailableException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public class org/dexpace/sdk/core/http/response/exception/TooManyRequestsException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public class org/dexpace/sdk/core/http/response/exception/UnauthorizedException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public class org/dexpace/sdk/core/http/response/exception/UnprocessableEntityException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public class org/dexpace/sdk/core/http/response/exception/UnsupportedMediaTypeException : org/dexpace/sdk/core/http/response/exception/HttpException { public fun (Lorg/dexpace/sdk/core/http/response/Response;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;)V - public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;)V + public synthetic fun (Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/Throwable;Ljava/lang/Object;ILkotlin/jvm/internal/DefaultConstructorMarker;)V } public final class org/dexpace/sdk/core/http/sse/ServerSentEvent { diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptionFactory.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptionFactory.kt index 3b58869e..4c912bd4 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptionFactory.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptionFactory.kt @@ -65,6 +65,26 @@ public object HttpExceptionFactory { private const val SC_SERVER_ERROR_MIN = 500 private const val SC_SERVER_ERROR_MAX = 599 + /** + * Whether [code] is in the 400..599 error range this factory maps to an [HttpException]. + * + * This is the single source of truth for the error-status boundary; callers that want to + * pre-check before mapping (e.g. a pipeline step that only acts on error responses) should + * use this rather than re-declaring the range. + */ + @JvmStatic + public fun isErrorStatus(code: Int): Boolean = code in SC_CLIENT_ERROR_MIN..SC_SERVER_ERROR_MAX + + /** + * Maps [response] to its [HttpException] subclass, or returns `null` when the status is not + * an error (1xx / 2xx / 3xx). Unlike [fromResponse], a non-error status is a no-op rather + * than an [IllegalArgumentException], so this is the convenient form for "map it if it is + * an error" call sites. + */ + @JvmStatic + public fun fromResponseOrNull(response: Response): HttpException? = + if (isErrorStatus(response.status.code)) fromResponse(response) else null + /** * Returns the [HttpException] subclass that corresponds to [response]'s status code. * diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptions.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptions.kt index 06c2bec4..4db3194c 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptions.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptions.kt @@ -35,12 +35,14 @@ public open class BadRequestException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -54,12 +56,14 @@ public open class UnauthorizedException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -73,12 +77,14 @@ public open class ForbiddenException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -91,12 +97,14 @@ public open class NotFoundException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -109,12 +117,14 @@ public open class MethodNotAllowedException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -128,12 +138,14 @@ public open class RequestTimeoutException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -147,12 +159,14 @@ public open class ConflictException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -165,12 +179,14 @@ public open class GoneException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -183,12 +199,14 @@ public open class PayloadTooLargeException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -201,12 +219,14 @@ public open class UnsupportedMediaTypeException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -220,12 +240,14 @@ public open class UnprocessableEntityException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -239,12 +261,14 @@ public open class TooManyRequestsException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -258,12 +282,14 @@ public open class InternalServerErrorException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -276,12 +302,14 @@ public open class BadGatewayException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -295,12 +323,14 @@ public open class ServiceUnavailableException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -313,12 +343,14 @@ public open class GatewayTimeoutException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -334,12 +366,14 @@ public open class ClientErrorException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) /** @@ -355,10 +389,12 @@ public open class ServerErrorException response: Response, message: String? = null, cause: Throwable? = null, + value: Any? = null, ) : HttpException( status = response.status, headers = response.headers, body = response.body, message = message, cause = cause, + value = value, ) diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStep.kt index eb5a0669..215c5e97 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStep.kt @@ -9,8 +9,11 @@ package org.dexpace.sdk.core.pipeline.step import org.dexpace.sdk.core.http.context.DispatchContext import org.dexpace.sdk.core.http.response.Response +import org.dexpace.sdk.core.http.response.ResponseBody import org.dexpace.sdk.core.http.response.exception.HttpException import org.dexpace.sdk.core.http.response.exception.HttpExceptionFactory +import org.dexpace.sdk.core.io.BufferedSource +import org.dexpace.sdk.core.io.Io /** * Response step that turns an HTTP error response into the matching typed [HttpException]. @@ -34,19 +37,26 @@ import org.dexpace.sdk.core.http.response.exception.HttpExceptionFactory * [org.dexpace.sdk.core.http.response.exception.Retryable], retry classification keys off it * uniformly. * - * Range classification matches [HttpExceptionFactory]: only 400..599 maps to an exception, so - * the success / informational / redirection ranges fall through untouched. Mapping the body - * into a typed error value is left to the generated layer (see [HttpException.value]); this - * step intentionally does not consume [Response.body]. + * Range classification is single-sourced from [HttpExceptionFactory.isErrorStatus]: only + * 400..599 maps to an exception, so the success / informational / redirection ranges fall + * through untouched. + * + * ## Error-body buffering + * + * The pipeline closes the in-hand response as soon as this step throws (close-before-propagate), + * which would also close the live [ResponseBody]. 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. The + * buffer is bounded at [HttpException.DEFAULT_SNAPSHOT_BYTES], consistent with the + * `bodySnapshot` cap, 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]). * * ## Thread-safety * * Stateless; safe to share across concurrent requests and reuse as a singleton. */ public object ThrowOnHttpErrorStep : ResponsePipelineStep { - private const val ERROR_RANGE_MIN = 400 - private const val ERROR_RANGE_MAX = 599 - /** * Returns [input] unchanged for a non-error status; throws the * [HttpExceptionFactory.fromResponse] mapping for a 4xx / 5xx status. @@ -57,10 +67,63 @@ public object ThrowOnHttpErrorStep : ResponsePipelineStep { input: Response, context: DispatchContext, ): Response { - val code = input.status.code - if (code in ERROR_RANGE_MIN..ERROR_RANGE_MAX) { - throw HttpExceptionFactory.fromResponse(input) + if (!HttpExceptionFactory.isErrorStatus(input.status.code)) { + return input + } + throw HttpExceptionFactory.fromResponse(bufferErrorBody(input)) + } + + /** + * Returns a copy of [response] whose body has been drained into a bounded, replayable + * in-memory body, so it survives the pipeline closing the original response. Bodies that are + * absent are left as-is. + */ + private fun bufferErrorBody(response: Response): Response { + val body = response.body ?: return response + val buffered = replayableBody(body) + return response.newBuilder().body(buffered).build() + } + + /** + * Reads up to [HttpException.DEFAULT_SNAPSHOT_BYTES] from [body] into memory and returns a + * [ResponseBody] that can be read repeatedly from those bytes. The original [body] is read + * here (the byte budget is bounded), so the caller must not also stream it. + */ + private fun replayableBody(body: ResponseBody): ResponseBody { + val cap = HttpException.DEFAULT_SNAPSHOT_BYTES + val mediaType = body.mediaType() + val bytes = + body.source().use { source -> + readUpTo(source, cap) + } + return object : ResponseBody() { + override fun mediaType() = mediaType + + override fun contentLength(): Long = bytes.size.toLong() + + override fun source(): BufferedSource = Io.provider.source(bytes) + + override fun close() { + // Bytes are held in memory; nothing transport-owned to release. + } + } + } + + /** Reads at most [cap] bytes from [source], returning however many were available. */ + private fun readUpTo( + source: BufferedSource, + cap: Int, + ): ByteArray { + if (cap == 0) return ByteArray(0) + val out = ByteArray(cap) + var read = 0 + source.inputStream().use { stream -> + while (read < cap) { + val n = stream.read(out, read, cap - read) + if (n < 0) break + read += n + } } - return input + return if (read == cap) out else out.copyOf(read) } } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptionFactoryTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptionFactoryTest.kt index eb18d209..d05ef86e 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptionFactoryTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/response/exception/HttpExceptionFactoryTest.kt @@ -27,6 +27,7 @@ import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertNotNull import kotlin.test.assertNull +import kotlin.test.assertSame import kotlin.test.assertTrue class HttpExceptionFactoryTest { @@ -224,7 +225,7 @@ class HttpExceptionFactoryTest { body = null, value = payload, ) {} - assertEquals(payload, ex.value) + assertSame(payload, ex.value, "value must be carried through as the same instance") assertEquals(true, ex.isRetryable, "429 is retryable") } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStepTest.kt index 7e4d31e7..5938923f 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStepTest.kt @@ -13,18 +13,24 @@ import org.dexpace.sdk.core.http.context.DispatchContext import org.dexpace.sdk.core.http.request.Method import org.dexpace.sdk.core.http.request.Request import org.dexpace.sdk.core.http.response.Response +import org.dexpace.sdk.core.http.response.ResponseBody import org.dexpace.sdk.core.http.response.Status import org.dexpace.sdk.core.http.response.exception.HttpException import org.dexpace.sdk.core.http.response.exception.NotFoundException import org.dexpace.sdk.core.http.response.exception.Retryable import org.dexpace.sdk.core.http.response.exception.ServiceUnavailableException import org.dexpace.sdk.core.http.response.exception.TooManyRequestsException +import org.dexpace.sdk.core.io.BufferedSource +import org.dexpace.sdk.core.io.Io import org.dexpace.sdk.core.pipeline.ResponseOutcome import org.dexpace.sdk.core.pipeline.ResponsePipeline +import org.dexpace.sdk.io.OkioIoProvider +import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertIs +import kotlin.test.assertNotNull import kotlin.test.assertSame import kotlin.test.assertTrue @@ -34,6 +40,11 @@ import kotlin.test.assertTrue * surfaces as the matching typed [HttpException], while a success response passes through. */ class ThrowOnHttpErrorStepTest { + @BeforeTest + fun installProvider() { + Io.installProvider(OkioIoProvider) + } + // ---- the step in isolation ---------------------------------------------------------- @Test @@ -109,9 +120,8 @@ class ThrowOnHttpErrorStepTest { @Test fun `the mapped failure is Retryable so the retry classifier can key off it`() { - // End-to-end check of the #23 / #37 seam: the error path emits an HttpException, and - // that exception is Retryable, which is exactly what RetryStep.isClassifiedRetryable - // matches on. + // The error path emits an HttpException that is Retryable, which is exactly what + // RetryStep.isClassifiedRetryable matches on. val pipeline = ResponsePipeline(responseSteps = listOf(ThrowOnHttpErrorStep)) val result = pipeline.apply(ResponseOutcome.Success(response(Status.BAD_GATEWAY)), ctx()) @@ -120,10 +130,60 @@ class ThrowOnHttpErrorStepTest { assertTrue(retryable.isRetryable, "502 must classify as retryable through the interface") } + @Test + fun `error body stays readable on the Failure after the pipeline closes the response`() { + // The pipeline closes the in-hand response as soon as the step throws. The step buffers + // the error body first, so bodySnapshot()/string() still work on the thrown exception + // even though the original transport body is closed by close-before-propagate. + val payload = """{"error":"boom"}""" + val original = closeTrackingBody(payload) + val pipeline = ResponsePipeline(responseSteps = listOf(ThrowOnHttpErrorStep)) + + val result = + pipeline.apply( + ResponseOutcome.Success(response(Status.INTERNAL_SERVER_ERROR, body = original)), + ctx(), + ) + + assertTrue(original.closed, "the pipeline must close the original transport body") + val failure = assertIs(result) + val error = assertIs(failure.error) + + val snapshot = assertNotNull(error.bodySnapshot(), "buffered body must be snapshot-able") + assertEquals(payload, String(snapshot, Charsets.UTF_8)) + // Replayable: the buffered body can be read again as the primary stream. + val streamed = error.body!!.source().use { it.readUtf8() } + assertEquals(payload, streamed) + } + // ---- helpers ------------------------------------------------------------------------ private fun ctx(): DispatchContext = DispatchContext.default() + /** + * A response body that records when it is closed, so a test can prove the pipeline closed + * the original transport body while the buffered copy on the exception remains readable. + */ + private class CloseTrackingBody( + private val bytes: ByteArray, + ) : ResponseBody() { + var closed: Boolean = false + private set + + override fun mediaType() = null + + override fun contentLength(): Long = bytes.size.toLong() + + override fun source(): BufferedSource = Io.provider.source(bytes) + + override fun close() { + closed = true + } + } + + private fun closeTrackingBody(content: String): CloseTrackingBody = + CloseTrackingBody(content.toByteArray(Charsets.UTF_8)) + private fun request(): Request = Request.builder() .url("https://api.example.com/resource") @@ -133,11 +193,15 @@ class ThrowOnHttpErrorStepTest { private fun response( status: Status, headers: Headers = Headers.Builder().build(), - ): Response = - Response.builder() - .request(request()) - .protocol(Protocol.HTTP_1_1) - .status(status) - .headers(headers) - .build() + body: ResponseBody? = null, + ): Response { + val builder = + Response.builder() + .request(request()) + .protocol(Protocol.HTTP_1_1) + .status(status) + .headers(headers) + if (body != null) builder.body(body) + return builder.build() + } } From 5e59e7bccc52ec8074f0336bcb206d9b06965cd8 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Tue, 16 Jun 2026 23:54:38 +0300 Subject: [PATCH 3/3] docs: flag error-body truncation in ThrowOnHttpErrorStep The buffered error body is capped at DEFAULT_SNAPSHOT_BYTES; an oversized body is hard-truncated with no marker. Document that downstream consumers deserializing the buffered body must tolerate a structurally incomplete payload, and that callers needing the full body must read it before this step runs. --- .../sdk/core/pipeline/step/ThrowOnHttpErrorStep.kt | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStep.kt index 215c5e97..21326999 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pipeline/step/ThrowOnHttpErrorStep.kt @@ -52,6 +52,14 @@ import org.dexpace.sdk.core.io.Io * `bodySnapshot` cap, 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]). * + * 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 + * 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 + * mid-token will not parse. Callers that need the full error body must read it from the live + * response before this step runs rather than relying on the buffered copy. + * * ## Thread-safety * * Stateless; safe to share across concurrent requests and reuse as a singleton.