From 17190a0534bbe42eb671b922ae2e9d5d878ccb09 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 12 Jul 2022 10:59:51 +0200 Subject: [PATCH 01/15] feat(angular): Add URL parameterization of transaction names set source to 'route' --- packages/angular/src/tracing.ts | 50 ++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index 73bf94c89f73..917bec08c2e9 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -1,5 +1,13 @@ import { AfterViewInit, Directive, Injectable, Input, NgModule, OnDestroy, OnInit } from '@angular/core'; -import { Event, NavigationEnd, NavigationStart, Router } from '@angular/router'; +import { + ActivatedRouteSnapshot, + ActivationEnd, + Event, + NavigationEnd, + NavigationStart, + Params, + Router, +} from '@angular/router'; import { getCurrentHub } from '@sentry/browser'; import { Span, Transaction, TransactionContext } from '@sentry/types'; import { getGlobalObject, logger, stripUrlQueryAndFragment, timestampWithMs } from '@sentry/utils'; @@ -83,6 +91,8 @@ export class TraceService implements OnDestroy { } if (activeTransaction) { + this._activeTransaction = activeTransaction; + if (this._routingSpan) { this._routingSpan.finish(); } @@ -101,6 +111,20 @@ export class TraceService implements OnDestroy { }), ); + public actEnd$: Observable = this._router.events.pipe( + filter(event => event instanceof ActivationEnd), + tap(() => { + const params = getParamsOfRoute(this._router.routerState.snapshot.root); + const route = getParameterizedRouteFromUrlAndParams(this._router.url, params); + + const transaction = this._activeTransaction; + if (transaction) { + transaction.setName(route); + transaction.setMetadata({ source: 'route' }); + } + }), + ); + public navEnd$: Observable = this._router.events.pipe( filter(event => event instanceof NavigationEnd), tap(() => { @@ -115,10 +139,13 @@ export class TraceService implements OnDestroy { ); private _routingSpan: Span | null = null; + private _activeTransaction?: Transaction; + private _subscription: Subscription = new Subscription(); public constructor(private readonly _router: Router) { this._subscription.add(this.navStart$.subscribe()); + this._subscription.add(this.actEnd$.subscribe()); this._subscription.add(this.navEnd$.subscribe()); } @@ -241,3 +268,24 @@ export function TraceMethodDecorator(): MethodDecorator { return descriptor; }; } + +function getParamsOfRoute(activatedRouteSnapshot: ActivatedRouteSnapshot): Params { + let params = {}; + const stack: ActivatedRouteSnapshot[] = [activatedRouteSnapshot]; + while (stack.length > 0) { + const route = stack.pop(); + params = { ...params, ...(route && route.params) }; + route && stack.push(...route.children); + } + return params; +} + +function getParameterizedRouteFromUrlAndParams(url: string, params: Params): string { + if (params && typeof params === 'object') { + const parameterized = Object.keys(params).reduce((prev, curr: string) => { + return prev.replace(params[curr], `:${curr}`); + }, url); + return parameterized; + } + return url; +} From f9ab851adcff8691dd9b3b9ca6522bcf92403ac4 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Jul 2022 11:01:07 +0200 Subject: [PATCH 02/15] add unit tests for URL parameterization --- packages/angular/src/tracing.ts | 1 - packages/angular/test/tracing.test.ts | 107 ++++++++++++++++++++++++-- 2 files changed, 100 insertions(+), 8 deletions(-) diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index 917bec08c2e9..0df8003c4087 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -92,7 +92,6 @@ export class TraceService implements OnDestroy { if (activeTransaction) { this._activeTransaction = activeTransaction; - if (this._routingSpan) { this._routingSpan.finish(); } diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index 403c9b7c3cf1..43bca9d6e202 100644 --- a/packages/angular/test/tracing.test.ts +++ b/packages/angular/test/tracing.test.ts @@ -1,10 +1,17 @@ -import { NavigationStart, Router, RouterEvent } from '@angular/router'; +import { ActivatedRouteSnapshot, ActivationEnd, Event, NavigationStart, Router } from '@angular/router'; import { Subject } from 'rxjs'; import { instrumentAngularRouting, TraceService } from '../src/index'; +type MockedRouter = Router & { + setMockParams: (params: any) => void; + setMockUrl: (url: string) => void; + mockUrl: string; +}; + describe('Angular Tracing', () => { const startTransaction = jest.fn(); + describe('instrumentAngularRouting', () => { it('should attach the transaction source on the pageload transaction', () => { instrumentAngularRouting(startTransaction); @@ -18,15 +25,36 @@ describe('Angular Tracing', () => { describe('TraceService', () => { let traceService: TraceService; - const routerEvents$: Subject = new Subject(); + const routerEvents$: Subject = new Subject(); + const mockedRouter = { + events: routerEvents$, + routerState: { + snapshot: { + root: { + params: {}, + children: [], + }, + }, + }, + + // router.url is readonly originally.Using a getter lets us return a previously changed URL + get url() { + return mockedRouter.mockUrl; + }, + + setMockParams: (params: any) => { + mockedRouter.routerState.snapshot.root.params = params; + }, + + setMockUrl: (url: string) => { + mockedRouter.mockUrl = url; + }, + } as unknown as MockedRouter; - beforeAll(() => instrumentAngularRouting(startTransaction)); beforeEach(() => { + instrumentAngularRouting(startTransaction); jest.resetAllMocks(); - - traceService = new TraceService({ - events: routerEvents$, - } as unknown as Router); + traceService = new TraceService(mockedRouter); }); afterEach(() => { @@ -43,5 +71,70 @@ describe('Angular Tracing', () => { metadata: { source: 'url' }, }); }); + + describe('URL parameterization', () => { + // TODO: These tests are real unit tests in the sense that they only test TraceService + // and we essentially just simulate a router navigation by firing the respective + // routing events and providing the raw URL + the resolved route parameters. + // In the future we should add more "wholesome" tests that let the Angular router + // do its thing (e.g. by calling router.navigate) and we check how our service + // reacts to it. + // Once we set up Jest for testing Angular, we can use TestBed to inject an actual + // router instance into TraceService and add more tests. + it.each([ + ['does not parameterize static routes', '/books/', {}, '/books/'], + ['parameterizes number IDs in the URL', '/books/1/details', { bookId: '1' }, '/books/:bookId/details'], + [ + 'parameterizes string IDs in the URL', + '/books/asd123/details', + { bookId: 'asd123' }, + '/books/:bookId/details', + ], + [ + 'parameterizes UUID4 IDs in the URL', + '/books/04bc6846-4a1e-4af5-984a-003258f33e31/details', + { bookId: '04bc6846-4a1e-4af5-984a-003258f33e31' }, + '/books/:bookId/details', + ], + [ + 'parameterizes multiple IDs in the URL', + '/org/sentry/projects/1234/events/04bc6846-4a1e-4af5-984a-003258f33e31', + { orgId: 'sentry', projId: '1234', eventId: '04bc6846-4a1e-4af5-984a-003258f33e31' }, + '/org/:orgId/projects/:projId/events/:eventId', + ], + ])('%s and sets the source to `route`', (_, url, params, result) => { + const transaction: any = { + setName: jest.fn(name => (transaction.name = name)), + setMetadata: jest.fn(metadata => (transaction.metadata = metadata)), + }; + + const customStartTransaction = jest.fn((ctx: any) => { + transaction.name = ctx.name; + transaction.op = ctx.op; + transaction.metadata = ctx.metadata; + return transaction; + }); + + instrumentAngularRouting(customStartTransaction); + + mockedRouter.setMockParams(params); + mockedRouter.setMockUrl(url); + + // this event starts the transaction + routerEvents$.next(new NavigationStart(0, url)); + + expect(customStartTransaction).toHaveBeenCalledWith({ + name: url, + op: 'navigation', + metadata: { source: 'url' }, + }); + + // this event starts the parameterization + routerEvents$.next(new ActivationEnd(new ActivatedRouteSnapshot())); + + expect(transaction.setName).toHaveBeenCalledWith(result); + expect(transaction.setMetadata).toHaveBeenCalledWith({ source: 'route' }); + }); + }); }); }); From 634e9062026aabe1c199f1a3d73277fbf6f2e355 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Jul 2022 12:34:12 +0200 Subject: [PATCH 03/15] use ResolveEnd event instead of ActivationEnd --- packages/angular/src/tracing.ts | 33 ++++++++++++++++----- packages/angular/test/tracing.test.ts | 42 ++++----------------------- 2 files changed, 32 insertions(+), 43 deletions(-) diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index 0df8003c4087..43234f760d48 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -1,11 +1,12 @@ +/* eslint-disable max-lines */ import { AfterViewInit, Directive, Injectable, Input, NgModule, OnDestroy, OnInit } from '@angular/core'; import { ActivatedRouteSnapshot, - ActivationEnd, Event, NavigationEnd, NavigationStart, Params, + ResolveEnd, Router, } from '@angular/router'; import { getCurrentHub } from '@sentry/browser'; @@ -110,11 +111,29 @@ export class TraceService implements OnDestroy { }), ); - public actEnd$: Observable = this._router.events.pipe( - filter(event => event instanceof ActivationEnd), - tap(() => { - const params = getParamsOfRoute(this._router.routerState.snapshot.root); - const route = getParameterizedRouteFromUrlAndParams(this._router.url, params); + // The ResolveEnd event is fired when the Angular router has resolved the URL + // and activated the route. It holds the new resolved router state and the + // new URL. + // Only After this event, the route is activated, meaning that the transaction + // can be updated with the parameterized route name before e.g. the route's root + // component is initialized. This should be early enough before outgoing requests + // are made from the new route. + // In any case, this is the earliest stage where we can reliably get a resolved + // + public resEnd: Observable = this._router.events.pipe( + filter(event => event instanceof ResolveEnd), + tap(event => { + const ev = event as ResolveEnd; + + const params = getParamsOfRoute(ev.state.root); + + // ev.urlAfterRedirects is the one we prefer because it should hold the most recent + // one that holds information about a redirect to another route if this was specified + // in the Angular router config. In case this doesn't exist (for whatever reason), + // we fall back to ev.url which holds the primarily resolved URL before a potential + // redirect. + const url = ev.urlAfterRedirects || ev.url; + const route = getParameterizedRouteFromUrlAndParams(url, params); const transaction = this._activeTransaction; if (transaction) { @@ -144,7 +163,7 @@ export class TraceService implements OnDestroy { public constructor(private readonly _router: Router) { this._subscription.add(this.navStart$.subscribe()); - this._subscription.add(this.actEnd$.subscribe()); + this._subscription.add(this.resEnd.subscribe()); this._subscription.add(this.navEnd$.subscribe()); } diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index 43bca9d6e202..2e754367146a 100644 --- a/packages/angular/test/tracing.test.ts +++ b/packages/angular/test/tracing.test.ts @@ -1,14 +1,8 @@ -import { ActivatedRouteSnapshot, ActivationEnd, Event, NavigationStart, Router } from '@angular/router'; +import { Event, NavigationStart, ResolveEnd, Router } from '@angular/router'; import { Subject } from 'rxjs'; import { instrumentAngularRouting, TraceService } from '../src/index'; -type MockedRouter = Router & { - setMockParams: (params: any) => void; - setMockUrl: (url: string) => void; - mockUrl: string; -}; - describe('Angular Tracing', () => { const startTransaction = jest.fn(); @@ -26,35 +20,14 @@ describe('Angular Tracing', () => { describe('TraceService', () => { let traceService: TraceService; const routerEvents$: Subject = new Subject(); - const mockedRouter = { + const mockedRouter: Partial = { events: routerEvents$, - routerState: { - snapshot: { - root: { - params: {}, - children: [], - }, - }, - }, - - // router.url is readonly originally.Using a getter lets us return a previously changed URL - get url() { - return mockedRouter.mockUrl; - }, - - setMockParams: (params: any) => { - mockedRouter.routerState.snapshot.root.params = params; - }, - - setMockUrl: (url: string) => { - mockedRouter.mockUrl = url; - }, - } as unknown as MockedRouter; + }; beforeEach(() => { instrumentAngularRouting(startTransaction); jest.resetAllMocks(); - traceService = new TraceService(mockedRouter); + traceService = new TraceService(mockedRouter as Router); }); afterEach(() => { @@ -82,7 +55,7 @@ describe('Angular Tracing', () => { // Once we set up Jest for testing Angular, we can use TestBed to inject an actual // router instance into TraceService and add more tests. it.each([ - ['does not parameterize static routes', '/books/', {}, '/books/'], + ['does not alter static routes', '/books/', {}, '/books/'], ['parameterizes number IDs in the URL', '/books/1/details', { bookId: '1' }, '/books/:bookId/details'], [ 'parameterizes string IDs in the URL', @@ -117,9 +90,6 @@ describe('Angular Tracing', () => { instrumentAngularRouting(customStartTransaction); - mockedRouter.setMockParams(params); - mockedRouter.setMockUrl(url); - // this event starts the transaction routerEvents$.next(new NavigationStart(0, url)); @@ -130,7 +100,7 @@ describe('Angular Tracing', () => { }); // this event starts the parameterization - routerEvents$.next(new ActivationEnd(new ActivatedRouteSnapshot())); + routerEvents$.next(new ResolveEnd(1, url, url, { root: { params, children: [] } } as any)); expect(transaction.setName).toHaveBeenCalledWith(result); expect(transaction.setMetadata).toHaveBeenCalledWith({ source: 'route' }); From 190963d93b6c5eabcb573c4095906b249eb51473 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Jul 2022 12:59:41 +0200 Subject: [PATCH 04/15] only update transaction name if its source is a raw URL --- packages/angular/src/tracing.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index 43234f760d48..01d3bfd79e0d 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -136,7 +136,7 @@ export class TraceService implements OnDestroy { const route = getParameterizedRouteFromUrlAndParams(url, params); const transaction = this._activeTransaction; - if (transaction) { + if (transaction && transaction.metadata.source === 'url') { transaction.setName(route); transaction.setMetadata({ source: 'route' }); } From 3ce61c899caa6f58d69a91fb9c6baddd892308bc Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Jul 2022 13:12:00 +0200 Subject: [PATCH 05/15] add test --- packages/angular/test/tracing.test.ts | 53 +++++++++++++++++++++------ 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index 2e754367146a..4309bb9c10a3 100644 --- a/packages/angular/test/tracing.test.ts +++ b/packages/angular/test/tracing.test.ts @@ -54,6 +54,23 @@ describe('Angular Tracing', () => { // reacts to it. // Once we set up Jest for testing Angular, we can use TestBed to inject an actual // router instance into TraceService and add more tests. + + let transaction: any; + let customStartTransaction: any; + beforeEach(() => { + transaction = { + setName: jest.fn(name => (transaction.name = name)), + setMetadata: jest.fn(metadata => (transaction.metadata = metadata)), + }; + + customStartTransaction = jest.fn((ctx: any) => { + transaction.name = ctx.name; + transaction.op = ctx.op; + transaction.metadata = ctx.metadata; + return transaction; + }); + }); + it.each([ ['does not alter static routes', '/books/', {}, '/books/'], ['parameterizes number IDs in the URL', '/books/1/details', { bookId: '1' }, '/books/:bookId/details'], @@ -76,18 +93,6 @@ describe('Angular Tracing', () => { '/org/:orgId/projects/:projId/events/:eventId', ], ])('%s and sets the source to `route`', (_, url, params, result) => { - const transaction: any = { - setName: jest.fn(name => (transaction.name = name)), - setMetadata: jest.fn(metadata => (transaction.metadata = metadata)), - }; - - const customStartTransaction = jest.fn((ctx: any) => { - transaction.name = ctx.name; - transaction.op = ctx.op; - transaction.metadata = ctx.metadata; - return transaction; - }); - instrumentAngularRouting(customStartTransaction); // this event starts the transaction @@ -105,6 +110,30 @@ describe('Angular Tracing', () => { expect(transaction.setName).toHaveBeenCalledWith(result); expect(transaction.setMetadata).toHaveBeenCalledWith({ source: 'route' }); }); + + it('does not change the transaction name if the source is something other than `url`', () => { + instrumentAngularRouting(customStartTransaction); + + const url = '/user/12345/test'; + + routerEvents$.next(new NavigationStart(0, url)); + + expect(customStartTransaction).toHaveBeenCalledWith({ + name: url, + op: 'navigation', + metadata: { source: 'url' }, + }); + + // Simulate that this transaction has a custom name: + transaction.metadata.source = 'custom'; + + // this event starts the parameterization + routerEvents$.next(new ResolveEnd(1, url, url, { root: { params: { userId: '12345' }, children: [] } } as any)); + + expect(transaction.setName).toHaveBeenCalledTimes(0); + expect(transaction.setMetadata).toHaveBeenCalledTimes(0); + expect(transaction.name).toEqual(url); + }); }); }); }); From 1395a34e5a9f6db73d7631f7d9087ac218cfc042 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 13 Jul 2022 13:13:57 +0200 Subject: [PATCH 06/15] fix naming typo --- packages/angular/src/tracing.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index 01d3bfd79e0d..db7135cc39a4 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -120,7 +120,7 @@ export class TraceService implements OnDestroy { // are made from the new route. // In any case, this is the earliest stage where we can reliably get a resolved // - public resEnd: Observable = this._router.events.pipe( + public resEnd$: Observable = this._router.events.pipe( filter(event => event instanceof ResolveEnd), tap(event => { const ev = event as ResolveEnd; @@ -163,7 +163,7 @@ export class TraceService implements OnDestroy { public constructor(private readonly _router: Router) { this._subscription.add(this.navStart$.subscribe()); - this._subscription.add(this.resEnd.subscribe()); + this._subscription.add(this.resEnd$.subscribe()); this._subscription.add(this.navEnd$.subscribe()); } From 23242aa88d02054c0f128c04389e0da476cc77ea Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 14 Jul 2022 09:58:05 +0200 Subject: [PATCH 07/15] apply first batch of review suggestions --- packages/angular/src/errorhandler.ts | 5 +++-- packages/angular/src/tracing.ts | 11 +++++++---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/angular/src/errorhandler.ts b/packages/angular/src/errorhandler.ts index 54449071ee89..1c622bebc545 100644 --- a/packages/angular/src/errorhandler.ts +++ b/packages/angular/src/errorhandler.ts @@ -1,6 +1,7 @@ import { HttpErrorResponse } from '@angular/common/http'; import { ErrorHandler as AngularErrorHandler, Inject, Injectable } from '@angular/core'; import * as Sentry from '@sentry/browser'; +import { ReportDialogOptions, showReportDialog } from '@sentry/browser'; import { runOutsideAngular } from './zone'; @@ -10,7 +11,7 @@ import { runOutsideAngular } from './zone'; export interface ErrorHandlerOptions { logErrors?: boolean; showDialog?: boolean; - dialogOptions?: Sentry.ReportDialogOptions; + dialogOptions?: ReportDialogOptions; /** * Custom implementation of error extraction from the raw value captured by the Angular. * @param error Value captured by Angular's ErrorHandler provider @@ -50,7 +51,7 @@ class SentryErrorHandler implements AngularErrorHandler { // Optionally show user dialog to provide details on what happened. if (this._options.showDialog) { - Sentry.showReportDialog({ ...this._options.dialogOptions, eventId }); + showReportDialog({ ...this._options.dialogOptions, eventId }); } } diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index db7135cc39a4..44b48f79b2b3 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -92,7 +92,6 @@ export class TraceService implements OnDestroy { } if (activeTransaction) { - this._activeTransaction = activeTransaction; if (this._routingSpan) { this._routingSpan.finish(); } @@ -135,7 +134,8 @@ export class TraceService implements OnDestroy { const url = ev.urlAfterRedirects || ev.url; const route = getParameterizedRouteFromUrlAndParams(url, params); - const transaction = this._activeTransaction; + const transaction = getActiveTransaction(); + // TODO (v8 / #5416): revisit the source condition. Do we want to make the parameterized route the default? if (transaction && transaction.metadata.source === 'url') { transaction.setName(route); transaction.setMetadata({ source: 'route' }); @@ -157,7 +157,6 @@ export class TraceService implements OnDestroy { ); private _routingSpan: Span | null = null; - private _activeTransaction?: Transaction; private _subscription: Subscription = new Subscription(); @@ -287,6 +286,8 @@ export function TraceMethodDecorator(): MethodDecorator { }; } +// TODO unit test +// TODO recursion function getParamsOfRoute(activatedRouteSnapshot: ActivatedRouteSnapshot): Params { let params = {}; const stack: ActivatedRouteSnapshot[] = [activatedRouteSnapshot]; @@ -298,8 +299,10 @@ function getParamsOfRoute(activatedRouteSnapshot: ActivatedRouteSnapshot): Param return params; } +// TODO unit tests +// TODO robustness function getParameterizedRouteFromUrlAndParams(url: string, params: Params): string { - if (params && typeof params === 'object') { + if (params) { const parameterized = Object.keys(params).reduce((prev, curr: string) => { return prev.replace(params[curr], `:${curr}`); }, url); From ca02d4968d152193bb7efe6df0a746d64fc16e32 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 14 Jul 2022 10:43:31 +0200 Subject: [PATCH 08/15] fix tests --- packages/angular/src/tracing.ts | 1 + packages/angular/test/tracing.test.ts | 40 +++++++++++++++++++++------ 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index 44b48f79b2b3..128ed45d6c81 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -132,6 +132,7 @@ export class TraceService implements OnDestroy { // we fall back to ev.url which holds the primarily resolved URL before a potential // redirect. const url = ev.urlAfterRedirects || ev.url; + const route = getParameterizedRouteFromUrlAndParams(url, params); const transaction = getActiveTransaction(); diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index 4309bb9c10a3..ae4054fab1c9 100644 --- a/packages/angular/test/tracing.test.ts +++ b/packages/angular/test/tracing.test.ts @@ -1,8 +1,33 @@ import { Event, NavigationStart, ResolveEnd, Router } from '@angular/router'; +import { Hub, Transaction } from '@sentry/types'; import { Subject } from 'rxjs'; import { instrumentAngularRouting, TraceService } from '../src/index'; +//import * as Sentry from '../src/index'; +// import { Transaction } from '@sentry/types'; +let transaction: any; +let customStartTransaction: (context: any) => Transaction | undefined; + +jest.mock('@sentry/browser', () => { + const original = jest.requireActual('@sentry/browser'); + + return { + ...original, + getCurrentHub: () => { + return { + getScope: () => { + return { + getTransaction: () => { + return transaction; + }, + }; + }, + } as unknown as Hub; + }, + }; +}); + describe('Angular Tracing', () => { const startTransaction = jest.fn(); @@ -55,18 +80,15 @@ describe('Angular Tracing', () => { // Once we set up Jest for testing Angular, we can use TestBed to inject an actual // router instance into TraceService and add more tests. - let transaction: any; - let customStartTransaction: any; beforeEach(() => { - transaction = { - setName: jest.fn(name => (transaction.name = name)), - setMetadata: jest.fn(metadata => (transaction.metadata = metadata)), - }; - + transaction = undefined; customStartTransaction = jest.fn((ctx: any) => { + transaction = {}; transaction.name = ctx.name; transaction.op = ctx.op; transaction.metadata = ctx.metadata; + transaction.setName = jest.fn(name => (transaction.name = name)); + transaction.setMetadata = jest.fn(metadata => (transaction.metadata = metadata)); return transaction; }); }); @@ -93,7 +115,7 @@ describe('Angular Tracing', () => { '/org/:orgId/projects/:projId/events/:eventId', ], ])('%s and sets the source to `route`', (_, url, params, result) => { - instrumentAngularRouting(customStartTransaction); + instrumentAngularRouting(customStartTransaction, false, true); // this event starts the transaction routerEvents$.next(new NavigationStart(0, url)); @@ -112,7 +134,7 @@ describe('Angular Tracing', () => { }); it('does not change the transaction name if the source is something other than `url`', () => { - instrumentAngularRouting(customStartTransaction); + instrumentAngularRouting(customStartTransaction, false, true); const url = '/user/12345/test'; From 9d5b14e5555b6f827a86ad4cfbc96a36f4a7629d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 14 Jul 2022 12:49:45 +0200 Subject: [PATCH 09/15] make parameter extraction and parameterization more robust --- packages/angular/src/tracing.ts | 60 ++++++++++++-------- packages/angular/test/tracing.test.ts | 82 ++++++++++++++++++++++++--- 2 files changed, 109 insertions(+), 33 deletions(-) diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index 128ed45d6c81..047b7cb90c2d 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -1,14 +1,6 @@ /* eslint-disable max-lines */ import { AfterViewInit, Directive, Injectable, Input, NgModule, OnDestroy, OnInit } from '@angular/core'; -import { - ActivatedRouteSnapshot, - Event, - NavigationEnd, - NavigationStart, - Params, - ResolveEnd, - Router, -} from '@angular/router'; +import { ActivatedRouteSnapshot, Event, NavigationEnd, NavigationStart, ResolveEnd, Router } from '@angular/router'; import { getCurrentHub } from '@sentry/browser'; import { Span, Transaction, TransactionContext } from '@sentry/types'; import { getGlobalObject, logger, stripUrlQueryAndFragment, timestampWithMs } from '@sentry/utils'; @@ -19,6 +11,8 @@ import { ANGULAR_INIT_OP, ANGULAR_OP, ANGULAR_ROUTING_OP } from './constants'; import { IS_DEBUG_BUILD } from './flags'; import { runOutsideAngular } from './zone'; +type ParamMap = { [key: string]: string[] }; + let instrumentationInitialized: boolean; let stashedStartTransaction: (context: TransactionContext) => Transaction | undefined; let stashedStartTransactionOnLocationChange: boolean; @@ -287,27 +281,43 @@ export function TraceMethodDecorator(): MethodDecorator { }; } -// TODO unit test -// TODO recursion -function getParamsOfRoute(activatedRouteSnapshot: ActivatedRouteSnapshot): Params { - let params = {}; - const stack: ActivatedRouteSnapshot[] = [activatedRouteSnapshot]; - while (stack.length > 0) { - const route = stack.pop(); - params = { ...params, ...(route && route.params) }; - route && stack.push(...route.children); +/** + * Recursively traverses the routerstate and its children to collect a map of parameters on the entire route. + * + * Because Angular supports child routes (e.g. when creating nested routes or lazy loaded routes), we have + * to not only visit the root router snapshot but also its children to get all parameters of the entire route. + * + * @param activatedRouteSnapshot the root router snapshot + * @returns a map of params, mapping from a key to an array of values + */ +export function getParamsOfRoute(activatedRouteSnapshot: ActivatedRouteSnapshot): ParamMap { + function getParamsOfRouteH(routeSnapshot: ActivatedRouteSnapshot, accParams: ParamMap): ParamMap { + Object.keys(routeSnapshot.params).forEach(key => { + accParams[key] = [...(accParams[key] || []), routeSnapshot.params[key]]; + }); + routeSnapshot.children.forEach(child => getParamsOfRouteH(child, accParams)); + return accParams; } - return params; + + return getParamsOfRouteH(activatedRouteSnapshot, {}); } -// TODO unit tests -// TODO robustness -function getParameterizedRouteFromUrlAndParams(url: string, params: Params): string { +/** + * Takes a raw URl and a map of params and replaces each values occuring in the raw URL with + * the name of the parameter. + * + * @param url raw URL (e.g. /user/1234/details) + * @param params a map of type ParamMap + * @returns the parameterized URL (e.g. /user/:userId/details) + */ +export function getParameterizedRouteFromUrlAndParams(url: string, params: ParamMap): string { if (params) { - const parameterized = Object.keys(params).reduce((prev, curr: string) => { - return prev.replace(params[curr], `:${curr}`); + return Object.keys(params).reduce((prevUrl: string, paramName: string) => { + return prevUrl + .split('/') + .map(segment => (params[paramName].find(p => p === segment) ? `:${paramName}` : segment)) + .join('/'); }, url); - return parameterized; } return url; } diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index ae4054fab1c9..21dddcc0f211 100644 --- a/packages/angular/test/tracing.test.ts +++ b/packages/angular/test/tracing.test.ts @@ -1,8 +1,9 @@ -import { Event, NavigationStart, ResolveEnd, Router } from '@angular/router'; +import { ActivatedRouteSnapshot, Event, NavigationStart, ResolveEnd, Router } from '@angular/router'; import { Hub, Transaction } from '@sentry/types'; import { Subject } from 'rxjs'; import { instrumentAngularRouting, TraceService } from '../src/index'; +import { getParameterizedRouteFromUrlAndParams, getParamsOfRoute } from '../src/tracing'; //import * as Sentry from '../src/index'; // import { Transaction } from '@sentry/types'; @@ -11,7 +12,6 @@ let customStartTransaction: (context: any) => Transaction | undefined; jest.mock('@sentry/browser', () => { const original = jest.requireActual('@sentry/browser'); - return { ...original, getCurrentHub: () => { @@ -83,12 +83,11 @@ describe('Angular Tracing', () => { beforeEach(() => { transaction = undefined; customStartTransaction = jest.fn((ctx: any) => { - transaction = {}; - transaction.name = ctx.name; - transaction.op = ctx.op; - transaction.metadata = ctx.metadata; - transaction.setName = jest.fn(name => (transaction.name = name)); - transaction.setMetadata = jest.fn(metadata => (transaction.metadata = metadata)); + transaction = { + ...ctx, + setName: jest.fn(name => (transaction.name = name)), + setMetadata: jest.fn(metadata => (transaction.metadata = metadata)), + }; return transaction; }); }); @@ -158,4 +157,71 @@ describe('Angular Tracing', () => { }); }); }); + + describe('getParamsOfRoute', () => { + it.each([ + ['returns an empty object if the route has no params', { params: {}, children: [] }, {}], + [ + 'returns an params of a route without children', + { params: { bookId: '1234' }, children: [] }, + { bookId: ['1234'] }, + ], + [ + 'returns an params of a route with children', + { + params: { bookId: '1234' }, + children: [ + { params: { authorId: 'abc' }, children: [] }, + { params: { pageNum: '66' }, children: [{ params: { userId: 'xyz' }, children: [] }] }, + ], + }, + { bookId: ['1234'], authorId: ['abc'], pageNum: ['66'], userId: ['xyz'] }, + ], + [ + 'returns an params of a route with children where param names are identical', + { + params: { id: '1234' }, + children: [ + { params: { id: 'abc' }, children: [] }, + { params: { pageNum: '66' }, children: [{ params: { id: 'xyz', somethingElse: '789' }, children: [] }] }, + ], + }, + { id: ['1234', 'abc', 'xyz'], pageNum: ['66'], somethingElse: ['789'] }, + ], + ])('%s', (_, routeSnapshot, expectedParams) => { + expect(getParamsOfRoute(routeSnapshot as unknown as ActivatedRouteSnapshot)).toEqual(expectedParams); + }); + }); + + describe('getParameterizedRouteFromUrlAndParams', () => { + it.each([ + ['returns untouched URL if no params are passed', '/static/route', {}, '/static/route'], + [ + 'returns parameterized URL if params are passed', + '/books/1234/read/0', + { bookId: ['1234'], pageNum: ['0'] }, + '/books/:bookId/read/:pageNum', + ], + [ + 'does not replace partially matching URL segments', + '/books/1234/read/0', + { wrongId: ['12'] }, + '/books/1234/read/0', + ], + [ + 'does not replace partially matching URL segments', + '/books/1234/read/04', + { wrongId: ['12'], bookId: ['1234'], pageNum: ['4'] }, + '/books/:bookId/read/04', + ], + [ + 'correctly matches URLs containing multiple identical param names', + '/books/1234/read/4/abc/test/123', + { id: ['1234', '123', 'abc'], pageNum: ['4'] }, + '/books/:id/read/:pageNum/:id/test/:id', + ], + ])('%s', (_, url, params, expectedRoute) => { + expect(getParameterizedRouteFromUrlAndParams(url, params)).toEqual(expectedRoute); + }); + }); }); From 856ba221e18b2bab6d388233e299b83ff6ff9362 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 14 Jul 2022 12:55:11 +0200 Subject: [PATCH 10/15] fix linter errors --- packages/angular/test/tracing.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index 21dddcc0f211..272575fcd5b3 100644 --- a/packages/angular/test/tracing.test.ts +++ b/packages/angular/test/tracing.test.ts @@ -5,8 +5,6 @@ import { Subject } from 'rxjs'; import { instrumentAngularRouting, TraceService } from '../src/index'; import { getParameterizedRouteFromUrlAndParams, getParamsOfRoute } from '../src/tracing'; -//import * as Sentry from '../src/index'; -// import { Transaction } from '@sentry/types'; let transaction: any; let customStartTransaction: (context: any) => Transaction | undefined; From be90bdc2a19ff5db851473616f4132c7f6ce545d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 14 Jul 2022 12:57:07 +0200 Subject: [PATCH 11/15] revert unintended changes --- packages/angular/src/errorhandler.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/angular/src/errorhandler.ts b/packages/angular/src/errorhandler.ts index 1c622bebc545..54449071ee89 100644 --- a/packages/angular/src/errorhandler.ts +++ b/packages/angular/src/errorhandler.ts @@ -1,7 +1,6 @@ import { HttpErrorResponse } from '@angular/common/http'; import { ErrorHandler as AngularErrorHandler, Inject, Injectable } from '@angular/core'; import * as Sentry from '@sentry/browser'; -import { ReportDialogOptions, showReportDialog } from '@sentry/browser'; import { runOutsideAngular } from './zone'; @@ -11,7 +10,7 @@ import { runOutsideAngular } from './zone'; export interface ErrorHandlerOptions { logErrors?: boolean; showDialog?: boolean; - dialogOptions?: ReportDialogOptions; + dialogOptions?: Sentry.ReportDialogOptions; /** * Custom implementation of error extraction from the raw value captured by the Angular. * @param error Value captured by Angular's ErrorHandler provider @@ -51,7 +50,7 @@ class SentryErrorHandler implements AngularErrorHandler { // Optionally show user dialog to provide details on what happened. if (this._options.showDialog) { - showReportDialog({ ...this._options.dialogOptions, eventId }); + Sentry.showReportDialog({ ...this._options.dialogOptions, eventId }); } } From 35b9ecae47ee43722fd29602509b60d4ef7c5ff6 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 14 Jul 2022 14:07:07 +0200 Subject: [PATCH 12/15] adjust comment --- packages/angular/src/tracing.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index 047b7cb90c2d..c706a61e7f00 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -104,15 +104,14 @@ export class TraceService implements OnDestroy { }), ); - // The ResolveEnd event is fired when the Angular router has resolved the URL - // and activated the route. It holds the new resolved router state and the - // new URL. + // The ResolveEnd event is fired when the Angular router has resolved the URL and + // the parameter<->value mapping. It holds the new resolved router state with + // the mapping and the new URL. // Only After this event, the route is activated, meaning that the transaction // can be updated with the parameterized route name before e.g. the route's root // component is initialized. This should be early enough before outgoing requests - // are made from the new route. - // In any case, this is the earliest stage where we can reliably get a resolved - // + // are made from the new route, with the exceptions of requests being made during + // a navigation. public resEnd$: Observable = this._router.events.pipe( filter(event => event instanceof ResolveEnd), tap(event => { From 195fe33df1e42383cc05a1a58b607d9b8728bf15 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 14 Jul 2022 14:44:48 +0200 Subject: [PATCH 13/15] apply code review suggestions --- packages/angular/src/tracing.ts | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index c706a61e7f00..25e792582625 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -65,15 +65,14 @@ export function getActiveTransaction(): Transaction | undefined { @Injectable({ providedIn: 'root' }) export class TraceService implements OnDestroy { public navStart$: Observable = this._router.events.pipe( - filter(event => event instanceof NavigationStart), - tap(event => { + filter((event): event is NavigationStart => event instanceof NavigationStart), + tap(navigationEvent => { if (!instrumentationInitialized) { IS_DEBUG_BUILD && logger.error('Angular integration has tracing enabled, but Tracing integration is not configured'); return; } - const navigationEvent = event as NavigationStart; const strippedUrl = stripUrlQueryAndFragment(navigationEvent.url); let activeTransaction = getActiveTransaction(); @@ -113,18 +112,16 @@ export class TraceService implements OnDestroy { // are made from the new route, with the exceptions of requests being made during // a navigation. public resEnd$: Observable = this._router.events.pipe( - filter(event => event instanceof ResolveEnd), + filter((event): event is ResolveEnd => event instanceof ResolveEnd), tap(event => { - const ev = event as ResolveEnd; - - const params = getParamsOfRoute(ev.state.root); + const params = getParamsOfRoute(event.state.root); - // ev.urlAfterRedirects is the one we prefer because it should hold the most recent + // event.urlAfterRedirects is the one we prefer because it should hold the most recent // one that holds information about a redirect to another route if this was specified // in the Angular router config. In case this doesn't exist (for whatever reason), - // we fall back to ev.url which holds the primarily resolved URL before a potential + // we fall back to event.url which holds the primarily resolved URL before a potential // redirect. - const url = ev.urlAfterRedirects || ev.url; + const url = event.urlAfterRedirects || event.url; const route = getParameterizedRouteFromUrlAndParams(url, params); From ffaf7f15882343f0bce130b69620c1c6fc7b538d Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 15 Jul 2022 09:44:29 +0200 Subject: [PATCH 14/15] Change parameterization approach --- packages/angular/src/tracing.ts | 57 +++-------- packages/angular/test/tracing.test.ts | 137 +++++++++++++------------- 2 files changed, 80 insertions(+), 114 deletions(-) diff --git a/packages/angular/src/tracing.ts b/packages/angular/src/tracing.ts index 25e792582625..56dc1cb60525 100644 --- a/packages/angular/src/tracing.ts +++ b/packages/angular/src/tracing.ts @@ -11,8 +11,6 @@ import { ANGULAR_INIT_OP, ANGULAR_OP, ANGULAR_ROUTING_OP } from './constants'; import { IS_DEBUG_BUILD } from './flags'; import { runOutsideAngular } from './zone'; -type ParamMap = { [key: string]: string[] }; - let instrumentationInitialized: boolean; let stashedStartTransaction: (context: TransactionContext) => Transaction | undefined; let stashedStartTransactionOnLocationChange: boolean; @@ -114,16 +112,7 @@ export class TraceService implements OnDestroy { public resEnd$: Observable = this._router.events.pipe( filter((event): event is ResolveEnd => event instanceof ResolveEnd), tap(event => { - const params = getParamsOfRoute(event.state.root); - - // event.urlAfterRedirects is the one we prefer because it should hold the most recent - // one that holds information about a redirect to another route if this was specified - // in the Angular router config. In case this doesn't exist (for whatever reason), - // we fall back to event.url which holds the primarily resolved URL before a potential - // redirect. - const url = event.urlAfterRedirects || event.url; - - const route = getParameterizedRouteFromUrlAndParams(url, params); + const route = getParameterizedRouteFromSnapshot(event.state.root); const transaction = getActiveTransaction(); // TODO (v8 / #5416): revisit the source condition. Do we want to make the parameterized route the default? @@ -278,42 +267,18 @@ export function TraceMethodDecorator(): MethodDecorator { } /** - * Recursively traverses the routerstate and its children to collect a map of parameters on the entire route. + * Takes the parameterized route from a given ActivatedRouteSnapshot and concatenates the snapshot's + * child route with its parent to produce the complete parameterized URL of the activated route. + * This happens recursively until the last child (i.e. the end of the URL) is reached. * - * Because Angular supports child routes (e.g. when creating nested routes or lazy loaded routes), we have - * to not only visit the root router snapshot but also its children to get all parameters of the entire route. - * - * @param activatedRouteSnapshot the root router snapshot - * @returns a map of params, mapping from a key to an array of values - */ -export function getParamsOfRoute(activatedRouteSnapshot: ActivatedRouteSnapshot): ParamMap { - function getParamsOfRouteH(routeSnapshot: ActivatedRouteSnapshot, accParams: ParamMap): ParamMap { - Object.keys(routeSnapshot.params).forEach(key => { - accParams[key] = [...(accParams[key] || []), routeSnapshot.params[key]]; - }); - routeSnapshot.children.forEach(child => getParamsOfRouteH(child, accParams)); - return accParams; - } - - return getParamsOfRouteH(activatedRouteSnapshot, {}); -} - -/** - * Takes a raw URl and a map of params and replaces each values occuring in the raw URL with - * the name of the parameter. + * @param route the ActivatedRouteSnapshot of which its path and its child's path is concantenated * - * @param url raw URL (e.g. /user/1234/details) - * @param params a map of type ParamMap - * @returns the parameterized URL (e.g. /user/:userId/details) + * @returns the concatenated parameterzited route string */ -export function getParameterizedRouteFromUrlAndParams(url: string, params: ParamMap): string { - if (params) { - return Object.keys(params).reduce((prevUrl: string, paramName: string) => { - return prevUrl - .split('/') - .map(segment => (params[paramName].find(p => p === segment) ? `:${paramName}` : segment)) - .join('/'); - }, url); +export function getParameterizedRouteFromSnapshot(route?: ActivatedRouteSnapshot | null): string { + const path = route && route.firstChild && route.firstChild.routeConfig && route.firstChild.routeConfig.path; + if (!path) { + return '/'; } - return url; + return `/${path}${getParameterizedRouteFromSnapshot(route && route.firstChild)}`; } diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index 272575fcd5b3..dee5599f8562 100644 --- a/packages/angular/test/tracing.test.ts +++ b/packages/angular/test/tracing.test.ts @@ -3,7 +3,7 @@ import { Hub, Transaction } from '@sentry/types'; import { Subject } from 'rxjs'; import { instrumentAngularRouting, TraceService } from '../src/index'; -import { getParameterizedRouteFromUrlAndParams, getParamsOfRoute } from '../src/tracing'; +import { getParameterizedRouteFromSnapshot } from '../src/tracing'; let transaction: any; let customStartTransaction: (context: any) => Transaction | undefined; @@ -91,27 +91,55 @@ describe('Angular Tracing', () => { }); it.each([ - ['does not alter static routes', '/books/', {}, '/books/'], - ['parameterizes number IDs in the URL', '/books/1/details', { bookId: '1' }, '/books/:bookId/details'], [ - 'parameterizes string IDs in the URL', - '/books/asd123/details', - { bookId: 'asd123' }, - '/books/:bookId/details', + 'handles the root URL correctly', + '', + { + root: { firstChild: { routeConfig: null } }, + }, + '/', ], [ - 'parameterizes UUID4 IDs in the URL', - '/books/04bc6846-4a1e-4af5-984a-003258f33e31/details', - { bookId: '04bc6846-4a1e-4af5-984a-003258f33e31' }, - '/books/:bookId/details', + 'does not alter static routes', + '/books/', + { + root: { firstChild: { routeConfig: { path: 'books' } } }, + }, + '/books/', + ], + [ + 'parameterizes IDs in the URL', + '/books/1/details', + { + root: { firstChild: { routeConfig: { path: 'books/:bookId/details' } } }, + }, + '/books/:bookId/details/', ], [ 'parameterizes multiple IDs in the URL', '/org/sentry/projects/1234/events/04bc6846-4a1e-4af5-984a-003258f33e31', - { orgId: 'sentry', projId: '1234', eventId: '04bc6846-4a1e-4af5-984a-003258f33e31' }, - '/org/:orgId/projects/:projId/events/:eventId', + { + root: { firstChild: { routeConfig: { path: 'org/:orgId/projects/:projId/events/:eventId' } } }, + }, + '/org/:orgId/projects/:projId/events/:eventId/', + ], + [ + 'parameterizes URLs from route with child routes', + '/org/sentry/projects/1234/events/04bc6846-4a1e-4af5-984a-003258f33e31', + { + root: { + firstChild: { + routeConfig: { path: 'org/:orgId' }, + firstChild: { + routeConfig: { path: 'projects/:projId' }, + firstChild: { routeConfig: { path: 'events/:eventId' } }, + }, + }, + }, + }, + '/org/:orgId/projects/:projId/events/:eventId/', ], - ])('%s and sets the source to `route`', (_, url, params, result) => { + ])('%s and sets the source to `route`', (_, url, routerState, result) => { instrumentAngularRouting(customStartTransaction, false, true); // this event starts the transaction @@ -124,7 +152,7 @@ describe('Angular Tracing', () => { }); // this event starts the parameterization - routerEvents$.next(new ResolveEnd(1, url, url, { root: { params, children: [] } } as any)); + routerEvents$.next(new ResolveEnd(1, url, url, routerState as any)); expect(transaction.setName).toHaveBeenCalledWith(result); expect(transaction.setMetadata).toHaveBeenCalledWith({ source: 'route' }); @@ -147,7 +175,11 @@ describe('Angular Tracing', () => { transaction.metadata.source = 'custom'; // this event starts the parameterization - routerEvents$.next(new ResolveEnd(1, url, url, { root: { params: { userId: '12345' }, children: [] } } as any)); + routerEvents$.next( + new ResolveEnd(1, url, url, { + root: { firstChild: { routeConfig: { path: 'org/:orgId/projects/:projId/events/:eventId' } } }, + } as any), + ); expect(transaction.setName).toHaveBeenCalledTimes(0); expect(transaction.setMetadata).toHaveBeenCalledTimes(0); @@ -156,70 +188,39 @@ describe('Angular Tracing', () => { }); }); - describe('getParamsOfRoute', () => { + describe('getParameterizedRouteFromSnapshot', () => { it.each([ - ['returns an empty object if the route has no params', { params: {}, children: [] }, {}], [ - 'returns an params of a route without children', - { params: { bookId: '1234' }, children: [] }, - { bookId: ['1234'] }, + 'returns `/` empty object if the route no children', + { + firstChild: { routeConfig: null }, + }, + '/', ], [ - 'returns an params of a route with children', + 'returns the route of a snapshot without children', { - params: { bookId: '1234' }, - children: [ - { params: { authorId: 'abc' }, children: [] }, - { params: { pageNum: '66' }, children: [{ params: { userId: 'xyz' }, children: [] }] }, - ], + firstChild: { routeConfig: { path: 'users/:id' } }, }, - { bookId: ['1234'], authorId: ['abc'], pageNum: ['66'], userId: ['xyz'] }, + '/users/:id/', ], [ - 'returns an params of a route with children where param names are identical', + 'returns the complete route of a snapshot with children', { - params: { id: '1234' }, - children: [ - { params: { id: 'abc' }, children: [] }, - { params: { pageNum: '66' }, children: [{ params: { id: 'xyz', somethingElse: '789' }, children: [] }] }, - ], + firstChild: { + routeConfig: { path: 'orgs/:orgId' }, + firstChild: { + routeConfig: { path: 'projects/:projId' }, + firstChild: { routeConfig: { path: 'overview' } }, + }, + }, }, - { id: ['1234', 'abc', 'xyz'], pageNum: ['66'], somethingElse: ['789'] }, + '/orgs/:orgId/projects/:projId/overview/', ], ])('%s', (_, routeSnapshot, expectedParams) => { - expect(getParamsOfRoute(routeSnapshot as unknown as ActivatedRouteSnapshot)).toEqual(expectedParams); - }); - }); - - describe('getParameterizedRouteFromUrlAndParams', () => { - it.each([ - ['returns untouched URL if no params are passed', '/static/route', {}, '/static/route'], - [ - 'returns parameterized URL if params are passed', - '/books/1234/read/0', - { bookId: ['1234'], pageNum: ['0'] }, - '/books/:bookId/read/:pageNum', - ], - [ - 'does not replace partially matching URL segments', - '/books/1234/read/0', - { wrongId: ['12'] }, - '/books/1234/read/0', - ], - [ - 'does not replace partially matching URL segments', - '/books/1234/read/04', - { wrongId: ['12'], bookId: ['1234'], pageNum: ['4'] }, - '/books/:bookId/read/04', - ], - [ - 'correctly matches URLs containing multiple identical param names', - '/books/1234/read/4/abc/test/123', - { id: ['1234', '123', 'abc'], pageNum: ['4'] }, - '/books/:id/read/:pageNum/:id/test/:id', - ], - ])('%s', (_, url, params, expectedRoute) => { - expect(getParameterizedRouteFromUrlAndParams(url, params)).toEqual(expectedRoute); + expect(getParameterizedRouteFromSnapshot(routeSnapshot as unknown as ActivatedRouteSnapshot)).toEqual( + expectedParams, + ); }); }); }); From a8b44b82331ca04f5ee49ed0ad9b37e110c68c95 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 15 Jul 2022 10:18:08 +0200 Subject: [PATCH 15/15] small test change --- packages/angular/test/tracing.test.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/angular/test/tracing.test.ts b/packages/angular/test/tracing.test.ts index dee5599f8562..ad4b67f2ecc9 100644 --- a/packages/angular/test/tracing.test.ts +++ b/packages/angular/test/tracing.test.ts @@ -190,13 +190,7 @@ describe('Angular Tracing', () => { describe('getParameterizedRouteFromSnapshot', () => { it.each([ - [ - 'returns `/` empty object if the route no children', - { - firstChild: { routeConfig: null }, - }, - '/', - ], + ['returns `/` empty object if the route no children', {}, '/'], [ 'returns the route of a snapshot without children', {