Fix URL encoding of query parameter delimiter in relative paths#170
Fix URL encoding of query parameter delimiter in relative paths#170
Conversation
Add test cases that verify query parameters in relative paths are preserved correctly. Currently fails because `?` is being URL-encoded as `%3F` when route contains query parameters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Parse route with url.Parse to separate path and query components before joining with base URL. This prevents `?` from being included in u.Path and subsequently encoded as `%3F` by u.String(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
/run-e2e |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in ResolveMethodAndURL where relative paths containing query parameters (e.g., /search?q=hello) were incorrectly percent-encoded by path.Join, turning ? into %3F. The fix parses the route with url.Parse to properly separate the path and query components, then assigns them independently on the base URL struct.
Changes:
http/client.go: Replaces the singlepath.Join(u.Path, route)call with aurl.Parse(route)step that separates the route's path and raw query, setting them onuindependently.http/client_test.go: Adds three new test cases covering relative paths with query parameters and an absolute URL with query parameters.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
http/client.go |
Core fix: parse the route URL before joining, and propagate RawQuery separately |
http/client_test.go |
New test cases for query-parameter URL handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| wantErr: false, | ||
| }, | ||
| { |
There was a problem hiding this comment.
There is no test case covering the scenario where the base URL already contains query parameters (e.g., "url": "https://api.example.com?token=abc") and the route also carries query parameters. Given the current implementation silently overwrites the base URL's RawQuery, this edge case should have a test to document and verify the intended behavior.
| { | |
| { | |
| name: "GET method with base URL and route both containing query parameters", | |
| input: map[string]any{ | |
| "get": "/search?q=hello&page=1", | |
| "url": "https://api.example.com?token=abc", | |
| }, | |
| expected: map[string]any{ | |
| "method": "GET", | |
| // The route's query parameters overwrite the base URL's RawQuery. | |
| "url": "https://api.example.com/search?q=hello&page=1", | |
| }, | |
| wantErr: false, | |
| }, | |
| { |
| if routeURL.RawQuery != "" { | ||
| u.RawQuery = routeURL.RawQuery | ||
| } |
There was a problem hiding this comment.
The condition if routeURL.RawQuery != "" only sets the query when the route has one, but does not handle the case where the base URL already contains query parameters AND the route also has query parameters. In that scenario, the base URL's existing query string is silently overwritten by the route's query string, which may discard important base-URL parameters (e.g., API keys or tokens in the base URL like https://api.example.com?token=abc).
If merging base and route query parameters is the intended behavior, the two query strings should be combined (e.g., using url.Values to merge them). If overwriting is intentional, a comment explaining this design decision would help future readers. There is also no test covering the case where both the base URL and the route carry query parameters.
| if routeURL.RawQuery != "" { | |
| u.RawQuery = routeURL.RawQuery | |
| } | |
| // Merge query parameters from base URL and route instead of overwriting. | |
| baseQuery := u.Query() | |
| if routeURL.RawQuery != "" { | |
| routeQuery := routeURL.Query() | |
| for key, values := range routeQuery { | |
| for _, v := range values { | |
| baseQuery.Add(key, v) | |
| } | |
| } | |
| } | |
| u.RawQuery = baseQuery.Encode() |
| routeURL, err := url.Parse(route) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid route: %w", err) | ||
| } | ||
| u.Path = path.Join(u.Path, routeURL.Path) |
There was a problem hiding this comment.
The PR description's test plan still shows two items as unchecked: "Implement fix in ResolveMethodAndURL" and "Verify all tests pass". The fix has clearly been implemented in the diff, so these checkboxes appear to have been left in their pre-implementation state by mistake.
|
✅ E2E test passed successfully! |
Document the current behavior where route query parameters overwrite the base URL's query string. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Code Metrics Report
Details | | main (147e47a) | #170 (ff438aa) | +/- |
|---------------------|----------------|----------------|-------|
+ | Coverage | 55.9% | 55.9% | +0.0% |
| Files | 65 | 65 | 0 |
| Lines | 6548 | 6553 | +5 |
+ | Covered | 3663 | 3666 | +3 |
| Code to Test Ratio | 1:1.0 | 1:1.0 | 0.0 |
| Code | 12848 | 12848 | 0 |
| Test | 13267 | 13267 | 0 |
+ | Test Execution Time | 24s | 11s | -13s |Code coverage of files in pull request scope (78.4% → 78.0%)
Reported by octocov |
Summary
ResolveMethodAndURLincorrectly encoding?as%3Fwhen relative paths contain query parameters (e.g.,/search?q=hello)path.Joinmerges the query string intou.Path, causingu.String()to percent-encode the?delimiterurl.Parseto separate path and query components before settingu.Pathandu.RawQueryindependentlyTest plan
ResolveMethodAndURL🤖 Generated with Claude Code