Skip to content

Commit

Permalink
fix(routing): return correct status code for 500.astro and `404.ast…
Browse files Browse the repository at this point in the history
…ro` (#11308)

* fix(routing): return correct status code for `500.astro` and `404.astro`

* changeset

* fix regression

* use `route` instead
  • Loading branch information
ematipico authored Jun 24, 2024
1 parent edd35d3 commit 44c61dd
Show file tree
Hide file tree
Showing 14 changed files with 100 additions and 10 deletions.
5 changes: 5 additions & 0 deletions .changeset/curvy-emus-mate.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fixes an issue where custom `404.astro` and `500.astro` were not returning the correct status code when rendered inside a rewriting cycle.
4 changes: 2 additions & 2 deletions packages/astro/src/core/cookies/response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export function responseHasCookies(response: Response): boolean {
return Reflect.has(response, astroCookiesSymbol);
}

export function getFromResponse(response: Response): AstroCookies | undefined {
export function getCookiesFromResponse(response: Response): AstroCookies | undefined {
let cookies = Reflect.get(response, astroCookiesSymbol);
if (cookies != null) {
return cookies as AstroCookies;
Expand All @@ -20,7 +20,7 @@ export function getFromResponse(response: Response): AstroCookies | undefined {
}

export function* getSetCookiesFromResponse(response: Response): Generator<string, string[]> {
const cookies = getFromResponse(response);
const cookies = getCookiesFromResponse(response);
if (!cookies) {
return [];
}
Expand Down
5 changes: 3 additions & 2 deletions packages/astro/src/core/render-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
responseSentSymbol,
} from './constants.js';
import { AstroCookies, attachCookiesToResponse } from './cookies/index.js';
import { getFromResponse } from './cookies/response.js';
import { getCookiesFromResponse } from './cookies/response.js';
import { AstroError, AstroErrorData } from './errors/index.js';
import { callMiddleware } from './middleware/callMiddleware.js';
import { sequence } from './middleware/index.js';
Expand Down Expand Up @@ -136,6 +136,7 @@ export class RenderContext {
const lastNext = async (ctx: APIContext, payload?: RewritePayload) => {
if (payload) {
if (this.pipeline.manifest.rewritingEnabled) {
pipeline.logger.debug('router', 'Called rewriting to:', payload);
// we intentionally let the error bubble up
const [routeData, component] = await pipeline.tryRewrite(
payload,
Expand Down Expand Up @@ -197,7 +198,7 @@ export class RenderContext {
}
// We need to merge the cookies from the response back into this.cookies
// because they may need to be passed along from a rewrite.
const responseCookies = getFromResponse(response);
const responseCookies = getCookiesFromResponse(response);
if (responseCookies) {
cookies.merge(responseCookies);
}
Expand Down
17 changes: 14 additions & 3 deletions packages/astro/src/runtime/server/render/page.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { RouteData, SSRResult } from '../../../@types/astro.js';
import type {AstroConfig, RouteData, SSRResult} from '../../../@types/astro.js';
import { type NonAstroPageComponent, renderComponentToString } from './component.js';
import type { AstroComponentFactory } from './index.js';

Expand Down Expand Up @@ -85,6 +85,17 @@ export async function renderPage(
if (route?.component.endsWith('.md')) {
headers.set('Content-Type', 'text/html; charset=utf-8');
}
const response = new Response(body, { ...init, headers });
return response;
let status = init.status;
// Custom 404.astro and 500.astro are particular routes that must return a fixed status code
if (route?.route === "/404") {
status = 404
} else if (route?.route === "/500") {
status = 500
}
if (status) {
return new Response(body, { ...init, headers, status });
} else {
return new Response(body, { ...init, headers });

}
}
2 changes: 1 addition & 1 deletion packages/astro/test/custom-404-implicit-rerouting.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ for (const caseNumber of [1, 2, 3, 4, 5]) {
});

// IMPORTANT: never skip
it('prod server stays responsive', { timeout: 1000 }, async () => {
it('prod server stays responsive for case number ' + caseNumber, { timeout: 1000 }, async () => {
const response = await app.render(new Request('https://example.com/alvsibdlvjks'));
assert.equal(response.status, 404);
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"name": "@test/rewrite-404-invalid",
"name": "@test/rewrite-custom-404",
"version": "0.0.0",
"private": true,
"dependencies": {
Expand Down
10 changes: 10 additions & 0 deletions packages/astro/test/fixtures/rewrite-custom-404/src/middleware.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@


export const onRequest = async (context, next) => {
if (context.url.pathname.startsWith("/404") || context.url.pathname.startsWith("/500")) {
context.locals = {
interjected: "Interjected"
}
}
return await next();
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
const interjected = Astro.locals.interjected;
---

<html>
<head>
<title>Custom error</title>
</head>
<body>
<h1>Custom error</h1>
<p>{interjected}</p>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
const interjected = Astro.locals.interjected;
---

<html>
<head>
<title>Custom error</title>
</head>
<body>
<h1>Custom error</h1>
<p>{interjected}</p>
</body>
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
return Astro.rewrite("/500")
---
34 changes: 34 additions & 0 deletions packages/astro/test/rewrite.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,40 @@ describe('Middleware', () => {
});
});


describe('Middleware with custom 404.astro and 500.astro', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
let devServer;

before(async () => {
fixture = await loadFixture({
root: './fixtures/rewrite-custom-404/',
});
devServer = await fixture.startDevServer();
});

after(async () => {
await devServer.stop();
});

it('The `next()` function should return a Response with status code 404', async () => {
const html = await fixture.fetch('/about').then((res) => res.text());
const $ = cheerioLoad(html);

assert.equal($('h1').text(), 'Custom error');
assert.equal($('p').text(), 'Interjected');
});

it('The `next()` function should return a Response with status code 500', async () => {
const html = await fixture.fetch('/about-2').then((res) => res.text());
const $ = cheerioLoad(html);

assert.equal($('h1').text(), 'Custom error');
assert.equal($('p').text(), 'Interjected');
});
});

describe('Runtime error, default 500', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;
Expand Down
2 changes: 1 addition & 1 deletion pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 44c61dd

Please sign in to comment.