From b5497c7ca156fd36ba0683b7524b768addfd5c87 Mon Sep 17 00:00:00 2001
From: Lukas Stracke <lukas.stracke@sentry.io>
Date: Thu, 27 Oct 2022 10:11:04 +0200
Subject: [PATCH] fix(vue): Don't overwrite custom transaction names of
 pageload transactions (#6060)

As brought to our attention in #6048, our pageload transaction start change (which makes the transaction start earlier) introduced in #5983 caused custom transaction names to be overwritten when `beforeNavigate` is used to update the transaction name.

This patch fixes this problem by checking for the transaction source before (not) updating the current transaction name with the resolved route name once the router's `beforeEach` hook was called. Furthermore, it adds a test to cover this case.

See https://github.com/getsentry/sentry-javascript/pull/6048#issuecomment-1293083543 for the motivation for creating this PR.
---
 packages/vue/src/router.ts       |  4 ++-
 packages/vue/test/router.test.ts | 53 ++++++++++++++++++++++++++++++--
 2 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/packages/vue/src/router.ts b/packages/vue/src/router.ts
index 1131943612e1..f782246e8fb2 100644
--- a/packages/vue/src/router.ts
+++ b/packages/vue/src/router.ts
@@ -92,7 +92,9 @@ export function vueRouterInstrumentation(router: VueRouter): VueRouterInstrument
       if (startTransactionOnPageLoad && isPageLoadNavigation) {
         const pageloadTransaction = getActiveTransaction();
         if (pageloadTransaction) {
-          pageloadTransaction.setName(transactionName, transactionSource);
+          if (pageloadTransaction.metadata.source !== 'custom') {
+            pageloadTransaction.setName(transactionName, transactionSource);
+          }
           pageloadTransaction.setData('params', data.params);
           pageloadTransaction.setData('query', data.query);
         }
diff --git a/packages/vue/test/router.test.ts b/packages/vue/test/router.test.ts
index d2e9248452a5..c4072616d9af 100644
--- a/packages/vue/test/router.test.ts
+++ b/packages/vue/test/router.test.ts
@@ -123,10 +123,11 @@ describe('vueRouterInstrumentation()', () => {
     ['initialPageloadRoute', 'unmatchedRoute', '/e8733846-20ac-488c-9871-a5cbcb647294', 'url'],
   ])(
     'should return instrumentation that instruments VueRouter.beforeEach(%s, %s) for pageloads',
-    (fromKey, toKey, _transactionName, _transactionSource) => {
+    (fromKey, toKey, transactionName, transactionSource) => {
       const mockedTxn = {
         setName: jest.fn(),
         setData: jest.fn(),
+        metadata: {},
       };
       const customMockStartTxn = { ...mockStartTransaction }.mockImplementation(_ => {
         return mockedTxn;
@@ -160,7 +161,7 @@ describe('vueRouterInstrumentation()', () => {
       beforeEachCallback(to, from, mockNext);
       expect(mockVueRouter.beforeEach).toHaveBeenCalledTimes(1);
 
-      expect(mockedTxn.setName).toHaveBeenCalledWith(_transactionName, _transactionSource);
+      expect(mockedTxn.setName).toHaveBeenCalledWith(transactionName, transactionSource);
       expect(mockedTxn.setData).toHaveBeenNthCalledWith(1, 'params', to.params);
       expect(mockedTxn.setData).toHaveBeenNthCalledWith(2, 'query', to.query);
 
@@ -168,6 +169,54 @@ describe('vueRouterInstrumentation()', () => {
     },
   );
 
+  it("doesn't overwrite a pageload transaction name it was set to custom before the router resolved the route", () => {
+    const mockedTxn = {
+      setName: jest.fn(),
+      setData: jest.fn(),
+      name: '',
+      metadata: {
+        source: 'url',
+      },
+    };
+    const customMockStartTxn = { ...mockStartTransaction }.mockImplementation(_ => {
+      return mockedTxn;
+    });
+    jest.spyOn(vueTracing, 'getActiveTransaction').mockImplementation(() => mockedTxn as unknown as Transaction);
+
+    // create instrumentation
+    const instrument = vueRouterInstrumentation(mockVueRouter);
+
+    // instrument
+    instrument(customMockStartTxn, true, true);
+
+    // check for transaction start
+    expect(customMockStartTxn).toHaveBeenCalledTimes(1);
+    expect(customMockStartTxn).toHaveBeenCalledWith({
+      name: '/',
+      metadata: {
+        source: 'url',
+      },
+      op: 'pageload',
+      tags: {
+        'routing.instrumentation': 'vue-router',
+      },
+    });
+
+    // now we give the transaction a custom name, thereby simulating what would
+    // happen when users use the `beforeNavigate` hook
+    mockedTxn.name = 'customTxnName';
+    mockedTxn.metadata.source = 'custom';
+
+    const beforeEachCallback = mockVueRouter.beforeEach.mock.calls[0][0];
+    beforeEachCallback(testRoutes['normalRoute1'], testRoutes['initialPageloadRoute'], mockNext);
+
+    expect(mockVueRouter.beforeEach).toHaveBeenCalledTimes(1);
+
+    expect(mockedTxn.setName).not.toHaveBeenCalled();
+    expect(mockedTxn.metadata.source).toEqual('custom');
+    expect(mockedTxn.name).toEqual('customTxnName');
+  });
+
   test.each([
     [undefined, 1],
     [false, 0],