fix(rerouting): attempt without middleware (#8814)

* fix(rerouting): attempt without middleware

* add test

* add changeset

* Update .changeset/shaggy-onions-try.md

* avoid extra variable

* document runMiddleware internal option

* document runMiddleware default

* Apply suggestions from code review

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

* runMiddleware -> skipMiddleware

---------

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

.changeset/shaggy-onions-try.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fix an issue where `500.astro` did not render when the middleware threw an error.

packages/astro/src/core/app/index.ts

@@ -41,6 +41,10 @@ export interface RenderErrorOptions {
 	routeData?: RouteData;
 	response?: Response;
 	status: 404 | 500;
+	/**
+	 * Whether to skip onRequest() while rendering the error page. Defaults to false.
+	*/
+	skipMiddleware?: boolean;
 }
 
 export class App {
@@ -252,7 +256,7 @@ export class App {
 	 * If it is a known error code, try sending the according page (e.g. 404.astro / 500.astro).
 	 * This also handles pre-rendered /404 or /500 routes
 	 */
-	async #renderError(request: Request, { status, response: originalResponse }: RenderErrorOptions) {
+	async #renderError(request: Request, { status, response: originalResponse, skipMiddleware = false }: RenderErrorOptions): Promise<Response> {
 		const errorRouteData = matchRoute('/' + status, this.#manifestData);
 		const url = new URL(request.url);
 		if (errorRouteData) {
@@ -280,12 +284,21 @@ export class App {
 					status
 				);
 				const page = (await mod.page()) as any;
-				if (mod.onRequest) {
+				if (skipMiddleware === false && mod.onRequest) {
 					this.#pipeline.setMiddlewareFunction(mod.onRequest as MiddlewareEndpointHandler);
 				}
+				if (skipMiddleware) {
+					// make sure middleware set by other requests is cleared out
+					this.#pipeline.unsetMiddlewareFunction();
+				}
 				const response = await this.#pipeline.renderRoute(newRenderContext, page);
 				return this.#mergeResponses(response, originalResponse);
-			} catch {}
+			} catch {
+				// Middleware may be the cause of the error, so we try rendering 404/500.astro without it.
+				if (skipMiddleware === false && mod.onRequest) {
+					return this.#renderError(request, { status, response: originalResponse, skipMiddleware: true });
+				}
+			}
 		}
 
 		const response = this.#mergeResponses(new Response(null, { status }), originalResponse);

packages/astro/src/core/pipeline.ts

@@ -55,6 +55,12 @@ export class Pipeline {
 		this.#onRequest = onRequest;
 	}
 
+	/**
+	 * Removes the current middleware function. Subsequent requests won't trigger any middleware.
+	 */
+	unsetMiddlewareFunction() {
+		this.#onRequest = undefined;
+	}
 	/**
 	 * Returns the current environment
 	 */

packages/astro/test/fixtures/middleware space/src/middleware.js

@@ -18,6 +18,8 @@ const first = defineMiddleware(async (context, next) => {
 		return new Response(JSON.stringify(object), {
 			headers: response.headers,
 		});
+	} else if (context.url.pathname === '/throw') {
+		throw new Error;
 	} else if (context.url.pathname === '/clone') {
 		const response = await next();
 		const newResponse = response.clone();

packages/astro/test/fixtures/middleware space/src/pages/500.astro

@@ -0,0 +1 @@
+<h1>There was an error rendering the page.</h1>

packages/astro/test/middleware.test.js

@@ -119,6 +119,8 @@ describe('Middleware API in PROD mode, SSR', () => {
 	/** @type {import('./test-utils').Fixture} */
 	let fixture;
 	let middlewarePath;
+	/** @type {import('../src/core/app/index').App} */
+	let app;
 
 	before(async () => {
 		fixture = await loadFixture({
@@ -127,10 +129,10 @@ describe('Middleware API in PROD mode, SSR', () => {
 			adapter: testAdapter({}),
 		});
 		await fixture.build();
+		app = await fixture.loadTestAdapterApp();
 	});
 
 	it('should render locals data', async () => {
-		const app = await fixture.loadTestAdapterApp();
 		const request = new Request('http://example.com/');
 		const response = await app.render(request);
 		const html = await response.text();
@@ -139,7 +141,6 @@ describe('Middleware API in PROD mode, SSR', () => {
 	});
 
 	it('should change locals data based on URL', async () => {
-		const app = await fixture.loadTestAdapterApp();
 		let response = await app.render(new Request('http://example.com/'));
 		let html = await response.text();
 		let $ = cheerio.load(html);
@@ -152,22 +153,19 @@ describe('Middleware API in PROD mode, SSR', () => {
 	});
 
 	it('should successfully redirect to another page', async () => {
-		const app = await fixture.loadTestAdapterApp();
 		const request = new Request('http://example.com/redirect');
 		const response = await app.render(request);
 		expect(response.status).to.equal(302);
 	});
 
 	it('should call a second middleware', async () => {
-		const app = await fixture.loadTestAdapterApp();
 		const response = await app.render(new Request('http://example.com/second'));
 		const html = await response.text();
 		const $ = cheerio.load(html);
 		expect($('p').html()).to.equal('second');
 	});
 
 	it('should successfully create a new response', async () => {
-		const app = await fixture.loadTestAdapterApp();
 		const request = new Request('http://example.com/rewrite');
 		const response = await app.render(request);
 		const html = await response.text();
@@ -177,23 +175,20 @@ describe('Middleware API in PROD mode, SSR', () => {
 	});
 
 	it('should return a new response that is a 500', async () => {
-		const app = await fixture.loadTestAdapterApp();
 		const request = new Request('http://example.com/broken-500');
 		const response = await app.render(request);
 		expect(response.status).to.equal(500);
 	});
 
 	it('should successfully render a page if the middleware calls only next() and returns nothing', async () => {
-		const app = await fixture.loadTestAdapterApp();
 		const request = new Request('http://example.com/not-interested');
 		const response = await app.render(request);
 		const html = await response.text();
 		const $ = cheerio.load(html);
 		expect($('p').html()).to.equal('Not interested');
 	});
 
-	it("should throws an error when the middleware doesn't call next or doesn't return a response", async () => {
-		const app = await fixture.loadTestAdapterApp();
+	it("should throw an error when the middleware doesn't call next or doesn't return a response", async () => {
 		const request = new Request('http://example.com/does-nothing');
 		const response = await app.render(request);
 		const html = await response.text();
@@ -202,23 +197,20 @@ describe('Middleware API in PROD mode, SSR', () => {
 	});
 
 	it('should correctly work for API endpoints that return a Response object', async () => {
-		const app = await fixture.loadTestAdapterApp();
 		const request = new Request('http://example.com/api/endpoint');
 		const response = await app.render(request);
 		expect(response.status).to.equal(200);
 		expect(response.headers.get('Content-Type')).to.equal('application/json');
 	});
 
 	it('should correctly manipulate the response coming from API endpoints (not simple)', async () => {
-		const app = await fixture.loadTestAdapterApp();
 		const request = new Request('http://example.com/api/endpoint');
 		const response = await app.render(request);
 		const text = await response.text();
 		expect(text.includes('REDACTED')).to.be.true;
 	});
 
 	it('should correctly call the middleware function for 404', async () => {
-		const app = await fixture.loadTestAdapterApp();
 		const request = new Request('http://example.com/funky-url');
 		const routeData = app.match(request, { matchNotFound: true });
 		const response = await app.render(request, routeData);
@@ -227,6 +219,17 @@ describe('Middleware API in PROD mode, SSR', () => {
 		expect(text.includes('bar')).to.be.true;
 	});
 
+	it('should render 500.astro when the middleware throws an error', async () => {
+		const request = new Request('http://example.com/throw');
+		const routeData = app.match(request, { matchNotFound: true });
+
+		const response = await app.render(request, routeData);
+		expect(response).to.deep.include({ status: 500 });
+
+		const text = await response.text();
+		expect(text).to.include("<h1>There was an error rendering the page.</h1>")
+	});
+
 	it('the integration should receive the path to the middleware', async () => {
 		fixture = await loadFixture({
 			root: './fixtures/middleware space/',