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

…router config This commit ensures proper handling of nested redirects that are implicitly structured but not explicitly defined in the router configuration. Closes #28903 (cherry picked from commit 34574b2)
angular · Nov 22, 2024 · c8cd90e · c8cd90e
1 parent 8cd6aa9
commit c8cd90e
Show file tree

Hide file tree

Showing 7 changed files with 277 additions and 42 deletions.
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 {
+  buildPathWithParams,
+  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(
+        new URL(buildPathWithParams(redirectTo, url.pathname), 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,7 @@ 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 { addTrailingSlash, joinUrlParts, stripLeadingSlash } from '../utils/url';
 import {
   PrerenderFallback,
   RenderMode,
@@ -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,14 @@ async function* handleSSGRoute(
           return value;
         });
 
-        yield { ...meta, route: routeWithResolvedParams };
+        yield {
+          ...meta,
+          route: routeWithResolvedParams,
+          redirectTo:
+            redirectTo === undefined
+              ? undefined
+              : resolveRedirectTo(routeWithResolvedParams, redirectTo),
+        };
       }
     } catch (error) {
       yield { error: `${(error as Error).message}` };
@@ -319,7 +337,7 @@ function resolveRedirectTo(routePath: string, redirectTo: string): string {
   }
 
   // Resolve relative redirectTo based on the current route path.
-  const segments = routePath.split('/');
+  const segments = routePath.replace(URL_PARAMETER_REGEXP, '*').split('/');
   segments.pop(); // Remove the last segment to make it relative.
 
   return joinUrlParts(...segments, redirectTo);
@@ -459,7 +477,6 @@ export async function getRoutesFromAngularRouterConfig(
         includePrerenderFallbackRoutes,
       });
 
-      let seenAppShellRoute: string | undefined;
       for await (const result of traverseRoutes) {
         if ('error' in result) {
           errors.push(result.error);
@@ -549,8 +566,17 @@ export async function extractRoutesAndCreateRouteTree(
       metadata.redirectTo = joinUrlParts(baseHref, metadata.redirectTo);
     }
 
+    // Remove undefined fields
+    // Helps avoid unnecessary test updates
+    for (const [key, value] of Object.entries(metadata)) {
+      if (value === undefined) {
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        delete (metadata as any)[key];
+      }
+    }
+
     const fullRoute = joinUrlParts(baseHref, route);
-    routeTree.insert(fullRoute, metadata);
+    routeTree.insert(fullRoute.replace(URL_PARAMETER_REGEXP, '*'), metadata);
   }
 
   return {

diff --git a/packages/angular/ssr/src/utils/url.ts b/packages/angular/ssr/src/utils/url.ts
@@ -61,6 +61,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 +145,54 @@ export function stripIndexHtmlFromURL(url: URL): URL {
 
   return url;
 }
+
+/**
+ * Resolves `*` placeholders in a path template by mapping them to corresponding segments
+ * from a base path. This is useful for constructing paths dynamically based on a given base path.
+ *
+ * The function processes the `toPath` string, replacing each `*` placeholder with
+ * the corresponding segment from the `fromPath`. If the `toPath` contains no placeholders,
+ * it is returned as-is. Invalid `toPath` formats (not starting with `/`) will throw an error.
+ *
+ * @param toPath - A path template string that may contain `*` placeholders. Each `*` is replaced
+ * by the corresponding segment from the `fromPath`. Static paths (e.g., `/static/path`) are returned
+ * directly without placeholder replacement.
+ * @param fromPath - A base path string, split into segments, that provides values for
+ * replacing `*` placeholders in the `toPath`.
+ * @returns A resolved path string with `*` placeholders replaced by segments from the `fromPath`,
+ * or the `toPath` returned unchanged if it contains no placeholders.
+ *
+ * @throws If the `toPath` does not start with a `/`, indicating an invalid path format.
+ *
+ * @example
+ * ```typescript
+ * // Example with placeholders resolved
+ * const resolvedPath = buildPathWithParams('/*\/details', '/123/abc');
+ * console.log(resolvedPath); // Outputs: '/123/details'
+ *
+ * // Example with a static path
+ * const staticPath = buildPathWithParams('/static/path', '/base/unused');
+ * console.log(staticPath); // Outputs: '/static/path'
+ * ```
+ */
+export function buildPathWithParams(toPath: string, fromPath: string): string {
+  if (toPath[0] !== '/') {
+    throw new Error(`Invalid toPath: The string must start with a '/'. Received: '${toPath}'`);
+  }
+
+  if (fromPath[0] !== '/') {
+    throw new Error(`Invalid fromPath: The string must start with a '/'. Received: '${fromPath}'`);
+  }
+
+  if (!toPath.includes('/*')) {
+    return toPath;
+  }
+
+  const fromPathParts = fromPath.split('/');
+  const toPathParts = toPath.split('/');
+  const resolvedParts = toPathParts.map((part, index) =>
+    toPathParts[index] === '*' ? fromPathParts[index] : part,
+  );
+
+  return joinUrlParts(...resolvedParts);
+}
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');