fix(tracing-internal): Parameterize express middleware parameters (#8668)

 Account for the use case when path params are define in express middleware (`use`) not in express endpoint route (`get`, `post`, .etc).

Author: LubomirIgonda1

packages/node-integration-tests/suites/express/multiple-routers/complex-router/server.ts

@@ -0,0 +1,35 @@
+import * as Sentry from '@sentry/node';
+import * as Tracing from '@sentry/tracing';
+import express from 'express';
+
+const app = express();
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  release: '1.0',
+  // eslint-disable-next-line deprecation/deprecation
+  integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
+  tracesSampleRate: 1.0,
+});
+
+app.use(Sentry.Handlers.requestHandler());
+app.use(Sentry.Handlers.tracingHandler());
+
+const APIv1 = express.Router();
+
+APIv1.use(
+  '/users/:userId',
+  APIv1.get('/posts/:postId', (_req, res) => {
+    Sentry.captureMessage('Custom Message');
+    return res.send('Success');
+  }),
+);
+
+const router = express.Router();
+
+app.use('/api', router);
+app.use('/api/api/v1', APIv1.use('/sub-router', APIv1));
+
+app.use(Sentry.Handlers.errorHandler());
+
+export default app;

packages/node-integration-tests/suites/express/multiple-routers/complex-router/test.ts

@@ -0,0 +1,27 @@
+import { assertSentryEvent, TestEnv } from '../../../../utils/index';
+
+test('should construct correct url with multiple parameterized routers, when param is also contain in middle layer route and express used multiple middlewares with route', async () => {
+  const env = await TestEnv.init(__dirname, `${__dirname}/server.ts`);
+  const event = await env.getEnvelopeRequest({
+    url: env.url.replace('test', 'api/api/v1/sub-router/users/123/posts/456'),
+    envelopeType: 'transaction',
+  });
+  // parse node.js major version
+  const [major] = process.versions.node.split('.').map(Number);
+  // Split test result base on major node version because regex d flag is support from node 16+
+  if (major >= 16) {
+    assertSentryEvent(event[2] as any, {
+      transaction: 'GET /api/api/v1/sub-router/users/:userId/posts/:postId',
+      transaction_info: {
+        source: 'route',
+      },
+    });
+  } else {
+    assertSentryEvent(event[2] as any, {
+      transaction: 'GET /api/api/v1/sub-router/users/123/posts/:postId',
+      transaction_info: {
+        source: 'route',
+      },
+    });
+  }
+});

packages/node-integration-tests/suites/express/multiple-routers/middle-layer-parameterized/server.ts

@@ -0,0 +1,35 @@
+import * as Sentry from '@sentry/node';
+import * as Tracing from '@sentry/tracing';
+import express from 'express';
+
+const app = express();
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  release: '1.0',
+  // eslint-disable-next-line deprecation/deprecation
+  integrations: [new Sentry.Integrations.Http({ tracing: true }), new Tracing.Integrations.Express({ app })],
+  tracesSampleRate: 1.0,
+});
+
+app.use(Sentry.Handlers.requestHandler());
+app.use(Sentry.Handlers.tracingHandler());
+
+const APIv1 = express.Router();
+
+APIv1.use(
+  '/users/:userId',
+  APIv1.get('/posts/:postId', (_req, res) => {
+    Sentry.captureMessage('Custom Message');
+    return res.send('Success');
+  }),
+);
+
+const root = express.Router();
+
+app.use('/api/v1', APIv1);
+app.use('/api', root);
+
+app.use(Sentry.Handlers.errorHandler());
+
+export default app;

packages/node-integration-tests/suites/express/multiple-routers/middle-layer-parameterized/test.ts

@@ -0,0 +1,28 @@
+import { assertSentryEvent, TestEnv } from '../../../../utils/index';
+
+test('should construct correct url with multiple parameterized routers, when param is also contain in middle layer route', async () => {
+  const env = await TestEnv.init(__dirname, `${__dirname}/server.ts`);
+  const event = await env.getEnvelopeRequest({
+    url: env.url.replace('test', 'api/v1/users/123/posts/456'),
+    envelopeType: 'transaction',
+  });
+
+  // parse node.js major version
+  const [major] = process.versions.node.split('.').map(Number);
+  // Split test result base on major node version because regex d flag is support from node 16+
+  if (major >= 16) {
+    assertSentryEvent(event[2] as any, {
+      transaction: 'GET /api/v1/users/:userId/posts/:postId',
+      transaction_info: {
+        source: 'route',
+      },
+    });
+  } else {
+    assertSentryEvent(event[2] as any, {
+      transaction: 'GET /api/v1/users/123/posts/:postId',
+      transaction_info: {
+        source: 'route',
+      },
+    });
+  }
+});

packages/node/src/requestdata.ts

@@ -109,7 +109,9 @@ function extractTransaction(req: PolymorphicRequest, type: boolean | Transaction
     }
     case 'methodPath':
     default: {
-      return extractPathForTransaction(req, { path: true, method: true })[0];
+      // if exist _reconstructedRoute return that path instead of route.path
+      const customRoute = req._reconstructedRoute ? req._reconstructedRoute : undefined;
+      return extractPathForTransaction(req, { path: true, method: true, customRoute })[0];
     }
   }
 }

packages/tracing-internal/src/node/integrations/express.ts

@@ -64,6 +64,8 @@ type Layer = {
   handle_request: (req: PatchedRequest, res: ExpressResponse, next: () => void) => void;
   route?: { path: RouteType | RouteType[] };
   path?: string;
+  regexp?: RegExp;
+  keys?: { name: string; offset: number; optional: boolean }[];
 };
 
 type RouteType = string | RegExp;
