Skip to content

Commit b3bbada

Browse files
bschaeublinbschaeublin
authored
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.
1 parent f2cd8c8 commit b3bbada

3 files changed

Lines changed: 147 additions & 17 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: 12 additions & 5 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',
@@ -205,19 +206,25 @@ describe('AngularServerApp', () => {
205206
expect(response?.status).toBe(302);
206207
});
207208

208-
it('should correctly handle relative nested redirects', async () => {
209+
it('should correctly handle relative redirects on multi-segment routes', 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

214-
it('should correctly handle relative nested redirects with parameter', async () => {
215+
it('should correctly handle relative redirects on multi-segment routes 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

220-
it('should correctly handle absolute nested redirects', async () => {
227+
it('should correctly handle absolute redirects on multi-segment routes', async () => {
221228
const response = await app.handle(new Request('http://localhost/redirect/absolute'));
222229
expect(response?.headers.get('location')).toBe('/home');
223230
expect(response?.status).toBe(302);

‎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)