From 9339ea3d3f1dae58e8f815172d330b36643aaa6b Mon Sep 17 00:00:00 2001 From: lilnasy <69170106+lilnasy@users.noreply.github.com> Date: Wed, 11 Oct 2023 23:54:39 +0000 Subject: [PATCH 1/9] fix(rerouting): attempt without middleware --- packages/astro/src/core/app/index.ts | 17 ++++++++++++++--- packages/astro/src/core/pipeline.ts | 6 ++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index 9cf01f82d2f0..4cd7753806b9 100644 --- a/packages/astro/src/core/app/index.ts +++ b/packages/astro/src/core/app/index.ts @@ -41,6 +41,7 @@ export interface RenderErrorOptions { routeData?: RouteData; response?: Response; status: 404 | 500; + runMiddleware?: boolean; } export class App { @@ -252,7 +253,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, runMiddleware = true }: RenderErrorOptions): Promise { const errorRouteData = matchRoute('/' + status, this.#manifestData); const url = new URL(request.url); if (errorRouteData) { @@ -271,6 +272,7 @@ export class App { return this.#mergeResponses(response, originalResponse, override); } const mod = await this.#getModuleForRoute(errorRouteData); + let usedMiddleware = false; try { const newRenderContext = await this.#createRenderContext( url, @@ -280,12 +282,21 @@ export class App { status ); const page = (await mod.page()) as any; - if (mod.onRequest) { + if (runMiddleware && mod.onRequest) { this.#pipeline.setMiddlewareFunction(mod.onRequest as MiddlewareEndpointHandler); + usedMiddleware = true; + } + if (!runMiddleware) { + 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 (runMiddleware && usedMiddleware) { + return this.#renderError(request, { status, response: originalResponse, runMiddleware: false }); + } + } } 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..70d6878f5728 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; } + /** + * Ensure that a route is rendered without its corresponding middleware. + */ + unsetMiddlewareFunction() { + this.#onRequest = undefined; + } /** * Returns the current environment */ From 443c872f6032fb87c971cf48229fea5b7170079d Mon Sep 17 00:00:00 2001 From: lilnasy <69170106+lilnasy@users.noreply.github.com> Date: Thu, 12 Oct 2023 01:00:39 +0000 Subject: [PATCH 2/9] add test --- .../middleware space/src/middleware.js | 2 ++ .../middleware space/src/pages/500.astro | 1 + .../middleware space/src/pages/throw.astro | 0 packages/astro/test/middleware.test.js | 27 ++++++++++--------- 4 files changed, 18 insertions(+), 12 deletions(-) create mode 100644 packages/astro/test/fixtures/middleware space/src/pages/500.astro create mode 100644 packages/astro/test/fixtures/middleware space/src/pages/throw.astro 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/', From 5f63617c5114ad055df3a8a720e5a6bd3fb430ce Mon Sep 17 00:00:00 2001 From: lilnasy <69170106+lilnasy@users.noreply.github.com> Date: Thu, 12 Oct 2023 01:02:07 +0000 Subject: [PATCH 3/9] add changeset --- .changeset/shaggy-onions-try.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/shaggy-onions-try.md diff --git a/.changeset/shaggy-onions-try.md b/.changeset/shaggy-onions-try.md new file mode 100644 index 000000000000..c14c22982dbf --- /dev/null +++ b/.changeset/shaggy-onions-try.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixed an issue where 500.astro did not render when the middleware threw an error. From 9ef94b220bf498511024e49b303a892649f2027f Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Thu, 12 Oct 2023 18:00:46 +0100 Subject: [PATCH 4/9] Update .changeset/shaggy-onions-try.md --- .changeset/shaggy-onions-try.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/shaggy-onions-try.md b/.changeset/shaggy-onions-try.md index c14c22982dbf..3ae58f2d81dd 100644 --- a/.changeset/shaggy-onions-try.md +++ b/.changeset/shaggy-onions-try.md @@ -2,4 +2,4 @@ 'astro': patch --- -Fixed an issue where 500.astro did not render when the middleware threw an error. +Fix an issue where `500.astro` did not render when the middleware threw an error. From afd52bc707154c7320848dbf29e6c4f0b74d50af Mon Sep 17 00:00:00 2001 From: lilnasy <69170106+lilnasy@users.noreply.github.com> Date: Fri, 13 Oct 2023 14:39:02 +0000 Subject: [PATCH 5/9] avoid extra variable --- packages/astro/src/core/app/index.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index 4cd7753806b9..3dce1d9e3224 100644 --- a/packages/astro/src/core/app/index.ts +++ b/packages/astro/src/core/app/index.ts @@ -272,7 +272,6 @@ export class App { return this.#mergeResponses(response, originalResponse, override); } const mod = await this.#getModuleForRoute(errorRouteData); - let usedMiddleware = false; try { const newRenderContext = await this.#createRenderContext( url, @@ -284,16 +283,16 @@ export class App { const page = (await mod.page()) as any; if (runMiddleware && mod.onRequest) { this.#pipeline.setMiddlewareFunction(mod.onRequest as MiddlewareEndpointHandler); - usedMiddleware = true; } if (!runMiddleware) { + // 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 { // Middleware may be the cause of the error, so we try rendering 404/500.astro without it. - if (runMiddleware && usedMiddleware) { + if (runMiddleware && mod.onRequest) { return this.#renderError(request, { status, response: originalResponse, runMiddleware: false }); } } From c344c33ab10495de7a0e120e9af06aa1fc74e5d4 Mon Sep 17 00:00:00 2001 From: lilnasy <69170106+lilnasy@users.noreply.github.com> Date: Fri, 13 Oct 2023 14:45:14 +0000 Subject: [PATCH 6/9] document runMiddleware internal option --- packages/astro/src/core/app/index.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index 3dce1d9e3224..5691e6f1101d 100644 --- a/packages/astro/src/core/app/index.ts +++ b/packages/astro/src/core/app/index.ts @@ -41,6 +41,9 @@ export interface RenderErrorOptions { routeData?: RouteData; response?: Response; status: 404 | 500; + /** + * Whether to call onRequest() while rendering the error page + */ runMiddleware?: boolean; } From 0234e89ad6d52bda14483549c047892964dd825c Mon Sep 17 00:00:00 2001 From: lilnasy <69170106+lilnasy@users.noreply.github.com> Date: Mon, 16 Oct 2023 13:23:19 +0000 Subject: [PATCH 7/9] document runMiddleware default --- packages/astro/src/core/app/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index 5691e6f1101d..a119497f59cc 100644 --- a/packages/astro/src/core/app/index.ts +++ b/packages/astro/src/core/app/index.ts @@ -42,7 +42,7 @@ export interface RenderErrorOptions { response?: Response; status: 404 | 500; /** - * Whether to call onRequest() while rendering the error page + * Whether to call onRequest() while rendering the error page. Defaults to true. */ runMiddleware?: boolean; } From 9dbb178aa8e48a45758e806093952f72a0d7ab22 Mon Sep 17 00:00:00 2001 From: Arsh <69170106+lilnasy@users.noreply.github.com> Date: Tue, 17 Oct 2023 22:47:49 +0530 Subject: [PATCH 8/9] Apply suggestions from code review Co-authored-by: Emanuele Stoppa --- packages/astro/src/core/pipeline.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/astro/src/core/pipeline.ts b/packages/astro/src/core/pipeline.ts index 70d6878f5728..dd1e66a52c03 100644 --- a/packages/astro/src/core/pipeline.ts +++ b/packages/astro/src/core/pipeline.ts @@ -56,7 +56,7 @@ export class Pipeline { } /** - * Ensure that a route is rendered without its corresponding middleware. + * Removes the current middleware function. Subsequent requests won't trigger any middleware. */ unsetMiddlewareFunction() { this.#onRequest = undefined; From 4242c6858ac9e4445e038cb04c99ae9599b87c6b Mon Sep 17 00:00:00 2001 From: lilnasy <69170106+lilnasy@users.noreply.github.com> Date: Tue, 17 Oct 2023 17:22:50 +0000 Subject: [PATCH 9/9] runMiddleware -> skipMiddleware --- packages/astro/src/core/app/index.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index a119497f59cc..6475718aca24 100644 --- a/packages/astro/src/core/app/index.ts +++ b/packages/astro/src/core/app/index.ts @@ -42,9 +42,9 @@ export interface RenderErrorOptions { response?: Response; status: 404 | 500; /** - * Whether to call onRequest() while rendering the error page. Defaults to true. + * Whether to skip onRequest() while rendering the error page. Defaults to false. */ - runMiddleware?: boolean; + skipMiddleware?: boolean; } export class App { @@ -256,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, runMiddleware = true }: RenderErrorOptions): Promise { + 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) { @@ -284,10 +284,10 @@ export class App { status ); const page = (await mod.page()) as any; - if (runMiddleware && mod.onRequest) { + if (skipMiddleware === false && mod.onRequest) { this.#pipeline.setMiddlewareFunction(mod.onRequest as MiddlewareEndpointHandler); } - if (!runMiddleware) { + if (skipMiddleware) { // make sure middleware set by other requests is cleared out this.#pipeline.unsetMiddlewareFunction(); } @@ -295,8 +295,8 @@ export class App { return this.#mergeResponses(response, originalResponse); } catch { // Middleware may be the cause of the error, so we try rendering 404/500.astro without it. - if (runMiddleware && mod.onRequest) { - return this.#renderError(request, { status, response: originalResponse, runMiddleware: false }); + if (skipMiddleware === false && mod.onRequest) { + return this.#renderError(request, { status, response: originalResponse, skipMiddleware: true }); } } }