fix resolve routes behavior when matching a dynamic segment (#54539)

When `fallback: false` is set and you visit a dynamic segment (e.g. `/[slug]`), the router server was getting stuck in a `x-no-fallback` loop and eventually would fail because it was matching the output at `check_fs` before attempting to resolve dynamic routes in the `check: true` block. Closes NEXT-1557
vercel · Aug 25, 2023 · dbac755 · dbac755
1 parent 36b45c6
commit dbac755
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 36 deletions.
diff --git a/packages/next/src/server/lib/route-resolver.ts b/packages/next/src/server/lib/route-resolver.ts
@@ -230,12 +230,12 @@ export async function makeResolver(
     req: IncomingMessage,
     res: ServerResponse
   ): Promise<RouteResult | void> {
-    const routeResult = await resolveRoutes(
+    const routeResult = await resolveRoutes({
       req,
-      new Set(),
-      false,
-      signalFromNodeResponse(res)
-    )
+      isUpgradeReq: false,
+      signal: signalFromNodeResponse(res),
+    })
+
     const {
       matchedOutput,
       bodyStream,

diff --git a/packages/next/src/server/lib/router-server.ts b/packages/next/src/server/lib/router-server.ts
@@ -321,7 +321,7 @@ export async function initialize(opts: {
       // TODO: log socket errors?
     })
 
-    const matchedDynamicRoutes = new Set<string>()
+    const invokedOutputs = new Set<string>()
 
     async function invokeRender(
       parsedUrl: NextUrlWithParsedQuery,
@@ -476,12 +476,12 @@ export async function initialize(opts: {
         resHeaders,
         bodyStream,
         matchedOutput,
-      } = await resolveRoutes(
+      } = await resolveRoutes({
         req,
-        matchedDynamicRoutes,
-        false,
-        signalFromNodeResponse(res)
-      )
+        isUpgradeReq: false,
+        signal: signalFromNodeResponse(res),
+        invokedOutputs,
+      })
 
       if (devInstance && matchedOutput?.type === 'devVirtualFsItem') {
         const origUrl = req.url || '/'
@@ -658,6 +658,8 @@ export async function initialize(opts: {
       }
 
       if (matchedOutput) {
+        invokedOutputs.add(matchedOutput.itemPath)
+
         return await invokeRender(
           parsedUrl,
           matchedOutput.type === 'appFile' ? 'app' : 'pages',
@@ -753,12 +755,11 @@ export async function initialize(opts: {
         }
       }
 
-      const { matchedOutput, parsedUrl } = await resolveRoutes(
+      const { matchedOutput, parsedUrl } = await resolveRoutes({
         req,
-        new Set(),
-        true,
-        signalFromNodeResponse(socket)
-      )
+        isUpgradeReq: true,
+        signal: signalFromNodeResponse(socket),
+      })
 
       // TODO: allow upgrade requests to pages/app paths?
       // this was not previously supported

diff --git a/packages/next/src/server/lib/router-utils/resolve-routes.ts b/packages/next/src/server/lib/router-utils/resolve-routes.ts
@@ -92,12 +92,17 @@ export function getResolveRoutes(
     ...(opts.minimalMode ? [] : fsChecker.rewrites.fallback),
   ]
 
-  async function resolveRoutes(
-    req: IncomingMessage,
-    matchedDynamicRoutes: Set<string>,
-    isUpgradeReq: boolean,
+  async function resolveRoutes({
+    req,
+    isUpgradeReq,
+    signal,
+    invokedOutputs,
+  }: {
+    req: IncomingMessage
+    isUpgradeReq: boolean
     signal: AbortSignal
-  ): Promise<{
+    invokedOutputs?: Set<string>
+  }): Promise<{
     finished: boolean
     statusCode?: number
     bodyStream?: ReadableStream | null
@@ -223,18 +228,22 @@ export function getResolveRoutes(
     }
 
     async function checkTrue() {
-      if (checkLocaleApi(parsedUrl.pathname || '')) {
+      const pathname = parsedUrl.pathname || ''
+
+      if (checkLocaleApi(pathname)) {
         return
       }
-      const output = await fsChecker.getItem(parsedUrl.pathname || '')
+      if (!invokedOutputs?.has(pathname)) {
+        const output = await fsChecker.getItem(pathname)
 
-      if (output) {
-        if (
-          config.useFileSystemPublicRoutes ||
-          didRewrite ||
-          (output.type !== 'appFile' && output.type !== 'pageFile')
-        ) {
-          return output
+        if (output) {
+          if (
+            config.useFileSystemPublicRoutes ||
+            didRewrite ||
+            (output.type !== 'appFile' && output.type !== 'pageFile')
+          ) {
+            return output
+          }
         }
       }
       const dynamicRoutes = fsChecker.getDynamicRoutes()
@@ -249,12 +258,12 @@ export function getResolveRoutes(
       const localeResult = fsChecker.handleLocale(curPathname || '')
 
       for (const route of dynamicRoutes) {
-        // when resolving fallback: false we attempt to
+        // when resolving fallback: false the
         // render worker may return a no-fallback response
         // which signals we need to continue resolving.
         // TODO: optimize this to collect static paths
         // to use at the routing layer
-        if (matchedDynamicRoutes.has(route.page)) {
+        if (invokedOutputs?.has(route.page)) {
           continue
         }
         const params = route.match(localeResult.pathname)
@@ -275,7 +284,6 @@ export function getResolveRoutes(
           if (pageOutput && curPathname?.startsWith('/_next/data')) {
             parsedUrl.query.__nextDataReq = '1'
           }
-          matchedDynamicRoutes.add(route.page)
 
           if (config.useFileSystemPublicRoutes || didRewrite) {
             return pageOutput
@@ -381,17 +389,19 @@ export function getResolveRoutes(
         }
 
         if (route.name === 'check_fs') {
-          if (checkLocaleApi(parsedUrl.pathname || '')) {
+          const pathname = parsedUrl.pathname || ''
+
+          if (invokedOutputs?.has(pathname) || checkLocaleApi(pathname)) {
             return
           }
-          const output = await fsChecker.getItem(parsedUrl.pathname || '')
+          const output = await fsChecker.getItem(pathname)
 
           if (
             output &&
             !(
               config.i18n &&
               initialLocaleResult?.detectedLocale &&
-              pathHasPrefix(parsedUrl.pathname || '', '/api')
+              pathHasPrefix(pathname, '/api')
             )
           ) {
             if (

diff --git a/test/integration/fallback-false-rewrite/test/index.test.js b/test/integration/fallback-false-rewrite/test/index.test.js
@@ -89,6 +89,21 @@ const runTests = () => {
       slug: 'second',
     })
   })
+
+  it('should behave properly when accessing the dynamic param directly', async () => {
+    const res = await fetchViaHTTP(appPort, '/[slug]', undefined, {
+      redirect: 'manual',
+    })
+    expect(res.status).toBe(200)
+
+    const html = await res.text()
+    const $ = cheerio.load(html)
+
+    expect($('#another').text()).toBe('another')
+    expect(JSON.parse($('#query').text())).toEqual({
+      path: ['[slug]'],
+    })
+  })
 }
 
 describe('fallback: false rewrite', () => {