refactor: unify the HTTP exception hierarchy and route errors through the factory (#100)

PR: #100

Author: OmarAlJarrah

README.md

@@ -247,8 +247,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`, plus the raw-vs-parsed seam: `ResponseHandler<T>` (with dep-free `string()`/`empty()` handlers) and a lazy, parse-once `ParsedResponse<T>`. |
-| `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`), plus the raw-vs-parsed seam: `ResponseHandler<T>` (with dep-free `string()`/`empty()` handlers) and a lazy, parse-once `ParsedResponse<T>`. |
+| `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`. |

docs/architecture.md

@@ -582,8 +582,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
@@ -660,8 +660,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                     |

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,14 @@ 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.
 
+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.
+
 ---
 
 ## Common Types

docs/pipelines.md

@@ -356,11 +356,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, keyed off whether the request carries
 a body. A request **with a body** is eligible only when its body is replayable (a non-replayable

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.