Skip to content

Commit 7d9aac3

Browse files
ematipicosarah11918
ematipico
and
sarah11918
authoredJun 11, 2024··
fix(rewrite): purge old data when rewriting (#11207)
* fix(rewrite): purge old data when rewriting * remove logs * Update .changeset/fuzzy-eggs-kneel.md Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca> --------- Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
·
create-astro@4.11.1astro@4.10.2
1 parent bd849a4 commit 7d9aac3

File tree

13 files changed

+168
-60
lines changed

13 files changed

+168
-60
lines changed
 

‎.changeset/fuzzy-eggs-kneel.md

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'astro': patch
3+
---
4+
5+
Fixes an issue in the rewriting logic where old data was not purged during the rewrite flow. This caused some false positives when checking the validity of URL path names during the rendering phase.

‎packages/astro/src/container/pipeline.ts

+17-18
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import {
1313
createModuleScriptElement,
1414
createStylesheetElementSet,
1515
} from '../core/render/ssr-element.js';
16-
import { default404Page } from '../core/routing/astro-designed-error-pages.js';
16+
import {default404Page, DEFAULT_404_ROUTE} from '../core/routing/astro-designed-error-pages.js';
1717

1818
export class ContainerPipeline extends Pipeline {
1919
/**
@@ -70,30 +70,29 @@ export class ContainerPipeline extends Pipeline {
7070
return { links, styles, scripts };
7171
}
7272

73-
async tryRewrite(rewritePayload: RewritePayload): Promise<[RouteData, ComponentInstance]> {
73+
async tryRewrite(payload: RewritePayload, request: Request): Promise<[RouteData, ComponentInstance, URL]> {
7474
let foundRoute: RouteData | undefined;
7575
// options.manifest is the actual type that contains the information
76+
let finalUrl: URL | undefined = undefined;
7677
for (const route of this.manifest.routes) {
77-
const routeData = route.routeData;
78-
if (rewritePayload instanceof URL) {
79-
if (routeData.pattern.test(rewritePayload.pathname)) {
80-
foundRoute = routeData;
81-
break;
82-
}
83-
} else if (rewritePayload instanceof Request) {
84-
const url = new URL(rewritePayload.url);
85-
if (routeData.pattern.test(url.pathname)) {
86-
foundRoute = routeData;
87-
break;
88-
}
89-
} else if (routeData.pattern.test(decodeURI(rewritePayload))) {
90-
foundRoute = routeData;
78+
if (payload instanceof URL) {
79+
finalUrl = payload;
80+
} else if (payload instanceof Request) {
81+
finalUrl = new URL(payload.url);
82+
} else {
83+
finalUrl = new URL(payload, new URL(request.url).origin);
84+
}
85+
if (route.routeData.pattern.test(decodeURI(finalUrl.pathname))) {
86+
foundRoute = route.routeData;
87+
break;
88+
} else if (finalUrl.pathname === '/404') {
89+
foundRoute = DEFAULT_404_ROUTE;
9190
break;
9291
}
9392
}
94-
if (foundRoute) {
93+
if (foundRoute && finalUrl) {
9594
const componentInstance = await this.getComponentByRoute(foundRoute);
96-
return [foundRoute, componentInstance];
95+
return [foundRoute, componentInstance, finalUrl];
9796
} else {
9897
throw new AstroError(RouteNotFound);
9998
}

‎packages/astro/src/core/app/pipeline.ts

+5-6
Original file line numberDiff line numberDiff line change
@@ -87,12 +87,11 @@ export class AppPipeline extends Pipeline {
8787
payload: RewritePayload,
8888
request: Request,
8989
sourceRoute: RouteData
90-
): Promise<[RouteData, ComponentInstance]> {
90+
): Promise<[RouteData, ComponentInstance, URL]> {
9191
let foundRoute;
9292

93+
let finalUrl: URL | undefined = undefined;
9394
for (const route of this.#manifestData!.routes) {
94-
let finalUrl: URL | undefined = undefined;
95-
9695
if (payload instanceof URL) {
9796
finalUrl = payload;
9897
} else if (payload instanceof Request) {
@@ -110,13 +109,13 @@ export class AppPipeline extends Pipeline {
110109
}
111110
}
112111

113-
if (foundRoute) {
112+
if (foundRoute && finalUrl) {
114113
if (foundRoute.pathname === '/404') {
115114
const componentInstance = this.rewriteKnownRoute(foundRoute.pathname, sourceRoute);
116-
return [foundRoute, componentInstance];
115+
return [foundRoute, componentInstance, finalUrl];
117116
} else {
118117
const componentInstance = await this.getComponentByRoute(foundRoute);
119-
return [foundRoute, componentInstance];
118+
return [foundRoute, componentInstance, finalUrl];
120119
}
121120
}
122121
throw new AstroError({

‎packages/astro/src/core/base-pipeline.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ export abstract class Pipeline {
8888
rewritePayload: RewritePayload,
8989
request: Request,
9090
sourceRoute: RouteData
91-
): Promise<[RouteData, ComponentInstance]>;
91+
): Promise<[RouteData, ComponentInstance, URL]>;
9292

9393
/**
9494
* Tells the pipeline how to retrieve a component give a `RouteData`

‎packages/astro/src/core/build/pipeline.ts

+5-6
Original file line numberDiff line numberDiff line change
@@ -281,12 +281,11 @@ export class BuildPipeline extends Pipeline {
281281
payload: RewritePayload,
282282
request: Request,
283283
sourceRoute: RouteData
284-
): Promise<[RouteData, ComponentInstance]> {
284+
): Promise<[RouteData, ComponentInstance, URL]> {
285285
let foundRoute: RouteData | undefined;
286286
// options.manifest is the actual type that contains the information
287+
let finalUrl: URL | undefined = undefined;
287288
for (const route of this.options.manifest.routes) {
288-
let finalUrl: URL | undefined = undefined;
289-
290289
if (payload instanceof URL) {
291290
finalUrl = payload;
292291
} else if (payload instanceof Request) {
@@ -303,13 +302,13 @@ export class BuildPipeline extends Pipeline {
303302
break;
304303
}
305304
}
306-
if (foundRoute) {
305+
if (foundRoute && finalUrl) {
307306
if (foundRoute.pathname === '/404') {
308307
const componentInstance = await this.rewriteKnownRoute(foundRoute.pathname, sourceRoute);
309-
return [foundRoute, componentInstance];
308+
return [foundRoute, componentInstance, finalUrl];
310309
} else {
311310
const componentInstance = await this.getComponentByRoute(foundRoute);
312-
return [foundRoute, componentInstance];
311+
return [foundRoute, componentInstance, finalUrl];
313312
}
314313
} else {
315314
throw new AstroError({

‎packages/astro/src/core/render-context.ts

+25-23
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import { renderEndpoint } from '../runtime/server/endpoint.js';
2020
import { renderPage } from '../runtime/server/index.js';
2121
import {
2222
ASTRO_VERSION,
23-
DEFAULT_404_COMPONENT,
2423
REROUTE_DIRECTIVE_HEADER,
2524
ROUTE_TYPE_HEADER,
2625
clientAddressSymbol,
@@ -46,7 +45,7 @@ export class RenderContext {
4645
readonly pipeline: Pipeline,
4746
public locals: App.Locals,
4847
readonly middleware: MiddlewareHandler,
49-
readonly pathname: string,
48+
public pathname: string,
5049
public request: Request,
5150
public routeData: RouteData,
5251
public status: number,
@@ -103,16 +102,17 @@ export class RenderContext {
103102
componentInstance: ComponentInstance | undefined,
104103
slots: Record<string, any> = {}
105104
): Promise<Response> {
106-
const { cookies, middleware, pathname, pipeline } = this;
107-
const { logger, routeCache, serverLike, streaming } = pipeline;
105+
const { cookies, middleware, pipeline } = this;
106+
const { logger, serverLike, streaming } = pipeline;
107+
108108
const props =
109109
Object.keys(this.props).length > 0
110110
? this.props
111111
: await getProps({
112112
mod: componentInstance,
113113
routeData: this.routeData,
114-
routeCache,
115-
pathname,
114+
routeCache: this.pipeline.routeCache,
115+
pathname: this.pathname,
116116
logger,
117117
serverLike,
118118
});
@@ -222,7 +222,7 @@ export class RenderContext {
222222

223223
const rewrite = async (reroutePayload: RewritePayload) => {
224224
pipeline.logger.debug('router', 'Called rewriting to:', reroutePayload);
225-
const [routeData, component] = await pipeline.tryRewrite(
225+
const [routeData, component, newURL] = await pipeline.tryRewrite(
226226
reroutePayload,
227227
this.request,
228228
this.originalRoute
@@ -231,15 +231,13 @@ export class RenderContext {
231231
if (reroutePayload instanceof Request) {
232232
this.request = reroutePayload;
233233
} else {
234-
this.request = this.#copyRequest(
235-
new URL(routeData.pathname ?? routeData.route, this.url.origin),
236-
this.request
237-
);
234+
this.request = this.#copyRequest(newURL, this.request);
238235
}
239-
this.url = new URL(this.request.url);
236+
this.url = newURL;
240237
this.cookies = new AstroCookies(this.request);
241-
this.params = getParams(routeData, url.toString());
238+
this.params = getParams(routeData, this.url.pathname);
242239
this.isRewriting = true;
240+
this.pathname = this.url.pathname;
243241
return await this.render(component);
244242
};
245243

@@ -359,11 +357,17 @@ export class RenderContext {
359357
props: Record<string, any>,
360358
slotValues: Record<string, any> | null
361359
): AstroGlobal {
360+
let astroPagePartial;
361+
// During rewriting, we must recompute the Astro global, because we need to purge the previous params/props/etc.
362+
if (this.isRewriting) {
363+
astroPagePartial = this.#astroPagePartial = this.createAstroPagePartial(result, astroStaticPartial);
364+
} else {
362365
// Create page partial with static partial so they can be cached together.
363-
const astroPagePartial = (this.#astroPagePartial ??= this.createAstroPagePartial(
364-
result,
365-
astroStaticPartial
366-
));
366+
astroPagePartial = this.#astroPagePartial ??= this.createAstroPagePartial(
367+
result,
368+
astroStaticPartial
369+
);
370+
}
367371
// Create component-level partials. `Astro.self` is added by the compiler.
368372
const astroComponentPartial = { props, self: null };
369373

@@ -410,7 +414,7 @@ export class RenderContext {
410414

411415
const rewrite = async (reroutePayload: RewritePayload) => {
412416
pipeline.logger.debug('router', 'Calling rewrite: ', reroutePayload);
413-
const [routeData, component] = await pipeline.tryRewrite(
417+
const [routeData, component, newURL] = await pipeline.tryRewrite(
414418
reroutePayload,
415419
this.request,
416420
this.originalRoute
@@ -419,14 +423,12 @@ export class RenderContext {
419423
if (reroutePayload instanceof Request) {
420424
this.request = reroutePayload;
421425
} else {
422-
this.request = this.#copyRequest(
423-
new URL(routeData.pathname ?? routeData.route, this.url.origin),
424-
this.request
425-
);
426+
this.request = this.#copyRequest(newURL, this.request);
426427
}
427428
this.url = new URL(this.request.url);
428429
this.cookies = new AstroCookies(this.request);
429-
this.params = getParams(routeData, url.toString());
430+
this.params = getParams(routeData, this.url.pathname);
431+
this.pathname = this.url.pathname;
430432
this.isRewriting = true;
431433
return await this.render(component);
432434
};

‎packages/astro/src/vite-plugin-astro-server/pipeline.ts

+5-6
Original file line numberDiff line numberDiff line change
@@ -195,15 +195,14 @@ export class DevPipeline extends Pipeline {
195195
payload: RewritePayload,
196196
request: Request,
197197
sourceRoute: RouteData
198-
): Promise<[RouteData, ComponentInstance]> {
198+
): Promise<[RouteData, ComponentInstance, URL]> {
199199
let foundRoute;
200200
if (!this.manifestData) {
201201
throw new Error('Missing manifest data. This is an internal error, please file an issue.');
202202
}
203203

204+
let finalUrl: URL | undefined = undefined;
204205
for (const route of this.manifestData.routes) {
205-
let finalUrl: URL | undefined = undefined;
206-
207206
if (payload instanceof URL) {
208207
finalUrl = payload;
209208
} else if (payload instanceof Request) {
@@ -221,13 +220,13 @@ export class DevPipeline extends Pipeline {
221220
}
222221
}
223222

224-
if (foundRoute) {
223+
if (foundRoute && finalUrl) {
225224
if (foundRoute.pathname === '/404') {
226225
const componentInstance = this.rewriteKnownRoute(foundRoute.pathname, sourceRoute);
227-
return [foundRoute, componentInstance];
226+
return [foundRoute, componentInstance, finalUrl];
228227
} else {
229228
const componentInstance = await this.getComponentByRoute(foundRoute);
230-
return [foundRoute, componentInstance];
229+
return [foundRoute, componentInstance, finalUrl];
231230
}
232231
} else {
233232
throw new AstroError({

‎packages/astro/test/fixtures/rewrite-server/astro.config.mjs

+10
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
import { defineConfig } from 'astro/config';
2+
3+
// https://astro.build/config
4+
export default defineConfig({
5+
output: "server",
6+
experimental: {
7+
rewriting: true
8+
},
9+
site: "https://example.com"
10+
});

‎packages/astro/test/fixtures/rewrite-server/package.json

+8
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
{
2+
"name": "@test/reroute-server",
3+
"version": "0.0.0",
4+
"private": true,
5+
"dependencies": {
6+
"astro": "workspace:*"
7+
}
8+
}

‎packages/astro/test/fixtures/rewrite-server/src/pages/[slug]/title.astro

+14
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
---
2+
const { slug } = Astro.params;
3+
console.log("is it here???", Astro.params)
4+
export const prerender = false;
5+
---
6+
<html>
7+
<head>
8+
<title>Title</title>
9+
</head>
10+
<body>
11+
<h1>Title</h1>
12+
<p>{slug}</p>
13+
</body>
14+
</html>

‎packages/astro/test/fixtures/rewrite-server/src/pages/index.astro

+13
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
return Astro.rewrite('/some-slug/title')
3+
4+
export const prerender = false
5+
---
6+
<html>
7+
<head>
8+
<title>Index</title>
9+
</head>
10+
<body>
11+
<h1>Index</h1>
12+
</body>
13+
</html>

‎packages/astro/test/rewrite.test.js

+54
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,31 @@ describe('Dev reroute', () => {
6262
});
6363
});
6464

65+
describe('Dev rewrite, hybrid/server', () => {
66+
/** @type {import('./test-utils').Fixture} */
67+
let fixture;
68+
let devServer;
69+
70+
before(async () => {
71+
fixture = await loadFixture({
72+
root: './fixtures/rewrite-server/',
73+
});
74+
devServer = await fixture.startDevServer();
75+
});
76+
77+
after(async () => {
78+
await devServer.stop();
79+
});
80+
81+
it('should rewrite the [slug]/title ', async () => {
82+
const html = await fixture.fetch('/').then((res) => res.text());
83+
const $ = cheerioLoad(html);
84+
85+
assert.match($('h1').text(), /Title/);
86+
assert.match($('p').text(), /some-slug/);
87+
});
88+
});
89+
6590
describe('Build reroute', () => {
6691
/** @type {import('./test-utils').Fixture} */
6792
let fixture;
@@ -220,6 +245,35 @@ describe('SSR reroute', () => {
220245
});
221246
});
222247

248+
describe('SSR rewrite, hybrid/server', () => {
249+
/** @type {import('./test-utils').Fixture} */
250+
let fixture;
251+
let app;
252+
253+
before(async () => {
254+
fixture = await loadFixture({
255+
root: './fixtures/rewrite-server/',
256+
output: 'server',
257+
adapter: testAdapter(),
258+
});
259+
260+
await fixture.build();
261+
app = await fixture.loadTestAdapterApp();
262+
});
263+
264+
it('should rewrite the [slug]/title ', async () => {
265+
const request = new Request('http://example.com/');
266+
const response = await app.render(request);
267+
const html = await response.text();
268+
const $ = cheerioLoad(html);
269+
270+
console.log(html);
271+
272+
assert.match($('h1').text(), /Title/);
273+
assert.match($('p').text(), /some-slug/);
274+
});
275+
});
276+
223277
describe('Middleware', () => {
224278
/** @type {import('./test-utils').Fixture} */
225279
let fixture;

‎pnpm-lock.yaml

+6
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)
Please sign in to comment.