From a69b3877e584a76f6202eceeeba34692affaebfd Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Tue, 2 Aug 2022 15:43:57 -0400 Subject: [PATCH 1/4] fix(react): Fix React Router v6 paramaterization Make sure paramaterization occurs in scenarios where the full path is not avaliable after all the branches are walked. In those scenarios, we have to construct the paramaterized name based on the previous branches that were analyzed. --- packages/react/src/reactrouterv6.tsx | 38 ++++++++++++++++++++-- packages/react/test/reactrouterv6.test.tsx | 34 +++++++++++++++++++ 2 files changed, 69 insertions(+), 3 deletions(-) diff --git a/packages/react/src/reactrouterv6.tsx b/packages/react/src/reactrouterv6.tsx index 8f2c2825a2b3..78583484f5d8 100644 --- a/packages/react/src/reactrouterv6.tsx +++ b/packages/react/src/reactrouterv6.tsx @@ -20,9 +20,23 @@ type Params = { readonly [key in Key]: string | undefined; }; +// https://github.com/remix-run/react-router/blob/9fa54d643134cd75a0335581a75db8100ed42828/packages/react-router/lib/router.ts#L114-L134 interface RouteMatch { + /** + * The names and values of dynamic parameters in the URL. + */ params: Params; + /** + * The portion of the URL pathname that was matched. + */ pathname: string; + /** + * The portion of the URL pathname that was matched before child routes. + */ + pathnameBase: string; + /** + * The route object that was used to match. + */ route: RouteObject; } @@ -94,13 +108,31 @@ function getNormalizedName( const branches = matchRoutes(routes, location); + let pathBuilder = ''; if (branches) { // eslint-disable-next-line @typescript-eslint/prefer-for-of for (let x = 0; x < branches.length; x++) { - if (branches[x].route && branches[x].route.path && branches[x].pathname === location.pathname) { - const path = branches[x].route.path; + const branch = branches[x]; + const route = branch.route; + if (route) { + // Early return if index route + if (route.index) { + return [branch.pathname, 'route']; + } + + const path = route.path; if (path) { - return [path, 'route']; + const newPath = path[0] === '/' ? path : `/${path}`; + pathBuilder += newPath; + if (branch.pathname === location.pathname) { + // If the route defined on the element is something like + // Product} /> + // We should check against the branch.pathname for the number of / seperators + if (pathBuilder.split('/').length !== branch.pathname.split('/').length) { + return [newPath, 'route']; + } + return [pathBuilder, 'route']; + } } } } diff --git a/packages/react/test/reactrouterv6.test.tsx b/packages/react/test/reactrouterv6.test.tsx index 2a5acb921ae8..0058f257aee5 100644 --- a/packages/react/test/reactrouterv6.test.tsx +++ b/packages/react/test/reactrouterv6.test.tsx @@ -196,4 +196,38 @@ describe('React Router v6', () => { metadata: { source: 'route' }, }); }); + + it('works with nested paths with parameters', () => { + const [mockStartTransaction] = createInstrumentation(); + const SentryRoutes = withSentryReactRouterV6Routing(Routes); + + render( + + + } /> + Account Page} /> + + Project Index} /> + Project Page}> + Project Page Root} /> + Editor}> + View Canvas} /> + Space Canvas} /> + + + + + No Match Page} /> + + , + ); + + expect(mockStartTransaction).toHaveBeenCalledTimes(2); + expect(mockStartTransaction).toHaveBeenLastCalledWith({ + name: '/projects/:projectId/views/:viewId', + op: 'navigation', + tags: { 'routing.instrumentation': 'react-router-v6' }, + metadata: { source: 'route' }, + }); + }); }); From 0a220fe661a53660c010329b5ed01708175c9f76 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 3 Aug 2022 15:30:38 -0400 Subject: [PATCH 2/4] extract getNumberOfUrlSegments into utils --- packages/react/src/reactrouterv6.tsx | 4 +- .../tracing/src/integrations/node/express.ts | 17 +++--- packages/utils/src/index.ts | 1 + packages/utils/src/misc.ts | 45 ---------------- packages/utils/src/url.ts | 52 +++++++++++++++++++ packages/utils/test/misc.test.ts | 22 -------- packages/utils/test/url.test.ts | 33 ++++++++++++ 7 files changed, 95 insertions(+), 79 deletions(-) create mode 100644 packages/utils/src/url.ts create mode 100644 packages/utils/test/url.test.ts diff --git a/packages/react/src/reactrouterv6.tsx b/packages/react/src/reactrouterv6.tsx index 78583484f5d8..a7950ce0b12f 100644 --- a/packages/react/src/reactrouterv6.tsx +++ b/packages/react/src/reactrouterv6.tsx @@ -2,7 +2,7 @@ // https://gist.github.com/wontondon/e8c4bdf2888875e4c755712e99279536 import { Transaction, TransactionContext, TransactionSource } from '@sentry/types'; -import { getGlobalObject, logger } from '@sentry/utils'; +import { getGlobalObject, getNumberOfUrlSegments, logger } from '@sentry/utils'; import hoistNonReactStatics from 'hoist-non-react-statics'; import React from 'react'; @@ -128,7 +128,7 @@ function getNormalizedName( // If the route defined on the element is something like // Product} /> // We should check against the branch.pathname for the number of / seperators - if (pathBuilder.split('/').length !== branch.pathname.split('/').length) { + if (getNumberOfUrlSegments(pathBuilder) !== getNumberOfUrlSegments(branch.pathname)) { return [newPath, 'route']; } return [pathBuilder, 'route']; diff --git a/packages/tracing/src/integrations/node/express.ts b/packages/tracing/src/integrations/node/express.ts index a211dc279e28..d82d29292e46 100644 --- a/packages/tracing/src/integrations/node/express.ts +++ b/packages/tracing/src/integrations/node/express.ts @@ -1,6 +1,12 @@ /* eslint-disable max-lines */ import { Integration, Transaction } from '@sentry/types'; -import { CrossPlatformRequest, extractPathForTransaction, isRegExp, logger } from '@sentry/utils'; +import { + CrossPlatformRequest, + extractPathForTransaction, + getNumberOfUrlSegments, + isRegExp, + logger, +} from '@sentry/utils'; type Method = | 'all' @@ -384,15 +390,6 @@ 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 diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index 7c8943da813f..b221bef38c91 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -25,3 +25,4 @@ export * from './envelope'; export * from './clientreport'; export * from './ratelimit'; export * from './baggage'; +export * from './url'; diff --git a/packages/utils/src/misc.ts b/packages/utils/src/misc.ts index 19025cbb846a..86ab32278ffe 100644 --- a/packages/utils/src/misc.ts +++ b/packages/utils/src/misc.ts @@ -41,40 +41,6 @@ export function uuid4(): string { ); } -/** - * Parses string form of URL into an object - * // borrowed from https://tools.ietf.org/html/rfc3986#appendix-B - * // intentionally using regex and not href parsing trick because React Native and other - * // environments where DOM might not be available - * @returns parsed URL object - */ -export function parseUrl(url: string): { - host?: string; - path?: string; - protocol?: string; - relative?: string; -} { - if (!url) { - return {}; - } - - const match = url.match(/^(([^:/?#]+):)?(\/\/([^/?#]*))?([^?#]*)(\?([^#]*))?(#(.*))?$/); - - if (!match) { - return {}; - } - - // coerce to undefined values to empty string so we don't get 'undefined' - const query = match[6] || ''; - const fragment = match[8] || ''; - return { - host: match[4], - path: match[5], - protocol: match[2], - relative: match[5] + query + fragment, // everything minus origin - }; -} - function getFirstException(event: Event): Exception | undefined { return event.exception && event.exception.values ? event.exception.values[0] : undefined; } @@ -197,17 +163,6 @@ export function addContextToFrame(lines: string[], frame: StackFrame, linesOfCon .map((line: string) => snipLine(line, 0)); } -/** - * Strip the query string and fragment off of a given URL or path (if present) - * - * @param urlPath Full URL or path, including possible query string and/or fragment - * @returns URL or path without query string or fragment - */ -export function stripUrlQueryAndFragment(urlPath: string): string { - // eslint-disable-next-line no-useless-escape - return urlPath.split(/[\?#]/, 1)[0]; -} - /** * Checks whether or not we've already captured the given exception (note: not an identical exception - the very object * in question), and marks it captured if not. diff --git a/packages/utils/src/url.ts b/packages/utils/src/url.ts new file mode 100644 index 000000000000..8dce48e7428b --- /dev/null +++ b/packages/utils/src/url.ts @@ -0,0 +1,52 @@ +/** + * Parses string form of URL into an object + * // borrowed from https://tools.ietf.org/html/rfc3986#appendix-B + * // intentionally using regex and not href parsing trick because React Native and other + * // environments where DOM might not be available + * @returns parsed URL object + */ +export function parseUrl(url: string): { + host?: string; + path?: string; + protocol?: string; + relative?: string; +} { + if (!url) { + return {}; + } + + const match = url.match(/^(([^:/?#]+):)?(\/\/([^/?#]*))?([^?#]*)(\?([^#]*))?(#(.*))?$/); + + if (!match) { + return {}; + } + + // coerce to undefined values to empty string so we don't get 'undefined' + const query = match[6] || ''; + const fragment = match[8] || ''; + return { + host: match[4], + path: match[5], + protocol: match[2], + relative: match[5] + query + fragment, // everything minus origin + }; +} + +/** + * Strip the query string and fragment off of a given URL or path (if present) + * + * @param urlPath Full URL or path, including possible query string and/or fragment + * @returns URL or path without query string or fragment + */ +export function stripUrlQueryAndFragment(urlPath: string): string { + // eslint-disable-next-line no-useless-escape + return urlPath.split(/[\?#]/, 1)[0]; +} + +/** + * Returns number of URL segments of a passed string URL. + */ +export function getNumberOfUrlSegments(url: string): number { + // split at '/' or at '\/' to split regex urls correctly + return url.split(/\\?\//).filter(s => s.length > 0 && s !== ',').length; +} diff --git a/packages/utils/test/misc.test.ts b/packages/utils/test/misc.test.ts index a92d3963e503..c7c98991efbf 100644 --- a/packages/utils/test/misc.test.ts +++ b/packages/utils/test/misc.test.ts @@ -5,7 +5,6 @@ import { addExceptionMechanism, checkOrSetAlreadyCaught, getEventDescription, - stripUrlQueryAndFragment, uuid4, } from '../src/misc'; @@ -187,27 +186,6 @@ describe('addContextToFrame', () => { }); }); -describe('stripQueryStringAndFragment', () => { - const urlString = 'http://dogs.are.great:1231/yay/'; - const queryString = '?furry=yes&funny=very'; - const fragment = '#adoptnotbuy'; - - it('strips query string from url', () => { - const urlWithQueryString = `${urlString}${queryString}`; - expect(stripUrlQueryAndFragment(urlWithQueryString)).toBe(urlString); - }); - - it('strips fragment from url', () => { - const urlWithFragment = `${urlString}${fragment}`; - expect(stripUrlQueryAndFragment(urlWithFragment)).toBe(urlString); - }); - - it('strips query string and fragment from url', () => { - const urlWithQueryStringAndFragment = `${urlString}${queryString}${fragment}`; - expect(stripUrlQueryAndFragment(urlWithQueryStringAndFragment)).toBe(urlString); - }); -}); - describe('addExceptionMechanism', () => { const defaultMechanism = { type: 'generic', handled: true }; diff --git a/packages/utils/test/url.test.ts b/packages/utils/test/url.test.ts new file mode 100644 index 000000000000..37ae3803a934 --- /dev/null +++ b/packages/utils/test/url.test.ts @@ -0,0 +1,33 @@ +import { stripUrlQueryAndFragment, getNumberOfUrlSegments } from '../src/url'; + +describe('stripQueryStringAndFragment', () => { + const urlString = 'http://dogs.are.great:1231/yay/'; + const queryString = '?furry=yes&funny=very'; + const fragment = '#adoptnotbuy'; + + it('strips query string from url', () => { + const urlWithQueryString = `${urlString}${queryString}`; + expect(stripUrlQueryAndFragment(urlWithQueryString)).toBe(urlString); + }); + + it('strips fragment from url', () => { + const urlWithFragment = `${urlString}${fragment}`; + expect(stripUrlQueryAndFragment(urlWithFragment)).toBe(urlString); + }); + + it('strips query string and fragment from url', () => { + const urlWithQueryStringAndFragment = `${urlString}${queryString}${fragment}`; + expect(stripUrlQueryAndFragment(urlWithQueryStringAndFragment)).toBe(urlString); + }); +}); + +describe('getNumberOfUrlSegments', () => { + test.each([ + ['regular path', '/projects/123/views/234', 4], + ['single param paramaterized path', '/users/:id/details', 3], + ['multi param paramaterized path', '/stores/:storeId/products/:productId', 4], + ['regex path', String(/\/api\/post[0-9]/), 2], + ])('%s', (_: string, input, output) => { + expect(getNumberOfUrlSegments(input)).toEqual(output); + }); +}); From 50e907fef217d37a227e4f7dd4ada026b24fe6f8 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 3 Aug 2022 15:37:22 -0400 Subject: [PATCH 3/4] fix build --- packages/utils/src/requestdata.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index 72c12b2e5399..b45f3e874cfc 100644 --- a/packages/utils/src/requestdata.ts +++ b/packages/utils/src/requestdata.ts @@ -15,8 +15,8 @@ import { Event, ExtractedNodeRequestData, Transaction, TransactionSource } from '@sentry/types'; import { isPlainObject, isString } from './is'; -import { stripUrlQueryAndFragment } from './misc'; import { normalize } from './normalize'; +import { stripUrlQueryAndFragment } from './url'; const DEFAULT_INCLUDES = { ip: false, From 8f10fa6d39b3b009d4eeaeb59bc9b8a83bcc0f47 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Wed, 3 Aug 2022 15:48:58 -0400 Subject: [PATCH 4/4] lint fix --- packages/utils/test/url.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/utils/test/url.test.ts b/packages/utils/test/url.test.ts index 37ae3803a934..e356662b9b29 100644 --- a/packages/utils/test/url.test.ts +++ b/packages/utils/test/url.test.ts @@ -1,4 +1,4 @@ -import { stripUrlQueryAndFragment, getNumberOfUrlSegments } from '../src/url'; +import { getNumberOfUrlSegments, stripUrlQueryAndFragment } from '../src/url'; describe('stripQueryStringAndFragment', () => { const urlString = 'http://dogs.are.great:1231/yay/';