fix(hapi): Handle routes registered with an array of methods

dev-packages/node-integration-tests/suites/tracing/hapi/scenario.mjs

@@ -18,6 +18,12 @@ const run = async () => {
     },
   });
 
+  server.route({
+    method: ['GET', 'POST'],
+    path: '/multi',
+    handler: () => 'ok',
+  });
+
   server.route({
     method: 'GET',
     path: '/error',

dev-packages/node-integration-tests/suites/tracing/hapi/test.ts

@@ -1,3 +1,5 @@
+import { HTTP_REQUEST_METHOD, HTTP_ROUTE, SENTRY_OP, SENTRY_ORIGIN } from '@sentry/conventions/attributes';
+import { ROUTER } from '@sentry/conventions/op';
 import { afterAll, describe, expect } from 'vitest';
 import { cleanupChildProcesses, createEsmAndCjsTests } from '../../../utils/runner';
 
@@ -47,6 +49,51 @@ describe('hapi auto-instrumentation', () => {
       await runner.completed();
     });
 
+    test('should instrument a route registered with an array of methods.', async () => {
+      const runner = createRunner()
+        .unordered()
+        .expect({
+          span: container => {
+            expect(container.items.find(item => item.is_segment)?.name).toBe('POST /multi');
+            expect(container.items).toContainEqual(
+              expect.objectContaining({
+                name: '/multi',
+                status: 'ok',
+                attributes: expect.objectContaining({
+                  [HTTP_ROUTE]: { type: 'string', value: '/multi' },
+                  [HTTP_REQUEST_METHOD]: { type: 'string', value: 'POST' },
+                  'hapi.type': { type: 'string', value: 'router' },
+                  [SENTRY_ORIGIN]: { type: 'string', value: origin },
+                  [SENTRY_OP]: { type: 'string', value: ROUTER },
+                }),
+              }),
+            );
+          },
+        })
+        .expect({
+          span: container => {
+            expect(container.items.find(item => item.is_segment)?.name).toBe('GET /multi');
+            expect(container.items).toContainEqual(
+              expect.objectContaining({
+                name: '/multi',
+                status: 'ok',
+                attributes: expect.objectContaining({
+                  [HTTP_ROUTE]: { type: 'string', value: '/multi' },
+                  [HTTP_REQUEST_METHOD]: { type: 'string', value: 'GET' },
+                  'hapi.type': { type: 'string', value: 'router' },
+                  [SENTRY_ORIGIN]: { type: 'string', value: origin },
+                  [SENTRY_OP]: { type: 'string', value: ROUTER },
+                }),
+              }),
+            );
+          },
+        })
+        .start();
+      await runner.makeRequest('post', '/multi');
+      await runner.makeRequest('get', '/multi');
+      await runner.completed();
+    });
+
     test('should instrument plugin routes and server extensions.', async () => {
       const runner = createRunner()
         .expect({

packages/server-utils/src/integrations/hapi/hapi-types.ts

@@ -29,7 +29,8 @@ export interface ServerRouteOptions {
 
 export interface ServerRoute {
   path: string;
-  method: string;
+  /** A single method (including `'*'`), or several methods registered together. */
+  method: string | string[];
   handler?: LifecycleMethod | unknown;
   options?: ((server: unknown) => ServerRouteOptions) | ServerRouteOptions;
   [key: string]: unknown;

packages/server-utils/src/integrations/hapi/hapi-utils.ts

@@ -107,13 +107,40 @@ const isPatchableExtMethod = (
   return !Array.isArray(variableToCheck);
 };
 
+/**
+ * Method hapi has already resolved for this request.
+ *
+ * `server.route()` accepts `method` as a string or an array and expands an array
+ * into one route per method only after instrumentation wraps the handler, so the
+ * closed-over config can still hold the array. The request carries the one method
+ * that matched (lower case).
+ */
+function matchedRequestMethod(request: unknown): string | undefined {
+  if (isObjectLike(request) && typeof request.method === 'string' && request.method.length > 0) {
+    return request.method;
+  }
+  return undefined;
+}
+
+/** A single method string safe to put on a span. Non-strings never reach `toUpperCase`. */
+function routeMethodForSpan(route: ServerRoute, requestMethod?: string): string | undefined {
+  if (typeof route.method === 'string') {
+    return route.method;
+  }
+  return requestMethod ? requestMethod.toUpperCase() : undefined;
+}
+
 /** Build the span name and attributes for a Hapi route. */
-export const getRouteMetadata = (route: ServerRoute, pluginName?: string): SpanMetadata => {
+export const getRouteMetadata = (route: ServerRoute, pluginName?: string, requestMethod?: string): SpanMetadata => {
+  const method = routeMethodForSpan(route, requestMethod);
   const attributes: SpanAttributes = {
     [HTTP_ROUTE]: route.path,
-    [HTTP_REQUEST_METHOD]: route.method,
   };
 
+  if (method !== undefined) {
+    attributes[HTTP_REQUEST_METHOD] = method;
+  }
+
   if (pluginName) {
     attributes[AttributeNames.HAPI_TYPE] = HapiLayerType.PLUGIN;
     attributes[AttributeNames.PLUGIN_NAME] = pluginName;
@@ -129,7 +156,11 @@ export const getRouteMetadata = (route: ServerRoute, pluginName?: string): SpanM
 
   return {
     attributes,
-    name: isStreamedSpan ? route.path || fallbackName : `${route.method.toUpperCase()} ${route.path}`,
+    name: isStreamedSpan
+      ? route.path || fallbackName
+      : method !== undefined
+        ? `${method.toUpperCase()} ${route.path}`
+        : route.path || fallbackName,
   };
 };
 
@@ -199,7 +230,7 @@ function wrapRouteHandler(route: PatchableServerRoute, pluginName?: string): Pat
         return oldHandler.call(this, ...params);
       }
       setHttpServerSpanRouteAttribute(route.path);
-      const metadata = getRouteMetadata(route, pluginName);
+      const metadata = getRouteMetadata(route, pluginName, matchedRequestMethod(params[0]));
       return startMetadataSpan(metadata, () => oldHandler.call(this, ...params));
     };
   };

packages/server-utils/test/integrations/hapi-utils.test.ts

@@ -1,6 +1,8 @@
-import { setCurrentClient } from '@sentry/core';
+import { HTTP_REQUEST_METHOD, HTTP_ROUTE } from '@sentry/conventions/attributes';
+import { setCurrentClient, spanToJSON, startSpan } from '@sentry/core';
+import type { Span } from '@sentry/core';
 import { afterEach, describe, expect, it } from 'vitest';
-import { getExtMetadata, getRouteMetadata } from '../../src/integrations/hapi/hapi-utils';
+import { getExtMetadata, getRouteMetadata, wrapRouteArguments } from '../../src/integrations/hapi/hapi-utils';
 import { getDefaultTestClientOptions, TestClient } from '../mocks/client';
 
 describe('getRouteMetadata', () => {
@@ -56,6 +58,205 @@ describe('getRouteMetadata', () => {
     expect(getRouteMetadata(pathlessRoute).name).toBe('Router');
     expect(getRouteMetadata(pathlessRoute, 'my-plugin').name).toBe('Request handler');
   });
+
+  it('uses one uppercase method when the route was registered with an array of methods', () => {
+    expect(getRouteMetadata({ path: '/multi', method: ['GET', 'POST'] } as any, undefined, 'post')).toEqual({
+      name: 'POST /multi',
+      attributes: {
+        [HTTP_ROUTE]: '/multi',
+        [HTTP_REQUEST_METHOD]: 'POST',
+        'hapi.type': 'router',
+      },
+    });
+  });
+
+  it('uses one uppercase method for a plugin route registered with an array of methods', () => {
+    expect(getRouteMetadata({ path: '/multi', method: ['GET', 'POST'] } as any, 'my-plugin', 'get')).toEqual({
+      name: 'GET /multi',
+      attributes: {
+        [HTTP_ROUTE]: '/multi',
+        [HTTP_REQUEST_METHOD]: 'GET',
+        'hapi.type': 'plugin',
+        'hapi.plugin.name': 'my-plugin',
+      },
+    });
+  });
+
+  it('records one uppercase method and keeps the path span name when span streaming is enabled', () => {
+    const client = new TestClient(getDefaultTestClientOptions({ traceLifecycle: 'stream' }));
+    setCurrentClient(client);
+
+    expect(getRouteMetadata({ path: '/multi', method: ['GET', 'POST'] } as any, undefined, 'post')).toEqual({
+      name: '/multi',
+      attributes: {
+        [HTTP_ROUTE]: '/multi',
+        [HTTP_REQUEST_METHOD]: 'POST',
+        'hapi.type': 'router',
+      },
+    });
+  });
+
+  it('does not throw when an array method has no resolved request method', () => {
+    expect(getRouteMetadata({ path: '/multi', method: ['GET', 'POST'] } as any)).toEqual({
+      name: '/multi',
+      attributes: {
+        [HTTP_ROUTE]: '/multi',
+        'hapi.type': 'router',
+      },
+    });
+  });
+
+  it('keeps a string route method when a request method is also provided', () => {
+    expect(getRouteMetadata({ path: '/single', method: 'GET' } as any, undefined, 'post')).toEqual({
+      name: 'GET /single',
+      attributes: {
+        [HTTP_ROUTE]: '/single',
+        [HTTP_REQUEST_METHOD]: 'GET',
+        'hapi.type': 'router',
+      },
+    });
+  });
+});
+
+describe('wrapRouteArguments', () => {
+  afterEach(() => {
+    setCurrentClient(undefined as unknown as TestClient);
+  });
+
+  function captureSpans(traceLifecycle?: 'stream'): Span[] {
+    const client = new TestClient(
+      getDefaultTestClientOptions({
+        dsn: 'https://public@dsn.ingest.sentry.io/1337',
+        tracesSampleRate: 1,
+        ...(traceLifecycle ? { traceLifecycle } : {}),
+      }),
+    );
+    setCurrentClient(client);
+    client.init();
+
+    const endedSpans: Span[] = [];
+    client.on('spanEnd', span => {
+      endedSpans.push(span);
+    });
+    return endedSpans;
+  }
+
+  function invokeWrapped(
+    route: { method: string | string[]; path: string; handler: (request: unknown, h: unknown) => unknown },
+    request: { method: string },
+  ): unknown {
+    wrapRouteArguments([route]);
+    return startSpan({ name: 'incoming' }, () => route.handler(request, {}));
+  }
+
+  function routerSpan(endedSpans: Span[]) {
+    return endedSpans.map(span => spanToJSON(span)).find(span => span.attributes['hapi.type'] === 'router');
+  }
+
+  it('reads the request method for a route registered with an array of methods', () => {
+    const endedSpans = captureSpans();
+
+    expect(
+      invokeWrapped(
+        {
+          method: ['GET', 'POST'],
+          path: '/multi',
+          handler: () => 'ok',
+        },
+        { method: 'post' },
+      ),
+    ).toBe('ok');
+
+    expect(routerSpan(endedSpans)).toMatchObject({
+      name: 'POST /multi',
+      attributes: {
+        [HTTP_ROUTE]: '/multi',
+        [HTTP_REQUEST_METHOD]: 'POST',
+        'hapi.type': 'router',
+      },
+    });
+  });
+
+  it('reads the request method and keeps the path span name when span streaming is enabled', () => {
+    const endedSpans = captureSpans('stream');
+
+    invokeWrapped(
+      {
+        method: ['GET', 'POST'],
+        path: '/multi',
+        handler: () => 'ok',
+      },
+      { method: 'get' },
+    );
+
+    expect(routerSpan(endedSpans)).toMatchObject({
+      name: '/multi',
+      attributes: {
+        [HTTP_ROUTE]: '/multi',
+        [HTTP_REQUEST_METHOD]: 'GET',
+      },
+    });
+  });
+
+  it('keeps the registered method for a string route', () => {
+    const endedSpans = captureSpans();
+
+    invokeWrapped(
+      {
+        method: 'GET',
+        path: '/single',
+        handler: () => 'ok',
+      },
+      { method: 'get' },
+    );
+
+    expect(routerSpan(endedSpans)).toMatchObject({
+      name: 'GET /single',
+      attributes: {
+        [HTTP_REQUEST_METHOD]: 'GET',
+      },
+    });
+  });
+
+  it('still runs the route handler when building span metadata cannot resolve a method', () => {
+    const endedSpans = captureSpans();
+
+    expect(
+      invokeWrapped(
+        {
+          method: ['GET', 'POST'],
+          path: '/multi',
+          handler: () => 'ok',
+        },
+        { method: '' },
+      ),
+    ).toBe('ok');
+
+    expect(routerSpan(endedSpans)).toMatchObject({
+      name: '/multi',
+      attributes: {
+        [HTTP_ROUTE]: '/multi',
+      },
+    });
+    expect(routerSpan(endedSpans)?.attributes[HTTP_REQUEST_METHOD]).toBeUndefined();
+  });
+
+  it('propagates errors thrown by the route handler', () => {
+    captureSpans();
+
+    expect(() =>
+      invokeWrapped(
+        {
+          method: ['GET', 'POST'],
+          path: '/multi',
+          handler: () => {
+            throw new Error('user');
+          },
+        },
+        { method: 'post' },
+      ),
+    ).toThrow('user');
+  });
 });
 
 describe('getExtMetadata', () => {