Skip to content

fix(uri): ? operator now appends to existing query string#25831

Open
n0madgang wants to merge 5 commits into
nim-lang:develfrom
n0madgang:fix/19782-uri-query-concat
Open

fix(uri): ? operator now appends to existing query string#25831
n0madgang wants to merge 5 commits into
nim-lang:develfrom
n0madgang:fix/19782-uri-query-concat

Conversation

@n0madgang
Copy link
Copy Markdown
Contributor

Summary

Fixes #19782.

The ? operator in std/uri was silently overwriting any query string already present in the URI. This PR makes it append instead — which matches the docstring ("Concatenates the query parameters") and the natural expectation when chaining operations.

Before:

let u = parseUri("https://example.com/foo?existing=1") ? {"bar": "qux"}
echo $u  # https://example.com/foo?bar=qux  (existing=1 lost)

After:

let u = parseUri("https://example.com/foo?existing=1") ? {"bar": "qux"}
echo $u  # https://example.com/foo?existing=1&bar=qux

Changes

  • lib/pure/uri.nim: fix ? to append with & when a query string already exists; add example to runnableExamples
  • tests/stdlib/turi.nim: two new test cases (append to existing query, empty params preserve existing)
  • changelog.md: entry under Standard library changes

Notes

I work with Claude as a co-processor. I'm 56, came to programming late, and this is genuinely how I learn and contribute. I understand what I'm submitting, but I didn't write it alone. If your project prefers human-only contributions, just say so and I'll close without friction.

Previously `?` would overwrite any existing query parameters in the URI.
Now it appends the new parameters with `&` when a query string is already present.

Fixes nim-lang#19782
Copilot AI review requested due to automatic review settings May 21, 2026 04:31
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates Nim’s std/uri query concatenation behavior so adding query parameters appends to an existing query string (fixing #19782), and adds regression tests plus a changelog entry.

Changes:

  • Change std/uri’s ? operator to append new encoded query parameters when a query already exists.
  • Add regression tests for appending behavior and for empty parameter lists.
  • Document the behavior change in changelog.md.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
tests/stdlib/turi.nim Adds regression coverage for appending to existing query strings and for empty param lists.
lib/pure/uri.nim Implements new ? operator behavior to append query parameters rather than replace.
changelog.md Documents the behavior change and links the fixed issue.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/pure/uri.nim Outdated
Comment on lines +496 to +502
result = u
result.query = encodeQuery(query)
let newQuery = encodeQuery(query)
if result.query.len > 0 and newQuery.len > 0:
result.query.add('&')
result.query.add(newQuery)
else:
result.query = newQuery
When `encodeQuery(query)` returns an empty string (e.g. `? {:}`), the
previous else-branch would assign `result.query = ""`, silently clearing
any existing query string. Restructure the condition so that an empty
newQuery leaves result.query unchanged.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@n0madgang
Copy link
Copy Markdown
Contributor Author

Thanks for catching this. Copilot is right: when query is an empty array, encodeQuery returns "", so the else branch was assigning result.query = "" and wiping out whatever was already there. Not what the test expected, not what the docstring says.

Fixed in 0addc30: restructured so that an empty newQuery simply does nothing. Now result.query is only touched when there is actually something to add.

Sorry this slipped through. The machine I was working on has Nim 1.6.14, and the devel test file uses std/assertions which isn't available there, so I couldn't run the full suite. I compiled the library, saw it succeed, and called that "tests pass" which was wrong. Won't happen again.

n0madgang and others added 2 commits May 24, 2026 13:32
Additional regression test for issue nim-lang#19782 covering the
"empty + empty" case: applying `?` with no params to a URI
that has no existing query string must be a no-op.

Verified locally with:
  nim c -r --lib:./lib tests/stdlib/turi.nim
All assertion blocks for nim-lang#19782 pass (4 cases).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@n0madgang
Copy link
Copy Markdown
Contributor Author

Status update on the Copilot review:

The earlier else: result.query = newQuery branch that silently cleared the existing query has been restructured (commit 0addc30). The new logic only mutates result.query when encodeQuery(query).len > 0, so an empty params list now leaves any existing query string untouched.

Verified locally with the forked stdlib:

nim c -r --lib:./lib tests/stdlib/turi.nim

Exit 0. The four relevant blocks all pass:

  1. parseUri("http://example.com/foo?existing=1") ? {"bar": "qux"}http://example.com/foo?existing=1&bar=qux
  2. parseUri("http://example.com/foo?existing=1") ? {:}http://example.com/foo?existing=1 (existing preserved)
  3. parseUri("http://example.com/foo") ? {:}http://example.com/foo (no-op, added in abd24b0)
  4. parseUri("http://example.com/foo") ? {"bar": "qux"}http://example.com/foo?bar=qux (original behavior preserved)

Latest commit abd24b0 adds the empty-on-empty edge case explicitly. Ready for re-review.

Co-authored with Claude Code (Anthropic) — disclosed in each commit footer.

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.

uri.? doesn't concatenate

2 participants