Skip to content

Commit 935c49c

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 6ecbada commit 935c49c

3 files changed

Lines changed: 119 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
};
196199
} else {
197200
yield metadata;
@@ -392,6 +395,7 @@ function appendPreloadToMetadata(
392395
*
393396
* @param serverConfigRouteTree - The tree representing the server's routing setup.
394397
* @param redirectTo - Optional path to redirect to, if specified.
398+
* @param parentRoute - The path of the parent route.
395399
* @param metadata - The metadata associated with the route tree node.
396400
* @param parentInjector - The dependency injection container for the parent route.
397401
* @param invokeGetPrerenderParams - A flag indicating whether to invoke the `getPrerenderParams` function.
@@ -401,6 +405,7 @@ function appendPreloadToMetadata(
401405
async function* handleSSGRoute(
402406
serverConfigRouteTree: RouteTree<ServerConfigRouteTreeAdditionalMetadata> | undefined,
403407
redirectTo: string | undefined,
408+
parentRoute: string,
404409
metadata: ServerConfigRouteTreeNodeMetadata,
405410
parentInjector: Injector,
406411
invokeGetPrerenderParams: boolean,
@@ -420,7 +425,7 @@ async function* handleSSGRoute(
420425
}
421426

422427
if (redirectTo !== undefined) {
423-
meta.redirectTo = resolveRedirectTo(currentRoutePath, redirectTo);
428+
meta.redirectTo = resolveRedirectTo(parentRoute, redirectTo);
424429
}
425430

426431
const isCatchAllRoute = CATCH_ALL_REGEXP.test(currentRoutePath);
@@ -478,7 +483,10 @@ async function* handleSSGRoute(
478483
redirectTo:
479484
redirectTo === undefined
480485
? undefined
481-
: resolveRedirectTo(routeWithResolvedParams, redirectTo),
486+
: resolveRedirectTo(
487+
parentRoute.replace(URL_PARAMETER_GLOBAL_REGEXP, replacer),
488+
redirectTo,
489+
),
482490
};
483491
}
484492
} catch (error) {
@@ -533,24 +541,21 @@ function handlePrerenderParamsReplacement(
533541
* Resolves the `redirectTo` property for a given route.
534542
*
535543
* This function processes the `redirectTo` property to ensure that it correctly
536-
* resolves relative to the current route path. If `redirectTo` is an absolute path,
537-
* it is returned as is. If it is a relative path, it is resolved based on the current route path.
544+
* resolves relative to the parent route path. If `redirectTo` is an absolute path,
545+
* it is returned as is. If it is a relative path, it is resolved based on the parent route path.
538546
*
539-
* @param routePath - The current route path.
547+
* @param parentRoute - The path of the parent route.
540548
* @param redirectTo - The target path for redirection.
541549
* @returns The resolved redirect path as a string.
542550
*/
543-
function resolveRedirectTo(routePath: string, redirectTo: string): string {
551+
function resolveRedirectTo(parentRoute: string, redirectTo: string): string {
544552
if (redirectTo[0] === '/') {
545553
// If the redirectTo path is absolute, return it as is.
546554
return redirectTo;
547555
}
548556

549-
// Resolve relative redirectTo based on the current route path.
550-
const segments = routePath.replace(URL_PARAMETER_GLOBAL_REGEXP, '*').split('/');
551-
segments.pop(); // Remove the last segment to make it relative.
552-
553-
return joinUrlParts(...segments, redirectTo);
557+
// Match Angular router behavior: replace all segments matched by the route.
558+
return joinUrlParts(parentRoute.replace(URL_PARAMETER_GLOBAL_REGEXP, '*'), redirectTo);
554559
}
555560

556561
/**

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

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ describe('AngularServerApp', () => {
8888
{ path: 'redirect/relative', redirectTo: 'home' },
8989
{ path: 'redirect/:param/relative', redirectTo: 'home' },
9090
{ path: 'redirect/absolute', redirectTo: '/home' },
91+
{ path: 'redirect-nested/:param', children: [{ path: 'old', redirectTo: 'new' }] },
9192
{
9293
path: 'redirect-to-function',
9394
redirectTo: () => 'home',
@@ -206,13 +207,19 @@ describe('AngularServerApp', () => {
206207

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

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

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

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,76 @@ describe('extractRoutesAndCreateRouteTree', () => {
378378
{ route: '/*/thing', renderMode: RenderMode.Server },
379379
]);
380380
});
381+
382+
it('should resolve multi-segment relative redirects of prerendered routes', async () => {
383+
setAngularAppTestingManifest(
384+
[
385+
{
386+
path: ':param',
387+
children: [
388+
{ path: 'old/page', redirectTo: 'new/page' },
389+
{ path: 'new/page', component: DummyComponent },
390+
],
391+
},
392+
],
393+
[
394+
{
395+
path: ':param/old/page',
396+
renderMode: RenderMode.Prerender,
397+
async getPrerenderParams() {
398+
return [{ param: 'some' }];
399+
},
400+
},
401+
{ path: '**', renderMode: RenderMode.Server },
402+
],
403+
);
404+
405+
const { routeTree, errors } = await extractRoutesAndCreateRouteTree({
406+
url,
407+
invokeGetPrerenderParams: true,
408+
includePrerenderFallbackRoutes: true,
409+
});
410+
411+
expect(errors).toHaveSize(0);
412+
expect(routeTree.toObject()).toEqual([
413+
{ route: '/*', renderMode: RenderMode.Server },
414+
{ route: '/*/old/page', renderMode: RenderMode.Server, redirectTo: '/*/new/page' },
415+
{ route: '/*/new/page', renderMode: RenderMode.Server },
416+
{ route: '/some/old/page', renderMode: RenderMode.Prerender, redirectTo: '/some/new/page' },
417+
]);
418+
});
419+
420+
it('should resolve relative redirects of prerendered catch-all routes', async () => {
421+
setAngularAppTestingManifest(
422+
[
423+
{ path: 'docs/**', redirectTo: 'new' },
424+
{ path: 'new', component: DummyComponent },
425+
],
426+
[
427+
{
428+
path: 'docs/**',
429+
renderMode: RenderMode.Prerender,
430+
async getPrerenderParams() {
431+
return [{ '**': 'a/b' }];
432+
},
433+
},
434+
{ path: '**', renderMode: RenderMode.Server },
435+
],
436+
);
437+
438+
const { routeTree, errors } = await extractRoutesAndCreateRouteTree({
439+
url,
440+
invokeGetPrerenderParams: true,
441+
includePrerenderFallbackRoutes: true,
442+
});
443+
444+
expect(errors).toHaveSize(0);
445+
expect(routeTree.toObject()).toEqual([
446+
{ route: '/docs/a/b', renderMode: RenderMode.Prerender, redirectTo: '/new' },
447+
{ route: '/docs/**', renderMode: RenderMode.Server, redirectTo: '/new' },
448+
{ route: '/new', renderMode: RenderMode.Server },
449+
]);
450+
});
381451
});
382452

