Skip to content

Commit 816ad70

Browse files
bschaeublinbschaeublin
committed
fix(@angular/ssr): resolve relative redirects against the parent route
When a route's own path has several segments, such as `old/page`, only its last segment was replaced when resolving a relative `redirectTo` on the server. A redirect to `new/page` was therefore sent to `/old/new/page`, while the router navigates to `/new/page` because it replaces all segments matched by the route. The server now does the same. This changes the redirect target of relative redirects on routes whose path has more than one segment. Fixes #34300
1 parent f2cd8c8 commit 816ad70

3 files changed

Lines changed: 144 additions & 14 deletions

File tree

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

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,7 @@ type EntryPointToBrowserMapping = AngularAppManifest['entryPointToBrowserMapping
146146
async function* handleRoute(options: {
147147
metadata: ServerConfigRouteTreeNodeMetadata;
148148
currentRoutePath: string;
149+
parentRoute: string;
149150
route: Route;
150151
compiler: Compiler;
151152
parentInjector: Injector;
@@ -158,6 +159,7 @@ async function* handleRoute(options: {
158159
const {
159160
metadata,
160161
currentRoutePath,
162+
parentRoute,
161163
route,
162164
compiler,
163165
parentInjector,
@@ -176,6 +178,7 @@ async function* handleRoute(options: {
176178
yield* handleSSGRoute(
177179
serverConfigRouteTree,
178180
typeof redirectTo === 'string' ? redirectTo : undefined,
181+
parentRoute,
179182
metadata,
180183
parentInjector,
181184
invokeGetPrerenderParams,
@@ -191,7 +194,7 @@ async function* handleRoute(options: {
191194
} else if (typeof redirectTo === 'string') {
192195
yield {
193196
...metadata,
194-
redirectTo: resolveRedirectTo(metadata.route, redirectTo),
197+
redirectTo: resolveRedirectTo(parentRoute, redirectTo),
195198
relativeRedirect: redirectTo[0] !== '/' ? true : undefined,
196199
};
197200
} else {
@@ -393,6 +396,7 @@ function appendPreloadToMetadata(
393396
*
394397
* @param serverConfigRouteTree - The tree representing the server's routing setup.
395398
* @param redirectTo - Optional path to redirect to, if specified.
399+
* @param parentRoute - The path of the parent route.
396400
* @param metadata - The metadata associated with the route tree node.
397401
* @param parentInjector - The dependency injection container for the parent route.
398402
* @param invokeGetPrerenderParams - A flag indicating whether to invoke the `getPrerenderParams` function.
@@ -402,6 +406,7 @@ function appendPreloadToMetadata(
402406
async function* handleSSGRoute(
403407
serverConfigRouteTree: RouteTree<ServerConfigRouteTreeAdditionalMetadata> | undefined,
404408
redirectTo: string | undefined,
409+
parentRoute: string,
405410
metadata: ServerConfigRouteTreeNodeMetadata,
406411
parentInjector: Injector,
407412
invokeGetPrerenderParams: boolean,
@@ -421,7 +426,7 @@ async function* handleSSGRoute(
421426
}
422427

423428
if (redirectTo !== undefined) {
424-
meta.redirectTo = resolveRedirectTo(currentRoutePath, redirectTo);
429+
meta.redirectTo = resolveRedirectTo(parentRoute, redirectTo);
425430
meta.relativeRedirect = redirectTo[0] !== '/' ? true : undefined;
426431
}
427432

@@ -480,7 +485,10 @@ async function* handleSSGRoute(
480485
redirectTo:
481486
redirectTo === undefined
482487
? undefined
483-
: resolveRedirectTo(routeWithResolvedParams, redirectTo),
488+
: resolveRedirectTo(
489+
parentRoute.replace(URL_PARAMETER_GLOBAL_REGEXP, replacer),
490+
redirectTo,
491+
),
484492
};
485493
}
486494
} catch (error) {
@@ -535,27 +543,24 @@ function handlePrerenderParamsReplacement(
535543
* Resolves the `redirectTo` property for a given route.
536544
*
537545
* This function processes the `redirectTo` property to ensure that it correctly
538-
* resolves relative to the current route path. If `redirectTo` is an absolute path,
539-
* it is returned as is. If it is a relative path, it is resolved based on the current route path,
546+
* resolves relative to the parent route path. If `redirectTo` is an absolute path,
547+
* it is returned as is. If it is a relative path, it is resolved based on the parent route path,
540548
* and, like in the Angular router, its query and fragment are ignored.
541549
*
542-
* @param routePath - The current route path.
550+
* @param parentRoute - The path of the parent route.
543551
* @param redirectTo - The target path for redirection.
544552
* @returns The resolved redirect path as a string.
545553
*/
546-
function resolveRedirectTo(routePath: string, redirectTo: string): string {
554+
function resolveRedirectTo(parentRoute: string, redirectTo: string): string {
547555
if (redirectTo[0] === '/') {
548556
// If the redirectTo path is absolute, return it as is.
549557
return redirectTo;
550558
}
551559

552-
// Resolve relative redirectTo based on the current route path.
553-
const segments = routePath.replace(URL_PARAMETER_GLOBAL_REGEXP, '*').split('/');
554-
segments.pop(); // Remove the last segment to make it relative.
555-
560+
// Match Angular router behavior: replace all segments matched by the route.
556561
const [path] = redirectTo.split(/[?#]/, 1);
557562

558-
return joinUrlParts(...segments, path);
563+
return joinUrlParts(parentRoute.replace(URL_PARAMETER_GLOBAL_REGEXP, '*'), path);
559564
}
560565

561566
/**

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ describe('AngularServerApp', () => {
8989
{ path: 'redirect/:param/relative', redirectTo: 'home' },
9090
{ path: 'redirect/absolute', redirectTo: '/home' },
9191
{ path: 'redirect-with-query', redirectTo: 'home?param=value' },
92+
{ path: 'redirect-nested/:param', children: [{ path: 'old', redirectTo: 'new' }] },
9293
{
9394
path: 'redirect-to-function',
9495
redirectTo: () => 'home',
@@ -207,13 +208,19 @@ describe('AngularServerApp', () => {
207208

208209
it('should correctly handle relative nested redirects', async () => {
209210
const response = await app.handle(new Request('http://localhost/redirect/relative'));
210-
expect(response?.headers.get('location')).toBe('/redirect/home');
211+
expect(response?.headers.get('location')).toBe('/home');
211212
expect(response?.status).toBe(302);
212213
});
213214

214215
it('should correctly handle relative nested redirects with parameter', async () => {
215216
const response = await app.handle(new Request('http://localhost/redirect/param/relative'));
216-
expect(response?.headers.get('location')).toBe('/redirect/param/home');
217+
expect(response?.headers.get('location')).toBe('/home');
218+
expect(response?.status).toBe(302);
219+
});
220+
221+
it('should correctly handle relative redirects of parameterized parent routes', async () => {
222+
const response = await app.handle(new Request('http://localhost/redirect-nested/x/old'));
223+
expect(response?.headers.get('location')).toBe('/redirect-nested/x/new');
217224
expect(response?.status).toBe(302);
218225
});
219226

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

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -399,6 +399,96 @@ describe('extractRoutesAndCreateRouteTree', () => {
399399
{ route: '/*/thing', renderMode: RenderMode.Server },
400400
]);
401401
});
402+
403+
it('should resolve multi-segment relative redirects of prerendered routes', async () => {
404+
setAngularAppTestingManifest(
405+
[
406+
{
407+
path: ':param',
408+
children: [
409+
{ path: 'old/page', redirectTo: 'new/page' },
410+
{ path: 'new/page', component: DummyComponent },
411+
],
412+
},
413+
],
414+
[
415+
{
416+
path: ':param/old/page',
417+
renderMode: RenderMode.Prerender,
418+
async getPrerenderParams() {
419+
return [{ param: 'some' }];
420+
},
421+
},
422+
{ path: '**', renderMode: RenderMode.Server },
423+
],
424+
);
425+
426+
const { routeTree, errors } = await extractRoutesAndCreateRouteTree({
427+
url,
428+
invokeGetPrerenderParams: true,
429+
includePrerenderFallbackRoutes: true,
430+
});
431+
432+
expect(errors).toHaveSize(0);
433+
expect(routeTree.toObject()).toEqual([
434+
{ route: '/*', renderMode: RenderMode.Server },
435+
{
436+
route: '/*/old/page',
437+
renderMode: RenderMode.Server,
438+
redirectTo: '/*/new/page',
439+
relativeRedirect: true,
440+
},
441+
{ route: '/*/new/page', renderMode: RenderMode.Server },
442+
{
443+
route: '/some/old/page',
444+
renderMode: RenderMode.Prerender,
445+
redirectTo: '/some/new/page',
446+
relativeRedirect: true,
447+
},
448+
]);
449+
});
450+
451+
it('should resolve relative redirects of prerendered catch-all routes', async () => {
452+
setAngularAppTestingManifest(
453+
[
454+
{ path: 'docs/**', redirectTo: 'new' },
455+
{ path: 'new', component: DummyComponent },
456+
],
457+
[
458+
{
459+
path: 'docs/**',
460+
renderMode: RenderMode.Prerender,
461+
async getPrerenderParams() {
462+
return [{ '**': 'a/b' }];
463+
},
464+
},
465+
{ path: '**', renderMode: RenderMode.Server },
466+
],
467+
);
468+
469+
const { routeTree, errors } = await extractRoutesAndCreateRouteTree({
470+
url,
471+
invokeGetPrerenderParams: true,
472+
includePrerenderFallbackRoutes: true,
473+
});
474+
475+
expect(errors).toHaveSize(0);
476+
expect(routeTree.toObject()).toEqual([
477+
{
478+
route: '/docs/a/b',
479+
renderMode: RenderMode.Prerender,
480+
redirectTo: '/new',
481+
relativeRedirect: true,
482+
},
483+
{
484+
route: '/docs/**',
485+
renderMode: RenderMode.Server,
486+
redirectTo: '/new',
487+
relativeRedirect: true,
488+
},
489+
{ route: '/new', renderMode: RenderMode.Server },
490+
]);
491+
});
402492
});
403493

404494
it('should extract routes with a route level matcher', async () => {
@@ -562,6 +652,34 @@ describe('extractRoutesAndCreateRouteTree', () => {
562652
]);
563653
});
564654

655+
it('should resolve multi-segment relative redirects against the parent route', async () => {
656+
setAngularAppTestingManifest(
657+
[
658+
{
659+
path: ':param',
660+
children: [
661+
{ path: 'old/page', redirectTo: 'new/page' },
662+
{ path: 'new/page', component: DummyComponent },
663+
],
664+
},
665+
],
666+
[{ path: '**', renderMode: RenderMode.Server }],
667+
);
668+
669+
const { routeTree, errors } = await extractRoutesAndCreateRouteTree({ url });
670+
expect(errors).toHaveSize(0);
671+
expect(routeTree.toObject()).toEqual([
672+
{ route: '/*', renderMode: RenderMode.Server },
673+
{
674+
route: '/*/old/page',
675+
renderMode: RenderMode.Server,
676+
redirectTo: '/*/new/page',
677+
relativeRedirect: true,
678+
},
679+
{ route: '/*/new/page', renderMode: RenderMode.Server },
680+
]);
681+
});
682+
565683
it('should not mark absolute redirects as relative', async () => {
566684
setAngularAppTestingManifest(
567685
[

0 commit comments

Comments
 (0)