Skip to content

feat: add a QueryParams multimap and route query manipulation through it#147

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
feat/query-params-multimap
Open

feat: add a QueryParams multimap and route query manipulation through it#147
OmarAlJarrah wants to merge 1 commit into
mainfrom
feat/query-params-multimap

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Summary

Adds a first-class QueryParams multimap and routes pagination's query manipulation through it, replacing the placeholder QueryParam stub and the split('&') string surgery in RequestRebuilder.

What changed

  • New QueryParams type (org.dexpace.sdk.core.http.common) — immutable, insertion-ordered, multi-valued, modelled on Headers: private constructor + Builder with add/set/remove, plus get(name) / values(name) / contains(name) / names() / entries() / size() / isEmpty(). External (decoded) vs encoded (application/x-www-form-urlencoded, UTF-8) forms via encode() and the inverse parse(query), which round-trips names, values, and order. Unlike Headers, names are case-sensitive and values may be empty or value-less (?flag), matching how query strings appear on the wire. @JvmStatic factories (builder(), empty(), parse(...)) for Java callers.
  • Removed the QueryParam stub (fun implementation(): Nothing = TODO()) and its NotImplementedError test.
  • Rewired RequestRebuilder to parse url.query into QueryParams, mutate through the builder, and re-encode — no more split('&'). The PageNumber / Cursor / LinkHeader strategies flow through the rebuilder unchanged.
  • docs/http.md — records the request-URL model decision (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) with rationale; adds the QueryParams Common Types section and File Index entry.
  • apiDump — regenerated sdk-core/api/sdk-core.api.

Note for reviewers

This overlaps with PR #145 (pagination unification) — both touch RequestRebuilder. Please sequence the merges; whichever lands second will need a small rebase on that file.

Gated build (scoped, run locally — no CI yet)

./gradlew :sdk-core:test :sdk-core:ktlintCheck :sdk-core:detekt :sdk-core:apiCheck --no-daemon

Result: BUILD SUCCESSFUL. QueryParamsTest and all pagination strategy tests (PageNumberPaginationTest, CursorPaginationTest, LinkHeaderPaginationTest, plus laziness/cap/single-read suites) pass; ktlint, detekt, and apiCheck are green. :sdk-core:apiDump was run and the regenerated snapshot committed.

Closes #28
Closes #29

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.
@OmarAlJarrah

Copy link
Copy Markdown
Member Author

This adds a public QueryParams immutable multimap in http.common and routes RequestRebuilder's query read/set/remove through QueryParams.parse/encode instead of the old hand-rolled split('&') surgery. It follows the immutable-data/Builder conventions, carries the license header, ships an updated .api snapshot, and comes with a solid test suite. A few things worth addressing before merging, but nothing blocking.

Issues

  • Untouched query params are now re-encoded instead of preserved verbatimsdk-core/src/main/kotlin/org/dexpace/sdk/core/pagination/RequestRebuilder.kt:72-81. The previous setQueryParam re-appended each non-targeted segment's raw string and only re-encoded the one param it changed. Parsing the whole query into QueryParams and re-encoding everything via URLEncoder means untouched params can change their on-wire form: ids=1,2,3ids=1%2C2%2C3, path=/a/bpath=%2Fa%2Fb, name=John%20Doename=John+Doe. These all decode to the same value, so any server that URL-decodes is fine, but a server treating query values as raw would see a param it never asked to change move. If this is the intended trade-off, the setQueryParam KDoc (lines 67-71) should say so — right now it only documents ordering, and the "preserved verbatim" note was dropped from the class doc.

Worth double-checking

  • QueryParams is a data class whose equality derives from the backing Map<String, List<String>>. Since Map.equals is order-insensitive, two instances with the same name→values in different insertion order compare equal and hash equal, yet encode() emits in insertion order and would produce different output for the two. Is order-insensitive equality the intended contract for a type that advertises itself as insertion-ordered? Either way a short doc note (and a test covering the differing-order case, which equality is by value doesn't) would lock the decision in.

  • The add(name: String, values: List<String>) Builder overload and toString() have no direct test. Not a correctness concern and unlikely to threaten the Kover floor, but they're public surface currently unverified — worth a couple of lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Decide between a resolved java.net.URL and a deconstructed request URL model Build a first-class QueryParams multimap and remove the QueryParam stub

1 participant