From 72c6855e3e3f5a6757a89dfe6d8aaa4cb286dded Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Tue, 22 Nov 2022 16:30:15 +0000 Subject: [PATCH] fix(express): Support multiple routers with common paths. (#6253) Handles the case when _at the end of reconstruction_ the original URL and the reconstructed URL don't match and there are no parameters. --- packages/node-integration-tests/.eslintrc.js | 3 ++ .../common-infix-parameterized/server.ts | 34 +++++++++++++++++++ .../common-infix-parameterized/test.ts | 11 ++++++ .../multiple-routers/common-infix/server.ts | 34 +++++++++++++++++++ .../multiple-routers/common-infix/test.ts | 11 ++++++ .../server.ts | 34 +++++++++++++++++++ .../test.ts | 11 ++++++ .../common-prefix-parameterized/server.ts | 34 +++++++++++++++++++ .../common-prefix-parameterized/test.ts | 11 ++++++ .../server.ts | 34 +++++++++++++++++++ .../test.ts | 11 ++++++ .../server.ts | 34 +++++++++++++++++++ .../test.ts | 11 ++++++ .../multiple-routers/common-prefix/server.ts | 34 +++++++++++++++++++ .../multiple-routers/common-prefix/test.ts | 11 ++++++ .../tracing/src/integrations/node/express.ts | 12 ++++++- 16 files changed, 329 insertions(+), 1 deletion(-) create mode 100644 packages/node-integration-tests/suites/express/multiple-routers/common-infix-parameterized/server.ts create mode 100644 packages/node-integration-tests/suites/express/multiple-routers/common-infix-parameterized/test.ts create mode 100644 packages/node-integration-tests/suites/express/multiple-routers/common-infix/server.ts create mode 100644 packages/node-integration-tests/suites/express/multiple-routers/common-infix/test.ts create mode 100644 packages/node-integration-tests/suites/express/multiple-routers/common-prefix-parameterized-reverse/server.ts create mode 100644 packages/node-integration-tests/suites/express/multiple-routers/common-prefix-parameterized-reverse/test.ts create mode 100644 packages/node-integration-tests/suites/express/multiple-routers/common-prefix-parameterized/server.ts create mode 100644 packages/node-integration-tests/suites/express/multiple-routers/common-prefix-parameterized/test.ts create mode 100644 packages/node-integration-tests/suites/express/multiple-routers/common-prefix-same-length-parameterized copy/server.ts create mode 100644 packages/node-integration-tests/suites/express/multiple-routers/common-prefix-same-length-parameterized copy/test.ts create mode 100644 packages/node-integration-tests/suites/express/multiple-routers/common-prefix-same-length-parameterized/server.ts create mode 100644 packages/node-integration-tests/suites/express/multiple-routers/common-prefix-same-length-parameterized/test.ts create mode 100644 packages/node-integration-tests/suites/express/multiple-routers/common-prefix/server.ts create mode 100644 packages/node-integration-tests/suites/express/multiple-routers/common-prefix/test.ts diff --git a/packages/node-integration-tests/.eslintrc.js b/packages/node-integration-tests/.eslintrc.js index 5a3ecdd7617a..6c8a493dccb3 100644 --- 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 new file mode 100644 index 000000000000..190a836d10c9 --- /dev/null +++ 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 new file mode 100644 index 000000000000..2bf79275a7a9 --- /dev/null +++ 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 new file mode 100644 index 000000000000..4751d8fa82fb --- /dev/null +++ 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 new file mode 100644 index 000000000000..fc921c23b350 --- /dev/null +++ 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 new file mode 100644 index 000000000000..15c8d63b64e3 --- /dev/null +++ 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 new file mode 100644 index 000000000000..55bc51f60ddf --- /dev/null +++ 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 new file mode 100644 index 000000000000..05861702e020 --- /dev/null +++ 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 new file mode 100644 index 000000000000..81b624e8bdb8 --- /dev/null +++ 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 new file mode 100644 index 000000000000..b1f2fd321409 --- /dev/null +++ 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 new file mode 100644 index 000000000000..2ccbd477a557 --- /dev/null +++ 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 new file mode 100644 index 000000000000..2ea0325b5fdd --- /dev/null +++ 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 new file mode 100644 index 000000000000..05449a0b18bf --- /dev/null +++ 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 new file mode 100644 index 000000000000..02f0bb20928e --- /dev/null +++ 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 new file mode 100644 index 000000000000..cd5b81be2992 --- /dev/null +++ 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 index 24260f627c0f..cb405664600a 100644 --- 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.