Skip to content

Commit bef613e

Browse files
mydeaclaude
andcommitted
fix(hono): classify matched handlers by position, not arity alone
Arity alone cannot tell a middleware from a route handler declared with an unused `next` param. The matched entries carry their registration routeMeta, so apply the same positional heuristic as wrapSubAppMiddleware: within a method+path group the last handler is the route handler and earlier ones are middleware; `.use()` (method 'ALL') falls back to arity. Adds node-integration coverage for the arity-2 route handler case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 5166755 commit bef613e

3 files changed

Lines changed: 56 additions & 5 deletions

File tree

dev-packages/node-integration-tests/suites/hono/scenario.mjs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,11 @@ app.get('/error/:param', () => {
1919
throw new Error('Test error from Hono app');
2020
});
2121

22+
// A route handler declared with an unused `next` param (arity 2). It is the last handler in its
23+
// method+path group, so it must be classified as the route handler — not wrapped as a middleware
24+
// span — even though arity alone would misclassify it.
25+
app.get('/arity-two-handler', (c, _next) => c.text('handler with two params'));
26+
2227
// A sub-app with a named middleware, mounted via `app.route()`. The sub-app is also
2328
// auto-instrumented (every `new Hono()` is), so its own Sentry middleware must NOT show up as an
2429
// `<anonymous>` middleware span when it is copied into the parent at mount time.

dev-packages/node-integration-tests/suites/hono/test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,26 @@ describe('hono auto-instrumentation', () => {
101101
await runner.completed();
102102
});
103103

104+
test('does not wrap a route handler declared with an unused next param as middleware', async () => {
105+
const runner = createRunner()
106+
.unordered()
107+
.expect({
108+
span: container => {
109+
const segment = container.items.find(item => item.is_segment && item.name === 'GET /arity-two-handler');
110+
if (!segment) {
111+
throw new Error('segment for `GET /arity-two-handler` not in this container');
112+
}
113+
// The arity-2 handler is the route handler (last in its method+path group), so it must not
114+
// produce a middleware span.
115+
const middlewareSpans = container.items.filter(item => op(item) === 'middleware');
116+
expect(middlewareSpans).toHaveLength(0);
117+
},
118+
})
119+
.start();
120+
runner.makeRequest('get', '/arity-two-handler');
121+
await runner.completed();
122+
});
123+
104124
test('does not create a middleware span for the Sentry middleware in a mounted sub-app', async () => {
105125
const runner = createRunner()
106126
.unordered()

packages/server-utils/src/integrations/hono/honoIntegration.ts

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -131,13 +131,39 @@ function injectHonoInstrumentation(
131131
}
132132
_injectedHandlerLists.add(handlers);
133133

134-
// Wrap matched middleware handlers. `isMiddleware` also unwraps `onError`-composed sub-app handlers
135-
// before checking, which a raw arity check would miss. `wrapMiddlewareWithSpan` is idempotent and
136-
// skips Sentry's own middleware, so this is safe even if a handler is shared across routes.
137-
for (const entry of handlers) {
134+
// Wrap the matched middleware handlers for spans, leaving the route handler alone. Each entry
135+
// carries its registration `routeMeta` (`entry[0][1]`: `{ method, path, … }`), so we use the same
136+
// positional heuristic as `wrapSubAppMiddleware`: within a method+path group the LAST matched
137+
// handler is the route handler and earlier ones are middleware (`app.get(path, mw, handler)`);
138+
// `.use()` registers as method 'ALL' where the sole entry is genuinely middleware, so fall back to
139+
// arity (via `isMiddleware`, which also unwraps `onError`-composed handlers) there. Position matters
140+
// because arity alone would misclassify a route handler declared with an unused `next` param.
141+
// `wrapMiddlewareWithSpan` is idempotent and skips Sentry's own middleware, so this is safe even
142+
// when a handler is shared across routes.
143+
const lastIndexByKey = new Map<string, number>();
144+
for (const [i, entry] of handlers.entries()) {
145+
const routeMeta = entry?.[0]?.[1] as { method?: string; path?: string } | undefined;
146+
if (routeMeta?.method != null && routeMeta.path != null) {
147+
// \0 is a collision-free delimiter: it cannot appear in a valid HTTP method or URL path.
148+
lastIndexByKey.set(`${routeMeta.method}\0${routeMeta.path}`, i);
149+
}
150+
}
151+
152+
for (const [i, entry] of handlers.entries()) {
138153
const pair = entry?.[0];
139154
const handler = pair?.[0];
140-
if (isMiddleware(handler)) {
155+
if (typeof handler !== 'function') {
156+
continue;
157+
}
158+
159+
const routeMeta = pair?.[1] as { method?: string; path?: string } | undefined;
160+
const isMW =
161+
routeMeta?.method != null && routeMeta.path != null
162+
? lastIndexByKey.get(`${routeMeta.method}\0${routeMeta.path}`) !== i ||
163+
(routeMeta.method === 'ALL' && isMiddleware(handler))
164+
: isMiddleware(handler);
165+
166+
if (isMW) {
141167
pair[0] = wrapMiddlewareWithSpan(handler as MiddlewareHandler);
142168
}
143169
}

0 commit comments

Comments
 (0)