fix(express): Support multiple routers with common paths. (#6253)

onurtemizkan · web-flow · commit 72c6855e3e3f · 2022-11-22T17:30:15.000+01:00
Handles the case when _at the end of reconstruction_ the original URL and the reconstructed URL don't match and there are no parameters.
diff --git a/packages/node-integration-tests/.eslintrc.js b/packages/node-integration-tests/.eslintrc.js
@@ -18,6 +18,9 @@ module.exports = {
         project: ['tsconfig.test.json'],
         sourceType: 'module',
       },
+      rules: {
+        '@typescript-eslint/typedef': 'off',
+      },
     },
   ],
 };
diff --git a/packages/node-integration-tests/suites/express/multiple-routers/common-infix-parameterized/server.ts b/packages/node-integration-tests/suites/express/multiple-routers/common-infix-parameterized/server.ts
@@ -0,0 +1,34 @@
+import * as Sentry from '@sentry/node';
+import * as Tracing from '@sentry/tracing';
+import cors from 'cors';
+import express from 'express';
+
+const app = express();
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  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());
+
+app.use(cors());
+
+const APIv1 = express.Router();
+
+APIv1.get('/user/:userId', function (_req, res) {
+  Sentry.captureMessage('Custom Message');
+  res.send('Success');
+});
+
+const root = express.Router();
+
+app.use('/api2/v1', root);
+app.use('/api/v1', APIv1);
+
+app.use(Sentry.Handlers.errorHandler());
+
+export default app;
diff --git a/packages/node-integration-tests/suites/express/multiple-routers/common-infix-parameterized/test.ts b/packages/node-integration-tests/suites/express/multiple-routers/common-infix-parameterized/test.ts
@@ -0,0 +1,11 @@
+import { assertSentryEvent, TestEnv } from '../../../../utils/index';
+
+test('should construct correct url with common infixes with multiple parameterized routers.', async () => {
+  const env = await TestEnv.init(__dirname, `${__dirname}/server.ts`);
+  const event = await env.getEnvelopeRequest({ url: env.url.replace('test', 'api/v1/user/3212') });
+
+  assertSentryEvent(event[2] as any, {
+    message: 'Custom Message',
+    transaction: 'GET /api/v1/user/:userId',
+  });
+});
diff --git a/packages/node-integration-tests/suites/express/multiple-routers/common-infix/server.ts b/packages/node-integration-tests/suites/express/multiple-routers/common-infix/server.ts
@@ -0,0 +1,34 @@
+import * as Sentry from '@sentry/node';
+import * as Tracing from '@sentry/tracing';
+import cors from 'cors';
+import express from 'express';
+
+const app = express();
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  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());
+
+app.use(cors());
+
+const APIv1 = express.Router();
+
+APIv1.get('/test', function (_req, res) {
+  Sentry.captureMessage('Custom Message');
+  res.send('Success');
+});
+
+const root = express.Router();
+
+app.use('/api/v1', root);
+app.use('/api2/v1', APIv1);
+
+app.use(Sentry.Handlers.errorHandler());
+
+export default app;
diff --git a/packages/node-integration-tests/suites/express/multiple-routers/common-infix/test.ts b/packages/node-integration-tests/suites/express/multiple-routers/common-infix/test.ts
@@ -0,0 +1,11 @@
+import { assertSentryEvent, TestEnv } from '../../../../utils/index';
+
+test('should construct correct url with common infixes with multiple routers.', async () => {
+  const env = await TestEnv.init(__dirname, `${__dirname}/server.ts`);
+  const event = await env.getEnvelopeRequest({ url: env.url.replace('test', 'api2/v1/test/') });
+
+  assertSentryEvent(event[2] as any, {
+    message: 'Custom Message',
+    transaction: 'GET /api2/v1/test',
+  });
+});
diff --git a/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-parameterized-reverse/server.ts b/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-parameterized-reverse/server.ts
@@ -0,0 +1,34 @@
+import * as Sentry from '@sentry/node';
+import * as Tracing from '@sentry/tracing';
+import cors from 'cors';
+import express from 'express';
+
+const app = express();
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  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());
+
+app.use(cors());
+
+const APIv1 = express.Router();
+
+APIv1.get('/user/:userId', function (_req, res) {
+  Sentry.captureMessage('Custom Message');
+  res.send('Success');
+});
+
+const root = express.Router();
+
+app.use('/api/v1', APIv1);
+app.use('/api', root);
+
+app.use(Sentry.Handlers.errorHandler());
+
+export default app;
diff --git a/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-parameterized-reverse/test.ts b/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-parameterized-reverse/test.ts
@@ -0,0 +1,11 @@
+import { assertSentryEvent, TestEnv } from '../../../../utils/index';
+
+test('should construct correct urls with multiple parameterized routers (use order reversed).', async () => {
+  const env = await TestEnv.init(__dirname, `${__dirname}/server.ts`);
+  const event = await env.getEnvelopeRequest({ url: env.url.replace('test', 'api/v1/user/1234/') });
+
+  assertSentryEvent(event[2] as any, {
+    message: 'Custom Message',
+    transaction: 'GET /api/v1/user/:userId',
+  });
+});
diff --git a/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-parameterized/server.ts b/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-parameterized/server.ts
@@ -0,0 +1,34 @@
+import * as Sentry from '@sentry/node';
+import * as Tracing from '@sentry/tracing';
+import cors from 'cors';
+import express from 'express';
+
+const app = express();
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  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());
+
+app.use(cors());
+
+const APIv1 = express.Router();
+
+APIv1.get('/user/:userId', function (_req, res) {
+  Sentry.captureMessage('Custom Message');
+  res.send('Success');
+});
+
+const root = express.Router();
+
+app.use('/api', root);
+app.use('/api/v1', APIv1);
+
+app.use(Sentry.Handlers.errorHandler());
+
+export default app;
diff --git a/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-parameterized/test.ts b/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-parameterized/test.ts
@@ -0,0 +1,11 @@
+import { assertSentryEvent, TestEnv } from '../../../../utils/index';
+
+test('should construct correct urls with multiple parameterized routers.', async () => {
+  const env = await TestEnv.init(__dirname, `${__dirname}/server.ts`);
+  const event = await env.getEnvelopeRequest({ url: env.url.replace('test', 'api/v1/user/1234/') });
+
+  assertSentryEvent(event[2] as any, {
+    message: 'Custom Message',
+    transaction: 'GET /api/v1/user/:userId',
+  });
+});
diff --git a/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-same-length-parameterized copy/server.ts b/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-same-length-parameterized copy/server.ts
@@ -0,0 +1,34 @@
+import * as Sentry from '@sentry/node';
+import * as Tracing from '@sentry/tracing';
+import cors from 'cors';
+import express from 'express';
+
+const app = express();
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  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());
+
+app.use(cors());
+
+const APIv1 = express.Router();
+
+APIv1.get('/:userId', function (_req, res) {
+  Sentry.captureMessage('Custom Message');
+  res.send('Success');
+});
+
+const root = express.Router();
+
+app.use('/api/v1', APIv1);
+app.use('/api', root);
+
+app.use(Sentry.Handlers.errorHandler());
+
+export default app;
diff --git a/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-same-length-parameterized copy/test.ts b/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-same-length-parameterized copy/test.ts
@@ -0,0 +1,11 @@
+import { assertSentryEvent, TestEnv } from '../../../../utils/index';
+
+test('should construct correct url with multiple parameterized routers of the same length (use order reversed).', async () => {
+  const env = await TestEnv.init(__dirname, `${__dirname}/server.ts`);
+  const event = await env.getEnvelopeRequest({ url: env.url.replace('test', 'api/v1/1234/') });
+
+  assertSentryEvent(event[2] as any, {
+    message: 'Custom Message',
+    transaction: 'GET /api/v1/:userId',
+  });
+});
diff --git a/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-same-length-parameterized/server.ts b/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-same-length-parameterized/server.ts
@@ -0,0 +1,34 @@
+import * as Sentry from '@sentry/node';
+import * as Tracing from '@sentry/tracing';
+import cors from 'cors';
+import express from 'express';
+
+const app = express();
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  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());
+
+app.use(cors());
+
+const APIv1 = express.Router();
+
+APIv1.get('/:userId', function (_req, res) {
+  Sentry.captureMessage('Custom Message');
+  res.send('Success');
+});
+
+const root = express.Router();
+
+app.use('/api', root);
+app.use('/api/v1', APIv1);
+
+app.use(Sentry.Handlers.errorHandler());
+
+export default app;
diff --git a/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-same-length-parameterized/test.ts b/packages/node-integration-tests/suites/express/multiple-routers/common-prefix-same-length-parameterized/test.ts
@@ -0,0 +1,11 @@
+import { assertSentryEvent, TestEnv } from '../../../../utils/index';
+
+test('should construct correct url with multiple parameterized routers of the same length.', async () => {
+  const env = await TestEnv.init(__dirname, `${__dirname}/server.ts`);
+  const event = await env.getEnvelopeRequest({ url: env.url.replace('test', 'api/v1/1234/') });
+
+  assertSentryEvent(event[2] as any, {
+    message: 'Custom Message',
+    transaction: 'GET /api/v1/:userId',
+  });
+});
diff --git a/packages/node-integration-tests/suites/express/multiple-routers/common-prefix/server.ts b/packages/node-integration-tests/suites/express/multiple-routers/common-prefix/server.ts
@@ -0,0 +1,34 @@
+import * as Sentry from '@sentry/node';
+import * as Tracing from '@sentry/tracing';
+import cors from 'cors';
+import express from 'express';
+
+const app = express();
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  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());
+
+app.use(cors());
+
+const APIv1 = express.Router();
+
+APIv1.get('/test', function (_req, res) {
+  Sentry.captureMessage('Custom Message');
+  res.send('Success');
+});
+
+const root = express.Router();
+
+app.use('/api', root);
+app.use('/api/v1', APIv1);
+
+app.use(Sentry.Handlers.errorHandler());
+
+export default app;
diff --git a/packages/node-integration-tests/suites/express/multiple-routers/common-prefix/test.ts b/packages/node-integration-tests/suites/express/multiple-routers/common-prefix/test.ts
@@ -0,0 +1,11 @@
+import { assertSentryEvent, TestEnv } from '../../../../utils/index';
+
+test('should construct correct urls with multiple routers.', async () => {
+  const env = await TestEnv.init(__dirname, `${__dirname}/server.ts`);
+  const event = await env.getEnvelopeRequest({ url: env.url.replace('test', 'api/v1/test/') });
+
+  assertSentryEvent(event[2] as any, {
+    message: 'Custom Message',
+    transaction: 'GET /api/v1/test',
+  });
+});
diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts
@@ -36,7 +36,7 @@ type Router = {
 };
 
 /* Extend the PolymorphicRequest type with a patched parameter to build a reconstructed route */
