fix(@angular/ssr): handle nested redirects not explicitly defined in router config

alan-agius4 · alan-agius4 · commit 7cec59de2629 · 2024-11-21T13:17:49.000Z
This commit ensures proper handling of nested redirects that are implicitly structured but not explicitly defined in the router configuration. Closes #28903
diff --git a/packages/angular/ssr/src/app.ts b/packages/angular/ssr/src/app.ts
@@ -24,7 +24,12 @@ import { sha256 } from './utils/crypto';
 import { InlineCriticalCssProcessor } from './utils/inline-critical-css';
 import { LRUCache } from './utils/lru-cache';
 import { AngularBootstrap, renderAngular } from './utils/ng';
-import { joinUrlParts, stripIndexHtmlFromURL, stripLeadingSlash } from './utils/url';
+import {
+  constructUrlWithPlaceholders,
+  joinUrlParts,
+  stripIndexHtmlFromURL,
+  stripLeadingSlash,
+} from './utils/url';
 
 /**
  * Maximum number of critical CSS entries the cache can store.
@@ -160,11 +165,14 @@ export class AngularServerApp {
 
     const { redirectTo, status, renderMode } = matchedRoute;
     if (redirectTo !== undefined) {
-      // Note: The status code is validated during route extraction.
-      // 302 Found is used by default for redirections
-      // See: https://developer.mozilla.org/en-US/docs/Web/API/Response/redirect_static#status
-      // eslint-disable-next-line @typescript-eslint/no-explicit-any
-      return Response.redirect(new URL(redirectTo, url), (status as any) ?? 302);
+      return Response.redirect(
+        constructUrlWithPlaceholders(redirectTo, url),
+        // Note: The status code is validated during route extraction.
+        // 302 Found is used by default for redirections
+        // See: https://developer.mozilla.org/en-US/docs/Web/API/Response/redirect_static#status
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        (status as any) ?? 302,
+      );
     }
 
     if (renderMode === RenderMode.Prerender) {
@@ -241,17 +249,8 @@ export class AngularServerApp {
     matchedRoute: RouteTreeNodeMetadata,
     requestContext?: unknown,
   ): Promise<Response | null> {
-    const { redirectTo, status } = matchedRoute;
-
-    if (redirectTo !== undefined) {
-      // Note: The status code is validated during route extraction.
-      // 302 Found is used by default for redirections
-      // See: https://developer.mozilla.org/en-US/docs/Web/API/Response/redirect_static#status
-      // eslint-disable-next-line @typescript-eslint/no-explicit-any
-      return Response.redirect(new URL(redirectTo, new URL(request.url)), (status as any) ?? 302);
-    }
+    const { renderMode, headers, status } = matchedRoute;
 
-    const { renderMode, headers } = matchedRoute;
     if (!this.allowStaticRouteRender && renderMode === RenderMode.Prerender) {
       return null;
     }
diff --git a/packages/angular/ssr/src/routes/ng-routes.ts b/packages/angular/ssr/src/routes/ng-routes.ts
@@ -21,7 +21,12 @@ import { ServerAssets } from '../assets';
 import { Console } from '../console';
 import { AngularAppManifest, getAngularAppManifest } from '../manifest';
 import { AngularBootstrap, isNgModule } from '../utils/ng';
-import { joinUrlParts, stripLeadingSlash } from '../utils/url';
+import {
+  URL_PARAMETER_REGEXP,
+  addTrailingSlash,
+  joinUrlParts,
+  stripLeadingSlash,
+} from '../utils/url';
 import {
   PrerenderFallback,
   RenderMode,
@@ -31,11 +36,6 @@ import {
 } from './route-config';
 import { RouteTree, RouteTreeNodeMetadata } from './route-tree';
 
-/**
- * Regular expression to match segments preceded by a colon in a string.
- */
-const URL_PARAMETER_REGEXP = /(?<!\\):([^/]+)/g;
-
 /**
  * An set of HTTP status codes that are considered valid for redirect responses.
  */
