From 2f7d2886adaa918f834286d50e925fb678eddbf4 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 28 Jun 2026 17:15:25 +0300 Subject: [PATCH 1/7] feat: add QueryParams multimap and OperationParams input-projection SPI Introduce a structured query-string model and the operation-input projection seam, replacing the placeholder query type and the split('&') string surgery in pagination. - QueryParams (http.common): immutable, insertion-ordered, multi-valued query model with RFC 3986 encoding (space -> %20, literal + -> %2B) and an order-sensitive equals that agrees with encode(). It is an origination model for building queries, not a fidelity-preserving URL editor. - PercentEncoding (http.common): shared RFC 3986 URL-component codec used for both query components and path segments (/ -> %2F, so a path value cannot inject extra segments). - OperationParams (operation): SPI that projects an operation's typed inputs (path / query / header / body) into a Request and the context chain, via toRequest(baseUrl) and toRequestContext(baseUrl, dispatch). Path templates (/pets/{id}) are substituted with path-segment encoding; a missing variable fails fast. Execution stays the pipeline's job. - RequestRebuilder: query edits now splice the raw query string, preserving untouched parameters byte-for-byte (a value-less ?flag stays value-less, reserved characters are not rewritten) and encoding only the targeted parameter; reads go through QueryParams. - Remove the dead QueryParam placeholder stub and its test. - Record the request-URL model decision (keep java.net.URL, layer QueryParams) and document the new types in docs/http.md and the README package map. Closes #28 Closes #29 Closes #57 --- README.md | 3 +- docs/http.md | 151 ++++++++++ sdk-core/api/sdk-core.api | 60 ++++ .../org/dexpace/sdk/core/http/QueryParam.kt | 20 -- .../sdk/core/http/common/PercentEncoding.kt | 49 +++ .../sdk/core/http/common/QueryParams.kt | 280 +++++++++++++++++ .../sdk/core/operation/OperationParams.kt | 95 ++++++ .../operation/OperationRequestAssembler.kt | 66 ++++ .../sdk/core/pagination/RequestRebuilder.kt | 70 ++--- .../dexpace/sdk/core/http/QueryParamTest.kt | 36 --- .../core/http/common/PercentEncodingTest.kt | 91 ++++++ .../sdk/core/http/common/QueryParamsTest.kt | 283 ++++++++++++++++++ .../sdk/core/operation/OperationParamsTest.kt | 138 +++++++++ .../core/pagination/RequestRebuilderTest.kt | 99 ++++++ 14 files changed, 1342 insertions(+), 99 deletions(-) delete mode 100644 sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/QueryParam.kt create mode 100644 sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/PercentEncoding.kt create mode 100644 sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/QueryParams.kt create mode 100644 sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationParams.kt create mode 100644 sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt delete mode 100644 sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/QueryParamTest.kt create mode 100644 sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/PercentEncodingTest.kt create mode 100644 sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/QueryParamsTest.kt create mode 100644 sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt create mode 100644 sdk-core/src/test/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilderTest.kt diff --git a/README.md b/README.md index 9b8617fa..2378844f 100644 --- a/README.md +++ b/README.md @@ -265,7 +265,7 @@ See [docs/pipelines.md](docs/pipelines.md) for the step-author walkthrough. | `http.request` | `Request`, `RequestBody`, `FileRequestBody`, `LoggableRequestBody`, `Method`. | | `http.response` | `Response`, `ResponseBody`, `LoggableResponseBody`, `Status` (a value-carrying class with a total `fromCode`), plus the raw-vs-parsed seam: `ResponseHandler` (with dep-free `string()`/`empty()` handlers) and a lazy, parse-once `ParsedResponse`. | | `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.common` | `Headers`, `HttpHeaderName` (interned), `QueryParams` (RFC 3986 query multimap), `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`. | | `http.pipeline.steps` | Concrete steps: `RetryStep`, `RedirectStep`, `AuthStep`, `KeyCredentialAuthStep`, `BearerTokenAuthStep`, `InstrumentationStep`, `SetDateStep`, and their `*Options` / `*Condition` types. | @@ -273,6 +273,7 @@ See [docs/pipelines.md](docs/pipelines.md) for the step-author walkthrough. | `http.paging` | `PagedIterable`, `PagedResponse`, `PagingOptions` with `byPage()` and `stream()` accessors. | | `auth` | `Credential` sealed hierarchy (`KeyCredential`, `NamedKeyCredential`, `BearerToken`), `BearerTokenProvider`, `AuthScheme`, per-operation `AuthRequirement` / `AuthDescriptor` with `AuthDescriptorResolver` precedence ladder, RFC 7235 challenge parser, `BasicChallengeHandler`, `DigestChallengeHandler`, `CompositeChallengeHandler`. | | `pagination` | `Paginator` (with a `maxPages` safety cap) over cursor / page-number / link-header `PaginationStrategy` implementations, plus `Page` / `SimplePage`. Token-style APIs use `CursorPaginationStrategy` with the query-param name set (e.g. `"page_token"`). | +| `operation` | `OperationParams` — SPI projecting an operation's typed inputs (path / query / header / body) into a `Request` and the context chain, via `toRequest(baseUrl)` / `toRequestContext(baseUrl, dispatch)`. | | `pipeline` | Recovery-aware primitives: `RequestPipeline`, `ResponsePipeline`, `ExecutionPipeline` over a sealed `ResponseOutcome`, with steps (`pipeline.step`, `pipeline.step.retry`) like `RetryStep`, `ResponseRecoveryStep`, `IdempotencyKeyStep`, `ClientIdentityStep`. | | `serde` | `Serde`, `Serializer`, `Deserializer` abstractions, `Tristate` (absent / null / present), and `SerdeException` (the unchecked failure adapters translate codec errors into). | | `io` | `Source`, `Sink`, `Buffer`, `BufferedSource`, `BufferedSink`, `IoProvider`, `Io`, `TeeSink`. | diff --git a/docs/http.md b/docs/http.md index 3a4d8c7c..c2ff3507 100644 --- a/docs/http.md +++ b/docs/http.md @@ -438,6 +438,67 @@ headers.get("content-type") // "application/json" (case-insensitive) headers.values("Cache-Control") // ["no-cache", "no-store"] ``` +### QueryParams + +`QueryParams` is an immutable, insertion-ordered, multi-valued model of a URL query string — +the `?name=value&...` portion of a URL. It mirrors `Headers` in shape (private constructor, +mutable `Builder`, multi-value semantics) but differs in three ways: names are +**case-sensitive** (`?page=1` and `?Page=1` are distinct), values may be **empty or value-less** +(`?flag` and `?flag=` both occur in the wild), and equality is **order-sensitive** — two +instances are equal only if they `encode()` identically. (That last point is the one divergence +from `Headers`, whose case-folded names make name order non-semantic; here order is a rendered +property, so it counts.) + +```kotlin +class QueryParams private constructor( + private val paramsMap: Map> +) +``` + +**Role — building queries, not editing URLs.** `QueryParams` is an *origination* model: it +builds a query string from decoded names/values (for example, projecting an operation's inputs +into a request). It is **not** a fidelity-preserving editor of an existing URL. `encode()` +re-renders every parameter in canonical form, so round-tripping an arbitrary URL through `parse` +then `encode` can change the wire form of parameters you never touched (`?flag` → `flag=`, +reserved characters percent-encoded). Code that must edit one parameter of an existing URL while +leaving the rest byte-for-byte — pagination's `RequestRebuilder` — splices the raw query string +directly instead of going through `encode()`. + +**API:** + +| Method | Description | +|------------------|----------------------------------------------------------------------------| +| `get(name)` | First value for the name, or `null` if absent (`""` for a value-less param)| +| `values(name)` | All values for the name (unmodifiable), or empty list | +| `contains(name)` | Whether any value is present for the name | +| `names()` | Immutable, insertion-ordered snapshot of all parameter names | +| `entries()` | Immutable snapshot as `Map.Entry>` | +| `size()` | Total number of values across all names (derived, not tracked) | +| `isEmpty()` | Whether there are no parameters | +| `encode()` | RFC 3986 query string (space → `%20`, literal `+` → `%2B`), no leading `?` | +| `newBuilder()` | Returns a pre-filled `Builder` for modification | + +**Encoding.** `encode()` / `parse()` use **RFC 3986 query semantics** (via the internal +`PercentEncoding` helper): a space is `%20` (not `+`), and a literal `+` is `%2B` — it is **not** +read back as a space. This is deliberately *not* `application/x-www-form-urlencoded`: a query +*assembled as a request body* uses the form scheme (`+` for spaces) and will be a separate +form-body type, not `QueryParams.encode()`. `parse(encode(...))` round-trips names, values, and +order; malformed percent-encoding falls back to raw text rather than throwing. + +**Builder:** + +```kotlin +val params = QueryParams.builder() + .add("tag", "a") + .add("tag", "b") // multi-value + .set("page", "2") // replaces any existing "page" + .build() + +params.values("tag") // ["a", "b"] +params.get("page") // "2" +params.encode() // "tag=a&tag=b&page=2" +``` + ### MediaType `MediaType` represents a parsed MIME type with optional parameters: @@ -685,6 +746,62 @@ Both implement `HttpClient` and `AsyncHttpClient` on a single class. See the REA --- +## Operation Input Projection + +`OperationParams` (`org.dexpace.sdk.core.operation`) is the SPI a thin generated service implements +once per operation to declare where each typed input belongs on the wire — **path**, **query**, +**header**, or **body** — so generated code (and typed pagination) never splices a URL string. The +runtime assembles the `Request` and feeds it into the context chain. + +```kotlin +interface OperationParams { + val method: Method + val pathTemplate: String // "/pets/{petId}"; leading "/" optional + val operationName: String? // for the tracing seam; default null + + fun pathParams(): Map // default emptyMap() + fun queryParams(): QueryParams // default empty + fun headers(): Headers // default empty + fun body(): RequestBody? // default null + + fun toRequest(baseUrl: String): Request + fun toRequestContext(baseUrl: String, dispatch: DispatchContext): RequestContext +} +``` + +Only `method` and `pathTemplate` are required; the four projections default to empty, so a +parameterless operation overrides almost nothing. + +**Assembly** (`toRequest`): + +- **Path** — each `{name}` in `pathTemplate` is replaced with its `pathParams()` value, + percent-encoded as a path segment (`/` → `%2F`), so a value cannot inject extra segments. A + `{name}` with no value throws `IllegalArgumentException`. +- **Query** — `queryParams().encode()` (RFC 3986) is appended after `?`. +- **Base URL** — treated as a verbatim prefix; a trailing `/` is trimmed and exactly one `/` joins + it to the resolved path, so `https://api.example.com/v1` + `/pets` → `…/v1/pets`. +- **Headers / body / method** — set verbatim from the projections; `Request.build()` validates + body/method compatibility. + +`toRequestContext` builds the `Request` and promotes a `DispatchContext` into a `RequestContext` +carrying it, in one step. Execution stays the pipeline's job — the SPI stops at producing the +request/context (error-mapping and deserialization compose at the service layer, not as pipeline +stages). + +```kotlin +class ListPets(private val limit: Int?) : OperationParams { + override val method = Method.GET + override val pathTemplate = "/pets" + override fun queryParams() = + QueryParams.builder().apply { limit?.let { set("limit", it.toString()) } }.build() +} + +val request = ListPets(limit = 20).toRequest("https://api.example.com") // GET …/pets?limit=20 +val response = httpClient.execute(request) +``` + +--- + ## Design Decisions ### Bodies Over the SDK's I/O Abstraction @@ -749,6 +866,36 @@ Specific API choices driven by JDK 8 targeting: | `java.net.http.HttpClient` (Java 11+) | `HttpClient` interface (transport-agnostic) | | `HttpHeaders` (Java 11+) | Custom `Headers` class | +### Request URL Model + +`Request` stores its target as a single resolved `java.net.URL` (a string-backed container), +**not** a fully deconstructed URL value object (scheme / host / port / path-segments / query). +Structured query manipulation is layered on top via the `QueryParams` multimap. + +**Decision: keep `java.net.URL` as the URL container; layer `QueryParams` for query +manipulation.** + +- **DNS-free equality is preserved.** `Request` equality compares `url.toExternalForm()` — a + pure string comparison with no network I/O — because `java.net.URL.equals` / `hashCode` + resolve the host via DNS (blocking, and wrong for virtual hosts sharing an address). Keeping + the resolved-URL container carries that contract over unchanged. +- **The query is where the manipulation pressure is.** Pagination and (later) operation-input + projection manipulate the query, not the host or path. `QueryParams` puts a structured, + multi-valued, well-tested model exactly there, without forcing a rewrite of how transports + consume a URL. +- **Transports already speak `java.net.URL` / strings.** Both reference transports accept a + resolved URL or string directly; a deconstructed model would add an assembly step at every + transport boundary for no functional gain today. + +Path-template *substitution* (`/pets/{id}` + values → an encoded path) lands minimally with the +`OperationParams` SPI — see "Operation Input Projection" above. What remains **deferred** is a +*structured* URL model: a deconstructed `Url` value object and/or a move from `java.net.URL` to +`java.net.URI`. `URI` gives DNS-free equality natively (no `toExternalForm()` workaround) and +exposes the raw query and path, but parses more strictly and touches every transport boundary. The +container choice (`URL` vs `URI` vs deconstructed) is best decided when richer path handling +(per-segment typing, matrix params) actually earns it; the minimal template substitution above does +not require it. + --- ## Usage Examples @@ -860,6 +1007,10 @@ exchangeCtx.close() | `NetworkException.kt` | `http.response.exception`| public | Transport-level failure (IOException sibling)| | `HttpExceptionFactory.kt` | `http.response.exception`| public | `Response` → typed exception dispatcher | | `Headers.kt` | `http.common` | public | Immutable multi-map + builder | +| `QueryParams.kt` | `http.common` | public | Immutable query-string multi-map + builder | +| `PercentEncoding.kt` | `http.common` | internal | RFC 3986 URL-component percent-encoding (query + path) | +| `OperationParams.kt` | `operation` | public | SPI: project operation inputs → `Request` + context | +| `OperationRequestAssembler.kt` | `operation` | internal | Assembles a `Request` from an `OperationParams` | | `MediaType.kt` | `http.common` | public | Parsed MIME type with charset extraction | | `CommonMediaTypes.kt` | `http.common` | public | Media type constants | | `Protocol.kt` | `http.common` | public | HTTP protocol version enum | diff --git a/sdk-core/api/sdk-core.api b/sdk-core/api/sdk-core.api index 5a93032d..95c010d3 100644 --- a/sdk-core/api/sdk-core.api +++ b/sdk-core/api/sdk-core.api @@ -580,6 +580,44 @@ public final class org/dexpace/sdk/core/http/common/Protocol$Companion { public final fun get (Ljava/lang/String;)Lorg/dexpace/sdk/core/http/common/Protocol; } +public final class org/dexpace/sdk/core/http/common/QueryParams { + public static final field Companion Lorg/dexpace/sdk/core/http/common/QueryParams$Companion; + public synthetic fun (Ljava/util/Map;Lkotlin/jvm/internal/DefaultConstructorMarker;)V + public static final fun builder ()Lorg/dexpace/sdk/core/http/common/QueryParams$Builder; + public final fun contains (Ljava/lang/String;)Z + public static final fun empty ()Lorg/dexpace/sdk/core/http/common/QueryParams; + public final fun encode ()Ljava/lang/String; + public final fun entries ()Ljava/util/Set; + public fun equals (Ljava/lang/Object;)Z + public final fun get (Ljava/lang/String;)Ljava/lang/String; + public fun hashCode ()I + public final fun isEmpty ()Z + public final fun names ()Ljava/util/Set; + public final fun newBuilder ()Lorg/dexpace/sdk/core/http/common/QueryParams$Builder; + public static final fun parse (Ljava/lang/String;)Lorg/dexpace/sdk/core/http/common/QueryParams; + public final fun size ()I + public fun toString ()Ljava/lang/String; + public final fun values (Ljava/lang/String;)Ljava/util/List; +} + +public final class org/dexpace/sdk/core/http/common/QueryParams$Builder { + public fun ()V + public fun (Lorg/dexpace/sdk/core/http/common/QueryParams;)V + public final fun add (Ljava/lang/String;Ljava/lang/String;)Lorg/dexpace/sdk/core/http/common/QueryParams$Builder; + public final fun add (Ljava/lang/String;Ljava/util/List;)Lorg/dexpace/sdk/core/http/common/QueryParams$Builder; + public final fun addAll (Lorg/dexpace/sdk/core/http/common/QueryParams;)Lorg/dexpace/sdk/core/http/common/QueryParams$Builder; + public final fun build ()Lorg/dexpace/sdk/core/http/common/QueryParams; + public final fun remove (Ljava/lang/String;)Lorg/dexpace/sdk/core/http/common/QueryParams$Builder; + public final fun set (Ljava/lang/String;Ljava/lang/String;)Lorg/dexpace/sdk/core/http/common/QueryParams$Builder; + public final fun set (Ljava/lang/String;Ljava/util/List;)Lorg/dexpace/sdk/core/http/common/QueryParams$Builder; +} + +public final class org/dexpace/sdk/core/http/common/QueryParams$Companion { + public final fun builder ()Lorg/dexpace/sdk/core/http/common/QueryParams$Builder; + public final fun empty ()Lorg/dexpace/sdk/core/http/common/QueryParams; + public final fun parse (Ljava/lang/String;)Lorg/dexpace/sdk/core/http/common/QueryParams; +} + public final class org/dexpace/sdk/core/http/common/RequestConditions { public static final field Companion Lorg/dexpace/sdk/core/http/common/RequestConditions$Companion; public synthetic fun (Ljava/util/List;Ljava/util/List;Ljava/time/Instant;Ljava/time/Instant;Lkotlin/jvm/internal/DefaultConstructorMarker;)V @@ -2234,6 +2272,28 @@ public abstract interface class org/dexpace/sdk/core/io/Source : java/io/Closeab public abstract fun read (Lorg/dexpace/sdk/core/io/Buffer;J)J } +public abstract interface class org/dexpace/sdk/core/operation/OperationParams { + public fun body ()Lorg/dexpace/sdk/core/http/request/RequestBody; + public abstract fun getMethod ()Lorg/dexpace/sdk/core/http/request/Method; + public fun getOperationName ()Ljava/lang/String; + public abstract fun getPathTemplate ()Ljava/lang/String; + public fun headers ()Lorg/dexpace/sdk/core/http/common/Headers; + public fun pathParams ()Ljava/util/Map; + public fun queryParams ()Lorg/dexpace/sdk/core/http/common/QueryParams; + public fun toRequest (Ljava/lang/String;)Lorg/dexpace/sdk/core/http/request/Request; + public fun toRequestContext (Ljava/lang/String;Lorg/dexpace/sdk/core/http/context/DispatchContext;)Lorg/dexpace/sdk/core/http/context/RequestContext; +} + +public final class org/dexpace/sdk/core/operation/OperationParams$DefaultImpls { + public static fun body (Lorg/dexpace/sdk/core/operation/OperationParams;)Lorg/dexpace/sdk/core/http/request/RequestBody; + public static fun getOperationName (Lorg/dexpace/sdk/core/operation/OperationParams;)Ljava/lang/String; + public static fun headers (Lorg/dexpace/sdk/core/operation/OperationParams;)Lorg/dexpace/sdk/core/http/common/Headers; + public static fun pathParams (Lorg/dexpace/sdk/core/operation/OperationParams;)Ljava/util/Map; + public static fun queryParams (Lorg/dexpace/sdk/core/operation/OperationParams;)Lorg/dexpace/sdk/core/http/common/QueryParams; + public static fun toRequest (Lorg/dexpace/sdk/core/operation/OperationParams;Ljava/lang/String;)Lorg/dexpace/sdk/core/http/request/Request; + public static fun toRequestContext (Lorg/dexpace/sdk/core/operation/OperationParams;Ljava/lang/String;Lorg/dexpace/sdk/core/http/context/DispatchContext;)Lorg/dexpace/sdk/core/http/context/RequestContext; +} + public final class org/dexpace/sdk/core/pagination/AsyncPaginator { public fun (Lorg/dexpace/sdk/core/client/AsyncHttpClient;Lorg/dexpace/sdk/core/http/request/Request;Lorg/dexpace/sdk/core/pagination/PaginationStrategy;)V public fun (Lorg/dexpace/sdk/core/client/AsyncHttpClient;Lorg/dexpace/sdk/core/http/request/Request;Lorg/dexpace/sdk/core/pagination/PaginationStrategy;J)V diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/QueryParam.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/QueryParam.kt deleted file mode 100644 index f738463d..00000000 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/QueryParam.kt +++ /dev/null @@ -1,20 +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 - -/** - * Placeholder for the query-parameter abstraction. - * - * This class is a stub — its implementation is not yet finalized. It is kept `internal` - * to prevent accidental use by consumers until the API is ready for public exposure. - * - * TODO(omar 2026-08-01): finalize QueryParam API and re-export as public - */ -internal class QueryParam { - fun implementation(): Nothing = TODO() -} diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/PercentEncoding.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/PercentEncoding.kt new file mode 100644 index 00000000..399aab86 --- /dev/null +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/PercentEncoding.kt @@ -0,0 +1,49 @@ +/* + * 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.common + +import java.net.URLDecoder +import java.net.URLEncoder + +/** + * Shared percent-encoding for a single URL **component** — a query parameter name/value, or a + * path segment — using RFC 3986 semantics for spaces and `+`. + * + * This is deliberately *not* `application/x-www-form-urlencoded`: a space is `%20` (not `+`), + * and a literal `+` is preserved as `%2B` rather than being read back as a space. Both + * directions go through the JDK's [URLEncoder] / [URLDecoder] (so multibyte UTF-8 and malformed + * input are handled by battle-tested code), with the `+`/space convention corrected on each side: + * + * - **encode**: `URLEncoder` emits `+` for a space, so the `+`s it produces are exactly the + * spaces — rewrite them to `%20`. Any literal `+` in the input is already `%2B` by then. + * - **decode**: `URLDecoder` would turn `+` into a space, so escape literal `+` to `%2B` + * before decoding, leaving real spaces (`%20`) to decode normally. + * + * The encoder percent-encodes everything except RFC 3986 unreserved characters, which makes it + * safe for both query components and path segments: a `/` becomes `%2F`, so a path-parameter + * value cannot inject extra path segments. A query string assembled as a form *body* would need + * the form scheme instead; that is a separate concern (a future form-body type). + */ +internal object PercentEncoding { + private const val UTF_8: String = "UTF-8" + + /** Percent-encodes a single URL component (space → `%20`, literal `+` → `%2B`, `/` → `%2F`). */ + internal fun encodeComponent(component: String): String = URLEncoder.encode(component, UTF_8).replace("+", "%20") + + /** + * Decodes a single URL component. A literal `+` stays a `+` (RFC 3986, not a space). + * Malformed percent-encoding falls back to the raw text rather than throwing, so a legacy + * URL a transport already accepted cannot break parsing. + */ + internal fun decodeComponent(component: String): String = + try { + URLDecoder.decode(component.replace("+", "%2B"), UTF_8) + } catch (ignored: IllegalArgumentException) { + component + } +} diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/QueryParams.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/QueryParams.kt new file mode 100644 index 00000000..76dd3242 --- /dev/null +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/QueryParams.kt @@ -0,0 +1,280 @@ +/* + * 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.common + +import java.util.Collections + +// Public API surface — not every accessor/mutator on this class is referenced within this module; SDK consumers may use any. + +/** + * Immutable, insertion-ordered, multi-valued model of a URL query string. + * + * Where [Headers] models HTTP header fields, [QueryParams] models the `?name=value&...` + * portion of a URL. The two share their shape deliberately — private constructor, mutable + * [Builder], multi-value semantics — but differ in three respects: + * + * 1. **Names are case-sensitive.** Unlike header field names, query-parameter names are + * opaque to HTTP and case-sensitive by convention (`?page=1` and `?Page=1` are distinct + * parameters), so no normalisation is applied. + * 2. **Values may be empty or value-less.** `?flag` (a name with no `=`) and `?flag=` (a + * name with an empty value) both occur in the wild; the former is modelled as an empty + * string value. + * 3. **Equality is order-sensitive.** Two [QueryParams] are equal only if they carry the + * same names *and* values in the same order — i.e. equality agrees with [encode]. (This + * is the one place the shape diverges from [Headers], whose case-folded names make + * name order non-semantic; here order is a rendered property, so it counts.) + * + * ## Role: building queries, not editing URLs + * + * [QueryParams] is an *origination* model — it builds a query string from decoded + * names/values (e.g. projecting an operation's inputs into a request). It is **not** a + * fidelity-preserving editor of an existing URL: [encode] re-renders every parameter in + * canonical form, so round-tripping an arbitrary URL through [parse] then [encode] may + * change the wire form of untouched parameters (`?flag` → `flag=`, reserved characters + * percent-encoded). Code that must edit one parameter of an existing URL while leaving the + * rest byte-for-byte (e.g. pagination's rebuilder) should splice the raw query instead. + * + * ## External vs encoded form + * + * The model stores **decoded** names and values. [encode] renders the query string with + * RFC 3986 percent-encoding (space → `%20`, literal `+` → `%2B`) via [PercentEncoding]; + * [parse] is its inverse. Round-tripping `parse(encode(...))` preserves names, values, and + * order. A query rendered as an `application/x-www-form-urlencoded` *body* is a separate + * concern (it uses `+` for spaces) and is not what [encode] produces. + * + * ## Thread-safety + * + * [QueryParams] itself is immutable and therefore freely shareable. [Builder] is **not** + * thread-safe — confine each builder instance to a single thread or guard externally. + */ +@Suppress("unused", "TooManyFunctions") +public class QueryParams private constructor( + private val paramsMap: Map>, +) { + /** + * Returns the first value for the given parameter name, or `null` if the name is absent. + * + * A value-less parameter (`?flag`) reports `""` here, which is distinct from `null` + * (absent). Use [contains] to disambiguate "present but empty" from "absent". + */ + public fun get(name: String): String? = paramsMap[name]?.firstOrNull() + + /** + * Returns all values for the given parameter name in insertion order, or an empty list + * if the name is absent. The returned list is unmodifiable. + */ + public fun values(name: String): List = paramsMap[name]?.let(Collections::unmodifiableList) ?: emptyList() + + /** + * Returns `true` if any value is present for the given parameter name. + */ + public fun contains(name: String): Boolean = paramsMap.containsKey(name) + + /** + * Returns an unmodifiable snapshot of all parameter names at the time of the call, in + * insertion order. + */ + public fun names(): Set = Collections.unmodifiableSet(LinkedHashSet(paramsMap.keys)) + + /** + * Returns an unmodifiable snapshot of all parameter entries at the time of the call. + * + * Each entry's value list is itself unmodifiable, so callers cannot mutate this + * [QueryParams] instance through the returned set, its entries + * ([Map.Entry.setValue]), or the per-name value lists. + */ + public fun entries(): Set>> { + val snapshot = LinkedHashMap>(paramsMap.size) + paramsMap.forEach { (key, value) -> + snapshot[key] = Collections.unmodifiableList(value) + } + return Collections.unmodifiableMap(snapshot).entries + } + + /** + * Returns `true` if no parameters are present. + */ + public fun isEmpty(): Boolean = paramsMap.isEmpty() + + /** + * Returns the total number of values across all names (not the number of names). + * + * `?a=1&a=2&b=3` reports `3`. Size is derived from the backing map rather than tracked, + * so it always agrees with [entries]. + */ + public fun size(): Int = paramsMap.values.sumOf { it.size } + + /** + * Renders this query string with RFC 3986 percent-encoding (space → `%20`, literal `+` + * → `%2B`), via [PercentEncoding]. Insertion order is preserved; repeated names are emitted + * once per value (`a=1&a=2`). Returns an empty string when there are no parameters. + * + * The leading `?` is **not** included — callers splice it in when assembling a URL. + */ + public fun encode(): String { + if (paramsMap.isEmpty()) return "" + val sb = StringBuilder() + paramsMap.forEach { (name, valueList) -> + val encodedName = PercentEncoding.encodeComponent(name) + valueList.forEach { value -> + if (sb.isNotEmpty()) sb.append('&') + sb.append(encodedName).append('=').append(PercentEncoding.encodeComponent(value)) + } + } + return sb.toString() + } + + /** + * Returns a new [Builder] initialised with these parameters. + */ + public fun newBuilder(): Builder = Builder(this) + + override fun toString(): String = paramsMap.toString() + + /** + * Value equality over names and values **in order** (see the class note). Two instances + * are equal iff they would [encode] identically. + */ + override fun equals(other: Any?): Boolean { + if (this === other) return true + if (other !is QueryParams) return false + return paramsMap.entries.toList() == other.paramsMap.entries.toList() + } + + override fun hashCode(): Int = paramsMap.entries.toList().hashCode() + + /** + * Builder for constructing [QueryParams] instances. Mutators return `this` so calls can + * be chained. + */ + @Suppress("TooManyFunctions") + public class Builder { + private val paramsMap: MutableMap> = LinkedHashMap() + + /** Creates an empty builder. */ + public constructor() + + /** Creates a builder pre-filled with the parameters from [params]. */ + public constructor(params: QueryParams) : this() { + params.paramsMap.forEach { (key, values) -> + paramsMap[key] = values.toMutableList() + } + } + + /** + * Appends a single value under [name]. Repeated calls accumulate, producing a + * multi-valued parameter (`?a=1&a=2`). A `null` [value] is stored as the empty + * string, modelling a value-less parameter (`?flag`). + */ + public fun add( + name: String, + value: String?, + ): Builder = apply { paramsMap.getOrPut(name) { mutableListOf() }.add(value ?: "") } + + /** + * Appends all [values] under [name] in order. Existing values for [name] are kept. + */ + public fun add( + name: String, + values: List, + ): Builder = apply { paramsMap.getOrPut(name) { mutableListOf() }.addAll(values) } + + /** + * Replaces all values for [name] with the single [value]. A `null` [value] removes + * the parameter entirely (matching [Headers.Builder.set]). + */ + public fun set( + name: String, + value: String?, + ): Builder = + apply { + if (value == null) { + remove(name) + } else { + paramsMap[name] = mutableListOf(value) + } + } + + /** + * Replaces all values for [name] with [values]. + */ + public fun set( + name: String, + values: List, + ): Builder = apply { paramsMap[name] = values.toMutableList() } + + /** + * Removes all values for [name]. A no-op if the name is absent. + */ + public fun remove(name: String): Builder = apply { paramsMap.remove(name) } + + /** + * Merges every entry from [params] into this builder, appending values for names + * that already exist. + */ + public fun addAll(params: QueryParams): Builder = + apply { + params.paramsMap.forEach { (key, values) -> + paramsMap.getOrPut(key) { mutableListOf() }.addAll(values) + } + } + + /** Builds an immutable [QueryParams] from the builder's current state. */ + public fun build(): QueryParams { + // Deep, defensive copy so later builder mutations cannot reach into the built + // instance and accessor-returned lists cannot be mutated via a cast. + val snapshot = LinkedHashMap>(paramsMap.size) + paramsMap.forEach { (key, values) -> + snapshot[key] = Collections.unmodifiableList(ArrayList(values)) + } + return QueryParams(snapshot) + } + } + + public companion object { + /** Returns an empty builder. */ + @JvmStatic + public fun builder(): Builder = Builder() + + /** Returns an empty [QueryParams]. */ + @JvmStatic + public fun empty(): QueryParams = Builder().build() + + /** + * Parses a raw query string (the part after `?`, without the leading `?`) into a + * [QueryParams], decoding names and values with RFC 3986 semantics (space `%20`, + * literal `+`) via [PercentEncoding]. + * + * - A `null` or blank input yields an empty [QueryParams]. + * - A leading `?` is tolerated and stripped. + * - `a=1&a=2` becomes a multi-valued `a`; `flag` (no `=`) becomes `flag` → `""`; + * `flag=` becomes `flag` → `""`. + * - Empty segments (a stray `&`) are skipped. + * - Malformed percent-encoding falls back to the raw text rather than throwing. + */ + @JvmStatic + public fun parse(query: String?): QueryParams { + if (query.isNullOrEmpty()) return empty() + val raw = if (query.startsWith("?")) query.substring(1) else query + if (raw.isEmpty()) return empty() + val builder = Builder() + for (segment in raw.split('&')) { + if (segment.isEmpty()) continue + val eq = segment.indexOf('=') + if (eq < 0) { + builder.add(PercentEncoding.decodeComponent(segment), "") + } else { + val name = PercentEncoding.decodeComponent(segment.substring(0, eq)) + val value = PercentEncoding.decodeComponent(segment.substring(eq + 1)) + builder.add(name, value) + } + } + return builder.build() + } + } +} diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationParams.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationParams.kt new file mode 100644 index 00000000..574e19e4 --- /dev/null +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationParams.kt @@ -0,0 +1,95 @@ +/* + * 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.operation + +import org.dexpace.sdk.core.http.common.Headers +import org.dexpace.sdk.core.http.common.QueryParams +import org.dexpace.sdk.core.http.context.DispatchContext +import org.dexpace.sdk.core.http.context.RequestContext +import org.dexpace.sdk.core.http.request.Method +import org.dexpace.sdk.core.http.request.Request +import org.dexpace.sdk.core.http.request.RequestBody + +/** + * SPI that projects an operation's typed inputs onto an HTTP request. + * + * A thin generated service implements this once per operation, exposing where each input + * belongs on the wire — **path**, **query**, **header**, or **body** — and the runtime + * ([toRequest] / [toRequestContext]) assembles the [Request] and feeds it into the context + * chain. This is the seam #57 defines so generated code never does URL string surgery: + * cursors, page numbers, and operation arguments flow through the typed projections below + * (and through [QueryParams] / [Headers]) rather than being spliced into a URL. + * + * Only [method] and [pathTemplate] are required; the four projections default to empty so a + * parameterless operation overrides almost nothing: + * + * ```kotlin + * class ListPets(private val limit: Int?) : OperationParams { + * override val method = Method.GET + * override val pathTemplate = "/pets" + * override fun queryParams() = + * QueryParams.builder().apply { limit?.let { set("limit", it.toString()) } }.build() + * } + * + * val request = ListPets(limit = 20).toRequest("https://api.example.com") + * ``` + * + * Path parameters are substituted into [pathTemplate] (`/pets/{petId}`) and percent-encoded as + * path segments, so a value cannot inject extra `/` segments. Query parameters are appended via + * [QueryParams.encode] (RFC 3986). The body is whatever [RequestBody] the operation already + * built (JSON via the serde seam, a form body, etc.) — this SPI carries it, it does not encode it. + */ +public interface OperationParams { + /** The HTTP method for this operation. */ + public val method: Method + + /** + * The path template, relative to the base URL, with `{name}` placeholders for path + * parameters — e.g. `/pets/{petId}`. A leading `/` is optional. + */ + public val pathTemplate: String + + /** + * A stable operation identifier (e.g. `"ListPets"`) for instrumentation, or `null`. Carried + * for the tracing seam; it does not affect the assembled request. + */ + public val operationName: String? get() = null + + /** + * Decoded path-parameter values keyed by placeholder name. Every `{name}` in [pathTemplate] + * must have an entry; values are percent-encoded as path segments during assembly. + */ + public fun pathParams(): Map = emptyMap() + + /** Query parameters to append to the URL. */ + public fun queryParams(): QueryParams = QueryParams.empty() + + /** Headers to set on the request. */ + public fun headers(): Headers = Headers.builder().build() + + /** The request body, or `null` for a bodyless operation. */ + public fun body(): RequestBody? = null + + /** + * Assembles the [Request] for this operation against [baseUrl] (e.g. + * `https://api.example.com` or `https://api.example.com/v1`). A trailing `/` on [baseUrl] is + * trimmed before the resolved path is joined. + * + * @throws IllegalArgumentException if a `{name}` in [pathTemplate] has no [pathParams] value. + */ + public fun toRequest(baseUrl: String): Request = OperationRequestAssembler.assemble(baseUrl, this) + + /** + * Assembles the [Request] ([toRequest]) and promotes [dispatch] into a [RequestContext] + * carrying it — feeding the request into the context chain in one step. + */ + public fun toRequestContext( + baseUrl: String, + dispatch: DispatchContext, + ): RequestContext = dispatch.toRequestContext(toRequest(baseUrl)) +} diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt new file mode 100644 index 00000000..12ce5b8f --- /dev/null +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.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.operation + +import org.dexpace.sdk.core.http.common.PercentEncoding +import org.dexpace.sdk.core.http.request.Request + +/** + * Assembles a [Request] from an [OperationParams] projection and a base URL. Internal — callers + * go through [OperationParams.toRequest]. + * + * The path template is resolved by substituting each `{name}` with its percent-encoded path + * parameter; the query is rendered by [org.dexpace.sdk.core.http.common.QueryParams.encode]; and + * the base URL is treated as a verbatim prefix (one `/` between it and the resolved path), so the + * scheme/host/port/base-path of [baseUrl] carry through unchanged. + */ +internal object OperationRequestAssembler { + private val TEMPLATE_VARIABLE = Regex("""\{([^/}]+)}""") + + internal fun assemble( + baseUrl: String, + params: OperationParams, + ): Request { + val path = resolvePath(params.pathTemplate, params.pathParams()) + val query = params.queryParams().encode() + return Request.builder() + .method(params.method) + .url(buildUrl(baseUrl, path, query)) + .headers(params.headers()) + .body(params.body()) + .build() + } + + private fun resolvePath( + template: String, + pathParams: Map, + ): String = + TEMPLATE_VARIABLE.replace(template) { match -> + val name = match.groupValues[1] + val value = + pathParams[name] + ?: throw IllegalArgumentException("Missing path parameter '$name' for template '$template'") + PercentEncoding.encodeComponent(value) + } + + private fun buildUrl( + baseUrl: String, + path: String, + query: String, + ): String { + val sb = StringBuilder(baseUrl.trimEnd('/')) + if (path.isNotEmpty()) { + if (!path.startsWith("/")) sb.append('/') + sb.append(path) + } + if (query.isNotEmpty()) { + sb.append('?').append(query) + } + return sb.toString() + } +} diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilder.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilder.kt index 50cee4ac..e353addf 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilder.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilder.kt @@ -7,27 +7,32 @@ package org.dexpace.sdk.core.pagination +import org.dexpace.sdk.core.http.common.PercentEncoding +import org.dexpace.sdk.core.http.common.QueryParams import org.dexpace.sdk.core.http.request.Request import java.net.URL -import java.net.URLDecoder -import java.net.URLEncoder /** * Internal helpers for building the next-page [Request] from an initial [Request] plus a * paging-state mutation. * - * The functions here are pure URL-rewriting utilities — no I/O, no mutation of the - * supplied [Request], and no allocation beyond the new request and URL. They exist so - * pagination strategies can stay focused on extracting paging state from responses, while - * the mechanical work of "produce a new request with this query param set" lives in one - * place. + * The functions here are pure URL-rewriting utilities — no I/O and no mutation of the + * supplied [Request]. They exist so pagination strategies can stay focused on extracting + * paging state from responses, while the mechanical work of "produce a new request with this + * query param set" lives in one place. * - * ## URL encoding + * ## Query manipulation * - * All query manipulation uses `application/x-www-form-urlencoded` semantics (the JDK's - * `URLEncoder` / `URLDecoder` with UTF-8) — the same convention browsers and servers use - * for query strings. Pre-existing query segments are preserved verbatim; only the targeted - * parameter is replaced. + * Reading a parameter ([getQueryParam]) goes through the structured [QueryParams] model. + * Writing one ([setQueryParam]) splices the raw query string directly — replacing or + * appending only the targeted parameter and copying every other segment **verbatim** — so + * parameters the caller did not touch keep their exact wire form (a value-less `?flag` stays + * value-less; reserved characters are not rewritten). Newly written names and values are + * percent-encoded with RFC 3986 semantics (space → `%20`) via [PercentEncoding]. + * + * [QueryParams.encode] is deliberately *not* used to reassemble the whole query: it re-renders + * every parameter in canonical form, which would alter untouched segments. This editor favours + * fidelity over funnelling everything through the model — see the note on [QueryParams]. * * ## URL.fragment / userInfo * @@ -35,8 +40,6 @@ import java.net.URLEncoder * (fragment) exactly. Only the query string is mutated. */ internal object RequestRebuilder { - private const val UTF_8: String = "UTF-8" - /** * Returns a new [Request] cloned from [request] with the query parameter [name] set to * [value]. If [value] is `null`, removes the parameter entirely. If [name] already @@ -76,7 +79,6 @@ internal object RequestRebuilder { name: String, value: String?, ): URL { - val encodedName = URLEncoder.encode(name, UTF_8) val existing = url.query val outParams: MutableList = ArrayList() var replaced = false @@ -84,54 +86,38 @@ internal object RequestRebuilder { for (segment in existing.split('&')) { if (segment.isEmpty()) continue val key = segment.substringBefore('=', segment) - if (decodeOrRaw(key) == name) { + if (PercentEncoding.decodeComponent(key) == name) { if (!replaced && value != null) { - outParams.add(encodedName + "=" + URLEncoder.encode(value, UTF_8)) + outParams.add(encodeParam(name, value)) replaced = true } // else: dropping this param (value == null) or already replaced once. } else { + // Untouched parameter — copy its raw segment byte-for-byte. outParams.add(segment) } } } if (!replaced && value != null) { - outParams.add(encodedName + "=" + URLEncoder.encode(value, UTF_8)) + outParams.add(encodeParam(name, value)) } val newQuery: String? = if (outParams.isEmpty()) null else outParams.joinToString("&") return rebuildUrl(url, newQuery) } + private fun encodeParam( + name: String, + value: String, + ): String = PercentEncoding.encodeComponent(name) + "=" + PercentEncoding.encodeComponent(value) + /** * Returns the value of the query parameter [name] from [url], or `null` if absent. - * The value is URL-decoded with UTF-8. + * The value is decoded with RFC 3986 semantics (see [QueryParams.parse]). */ fun getQueryParam( url: URL, name: String, - ): String? { - val existing = url.query ?: return null - if (existing.isEmpty()) return null - for (segment in existing.split('&')) { - if (segment.isEmpty()) continue - val key = segment.substringBefore('=', segment) - if (decodeOrRaw(key) == name) { - return decodeOrRaw(segment.substringAfter('=', "")) - } - } - return null - } - - private fun decodeOrRaw(raw: String): String = - try { - URLDecoder.decode(raw, UTF_8) - } catch (ignored: IllegalArgumentException) { - // Malformed percent-encoding — return raw so equality with caller's name still works - // for unencoded ASCII identifiers (the common case for pagination params). - // We intentionally swallow the exception here; pagination should not fail on malformed - // legacy URLs that the transport accepted. - raw - } + ): String? = QueryParams.parse(url.query).get(name) private fun rebuildUrl( source: URL, diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/QueryParamTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/QueryParamTest.kt deleted file mode 100644 index 3bb6ea59..00000000 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/QueryParamTest.kt +++ /dev/null @@ -1,36 +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 - -import kotlin.test.Test -import kotlin.test.assertFailsWith -import kotlin.test.assertNotNull - -/** - * Covers the placeholder [QueryParam] class. The class is currently a stub — its - * single method `implementation()` throws `NotImplementedError` via `TODO()`. The - * test exists to (a) bring the type onto the coverage instrumentation's radar and - * (b) document the placeholder behaviour so a real implementation arrives with a - * test that should be replaced rather than added alongside. - */ -class QueryParamTest { - @Test - fun `default constructor produces an instance`() { - // The no-arg constructor itself must succeed even though all methods are TODO. - assertNotNull(QueryParam()) - } - - @Test - fun `implementation method throws NotImplementedError`() { - // `TODO()` raises kotlin.NotImplementedError. Verify the contract so callers - // know touching this method is an explicit "not ready" signal. - assertFailsWith { - QueryParam().implementation() - } - } -} diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/PercentEncodingTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/PercentEncodingTest.kt new file mode 100644 index 00000000..279c93ec --- /dev/null +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/PercentEncodingTest.kt @@ -0,0 +1,91 @@ +/* + * 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.common + +import kotlin.test.Test +import kotlin.test.assertEquals + +/** + * Unit tests for the [PercentEncoding] helper — the shared RFC 3986 URL-component + * percent-encoder used by [QueryParams] (query names/values) and operation path segments. + * + * The defining property versus `application/x-www-form-urlencoded` is space and `+` + * handling: a space encodes to `%20` (not `+`), and a literal `+` is preserved as `%2B` + * (it is **not** treated as a space on the way back). + */ +class PercentEncodingTest { + @Test + fun `encode renders a space as percent-20 not plus`() { + assertEquals("a%20b", PercentEncoding.encodeComponent("a b")) + } + + @Test + fun `encode renders a literal plus as percent-2B`() { + assertEquals("a%2Bb", PercentEncoding.encodeComponent("a+b")) + } + + @Test + fun `encode percent-encodes the structural delimiters and slash`() { + assertEquals("a%26b", PercentEncoding.encodeComponent("a&b")) + assertEquals("a%3Db", PercentEncoding.encodeComponent("a=b")) + assertEquals("a%2Fb", PercentEncoding.encodeComponent("a/b")) + assertEquals("a%23b", PercentEncoding.encodeComponent("a#b")) + } + + @Test + fun `encode leaves unreserved ascii untouched`() { + assertEquals("Abc-123_x.y", PercentEncoding.encodeComponent("Abc-123_x.y")) + } + + @Test + fun `encode percent-encodes multibyte utf8 as utf8 bytes`() { + assertEquals("%C3%A9", PercentEncoding.encodeComponent("é")) // é + } + + @Test + fun `encode of empty string is empty`() { + assertEquals("", PercentEncoding.encodeComponent("")) + } + + @Test + fun `decode turns percent-20 into a space`() { + assertEquals("a b", PercentEncoding.decodeComponent("a%20b")) + } + + @Test + fun `decode keeps a literal plus as a plus and does not turn it into a space`() { + assertEquals("a+b", PercentEncoding.decodeComponent("a+b")) + } + + @Test + fun `decode turns percent-2B into a literal plus`() { + assertEquals("a+b", PercentEncoding.decodeComponent("a%2Bb")) + } + + @Test + fun `decode reconstructs multibyte utf8`() { + assertEquals("é", PercentEncoding.decodeComponent("%C3%A9")) + } + + @Test + fun `decode keeps raw text on malformed percent-encoding rather than throwing`() { + assertEquals("%zz", PercentEncoding.decodeComponent("%zz")) + assertEquals("%2", PercentEncoding.decodeComponent("%2")) + } + + @Test + fun `decode keeps a literal plus even when the rest is malformed`() { + assertEquals("a+%zz", PercentEncoding.decodeComponent("a+%zz")) + } + + @Test + fun `encode then decode round-trips spaces plus slashes and unicode`() { + val original = "hello world+1/2=3 é" + assertEquals(original, PercentEncoding.decodeComponent(PercentEncoding.encodeComponent(original))) + } +} diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/QueryParamsTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/QueryParamsTest.kt new file mode 100644 index 00000000..4c3d55ce --- /dev/null +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/QueryParamsTest.kt @@ -0,0 +1,283 @@ +/* + * 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.common + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertNotEquals +import kotlin.test.assertNull +import kotlin.test.assertTrue + +class QueryParamsTest { + @Test + fun `empty params have no entries`() { + val params = QueryParams.empty() + assertTrue(params.isEmpty()) + assertEquals(0, params.size()) + assertTrue(params.names().isEmpty()) + assertTrue(params.entries().isEmpty()) + assertEquals("", params.encode()) + } + + @Test + fun `add accumulates multiple values for one name in insertion order`() { + val params = + QueryParams.builder() + .add("tag", "a") + .add("tag", "b") + .add("tag", "c") + .build() + assertEquals(listOf("a", "b", "c"), params.values("tag")) + assertEquals("a", params.get("tag")) + assertEquals(3, params.size()) + } + + @Test + fun `set replaces all existing values`() { + val params = + QueryParams.builder() + .add("page", "1") + .add("page", "2") + .set("page", "9") + .build() + assertEquals(listOf("9"), params.values("page")) + } + + @Test + fun `set list replaces with multiple values`() { + val params = + QueryParams.builder() + .add("k", "old") + .set("k", listOf("x", "y")) + .build() + assertEquals(listOf("x", "y"), params.values("k")) + } + + @Test + fun `set null removes the parameter`() { + val params = + QueryParams.builder() + .add("a", "1") + .add("b", "2") + .set("a", null) + .build() + assertFalse(params.contains("a")) + assertTrue(params.contains("b")) + } + + @Test + fun `remove drops the name and is a no-op when absent`() { + val params = + QueryParams.builder() + .add("a", "1") + .remove("a") + .remove("missing") + .build() + assertTrue(params.isEmpty()) + } + + @Test + fun `add with null value stores empty string for a value-less param`() { + val params = QueryParams.builder().add("flag", null as String?).build() + assertTrue(params.contains("flag")) + assertEquals("", params.get("flag")) + } + + @Test + fun `names are case-sensitive`() { + val params = + QueryParams.builder() + .add("Page", "1") + .add("page", "2") + .build() + assertEquals(listOf("1"), params.values("Page")) + assertEquals(listOf("2"), params.values("page")) + assertEquals(setOf("Page", "page"), params.names()) + } + + @Test + fun `get returns null and values empty for absent name`() { + val params = QueryParams.builder().add("a", "1").build() + assertNull(params.get("b")) + assertTrue(params.values("b").isEmpty()) + assertFalse(params.contains("b")) + } + + @Test + fun `encode percent-encodes names and values per rfc3986`() { + val params = + QueryParams.builder() + .add("q", "a b&c") + .add("name+key", "x") + .build() + // Space is %20 (not +); literal + is %2B; & is %26. + assertEquals("q=a%20b%26c&name%2Bkey=x", params.encode()) + } + + @Test + fun `encode renders a space as percent-20`() { + val params = QueryParams.builder().add("q", "hello world").build() + assertEquals("q=hello%20world", params.encode()) + } + + @Test + fun `encode emits one pair per value for repeated names`() { + val params = + QueryParams.builder() + .add("a", "1") + .add("a", "2") + .add("b", "3") + .build() + assertEquals("a=1&a=2&b=3", params.encode()) + } + + @Test + fun `parse decodes names and values`() { + val params = QueryParams.parse("q=a%20b%26c&page=2") + assertEquals("a b&c", params.get("q")) + assertEquals("2", params.get("page")) + } + + @Test + fun `parse reads a literal plus as a plus not a space`() { + // RFC 3986: in a URL query, `+` is a literal `+`, unlike form-encoding. + val params = QueryParams.parse("q=a+b") + assertEquals("a+b", params.get("q")) + } + + @Test + fun `parse handles multi-value names`() { + val params = QueryParams.parse("tag=a&tag=b&tag=c") + assertEquals(listOf("a", "b", "c"), params.values("tag")) + } + + @Test + fun `parse treats a bare name as a value-less param`() { + val params = QueryParams.parse("flag&other=1") + assertTrue(params.contains("flag")) + assertEquals("", params.get("flag")) + assertEquals("1", params.get("other")) + } + + @Test + fun `parse treats name equals empty as empty value`() { + val params = QueryParams.parse("flag=") + assertEquals("", params.get("flag")) + } + + @Test + fun `parse tolerates a leading question mark`() { + val params = QueryParams.parse("?a=1&b=2") + assertEquals("1", params.get("a")) + assertEquals("2", params.get("b")) + } + + @Test + fun `parse skips empty segments`() { + val params = QueryParams.parse("a=1&&b=2&") + assertEquals(listOf("a", "b"), params.names().toList()) + } + + @Test + fun `parse of null or empty yields empty`() { + assertTrue(QueryParams.parse(null).isEmpty()) + assertTrue(QueryParams.parse("").isEmpty()) + assertTrue(QueryParams.parse("?").isEmpty()) + } + + @Test + fun `parse keeps raw text on malformed percent-encoding`() { + // A stray '%' that is not a valid escape must not throw. + val params = QueryParams.parse("bad=%zz") + assertEquals("%zz", params.get("bad")) + } + + @Test + fun `encode then parse round-trips names values and order`() { + val original = + QueryParams.builder() + .add("q", "hello world") + .add("tag", "a") + .add("tag", "b") + .add("special", "100%") + .add("plus", "a+b") + .build() + val roundTripped = QueryParams.parse(original.encode()) + assertEquals(original, roundTripped) + } + + @Test + fun `newBuilder is prefilled and does not mutate the source`() { + val original = QueryParams.builder().add("a", "1").build() + val derived = original.newBuilder().add("b", "2").build() + assertFalse(original.contains("b")) + assertEquals("1", derived.get("a")) + assertEquals("2", derived.get("b")) + } + + @Test + fun `addAll merges and appends values for existing names`() { + val base = QueryParams.builder().add("a", "1").add("b", "2").build() + val extra = QueryParams.builder().add("a", "3").add("c", "4").build() + val merged = base.newBuilder().addAll(extra).build() + assertEquals(listOf("1", "3"), merged.values("a")) + assertEquals(listOf("2"), merged.values("b")) + assertEquals(listOf("4"), merged.values("c")) + } + + @Test + fun `values list is unmodifiable`() { + val params = QueryParams.builder().add("a", "1").build() + + @Suppress("UNCHECKED_CAST") + val asMutable = params.values("a") as MutableList + assertFailsWith { asMutable.add("x") } + } + + @Test + fun `entries snapshot is immutable and reflects all names`() { + val params = QueryParams.builder().add("a", "1").add("a", "2").add("b", "3").build() + val entries = params.entries() + assertEquals(2, entries.size) + assertFailsWith { + (entries as MutableSet<*>).clear() + } + } + + @Test + fun `build is decoupled from the builder after the fact`() { + val builder = QueryParams.builder().add("a", "1") + val first = builder.build() + builder.add("a", "2") + assertEquals(listOf("1"), first.values("a")) + } + + @Test + fun `equality is by value and includes hashCode`() { + val a = QueryParams.builder().add("x", "1").add("y", "2").build() + val b = QueryParams.builder().add("x", "1").add("y", "2").build() + assertEquals(a, b) + assertEquals(a.hashCode(), b.hashCode()) + } + + @Test + fun `equality is order-sensitive across names`() { + val a = QueryParams.builder().add("x", "1").add("y", "2").build() + val b = QueryParams.builder().add("y", "2").add("x", "1").build() + assertNotEquals(a, b) + } + + @Test + fun `equality is order-sensitive across values of one name`() { + val a = QueryParams.builder().add("t", "1").add("t", "2").build() + val b = QueryParams.builder().add("t", "2").add("t", "1").build() + assertNotEquals(a, b) + } +} diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt new file mode 100644 index 00000000..fad7b827 --- /dev/null +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt @@ -0,0 +1,138 @@ +/* + * 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.operation + +import org.dexpace.sdk.core.http.common.CommonMediaTypes +import org.dexpace.sdk.core.http.common.Headers +import org.dexpace.sdk.core.http.common.QueryParams +import org.dexpace.sdk.core.http.context.ContextStore +import org.dexpace.sdk.core.http.context.DispatchContext +import org.dexpace.sdk.core.http.request.Method +import org.dexpace.sdk.core.http.request.RequestBody +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertNull +import kotlin.test.assertSame +import kotlin.test.assertTrue + +class OperationParamsTest { + /** Fully-specified test operation. */ + private class TestOp( + override val method: Method, + override val pathTemplate: String, + private val path: Map = emptyMap(), + private val query: QueryParams = QueryParams.empty(), + private val hdrs: Headers = Headers.builder().build(), + private val bdy: RequestBody? = null, + override val operationName: String? = null, + ) : OperationParams { + override fun pathParams(): Map = path + + override fun queryParams(): QueryParams = query + + override fun headers(): Headers = hdrs + + override fun body(): RequestBody? = bdy + } + + /** Minimal operation — exercises every default projection. */ + private class MinimalOp : OperationParams { + override val method: Method = Method.GET + override val pathTemplate: String = "/ping" + } + + private val base = "https://api.example.com" + + @Test + fun `toRequest builds method and url from base and static path`() { + val request = TestOp(Method.GET, "/pets").toRequest(base) + assertEquals(Method.GET, request.method) + assertEquals("https://api.example.com/pets", request.url.toExternalForm()) + } + + @Test + fun `toRequest substitutes and percent-encodes path params`() { + val request = TestOp(Method.GET, "/pets/{petId}", path = mapOf("petId" to "a b/c")).toRequest(base) + assertEquals("https://api.example.com/pets/a%20b%2Fc", request.url.toExternalForm()) + } + + @Test + fun `toRequest appends query params in order`() { + val query = QueryParams.builder().add("tag", "a").add("tag", "b").add("page", "2").build() + val request = TestOp(Method.GET, "/pets", query = query).toRequest(base) + assertEquals("https://api.example.com/pets?tag=a&tag=b&page=2", request.url.toExternalForm()) + } + + @Test + fun `toRequest composes path query and headers`() { + val request = + TestOp( + Method.GET, + "/pets/{id}", + path = mapOf("id" to "7"), + query = QueryParams.builder().add("limit", "10").build(), + hdrs = Headers.builder().add("Accept", "application/json").build(), + ).toRequest(base) + assertEquals("https://api.example.com/pets/7?limit=10", request.url.toExternalForm()) + assertEquals("application/json", request.headers.get("Accept")) + } + + @Test + fun `toRequest carries the body and method`() { + val body = RequestBody.create("{}", CommonMediaTypes.APPLICATION_JSON) + val request = TestOp(Method.POST, "/pets", bdy = body).toRequest(base) + assertEquals(Method.POST, request.method) + assertSame(body, request.body) + } + + @Test + fun `toRequest trims a trailing slash on the base before joining`() { + val request = TestOp(Method.GET, "/pets").toRequest("https://api.example.com/") + assertEquals("https://api.example.com/pets", request.url.toExternalForm()) + } + + @Test + fun `toRequest joins a base that already has a path prefix`() { + val request = TestOp(Method.GET, "/pets").toRequest("https://api.example.com/v1") + assertEquals("https://api.example.com/v1/pets", request.url.toExternalForm()) + } + + @Test + fun `toRequest throws when a template variable has no value`() { + assertFailsWith { + TestOp(Method.GET, "/pets/{petId}").toRequest(base) + } + } + + @Test + fun `minimal operation uses empty defaults`() { + val op = MinimalOp() + assertTrue(op.pathParams().isEmpty()) + assertTrue(op.queryParams().isEmpty()) + assertTrue(op.headers().names().isEmpty()) + assertNull(op.body()) + assertNull(op.operationName) + assertEquals("https://api.example.com/ping", op.toRequest(base).url.toExternalForm()) + } + + @Test + fun `toRequestContext feeds the request into the context chain`() { + val dispatch = DispatchContext.default() + val op = TestOp(Method.GET, "/pets") + val ctx = op.toRequestContext(base, dispatch) + try { + assertEquals(op.toRequest(base), ctx.request) + assertEquals(dispatch.callKey, ctx.callKey) + assertSame(dispatch.instrumentationContext, ctx.instrumentationContext) + assertSame(ctx, ContextStore.get(ctx.callKey)) + } finally { + ctx.close() + } + } +} diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilderTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilderTest.kt new file mode 100644 index 00000000..ef75344d --- /dev/null +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilderTest.kt @@ -0,0 +1,99 @@ +/* + * 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.pagination + +import java.net.URL +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull + +/** + * Direct tests for the internal [RequestRebuilder] URL helpers. + * + * Two properties matter here and are easy to regress: + * 1. **New values use RFC 3986 encoding** — a space becomes `%20`, not `+`. + * 2. **Untouched parameters are preserved byte-for-byte** — the rebuilder splices the raw + * query rather than decoding and re-encoding the whole thing, so a value-less flag stays + * value-less and reserved characters in other params are not rewritten. + */ +class RequestRebuilderTest { + private fun url(spec: String): URL = URL(spec) + + @Test + fun `setQueryParam appends a new param when absent`() { + val result = RequestRebuilder.setQueryParam(url("https://api.example.com/items"), "page", "2") + assertEquals("page=2", result.query) + } + + @Test + fun `setQueryParam replaces an existing param in place`() { + val result = RequestRebuilder.setQueryParam(url("https://api.example.com/items?page=1&sort=asc"), "page", "2") + assertEquals("page=2&sort=asc", result.query) + } + + @Test + fun `setQueryParam with null removes the param but keeps the others`() { + val result = RequestRebuilder.setQueryParam(url("https://api.example.com/items?page=1&sort=asc"), "page", null) + assertEquals("sort=asc", result.query) + } + + @Test + fun `setQueryParam dropping the only param yields no query`() { + val result = RequestRebuilder.setQueryParam(url("https://api.example.com/items?page=1"), "page", null) + assertNull(result.query) + } + + @Test + fun `setQueryParam encodes a space in the new value as percent-20`() { + val result = RequestRebuilder.setQueryParam(url("https://api.example.com/items"), "q", "a b") + assertEquals("q=a%20b", result.query) + } + + @Test + fun `setQueryParam encodes reserved characters in the new value`() { + val result = RequestRebuilder.setQueryParam(url("https://api.example.com/items"), "token", "a+b/c=") + assertEquals("token=a%2Bb%2Fc%3D", result.query) + } + + @Test + fun `setQueryParam preserves a value-less flag verbatim`() { + val result = RequestRebuilder.setQueryParam(url("https://api.example.com/items?flag&page=1"), "page", "2") + assertEquals("flag&page=2", result.query) + } + + @Test + fun `setQueryParam preserves reserved chars in untouched params verbatim`() { + val result = RequestRebuilder.setQueryParam(url("https://api.example.com/items?filter=a:b&page=1"), "page", "2") + assertEquals("filter=a:b&page=2", result.query) + } + + @Test + fun `getQueryParam returns the decoded value`() { + assertEquals("2", RequestRebuilder.getQueryParam(url("https://api.example.com/items?page=2"), "page")) + } + + @Test + fun `getQueryParam returns null when absent`() { + assertNull(RequestRebuilder.getQueryParam(url("https://api.example.com/items?page=2"), "cursor")) + } + + @Test + fun `getQueryParam returns null when there is no query`() { + assertNull(RequestRebuilder.getQueryParam(url("https://api.example.com/items"), "page")) + } + + @Test + fun `getQueryParam decodes percent-escapes`() { + assertEquals("a b", RequestRebuilder.getQueryParam(url("https://api.example.com/items?q=a%20b"), "q")) + } + + @Test + fun `getQueryParam reads a literal plus as a plus`() { + assertEquals("a+b", RequestRebuilder.getQueryParam(url("https://api.example.com/items?q=a+b"), "q")) + } +} From 55a7447cf387d529f3d46b940571854fb806e13b Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 28 Jun 2026 17:20:28 +0300 Subject: [PATCH 2/7] docs: record the request-URL model decision in architecture.md Add a Request URL Model subsection (and TOC entry) under Cross-Cutting Design Decisions, summarising the keep-java.net.URL-plus-QueryParams choice and the preserved DNS-free equality, with a pointer to the full rationale in http.md. Relates to #29. --- docs/architecture.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/docs/architecture.md b/docs/architecture.md index f0bc5d5a..df227a5f 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -28,6 +28,7 @@ concerns. - [JDK 8 Compatibility](#jdk-8-compatibility) - [Cross-Compile Toolchain Discipline](#cross-compile-toolchain-discipline) - [Immutability and Builders](#immutability-and-builders) + - [Request URL Model](#request-url-model) - [Virtual Thread Safety](#virtual-thread-safety) - [Internal Visibility](#internal-visibility) - [Cancellation](#cancellation) @@ -596,6 +597,18 @@ data class Request private constructor( - **`newBuilder()`**: Creates a pre-filled builder for modification - **`Builder`**: Generic interface ensuring all builders have `fun build(): T` +### Request URL Model + +`Request` stores its target as a single resolved `java.net.URL` and layers the `QueryParams` +multimap (`http.common`) on top for structured query manipulation, rather than modelling a fully +deconstructed URL value object. This preserves the DNS-free equality contract — `Request` compares +`url.toExternalForm()`, a pure string comparison, because `java.net.URL.equals` / `hashCode` +resolve the host via DNS (blocking, and wrong for virtual hosts) — and puts a structured model +where the manipulation pressure actually is (the query string). A deconstructed `Url` model and/or +a `java.net.URI` migration remain deferred until richer path handling earns them; minimal +path-template substitution lives in the `OperationParams` SPI. Full rationale and the alternatives +weighed: see the "Request URL Model" section in [`http.md`](http.md). + ### Virtual Thread Safety The SDK uses `ReentrantLock` over `synchronized` wherever locking is needed: From 7f6798337dcb9f31419dac19920bf1adc4cdc97a Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 28 Jun 2026 17:37:48 +0300 Subject: [PATCH 3/7] fix: drop empty-valued query params so equality agrees with encode() QueryParams.build() retained a name whose value list was empty, leaving an entry that reported contains() == true but rendered nothing from encode(). Two instances that encode() to the same string could therefore compare unequal and hash differently, breaking the documented "equal iff encode() identical" invariant. Skip names with no values when building, so a name added with an empty list never persists as a phantom entry. A value-less parameter keeps its single empty-string value and is unaffected. --- .../sdk/core/http/common/QueryParams.kt | 14 ++++++++-- .../sdk/core/http/common/QueryParamsTest.kt | 28 +++++++++++++++++++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/QueryParams.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/QueryParams.kt index 76dd3242..ad8be243 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/QueryParams.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/QueryParams.kt @@ -201,7 +201,9 @@ public class QueryParams private constructor( } /** - * Replaces all values for [name] with [values]. + * Replaces all values for [name] with [values]. An empty [values] list drops the + * parameter on [build] (a name with no values renders nothing), so it never leaves a + * phantom entry; pass `null` to [set] to remove a parameter explicitly. */ public fun set( name: String, @@ -228,9 +230,17 @@ public class QueryParams private constructor( public fun build(): QueryParams { // Deep, defensive copy so later builder mutations cannot reach into the built // instance and accessor-returned lists cannot be mutated via a cast. + // + // A name whose value list is empty (e.g. `add(name, emptyList())`) contributes + // nothing to encode(), so it is dropped here: keeping it would leave a phantom + // entry that is contains()==true yet invisible to encode(), breaking the + // "equal iff encode() identical" invariant. A value-less param keeps a single + // empty-string value (`[""]`), which is not an empty list and is retained. val snapshot = LinkedHashMap>(paramsMap.size) paramsMap.forEach { (key, values) -> - snapshot[key] = Collections.unmodifiableList(ArrayList(values)) + if (values.isNotEmpty()) { + snapshot[key] = Collections.unmodifiableList(ArrayList(values)) + } } return QueryParams(snapshot) } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/QueryParamsTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/QueryParamsTest.kt index 4c3d55ce..3f8b6851 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/QueryParamsTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/QueryParamsTest.kt @@ -280,4 +280,32 @@ class QueryParamsTest { val b = QueryParams.builder().add("t", "2").add("t", "1").build() assertNotEquals(a, b) } + + @Test + fun `a name added with an empty value list yields params equal to empty`() { + // A name with no values contributes nothing to encode(), so it must not leave a + // phantom entry that breaks the "equal iff encode() identical" invariant. + val params = QueryParams.builder().add("a", emptyList()).build() + assertFalse(params.contains("a")) + assertTrue(params.isEmpty()) + assertEquals("", params.encode()) + assertEquals(QueryParams.empty(), params) + assertEquals(QueryParams.empty().hashCode(), params.hashCode()) + } + + @Test + fun `set with an empty value list drops the name`() { + val params = QueryParams.builder().add("a", "1").set("a", emptyList()).build() + assertFalse(params.contains("a")) + assertTrue(params.isEmpty()) + } + + @Test + fun `a value-less param is retained and not confused with an empty value list`() { + // add(name, null) stores a single empty-string value ("?flag"); that is NOT an empty + // list and must survive build(). + val params = QueryParams.builder().add("flag", null as String?).build() + assertTrue(params.contains("flag")) + assertEquals("flag=", params.encode()) + } } From 991adf4a4e5e154cd79ab4321244ec61f9132908 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 28 Jun 2026 17:37:55 +0300 Subject: [PATCH 4/7] fix: compose operation request URLs around an existing base query OperationRequestAssembler appended the resolved path and query to the base URL verbatim, with no handling for a base URL that already carried a query. A signed base such as https://host/container?sig=... therefore had the path spliced into the query value (https://host/container?sig=.../pets?limit=20), corrupting both the path and the signature, and java.net.URL accepted the result so the breakage only surfaced at the server. Split any query off the base URL first, insert the resolved path before it, and append the operation query after it, so base + path + query compose correctly. Reject a fragment on the base URL (it cannot be composed with a path/query and is never sent on the wire), and translate a malformed base URL into IllegalArgumentException instead of leaking the checked MalformedURLException out of toRequest(). --- docs/http.md | 10 +++- .../sdk/core/operation/OperationParams.kt | 8 ++- .../operation/OperationRequestAssembler.kt | 54 +++++++++++++++---- .../sdk/core/operation/OperationParamsTest.kt | 27 ++++++++++ 4 files changed, 84 insertions(+), 15 deletions(-) diff --git a/docs/http.md b/docs/http.md index c2ff3507..ca33afb3 100644 --- a/docs/http.md +++ b/docs/http.md @@ -778,8 +778,14 @@ parameterless operation overrides almost nothing. percent-encoded as a path segment (`/` → `%2F`), so a value cannot inject extra segments. A `{name}` with no value throws `IllegalArgumentException`. - **Query** — `queryParams().encode()` (RFC 3986) is appended after `?`. -- **Base URL** — treated as a verbatim prefix; a trailing `/` is trimmed and exactly one `/` joins - it to the resolved path, so `https://api.example.com/v1` + `/pets` → `…/v1/pets`. +- **Base URL** — the scheme/host/port/base-path carry through unchanged; a trailing `/` is trimmed + and exactly one `/` joins it to the resolved path, so `https://api.example.com/v1` + `/pets` → + `…/v1/pets`. A query already on the base URL is preserved: the resolved path is inserted **before** + it and the operation's query is appended after it, so a signed base + `https://host/c?sig=…` + `/pets?limit=20` → `https://host/c/pets?sig=…&limit=20`. A **fragment** + on the base URL is rejected (`IllegalArgumentException`) — it cannot be composed with a path/query + and is never sent on the wire — and a base URL that resolves to a malformed URL (e.g. no scheme) + also throws `IllegalArgumentException` rather than leaking a checked `MalformedURLException`. - **Headers / body / method** — set verbatim from the projections; `Request.build()` validates body/method compatibility. diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationParams.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationParams.kt index 574e19e4..896f7312 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationParams.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationParams.kt @@ -78,9 +78,13 @@ public interface OperationParams { /** * Assembles the [Request] for this operation against [baseUrl] (e.g. * `https://api.example.com` or `https://api.example.com/v1`). A trailing `/` on [baseUrl] is - * trimmed before the resolved path is joined. + * trimmed before the resolved path is joined. If [baseUrl] already carries a query (e.g. a + * signed/SAS base such as `https://host/c?sig=…`), the resolved path is inserted before it and + * this operation's query is appended after it. * - * @throws IllegalArgumentException if a `{name}` in [pathTemplate] has no [pathParams] value. + * @throws IllegalArgumentException if a `{name}` in [pathTemplate] has no [pathParams] value, + * if [baseUrl] carries a fragment (`#…`), or if [baseUrl] resolves to a malformed URL + * (e.g. it has no scheme). */ public fun toRequest(baseUrl: String): Request = OperationRequestAssembler.assemble(baseUrl, this) diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt index 12ce5b8f..6be3d28a 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt @@ -9,6 +9,7 @@ package org.dexpace.sdk.core.operation import org.dexpace.sdk.core.http.common.PercentEncoding import org.dexpace.sdk.core.http.request.Request +import java.net.MalformedURLException /** * Assembles a [Request] from an [OperationParams] projection and a base URL. Internal — callers @@ -16,8 +17,11 @@ import org.dexpace.sdk.core.http.request.Request * * The path template is resolved by substituting each `{name}` with its percent-encoded path * parameter; the query is rendered by [org.dexpace.sdk.core.http.common.QueryParams.encode]; and - * the base URL is treated as a verbatim prefix (one `/` between it and the resolved path), so the - * scheme/host/port/base-path of [baseUrl] carry through unchanged. + * the base URL's scheme/host/port/base-path carry through unchanged. The resolved path is inserted + * after the base path but **before** any query the base URL already carries (e.g. a signed/SAS + * base such as `https://host/c?sig=…`), and the operation's query is appended after the base + * query. A fragment on the base URL is rejected — it cannot be composed with a path/query and is + * never sent on the wire, so it is almost certainly a mistake. */ internal object OperationRequestAssembler { private val TEMPLATE_VARIABLE = Regex("""\{([^/}]+)}""") @@ -28,12 +32,22 @@ internal object OperationRequestAssembler { ): Request { val path = resolvePath(params.pathTemplate, params.pathParams()) val query = params.queryParams().encode() - return Request.builder() - .method(params.method) - .url(buildUrl(baseUrl, path, query)) - .headers(params.headers()) - .body(params.body()) - .build() + val url = buildUrl(baseUrl, path, query) + return try { + Request.builder() + .method(params.method) + .url(url) + .headers(params.headers()) + .body(params.body()) + .build() + } catch (e: MalformedURLException) { + // Surface a malformed base URL (e.g. missing scheme) as the SPI's documented + // IllegalArgumentException rather than leaking a checked IOException to callers. + throw IllegalArgumentException( + "Cannot assemble a request: base URL '$baseUrl' resolved to a malformed URL '$url'", + e, + ) + } } private fun resolvePath( @@ -53,13 +67,31 @@ internal object OperationRequestAssembler { path: String, query: String, ): String { - val sb = StringBuilder(baseUrl.trimEnd('/')) + require('#' !in baseUrl) { + "Base URL must not contain a fragment ('#'): '$baseUrl'" + } + // Split off any query the base URL already carries so the resolved path is inserted + // before it (`host/base?sig=…` + `/pets` → `host/base/pets?sig=…`), instead of being + // swallowed into a query value. + val queryStart = baseUrl.indexOf('?') + val prefix = if (queryStart < 0) baseUrl else baseUrl.substring(0, queryStart) + val baseQuery = if (queryStart < 0) "" else baseUrl.substring(queryStart + 1) + + val sb = StringBuilder(prefix.trimEnd('/')) if (path.isNotEmpty()) { if (!path.startsWith("/")) sb.append('/') sb.append(path) } - if (query.isNotEmpty()) { - sb.append('?').append(query) + // Base query first (it is ambient — auth tokens, API keys), operation query appended. + // Both sides are already percent-encoded wire form, so they concatenate verbatim. + val mergedQuery = + when { + baseQuery.isEmpty() -> query + query.isEmpty() -> baseQuery + else -> "$baseQuery&$query" + } + if (mergedQuery.isNotEmpty()) { + sb.append('?').append(mergedQuery) } return sb.toString() } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt index fad7b827..ccebb9f7 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt @@ -135,4 +135,31 @@ class OperationParamsTest { ctx.close() } } + + @Test + fun `toRequest merges a base URL query ahead of the operation query`() { + val query = QueryParams.builder().add("limit", "20").build() + val request = TestOp(Method.GET, "/pets", query = query).toRequest("https://api.example.com/v1?sv=X&sig=Y") + assertEquals("https://api.example.com/v1/pets?sv=X&sig=Y&limit=20", request.url.toExternalForm()) + } + + @Test + fun `toRequest preserves a base URL query when the operation contributes none`() { + val request = TestOp(Method.GET, "/pets").toRequest("https://api.example.com?sv=X") + assertEquals("https://api.example.com/pets?sv=X", request.url.toExternalForm()) + } + + @Test + fun `toRequest rejects a base URL carrying a fragment`() { + assertFailsWith { + TestOp(Method.GET, "/pets").toRequest("https://api.example.com/v1#frag") + } + } + + @Test + fun `toRequest throws IllegalArgumentException for a malformed base URL`() { + assertFailsWith { + TestOp(Method.GET, "/pets").toRequest("api.example.com") + } + } } From f6b0e1d9121ed89ba950137fc413df0431760275 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 28 Jun 2026 18:21:22 +0300 Subject: [PATCH 5/7] fix: correct RFC 3986 component encoding and harden operation URL composition - encode reserved '*' as %2A and keep unreserved '~' literal in PercentEncoding - preserve a significant trailing slash and avoid a doubled '&' when composing operation request URLs over a base URL that already carries path/query - read a query param via a direct scan instead of building a throwaway QueryParams multimap per page --- .../sdk/core/http/common/PercentEncoding.kt | 27 ++++++++++---- .../operation/OperationRequestAssembler.kt | 18 +++++++-- .../sdk/core/pagination/RequestRebuilder.kt | 37 +++++++++++++------ .../core/http/common/PercentEncodingTest.kt | 23 ++++++++++++ .../sdk/core/operation/OperationParamsTest.kt | 15 ++++++++ .../core/pagination/RequestRebuilderTest.kt | 5 +++ 6 files changed, 104 insertions(+), 21 deletions(-) diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/PercentEncoding.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/PercentEncoding.kt index 399aab86..0a6afff2 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/PercentEncoding.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/PercentEncoding.kt @@ -21,19 +21,32 @@ import java.net.URLEncoder * * - **encode**: `URLEncoder` emits `+` for a space, so the `+`s it produces are exactly the * spaces — rewrite them to `%20`. Any literal `+` in the input is already `%2B` by then. + * `URLEncoder`'s notion of "safe" also diverges from RFC 3986 in two characters, both + * corrected here so the output matches the RFC 3986 unreserved set exactly: it leaves `*` + * (a sub-delimiter, *reserved*) unencoded — rewritten to `%2A` — and over-encodes `~` + * (unreserved) to `%7E` — rewritten back to `~`. * - **decode**: `URLDecoder` would turn `+` into a space, so escape literal `+` to `%2B` - * before decoding, leaving real spaces (`%20`) to decode normally. + * before decoding, leaving real spaces (`%20`) to decode normally. (`%2A` decodes back to + * `*` and a bare `~` is left untouched, so the encode corrections round-trip cleanly.) * - * The encoder percent-encodes everything except RFC 3986 unreserved characters, which makes it - * safe for both query components and path segments: a `/` becomes `%2F`, so a path-parameter - * value cannot inject extra path segments. A query string assembled as a form *body* would need - * the form scheme instead; that is a separate concern (a future form-body type). + * The encoder percent-encodes everything except the RFC 3986 unreserved set + * (`A–Z`, `a–z`, `0–9`, `-`, `.`, `_`, `~`), which makes it safe for both query components and + * path segments: a `/` becomes `%2F`, so a path-parameter value cannot inject extra path + * segments. A query string assembled as a form *body* would need the form scheme instead; that + * is a separate concern (a future form-body type). */ internal object PercentEncoding { private const val UTF_8: String = "UTF-8" - /** Percent-encodes a single URL component (space → `%20`, literal `+` → `%2B`, `/` → `%2F`). */ - internal fun encodeComponent(component: String): String = URLEncoder.encode(component, UTF_8).replace("+", "%20") + /** + * Percent-encodes a single URL component to the RFC 3986 unreserved set: space → `%20`, + * literal `+` → `%2B`, `/` → `%2F`, `*` → `%2A`; the unreserved `~` is left as `~`. + */ + internal fun encodeComponent(component: String): String = + URLEncoder.encode(component, UTF_8) + .replace("+", "%20") + .replace("*", "%2A") + .replace("%7E", "~") /** * Decodes a single URL component. A literal `+` stays a `+` (RFC 3986, not a space). diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt index 6be3d28a..90fb1942 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt @@ -77,17 +77,29 @@ internal object OperationRequestAssembler { val prefix = if (queryStart < 0) baseUrl else baseUrl.substring(0, queryStart) val baseQuery = if (queryStart < 0) "" else baseUrl.substring(queryStart + 1) - val sb = StringBuilder(prefix.trimEnd('/')) + val sb = StringBuilder(prefix) + // Join the resolved path with exactly one '/'. A trailing '/' on the base is only + // significant when there is no path to join, so an empty path leaves the base untouched + // (e.g. `https://host/v1/` stays `…/v1/`) rather than being silently stripped. if (path.isNotEmpty()) { - if (!path.startsWith("/")) sb.append('/') + val baseEndsWithSlash = prefix.endsWith('/') + val pathStartsWithSlash = path.startsWith('/') + if (baseEndsWithSlash && pathStartsWithSlash) { + sb.setLength(sb.length - 1) + } else if (!baseEndsWithSlash && !pathStartsWithSlash) { + sb.append('/') + } sb.append(path) } // Base query first (it is ambient — auth tokens, API keys), operation query appended. - // Both sides are already percent-encoded wire form, so they concatenate verbatim. + // Both sides are already percent-encoded wire form, so they concatenate verbatim; a + // trailing '&' already on the base query is reused as the separator rather than doubled + // into an empty `&&` segment. val mergedQuery = when { baseQuery.isEmpty() -> query query.isEmpty() -> baseQuery + baseQuery.endsWith('&') -> baseQuery + query else -> "$baseQuery&$query" } if (mergedQuery.isNotEmpty()) { diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilder.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilder.kt index e353addf..5b73add1 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilder.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilder.kt @@ -23,16 +23,16 @@ import java.net.URL * * ## Query manipulation * - * Reading a parameter ([getQueryParam]) goes through the structured [QueryParams] model. - * Writing one ([setQueryParam]) splices the raw query string directly — replacing or - * appending only the targeted parameter and copying every other segment **verbatim** — so - * parameters the caller did not touch keep their exact wire form (a value-less `?flag` stays - * value-less; reserved characters are not rewritten). Newly written names and values are - * percent-encoded with RFC 3986 semantics (space → `%20`) via [PercentEncoding]. + * Both reading ([getQueryParam]) and writing ([setQueryParam]) operate on the raw query string + * directly — scanning or splicing segments and decoding/encoding only the targeted parameter via + * the shared [PercentEncoding] codec (RFC 3986 semantics: space → `%20`, literal `+` preserved). + * Every other segment is copied **verbatim**, so parameters the caller did not touch keep their + * exact wire form (a value-less `?flag` stays value-less; reserved characters are not rewritten). * * [QueryParams.encode] is deliberately *not* used to reassemble the whole query: it re-renders - * every parameter in canonical form, which would alter untouched segments. This editor favours - * fidelity over funnelling everything through the model — see the note on [QueryParams]. + * every parameter in canonical form (altering untouched segments) and would allocate a full + * multimap per page. This editor favours fidelity — and avoids that per-page allocation — over + * funnelling everything through the model; see the note on [QueryParams]. * * ## URL.fragment / userInfo * @@ -111,13 +111,28 @@ internal object RequestRebuilder { ): String = PercentEncoding.encodeComponent(name) + "=" + PercentEncoding.encodeComponent(value) /** - * Returns the value of the query parameter [name] from [url], or `null` if absent. - * The value is decoded with RFC 3986 semantics (see [QueryParams.parse]). + * Returns the value of the query parameter [name] from [url], or `null` if absent. The raw + * query is scanned directly (mirroring [setQueryParam]) and the matched name/value are + * decoded with RFC 3986 semantics via [PercentEncoding] — first match wins, and a value-less + * `?flag` reports `""`. No full [QueryParams] model is built per page. */ fun getQueryParam( url: URL, name: String, - ): String? = QueryParams.parse(url.query).get(name) + ): String? { + val existing = url.query + if (existing.isNullOrEmpty()) return null + for (segment in existing.split('&')) { + if (segment.isEmpty()) continue + val eq = segment.indexOf('=') + val key = if (eq < 0) segment else segment.substring(0, eq) + if (PercentEncoding.decodeComponent(key) == name) { + val rawValue = if (eq < 0) "" else segment.substring(eq + 1) + return PercentEncoding.decodeComponent(rawValue) + } + } + return null + } private fun rebuildUrl( source: URL, diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/PercentEncodingTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/PercentEncodingTest.kt index 279c93ec..a5e65fee 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/PercentEncodingTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/PercentEncodingTest.kt @@ -42,6 +42,29 @@ class PercentEncodingTest { assertEquals("Abc-123_x.y", PercentEncoding.encodeComponent("Abc-123_x.y")) } + @Test + fun `encode leaves the unreserved tilde untouched`() { + // '~' is in the RFC 3986 unreserved set; URLEncoder over-encodes it to %7E, which the + // codec corrects back to '~'. + assertEquals("~", PercentEncoding.encodeComponent("~")) + assertEquals("~user", PercentEncoding.encodeComponent("~user")) + } + + @Test + fun `encode percent-encodes the reserved asterisk`() { + // '*' is a sub-delimiter (reserved), not unreserved; URLEncoder leaves it raw, which the + // codec corrects to %2A so a component value cannot smuggle a reserved character. + assertEquals("%2A", PercentEncoding.encodeComponent("*")) + assertEquals("a%2Ab", PercentEncoding.encodeComponent("a*b")) + } + + @Test + fun `encode then decode round-trips the corrected tilde and asterisk`() { + val original = "~a*b" + assertEquals("~a%2Ab", PercentEncoding.encodeComponent(original)) + assertEquals(original, PercentEncoding.decodeComponent(PercentEncoding.encodeComponent(original))) + } + @Test fun `encode percent-encodes multibyte utf8 as utf8 bytes`() { assertEquals("%C3%A9", PercentEncoding.encodeComponent("é")) // é diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt index ccebb9f7..02c7cac8 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt @@ -149,6 +149,21 @@ class OperationParamsTest { assertEquals("https://api.example.com/pets?sv=X", request.url.toExternalForm()) } + @Test + fun `toRequest preserves a significant trailing slash when the path is empty`() { + // An empty path template targets the base resource itself; the base's trailing slash is + // semantic (some routers distinguish /v1/ from /v1) and must not be silently stripped. + val request = TestOp(Method.GET, "").toRequest("https://api.example.com/v1/") + assertEquals("https://api.example.com/v1/", request.url.toExternalForm()) + } + + @Test + fun `toRequest does not double the separator when the base query ends with an ampersand`() { + val query = QueryParams.builder().add("b", "2").build() + val request = TestOp(Method.GET, "/pets", query = query).toRequest("https://api.example.com/v1?a=1&") + assertEquals("https://api.example.com/v1/pets?a=1&b=2", request.url.toExternalForm()) + } + @Test fun `toRequest rejects a base URL carrying a fragment`() { assertFailsWith { diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilderTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilderTest.kt index ef75344d..0b20c8c6 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilderTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilderTest.kt @@ -96,4 +96,9 @@ class RequestRebuilderTest { fun `getQueryParam reads a literal plus as a plus`() { assertEquals("a+b", RequestRebuilder.getQueryParam(url("https://api.example.com/items?q=a+b"), "q")) } + + @Test + fun `getQueryParam reports an empty string for a value-less flag`() { + assertEquals("", RequestRebuilder.getQueryParam(url("https://api.example.com/items?flag&page=2"), "flag")) + } } From 55986dd60ade2da3484995910aab5c41ce0358d5 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 28 Jun 2026 18:41:44 +0300 Subject: [PATCH 6/7] feat: carry operationName through the request context chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OperationParams exposes an operationName for instrumentation, but toRequestContext discarded it — it promoted the DispatchContext without passing the name on, so the value never reached the context chain that the HttpTracerFactory.newTracer(operationName, ...) seam reads from. A service that set operationName still produced unlabelled contexts. Thread operationName through the promotion chain: - RequestContext and ExchangeContext gain an optional operationName, forwarded on promotion (RequestContext -> ExchangeContext). - DispatchContext.toRequestContext takes an optional operationName; @JvmOverloads keeps the existing single-argument method. - OperationParams.toRequestContext passes its operationName, so a named operation labels its context for tracing without affecting the assembled request's URL, headers, or body. Regenerate the sdk-core API snapshot and update the Operation Input Projection docs. Tests cover propagation through each link of the chain. --- docs/http.md | 6 +++-- sdk-core/api/sdk-core.api | 22 +++++++++++------- .../sdk/core/http/context/DispatchContext.kt | 16 +++++++++---- .../sdk/core/http/context/ExchangeContext.kt | 6 +++++ .../sdk/core/http/context/RequestContext.kt | 13 ++++++++--- .../sdk/core/operation/OperationParams.kt | 11 +++++---- .../core/http/context/DispatchContextTest.kt | 14 +++++++++++ .../core/http/context/RequestContextTest.kt | 11 +++++++++ .../sdk/core/operation/OperationParamsTest.kt | 23 +++++++++++++++++++ 9 files changed, 101 insertions(+), 21 deletions(-) diff --git a/docs/http.md b/docs/http.md index ca33afb3..25ac8ac1 100644 --- a/docs/http.md +++ b/docs/http.md @@ -790,8 +790,10 @@ parameterless operation overrides almost nothing. body/method compatibility. `toRequestContext` builds the `Request` and promotes a `DispatchContext` into a `RequestContext` -carrying it, in one step. Execution stays the pipeline's job — the SPI stops at producing the -request/context (error-mapping and deserialization compose at the service layer, not as pipeline +carrying it, in one step. `operationName` (when set) is carried onto that `RequestContext` and +forwarded down the chain to the `ExchangeContext`, so the tracing seam can label the operation; it +never alters the assembled request. Execution stays the pipeline's job — the SPI stops at producing +the request/context (error-mapping and deserialization compose at the service layer, not as pipeline stages). ```kotlin diff --git a/sdk-core/api/sdk-core.api b/sdk-core/api/sdk-core.api index 95c010d3..bb720f5e 100644 --- a/sdk-core/api/sdk-core.api +++ b/sdk-core/api/sdk-core.api @@ -677,6 +677,8 @@ public final class org/dexpace/sdk/core/http/context/DispatchContext : org/dexpa public fun getInstrumentationContext ()Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext; public fun hashCode ()I public final fun toRequestContext (Lorg/dexpace/sdk/core/http/request/Request;)Lorg/dexpace/sdk/core/http/context/RequestContext; + public final fun toRequestContext (Lorg/dexpace/sdk/core/http/request/Request;Ljava/lang/String;)Lorg/dexpace/sdk/core/http/context/RequestContext; + public static synthetic fun toRequestContext$default (Lorg/dexpace/sdk/core/http/context/DispatchContext;Lorg/dexpace/sdk/core/http/request/Request;Ljava/lang/String;ILjava/lang/Object;)Lorg/dexpace/sdk/core/http/context/RequestContext; public fun toString ()Ljava/lang/String; } @@ -685,18 +687,20 @@ public final class org/dexpace/sdk/core/http/context/DispatchContext$Companion { } public final class org/dexpace/sdk/core/http/context/ExchangeContext : org/dexpace/sdk/core/http/context/CallContext { - public fun (Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)V - public synthetic fun (Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/String;)V + public synthetic fun (Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/String;ILkotlin/jvm/internal/DefaultConstructorMarker;)V public fun close ()V public final fun component1 ()Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext; public final fun component2 ()Lorg/dexpace/sdk/core/http/request/Request; public final fun component3 ()Lorg/dexpace/sdk/core/http/response/Response; public final fun component4 ()Ljava/lang/String; - public final fun copy (Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;)Lorg/dexpace/sdk/core/http/context/ExchangeContext; - public static synthetic fun copy$default (Lorg/dexpace/sdk/core/http/context/ExchangeContext;Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;ILjava/lang/Object;)Lorg/dexpace/sdk/core/http/context/ExchangeContext; + public final fun component5 ()Ljava/lang/String; + public final fun copy (Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/String;)Lorg/dexpace/sdk/core/http/context/ExchangeContext; + public static synthetic fun copy$default (Lorg/dexpace/sdk/core/http/context/ExchangeContext;Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Lorg/dexpace/sdk/core/http/response/Response;Ljava/lang/String;Ljava/lang/String;ILjava/lang/Object;)Lorg/dexpace/sdk/core/http/context/ExchangeContext; public fun equals (Ljava/lang/Object;)Z public fun getCallKey ()Ljava/lang/String; public fun getInstrumentationContext ()Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext; + public final fun getOperationName ()Ljava/lang/String; public final fun getRequest ()Lorg/dexpace/sdk/core/http/request/Request; public final fun getResponse ()Lorg/dexpace/sdk/core/http/response/Response; public fun hashCode ()I @@ -704,17 +708,19 @@ public final class org/dexpace/sdk/core/http/context/ExchangeContext : org/dexpa } public final class org/dexpace/sdk/core/http/context/RequestContext : org/dexpace/sdk/core/http/context/CallContext { - public fun (Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Ljava/lang/String;)V - public synthetic fun (Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Ljava/lang/String;ILkotlin/jvm/internal/DefaultConstructorMarker;)V + public fun (Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Ljava/lang/String;Ljava/lang/String;)V + public synthetic fun (Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Ljava/lang/String;Ljava/lang/String;ILkotlin/jvm/internal/DefaultConstructorMarker;)V public fun close ()V public final fun component1 ()Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext; public final fun component2 ()Lorg/dexpace/sdk/core/http/request/Request; public final fun component3 ()Ljava/lang/String; - public final fun copy (Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Ljava/lang/String;)Lorg/dexpace/sdk/core/http/context/RequestContext; - public static synthetic fun copy$default (Lorg/dexpace/sdk/core/http/context/RequestContext;Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Ljava/lang/String;ILjava/lang/Object;)Lorg/dexpace/sdk/core/http/context/RequestContext; + public final fun component4 ()Ljava/lang/String; + public final fun copy (Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Ljava/lang/String;Ljava/lang/String;)Lorg/dexpace/sdk/core/http/context/RequestContext; + public static synthetic fun copy$default (Lorg/dexpace/sdk/core/http/context/RequestContext;Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext;Lorg/dexpace/sdk/core/http/request/Request;Ljava/lang/String;Ljava/lang/String;ILjava/lang/Object;)Lorg/dexpace/sdk/core/http/context/RequestContext; public fun equals (Ljava/lang/Object;)Z public fun getCallKey ()Ljava/lang/String; public fun getInstrumentationContext ()Lorg/dexpace/sdk/core/instrumentation/InstrumentationContext; + public final fun getOperationName ()Ljava/lang/String; public final fun getRequest ()Lorg/dexpace/sdk/core/http/request/Request; public fun hashCode ()I public final fun toExchangeContext (Lorg/dexpace/sdk/core/http/response/Response;)Lorg/dexpace/sdk/core/http/context/ExchangeContext; diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/DispatchContext.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/DispatchContext.kt index a0e0b03c..2e80a281 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/DispatchContext.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/DispatchContext.kt @@ -37,15 +37,23 @@ public data class DispatchContext( ) : CallContext { /** * Promotes this dispatch context into a [RequestContext] bound to [request] and stores - * the new context in [ContextStore] under this chain's [callKey]. After promotion this - * dispatch context becomes an intermediate link and must not be closed independently — - * close the returned [RequestContext] (or its own successor) instead. + * the new context in [ContextStore] under this chain's [callKey]. The optional + * [operationName] (a schema-defined operation id such as `"GetUser"`) is carried onto the + * [RequestContext] and forwarded down the chain so the tracing seam can label the operation; + * it does not affect the request or dispatch decision. After promotion this dispatch context + * becomes an intermediate link and must not be closed independently — close the returned + * [RequestContext] (or its own successor) instead. */ - public fun toRequestContext(request: Request): RequestContext = + @JvmOverloads + public fun toRequestContext( + request: Request, + operationName: String? = null, + ): RequestContext = RequestContext( instrumentationContext = instrumentationContext, request = request, callKey = callKey, + operationName = operationName, ).also { ContextStore.set(it.callKey, it) } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/ExchangeContext.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/ExchangeContext.kt index 58720e2f..3b647f5d 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/ExchangeContext.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/ExchangeContext.kt @@ -25,10 +25,16 @@ import org.dexpace.sdk.core.instrumentation.InstrumentationContext * a collision-safe store key. One consequence of the generated default: two default-constructed * instances are not structurally equal, since each generates a distinct key — pin an explicit * [callKey] if you need equality. + * + * @property operationName Optional schema-defined operation id (e.g. `"GetUser"`) for this call, + * or `null` for a raw request with no associated operation. Carried forward from the + * [RequestContext] for the tracing seam ([InstrumentationContext.httpTracerFactory]) so + * post-exchange span finalizers can label the operation. */ public data class ExchangeContext( override val instrumentationContext: InstrumentationContext, val request: Request, val response: Response, override val callKey: String = DispatchContext.generateCallKey(instrumentationContext), + val operationName: String? = null, ) : CallContext diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/RequestContext.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/RequestContext.kt index 8f3f1f41..9effc0e3 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/RequestContext.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/context/RequestContext.kt @@ -24,17 +24,23 @@ import org.dexpace.sdk.core.instrumentation.InstrumentationContext * a collision-safe store key. One consequence of the generated default: two default-constructed * instances are not structurally equal, since each generates a distinct key — pin an explicit * [callKey] if you need equality. + * + * @property operationName Optional schema-defined operation id (e.g. `"GetUser"`) for this call, + * or `null` for a raw request with no associated operation. Carried for the tracing seam + * ([InstrumentationContext.httpTracerFactory]) and forwarded to the [ExchangeContext]; it does + * not affect the request or dispatch decision. */ public data class RequestContext( override val instrumentationContext: InstrumentationContext, val request: Request, override val callKey: String = DispatchContext.generateCallKey(instrumentationContext), + val operationName: String? = null, ) : CallContext { /** * Promotes this request context into an [ExchangeContext] bound to [response] and stores - * the new context in [ContextStore] under this chain's [callKey]. After promotion this - * request context becomes an intermediate link and must not be closed independently — - * close the returned [ExchangeContext] instead. + * the new context in [ContextStore] under this chain's [callKey]. The [operationName] is + * carried forward. After promotion this request context becomes an intermediate link and + * must not be closed independently — close the returned [ExchangeContext] instead. */ public fun toExchangeContext(response: Response): ExchangeContext = ExchangeContext( @@ -42,6 +48,7 @@ public data class RequestContext( request = request, response = response, callKey = callKey, + operationName = operationName, ).also { ContextStore.set(it.callKey, it) } diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationParams.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationParams.kt index 896f7312..af4548f8 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationParams.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationParams.kt @@ -55,8 +55,10 @@ public interface OperationParams { public val pathTemplate: String /** - * A stable operation identifier (e.g. `"ListPets"`) for instrumentation, or `null`. Carried - * for the tracing seam; it does not affect the assembled request. + * A stable operation identifier (e.g. `"ListPets"`) for instrumentation, or `null`. + * [toRequestContext] carries it onto the [RequestContext] (and through to the + * [org.dexpace.sdk.core.http.context.ExchangeContext]) so the tracing seam can label the + * operation; it does **not** affect the assembled request's URL, headers, or body. */ public val operationName: String? get() = null @@ -90,10 +92,11 @@ public interface OperationParams { /** * Assembles the [Request] ([toRequest]) and promotes [dispatch] into a [RequestContext] - * carrying it — feeding the request into the context chain in one step. + * carrying it — feeding the request into the context chain in one step. [operationName] is + * carried onto the resulting context for the tracing seam. */ public fun toRequestContext( baseUrl: String, dispatch: DispatchContext, - ): RequestContext = dispatch.toRequestContext(toRequest(baseUrl)) + ): RequestContext = dispatch.toRequestContext(toRequest(baseUrl), operationName) } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/context/DispatchContextTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/context/DispatchContextTest.kt index a821aa2c..a40152b4 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/context/DispatchContextTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/context/DispatchContextTest.kt @@ -14,6 +14,7 @@ import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertNotEquals import kotlin.test.assertNotNull +import kotlin.test.assertNull import kotlin.test.assertSame class DispatchContextTest { @@ -49,6 +50,19 @@ class DispatchContextTest { assertEquals(dispatch.callKey, promoted.callKey) // Promotion registers the new context under the chain's call key. assertSame(promoted, ContextStore.get(promoted.callKey)) + // No operation name supplied — the context carries null. + assertNull(promoted.operationName) + } + + @Test + fun `toRequestContext carries an explicit operation name onto the promoted context`() { + val id = owned("opname") + val dispatch = DispatchContext(FakeInstrumentationContext(TraceId(id))) + ownedIds.add(dispatch.callKey) + + val promoted = dispatch.toRequestContext(request(), "GetUser") + + assertEquals("GetUser", promoted.operationName) } @Test diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/context/RequestContextTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/context/RequestContextTest.kt index 594214c7..35208751 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/context/RequestContextTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/context/RequestContextTest.kt @@ -52,6 +52,17 @@ class RequestContextTest { assertSame(promoted, ContextStore.get(promoted.callKey)) } + @Test + fun `toExchangeContext carries the operation name forward`() { + val instr = FakeInstrumentationContext(TraceId(owned("opname"))) + val parent = RequestContext(instr, request(), operationName = "GetUser") + ownedIds.add(parent.callKey) + + val promoted = parent.toExchangeContext(response()) + + assertEquals("GetUser", promoted.operationName) + } + @Test fun `data class equality is by content`() { val instr = FakeInstrumentationContext(TraceId(owned("eq"))) diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt index 02c7cac8..d1683617 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt @@ -136,6 +136,29 @@ class OperationParamsTest { } } + @Test + fun `toRequestContext carries the operation name onto the context for the tracing seam`() { + val dispatch = DispatchContext.default() + val op = TestOp(Method.GET, "/pets", operationName = "ListPets") + val ctx = op.toRequestContext(base, dispatch) + try { + assertEquals("ListPets", ctx.operationName) + } finally { + ctx.close() + } + } + + @Test + fun `toRequestContext leaves the operation name null when the operation declares none`() { + val dispatch = DispatchContext.default() + val ctx = TestOp(Method.GET, "/pets").toRequestContext(base, dispatch) + try { + assertNull(ctx.operationName) + } finally { + ctx.close() + } + } + @Test fun `toRequest merges a base URL query ahead of the operation query`() { val query = QueryParams.builder().add("limit", "20").build() From 5d16d0853575ccc9e3e38bffdb78fa499fda1bed Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 28 Jun 2026 18:54:49 +0300 Subject: [PATCH 7/7] fix: tidy operation URL composition edge cases OperationRequestAssembler left a trailing '&' from the base URL's query in place when the operation contributed no query of its own, emitting a dangling `?a=1&`. Trim a single trailing separator before merging so the base query joins cleanly whether or not an operation query follows. Widen the path-template variable pattern from `[^/}]` to `[^}]` so a malformed `{a/b}` placeholder is captured whole and fails fast as a missing path parameter, instead of leaking an unsubstituted `{a/b}` literal into the assembled URL. --- .../operation/OperationRequestAssembler.kt | 20 +++++++++++-------- .../sdk/core/operation/OperationParamsTest.kt | 17 ++++++++++++++++ 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt index 90fb1942..036e47eb 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/operation/OperationRequestAssembler.kt @@ -24,7 +24,10 @@ import java.net.MalformedURLException * never sent on the wire, so it is almost certainly a mistake. */ internal object OperationRequestAssembler { - private val TEMPLATE_VARIABLE = Regex("""\{([^/}]+)}""") + // A `{name}` placeholder: any run of characters up to the first '}'. The class is `[^}]` (not + // `[^/}]`) so a stray '/' inside the braces is captured as the variable name and fails fast as + // a missing path parameter, rather than leaving an unsubstituted `{a/b}` literal in the URL. + private val TEMPLATE_VARIABLE = Regex("""\{([^}]+)}""") internal fun assemble( baseUrl: String, @@ -92,15 +95,16 @@ internal object OperationRequestAssembler { sb.append(path) } // Base query first (it is ambient — auth tokens, API keys), operation query appended. - // Both sides are already percent-encoded wire form, so they concatenate verbatim; a - // trailing '&' already on the base query is reused as the separator rather than doubled - // into an empty `&&` segment. + // Both sides are already percent-encoded wire form, so they concatenate verbatim. A + // trailing '&' on the base query is its own separator: drop it before joining, so it + // neither doubles into an empty `&&` segment when an operation query follows nor survives + // as a dangling separator when the base contributes the only query. + val trimmedBase = baseQuery.removeSuffix("&") val mergedQuery = when { - baseQuery.isEmpty() -> query - query.isEmpty() -> baseQuery - baseQuery.endsWith('&') -> baseQuery + query - else -> "$baseQuery&$query" + trimmedBase.isEmpty() -> query + query.isEmpty() -> trimmedBase + else -> "$trimmedBase&$query" } if (mergedQuery.isNotEmpty()) { sb.append('?').append(mergedQuery) diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt index d1683617..26ed7a64 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/operation/OperationParamsTest.kt @@ -110,6 +110,15 @@ class OperationParamsTest { } } + @Test + fun `toRequest fails fast on a template variable containing a slash rather than emitting it literally`() { + // A malformed `{a/b}` placeholder is captured whole as the variable name; with no matching + // path parameter it fails fast instead of leaking an unsubstituted `{a/b}` into the URL. + assertFailsWith { + TestOp(Method.GET, "/pets/{a/b}").toRequest(base) + } + } + @Test fun `minimal operation uses empty defaults`() { val op = MinimalOp() @@ -187,6 +196,14 @@ class OperationParamsTest { assertEquals("https://api.example.com/v1/pets?a=1&b=2", request.url.toExternalForm()) } + @Test + fun `toRequest drops a dangling base query separator when the operation contributes none`() { + // A trailing '&' on the base query is its own separator, not an empty parameter; with no + // operation query to join it must not survive as a dangling `?a=1&`. + val request = TestOp(Method.GET, "/pets").toRequest("https://api.example.com/v1?a=1&") + assertEquals("https://api.example.com/v1/pets?a=1", request.url.toExternalForm()) + } + @Test fun `toRequest rejects a base URL carrying a fragment`() { assertFailsWith {