fix: include query string in route handler ISR cache key#603
fix: include query string in route handler ISR cache key#603southpolesteve wants to merge 2 commits intomainfrom
Conversation
Route handler ISR cache keys were derived from pathname only, so requests with different query parameters (e.g. /api/data?user=alice vs /api/data?user=bob) would share the same cache entry. This caused one user's response to be served to another user with different query parameters. Include the full query string in the cache key for route handler ISR entries, in both the cache read (HIT) and cache write (MISS) paths.
|
/bigbonk review this PR |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
The core logic change is correct — route handlers that vary responses by query parameters need query-string-aware cache keys. The fix is scoped to the right place (only APP_ROUTE, only GET/HEAD), and all three cache operation sites (read, write, stale regen) use the same __routeKey that now includes __routeQs. No other server entry points need fixing since they all delegate to this generated RSC entry.
Two issues need to be addressed before merging:
1. Broken tests (must fix)
The PR breaks 7 tests:
tests/app-router.test.ts:3819— asserts__isrRouteKey(cleanPathname)but the generated code now produces__isrRouteKey(cleanPathname + __routeQs). Update the assertion.tests/entry-templates.test.ts— 6 snapshot tests fail because they still contain the old__isrRouteKey(cleanPathname)form. Runvp test tests/entry-templates.test.ts --updateto regenerate snapshots.
2. Cache cardinality concern (design discussion)
Including the raw query string in the cache key means ?a=1&b=2 and ?b=2&a=1 produce different cache entries for the same logical request. It also means an attacker could bust the cache by varying parameter order or adding junk parameters like ?_=random. Consider whether query parameters should be sorted/normalized before inclusion in the key.
This is worth a conscious decision — if the current behavior is intentional (matching exactly how the handler receives the raw request), that's defensible, but document the tradeoff. If normalization is desired, sorting the params is straightforward:
const __qs = new URLSearchParams(url.search);
__qs.sort();
const __routeQs = (method === "GET" || isAutoHead) ? (__qs.size ? "?" + __qs.toString() : "") : "";|
|
||
| // Include query string in route handler ISR cache key so different | ||
| // query parameters produce distinct cache entries. | ||
| const __routeQs = (method === "GET" || isAutoHead) ? new URL(request.url).search : ""; |
There was a problem hiding this comment.
Two things to consider here:
-
Redundant URL parse:
request.urlhas already been parsed into aURLobject (theurlvariable) earlier in this function. You can useurl.searchinstead ofnew URL(request.url).searchto avoid constructing a secondURLinstance on every request. -
Parameter order sensitivity:
new URL(request.url).searchpreserves the raw query string, so?a=1&b=2and?b=2&a=1produce different cache keys for what may be the same logical request. If cache key normalization matters here, consider sorting the search params:const __qs = new URLSearchParams(url.search); __qs.sort(); const __routeQs = (method === "GET" || isAutoHead) ? (__qs.size ? "?" + __qs.toString() : "") : "";
If the raw order is intentional (matching exactly what the handler receives), that's fine — but worth a comment explaining the choice.
| typeof handlerFn === "function" | ||
| ) { | ||
| const __routeKey = __isrRouteKey(cleanPathname); | ||
| const __routeKey = __isrRouteKey(cleanPathname + __routeQs); |
There was a problem hiding this comment.
nit: The __isrCacheKey function (line 478) normalizes trailing slashes via pathname.replace(/\/$/, ""). With the query string appended, the input is now e.g. /api/data?user=alice — the trailing-slash regex won't match when a query string is present, which is fine since cleanPathname already has its trailing slash stripped before the query string is appended. Just noting this works correctly as-is.
|
Review posted successfully. Here's a summary of what I found: PR #603 Review SummaryThe fix is correct in principle — route handler ISR cache keys using only the pathname is a real bug that causes one user's cached response to be served to another when query parameters differ. Two issues flagged:
Inline comments also noted that |
|
/bigbonk Addressed both review items: (1) sorted query params to normalize key order, (2) updated assertion and regenerated 6 snapshots. All tests pass locally. Please re-review. |
There was a problem hiding this comment.
The fix is correct and the bug is real — caching route handler responses by pathname alone means /api/data?user=alice and /api/data?user=bob serve the same cached response. The scoping is right (only APP_ROUTE, only GET/HEAD), and all three ISR paths (HIT, STALE, MISS) use the same __routeKey that now includes __routeQs. CI is all green.
The previous review's concern about parameter ordering has already been addressed — the PR sorts searchParams before building the key. Good.
One remaining issue:
Redundant new URL(request.url) parse — The generated _handleRequest function already has const url = new URL(request.url) at the top of its scope (generated at line 1576 of the template). Line 2264 creates a second URL object from the same request.url on every request. This should use url.searchParams instead. It's a minor perf nit (one extra URL parse per ISR-eligible route handler request), but since url is already right there, there's no reason not to use it.
| // same key) and prevent cache-busting via parameter reordering. | ||
| let __routeQs = ""; | ||
| if (method === "GET" || isAutoHead) { | ||
| const __qs = new URL(request.url).searchParams; |
There was a problem hiding this comment.
url is already defined as new URL(request.url) at the top of the generated _handleRequest function (line 1576 in the template). This creates a second URL object on every ISR-eligible request unnecessarily. In fact, the STALE regen path on line 2303 already uses url.searchParams directly — this line should do the same for consistency and to avoid the redundant parse:
| const __qs = new URL(request.url).searchParams; | |
| const __qs = url.searchParams; |
(The url variable is a const in the same function scope, so it is always available here.)
|
Review posted successfully on PR #603. Here's a summary of what I found: The fix is correct — route handler ISR cache keys using only the pathname is a real bug, and including the query string is the right fix. The PR is well-scoped: only affects One suggestion: |
Address bigbonk review feedback: - Sort query parameters before including in cache key so ?a=1&b=2 and ?b=2&a=1 produce the same key (prevents cache-busting via parameter reordering) - Update assertion in app-router.test.ts to match new key format - Regenerate 6 entry-templates snapshots
southpolesteve
force-pushed
the
fix/include-query-in-route-handler-isr-key
branch
from
0888f5e to
3e651ea
Compare
|
/bigbonk Both items addressed: (1) uses |
There was a problem hiding this comment.
The fix is correct and complete. All previous review feedback has been addressed:
-
Redundant URL parse — Fixed. Line 2264 now uses
url.searchParams(the existingurlvariable) instead of creating a secondnew URL(request.url). -
Query parameter normalization — Addressed.
URLSearchParams.sort()is called on a defensive copy before serializing, so?b=2&a=1and?a=1&b=2produce the same cache key. Good use of the standard Web API for this. -
Tests — The assertion at
tests/app-router.test.ts:3819correctly checks for__isrRouteKey(cleanPathname + __routeQs), and all 6 snapshots have been regenerated. -
Consistency — All three ISR paths (HIT, STALE, MISS) use the same
cleanPathname + __routeQsconstruction. The__routeQsvariable defaults to""for non-GET/HEAD methods and for requests without query params, preserving backward compatibility.
No issues remaining.
|
Approved PR #603. The fix is solid — all three ISR paths consistently include the sorted query string in the cache key, the redundant URL parse has been removed, and tests are updated. No remaining issues. |
james-elicx
left a comment
james-elicx
left a comment
There was a problem hiding this comment.
I believe this would deviate from Next.js
https://github.com/vercel/next.js/blob/9f181bd11af5f532c529a6f168fe40505f0a4d75/packages/next/src/build/templates/app-route.ts#L181-L187
…cache hits Route handlers that access request.headers or request.nextUrl.searchParams are dynamic and should not be served from the ISR cache. Previously, the cache key was pathname-only and there was no mechanism to skip the cache read for handlers that use dynamic APIs. Add three mechanisms matching Next.js behavior: 1. __proxyRouteRequest: wraps the Request passed to route handlers in a Proxy. Accessing .headers or .nextUrl.searchParams calls markDynamicUsage(), which the existing consumeDynamicUsage() check on the write path already respects. 2. __dynamicRouteHandlers: a module-level Set<string> that remembers route patterns whose handlers used dynamic APIs. On subsequent requests, the cache read path checks this set and skips the cache for known-dynamic handlers. 3. Cache read guard: the ISR cache read condition now includes !__dynamicRouteHandlers.has(handler.pattern), preventing stale cache hits for dynamic handlers. This replaces the query-string-in-cache-key approach (PR #603) with proper dynamic detection that matches how Next.js handles route handler caching: dynamic handlers are never cached, static handlers use pathname-only keys.
|
Superseded by PR #610, which implements proper dynamic detection matching Next.js behavior instead of widening the cache key. See James's review comment for the motivation. |
2 participants
Summary
/api/data?user=aliceand/api/data?user=bobproduce distinct cache entries.Details
Previously,
__isrRouteKey(cleanPathname)used only the pathname for the cache key. Route handlers that return different responses based onsearchParamswould cache the first response and serve it for all subsequent requests regardless of query parameters.The fix extracts the query string from
request.urland appends it to the pathname before computing the cache key. Only GET/HEAD requests are affected (POST/PUT/etc. are never ISR-cached).