diff --git a/.changeset/long-starfishes-raise.md b/.changeset/long-starfishes-raise.md new file mode 100644 index 000000000..e081aedbe --- /dev/null +++ b/.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`. diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index d7d4241d2..0451b4ed5 100644 --- a/packages/astro/src/core/app/index.ts +++ b/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( + this.#env.logging, onRequest as MiddlewareResponseHandler, apiContext, () => { diff --git a/packages/astro/src/core/build/generate.ts b/packages/astro/src/core/build/generate.ts index 8a85232c4..26e53d367 100644 --- a/packages/astro/src/core/build/generate.ts +++ b/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( + env.logging, onRequest as MiddlewareResponseHandler, apiContext, () => { diff --git a/packages/astro/src/core/endpoint/dev/index.ts b/packages/astro/src/core/endpoint/dev/index.ts index 24d2aa3bd..b82975476 100644 --- a/packages/astro/src/core/endpoint/dev/index.ts +++ b/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 { diff --git a/packages/astro/src/core/endpoint/index.ts b/packages/astro/src/core/endpoint/index.ts index e49ce8a43..42f707601 100644 --- a/packages/astro/src/core/endpoint/index.ts +++ b/packages/astro/src/core/endpoint/index.ts @@ -93,7 +93,7 @@ export function createAPIContext({ return context; } -export async function call( +export async function callEndpoint( mod: EndpointHandler, env: Environment, ctx: RenderContext, @@ -108,26 +108,25 @@ export async function call( 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(onRequest, context, async () => { + const onRequest = middleware.onRequest as MiddlewareEndpointHandler; + response = await callMiddleware( + 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) { diff --git a/packages/astro/src/core/middleware/callMiddleware.ts b/packages/astro/src/core/middleware/callMiddleware.ts index 98220bbb5..1173ef54f 100644 --- a/packages/astro/src/core/middleware/callMiddleware.ts +++ b/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( + logging: Environment['logging'], onRequest: MiddlewareHandler, apiContext: APIContext, responseFunction: () => Promise @@ -56,6 +61,15 @@ export async function callMiddleware( 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( }); } -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' + ); } diff --git a/packages/astro/src/core/render/dev/index.ts b/packages/astro/src/core/render/dev/index.ts index fbbe0d48d..267c6515d 100644 --- a/packages/astro/src/core/render/dev/index.ts +++ b/packages/astro/src/core/render/dev/index.ts @@ -190,7 +190,7 @@ export async function renderPage(options: SSROptions): Promise { }); const onRequest = options.middleware.onRequest as MiddlewareResponseHandler; - const response = await callMiddleware(onRequest, apiContext, () => { + const response = await callMiddleware(env.logging, onRequest, apiContext, () => { return coreRenderPage({ mod, renderContext, env: options.env, apiContext }); }); diff --git a/packages/astro/test/fixtures/middleware-dev/src/middleware.js b/packages/astro/test/fixtures/middleware-dev/src/middleware.js index 89b036543..c8b440239 100644 --- a/packages/astro/test/fixtures/middleware-dev/src/middleware.js +++ b/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'; } diff --git a/packages/astro/test/fixtures/middleware-dev/src/pages/api/endpoint.js b/packages/astro/test/fixtures/middleware-dev/src/pages/api/endpoint.js new file mode 100644 index 000000000..dadff6edb --- /dev/null +++ b/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', + }, + }); +} diff --git a/packages/astro/test/middleware.test.js b/packages/astro/test/middleware.test.js index 7a1d6843e..8e2e0dd0a 100644 --- a/packages/astro/test/middleware.test.js +++ b/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', () => {