feat(cache): add allowQuery option to filter query params in cache key#4078
Open
fmoessle wants to merge 3 commits intonitrojs:mainfrom
Open
feat(cache): add allowQuery option to filter query params in cache key#4078fmoessle wants to merge 3 commits intonitrojs:mainfrom
allowQuery option to filter query params in cache key#4078fmoessle wants to merge 3 commits intonitrojs:mainfrom
Conversation
Allow users to specify which query parameters should be included in the cache key via `allowQuery` option on `defineCachedHandler` and route rules cache config. - `undefined`: all query params included (default, no behavior change) - `[]`: all query params ignored (only pathname used) - `["q"]`: only listed params included Closes nuxt/nuxt#33728 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@fmoessle is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Route rules config passes `allowQuery` as `readonly string[]`, so the type needs to accept both mutable and readonly arrays (matching `varies`). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/types/runtime/cache.ts (1)
41-47: Accept readonly arrays forallowQuery.This option is only read, and
variesalready accepts readonly arrays. KeepingallowQueryasstring[]needlessly rejects commonas const/ shared config values in route rules anddefineCachedHandler.♻️ Proposed type tweak
- allowQuery?: string[]; + allowQuery?: string[] | readonly string[];As per coding guidelines, "Update types and JSDoc for API changes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/runtime/cache.ts` around lines 41 - 47, Change the allowQuery option type from mutable array to an immutable/readonly array type so callers can pass `as const` and shared configs; specifically update the declaration for allowQuery to use `readonly string[]` (or `ReadonlyArray<string>`) instead of `string[]`, and update the JSDoc sentence to state that readonly arrays are accepted; ensure this matches the existing `varies` usage (which already accepts readonly arrays) so downstream callers like route rules and `defineCachedHandler` accept `as const` values.test/tests.ts (1)
696-715: Add a route-rule coverage case too.This only exercises
allowQueryondefineCachedHandlervia/api/cached-allow-query. The newrouteRules.cache.allowQuerypath added intest/fixture/nitro.config.tsis still untested, so a regression in the route-rule plumbing could slip through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/tests.ts` around lines 696 - 715, Add a parallel test that exercises the route configured via routeRules.cache.allowQuery in the fixture (the route defined in test/fixture/nitro.config.ts) using the same callHandler pattern as the existing defineCachedHandler test: call the route twice with the same listed query param and differing unlisted params to assert the timestamps are identical, then call with a different listed query param to assert a different timestamp; use the same skip condition (it.skipIf(...)) and mirror the assertions used for /api/cached-allow-query so route-rule plumbing is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/internal/cache.ts`:
- Around line 205-213: The cache key builder currently collapses repeated query
values because it uses event.url.searchParams.get() and params.set(), so e.g.
?tag=a&tag=b and ?tag=a become identical; change the logic in the block that
references opts.allowQuery and event.url.searchParams to retrieve all values for
each allowed key (use getAll or iterate searchParams entries for that key) and
append each value to the local URLSearchParams (use params.append) so duplicate
values and their order are preserved when computing the search string assigned
to the search variable.
---
Nitpick comments:
In `@src/types/runtime/cache.ts`:
- Around line 41-47: Change the allowQuery option type from mutable array to an
immutable/readonly array type so callers can pass `as const` and shared configs;
specifically update the declaration for allowQuery to use `readonly string[]`
(or `ReadonlyArray<string>`) instead of `string[]`, and update the JSDoc
sentence to state that readonly arrays are accepted; ensure this matches the
existing `varies` usage (which already accepts readonly arrays) so downstream
callers like route rules and `defineCachedHandler` accept `as const` values.
In `@test/tests.ts`:
- Around line 696-715: Add a parallel test that exercises the route configured
via routeRules.cache.allowQuery in the fixture (the route defined in
test/fixture/nitro.config.ts) using the same callHandler pattern as the existing
defineCachedHandler test: call the route twice with the same listed query param
and differing unlisted params to assert the timestamps are identical, then call
with a different listed query param to assert a different timestamp; use the
same skip condition (it.skipIf(...)) and mirror the assertions used for
/api/cached-allow-query so route-rule plumbing is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83f13b78-1432-4fc3-9dd2-ad6166aa0928
📒 Files selected for processing (6)
docs/1.docs/7.cache.mdsrc/runtime/internal/cache.tssrc/types/runtime/cache.tstest/fixture/nitro.config.tstest/fixture/server/routes/api/cached-allow-query.tstest/tests.ts
Comment on lines
+205
to
+213
| if (opts.allowQuery) { | ||
| const params = new URLSearchParams(); | ||
| for (const key of opts.allowQuery) { | ||
| const value = event.url.searchParams.get(key); | ||
| if (value !== null) { | ||
| params.set(key, value); | ||
| } | ||
| } | ||
| const search = params.size > 0 ? `?${params.toString()}` : ""; |
There was a problem hiding this comment.
Preserve repeated allowed query values in the cache key.
Line 208 only reads the first value with searchParams.get(), so ?tag=a&tag=b and ?tag=a collapse to the same cache entry when tag is allowed. That can serve the wrong cached response for multi-value filters.
🐛 Proposed fix
const params = new URLSearchParams();
for (const key of opts.allowQuery) {
- const value = event.url.searchParams.get(key);
- if (value !== null) {
- params.set(key, value);
+ for (const value of event.url.searchParams.getAll(key)) {
+ params.append(key, value);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (opts.allowQuery) { | |
| const params = new URLSearchParams(); | |
| for (const key of opts.allowQuery) { | |
| const value = event.url.searchParams.get(key); | |
| if (value !== null) { | |
| params.set(key, value); | |
| } | |
| } | |
| const search = params.size > 0 ? `?${params.toString()}` : ""; | |
| if (opts.allowQuery) { | |
| const params = new URLSearchParams(); | |
| for (const key of opts.allowQuery) { | |
| for (const value of event.url.searchParams.getAll(key)) { | |
| params.append(key, value); | |
| } | |
| } | |
| const search = params.size > 0 ? `?${params.toString()}` : ""; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/internal/cache.ts` around lines 205 - 213, The cache key builder
currently collapses repeated query values because it uses
event.url.searchParams.get() and params.set(), so e.g. ?tag=a&tag=b and ?tag=a
become identical; change the logic in the block that references opts.allowQuery
and event.url.searchParams to retrieve all values for each allowed key (use
getAll or iterate searchParams entries for that key) and append each value to
the local URLSearchParams (use params.append) so duplicate values and their
order are preserved when computing the search string assigned to the search
variable.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/types/runtime/cache.ts`:
- Around line 41-47: The JSDoc for the allowQuery field is inaccurate: the cache
key currently uses event.url.searchParams.get(key) which only reads the first
value for repeated query parameters, so repeated params (e.g., q=a&q=b) will
collide with single occurrences; update the allowQuery JSDoc comment (for the
allowQuery property) to explicitly state "only the first occurrence/value of
each allowed query parameter is used when building the cache key" OR change the
implementation that builds the cache key to use all occurrences (e.g.,
searchParams.getAll or iterate entries) so every value contributes to the key —
reference allowQuery and the code path using event.url.searchParams.get(key) to
locate where to update either the docs or the key-construction logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a5f135e-4ef4-4662-a495-af4a413fc4e0
📒 Files selected for processing (1)
src/types/runtime/cache.ts
Comment on lines
+41
to
+47
| /** | ||
| * List of query string parameter names that will be considered for caching. | ||
| * - If undefined, all query parameters are included in the cache key. | ||
| * - If an empty array `[]`, all query parameters are ignored (only pathname is used for caching). | ||
| * - If a list of parameter names, only those parameters are included in the cache key. | ||
| */ | ||
| allowQuery?: string[] | readonly string[]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how `allowQuery` values are incorporated into the cache key.
# Expected result: the implementation uses `searchParams.get(key)`,
# which confirms only one value per query name is considered.
sed -n '201,216p' src/runtime/internal/cache.tsRepository: nitrojs/nitro
Length of output: 607
JSDoc must clarify that only the first value per query parameter is used in the cache key.
The implementation uses event.url.searchParams.get(key), which reads only the first value when a query parameter appears multiple times. Requests like /foo?q=a&q=b and /foo?q=a will collide when allowQuery: ["q"] because both produce the same cache key. Update the JSDoc on lines 43-44 to explicitly state that only the first occurrence of each allowed parameter contributes to the key, or modify the implementation to include all occurrences. Per coding guidelines, JSDoc must accurately reflect API behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types/runtime/cache.ts` around lines 41 - 47, The JSDoc for the
allowQuery field is inaccurate: the cache key currently uses
event.url.searchParams.get(key) which only reads the first value for repeated
query parameters, so repeated params (e.g., q=a&q=b) will collide with single
occurrences; update the allowQuery JSDoc comment (for the allowQuery property)
to explicitly state "only the first occurrence/value of each allowed query
parameter is used when building the cache key" OR change the implementation that
builds the cache key to use all occurrences (e.g., searchParams.getAll or
iterate entries) so every value contributes to the key — reference allowQuery
and the code path using event.url.searchParams.get(key) to locate where to
update either the docs or the key-construction logic.
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/presets/vercel.test.ts`:
- Around line 472-473: Update the test in test/presets/vercel.test.ts so it
asserts the actual JSON contents for the new allow-query prerender config
instead of (or in addition to) the existing isr path: locate the assertion that
reads the file 'functions/rules/isr/[...]-isr.prerender-config.json' and change
it to read and assert the contents of
'functions/rules/allow-query/[...]-isr.prerender-config.json' (or add a second
assertion that reads that allow-query path and matches its expected
snapshot/JSON), ensuring the test verifies the serialized allowQuery field is
present; reference the file-reading/assertion block in vercel.test.ts when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dbf2e45b-bebd-4353-a12d-bf5e9ae4686d
📒 Files selected for processing (1)
test/presets/vercel.test.ts
Comment on lines
+472
to
+473
| "functions/rules/allow-query/[...]-isr.func (symlink)", | ||
| "functions/rules/allow-query/[...]-isr.prerender-config.json", |
There was a problem hiding this comment.
Assert the new allow-query prerender config contents.
This snapshot only proves the file exists. The actual content check still reads functions/rules/isr/[...]-isr.prerender-config.json, so a regression where /rules/allow-query/** stops serializing allowQuery would slip through. Please point that assertion at functions/rules/allow-query/[...]-isr.prerender-config.json or add a second assertion for it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/presets/vercel.test.ts` around lines 472 - 473, Update the test in
test/presets/vercel.test.ts so it asserts the actual JSON contents for the new
allow-query prerender config instead of (or in addition to) the existing isr
path: locate the assertion that reads the file
'functions/rules/isr/[...]-isr.prerender-config.json' and change it to read and
assert the contents of
'functions/rules/allow-query/[...]-isr.prerender-config.json' (or add a second
assertion that reads that allow-query path and matches its expected
snapshot/JSON), ensuring the test verifies the serialized allowQuery field is
present; reference the file-reading/assertion block in vercel.test.ts when
making the change.
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
1 participant
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
allowQueryoption todefineCachedHandlerand route rulescacheconfig[], all query parameters are ignored (only pathname is used)undefined(default), all query parameters are included — no behavior changeutm_source,fbclid, etc.Closes nuxt/nuxt#33728
Test plan
api/cached-allow-query.tswithallowQuery: ["q"]/rules/allow-query/**withallowQuery: ["q"]qparam with different unlisted params hits cacheqparam gets different cache entryallowQueryfield🤖 Generated with Claude Code