Skip to content

Commit 03996ec

Browse files
committed
fix(@angular/ssr): resolve route parameters in redirect targets
Route parameters in the path of a `redirectTo` target, such as `:id` in `profile/:id`, were sent unchanged in the `Location` header by the server, while the router replaces them with the matching parameters of the current URL. The server now does the same.
1 parent f0a74e8 commit 03996ec

5 files changed

Lines changed: 143 additions & 7 deletions

File tree

‎packages/angular/ssr/src/routes/ng-routes.ts‎

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -485,9 +485,12 @@ async function* handleSSGRoute(
485485
redirectTo:
486486
redirectTo === undefined
487487
? undefined
488-
: resolveRedirectTo(
489-
parentRoute.replace(URL_PARAMETER_GLOBAL_REGEXP, replacer),
490-
redirectTo,
488+
: replaceRedirectParams(
489+
resolveRedirectTo(
490+
parentRoute.replace(URL_PARAMETER_GLOBAL_REGEXP, replacer),
491+
redirectTo,
492+
),
493+
(parameter) => params[parameter.slice(1)],
491494
),
492495
};
493496
}
@@ -563,6 +566,29 @@ function resolveRedirectTo(parentRoute: string, redirectTo: string): string {
563566
return joinUrlParts(parentRoute.replace(URL_PARAMETER_GLOBAL_REGEXP, '*'), path);
564567
}
565568

