fix: Extract transaction from nested express paths correctly (#2714)

* fix: Extract transaction from nested express paths correctly
getsentry · Jul 1, 2020 · 5325fe3 · 5325fe3
1 parent b210907
commit 5325fe3
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 18 deletions.
diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts
@@ -80,6 +80,8 @@ function extractTransaction(req: { [key: string]: any }, type: boolean | Transac
   try {
     // Express.js shape
     const request = req as {
+      url: string;
+      originalUrl: string;
       method: string;
       route: {
         path: string;
@@ -91,18 +93,24 @@ function extractTransaction(req: { [key: string]: any }, type: boolean | Transac
       };
     };
 
+    let routePath;
+    try {
+      routePath = url.parse(request.originalUrl || request.url).pathname;
+    } catch (_oO) {
+      routePath = request.route.path;
+    }
+
     switch (type) {
       case 'path': {
-        return request.route.path;
+        return routePath;
       }
       case 'handler': {
         return request.route.stack[0].name;
       }
       case 'methodPath':
       default: {
         const method = request.method.toUpperCase();
-        const path = request.route.path;
-        return `${method}|${path}`;
+        return `${method}|${routePath}`;
       }
     }
   } catch (_oO) {

diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts
@@ -4,21 +4,34 @@ import { Event, Request, User } from '../src';
 import { parseRequest } from '../src/handlers';
 
 describe('parseRequest', () => {
-  const mockReq = {
-    body: 'foo',
-    cookies: { test: 'test' },
-    headers: {
-      host: 'mattrobenolt.com',
-    },
-    method: 'POST',
-    url: '/some/path?key=value',
-    user: {
-      custom_property: 'foo',
-      email: 'tobias@mail.com',
-      id: 123,
-      username: 'tobias',
-    },
-  };
+  let mockReq: { [key: string]: any };
+
+  beforeEach(() => {
+    mockReq = {
+      body: 'foo',
+      cookies: { test: 'test' },
+      headers: {
+        host: 'mattrobenolt.com',
+      },
+      method: 'POST',
+      originalUrl: '/some/originalUrl?key=value',
+      route: {
+        path: '/path',
+        stack: [
+          {
+            name: 'routeHandler',
+          },
+        ],
+      },
+      url: '/some/url?key=value',
+      user: {
+        custom_property: 'foo',
+        email: 'tobias@mail.com',
+        id: 123,
+        username: 'tobias',
+      },
+    };
+  });
 
   describe('parseRequest.contexts runtime', () => {
     test('runtime name must contain node', () => {
@@ -121,4 +134,34 @@ describe('parseRequest', () => {
       expect(parseRequest({}, { ...mockReq, method: 'HEAD' }, {}).request).not.toHaveProperty('data');
     });
   });
+
+  describe('parseRequest.transaction property', () => {
+    test('extracts method and full route path by default from `originalUrl`', () => {
+      const parsedRequest: Event = parseRequest({}, mockReq);
+      expect(parsedRequest.transaction).toEqual('POST|/some/originalUrl');
+    });
+
+    test('extracts method and full route path by default from `url` if `originalUrl` is not present', () => {
+      delete mockReq.originalUrl;
+      const parsedRequest: Event = parseRequest({}, mockReq);
+      expect(parsedRequest.transaction).toEqual('POST|/some/url');
+    });
+
+    test('fallback to method and `route.path` if previous attempts failed', () => {
+      delete mockReq.originalUrl;
+      delete mockReq.url;
+      const parsedRequest: Event = parseRequest({}, mockReq);
+      expect(parsedRequest.transaction).toEqual('POST|/path');
+    });
+
+    test('can extract path only instead if configured', () => {
+      const parsedRequest: Event = parseRequest({}, mockReq, { transaction: 'path' });
+      expect(parsedRequest.transaction).toEqual('/some/originalUrl');
+    });
+
+    test('can extract handler name instead if configured', () => {
+      const parsedRequest: Event = parseRequest({}, mockReq, { transaction: 'handler' });
+      expect(parsedRequest.transaction).toEqual('routeHandler');
+    });
+  });
 });