-type PatchedRequest = PolymorphicRequest & { _reconstructedRoute?: string };
+type PatchedRequest = PolymorphicRequest & { _reconstructedRoute?: string; _hasParameters?: boolean };
 
 /* Types used for patching the express router prototype */
 type ExpressRouter = Router & {
@@ -303,6 +303,10 @@ function instrumentRouter(appOrRouter: ExpressRouter): void {
     // If the layer's partial route has params, is a regex or an array, the route is stored in layer.route.
     const { layerRoutePath, isRegex, isArray, numExtraSegments }: LayerRoutePathInfo = getLayerRoutePathInfo(layer);
 
+    if (layerRoutePath || isRegex || isArray) {
+      req._hasParameters = true;
+    }
+
     // Otherwise, the hardcoded path (i.e. a partial route without params) is stored in layer.path
     const partialRoute = layerRoutePath || layer.path || '';
 
@@ -329,6 +333,12 @@ function instrumentRouter(appOrRouter: ExpressRouter): void {
     const routeLength = getNumberOfUrlSegments(req._reconstructedRoute);
 
     if (urlLength === routeLength) {
+      if (!req._hasParameters) {
+        if (req._reconstructedRoute !== req.originalUrl) {
+          req._reconstructedRoute = req.originalUrl;
+        }
+      }
+
       const transaction = res.__sentry_transaction;
       if (transaction && transaction.metadata.source !== 'custom') {
         // If the request URL is '/' or empty, the reconstructed route will be empty.