569+
/**
570+
* Replaces the route parameters, such as `:id`, in the path of `redirectTo`.
571+
*
572+
* Like in the Angular router, only whole path segments starting with `:` are parameters.
573+
* Parameters for which `resolve` returns `undefined` are kept as is.
574+
*
575+
* @param redirectTo - The target path for redirection.
576+
* @param resolve - A function that returns the replacement for a parameter, such as `:id`.
577+
* @returns The redirect path with its route parameters replaced.
578+
*/
579+
function replaceRedirectParams(
580+
redirectTo: string,
581+
resolve: (parameter: string) => string | undefined,
582+
): string {
583+
const [path] = redirectTo.split(/[?#]/, 1);
584+
const resolvedPath = path
585+
.split('/')
586+
.map((segment) => (segment[0] === ':' ? (resolve(segment) ?? segment) : segment))
587+
.join('/');
588+
589+
return resolvedPath + redirectTo.slice(path.length);
590+
}
591+
566592
/**
567593
* Builds a server configuration route tree from the given server routes configuration.
568594
*
@@ -836,8 +862,19 @@ export function extractRoutesAndCreateRouteTree(options: {
836862
);
837863

838864
for (const { route, ...metadata } of routes) {
865+
const fullRoute = joinUrlParts(baseHref, route);
866+
839867
if (metadata.redirectTo !== undefined) {
840-
metadata.redirectTo = joinUrlParts(baseHref, metadata.redirectTo);
868+
// Reference the route parameters by their position in the URL (see `buildPathWithParams`).
869+
const routeSegments = fullRoute.split('/');
870+
metadata.redirectTo = replaceRedirectParams(
871+
joinUrlParts(baseHref, metadata.redirectTo),
872+
(parameter) => {
873+
const position = routeSegments.lastIndexOf(parameter);
874+
875+
return position === -1 ? undefined : `*${position}`;
876+
},
877+
);
841878
}
842879

843880
// Remove undefined fields
@@ -849,7 +886,6 @@ export function extractRoutesAndCreateRouteTree(options: {
849886
}
850887
}
851888

852-
const fullRoute = joinUrlParts(baseHref, route);
853889
routeTree.insert(fullRoute, metadata);
854890
}
855891

‎packages/angular/ssr/src/utils/url.ts‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,12 +152,19 @@ export function stripIndexHtmlFromURL(url: URL): URL {
152152
return url;
153153
}
154154

155+
/**
156+
* Matches a `*<n>` placeholder that makes up a whole path segment, which may be followed by
157+
* the query or fragment of the URL.
158+
*/
159+
const POSITIONAL_PLACEHOLDER_REGEXP = /^\*(\d+)(?=[?#]|$)/;
160+
155161
/**
156162
* Resolves `*` placeholders in a path template by mapping them to corresponding segments
157163
* from a base path. This is useful for constructing paths dynamically based on a given base path.
158164
*
159165
* The function processes the `toPath` string, replacing each `*` placeholder with
160-
* the corresponding segment from the `fromPath`. If the `toPath` contains no placeholders,
166+
* the corresponding segment from the `fromPath`, and each `*<n>` placeholder with the
167+
* segment at position `n` of the `fromPath`. If the `toPath` contains no placeholders,
161168
* it is returned as-is. Invalid `toPath` formats (not starting with `/`) will throw an error.
162169
*
163170
* @param toPath - A path template string that may contain `*` placeholders. Each `*` is replaced
@@ -176,6 +183,10 @@ export function stripIndexHtmlFromURL(url: URL): URL {
176183
* const resolvedPath = buildPathWithParams('/*\/details', '/123/abc');
177184
* console.log(resolvedPath); // Outputs: '/123/details'
178185
*
186+
* // Example with a positional placeholder
187+
* const positionalPath = buildPathWithParams('/user/*1', '/123/abc');
188+
* console.log(positionalPath); // Outputs: '/user/123'
189+
*
179190
* // Example with a static path
180191
* const staticPath = buildPathWithParams('/static/path', '/base/unused');
181192
* console.log(staticPath); // Outputs: '/static/path'
@@ -197,7 +208,9 @@ export function buildPathWithParams(toPath: string, fromPath: string): string {
197208
const fromPathParts = fromPath.split('/');
198209
const toPathParts = toPath.split('/');
199210
const resolvedParts = toPathParts.map((part, index) =>
200-
toPathParts[index] === '*' ? fromPathParts[index] : part,
211+
part === '*'
212+
? fromPathParts[index]
213+
: part.replace(POSITIONAL_PLACEHOLDER_REGEXP, (_, position) => fromPathParts[position]),
201214
);
202215

203216
return joinUrlParts(...resolvedParts);

‎packages/angular/ssr/test/app_spec.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,8 @@ describe('AngularServerApp', () => {
9090
{ path: 'redirect/absolute', redirectTo: '/home' },
9191
{ path: 'redirect-with-query', redirectTo: 'home?param=value' },
9292
{ path: 'redirect-nested/:param', children: [{ path: 'old', redirectTo: 'new' }] },
93+
{ path: 'redirect-param/:a/:b', redirectTo: '/home/:b/:a' },
94+
{ path: 'redirect-param-relative/:id', redirectTo: 'target/:id' },
9395
{
9496
path: 'redirect-to-function',
9597
redirectTo: () => 'home',
@@ -252,6 +254,20 @@ describe('AngularServerApp', () => {
252254
expect(response?.status).toBe(302);
253255
});
254256

257+
it('should resolve route parameters in absolute redirect targets', async () => {
258+
const response = await app.handle(new Request('http://localhost/redirect-param/x/y'));
259+
expect(response?.headers.get('location')).toBe('/home/y/x');
260+
expect(response?.status).toBe(302);
261+
});
262+
263+
it('should resolve route parameters in relative redirect targets', async () => {
264+
const response = await app.handle(
265+
new Request('http://localhost/redirect-param-relative/7?filter=test'),
266+
);
267+
expect(response?.headers.get('location')).toBe('/target/7?filter=test');
268+
expect(response?.status).toBe(302);
269+
});
270+
255271
it('should handle request abortion gracefully', async () => {
256272
const controller = new AbortController();
257273
const request = new Request('http://localhost/home', { signal: controller.signal });

‎packages/angular/ssr/test/routes/ng-routes_spec.ts‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -489,6 +489,38 @@ describe('extractRoutesAndCreateRouteTree', () => {
489489
{ route: '/new', renderMode: RenderMode.Server },
490490
]);
491491
});
492+
493+
it('should resolve route parameters of redirect targets of prerendered routes', async () => {
494+
setAngularAppTestingManifest(
495+
[
496+
{ path: 'user/:id', redirectTo: '/profile/:id' },
497+
{ path: 'profile/:id', component: DummyComponent },
498+
],
499+
[
500+
{
501+
path: 'user/:id',
502+
renderMode: RenderMode.Prerender,
503+
async getPrerenderParams() {
504+
return [{ id: '7' }];
505+
},
506+
},
507+
{ path: '**', renderMode: RenderMode.Server },
508+
],
509+
);
510+
511+
const { routeTree, errors } = await extractRoutesAndCreateRouteTree({
512+
url,
513+
invokeGetPrerenderParams: true,
514+
includePrerenderFallbackRoutes: true,
515+
});
516+
517+
expect(errors).toHaveSize(0);
518+
expect(routeTree.toObject()).toEqual([
519+
{ route: '/user/7', renderMode: RenderMode.Prerender, redirectTo: '/profile/7' },
520+
{ route: '/user/*', renderMode: RenderMode.Server, redirectTo: '/profile/*2' },
521+
{ route: '/profile/*', renderMode: RenderMode.Server },
522+
]);
523+
});
492524
});
493525

494526
it('should extract routes with a route level matcher', async () => {
@@ -652,6 +684,36 @@ describe('extractRoutesAndCreateRouteTree', () => {
652684
]);
653685
});
654686

687+
it('should reference route parameters of redirect targets by their position', async () => {
688+
setAngularAppTestingManifest(
689+
[
690+
{
691+
path: ':lang',
692+
children: [
693+
{ path: 'user/:id', redirectTo: 'profile/:id' },
694+
{ path: 'profile/:id', component: DummyComponent },
695+
],
696+
},
697+
{ path: 'search/:term', redirectTo: '/find/:term' },
698+
],
699+
[{ path: '**', renderMode: RenderMode.Server }],
700+
);
701+
702+
const { routeTree, errors } = await extractRoutesAndCreateRouteTree({ url });
703+
expect(errors).toHaveSize(0);
704+
expect(routeTree.toObject()).toEqual([
705+
{ route: '/*', renderMode: RenderMode.Server },
706+
{
707+
route: '/*/user/*',
708+
renderMode: RenderMode.Server,
709+
redirectTo: '/*/profile/*3',
710+
relativeRedirect: true,
711+
},
712+
{ route: '/*/profile/*', renderMode: RenderMode.Server },
713+
{ route: '/search/*', renderMode: RenderMode.Server, redirectTo: '/find/*2' },
714+
]);
715+
});
716+
655717
it('should resolve multi-segment relative redirects against the parent route', async () => {
656718
setAngularAppTestingManifest(
657719
[

‎packages/angular/ssr/test/utils/url_spec.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,15 @@ describe('URL Utils', () => {
184184
expect(result.toString()).toBe('/base/path/details');
185185
});
186186

187+
it('should replace positional placeholders with the segments at those positions', () => {
188+
expect(buildPathWithParams('/user/*2/*1', '/a/b')).toBe('/user/b/a');
189+
});
190+
191+
it('should replace a positional placeholder followed by a query or fragment', () => {
192+
expect(buildPathWithParams('/user/*1?tab=1', '/a')).toBe('/user/a?tab=1');
193+
expect(buildPathWithParams('/user/*1#top', '/a')).toBe('/user/a#top');
194+
});
195+
187196
it('should throw an error if the toPath does not start with "/"', () => {
188197
const fromPath = '/base/path';
189198
const toPath = 'details';

0 commit comments

Comments
 (0)