Fix query parameters being silently dropped from HTTP requests#57
Merged
dcrockwell merged 1 commit intodevelopfrom Mar 3, 2026
Merged
Conversation
## Why This Change Was Made
- The `query()` builder function let callers set query parameters on a request,
and the value was correctly stored on the ClientRequest and copied through
`to_http_request` — but the final URL assembly step silently dropped it.
Every request made with `.query("key=value")` sent the request without any
query parameters. The server never received them. This affected all three
execution modes: `send()`, `stream_yielder()`, and `start_stream()`.
## What Was Changed
- `build_url` in `client.gleam` now appends `?query` to the URL when
`request.query` is `Some(query)`. This fixes `send()` and `start_stream()`.
- `start_httpc_stream` in `internal.gleam` had its own independent URL
construction that also omitted the query string — same fix applied. This
fixes `stream_yielder()`.
- Mock server's `GET /get` endpoint now actually echoes query parameters
(was documented since 1.0.0 but never implemented — the controller only
passed `request.path` to the view).
- 8 regression tests added with exact JSON field assertions covering all
three execution modes, recorder integration, URL-encoded special characters,
and the empty query string edge case.
- Version bumped: http_client 5.1.1 → 5.1.2, mock_server 1.1.0 → 1.1.1.
- CHANGELOGs, READMEs, and release notes updated for both modules.
## Note to Future Engineer
- There are TWO separate URL construction sites: `build_url` in client.gleam
(used by send/start_stream) and inline URL building in `start_httpc_stream`
in internal.gleam (used by stream_yielder). If you ever add a fourth
execution mode, you'll need to make sure it also includes query params —
or better yet, refactor both call sites to share a single URL builder.
- The mock server's GET /get endpoint claimed to echo query params for over
three months before anyone noticed it didn't. The CHANGELOG said it did.
The code said otherwise. Trust the code, not the comments. Or the CHANGELOG.
Especially not the CHANGELOG.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.query()builder were silently dropped from all outgoing HTTP requests — the server never received themGET /getendpoint to actually echo query parameters (was documented but never implemented)Why
The
.query("key=value")builder function correctly stored the query string internally, but the final step that assembled the URL for the actual HTTP request never included it. Any user relying on.query()to pass query parameters to a server was sending requests without them — with no error or warning. This affected all three ways to make a request: blocking, yielder streaming, and callback streaming.What
Two bug fixes in the HTTP client:
The URL was assembled in two separate places in the codebase, and both had the same omission. One is used by
send()andstart_stream(), the other bystream_yielder(). Both now append?query_stringto the URL when query parameters are present.One fix in the mock server:
The
GET /getendpoint was documented as echoing query parameters since v1.0.0, but the implementation only usedrequest.path— the query string was ignored. This is why no existing test could have caught the client bug. Now the endpoint includes a"query"field in its JSON response.Eight regression tests:
All three execution modes are tested end-to-end against the mock server. Assertions parse the JSON response and do exact field matching (not substring checks). Additional tests cover URL-encoded special characters, empty query strings, and recorder record/playback round-trips with query parameters.
How
The fix itself is minimal — four lines of Gleam in each of the two URL construction sites:
The query string is then concatenated onto the end of the assembled URL. When no query is set (the default), nothing changes.
Test plan
send()delivers query params to server (exact JSON assertion)stream_yielder()delivers query params to serverstart_stream()delivers query params to server