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 <[email protected]> --------- Co-authored-by: Sarah Rainsberger <[email protected]>
withastro · Jun 11, 2024 · 7d9aac3 · 7d9aac3
1 parent bd849a4
commit 7d9aac3
Show file tree

Hide file tree

Showing 13 changed files with 168 additions and 60 deletions.
diff --git a/.changeset/fuzzy-eggs-kneel.md b/.changeset/fuzzy-eggs-kneel.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+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.
diff --git a/packages/astro/src/container/pipeline.ts b/packages/astro/src/container/pipeline.ts
@@ -13,7 +13,7 @@ import {
 	createModuleScriptElement,
 	createStylesheetElementSet,
 } from '../core/render/ssr-element.js';
-import { default404Page } from '../core/routing/astro-designed-error-pages.js';
+import {default404Page, DEFAULT_404_ROUTE} from '../core/routing/astro-designed-error-pages.js';
 
 export class ContainerPipeline extends Pipeline {
 	/**
@@ -70,30 +70,29 @@ export class ContainerPipeline extends Pipeline {
 		return { links, styles, scripts };
 	}
 
-	async tryRewrite(rewritePayload: RewritePayload): Promise<[RouteData, ComponentInstance]> {
+	async tryRewrite(payload: RewritePayload, request: Request): Promise<[RouteData, ComponentInstance, URL]> {
 		let foundRoute: RouteData | undefined;
 		// options.manifest is the actual type that contains the information
+		let finalUrl: URL | undefined = undefined;
 		for (const route of this.manifest.routes) {
-			const routeData = route.routeData;
-			if (rewritePayload instanceof URL) {
-				if (routeData.pattern.test(rewritePayload.pathname)) {
-					foundRoute = routeData;
-					break;
-				}
-			} else if (rewritePayload instanceof Request) {
-				const url = new URL(rewritePayload.url);
-				if (routeData.pattern.test(url.pathname)) {
-					foundRoute = routeData;
-					break;
-				}
-			} else if (routeData.pattern.test(decodeURI(rewritePayload))) {
-				foundRoute = routeData;
+			if (payload instanceof URL) {
+				finalUrl = payload;
+			} else if (payload instanceof Request) {
+				finalUrl = new URL(payload.url);
+			} else {
+				finalUrl = new URL(payload, new URL(request.url).origin);
+			}
+			if (route.routeData.pattern.test(decodeURI(finalUrl.pathname))) {
+				foundRoute = route.routeData;
+				break;
+			} else if (finalUrl.pathname === '/404') {
+				foundRoute = DEFAULT_404_ROUTE;
 				break;
 			}
 		}
-		if (foundRoute) {
+		if (foundRoute && finalUrl) {
 			const componentInstance = await this.getComponentByRoute(foundRoute);
-			return [foundRoute, componentInstance];
+			return [foundRoute, componentInstance, finalUrl];
 		} else {
 			throw new AstroError(RouteNotFound);
 		}

diff --git a/packages/astro/src/core/app/pipeline.ts b/packages/astro/src/core/app/pipeline.ts
@@ -87,12 +87,11 @@ export class AppPipeline extends Pipeline {
 		payload: RewritePayload,
 		request: Request,
 		sourceRoute: RouteData
-	): Promise<[RouteData, ComponentInstance]> {
+	): Promise<[RouteData, ComponentInstance, URL]> {
 		let foundRoute;
 
+		let finalUrl: URL | undefined = undefined;
 		for (const route of this.#manifestData!.routes) {
-			let finalUrl: URL | undefined = undefined;
-
 			if (payload instanceof URL) {
 				finalUrl = payload;
 			} else if (payload instanceof Request) {
@@ -110,13 +109,13 @@ export class AppPipeline extends Pipeline {
 			}
 		}
 
-		if (foundRoute) {
+		if (foundRoute && finalUrl) {
 			if (foundRoute.pathname === '/404') {
 				const componentInstance = this.rewriteKnownRoute(foundRoute.pathname, sourceRoute);
-				return [foundRoute, componentInstance];
+				return [foundRoute, componentInstance, finalUrl];
 			} else {
 				const componentInstance = await this.getComponentByRoute(foundRoute);
-				return [foundRoute, componentInstance];
+				return [foundRoute, componentInstance, finalUrl];
 			}
 		}
 		throw new AstroError({

diff --git a/packages/astro/src/core/base-pipeline.ts b/packages/astro/src/core/base-pipeline.ts
@@ -88,7 +88,7 @@ export abstract class Pipeline {
 		rewritePayload: RewritePayload,
 		request: Request,
 		sourceRoute: RouteData
-	): Promise<[RouteData, ComponentInstance]>;
+	): Promise<[RouteData, ComponentInstance, URL]>;
 
 	/**
 	 * Tells the pipeline how to retrieve a component give a `RouteData`

diff --git a/packages/astro/src/core/build/pipeline.ts b/packages/astro/src/core/build/pipeline.ts
@@ -281,12 +281,11 @@ export class BuildPipeline extends Pipeline {
 		payload: RewritePayload,
 		request: Request,
 		sourceRoute: RouteData
-	): Promise<[RouteData, ComponentInstance]> {
+	): Promise<[RouteData, ComponentInstance, URL]> {
 		let foundRoute: RouteData | undefined;
 		// options.manifest is the actual type that contains the information
+		let finalUrl: URL | undefined = undefined;
 		for (const route of this.options.manifest.routes) {
-			let finalUrl: URL | undefined = undefined;
-
 			if (payload instanceof URL) {
 				finalUrl = payload;
 			} else if (payload instanceof Request) {
@@ -303,13 +302,13 @@ export class BuildPipeline extends Pipeline {
 				break;
 			}
 		}
-		if (foundRoute) {
+		if (foundRoute && finalUrl) {
 			if (foundRoute.pathname === '/404') {
 				const componentInstance = await this.rewriteKnownRoute(foundRoute.pathname, sourceRoute);
-				return [foundRoute, componentInstance];
+				return [foundRoute, componentInstance, finalUrl];
 			} else {
 				const componentInstance = await this.getComponentByRoute(foundRoute);
-				return [foundRoute, componentInstance];
+				return [foundRoute, componentInstance, finalUrl];
 			}
 		} else {
 			throw new AstroError({

diff --git a/packages/astro/src/core/render-context.ts b/packages/astro/src/core/render-context.ts
@@ -20,7 +20,6 @@ import { renderEndpoint } from '../runtime/server/endpoint.js';
 import { renderPage } from '../runtime/server/index.js';
 import {
 	ASTRO_VERSION,
-	DEFAULT_404_COMPONENT,
 	REROUTE_DIRECTIVE_HEADER,
 	ROUTE_TYPE_HEADER,
 	clientAddressSymbol,
@@ -46,7 +45,7 @@ export class RenderContext {
 		readonly pipeline: Pipeline,
 		public locals: App.Locals,
 		readonly middleware: MiddlewareHandler,
-		readonly pathname: string,
+		public pathname: string,
 		public request: Request,
 		public routeData: RouteData,
 		public status: number,
@@ -103,16 +102,17 @@ export class RenderContext {
 		componentInstance: ComponentInstance | undefined,
 		slots: Record<string, any> = {}
 	): Promise<Response> {
-		const { cookies, middleware, pathname, pipeline } = this;
-		const { logger, routeCache, serverLike, streaming } = pipeline;
+		const { cookies, middleware, pipeline } = this;
+		const { logger, serverLike, streaming } = pipeline;
+
 		const props =
 			Object.keys(this.props).length > 0
 				? this.props
 				: await getProps({
 						mod: componentInstance,
 						routeData: this.routeData,
-						routeCache,
-						pathname,
+						routeCache: this.pipeline.routeCache,
+						pathname: this.pathname,
 						logger,
 						serverLike,
 					});
@@ -222,7 +222,7 @@ export class RenderContext {
 
 		const rewrite = async (reroutePayload: RewritePayload) => {
 			pipeline.logger.debug('router', 'Called rewriting to:', reroutePayload);
-			const [routeData, component] = await pipeline.tryRewrite(
+			const [routeData, component, newURL] = await pipeline.tryRewrite(
 				reroutePayload,
 				this.request,
 				this.originalRoute
@@ -231,15 +231,13 @@ export class RenderContext {
 			if (reroutePayload instanceof Request) {
 				this.request = reroutePayload;
 			} else {
-				this.request = this.#copyRequest(
-					new URL(routeData.pathname ?? routeData.route, this.url.origin),
-					this.request
-				);
+				this.request = this.#copyRequest(newURL, this.request);
 			}
-			this.url = new URL(this.request.url);
+			this.url = newURL;
 			this.cookies = new AstroCookies(this.request);
-			this.params = getParams(routeData, url.toString());
+			this.params = getParams(routeData, this.url.pathname);
 			this.isRewriting = true;
+			this.pathname = this.url.pathname;
 			return await this.render(component);
 		};
 
@@ -359,11 +357,17 @@ export class RenderContext {
 		props: Record<string, any>,
 		slotValues: Record<string, any> | null
 	): AstroGlobal {
+		let astroPagePartial;
+		// During rewriting, we must recompute the Astro global, because we need to purge the previous params/props/etc.  
+		if (this.isRewriting) {
+			astroPagePartial = this.#astroPagePartial = this.createAstroPagePartial(result, astroStaticPartial);
+		} else {
 		// Create page partial with static partial so they can be cached together.
-		const astroPagePartial = (this.#astroPagePartial ??= this.createAstroPagePartial(
-			result,
-			astroStaticPartial
-		));
+			astroPagePartial = this.#astroPagePartial ??= this.createAstroPagePartial(
+				result,
+				astroStaticPartial
+			);
+		}
 		// Create component-level partials. `Astro.self` is added by the compiler.
 		const astroComponentPartial = { props, self: null };
 
@@ -410,7 +414,7 @@ export class RenderContext {
 
 		const rewrite = async (reroutePayload: RewritePayload) => {
 			pipeline.logger.debug('router', 'Calling rewrite: ', reroutePayload);
-			const [routeData, component] = await pipeline.tryRewrite(
+			const [routeData, component, newURL] = await pipeline.tryRewrite(
 				reroutePayload,
 				this.request,
 				this.originalRoute
@@ -419,14 +423,12 @@ export class RenderContext {
 			if (reroutePayload instanceof Request) {
 				this.request = reroutePayload;
 			} else {
-				this.request = this.#copyRequest(
-					new URL(routeData.pathname ?? routeData.route, this.url.origin),
-					this.request
-				);
+				this.request = this.#copyRequest(newURL, this.request);
 			}
 			this.url = new URL(this.request.url);
 			this.cookies = new AstroCookies(this.request);
-			this.params = getParams(routeData, url.toString());
+			this.params = getParams(routeData, this.url.pathname);
+			this.pathname = this.url.pathname;
 			this.isRewriting = true;
 			return await this.render(component);
 		};

diff --git a/packages/astro/src/vite-plugin-astro-server/pipeline.ts b/packages/astro/src/vite-plugin-astro-server/pipeline.ts
@@ -195,15 +195,14 @@ export class DevPipeline extends Pipeline {
 		payload: RewritePayload,
 		request: Request,
 		sourceRoute: RouteData
-	): Promise<[RouteData, ComponentInstance]> {
+	): Promise<[RouteData, ComponentInstance, URL]> {
 		let foundRoute;
 		if (!this.manifestData) {
 			throw new Error('Missing manifest data. This is an internal error, please file an issue.');
 		}
 
+		let finalUrl: URL | undefined = undefined;
 		for (const route of this.manifestData.routes) {
-			let finalUrl: URL | undefined = undefined;
-
 			if (payload instanceof URL) {
 				finalUrl = payload;
 			} else if (payload instanceof Request) {
@@ -221,13 +220,13 @@ export class DevPipeline extends Pipeline {
 			}
 		}
 
-		if (foundRoute) {
+		if (foundRoute && finalUrl) {
 			if (foundRoute.pathname === '/404') {
 				const componentInstance = this.rewriteKnownRoute(foundRoute.pathname, sourceRoute);
-				return [foundRoute, componentInstance];
+				return [foundRoute, componentInstance, finalUrl];
 			} else {
 				const componentInstance = await this.getComponentByRoute(foundRoute);
-				return [foundRoute, componentInstance];
+				return [foundRoute, componentInstance, finalUrl];
 			}
 		} else {
 			throw new AstroError({

diff --git a/packages/astro/test/fixtures/rewrite-server/astro.config.mjs b/packages/astro/test/fixtures/rewrite-server/astro.config.mjs
@@ -0,0 +1,10 @@
+import { defineConfig } from 'astro/config';
+
+// https://astro.build/config
+export default defineConfig({
+	output: "server",
+	experimental: {
+		rewriting: true
+	},
+	site: "https://example.com"
+});
diff --git a/packages/astro/test/fixtures/rewrite-server/package.json b/packages/astro/test/fixtures/rewrite-server/package.json
@@ -0,0 +1,8 @@
+{
+  "name": "@test/reroute-server",
+  "version": "0.0.0",
+  "private": true,
+  "dependencies": {
+    "astro": "workspace:*"
+  }
+}
diff --git a/packages/astro/test/fixtures/rewrite-server/src/pages/[slug]/title.astro b/packages/astro/test/fixtures/rewrite-server/src/pages/[slug]/title.astro
@@ -0,0 +1,14 @@
+---
+const { slug } = Astro.params;
+console.log("is it here???", Astro.params)
+export const prerender = false;
+---
+<html>
+<head>
+	<title>Title</title>
+</head>
+<body>
+<h1>Title</h1>
+<p>{slug}</p>
+</body>
+</html>
diff --git a/packages/astro/test/fixtures/rewrite-server/src/pages/index.astro b/packages/astro/test/fixtures/rewrite-server/src/pages/index.astro
@@ -0,0 +1,13 @@
+---
+return Astro.rewrite('/some-slug/title')
+
+export const prerender = false
+---
+<html>
+<head>
+	<title>Index</title>
+</head>
+<body>
+<h1>Index</h1>
+</body>
+</html>