383453
it('should extract routes with a route level matcher', async () => {
@@ -531,6 +601,29 @@ describe('extractRoutesAndCreateRouteTree', () => {
531601
]);
532602
});
533603

604+
it('should resolve multi-segment relative redirects against the parent route', async () => {
605+
setAngularAppTestingManifest(
606+
[
607+
{
608+
path: ':param',
609+
children: [
610+
{ path: 'old/page', redirectTo: 'new/page' },
611+
{ path: 'new/page', component: DummyComponent },
612+
],
613+
},
614+
],
615+
[{ path: '**', renderMode: RenderMode.Server }],
616+
);
617+
618+
const { routeTree, errors } = await extractRoutesAndCreateRouteTree({ url });
619+
expect(errors).toHaveSize(0);
620+
expect(routeTree.toObject()).toEqual([
621+
{ route: '/*', renderMode: RenderMode.Server },
622+
{ route: '/*/old/page', renderMode: RenderMode.Server, redirectTo: '/*/new/page' },
623+
{ route: '/*/new/page', renderMode: RenderMode.Server },
624+
]);
625+
});
626+
534627
it('should not resolve parameterized routes for SSG when `invokeGetPrerenderParams` is false', async () => {
535628
setAngularAppTestingManifest(
536629
[

0 commit comments

Comments
 (0)