diff --git a/.changeset/shaggy-onions-try.md b/.changeset/shaggy-onions-try.md new file mode 100644 index 000000000000..3ae58f2d81dd --- /dev/null +++ b/.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. diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index 9cf01f82d2f0..6475718aca24 100644 --- a/packages/astro/src/core/app/index.ts +++ b/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 { 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); diff --git a/packages/astro/src/core/pipeline.ts b/packages/astro/src/core/pipeline.ts index 438ff275d1e8..dd1e66a52c03 100644 --- a/packages/astro/src/core/pipeline.ts +++ b/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 */ diff --git a/packages/astro/test/fixtures/middleware space/src/middleware.js b/packages/astro/test/fixtures/middleware space/src/middleware.js index b2e5c7e2d6b1..a889569c4344 100644 --- a/packages/astro/test/fixtures/middleware space/src/middleware.js +++ b/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(); diff --git a/packages/astro/test/fixtures/middleware space/src/pages/500.astro b/packages/astro/test/fixtures/middleware space/src/pages/500.astro new file mode 100644 index 000000000000..fc6b2588b72b --- /dev/null +++ b/packages/astro/test/fixtures/middleware space/src/pages/500.astro @@ -0,0 +1 @@ +

There was an error rendering the page.

\ No newline at end of file diff --git a/packages/astro/test/fixtures/middleware space/src/pages/throw.astro b/packages/astro/test/fixtures/middleware space/src/pages/throw.astro new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/packages/astro/test/middleware.test.js b/packages/astro/test/middleware.test.js index d3cb83310135..b882fcc84cb7 100644 --- a/packages/astro/test/middleware.test.js +++ b/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,14 +153,12 @@ 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); @@ -167,7 +166,6 @@ describe('Middleware API in PROD mode, SSR', () => { }); 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,14 +175,12 @@ 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(); @@ -192,8 +188,7 @@ describe('Middleware API in PROD mode, SSR', () => { 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,7 +197,6 @@ 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); @@ -210,7 +204,6 @@ describe('Middleware API in PROD mode, SSR', () => { }); 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(); @@ -218,7 +211,6 @@ describe('Middleware API in PROD mode, SSR', () => { }); 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("

There was an error rendering the page.

") + }); + it('the integration should receive the path to the middleware', async () => { fixture = await loadFixture({ root: './fixtures/middleware space/',