From 8dd304c7bec251a551e4df38fbfc854866b58c01 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 29 Jul 2022 12:53:58 +0200 Subject: [PATCH 1/9] fix(node): Adjust Express URL parameterization for array routes --- .../suites/express/tracing/server.ts | 4 ++ .../suites/express/tracing/test.ts | 29 ++++++++++ .../tracing/src/integrations/node/express.ts | 56 ++++++++++++++++--- 3 files changed, 81 insertions(+), 8 deletions(-) diff --git a/packages/node-integration-tests/suites/express/tracing/server.ts b/packages/node-integration-tests/suites/express/tracing/server.ts index acfaca932f49..075e5f451e29 100644 --- a/packages/node-integration-tests/suites/express/tracing/server.ts +++ b/packages/node-integration-tests/suites/express/tracing/server.ts @@ -25,6 +25,10 @@ app.get(/\/test\/regex/, (_req, res) => { res.send({ response: 'response 2' }); }); +app.get(['/test/array1', /\/test\/array[2-9]/], (_req, res) => { + res.send({ response: 'response 3' }); +}); + app.use(Sentry.Handlers.errorHandler()); export default app; diff --git a/packages/node-integration-tests/suites/express/tracing/test.ts b/packages/node-integration-tests/suites/express/tracing/test.ts index 8190f4bbed6a..bda32c1c24ec 100644 --- a/packages/node-integration-tests/suites/express/tracing/test.ts +++ b/packages/node-integration-tests/suites/express/tracing/test.ts @@ -53,3 +53,32 @@ test('should set a correct transaction name for routes specified in RegEx', asyn }, }); }); + +test.each([ + ['', 'array1'], + ['', 'array5'], +])('%s should set a correct transaction name for routes consisting of arrays of routes', async (_, segment) => { + const url = await runServer(__dirname, `${__dirname}/server.ts`); + const envelope = await getEnvelopeRequest(`${url}/${segment}`); + + expect(envelope).toHaveLength(3); + + assertSentryTransaction(envelope[2], { + transaction: 'GET /test/array1,/\\/test\\/array[2-9]', + transaction_info: { + source: 'route', + }, + contexts: { + trace: { + data: { + url: `/test/${segment}`, + }, + op: 'http.server', + status: 'ok', + tags: { + 'http.status_code': '200', + }, + }, + }, + }); +}); diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts index dc3ae0696c09..32a8baf3f4ba 100644 --- a/packages/tracing/src/integrations/node/express.ts +++ b/packages/tracing/src/integrations/node/express.ts @@ -51,11 +51,13 @@ type ExpressRouter = Router & { ) => unknown; }; +type RouteType = string | RegExp; + /* Type used for pathing the express router prototype */ type Layer = { match: (path: string) => boolean; handle_request: (req: PatchedRequest, res: ExpressResponse, next: () => void) => void; - route?: { path: string | RegExp }; + route?: { path: RouteType | RouteType[] }; path?: string; }; @@ -273,11 +275,8 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { req._reconstructedRoute = ''; } - // If the layer's partial route has params, the route is stored in layer.route. - // Since a route might be defined with a RegExp, we convert it toString to make sure we end up with a string - const lrp = layer.route?.path; - const isRegex = isRegExp(lrp); - const layerRoutePath = isRegex ? lrp?.toString() : (lrp as string); + // If the layer's partial route has params, is a regex or an array, the route is stored in layer.route. + const { layerRoutePath, isRegex, numExtraSegments }: LayerRoutePathInfo = getLayerRoutePathString(layer); // Otherwise, the hardcoded path (i.e. a partial route without params) is stored in layer.path const partialRoute = layerRoutePath || layer.path || ''; @@ -301,7 +300,7 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { // Now we check if we are in the "last" part of the route. We determine this by comparing the // number of URL segments from the original URL to that of our reconstructed parameterized URL. // If we've reached our final destination, we update the transaction name. - const urlLength = getNumberOfUrlSegments(req.originalUrl || ''); + const urlLength = getNumberOfUrlSegments(req.originalUrl || '') + numExtraSegments; const routeLength = getNumberOfUrlSegments(req._reconstructedRoute); if (urlLength === routeLength) { @@ -319,7 +318,48 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { }; } +type LayerRoutePathInfo = { + layerRoutePath?: string; + isRegex: boolean; + numExtraSegments: number; +}; + +/** + * 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]`). + * + * @param layer the layer to extract the stringified route from + * + * @returns an object containing the stringified route, a flag determining if the route was a regex + * and the number of extra segments to the matched path that are additionally in the route, + * if the route was an array (defaults to 0). + */ +function getLayerRoutePathString(layer: Layer): LayerRoutePathInfo { + const lrp = layer.route?.path; + + const isRegex = isRegExp(lrp); + const isArray = Array.isArray(lrp); + + const numExtraSegments = isArray ? getNumberOfArrayUrlSegments(lrp) - getNumberOfUrlSegments(layer.path || '') : 0; + + const layerRoutePath = isArray ? lrp.join(',') : isRegex ? lrp?.toString() : (lrp as string | undefined); + + return { layerRoutePath, isRegex, numExtraSegments }; +} + +/** + * Returns the number of URL segments in an array of routes + * + * Example: ['/api/test', /\/api\/post[0-9]/, '/users/:id/details`] -> 7 + */ +function getNumberOfArrayUrlSegments(routesArray: RouteType[]): number { + return routesArray.reduce((accNumSegments: number, currentRoute: RouteType) => { + // array members can be a RegEx -> convert them toString + return accNumSegments + getNumberOfUrlSegments(currentRoute.toString()); + }, 0); +} + function getNumberOfUrlSegments(url: string): number { // split at '/' or at '\/' to split regex urls correctly - return url.split(/\\?\//).filter(s => s.length > 0).length; + return url.split(/\\?\//).filter(s => s.length > 0 && s !== ',').length; } From 10fededc39fe15e17febf517d0f9fe3751211a72 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 29 Jul 2022 13:20:40 +0200 Subject: [PATCH 2/9] rearrange types --- packages/tracing/src/integrations/node/express.ts | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts index 32a8baf3f4ba..2d3cd5caa5d5 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 CrossPlatformRequest type with a patched parameter to build a reconstructed route */ type PatchedRequest = CrossPlatformRequest & { _reconstructedRoute?: string }; -/* Type used for pathing the express router prototype */ +/* Types used for patching the express router prototype */ type ExpressRouter = Router & { _router?: ExpressRouter; stack?: Layer[]; @@ -51,9 +51,6 @@ type ExpressRouter = Router & { ) => unknown; }; -type RouteType = string | RegExp; - -/* Type used for pathing the express router prototype */ type Layer = { match: (path: string) => boolean; handle_request: (req: PatchedRequest, res: ExpressResponse, next: () => void) => void; @@ -61,6 +58,8 @@ type Layer = { path?: string; }; +type RouteType = string | RegExp; + interface ExpressResponse { once(name: string, callback: () => void): void; } From 9512e0d4311377cf95e7ee37af3ff9ae7ec59df7 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 29 Jul 2022 13:55:04 +0200 Subject: [PATCH 3/9] fix type errors --- packages/tracing/src/integrations/node/express.ts | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts index 2d3cd5caa5d5..7eaa0bfc1708 100644 --- a/packages/tracing/src/integrations/node/express.ts +++ b/packages/tracing/src/integrations/node/express.ts @@ -339,9 +339,19 @@ function getLayerRoutePathString(layer: Layer): LayerRoutePathInfo { const isRegex = isRegExp(lrp); const isArray = Array.isArray(lrp); - const numExtraSegments = isArray ? getNumberOfArrayUrlSegments(lrp) - getNumberOfUrlSegments(layer.path || '') : 0; + if (!lrp) { + return { isRegex, numExtraSegments: 0 }; + } + + const numExtraSegments = isArray + ? getNumberOfArrayUrlSegments(lrp as RouteType[]) - getNumberOfUrlSegments(layer.path || '') + : 0; - const layerRoutePath = isArray ? lrp.join(',') : isRegex ? lrp?.toString() : (lrp as string | undefined); + const layerRoutePath = isArray + ? (lrp as RouteType[]).map((r: RouteType) => r.toString()).join(',') + : isRegex + ? lrp.toString() + : (lrp as string | undefined); return { layerRoutePath, isRegex, numExtraSegments }; } From da3e66fbafe9763a0daad70b85aaf83452f7afcb Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 29 Jul 2022 15:20:13 +0200 Subject: [PATCH 4/9] remove unnecessary type cast --- packages/tracing/src/integrations/node/express.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts index 7eaa0bfc1708..f028ea207278 100644 --- a/packages/tracing/src/integrations/node/express.ts +++ b/packages/tracing/src/integrations/node/express.ts @@ -348,7 +348,7 @@ function getLayerRoutePathString(layer: Layer): LayerRoutePathInfo { : 0; const layerRoutePath = isArray - ? (lrp as RouteType[]).map((r: RouteType) => r.toString()).join(',') + ? (lrp as RouteType[]).map(r => r.toString()).join(',') : isRegex ? lrp.toString() : (lrp as string | undefined); From 24806670e6428743ee452b97824aae8f2e4cd6b8 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 1 Aug 2022 11:38:19 +0200 Subject: [PATCH 5/9] ensure numExtraSegments is greater than 0 --- packages/tracing/src/integrations/node/express.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts index f028ea207278..8e8342fe0249 100644 --- a/packages/tracing/src/integrations/node/express.ts +++ b/packages/tracing/src/integrations/node/express.ts @@ -344,7 +344,7 @@ function getLayerRoutePathString(layer: Layer): LayerRoutePathInfo { } const numExtraSegments = isArray - ? getNumberOfArrayUrlSegments(lrp as RouteType[]) - getNumberOfUrlSegments(layer.path || '') + ? Math.max(getNumberOfArrayUrlSegments(lrp as RouteType[]) - getNumberOfUrlSegments(layer.path || ''), 0) : 0; const layerRoutePath = isArray From 6054e1cbeabe55fbe2138d5a002d0f9060b3363e Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 1 Aug 2022 11:47:54 +0200 Subject: [PATCH 6/9] extract layer route path stringification to a function --- .../tracing/src/integrations/node/express.ts | 30 ++++++++++++++----- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts index 8e8342fe0249..198def361077 100644 --- a/packages/tracing/src/integrations/node/express.ts +++ b/packages/tracing/src/integrations/node/express.ts @@ -275,7 +275,7 @@ 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, numExtraSegments }: LayerRoutePathInfo = getLayerRoutePathString(layer); + const { layerRoutePath, isRegex, numExtraSegments }: LayerRoutePathInfo = getLayerRoutePathInfo(layer); // Otherwise, the hardcoded path (i.e. a partial route without params) is stored in layer.path const partialRoute = layerRoutePath || layer.path || ''; @@ -325,7 +325,8 @@ type LayerRoutePathInfo = { /** * 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]`). + * a RegEx (`/test/`) or an array of strings and regexes (`['/path1', /\/path[2-5]/, /path/:id]`). Additionally + * returns extra information about the route, such as if the route is defined as regex or as an array. * * @param layer the layer to extract the stringified route from * @@ -333,7 +334,7 @@ type LayerRoutePathInfo = { * and the number of extra segments to the matched path that are additionally in the route, * if the route was an array (defaults to 0). */ -function getLayerRoutePathString(layer: Layer): LayerRoutePathInfo { +function getLayerRoutePathInfo(layer: Layer): LayerRoutePathInfo { const lrp = layer.route?.path; const isRegex = isRegExp(lrp); @@ -347,11 +348,7 @@ function getLayerRoutePathString(layer: Layer): LayerRoutePathInfo { ? Math.max(getNumberOfArrayUrlSegments(lrp as RouteType[]) - getNumberOfUrlSegments(layer.path || ''), 0) : 0; - const layerRoutePath = isArray - ? (lrp as RouteType[]).map(r => r.toString()).join(',') - : isRegex - ? lrp.toString() - : (lrp as string | undefined); + const layerRoutePath = getLayerRoutePathString(isArray, lrp); return { layerRoutePath, isRegex, numExtraSegments }; } @@ -368,7 +365,24 @@ function getNumberOfArrayUrlSegments(routesArray: RouteType[]): number { }, 0); } +/** + * Returns number of URL segments of a passed URL. + * Also handles URLs of type RegExp + */ function getNumberOfUrlSegments(url: string): number { // split at '/' or at '\/' to split regex urls correctly return url.split(/\\?\//).filter(s => s.length > 0 && s !== ',').length; } + +/** + * Extracts and returns the stringified version of the layers route path + * Handles route arrays (by joining the paths together) as well as RegExp and normal + * string values (in the latter case the toString conversion is technically unnecessary but + * it doesn't hurt us either). + */ +function getLayerRoutePathString(isArray: boolean, lrp?: RouteType | RouteType[]): string | undefined { + if (isArray) { + return (lrp as RouteType[]).map(r => r.toString()).join(','); + } + return lrp && lrp.toString(); +} From 3105f205d1e0842fa85531d0979453d3656de351 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 1 Aug 2022 11:55:25 +0200 Subject: [PATCH 7/9] simplify test syntax --- .../suites/express/tracing/test.ts | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/packages/node-integration-tests/suites/express/tracing/test.ts b/packages/node-integration-tests/suites/express/tracing/test.ts index bda32c1c24ec..9689c259edc0 100644 --- a/packages/node-integration-tests/suites/express/tracing/test.ts +++ b/packages/node-integration-tests/suites/express/tracing/test.ts @@ -54,31 +54,31 @@ test('should set a correct transaction name for routes specified in RegEx', asyn }); }); -test.each([ - ['', 'array1'], - ['', 'array5'], -])('%s should set a correct transaction name for routes consisting of arrays of routes', async (_, segment) => { - const url = await runServer(__dirname, `${__dirname}/server.ts`); - const envelope = await getEnvelopeRequest(`${url}/${segment}`); +test.each([['array1'], ['array5']])( + 'should set a correct transaction name for routes consisting of arrays of routes', + async segment => { + const url = await runServer(__dirname, `${__dirname}/server.ts`); + const envelope = await getEnvelopeRequest(`${url}/${segment}`); - expect(envelope).toHaveLength(3); + expect(envelope).toHaveLength(3); - assertSentryTransaction(envelope[2], { - transaction: 'GET /test/array1,/\\/test\\/array[2-9]', - transaction_info: { - source: 'route', - }, - contexts: { - trace: { - data: { - url: `/test/${segment}`, - }, - op: 'http.server', - status: 'ok', - tags: { - 'http.status_code': '200', + assertSentryTransaction(envelope[2], { + transaction: 'GET /test/array1,/\\/test\\/array[2-9]', + transaction_info: { + source: 'route', + }, + contexts: { + trace: { + data: { + url: `/test/${segment}`, + }, + op: 'http.server', + status: 'ok', + tags: { + 'http.status_code': '200', + }, }, }, - }, - }); -}); + }); + }, +); From 84e7342a61d0d59e2ed1674d3d30cf601c04ce76 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 1 Aug 2022 12:49:11 +0200 Subject: [PATCH 8/9] add tests and fix small partialRoute bug --- .../suites/express/tracing/server.ts | 4 +++ .../suites/express/tracing/test.ts | 33 +++++++++++++++++++ .../tracing/src/integrations/node/express.ts | 9 ++--- 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/packages/node-integration-tests/suites/express/tracing/server.ts b/packages/node-integration-tests/suites/express/tracing/server.ts index 075e5f451e29..faf5a50f95ed 100644 --- a/packages/node-integration-tests/suites/express/tracing/server.ts +++ b/packages/node-integration-tests/suites/express/tracing/server.ts @@ -29,6 +29,10 @@ app.get(['/test/array1', /\/test\/array[2-9]/], (_req, res) => { res.send({ response: 'response 3' }); }); +app.get(['/test/arr/:id', /\/test\/arr[0-9]*\/required(path)?(\/optionalPath)?\/(lastParam)?/], (_req, res) => { + res.send({ response: 'response 4' }); +}); + app.use(Sentry.Handlers.errorHandler()); export default app; diff --git a/packages/node-integration-tests/suites/express/tracing/test.ts b/packages/node-integration-tests/suites/express/tracing/test.ts index 9689c259edc0..3949a62afb9d 100644 --- a/packages/node-integration-tests/suites/express/tracing/test.ts +++ b/packages/node-integration-tests/suites/express/tracing/test.ts @@ -82,3 +82,36 @@ test.each([['array1'], ['array5']])( }); }, ); + +test.each([ + ['arr/545'], + ['arr/required'], + ['arr/requiredPath'], + ['arr/required/lastParam'], + ['arr/requiredPath/optionalPath/'], + ['arr/requiredPath/optionalPath/lastParam'], +])('should handle more complex regexes in route arrays correctly', async segment => { + const url = await runServer(__dirname, `${__dirname}/server.ts`); + const envelope = await getEnvelopeRequest(`${url}/${segment}`); + + expect(envelope).toHaveLength(3); + + assertSentryTransaction(envelope[2], { + transaction: 'GET /test/arr/:id,/\\/test\\/arr[0-9]*\\/required(path)?(\\/optionalPath)?\\/(lastParam)?', + transaction_info: { + source: 'route', + }, + contexts: { + trace: { + data: { + url: `/test/${segment}`, + }, + op: 'http.server', + status: 'ok', + tags: { + 'http.status_code': '200', + }, + }, + }, + }); +}); diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts index 198def361077..7e6037f9866a 100644 --- a/packages/tracing/src/integrations/node/express.ts +++ b/packages/tracing/src/integrations/node/express.ts @@ -275,7 +275,7 @@ 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, numExtraSegments }: LayerRoutePathInfo = getLayerRoutePathInfo(layer); + const { layerRoutePath, isRegex, isArray, numExtraSegments }: LayerRoutePathInfo = getLayerRoutePathInfo(layer); // Otherwise, the hardcoded path (i.e. a partial route without params) is stored in layer.path const partialRoute = layerRoutePath || layer.path || ''; @@ -287,7 +287,7 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { // We want to end up with the parameterized URL of the incoming request without any extraneous path segments. const finalPartialRoute = partialRoute .split('/') - .filter(segment => segment.length > 0 && !segment.includes('*')) + .filter(segment => segment.length > 0 && (isRegex || isArray || !segment.includes('*'))) .join('/'); // If we found a valid partial URL, we append it to the reconstructed route @@ -320,6 +320,7 @@ function instrumentRouter(appOrRouter: ExpressRouter): void { type LayerRoutePathInfo = { layerRoutePath?: string; isRegex: boolean; + isArray: boolean; numExtraSegments: number; }; @@ -341,7 +342,7 @@ function getLayerRoutePathInfo(layer: Layer): LayerRoutePathInfo { const isArray = Array.isArray(lrp); if (!lrp) { - return { isRegex, numExtraSegments: 0 }; + return { isRegex, isArray, numExtraSegments: 0 }; } const numExtraSegments = isArray @@ -350,7 +351,7 @@ function getLayerRoutePathInfo(layer: Layer): LayerRoutePathInfo { const layerRoutePath = getLayerRoutePathString(isArray, lrp); - return { layerRoutePath, isRegex, numExtraSegments }; + return { layerRoutePath, isRegex, isArray, numExtraSegments }; } /** From 5e8fd40605c91353aa74e6ab29c4061414752106 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 1 Aug 2022 12:56:22 +0200 Subject: [PATCH 9/9] add one more test --- packages/node-integration-tests/suites/express/tracing/test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/node-integration-tests/suites/express/tracing/test.ts b/packages/node-integration-tests/suites/express/tracing/test.ts index 3949a62afb9d..043a91aeeb60 100644 --- a/packages/node-integration-tests/suites/express/tracing/test.ts +++ b/packages/node-integration-tests/suites/express/tracing/test.ts @@ -86,8 +86,10 @@ test.each([['array1'], ['array5']])( test.each([ ['arr/545'], ['arr/required'], + ['arr/required'], ['arr/requiredPath'], ['arr/required/lastParam'], + ['arr55/required/lastParam'], ['arr/requiredPath/optionalPath/'], ['arr/requiredPath/optionalPath/lastParam'], ])('should handle more complex regexes in route arrays correctly', async segment => {