From fad482f366b734333dee2219ae6b0a325e91609f Mon Sep 17 00:00:00 2001 From: s1gr1d <32902192+s1gr1d@users.noreply.github.com> Date: Tue, 28 Apr 2026 09:43:25 +0200 Subject: [PATCH 1/5] fix(hono): Use arity heuristic to distinguish .use() middleware from .all() handlers Hono registers both .use() middleware and .all() route handlers with method:'ALL' internally. Previously patchRoute wrapped all method:'ALL' handlers as middleware spans, incorrectly tracing .all() final handlers. Use handler.length >= 2 (accepts context + next) to identify middleware vs handler.length < 2 (accepts only context) for route handlers. Also guard responseHandler against re-capturing errors already captured by wrapMiddlewareWithSpan via isAlreadyCaptured check. Made-with: Cursor --- .../hono/src/shared/middlewareHandlers.ts | 3 +- packages/hono/src/shared/patchRoute.ts | 54 +++++++++++++++---- 2 files changed, 46 insertions(+), 11 deletions(-) diff --git a/packages/hono/src/shared/middlewareHandlers.ts b/packages/hono/src/shared/middlewareHandlers.ts index a470733b47a8..fc9560f5ba2b 100644 --- a/packages/hono/src/shared/middlewareHandlers.ts +++ b/packages/hono/src/shared/middlewareHandlers.ts @@ -4,6 +4,7 @@ import { getDefaultIsolationScope, getIsolationScope, getRootSpan, + isAlreadyCaptured, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, updateSpanName, winterCGRequestToRequestData, @@ -42,7 +43,7 @@ export function responseHandler(context: Context): void { getIsolationScope().setTransactionName(`${context.req.method} ${routePath(context)}`); - if (context.error) { + if (context.error && !isAlreadyCaptured(context.error)) { getClient()?.captureException(context.error, { mechanism: { handled: false, type: 'auto.http.hono.context_error' }, }); diff --git a/packages/hono/src/shared/patchRoute.ts b/packages/hono/src/shared/patchRoute.ts index 6e821d2af64a..f088bd887768 100644 --- a/packages/hono/src/shared/patchRoute.ts +++ b/packages/hono/src/shared/patchRoute.ts @@ -1,8 +1,14 @@ import { getOriginalFunction, markFunctionWrapped } from '@sentry/core'; import type { WrappedFunction } from '@sentry/core'; -import type { Env, Hono } from 'hono'; +import type { Env, Hono, MiddlewareHandler } from 'hono'; import { wrapMiddlewareWithSpan } from './wrapMiddlewareSpan'; +interface HonoRoute { + method: string; + path: string; + handler: MiddlewareHandler; +} + interface HonoBaseProto { // oxlint-disable-next-line typescript/no-explicit-any route: (path: string, app: Hono) => Hono; @@ -29,15 +35,7 @@ export function patchRoute(app: Hono): void { // oxlint-disable-next-line typescript/no-explicit-any const patchedRoute = function (this: Hono, path: string, subApp: Hono): Hono { if (subApp && Array.isArray(subApp.routes)) { - for (const route of subApp.routes) { - /* Internally, `app.use()` always registers with `method: 'ALL'` (via the constant `METHOD_NAME_ALL`), - * while `app.get()` / `.post()` / etc. use their respective uppercase method name. - * https://github.com/honojs/hono/blob/18fe604c8cefc2628240651b1af219692e1918c1/src/hono-base.ts#L156-L168 - */ - if (route.method === 'ALL' && typeof route.handler === 'function') { - route.handler = wrapMiddlewareWithSpan(route.handler); - } - } + wrapSubAppMiddleware(subApp.routes as HonoRoute[]); } return originalRoute.call(this, path, subApp); }; @@ -45,3 +43,39 @@ export function patchRoute(app: Hono): void { markFunctionWrapped(patchedRoute as unknown as WrappedFunction, originalRoute as unknown as WrappedFunction); honoBaseProto.route = patchedRoute; } + +/** + * Wraps middleware handlers in a sub-app's routes array with Sentry spans. + * + * When multiple handlers share the same method+path (e.g. `app.get('/path', mw, handler)`), + * Hono registers each as a separate route entry. We wrap all but the last entry per group + * (those are the middleware), leaving the final handler unwrapped. + * + * For `method: 'ALL'` handlers that are last-for-group (from `.use()` or `.all()`), + * we use an arity (# of params) heuristic: middleware takes `(context, next)` (length >= 2), + * while final handlers take only `(context)` (length < 2). This distinguishes + * `.use()` middleware (should be traced) from `.all()` route handlers (should not). + * + * Hono's .use() and .all() both register as method 'ALL', but .use() middleware + * always accepts (context, next) while .all() handlers typically accept only (context). + * https://github.com/honojs/hono/blob/18fe604c8cefc2628240651b1af219692e1918c1/src/hono-base.ts#L156-L168 + */ +function wrapSubAppMiddleware(routes: HonoRoute[]): void { + const lastIndexByKey = new Map(); + for (const [i, route] of routes.entries()) { + lastIndexByKey.set(`${route.method}\0${route.path}`, i); + } + + for (const [i, route] of routes.entries()) { + if (typeof route.handler !== 'function') { + continue; + } + + const isLastForGroup = lastIndexByKey.get(`${route.method}\0${route.path}`) === i; + + const isMiddleware = !isLastForGroup || (route.method === 'ALL' && route.handler.length >= 2); + if (isMiddleware) { + route.handler = wrapMiddlewareWithSpan(route.handler); + } + } +} From b92fc69a899a11160e8986f2b1459c8e9e8b7318 Mon Sep 17 00:00:00 2001 From: s1gr1d <32902192+s1gr1d@users.noreply.github.com> Date: Tue, 28 Apr 2026 09:43:40 +0200 Subject: [PATCH 2/5] test(hono): Add E2E and unit tests for middleware tracing and route patterns Add comprehensive test coverage for the Hono middleware instrumentation: - E2E route-patterns tests for HTTP methods, registration styles, error capture - E2E inline middleware tests for direct method, .all(), .on() with inline and separately registered middleware across all HTTP methods - E2E negative test asserting .all() handlers don't create middleware spans - Unit tests for responseHandler error capture and isAlreadyCaptured guard - Unit tests for patchRoute arity heuristic (inline mw, separate mw, .on()) - Unit tests for sentry middleware without options (external init pattern) Made-with: Cursor --- .../src/route-groups/test-middleware.ts | 59 ++++++- .../src/route-groups/test-route-patterns.ts | 55 +++++++ .../test-applications/hono-4/src/routes.ts | 9 +- .../hono-4/tests/middleware.test.ts | 70 ++++++--- .../hono-4/tests/route-patterns.test.ts | 144 ++++++++++++++++++ packages/hono/test/node/middleware.test.ts | 89 ++++++++++- .../test/shared/middlewareHandlers.test.ts | 116 ++++++++++++++ packages/hono/test/shared/patchAppUse.test.ts | 88 ++++++++++- 8 files changed, 600 insertions(+), 30 deletions(-) create mode 100644 dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-route-patterns.ts create mode 100644 dev-packages/e2e-tests/test-applications/hono-4/tests/route-patterns.test.ts create mode 100644 packages/hono/test/shared/middlewareHandlers.test.ts diff --git a/dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-middleware.ts b/dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-middleware.ts index 23004976ef08..49ca50c591bf 100644 --- a/dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-middleware.ts +++ b/dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-middleware.ts @@ -8,7 +8,7 @@ middlewareRoutes.get('/anonymous', c => c.json({ middleware: 'anonymous' })); middlewareRoutes.get('/multi', c => c.json({ middleware: 'multi' })); middlewareRoutes.get('/error', c => c.text('should not reach')); -// Self-contained sub-app registering its own middleware +// Self-contained sub-app registering its own middleware via .use() const subAppWithMiddleware = new Hono(); subAppWithMiddleware.use('/named/*', middlewareA); @@ -19,12 +19,63 @@ subAppWithMiddleware.use('/anonymous/*', async (c, next) => { subAppWithMiddleware.use('/multi/*', middlewareA, middlewareB); subAppWithMiddleware.use('/error/*', failingMiddleware); -// .all() produces the same method:'ALL' as .use() in Hono's route record. -// Wrapping it is harmless (onlyIfParent:true) — this route exists to prove that. +// .all() handler (1 parameter) — should NOT be wrapped as middleware by patchRoute. subAppWithMiddleware.all('/all-handler', async function allCatchAll(c) { return c.json({ handler: 'all' }); }); subAppWithMiddleware.route('/', middlewareRoutes); -export { middlewareRoutes, subAppWithMiddleware }; +// Sub-app with inline middleware for different registration styles. +// patchRoute wraps non-last handlers per method+path group as middleware. +const subAppWithInlineMiddleware = new Hono(); + +const METHODS = ['get', 'post', 'put', 'delete', 'patch'] as const; + +// Direct method registration for each HTTP method +METHODS.forEach(method => { + subAppWithInlineMiddleware[method]( + '/direct', + async function inlineMiddleware(_c, next) { + await next(); + }, + c => c.text(`${method} direct response`), + ); + + subAppWithInlineMiddleware[method]('/direct/separately', async function inlineSeparateMiddleware(_c, next) { + await next(); + }); + subAppWithInlineMiddleware[method]('/direct/separately', c => c.text(`${method} direct separate response`)); +}); + +// .all(): .all('/path', mw, handler) +subAppWithInlineMiddleware.all( + '/all', + async function inlineMiddlewareAll(_c, next) { + await next(); + }, + c => c.text('all response'), +); +subAppWithInlineMiddleware.all('/all/separately', async function inlineSeparateMiddlewareAll(_c, next) { + await next(); +}); +subAppWithInlineMiddleware.all('/all/separately', c => c.text('all separate response')); + +// .on() registration for each HTTP method +METHODS.forEach(method => { + subAppWithInlineMiddleware.on( + method, + '/on', + async function inlineMiddlewareOn(_c, next) { + await next(); + }, + c => c.text(`${method} on response`), + ); + + subAppWithInlineMiddleware.on(method, '/on/separately', async function inlineSeparateMiddlewareOn(_c, next) { + await next(); + }); + subAppWithInlineMiddleware.on(method, '/on/separately', c => c.text(`${method} on separate response`)); +}); + +export { middlewareRoutes, subAppWithMiddleware, subAppWithInlineMiddleware }; diff --git a/dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-route-patterns.ts b/dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-route-patterns.ts new file mode 100644 index 000000000000..e32662fb3b18 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/hono-4/src/route-groups/test-route-patterns.ts @@ -0,0 +1,55 @@ +import { Hono } from 'hono'; +import { HTTPException } from 'hono/http-exception'; + +const routePatterns = new Hono(); + +const METHODS = ['get', 'post', 'put', 'delete', 'patch'] as const; + +// Direct method registration for each HTTP method (sync handlers) +METHODS.forEach(method => { + routePatterns[method]('/', c => c.text(`${method} response`)); +}); + +// Async handler +routePatterns.get('/async', async c => { + await new Promise(resolve => setTimeout(resolve, 10)); + return c.text('async response'); +}); + +// .all() registration +routePatterns.all('/all', c => c.text('all handler response')); + +// .on() registration +METHODS.forEach(method => { + routePatterns.on(method, '/on', c => c.text(`${method} on response`)); +}); + +// Error routes for direct method registration +METHODS.forEach(method => { + routePatterns[method]('/500', () => { + throw new HTTPException(500, { message: 'response 500' }); + }); + routePatterns[method]('/401', () => { + throw new HTTPException(401, { message: 'response 401' }); + }); + routePatterns[method]('/402', () => { + throw new HTTPException(402, { message: 'response 402' }); + }); + routePatterns[method]('/403', () => { + throw new HTTPException(403, { message: 'response 403' }); + }); +}); + +// Error routes for .all() +routePatterns.all('/all/500', () => { + throw new HTTPException(500, { message: 'response 500' }); +}); + +// Error routes for .on() +METHODS.forEach(method => { + routePatterns.on(method, '/on/500', () => { + throw new HTTPException(500, { message: 'response 500' }); + }); +}); + +export { routePatterns }; diff --git a/dev-packages/e2e-tests/test-applications/hono-4/src/routes.ts b/dev-packages/e2e-tests/test-applications/hono-4/src/routes.ts index 0ff66a589b72..f6efc6dde03c 100644 --- a/dev-packages/e2e-tests/test-applications/hono-4/src/routes.ts +++ b/dev-packages/e2e-tests/test-applications/hono-4/src/routes.ts @@ -1,7 +1,8 @@ import type { Hono } from 'hono'; import { HTTPException } from 'hono/http-exception'; import { failingMiddleware, middlewareA, middlewareB } from './middleware'; -import { middlewareRoutes, subAppWithMiddleware } from './route-groups/test-middleware'; +import { middlewareRoutes, subAppWithInlineMiddleware, subAppWithMiddleware } from './route-groups/test-middleware'; +import { routePatterns } from './route-groups/test-route-patterns'; export function addRoutes(app: Hono<{ Bindings?: { E2E_TEST_DSN: string } }>): void { app.get('/', c => { @@ -36,4 +37,10 @@ export function addRoutes(app: Hono<{ Bindings?: { E2E_TEST_DSN: string } }>): v // Sub-app middleware: registered on the sub-app, wrapped at mount time by route() patching app.route('/test-subapp-middleware', subAppWithMiddleware); + + // Inline middleware patterns: direct method, .all(), .on() with inline/separate middleware + app.route('/test-inline-middleware', subAppWithInlineMiddleware); + + // Route patterns: HTTP methods, .all(), .on(), sync/async, errors + app.route('/test-routes', routePatterns); } diff --git a/dev-packages/e2e-tests/test-applications/hono-4/tests/middleware.test.ts b/dev-packages/e2e-tests/test-applications/hono-4/tests/middleware.test.ts index e3b4556bfd18..8803fe9b3e31 100644 --- a/dev-packages/e2e-tests/test-applications/hono-4/tests/middleware.test.ts +++ b/dev-packages/e2e-tests/test-applications/hono-4/tests/middleware.test.ts @@ -97,10 +97,7 @@ for (const { name, prefix } of SCENARIOS) { test('captures error thrown in middleware', async ({ baseURL }) => { const errorPromise = waitForError(APP_NAME, event => { - return ( - event.exception?.values?.[0]?.value === 'Middleware error' && - event.exception?.values?.[0]?.mechanism?.type === 'auto.middleware.hono' - ); + return event.exception?.values?.[0]?.value === 'Middleware error'; }); const response = await fetch(`${baseURL}${prefix}/error`); @@ -152,8 +149,8 @@ for (const { name, prefix } of SCENARIOS) { }); } -test.describe('patchRoute wraps .all() as middleware span (in sub-app)', () => { - test('patchRoute wraps .all() as middleware span', async ({ baseURL }) => { +test.describe('.all() handler in sub-app', () => { + test('does not create middleware span for .all() route handler', async ({ baseURL }) => { const transactionPromise = waitForTransaction(APP_NAME, event => { return ( event.contexts?.trace?.op === 'http.server' && event.transaction === 'GET /test-subapp-middleware/all-handler' @@ -169,20 +166,57 @@ test.describe('patchRoute wraps .all() as middleware span (in sub-app)', () => { const transaction = await transactionPromise; const spans = transaction.spans || []; - // On Bun/Cloudflare, patchRoute is the sole wrapper and sees the original - // function name. It wraps .all() handlers identically to .use() middleware - // because both produce method:'ALL' in Hono's route record. const allHandlerSpan = spans.find( (span: SpanJSON) => span.op === 'middleware.hono' && span.description === 'allCatchAll', ); - - expect(allHandlerSpan).toEqual( - expect.objectContaining({ - description: 'allCatchAll', - op: 'middleware.hono', - origin: 'auto.middleware.hono', - status: 'ok', - }), - ); + expect(allHandlerSpan).toBeUndefined(); }); }); + +const INLINE_PREFIX = '/test-inline-middleware'; + +const REGISTRATION_STYLES = [ + { name: 'direct method (.get())', path: '/direct' }, + { name: '.all()', path: '/all' }, + { name: '.on()', path: '/on' }, +] as const; + +const MIDDLEWARE_STYLES = [ + { name: 'inline', path: '' }, + { name: 'separately registered', path: '/separately' }, +] as const; + +test.describe('inline middleware spans (sub-app)', () => { + for (const { name: regName, path: regPath } of REGISTRATION_STYLES) { + for (const { name: mwName, path: mwPath } of MIDDLEWARE_STYLES) { + test(`creates middleware span for ${mwName} middleware via ${regName}`, async ({ baseURL }) => { + const fullPath = `${INLINE_PREFIX}${regPath}${mwPath}`; + + const transactionPromise = waitForTransaction(APP_NAME, event => { + return event.contexts?.trace?.op === 'http.server' && event.transaction === `GET ${fullPath}`; + }); + + const response = await fetch(`${baseURL}${fullPath}`); + expect(response.status).toBe(200); + + const transaction = await transactionPromise; + + const EXPECTED_DESCRIPTIONS: Record> = { + '/direct': { '': 'inlineMiddleware', '/separately': 'inlineSeparateMiddleware' }, + '/all': { '': 'inlineMiddlewareAll', '/separately': 'inlineSeparateMiddlewareAll' }, + '/on': { '': 'inlineMiddlewareOn', '/separately': 'inlineSeparateMiddlewareOn' }, + }; + const expectedDescription = EXPECTED_DESCRIPTIONS[regPath]![mwPath]!; + + expect(transaction.spans).toContainEqual( + expect.objectContaining({ + description: expectedDescription, + op: 'middleware.hono', + origin: 'auto.middleware.hono', + status: 'ok', + }), + ); + }); + } + } +}); diff --git a/dev-packages/e2e-tests/test-applications/hono-4/tests/route-patterns.test.ts b/dev-packages/e2e-tests/test-applications/hono-4/tests/route-patterns.test.ts new file mode 100644 index 000000000000..fd6579fe3b17 --- /dev/null +++ b/dev-packages/e2e-tests/test-applications/hono-4/tests/route-patterns.test.ts @@ -0,0 +1,144 @@ +import { expect, test } from '@playwright/test'; +import { waitForError, waitForTransaction } from '@sentry-internal/test-utils'; +import { APP_NAME } from './constants'; + +const PREFIX = '/test-routes'; + +const REGISTRATION_STYLES = [ + { name: 'direct method', path: '' }, + { name: '.all()', path: '/all' }, + { name: '.on()', path: '/on' }, +] as const; + +test.describe('HTTP methods', () => { + for (const method of ['POST', 'PUT', 'DELETE', 'PATCH']) { + test(`sends transaction for ${method}`, async ({ baseURL }) => { + const transactionPromise = waitForTransaction(APP_NAME, event => { + return event.contexts?.trace?.op === 'http.server' && event.transaction === `${method} ${PREFIX}`; + }); + + const response = await fetch(`${baseURL}${PREFIX}`, { method }); + expect(response.status).toBe(200); + + const transaction = await transactionPromise; + expect(transaction.contexts?.trace?.op).toBe('http.server'); + expect(transaction.transaction).toBe(`${method} ${PREFIX}`); + }); + } +}); + +test.describe('route registration styles', () => { + for (const { name, path } of REGISTRATION_STYLES) { + test(`${name} sends transaction`, async ({ baseURL }) => { + const transactionPromise = waitForTransaction(APP_NAME, event => { + return event.contexts?.trace?.op === 'http.server' && event.transaction === `GET ${PREFIX}${path}`; + }); + + const response = await fetch(`${baseURL}${PREFIX}${path}`); + expect(response.status).toBe(200); + + const transaction = await transactionPromise; + expect(transaction.contexts?.trace?.op).toBe('http.server'); + expect(transaction.transaction).toBe(`GET ${PREFIX}${path}`); + }); + } + + for (const { name, path } of [ + { name: '.all()', path: '/all' }, + { name: '.on()', path: '/on' }, + ]) { + test(`${name} responds to POST`, async ({ baseURL }) => { + const transactionPromise = waitForTransaction(APP_NAME, event => { + return event.contexts?.trace?.op === 'http.server' && event.transaction === `POST ${PREFIX}${path}`; + }); + + const response = await fetch(`${baseURL}${PREFIX}${path}`, { method: 'POST' }); + expect(response.status).toBe(200); + + const transaction = await transactionPromise; + expect(transaction.transaction).toBe(`POST ${PREFIX}${path}`); + }); + } +}); + +test('async handler sends transaction', async ({ baseURL }) => { + const transactionPromise = waitForTransaction(APP_NAME, event => { + return event.contexts?.trace?.op === 'http.server' && event.transaction === `GET ${PREFIX}/async`; + }); + + const response = await fetch(`${baseURL}${PREFIX}/async`); + expect(response.status).toBe(200); + + const transaction = await transactionPromise; + expect(transaction.contexts?.trace?.op).toBe('http.server'); +}); + +test.describe('500 HTTPException capture', () => { + for (const { name, path } of REGISTRATION_STYLES) { + test(`captures 500 from ${name} route with correct mechanism`, async ({ baseURL }) => { + const fullPath = `${PREFIX}${path}/500`; + + const errorPromise = waitForError(APP_NAME, event => { + return event.exception?.values?.[0]?.value === 'response 500' && !!event.request?.url?.includes(fullPath); + }); + + const response = await fetch(`${baseURL}${fullPath}`); + expect(response.status).toBe(500); + + const errorEvent = await errorPromise; + expect(errorEvent.exception?.values?.[0]?.value).toBe('response 500'); + expect(errorEvent.exception?.values?.[0]?.mechanism).toEqual( + expect.objectContaining({ + handled: false, + type: 'auto.http.hono.context_error', + }), + ); + }); + } + + test('captures 500 error with POST method', async ({ baseURL }) => { + const errorPromise = waitForError(APP_NAME, event => { + return ( + event.exception?.values?.[0]?.value === 'response 500' && + !!event.request?.url?.includes(`${PREFIX}/500`) && + event.request?.method === 'POST' + ); + }); + + const response = await fetch(`${baseURL}${PREFIX}/500`, { method: 'POST' }); + expect(response.status).toBe(500); + + const errorEvent = await errorPromise; + expect(errorEvent.exception?.values?.[0]?.value).toBe('response 500'); + expect(errorEvent.exception?.values?.[0]?.mechanism).toEqual( + expect.objectContaining({ + handled: false, + type: 'auto.http.hono.context_error', + }), + ); + }); +}); + +test.describe('4xx HTTPException capture', () => { + for (const code of [401, 402, 403]) { + test(`captures ${code} HTTPException`, async ({ baseURL }) => { + const fullPath = `${PREFIX}/${code}`; + + const errorPromise = waitForError(APP_NAME, event => { + return event.exception?.values?.[0]?.value === `response ${code}` && !!event.request?.url?.includes(fullPath); + }); + + const response = await fetch(`${baseURL}${fullPath}`); + expect(response.status).toBe(code); + + const errorEvent = await errorPromise; + expect(errorEvent.exception?.values?.[0]?.value).toBe(`response ${code}`); + expect(errorEvent.exception?.values?.[0]?.mechanism).toEqual( + expect.objectContaining({ + handled: false, + type: 'auto.http.hono.context_error', + }), + ); + }); + } +}); diff --git a/packages/hono/test/node/middleware.test.ts b/packages/hono/test/node/middleware.test.ts index 546350fd8377..745e924e7804 100644 --- a/packages/hono/test/node/middleware.test.ts +++ b/packages/hono/test/node/middleware.test.ts @@ -1,6 +1,6 @@ import * as SentryCore from '@sentry/core'; import { Hono } from 'hono'; -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { beforeEach, describe, expect, it, type Mock, vi } from 'vitest'; import { sentry } from '../../src/node/middleware'; import { init } from '../../src/node/sdk'; @@ -11,6 +11,18 @@ vi.mock('@sentry/node', () => ({ // eslint-disable-next-line @typescript-eslint/consistent-type-imports const { init: initNodeMock } = await vi.importMock('@sentry/node'); +vi.mock('@sentry/core', async () => { + const actual = await vi.importActual('@sentry/core'); + return { + ...actual, + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore + applySdkMetadata: vi.fn(actual.applySdkMetadata), + }; +}); + +const applySdkMetadataMock = SentryCore.applySdkMetadata as Mock; + describe('Hono Node Middleware', () => { beforeEach(() => { vi.clearAllMocks(); @@ -87,4 +99,79 @@ describe('Hono Node Middleware', () => { getClientSpy.mockRestore(); }); }); + + describe('sentry middleware without options (external init)', () => { + it('does not call init when no options are provided', () => { + const app = new Hono(); + sentry(app); + + expect(initNodeMock).not.toHaveBeenCalled(); + expect(applySdkMetadataMock).not.toHaveBeenCalled(); + }); + + it('returns a middleware handler function', () => { + const app = new Hono(); + const middleware = sentry(app); + + expect(middleware).toBeDefined(); + expect(typeof middleware).toBe('function'); + expect(middleware).toHaveLength(2); + }); + + it('returns an async middleware handler', () => { + const app = new Hono(); + const middleware = sentry(app); + + expect(middleware.constructor.name).toBe('AsyncFunction'); + }); + + it('emits a warning when Sentry is not initialized', () => { + const warnSpy = vi.spyOn(SentryCore.debug, 'warn'); + vi.spyOn(SentryCore, 'getClient').mockReturnValue(undefined); + + const app = new Hono(); + sentry(app); + + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('Sentry is not initialized')); + }); + + it('does not emit a warning when Sentry is already initialized', () => { + const warnSpy = vi.spyOn(SentryCore.debug, 'warn'); + const fakeClient = { getOptions: () => ({ debug: false }) }; + vi.spyOn(SentryCore, 'getClient').mockReturnValue(fakeClient as unknown as SentryCore.Client); + + const app = new Hono(); + const middleware = sentry(app); + + expect(warnSpy).not.toHaveBeenCalled(); + expect(middleware.constructor.name).toBe('AsyncFunction'); + }); + }); + + describe('double-init guard', () => { + it('skips re-initialization when a client already exists', () => { + const fakeClient = { getOptions: () => ({}) }; + const getClientSpy = vi + .spyOn(SentryCore, 'getClient') + .mockReturnValue(fakeClient as unknown as SentryCore.Client); + + const result = init({ dsn: 'https://public@dsn.ingest.sentry.io/1337' }); + + expect(result).toBe(fakeClient); + expect(initNodeMock).not.toHaveBeenCalled(); + expect(applySdkMetadataMock).not.toHaveBeenCalled(); + + getClientSpy.mockRestore(); + }); + + it('initializes normally when no client exists yet', () => { + const getClientSpy = vi.spyOn(SentryCore, 'getClient').mockReturnValue(undefined); + + init({ dsn: 'https://public@dsn.ingest.sentry.io/1337' }); + + expect(initNodeMock).toHaveBeenCalledTimes(1); + + getClientSpy.mockRestore(); + }); + }); }); diff --git a/packages/hono/test/shared/middlewareHandlers.test.ts b/packages/hono/test/shared/middlewareHandlers.test.ts new file mode 100644 index 000000000000..4708054855fb --- /dev/null +++ b/packages/hono/test/shared/middlewareHandlers.test.ts @@ -0,0 +1,116 @@ +import * as SentryCore from '@sentry/core'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { responseHandler } from '../../src/shared/middlewareHandlers'; + +vi.mock('hono/route', () => ({ + routePath: () => '/test', +})); + +vi.mock('../../src/utils/hono-context', () => ({ + hasFetchEvent: () => false, +})); + +const mockSetTransactionName = vi.fn(); + +vi.mock('@sentry/core', async () => { + const actual = await vi.importActual('@sentry/core'); + return { + ...actual, + getActiveSpan: vi.fn(() => null), + getIsolationScope: vi.fn(() => ({ + setTransactionName: mockSetTransactionName, + })), + getClient: vi.fn(() => undefined), + }; +}); + +const getClientMock = SentryCore.getClient as ReturnType; + +function createMockContext(status: number, error?: Error): unknown { + return { + req: { method: 'GET', raw: new Request('http://localhost/test') }, + res: { status }, + error, + }; +} + +describe('responseHandler', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe('error capture', () => { + it('captures error when context.error is set', () => { + const mockCaptureException = vi.fn(); + getClientMock.mockReturnValue({ + captureException: mockCaptureException, + }); + + const error = new Error('server error'); + // oxlint-disable-next-line typescript/no-explicit-any + responseHandler(createMockContext(500, error) as any); + + expect(mockCaptureException).toHaveBeenCalledWith(error, { + mechanism: { handled: false, type: 'auto.http.hono.context_error' }, + }); + }); + + it('captures error regardless of status code', () => { + const mockCaptureException = vi.fn(); + getClientMock.mockReturnValue({ + captureException: mockCaptureException, + }); + + const error = new Error('not found'); + // oxlint-disable-next-line typescript/no-explicit-any + responseHandler(createMockContext(404, error) as any); + + expect(mockCaptureException).toHaveBeenCalledWith(error, { + mechanism: { handled: false, type: 'auto.http.hono.context_error' }, + }); + }); + + it('does not call captureException when there is no error', () => { + const mockCaptureException = vi.fn(); + getClientMock.mockReturnValue({ + captureException: mockCaptureException, + }); + + // oxlint-disable-next-line typescript/no-explicit-any + responseHandler(createMockContext(200) as any); + + expect(mockCaptureException).not.toHaveBeenCalled(); + }); + + it('does not throw when client is undefined', () => { + getClientMock.mockReturnValue(undefined); + + // oxlint-disable-next-line typescript/no-explicit-any + expect(() => responseHandler(createMockContext(500, new Error('boom')) as any)).not.toThrow(); + }); + + it('does not re-capture errors already captured by wrapMiddlewareWithSpan', () => { + const mockCaptureException = vi.fn(); + getClientMock.mockReturnValue({ + captureException: mockCaptureException, + }); + + const error = new Error('already captured'); + Object.defineProperty(error, '__sentry_captured__', { value: true, writable: false }); + + // oxlint-disable-next-line typescript/no-explicit-any + responseHandler(createMockContext(500, error) as any); + + expect(mockCaptureException).not.toHaveBeenCalled(); + }); + }); + + describe('transaction name', () => { + it('sets transaction name on isolation scope', () => { + // oxlint-disable-next-line typescript/no-explicit-any + responseHandler(createMockContext(200) as any); + + expect(mockSetTransactionName).toHaveBeenCalledWith('GET /test'); + }); + }); +}); diff --git a/packages/hono/test/shared/patchAppUse.test.ts b/packages/hono/test/shared/patchAppUse.test.ts index 84dd510113e1..ee376127baaa 100644 --- a/packages/hono/test/shared/patchAppUse.test.ts +++ b/packages/hono/test/shared/patchAppUse.test.ts @@ -187,7 +187,7 @@ describe('patchAppUse (middleware spans)', () => { expect(startInactiveSpanMock).toHaveBeenCalledWith(expect.objectContaining({ name: 'subMiddleware' })); }); - it('does not wrap route handlers (only method ALL from use())', async () => { + it('does not wrap sole route handlers on sub-apps', async () => { const app = new Hono(); patchAppUse(app); patchRoute(app); @@ -260,7 +260,7 @@ describe('patchAppUse (middleware spans)', () => { expect(startInactiveSpanMock).toHaveBeenCalledWith(expect.objectContaining({ name: 'adminAuth' })); }); - it('also wraps .all() handlers on sub-apps (same method: ALL in route record)', async () => { + it('does not wrap .all() handlers with less than 2 params (they are route handlers, not middleware)', async () => { const app = new Hono(); patchAppUse(app); patchRoute(app); @@ -273,10 +273,10 @@ describe('patchAppUse (middleware spans)', () => { app.route('/api', subApp); await app.fetch(new Request('http://localhost/api/catch-all')); - expect(startInactiveSpanMock).toHaveBeenCalledWith(expect.objectContaining({ name: 'allHandler' })); + expect(startInactiveSpanMock).not.toHaveBeenCalled(); }); - it('wraps mixed .use() and .all() handlers on the same sub-app', async () => { + it('wraps .use() middleware but not .all() handlers on the same sub-app', async () => { const app = new Hono(); patchAppUse(app); patchRoute(app); @@ -295,10 +295,10 @@ describe('patchAppUse (middleware spans)', () => { const spanNames = startInactiveSpanMock.mock.calls.map((c: unknown[]) => (c[0] as { name: string }).name); expect(spanNames).toContain('mw'); - expect(spanNames).toContain('allRoute'); + expect(spanNames).not.toContain('allRoute'); }); - it('does not wrap .get()/.post()/.put()/.delete() handlers on sub-apps', async () => { + it('does not wrap sole .get()/.post()/.put()/.delete() handlers on sub-apps (they are final handlers, not middleware)', async () => { const app = new Hono(); patchAppUse(app); patchRoute(app); @@ -310,6 +310,12 @@ describe('patchAppUse (middleware spans)', () => { subApp.post('/resource', async function postHandler() { return new Response('post'); }); + subApp.put('/resource', async function postHandler() { + return new Response('put'); + }); + subApp.delete('/resource', async function postHandler() { + return new Response('delete'); + }); app.route('/api', subApp); await app.fetch(new Request('http://localhost/api/resource')); @@ -317,6 +323,76 @@ describe('patchAppUse (middleware spans)', () => { expect(startInactiveSpanMock).not.toHaveBeenCalled(); }); + it('wraps inline middleware in .get(path, mw, handler) on sub-apps', async () => { + const app = new Hono(); + patchAppUse(app); + patchRoute(app); + + const subApp = new Hono(); + subApp.get( + '/resource', + async function inlineMw(_c: unknown, next: () => Promise) { + await next(); + }, + async function getHandler() { + return new Response('get'); + }, + ); + + app.route('/api', subApp); + await app.fetch(new Request('http://localhost/api/resource')); + + const spanNames = startInactiveSpanMock.mock.calls.map((c: unknown[]) => (c[0] as { name: string }).name); + expect(spanNames).toContain('inlineMw'); + expect(spanNames).not.toContain('getHandler'); + }); + + it('wraps separately registered middleware for .get() on sub-apps', async () => { + const app = new Hono(); + patchAppUse(app); + patchRoute(app); + + const subApp = new Hono(); + subApp.get('/resource', async function separateMw(_c: unknown, next: () => Promise) { + await next(); + }); + subApp.get('/resource', async function getHandler() { + return new Response('get'); + }); + + app.route('/api', subApp); + await app.fetch(new Request('http://localhost/api/resource')); + + const spanNames = startInactiveSpanMock.mock.calls.map((c: unknown[]) => (c[0] as { name: string }).name); + expect(spanNames).toContain('separateMw'); + expect(spanNames).not.toContain('getHandler'); + }); + + it('wraps inline middleware registered via .on() on sub-apps', async () => { + const app = new Hono(); + patchAppUse(app); + patchRoute(app); + + const subApp = new Hono(); + subApp.on( + 'GET', + '/resource', + async function onMw(_c: unknown, next: () => Promise) { + await next(); + }, + async function onHandler() { + return new Response('on'); + }, + ); + + app.route('/api', subApp); + await app.fetch(new Request('http://localhost/api/resource')); + + const spanNames = startInactiveSpanMock.mock.calls.map((c: unknown[]) => (c[0] as { name: string }).name); + expect(spanNames).toContain('onMw'); + expect(spanNames).not.toContain('onHandler'); + }); + it('wraps middleware in nested sub-apps (sub-app mounting another sub-app)', async () => { const app = new Hono(); patchAppUse(app); From fbd0b0b96243afe38ac9fee689b0ec6ec0a39f5e Mon Sep 17 00:00:00 2001 From: s1gr1d <32902192+s1gr1d@users.noreply.github.com> Date: Tue, 28 Apr 2026 10:34:03 +0200 Subject: [PATCH 3/5] increase size limits --- .../test-applications/hono-4/tests/middleware.test.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/hono-4/tests/middleware.test.ts b/dev-packages/e2e-tests/test-applications/hono-4/tests/middleware.test.ts index 8803fe9b3e31..e8431bed67ce 100644 --- a/dev-packages/e2e-tests/test-applications/hono-4/tests/middleware.test.ts +++ b/dev-packages/e2e-tests/test-applications/hono-4/tests/middleware.test.ts @@ -166,10 +166,8 @@ test.describe('.all() handler in sub-app', () => { const transaction = await transactionPromise; const spans = transaction.spans || []; - const allHandlerSpan = spans.find( - (span: SpanJSON) => span.op === 'middleware.hono' && span.description === 'allCatchAll', - ); - expect(allHandlerSpan).toBeUndefined(); + // No middleware is called for this route, so there should be no spans. + expect(spans).toEqual([]); }); }); From 011bfd60f2ba85ed05adf73dfe87083d52b86225 Mon Sep 17 00:00:00 2001 From: s1gr1d <32902192+s1gr1d@users.noreply.github.com> Date: Wed, 29 Apr 2026 10:57:55 +0200 Subject: [PATCH 4/5] remove `alreadyCaptured`, add tests --- .../hono/src/shared/middlewareHandlers.ts | 3 +- packages/hono/src/shared/patchRoute.ts | 3 +- .../test/shared/middlewareHandlers.test.ts | 7 +- packages/hono/test/shared/patchRoute.test.ts | 141 ++++++++++++++++++ 4 files changed, 149 insertions(+), 5 deletions(-) create mode 100644 packages/hono/test/shared/patchRoute.test.ts diff --git a/packages/hono/src/shared/middlewareHandlers.ts b/packages/hono/src/shared/middlewareHandlers.ts index fc9560f5ba2b..a470733b47a8 100644 --- a/packages/hono/src/shared/middlewareHandlers.ts +++ b/packages/hono/src/shared/middlewareHandlers.ts @@ -4,7 +4,6 @@ import { getDefaultIsolationScope, getIsolationScope, getRootSpan, - isAlreadyCaptured, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, updateSpanName, winterCGRequestToRequestData, @@ -43,7 +42,7 @@ export function responseHandler(context: Context): void { getIsolationScope().setTransactionName(`${context.req.method} ${routePath(context)}`); - if (context.error && !isAlreadyCaptured(context.error)) { + if (context.error) { getClient()?.captureException(context.error, { mechanism: { handled: false, type: 'auto.http.hono.context_error' }, }); diff --git a/packages/hono/src/shared/patchRoute.ts b/packages/hono/src/shared/patchRoute.ts index f088bd887768..9807f4044d06 100644 --- a/packages/hono/src/shared/patchRoute.ts +++ b/packages/hono/src/shared/patchRoute.ts @@ -60,9 +60,10 @@ export function patchRoute(app: Hono): void { * always accepts (context, next) while .all() handlers typically accept only (context). * https://github.com/honojs/hono/blob/18fe604c8cefc2628240651b1af219692e1918c1/src/hono-base.ts#L156-L168 */ -function wrapSubAppMiddleware(routes: HonoRoute[]): void { +export function wrapSubAppMiddleware(routes: HonoRoute[]): void { const lastIndexByKey = new Map(); for (const [i, route] of routes.entries()) { + // \0 (null byte) is a collision-free delimiter: it cannot appear in a valid HTTP method name or URL path lastIndexByKey.set(`${route.method}\0${route.path}`, i); } diff --git a/packages/hono/test/shared/middlewareHandlers.test.ts b/packages/hono/test/shared/middlewareHandlers.test.ts index 4708054855fb..83099370320c 100644 --- a/packages/hono/test/shared/middlewareHandlers.test.ts +++ b/packages/hono/test/shared/middlewareHandlers.test.ts @@ -89,7 +89,7 @@ describe('responseHandler', () => { expect(() => responseHandler(createMockContext(500, new Error('boom')) as any)).not.toThrow(); }); - it('does not re-capture errors already captured by wrapMiddlewareWithSpan', () => { + it('delegates deduplication to captureException — calls it even for errors with __sentry_captured__', () => { const mockCaptureException = vi.fn(); getClientMock.mockReturnValue({ captureException: mockCaptureException, @@ -101,7 +101,10 @@ describe('responseHandler', () => { // oxlint-disable-next-line typescript/no-explicit-any responseHandler(createMockContext(500, error) as any); - expect(mockCaptureException).not.toHaveBeenCalled(); + // captureException is called — it handles deduplication internally via checkOrSetAlreadyCaught + expect(mockCaptureException).toHaveBeenCalledWith(error, { + mechanism: { handled: false, type: 'auto.http.hono.context_error' }, + }); }); }); diff --git a/packages/hono/test/shared/patchRoute.test.ts b/packages/hono/test/shared/patchRoute.test.ts new file mode 100644 index 000000000000..d9dd4d6795ad --- /dev/null +++ b/packages/hono/test/shared/patchRoute.test.ts @@ -0,0 +1,141 @@ +import * as SentryCore from '@sentry/core'; +import { Hono } from 'hono'; +import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'; +import { patchRoute } from '../../src/shared/patchRoute'; + +vi.mock('@sentry/core', async () => { + const actual = await vi.importActual('@sentry/core'); + return { + ...actual, + startInactiveSpan: vi.fn((_opts: unknown) => ({ + setStatus: vi.fn(), + end: vi.fn(), + })), + }; +}); + +const startInactiveSpanMock = SentryCore.startInactiveSpan as ReturnType; + +const honoBaseProto = Object.getPrototypeOf(Object.getPrototypeOf(new Hono())); +const originalRoute = honoBaseProto.route; + +describe('patchRoute', () => { + beforeEach(() => { + vi.clearAllMocks(); + honoBaseProto.route = originalRoute; + }); + + afterAll(() => { + honoBaseProto.route = originalRoute; + }); + + it('is a no-op when honoBaseProto.route is not a function', () => { + const fakeApp = Object.create({ notRoute: () => {} }) as Hono; + // Should not throw even when the expected method shape is missing + expect(() => patchRoute(fakeApp)).not.toThrow(); + expect(honoBaseProto.route).toBe(originalRoute); + }); + + describe('wrapSubAppMiddleware', () => { + it('does nothing when a sub-app has an empty routes array', async () => { + const app = new Hono(); + patchRoute(app); + + const emptySubApp = new Hono(); + // routes is an empty array — nothing to wrap, nothing should throw + app.route('/empty', emptySubApp); + + const res = await app.fetch(new Request('http://localhost/empty')); + expect(res.status).toBe(404); + expect(startInactiveSpanMock).not.toHaveBeenCalled(); + }); + + it('skips route entries whose handler is not a function', async () => { + const app = new Hono(); + patchRoute(app); + + const subApp = new Hono(); + subApp.get('/resource', () => new Response('ok')); + + // Corrupt one handler to a non-function to simulate unexpected route shapes + (subApp.routes as unknown as Array<{ handler: unknown }>)[0]!.handler = 'not-a-function'; + + // Should not throw when iterating over the corrupted routes + expect(() => app.route('/api', subApp)).not.toThrow(); + expect(startInactiveSpanMock).not.toHaveBeenCalled(); + }); + + it('treats same path with different HTTP methods as separate groups', async () => { + const app = new Hono(); + patchRoute(app); + + const subApp = new Hono(); + // Each of these is the sole (last) handler for its method+path group, + // so none should be wrapped as middleware. + subApp.get('/resource', async function getHandler() { + return new Response('get'); + }); + subApp.post('/resource', async function postHandler() { + return new Response('post'); + }); + + app.route('/api', subApp); + + await app.fetch(new Request('http://localhost/api/resource', { method: 'GET' })); + await app.fetch(new Request('http://localhost/api/resource', { method: 'POST' })); + + expect(startInactiveSpanMock).not.toHaveBeenCalled(); + }); + + it('treats same HTTP method with different paths as separate groups', async () => { + const app = new Hono(); + patchRoute(app); + + const subApp = new Hono(); + // Each is the sole handler for its own method+path group — neither is middleware. + subApp.get('/alpha', async function alphaHandler() { + return new Response('alpha'); + }); + subApp.get('/beta', async function betaHandler() { + return new Response('beta'); + }); + + app.route('/api', subApp); + + await app.fetch(new Request('http://localhost/api/alpha')); + await app.fetch(new Request('http://localhost/api/beta')); + + expect(startInactiveSpanMock).not.toHaveBeenCalled(); + }); + + it('wraps inline middleware for GET /alpha but not the sole handler for GET /beta', async () => { + const app = new Hono(); + patchRoute(app); + + const subApp = new Hono(); + subApp.get( + '/alpha', + async function alphaMw(_c: unknown, next: () => Promise) { + await next(); + }, + async function alphaHandler() { + return new Response('alpha'); + }, + ); + subApp.get('/beta', async function betaHandler() { + return new Response('beta'); + }); + + app.route('/api', subApp); + + await app.fetch(new Request('http://localhost/api/alpha')); + await app.fetch(new Request('http://localhost/api/beta')); + + const spanNames = startInactiveSpanMock.mock.calls.map((c: unknown[]) => (c[0] as { name: string }).name); + expect(spanNames).toHaveLength(1); + expect(spanNames).toContain('alphaMw'); + expect(spanNames).not.toContain('alphaHandler'); + expect(spanNames).not.toContain('betaHandler'); + }); + }); +}); From fb1626e7e6d8abcac7e18ace50dac416a43b1921 Mon Sep 17 00:00:00 2001 From: s1gr1d <32902192+s1gr1d@users.noreply.github.com> Date: Wed, 29 Apr 2026 12:00:48 +0200 Subject: [PATCH 5/5] improve comment --- packages/hono/src/shared/patchRoute.ts | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/packages/hono/src/shared/patchRoute.ts b/packages/hono/src/shared/patchRoute.ts index 9807f4044d06..d3f732e30793 100644 --- a/packages/hono/src/shared/patchRoute.ts +++ b/packages/hono/src/shared/patchRoute.ts @@ -45,20 +45,19 @@ export function patchRoute(app: Hono): void { } /** - * Wraps middleware handlers in a sub-app's routes array with Sentry spans. + * Figures out which handlers in a sub-app's flat routes array are middleware (and should get a span), then wraps them. * - * When multiple handlers share the same method+path (e.g. `app.get('/path', mw, handler)`), - * Hono registers each as a separate route entry. We wrap all but the last entry per group - * (those are the middleware), leaving the final handler unwrapped. + * The challenge: Hono stores every handler as a plain { method, path, handler } entry. There is no "isMiddleware" flag. + * Two heuristics identify middleware: * - * For `method: 'ALL'` handlers that are last-for-group (from `.use()` or `.all()`), - * we use an arity (# of params) heuristic: middleware takes `(context, next)` (length >= 2), - * while final handlers take only `(context)` (length < 2). This distinguishes - * `.use()` middleware (should be traced) from `.all()` route handlers (should not). + * 1. Position within a group. `app.get('/path', mw, handler)` produces two entries with the same method+path. + * All but the last one must be middleware, because only middleware calls `next()` to pass control to the next handler. * - * Hono's .use() and .all() both register as method 'ALL', but .use() middleware - * always accepts (context, next) while .all() handlers typically accept only (context). - * https://github.com/honojs/hono/blob/18fe604c8cefc2628240651b1af219692e1918c1/src/hono-base.ts#L156-L168 + * 2. Function arity (# of params) for method 'ALL'. Both `.use()` and `.all()` store their handlers under method 'ALL', + * so we can't use position alone to tell them apart when one is the last (or only) entry in its group. + * The deciding factor: Hono's `.use()` only accepts `(context, next)` (handlers with 2+ params). While `.all()` route + * handlers typically only accept `(context)`. + * See: https://github.com/honojs/hono/blob/18fe604c8cefc2628240651b1af219692e1918c1/src/hono-base.ts#L156-L168 */ export function wrapSubAppMiddleware(routes: HonoRoute[]): void { const lastIndexByKey = new Map();