bugfix: ensure prefetch aliasing doesn't apply URL redirect (#69153)

Another follow up to #68340: When a prefetch is aliased (meaning we
returned a prefetch entry that corresponded with the same pathname, but
different / missing search params), we signal to the router that it
should only copy the loading state from this prefetch entry, and null
everything else out so it can be fetched from the server. However it's
possible that we return an aliased entry that corresponds with a
redirect (such as in middleware) meaning the server will supply a
`canonicalUrlOverride` value that will correspond with the aliased
entry, rather than the requested entry.

This updates the handling to only apply the canonical URL override when
we're not using an aliased entry. This will defer the redirect handling
to when the actual dynamic request (in the server patch reducer) so that
the aliased entry will only handle the loading copying and not do any
redirects.

This copies the existing tests that covered this behavior but adds a
middleware redirect to verify the behavior is the same.

packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts

@@ -148,9 +148,15 @@ export function navigateReducer(
         return handleExternalUrl(state, mutable, href, pendingPush)
       }
 
-      const updatedCanonicalUrl = canonicalUrlOverride
-        ? createHrefFromUrl(canonicalUrlOverride)
-        : href
+      // When the server indicates an override for the canonical URL (such as a redirect in middleware)
+      // we only want to use that if we're not using an aliased entry as the redirect will correspond with
+      // the aliased prefetch which might have different search params. Since we're only using the aliased entry
+      // for the loading state, the proper override will happen in the server patch action when the dynamic
+      // data is loaded.
+      const updatedCanonicalUrl =
+        canonicalUrlOverride && !prefetchValues.aliased
+          ? createHrefFromUrl(canonicalUrlOverride)
+          : href
 
       // Track if the navigation was only an update to the hash fragment
       mutable.onlyHashChange =

test/e2e/app-dir/searchparams-reuse-loading/app/with-middleware/page.tsx

@@ -0,0 +1,28 @@
+import Link from 'next/link'
+
+export default function Page() {
+  return (
+    <ul>
+      <li>
+        <Link href="/with-middleware/search-params?id=1">
+          /search-params?id=1 (prefetch: true)
+        </Link>
+      </li>
+      <li>
+        <Link href="/with-middleware/search-params?id=2">
+          /search-params?id=2
+        </Link>
+      </li>
+      <li>
+        <Link href="/with-middleware/search-params?id=3" prefetch>
+          /search-params?id=3 (prefetch: true)
+        </Link>
+      </li>
+      <li>
+        <Link href="/with-middleware/search-params" prefetch>
+          /search-params (prefetch: true)
+        </Link>
+      </li>
+    </ul>
+  )
+}

test/e2e/app-dir/searchparams-reuse-loading/app/with-middleware/search-params/loading.tsx

@@ -0,0 +1,3 @@
+export default function Loading() {
+  return <h1 id="loading">Loading...</h1>
+}

test/e2e/app-dir/searchparams-reuse-loading/app/with-middleware/search-params/someValue/page.tsx

@@ -0,0 +1,16 @@
+import Link from 'next/link'
+
+export default async function Page({
+  searchParams,
+}: {
+  searchParams: Record<string, string>
+}) {
+  // sleep for 500ms
+  await new Promise((resolve) => setTimeout(resolve, 500))
+  return (
+    <>
+      <h1 id="params">{JSON.stringify(searchParams)}</h1>
+      <Link href="/with-middleware">Back</Link>
+    </>
+  )
+}

test/e2e/app-dir/searchparams-reuse-loading/middleware.ts

@@ -0,0 +1,26 @@
+import { NextResponse } from 'next/server'
+import type { NextRequest } from 'next/server'
+
+export function middleware(request: NextRequest) {
+  const { pathname, search } = request.nextUrl
+
+  if (
+    pathname.startsWith('/with-middleware/search-params') &&
+    !pathname.includes('someValue')
+  ) {
+    const newPathname = pathname.replace(
+      '/with-middleware/search-params',
+      '/with-middleware/search-params/someValue'
+    )
+    console.log('performing redirect')
+    return NextResponse.redirect(
+      new URL(`${newPathname}${search}`, request.url)
+    )
+  }
+
+  return NextResponse.next()
+}
+
+export const config = {
+  matcher: '/with-middleware/search-params',
+}

