From e569731d42271ebaa9aa6e4fb87764b34ca8953b Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Wed, 17 Jun 2026 05:25:18 +0300 Subject: [PATCH] feat: add a QueryParams multimap and route query manipulation through it Introduce an immutable, insertion-ordered, multi-valued QueryParams type in http.common, modelled on Headers: private constructor, mutable Builder with add/set/remove, get(name)/values(name)/contains(name)/names()/entries(), and external (decoded) vs encoded (application/x-www-form-urlencoded, UTF-8) forms via encode()/parse(). Unlike Headers, names are case-sensitive and values may be empty or value-less (?flag), matching how query strings appear on the wire. Replace the placeholder QueryParam stub (and its NotImplementedError test) with this real, public type. Rewire pagination's RequestRebuilder to use QueryParams for query manipulation instead of split('&') string surgery; the PageNumber/Cursor/LinkHeader strategies flow through the rebuilder unchanged. The previous approach only handled single-valued params and re-implemented percent-encoding inline. Document, in docs/http.md, the request-URL model decision this commits to: keep java.net.URL as the resolved URL container (preserving DNS-free textual equality) and layer QueryParams for query manipulation, rather than a fully deconstructed URL value object. Add the QueryParams Common Types section and File Index entry, and regenerate the sdk-core API snapshot. --- docs/http.md | 99 +++++++ sdk-core/api/sdk-core.api | 38 +++ .../org/dexpace/sdk/core/http/QueryParam.kt | 20 -- .../sdk/core/http/common/QueryParams.kt | 269 ++++++++++++++++++ .../sdk/core/pagination/RequestRebuilder.kt | 70 +---- .../dexpace/sdk/core/http/QueryParamTest.kt | 36 --- .../sdk/core/http/common/QueryParamsTest.kt | 256 +++++++++++++++++ 7 files changed, 673 insertions(+), 115 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/QueryParams.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/QueryParamsTest.kt diff --git a/docs/http.md b/docs/http.md index 18ae9fe9..9ca8fcd6 100644 --- a/docs/http.md +++ b/docs/http.md @@ -438,6 +438,59 @@ 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 two ways: names are **case-sensitive** +(query-parameter names are opaque to HTTP, so `?page=1` and `?Page=1` are distinct), and values +may be **empty or value-less** (`?flag` and `?flag=` both occur in the wild). + +```kotlin +@ConsistentCopyVisibility +data class QueryParams private constructor( + private val paramsMap: Map> +) +``` + +**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()` | `application/x-www-form-urlencoded` wire form (UTF-8), without leading `?` | +| `newBuilder()` | Returns a pre-filled `Builder` for modification | + +The model stores **decoded** names and values. `QueryParams.parse(query)` is the inverse of +`encode()` — it decodes a raw query string (with or without a leading `?`) back into a +`QueryParams`, tolerating value-less params, empty segments, and malformed percent-encoding +(which falls back to the raw text rather than throwing). `parse(encode(...))` round-trips +names, values, and order. + +**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" +``` + +Pagination's `RequestRebuilder` and the `PageNumber`/`Cursor`/`LinkHeader` strategies use +`QueryParams` internally for query manipulation, replacing the previous `split('&')` string +surgery (which only handled single-valued params). + ### MediaType `MediaType` represents a parsed MIME type with optional parameters: @@ -749,6 +802,51 @@ 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 rather than +folded into the URL type itself. + +**Decision: keep `java.net.URL` as the URL container; layer `QueryParams` for query +manipulation.** + +Two models were weighed: + +1. **Resolved `java.net.URL` + a `QueryParams` layer (chosen).** The URL stays an opaque, + already-resolved string container; when a component needs to read or rewrite the query, it + parses `url.query` into a `QueryParams`, mutates through the builder, and re-encodes + (see `pagination/RequestRebuilder`). Path, host, port, userInfo, and fragment are spliced + back verbatim. +2. **A fully deconstructed `Url` value object.** Every component (scheme, host, port, + path-segments, query) modelled as typed fields, assembled into a string only at the + transport edge. + +Rationale for (1): + +- **DNS-free equality is preserved trivially.** `Request` equality compares `url.toExternalForm()` + — a pure string comparison that performs **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 means that contract carries over unchanged. A deconstructed + model would have to re-establish equivalent textual equality across all its components, with + more surface area for it to drift. +- **The query string is where the real manipulation pressure is.** Pagination, request signing, + and parameter projection all manipulate the **query**, not the host or path. `QueryParams` + puts a structured, multi-valued, well-tested model exactly where it is needed without forcing a + rewrite of how every transport consumes a URL. +- **Transports already speak `java.net.URL` / strings.** Both reference transports (OkHttp, JDK + `HttpClient`) accept a resolved URL or string directly. A deconstructed model would add an + assembly step at every transport boundary for no functional gain today. +- **No premature generality.** A deconstructed model earns its cost when path templating and + per-segment encoding become first-class concerns (e.g. an operation/path-template SPI). Until + then it is speculative structure. `QueryParams` is the runtime primitive a future path/URL + model would compose with, not something it would have to replace. + +The trade-off accepted is that path-segment / host manipulation still goes through string work +(as in `RequestRebuilder.rebuildUrl`). That is deliberate: it is rare, localised, and not worth +a deconstructed URL type pre-1.0. + --- ## Usage Examples @@ -860,6 +958,7 @@ 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 | | `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 7ebcc312..abdf3518 100644 --- a/sdk-core/api/sdk-core.api +++ b/sdk-core/api/sdk-core.api @@ -474,6 +474,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 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/QueryParams.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/QueryParams.kt new file mode 100644 index 00000000..92983a3e --- /dev/null +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/common/QueryParams.kt @@ -0,0 +1,269 @@ +/* + * 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 +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 two 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, matching `application/x-www-form-urlencoded` parsing. + * + * ## External vs encoded form + * + * The model stores **decoded** names and values. [encode] renders the + * `application/x-www-form-urlencoded` wire form (UTF-8 percent-encoding via the JDK's + * [URLEncoder]); [parse] is its inverse, decoding a raw query string back into a + * [QueryParams]. Round-tripping `parse(encode(...))` preserves names, values, and order. + * + * ## 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") +@ConsistentCopyVisibility +public data 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 number of distinct *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 in `application/x-www-form-urlencoded` form, percent-encoding + * names and values with UTF-8. 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 = URLEncoder.encode(name, UTF_8) + valueList.forEach { value -> + if (sb.isNotEmpty()) sb.append('&') + sb.append(encodedName).append('=').append(URLEncoder.encode(value, UTF_8)) + } + } + return sb.toString() + } + + /** + * Returns a new [Builder] initialised with these parameters. + */ + public fun newBuilder(): Builder = Builder(this) + + override fun toString(): String = paramsMap.toString() + + /** + * 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.entries().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 { + private const val UTF_8: String = "UTF-8" + + /** 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 UTF-8 per + * `application/x-www-form-urlencoded`. + * + * - 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, so a + * legacy URL a transport already accepted cannot break parsing. + */ + @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(decodeOrRaw(segment), "") + } else { + val name = decodeOrRaw(segment.substring(0, eq)) + val value = decodeOrRaw(segment.substring(eq + 1)) + builder.add(name, value) + } + } + return builder.build() + } + + private fun decodeOrRaw(raw: String): String = + try { + URLDecoder.decode(raw, UTF_8) + } catch (ignored: IllegalArgumentException) { + // Malformed percent-encoding — keep the raw text so an unencoded ASCII + // identifier (the common case) still round-trips and equality with a + // caller-supplied name still holds. + raw + } + } +} 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 16bd0655..9ca3a1c1 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,10 +7,9 @@ package org.dexpace.sdk.core.pagination +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 @@ -22,12 +21,13 @@ import java.net.URLEncoder * 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. + * Query manipulation goes through the structured [QueryParams] multimap rather than + * `split('&')` string surgery: the URL's query string is parsed into [QueryParams], the + * targeted parameter is mutated through the builder, and the result is re-encoded. + * [QueryParams] uses `application/x-www-form-urlencoded` semantics (UTF-8 + * percent-encoding) — the same convention browsers and servers use for query strings. * * ## URL.fragment / userInfo * @@ -35,8 +35,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,29 +74,9 @@ 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 - if (!existing.isNullOrEmpty()) { - for (segment in existing.split('&')) { - if (segment.isEmpty()) continue - val key = segment.substringBefore('=', segment) - if (decodeOrRaw(key) == name) { - if (!replaced && value != null) { - outParams.add(encodedName + "=" + URLEncoder.encode(value, UTF_8)) - replaced = true - } - // else: dropping this param (value == null) or already replaced once. - } else { - outParams.add(segment) - } - } - } - if (!replaced && value != null) { - outParams.add(encodedName + "=" + URLEncoder.encode(value, UTF_8)) - } - val newQuery: String? = if (outParams.isEmpty()) null else outParams.joinToString("&") + val params = QueryParams.parse(url.query) + val updated = params.newBuilder().set(name, value).build() + val newQuery: String? = if (updated.isEmpty()) null else updated.encode() return rebuildUrl(url, newQuery) } @@ -109,33 +87,7 @@ internal object RequestRebuilder { 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 eq = segment.indexOf('=') - val rawKey = if (eq >= 0) segment.substring(0, eq) else segment - if (decodeOrRaw(rawKey) == name) { - val rawValue = if (eq >= 0) segment.substring(eq + 1) else "" - return decodeOrRaw(rawValue) - } - } - return null - } - - private fun decodeOrRaw(raw: String): String = - try { - URLDecoder.decode(raw, UTF_8) - } catch (e: 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 `e` here; pagination should not fail on malformed legacy - // URLs that the transport accepted. - @Suppress("UNUSED_VARIABLE") - val ignored = e - 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/QueryParamsTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/QueryParamsTest.kt new file mode 100644 index 00000000..54e1d259 --- /dev/null +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/common/QueryParamsTest.kt @@ -0,0 +1,256 @@ +/* + * 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.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 with utf8`() { + val params = + QueryParams.builder() + .add("q", "a b&c") + .add("name+key", "x") + .build() + assertEquals("q=a+b%26c&name%2Bkey=x", 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+b%26c&page=2") + assertEquals("a b&c", params.get("q")) + assertEquals("2", params.get("page")) + } + + @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%") + .build() + val roundTripped = QueryParams.parse(original.encode()) + assertEquals(original.names().toList(), roundTripped.names().toList()) + assertEquals(original.values("q"), roundTripped.values("q")) + assertEquals(original.values("tag"), roundTripped.values("tag")) + assertEquals(original.values("special"), roundTripped.values("special")) + } + + @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`() { + 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()) + } +}