@@ -318,7 +320,24 @@ function instrumentRouter(appOrRouter: ExpressRouter): void {
     }
 
     // Otherwise, the hardcoded path (i.e. a partial route without params) is stored in layer.path
-    const partialRoute = layerRoutePath || layer.path || '';
+    let partialRoute;
+
+    if (layerRoutePath) {
+      partialRoute = layerRoutePath;
+    } else {
+      /**
+       * prevent duplicate segment in _reconstructedRoute param if router match multiple routes before final path
+       * example:
+       * original url: /api/v1/1234
+       * prevent: /api/api/v1/:userId
+       * router structure
+       * /api -> middleware
+       * /api/v1 -> middleware
+       * /1234 -> endpoint with param :userId
+       * final _reconstructedRoute is /api/v1/:userId
+       */
+      partialRoute = preventDuplicateSegments(req.originalUrl, req._reconstructedRoute, layer.path) || '';
+    }
 
     // Normalize the partial route so that it doesn't contain leading or trailing slashes
     // and exclude empty or '*' wildcard routes.
@@ -370,6 +389,79 @@ type LayerRoutePathInfo = {
   numExtraSegments: number;
 };
 
+/**
+ * Recreate layer.route.path from layer.regexp and layer.keys.
+ * Works until express.js used package [email protected]
+ * or until layer.keys contain offset attribute
+ *
+ * @param layer the layer to extract the stringified route from
+ *
+ * @returns string in layer.route.path structure 'router/:pathParam' or undefined
+ */
+export const extractOriginalRoute = (
+  path?: Layer['path'],
+  regexp?: Layer['regexp'],
+  keys?: Layer['keys'],
+): string | undefined => {
+  if (!path || !regexp || !keys || Object.keys(keys).length === 0 || !keys[0]?.offset) {
+    return undefined;
+  }
+
+  const orderedKeys = keys.sort((a, b) => a.offset - b.offset);
+
+  // add d flag for getting indices from regexp result
+  const pathRegex = new RegExp(regexp, `${regexp.flags}d`);
+  /**
+   * use custom type cause of TS error with missing indices in RegExpExecArray
+   */
+  const execResult = pathRegex.exec(path) as (RegExpExecArray & { indices: [number, number][] }) | null;
+
+  if (!execResult || !execResult.indices) {
+    return undefined;
+  }
+  /**
+   * remove first match from regex cause contain whole layer.path
+   */
+  const [, ...paramIndices] = execResult.indices;
+
+  if (paramIndices.length !== orderedKeys.length) {
+    return undefined;
+  }
+  let resultPath = path;
+  let indexShift = 0;
+
+  /**
+   * iterate param matches from regexp.exec
+   */
+  paramIndices.forEach(([startOffset, endOffset], index: number) => {
+    /**
+     * isolate part before param
+     */
+    const substr1 = resultPath.substring(0, startOffset - indexShift);
+    /**
+     * define paramName as replacement in format :pathParam
+     */
+    const replacement = `:${orderedKeys[index].name}`;
+
+    /**
+     * isolate part after param
+     */
+    const substr2 = resultPath.substring(endOffset - indexShift);
+
+    /**
+     * recreate original path but with param replacement
+     */
+    resultPath = substr1 + replacement + substr2;
+
+    /**
+     * calculate new index shift after resultPath was modified
+     */
+    indexShift = indexShift + (endOffset - startOffset - replacement.length);
+  });
+
+  return resultPath;
+};
+
 /**
  * Extracts and stringifies the layer's route which can either be a string with parameters (`users/:id`),
  * a RegEx (`/test/`) or an array of strings and regexes (`['/path1', /\/path[2-5]/, /path/:id]`). Additionally
@@ -382,11 +474,24 @@ type LayerRoutePathInfo = {
  *          if the route was an array (defaults to 0).
  */
 function getLayerRoutePathInfo(layer: Layer): LayerRoutePathInfo {
-  const lrp = layer.route?.path;
+  let lrp = layer.route?.path;
 
   const isRegex = isRegExp(lrp);
   const isArray = Array.isArray(lrp);
 
+  if (!lrp) {
+    // parse node.js major version
+    const [major] = process.versions.node.split('.').map(Number);
+
+    // allow call extractOriginalRoute only if node version support Regex d flag, node 16+
+    if (major >= 16) {
+      /**
+       * If lrp does not exist try to recreate original layer path from route regexp
+       */
+      lrp = extractOriginalRoute(layer.path, layer.regexp, layer.keys);
+    }
+  }
+
   if (!lrp) {
     return { isRegex, isArray, numExtraSegments: 0 };
   }
@@ -424,3 +529,28 @@ function getLayerRoutePathString(isArray: boolean, lrp?: RouteType | RouteType[]
   }
   return lrp && lrp.toString();
 }
+
+/**
+ * remove duplicate segment contain in layerPath against reconstructedRoute,
+ * and return only unique segment that can be added into reconstructedRoute
+ */
+export function preventDuplicateSegments(
+  originalUrl?: string,
+  reconstructedRoute?: string,
+  layerPath?: string,
+): string | undefined {
+  const originalUrlSplit = originalUrl?.split('/').filter(v => !!v);
+  let tempCounter = 0;
+  const currentOffset = reconstructedRoute?.split('/').filter(v => !!v).length || 0;
+  const result = layerPath
+    ?.split('/')
+    .filter(segment => {
+      if (originalUrlSplit?.[currentOffset + tempCounter] === segment) {
+        tempCounter += 1;
+        return true;
+      }
+      return false;
+    })
+    .join('/');
+  return result;
+}