fix(react): Fix React Router v6 paramaterization (#5515)

Make sure paramaterization occurs in scenarios where the full path is not available after all the branches are walked. In those scenarios, we have to construct the paramaterized name based on the previous branches that were analyzed.

This patch also creates a new file in `@sentry/utils` - `url.ts`, where we store some url / string related utils. This was made so that the react package would get access to the `getNumberOfUrlSegments` util.

Author: AbhiPrasad

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';
 
@@ -20,9 +20,23 @@ type Params<Key extends string = string> = {
   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<ParamKey extends string = string> {
+  /**
+   * The names and values of dynamic parameters in the URL.
+   */
   params: Params<ParamKey>;
+  /**
+   * 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
+            // <Route path="/stores/:storeId/products/:productId" element={<div>Product</div>} />
+            // We should check against the branch.pathname for the number of / seperators
+            if (getNumberOfUrlSegments(pathBuilder) !== getNumberOfUrlSegments(branch.pathname)) {
+              return [newPath, 'route'];
+            }
+            return [pathBuilder, 'route'];
+          }
         }
       }
     }

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(
+      <MemoryRouter initialEntries={['/']}>
+        <SentryRoutes>
+          <Route index element={<Navigate to="/projects/123/views/234" />} />
+          <Route path="account" element={<div>Account Page</div>} />
+          <Route path="projects">
+            <Route index element={<div>Project Index</div>} />
+            <Route path=":projectId" element={<div>Project Page</div>}>
+              <Route index element={<div>Project Page Root</div>} />
+              <Route element={<div>Editor</div>}>
+                <Route path="views/:viewId" element={<div>View Canvas</div>} />
+                <Route path="spaces/:spaceId" element={<div>Space Canvas</div>} />
+              </Route>
+            </Route>
+          </Route>
+
+          <Route path="*" element={<div>No Match Page</div>} />
+        </SentryRoutes>
+      </MemoryRouter>,
+    );
+
+    expect(mockStartTransaction).toHaveBeenCalledTimes(2);
+    expect(mockStartTransaction).toHaveBeenLastCalledWith({
+      name: '/projects/:projectId/views/:viewId',
+      op: 'navigation',
+      tags: { 'routing.instrumentation': 'react-router-v6' },
+      metadata: { source: 'route' },
+    });
+  });
 });

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

packages/utils/src/index.ts

@@ -25,3 +25,4 @@ export * from './envelope';
 export * from './clientreport';
 export * from './ratelimit';
 export * from './baggage';
+export * from './url';

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 <a/> 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.

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,

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 <a/> 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;
+}

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 };
 

packages/utils/test/url.test.ts

@@ -0,0 +1,33 @@
+import { getNumberOfUrlSegments, stripUrlQueryAndFragment } 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);
+  });
+});