From c8cd90e0f601a6baa05b84e45bbd37b4bf6049f5 Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Fri, 22 Nov 2024 09:48:49 +0000 Subject: [PATCH] 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 34574b290695afe2bde8ba30481a5535b097fae3) --- packages/angular/ssr/src/app.ts | 31 +++--- packages/angular/ssr/src/routes/ng-routes.ts | 62 ++++++++---- packages/angular/ssr/src/utils/url.ts | 68 +++++++++++++ packages/angular/ssr/test/app_spec.ts | 7 ++ .../angular/ssr/test/routes/ng-routes_spec.ts | 96 ++++++++++++++++++- .../angular/ssr/test/routes/router_spec.ts | 6 +- packages/angular/ssr/test/utils/url_spec.ts | 49 ++++++++++ 7 files changed, 277 insertions(+), 42 deletions(-) diff --git a/packages/angular/ssr/src/app.ts b/packages/angular/ssr/src/app.ts index e49a0b0e6c0e..eec56efefe80 100644 --- 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 { - 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 index 9e48f49b2b11..c77c70ffa18f 100644 --- 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,6 +219,7 @@ 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. @@ -221,6 +227,7 @@ async function* traverseRoutesConfig(options: { * @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 index db68ce3570e6..9d42ddd6db00 100644 --- 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 index df0898ca41bf..4b30d039bbef 100644 --- 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 index 050e0e09cd33..7c79a6669c70 100644 --- a/packages/angular/ssr/test/routes/ng-routes_spec.ts +++ b/packages/angular/ssr/test/routes/ng-routes_spec.ts @@ -47,7 +47,7 @@ describe('extractRoutesAndCreateRouteTree', () => { { route: '/', renderMode: RenderMode.Server }, { route: '/home', renderMode: RenderMode.Client }, { route: '/redirect', renderMode: RenderMode.Server, status: 301, redirectTo: '/home' }, - { route: '/user/:id', renderMode: RenderMode.Server }, + { route: '/user/*', renderMode: RenderMode.Server }, ]); }); @@ -93,7 +93,7 @@ describe('extractRoutesAndCreateRouteTree', () => { route: '/user/jane/role/writer', renderMode: RenderMode.Prerender, }, - { route: '/user/:id/role/:role', renderMode: RenderMode.Server }, + { route: '/user/*/role/*', renderMode: RenderMode.Server }, ]); }); @@ -128,7 +128,7 @@ describe('extractRoutesAndCreateRouteTree', () => { route: '/user/jane/role/writer', renderMode: RenderMode.Prerender, }, - { route: '/user/:id/role/:role', renderMode: RenderMode.Client }, + { route: '/user/*/role/*', renderMode: RenderMode.Client }, ]); }); @@ -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: '/*/thing', route: '/*', renderMode: RenderMode.Server }, + { route: '/*/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: '/*', renderMode: RenderMode.Server, redirectTo: '/*/thing' }, + { route: '/*/thing', renderMode: RenderMode.Server }, + ]); }); it('should not resolve parameterized routes for SSG when `invokeGetPrerenderParams` is false', async () => { @@ -192,7 +278,7 @@ describe('extractRoutesAndCreateRouteTree', () => { expect(errors).toHaveSize(0); expect(routeTree.toObject()).toEqual([ { route: '/home', renderMode: RenderMode.Server }, - { route: '/user/:id/role/:role', renderMode: RenderMode.Server }, + { route: '/user/*/role/*', renderMode: RenderMode.Server }, ]); }); @@ -273,7 +359,7 @@ describe('extractRoutesAndCreateRouteTree', () => { route: '/user/jane/role/writer', renderMode: RenderMode.Prerender, }, - { route: '/user/:id/role/:role', renderMode: RenderMode.Client }, + { route: '/user/*/role/*', renderMode: RenderMode.Client }, ]); }); diff --git a/packages/angular/ssr/test/routes/router_spec.ts b/packages/angular/ssr/test/routes/router_spec.ts index 4a64e0713eaf..3a8dfe30b00f 100644 --- a/packages/angular/ssr/test/routes/router_spec.ts +++ b/packages/angular/ssr/test/routes/router_spec.ts @@ -61,7 +61,7 @@ describe('ServerRouter', () => { status: 301, }); expect(router.match(new URL('http://localhost/user/123'))).toEqual({ - route: '/user/:id', + route: '/user/*', renderMode: RenderMode.Server, }); }); @@ -95,7 +95,7 @@ describe('ServerRouter', () => { renderMode: RenderMode.Server, }); expect(userMetadata).toEqual({ - route: '/user/:id', + route: '/user/*', renderMode: RenderMode.Server, }); }); @@ -116,7 +116,7 @@ describe('ServerRouter', () => { renderMode: RenderMode.Server, }); expect(userMetadata).toEqual({ - route: '/user/:id', + route: '/user/*', renderMode: RenderMode.Server, }); }); diff --git a/packages/angular/ssr/test/utils/url_spec.ts b/packages/angular/ssr/test/utils/url_spec.ts index 2cebcf0c5b53..b6fcd4e7e767 100644 --- a/packages/angular/ssr/test/utils/url_spec.ts +++ b/packages/angular/ssr/test/utils/url_spec.ts @@ -8,6 +8,8 @@ import { addLeadingSlash, + addTrailingSlash, + buildPathWithParams, joinUrlParts, stripIndexHtmlFromURL, stripLeadingSlash, @@ -53,6 +55,7 @@ describe('URL Utils', () => { describe('addLeadingSlash', () => { it('should add a leading slash to a URL without one', () => { + expect(addLeadingSlash('path')).toBe('/path'); expect(addLeadingSlash('path/')).toBe('/path/'); }); @@ -65,6 +68,21 @@ describe('URL Utils', () => { }); }); + describe('addTrailingSlash', () => { + it('should add a trailing slash to a URL without one', () => { + expect(addTrailingSlash('path')).toBe('path/'); + expect(addTrailingSlash('/path')).toBe('/path/'); + }); + + it('should not modify URL if it already has a trailing slash', () => { + expect(addLeadingSlash('/path/')).toBe('/path/'); + }); + + it('should handle empty URL', () => { + expect(addLeadingSlash('')).toBe('/'); + }); + }); + describe('joinUrlParts', () => { it('should join multiple URL parts with normalized slashes', () => { expect(joinUrlParts('', 'path/', '/to/resource')).toBe('/path/to/resource'); @@ -132,4 +150,35 @@ describe('URL Utils', () => { expect(result.href).toBe('https://www.example.com/page'); }); }); + + describe('buildPathWithParams', () => { + it('should return the same URL when there are no placeholders in the toPath', () => { + const fromPath = '/base/path'; + const toPath = '/static/path'; + + const result = buildPathWithParams(toPath, fromPath); + + // Since there are no placeholders, the URL remains the same. + expect(result.toString()).toBe('/static/path'); + }); + + it('should replace placeholders with corresponding segments from the base URL path', () => { + const fromPath = '/base/path'; + const toPath = '/*/*/details'; + + const result = buildPathWithParams(toPath, fromPath); + + expect(result.toString()).toBe('/base/path/details'); + }); + + it('should throw an error if the toPath does not start with "/"', () => { + const fromPath = '/base/path'; + const toPath = 'details'; + + // This should throw an error because toPath doesn't start with "/" + expect(() => { + buildPathWithParams(toPath, fromPath); + }).toThrowError(`Invalid toPath: The string must start with a '/'. Received: 'details'`); + }); + }); });