test/e2e/app-dir/searchparams-reuse-loading/searchparams-reuse-loading.test.ts

@@ -34,99 +34,116 @@ describe('searchparams-reuse-loading', () => {
   // Dev doesn't perform prefetching, so this test is skipped, as it relies on intercepting
   // prefetch network requests.
   if (!isNextDev) {
-    it('should correctly return different RSC data for full prefetches with different searchParam values', async () => {
-      const rscRequestPromise = new Map<
-        string,
-        { resolve: () => Promise<void> }
-      >()
-
-      let interceptRequests = false
-      const browser = await next.browser('/', {
-        beforePageLoad(page: Page) {
-          page.route('**/search-params*', async (route: Route) => {
-            if (!interceptRequests) {
-              return route.continue()
-            }
-
-            const request = route.request()
-            const headers = await request.allHeaders()
-            const url = new URL(request.url())
-            const promiseKey =
-              url.pathname + '?id=' + url.searchParams.get('id')
-
-            if (headers['rsc'] === '1' && !headers['next-router-prefetch']) {
-              // Create a promise that will be resolved by the later test code
-              let resolvePromise: () => void
-              const promise = new Promise<void>((res) => {
-                resolvePromise = res
-              })
-
-              if (rscRequestPromise.has(promiseKey)) {
-                throw new Error('Duplicate request')
+    describe.each([
+      { path: '/', label: 'Without Middleware' },
+      { path: '/with-middleware', label: 'With Middleware' },
+    ])('$label', ({ path }) => {
+      it('should correctly return different RSC data for full prefetches with different searchParam values', async () => {
+        const rscRequestPromise = new Map<
+          string,
+          { resolve: () => Promise<void> }
+        >()
+
+        let interceptRequests = false
+        const browser = await next.browser(path, {
+          beforePageLoad(page: Page) {
+            page.route('**/search-params*', async (route: Route) => {
+              if (!interceptRequests) {
+                return route.continue()
               }
 
-              rscRequestPromise.set(promiseKey, {
-                resolve: async () => {
-                  await route.continue()
-                  // wait a moment to ensure the response is received
-                  await new Promise((res) => setTimeout(res, 500))
-                  resolvePromise()
-                },
-              })
-
-              // Await the promise to effectively stall the request
-              await promise
-            } else {
-              await route.continue()
-            }
-          })
-        },
+              const request = route.request()
+              const headers = await request.allHeaders()
+              const url = new URL(request.url())
+              const promiseKey =
+                url.pathname + '?id=' + url.searchParams.get('id')
+
+              if (headers['rsc'] === '1' && !headers['next-router-prefetch']) {
+                // Create a promise that will be resolved by the later test code
+                let resolvePromise: () => void
+                const promise = new Promise<void>((res) => {
+                  resolvePromise = res
+                })
+
+                if (rscRequestPromise.has(promiseKey)) {
+                  throw new Error('Duplicate request')
+                }
+
+                rscRequestPromise.set(promiseKey, {
+                  resolve: async () => {
+                    await route.continue()
+                    // wait a moment to ensure the response is received
+                    await new Promise((res) => setTimeout(res, 500))
+                    resolvePromise()
+                  },
+                })
+
+                // Await the promise to effectively stall the request
+                await promise
+              } else {
+                await route.continue()
+              }
+            })
+          },
+        })
+
+        const basePath = path === '/' ? '' : path
+
+        await browser.waitForIdleNetwork()
+        interceptRequests = true
+        // The first link we click is "auto" prefetched.
+        await browser
+          .elementByCss(`[href="${basePath}/search-params?id=1"]`)
+          .click()
+
+        // We expect to click it and immediately see a loading state
+        expect(await browser.elementById('loading').text()).toBe('Loading...')
+        // We only resolve the dynamic request after we've confirmed loading exists,
+        // to avoid a race where the dynamic request handles the loading state instead.
+        let dynamicRequest = rscRequestPromise.get(
+          `${basePath}/search-params?id=1`
+        )
+        expect(dynamicRequest).toBeDefined()
+
+        // resolve the promise
+        await dynamicRequest.resolve()
+        dynamicRequest = undefined
+
+        // Confirm the params are correct
+        const params = await browser.waitForElementByCss('#params').text()
+        expect(params).toBe('{"id":"1"}')
+
+        await browser.elementByCss(`[href='${path}']`).click()
+
+        // Do the exact same thing again, for another prefetch auto link, to ensure
+        // loading works as expected and we get different search params
+        await browser
+          .elementByCss(`[href="${basePath}/search-params?id=2"]`)
+          .click()
+        expect(await browser.elementById('loading').text()).toBe('Loading...')
+        dynamicRequest = rscRequestPromise.get(`${basePath}/search-params?id=2`)
+        expect(dynamicRequest).toBeDefined()
+
+        // resolve the promise
+        await dynamicRequest.resolve()
+        dynamicRequest = undefined
+
+        const params2 = await browser.waitForElementByCss('#params').text()
+        expect(params2).toBe('{"id":"2"}')
+
+        // Dev mode doesn't perform full prefetches, so this test is conditional
+        await browser.elementByCss(`[href='${path}']`).click()
+
+        await browser
+          .elementByCss(`[href="${basePath}/search-params?id=3"]`)
+          .click()
+        expect(rscRequestPromise.has(`${basePath}/search-params?id=3`)).toBe(
+          false
+        )
+        // no need to resolve any dynamic requests, as this is a full prefetch
+        const params3 = await browser.waitForElementByCss('#params').text()
+        expect(params3).toBe('{"id":"3"}')
       })
-
-      await browser.waitForIdleNetwork()
-      interceptRequests = true
-      // The first link we click is "auto" prefetched.
-      await browser.elementByCss('[href="/search-params?id=1"]').click()
-
-      // We expect to click it and immediately see a loading state
-      expect(await browser.elementById('loading').text()).toBe('Loading...')
-      // We only resolve the dynamic request after we've confirmed loading exists,
-      // to avoid a race where the dynamic request handles the loading state instead.
-      let dynamicRequest = rscRequestPromise.get('/search-params?id=1')
-      expect(dynamicRequest).toBeDefined()
-
-      // resolve the promise
-      await dynamicRequest.resolve()
-      dynamicRequest = undefined
-
-      // Confirm the params are correct
-      const params = await browser.waitForElementByCss('#params').text()
-      expect(params).toBe('{"id":"1"}')
-
-      await browser.elementByCss("[href='/']").click()
-
-      // Do the exact same thing again, for another prefetch auto link, to ensure
-      // loading works as expected and we get different search params
-      await browser.elementByCss('[href="/search-params?id=2"]').click()
-      expect(await browser.elementById('loading').text()).toBe('Loading...')
-      dynamicRequest = rscRequestPromise.get('/search-params?id=2')
-      expect(dynamicRequest).toBeDefined()
-
-      // resolve the promise
-      await dynamicRequest.resolve()
-      dynamicRequest = undefined
-
-      const params2 = await browser.waitForElementByCss('#params').text()
-      expect(params2).toBe('{"id":"2"}')
-
-      // Dev mode doesn't perform full prefetches, so this test is conditional
-      await browser.elementByCss("[href='/']").click()
-
-      await browser.elementByCss('[href="/search-params?id=3"]').click()
-      expect(rscRequestPromise.has('/search-params?id=3')).toBe(false)
-      // no need to resolve any dynamic requests, as this is a full prefetch
-      const params3 = await browser.waitForElementByCss('#params').text()
-      expect(params3).toBe('{"id":"3"}')
     })
 
     // /search-params (full) to /search-params?id=1 (missing)