fix: merge incoming request query params with redirect destination query params#12627
fix: merge incoming request query params with redirect destination query params#12627
Conversation
…ery params When a _redirects rule's destination contained query parameters, any additional query parameters from the incoming request were dropped. The existing logic used an either/or choice (destination.search || search) rather than merging both sets of parameters. This adds a mergeSearchParams helper that combines both sets, with destination parameters taking precedence when the same key exists in both.
🦋 Changeset detectedLatest commit: ba0c434 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Claude finished @MattieTK's task —— View job Changeset ReviewTasks
✅ All changesets look good The changeset
The changeset follows the format guidelines with a clear imperative title and a well-written body that explains both the problem and the solution. |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
The PR template on this repo has:
|
| /** | ||
| * Merge query parameters from the incoming request with those from the | ||
| * redirect destination. Destination params take precedence – if the same | ||
| * key appears in both, the destination value wins. | ||
| * | ||
| * Returns the merged query string (including the leading `?`), or an | ||
| * empty string if there are no params. | ||
| */ | ||
| const mergeSearchParams = ( | ||
| incomingSearch: string, | ||
| destinationSearch: string |
There was a problem hiding this comment.
We should make this more generic by removing the concept of incoming and destination. I don't think they are menaingfull here. The only thing we need to know if is A or B has higher priority.
Please add JSDonc for both the parameters @param and @return value. The comment is missing the parameter format
| if (!incomingSearch && !destinationSearch) { | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
Covered by the 2 following ifs
| if (!incomingSearch && !destinationSearch) { | |
| return ""; | |
| } |
|
|
||
| // Destination params overwrite incoming params | ||
| for (const [key, value] of destination) { | ||
| merged.set(key, value); |
There was a problem hiding this comment.
I don't think this handles multiple values (i.e. .getAll()) correctly.
What this code should do:
- document what we want to do with multiples values (i.e. merge or replace)
- add tests
- fix the implementation
|
Is this actually the desired behaviour? If I understand Netlify's docs, it doesn't merge parameters between the two; it looks like it only copies over params if none are on the source URL of the redirect. Otherwise, if there is any param in the source, it will not match if there are other params. and the user has to construct the destination URL from the captured params. |
|
Sorry folks, I'll set this back to draft and review this. There is something here I'm pretty sure, I've just probably missed the mark. |
4 participants
Summary
mergeSearchParamshelper that combines both sets of params, with destination params taking precedenceBehaviour change
Given a
_redirectsrule:A request to
/products/42/widget?tracking=abccurrently produces:The
tracking=abcparameter from the incoming request is not carried through. The existing logic athandler.ts:1035usesdestination.search || search, which selects one or the other rather than merging.After this change
Incoming params are merged in, with destination params taking precedence for duplicate keys.
Prior art
Netlify's
_redirectsformat documents this merging behaviour. From their redirect options docs:Our docs for Pages redirects and Workers static assets redirects include the
/products/:code/:name /products?code=:code&name=:nameexample, where this behaviour is relevant. A follow-up docs PR could document the expected query string handling.Changes
packages/workers-shared/asset-worker/src/handler.ts– AddedmergeSearchParams()helper, replaced the||/ternary logic with a call to itpackages/workers-shared/asset-worker/tests/handler.test.ts– Added 5 tests covering query string preservation on static redirects, dynamic redirects, param merging with precedence, and cross-origin redirects