@@ -146,31 +146,36 @@ async function* traverseRoutesConfig(options: {
       const metadata: ServerConfigRouteTreeNodeMetadata = {
         renderMode: RenderMode.Prerender,
         ...matchedMetaData,
-        route: currentRoutePath,
+        // Match Angular router behavior
+        // ['one', 'two', ''] -> 'one/two/'
+        // ['one', 'two', 'three'] -> 'one/two/three'
+        route: path === '' ? addTrailingSlash(currentRoutePath) : currentRoutePath,
       };
 
       delete metadata.presentInClientRouter;
 
-      // Handle redirects
-      if (typeof redirectTo === 'string') {
-        const redirectToResolved = resolveRedirectTo(currentRoutePath, redirectTo);
+      if (metadata.renderMode === RenderMode.Prerender) {
+        // Handle SSG routes
+        yield* handleSSGRoute(
+          typeof redirectTo === 'string' ? redirectTo : undefined,
+          metadata,
+          parentInjector,
+          invokeGetPrerenderParams,
+          includePrerenderFallbackRoutes,
+        );
+      } else if (typeof redirectTo === 'string') {
+        // Handle redirects
         if (metadata.status && !VALID_REDIRECT_RESPONSE_CODES.has(metadata.status)) {
           yield {
             error:
               `The '${metadata.status}' status code is not a valid redirect response code. ` +
               `Please use one of the following redirect response codes: ${[...VALID_REDIRECT_RESPONSE_CODES.values()].join(', ')}.`,
           };
+
           continue;
         }
-        yield { ...metadata, redirectTo: redirectToResolved };
-      } else if (metadata.renderMode === RenderMode.Prerender) {
-        // Handle SSG routes
-        yield* handleSSGRoute(
-          metadata,
-          parentInjector,
-          invokeGetPrerenderParams,
-          includePrerenderFallbackRoutes,
-        );
+
+        yield { ...metadata, redirectTo: resolveRedirectTo(metadata.route, redirectTo) };
       } else {
         yield metadata;
       }
@@ -214,13 +219,15 @@ async function* traverseRoutesConfig(options: {
  * Handles SSG (Static Site Generation) routes by invoking `getPrerenderParams` and yielding
  * all parameterized paths, returning any errors encountered.
  *
+ * @param redirectTo - Optional path to redirect to, if specified.
  * @param metadata - The metadata associated with the route tree node.
  * @param parentInjector - The dependency injection container for the parent route.
  * @param invokeGetPrerenderParams - A flag indicating whether to invoke the `getPrerenderParams` function.
  * @param includePrerenderFallbackRoutes - A flag indicating whether to include fallback routes in the result.
  * @returns An async iterable iterator that yields route tree node metadata for each SSG path or errors.
  */
 async function* handleSSGRoute(
+  redirectTo: string | undefined,
   metadata: ServerConfigRouteTreeNodeMetadata,
   parentInjector: Injector,
   invokeGetPrerenderParams: boolean,
@@ -239,6 +246,10 @@ async function* handleSSGRoute(
     delete meta['getPrerenderParams'];
   }
 
+  if (redirectTo !== undefined) {
+    meta.redirectTo = resolveRedirectTo(currentRoutePath, redirectTo);
+  }
+
   if (!URL_PARAMETER_REGEXP.test(currentRoutePath)) {
     // Route has no parameters
     yield {
@@ -279,7 +290,12 @@ async function* handleSSGRoute(
           return value;
         });
 
-        yield { ...meta, route: routeWithResolvedParams };
+        const result = { ...meta, route: routeWithResolvedParams };
+        if (redirectTo !== undefined) {
+          result.redirectTo = resolveRedirectTo(routeWithResolvedParams, redirectTo);
+        }
+
+        yield result;
       }
     } catch (error) {
       yield { error: `${(error as Error).message}` };
@@ -456,7 +472,6 @@ export async function getRoutesFromAngularRouterConfig(
         includePrerenderFallbackRoutes,
       });
 
-      let seenAppShellRoute: string | undefined;
       for await (const result of traverseRoutes) {
         if ('error' in result) {
           errors.push(result.error);
diff --git a/packages/angular/ssr/src/routes/router.ts b/packages/angular/ssr/src/routes/router.ts
@@ -7,7 +7,7 @@
  */
 
 import { AngularAppManifest } from '../manifest';
-import { stripIndexHtmlFromURL } from '../utils/url';
+import { joinUrlParts, stripIndexHtmlFromURL } from '../utils/url';
 import { extractRoutesAndCreateRouteTree } from './ng-routes';
 import { RouteTree, RouteTreeNodeMetadata } from './route-tree';
 
diff --git a/packages/angular/ssr/src/utils/url.ts b/packages/angular/ssr/src/utils/url.ts
@@ -6,6 +6,11 @@
  * found in the LICENSE file at https://angular.dev/license
  */
 
+/**
+ * Regular expression to match segments preceded by a colon in a string.
+ */
+export const URL_PARAMETER_REGEXP = /(?<!\\):([^/]+)/g;
+
 /**
  * Removes the trailing slash from a URL if it exists.
  *
@@ -61,6 +66,23 @@ export function addLeadingSlash(url: string): string {
   return url[0] === '/' ? url : `/${url}`;
 }
 
+/**
+ * Adds a trailing slash to a URL if it does not already have one.
+ *
+ * @param url - The URL string to which the trailing slash will be added.
+ * @returns The URL string with a trailing slash.
+ *
+ * @example
+ * ```js
+ * addTrailingSlash('path'); // 'path/'
+ * addTrailingSlash('path/'); // 'path/'
+ * ```
+ */
+export function addTrailingSlash(url: string): string {
+  // Check if the URL already end with a slash
+  return url[url.length - 1] === '/' ? url : `${url}/`;
+}
+
 /**
  * Joins URL parts into a single URL string.
  *
@@ -128,3 +150,51 @@ export function stripIndexHtmlFromURL(url: URL): URL {
 
   return url;
 }
+
+/**
+ * Resolves dynamic placeholders in a URL template by mapping them to corresponding segments
+ * from a base URL's path.
+ *
+ * This function processes the `urlTemplate` string, replacing any placeholders (e.g., `:param`)
+ * with segments from the `baseUrl`. If the `urlTemplate` contains no placeholders, it resolves
+ * it as a relative URL against the `baseUrl`.
+ *
+ * @param urlTemplate - A string representing the URL template, which may include dynamic placeholders
+ * (e.g., `/:param`) to be replaced with segments from the `baseUrl`'s path.
+ * @param baseUrl - The base URL used as a reference for resolving relative paths and dynamic segments
+ * in the `urlTemplate`. The base URL's pathname is split into parts to map to placeholders.
+ * @returns A fully constructed `URL` object with placeholders replaced, or the resolved relative URL
+ * if no placeholders are present.
+ *
+ * @example
+ * ```typescript
+ * const resolvedUrl = constructUrlWithPlaceholders('/:id/details', new URL('http://example.com/user/123'));
+ * console.log(resolvedUrl.toString()); // Outputs: 'http://example.com/123/details'
+
+ * const staticUrl = constructUrlWithPlaceholders('/static/path', new URL('http://example.com/base'));
+ * console.log(staticUrl.toString()); // Outputs: 'http://example.com/static/path'
+ * ```
+ */
+export function constructUrlWithPlaceholders(urlTemplate: string, baseUrl: URL): URL {
+  if (!urlTemplate.includes(':')) {
+    return new URL(urlTemplate, baseUrl);
+  }
+
+  if (urlTemplate[0] !== '/') {
+    throw new Error(
+      `Invalid urlTemplate: The template must start with a '/' to be a relative path. Received: '${urlTemplate}'`,
+    );
+  }
+
+  const templateParts = urlTemplate.split('/');
+  const basePathParts = baseUrl.pathname.split('/');
+
+  // Replace placeholders in the template with corresponding segments from the base path.
+  const resolvedParts = templateParts.map((part, index) => {
+    const val = basePathParts[index];
+
+    return val == undefined ? part : part.replace(URL_PARAMETER_REGEXP, val);
+  });
+
+  return new URL(joinUrlParts(...resolvedParts), baseUrl);
+}
diff --git a/packages/angular/ssr/test/app_spec.ts b/packages/angular/ssr/test/app_spec.ts
@@ -38,6 +38,7 @@ describe('AngularServerApp', () => {
         { path: 'page-with-status', component: HomeComponent },
         { path: 'redirect', redirectTo: 'home' },
         { path: 'redirect/relative', redirectTo: 'home' },
+        { path: 'redirect/:param/relative', redirectTo: 'home' },
         { path: 'redirect/absolute', redirectTo: '/home' },
       ],
       [
@@ -117,6 +118,12 @@ describe('AngularServerApp', () => {
         expect(response?.status).toBe(302);
       });
 
+      it('should correctly handle relative nested redirects with parameter', async () => {
+        const response = await app.handle(new Request('http://localhost/redirect/param/relative'));
+        expect(response?.headers.get('location')).toContain('http://localhost/redirect/param/home');
+        expect(response?.status).toBe(302);
+      });
+
       it('should correctly handle absolute nested redirects', async () => {
         const response = await app.handle(new Request('http://localhost/redirect/absolute'));
         expect(response?.headers.get('location')).toContain('http://localhost/home');
diff --git a/packages/angular/ssr/test/routes/ng-routes_spec.ts b/packages/angular/ssr/test/routes/ng-routes_spec.ts
@@ -165,6 +165,92 @@ describe('extractRoutesAndCreateRouteTree', () => {
         },
       ]);
     });
+
+    it('should extract nested redirects that are not explicitly defined.', async () => {
+      setAngularAppTestingManifest(
+        [
+          {
+            path: '',
+            pathMatch: 'full',
+            redirectTo: 'some',
+          },
+          {
+            path: ':param',
+            children: [
+              {
+                path: '',
+                pathMatch: 'full',
+                redirectTo: 'thing',
+              },
+              {
+                path: 'thing',
+                component: DummyComponent,
+              },
+            ],
+          },
+        ],
+        [
+          {
+            path: ':param',
+            renderMode: RenderMode.Prerender,
+            async getPrerenderParams() {
+              return [{ param: 'some' }];
+            },
+          },
+          { path: '**', renderMode: RenderMode.Prerender },
+        ],
+      );
+
+      const { routeTree, errors } = await extractRoutesAndCreateRouteTree(
+        url,
+        /** manifest */ undefined,
+        /** invokeGetPrerenderParams */ true,
+        /** includePrerenderFallbackRoutes */ true,
+      );
+      expect(errors).toHaveSize(0);
+      expect(routeTree.toObject()).toEqual([
+        { route: '/', renderMode: RenderMode.Prerender, redirectTo: '/some' },
+        { route: '/some', renderMode: RenderMode.Prerender, redirectTo: '/some/thing' },
+        { route: '/some/thing', renderMode: RenderMode.Prerender },
+        { redirectTo: '/:param/thing', route: '/:param', renderMode: RenderMode.Server },
+        { route: '/:param/thing', renderMode: RenderMode.Server },
+      ]);
+    });
+  });
+
+  it('should extract nested redirects that are not explicitly defined.', async () => {
+    setAngularAppTestingManifest(
+      [
+        {
+          path: '',
+          pathMatch: 'full',
+          redirectTo: 'some',
+        },
+        {
+          path: ':param',
+          children: [
+            {
+              path: '',
+              pathMatch: 'full',
+              redirectTo: 'thing',
+            },
+            {
+              path: 'thing',
+              component: DummyComponent,
+            },
+          ],
+        },
+      ],
+      [{ path: '**', renderMode: RenderMode.Server }],
+    );
+
+    const { routeTree, errors } = await extractRoutesAndCreateRouteTree(url);
+    expect(errors).toHaveSize(0);
+    expect(routeTree.toObject()).toEqual([
+      { route: '/', renderMode: RenderMode.Server, redirectTo: '/some' },
+      { route: '/:param', renderMode: RenderMode.Server, redirectTo: '/:param/thing' },
+      { route: '/:param/thing', renderMode: RenderMode.Server },
+    ]);
   });
 
   it('should not resolve parameterized routes for SSG when `invokeGetPrerenderParams` is false', async () => {
diff --git a/packages/angular/ssr/test/utils/url_spec.ts b/packages/angular/ssr/test/utils/url_spec.ts
