fix: middleware for API endpoints (#7106)

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>

.changeset/long-starfishes-raise.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fix middleware for API endpoints that use `Response`, and log a warning for endpoints that don't use `Response`. 

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

@@ -10,7 +10,7 @@ import type { RouteInfo, SSRManifest as Manifest } from './types';
 
 import mime from 'mime';
 import { attachToResponse, getSetCookiesFromResponse } from '../cookies/index.js';
-import { call as callEndpoint, createAPIContext } from '../endpoint/index.js';
+import { callEndpoint, createAPIContext } from '../endpoint/index.js';
 import { consoleLogDestination } from '../logger/console.js';
 import { error, type LogOptions } from '../logger/core.js';
 import { callMiddleware } from '../middleware/callMiddleware.js';
@@ -224,6 +224,7 @@ export class App {
 			let response;
 			if (onRequest) {
 				response = await callMiddleware<Response>(
+					this.#env.logging,
 					onRequest as MiddlewareResponseHandler,
 					apiContext,
 					() => {

packages/astro/src/core/build/generate.ts

@@ -28,11 +28,7 @@ import {
 } from '../../core/path.js';
 import { runHookBuildGenerated } from '../../integrations/index.js';
 import { BEFORE_HYDRATION_SCRIPT_ID, PAGE_SCRIPT_ID } from '../../vite-plugin-scripts/index.js';
-import {
-	call as callEndpoint,
-	createAPIContext,
-	throwIfRedirectNotAllowed,
-} from '../endpoint/index.js';
+import { callEndpoint, createAPIContext, throwIfRedirectNotAllowed } from '../endpoint/index.js';
 import { AstroError } from '../errors/index.js';
 import { debug, info } from '../logger/core.js';
 import { callMiddleware } from '../middleware/callMiddleware.js';
@@ -495,6 +491,7 @@ async function generatePath(
 			const onRequest = middleware?.onRequest;
 			if (onRequest) {
 				response = await callMiddleware<Response>(
+					env.logging,
 					onRequest as MiddlewareResponseHandler,
 					apiContext,
 					() => {

packages/astro/src/core/endpoint/dev/index.ts

@@ -2,7 +2,7 @@ import type { EndpointHandler } from '../../../@types/astro';
 import type { LogOptions } from '../../logger/core';
 import type { SSROptions } from '../../render/dev';
 import { createRenderContext } from '../../render/index.js';
-import { call as callEndpoint } from '../index.js';
+import { callEndpoint } from '../index.js';
 
 export async function call(options: SSROptions, logging: LogOptions) {
 	const {

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

@@ -93,7 +93,7 @@ export function createAPIContext({
 	return context;
 }
 
-export async function call<MiddlewareResult = Response | EndpointOutput>(
+export async function callEndpoint<MiddlewareResult = Response | EndpointOutput>(
 	mod: EndpointHandler,
 	env: Environment,
 	ctx: RenderContext,
@@ -108,26 +108,25 @@ export async function call<MiddlewareResult = Response | EndpointOutput>(
 		adapterName: env.adapterName,
 	});
 
-	let response = await renderEndpoint(mod, context, env.ssr);
+	let response;
 	if (middleware && middleware.onRequest) {
-		if (response.body === null) {
-			const onRequest = middleware.onRequest as MiddlewareEndpointHandler;
-			response = await callMiddleware<Response | EndpointOutput>(onRequest, context, async () => {
+		const onRequest = middleware.onRequest as MiddlewareEndpointHandler;
+		response = await callMiddleware<Response | EndpointOutput>(
+			env.logging,
+			onRequest,
+			context,
+			async () => {
 				if (env.mode === 'development' && !isValueSerializable(context.locals)) {
 					throw new AstroError({
 						...AstroErrorData.LocalsNotSerializable,
 						message: AstroErrorData.LocalsNotSerializable.message(ctx.pathname),
 					});
 				}
-				return response;
-			});
-		} else {
-			warn(
-				env.logging,
-				'middleware',
-				"Middleware doesn't work for endpoints that return a simple body. The middleware will be disabled for this page."
-			);
-		}
+				return await renderEndpoint(mod, context, env.ssr);
+			}
+		);
+	} else {
+		response = await renderEndpoint(mod, context, env.ssr);
 	}
 
 	if (response instanceof Response) {

packages/astro/src/core/middleware/callMiddleware.ts

@@ -1,5 +1,9 @@
 import type { APIContext, MiddlewareHandler, MiddlewareNext } from '../../@types/astro';
 import { AstroError, AstroErrorData } from '../errors/index.js';
+import type { EndpointOutput } from '../../@types/astro';
+import { warn } from '../logger/core.js';
+import type { Environment } from '../render';
+import { bold } from 'kleur/colors';
 
 /**
  * Utility function that is in charge of calling the middleware.
@@ -36,6 +40,7 @@ import { AstroError, AstroErrorData } from '../errors/index.js';
  * @param responseFunction A callback function that should return a promise with the response
  */
 export async function callMiddleware<R>(
+	logging: Environment['logging'],
 	onRequest: MiddlewareHandler<R>,
 	apiContext: APIContext,
 	responseFunction: () => Promise<R>
@@ -56,6 +61,15 @@ export async function callMiddleware<R>(
 	let middlewarePromise = onRequest(apiContext, next);
 
 	return await Promise.resolve(middlewarePromise).then(async (value) => {
+		if (isEndpointOutput(value)) {
+			warn(
+				logging,
+				'middleware',
+				'Using simple endpoints can cause unexpected issues in the chain of middleware functions.' +
+					`\nIt's strongly suggested to use full ${bold('Response')} objects.`
+			);
+		}
+
 		// first we check if `next` was called
 		if (nextCalled) {
 			/**
@@ -99,6 +113,10 @@ export async function callMiddleware<R>(
 	});
 }
 
-function isEndpointResult(response: any): boolean {
-	return response && typeof response.body !== 'undefined';
+function isEndpointOutput(endpointResult: any): endpointResult is EndpointOutput {
+	return (
+		!(endpointResult instanceof Response) &&
+		typeof endpointResult === 'object' &&
+		typeof endpointResult.body === 'string'
+	);
 }

packages/astro/src/core/render/dev/index.ts

@@ -190,7 +190,7 @@ export async function renderPage(options: SSROptions): Promise<Response> {
 			});
 
 			const onRequest = options.middleware.onRequest as MiddlewareResponseHandler;
-			const response = await callMiddleware<Response>(onRequest, apiContext, () => {
+			const response = await callMiddleware<Response>(env.logging, onRequest, apiContext, () => {
 				return coreRenderPage({ mod, renderContext, env: options.env, apiContext });
 			});
 

packages/astro/test/fixtures/middleware-dev/src/middleware.js

@@ -11,6 +11,13 @@ const first = defineMiddleware(async (context, next) => {
 		return new Response(null, {
 			status: 500,
 		});
+	} else if (context.request.url.includes('/api/endpoint')) {
+		const response = await next();
+		const object = await response.json();
+		object.name = 'REDACTED';
+		return new Response(JSON.stringify(object), {
+			headers: response.headers,
+		});
 	} else {
 		context.locals.name = 'bar';
 	}

packages/astro/test/fixtures/middleware-dev/src/pages/api/endpoint.js

@@ -0,0 +1,10 @@
+export function get() {
+	const object = {
+		name: 'Endpoint!!',
+	};
+	return new Response(JSON.stringify(object), {
+		headers: {
+			'Content-Type': 'application/json',
+		},
+	});
+}

packages/astro/test/middleware.test.js

@@ -197,6 +197,22 @@ describe('Middleware API in PROD mode, SSR', () => {
 		const $ = cheerio.load(html);
 		expect($('title').html()).to.not.equal('MiddlewareNoDataReturned');
 	});
+
+	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;
+	});
 });
 
 describe('Middleware with tailwind', () => {