diff --git a/packages/sveltekit/src/client/load.ts b/packages/sveltekit/src/client/load.ts index aa288b9e2ddd..1996523346e2 100644 --- a/packages/sveltekit/src/client/load.ts +++ b/packages/sveltekit/src/client/load.ts @@ -6,6 +6,7 @@ import { captureException } from '@sentry/svelte'; import type { ClientOptions, SanitizedRequestData } from '@sentry/types'; import { addExceptionMechanism, + addNonEnumerableProperty, getSanitizedUrlString, objectify, parseFetchArgs, @@ -14,8 +15,11 @@ import { } from '@sentry/utils'; import type { LoadEvent } from '@sveltejs/kit'; +import type { SentryWrappedFlag } from '../common/utils'; import { isRedirect } from '../common/utils'; +type PatchedLoadEvent = LoadEvent & Partial; + function sendErrorToSentry(e: unknown): unknown { // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can // store a seen flag on it. @@ -66,13 +70,20 @@ export function wrapLoadWithSentry any>(origLoad: T) return new Proxy(origLoad, { apply: (wrappingTarget, thisArg, args: Parameters) => { // Type casting here because `T` cannot extend `Load` (see comment above function signature) - const event = args[0] as LoadEvent; + const event = args[0] as PatchedLoadEvent; - const patchedEvent = { + // Check if already wrapped + if (event.__sentry_wrapped__) { + return wrappingTarget.apply(thisArg, args); + } + + const patchedEvent: PatchedLoadEvent = { ...event, fetch: instrumentSvelteKitFetch(event.fetch), }; + addNonEnumerableProperty(patchedEvent as unknown as Record, '__sentry_wrapped__', true); + const routeId = event.route.id; return trace( { diff --git a/packages/sveltekit/src/common/utils.ts b/packages/sveltekit/src/common/utils.ts index 86035117b6f6..ecf762cee8c4 100644 --- a/packages/sveltekit/src/common/utils.ts +++ b/packages/sveltekit/src/common/utils.ts @@ -1,5 +1,13 @@ import type { Redirect } from '@sveltejs/kit'; +export type SentryWrappedFlag = { + /** + * If this flag is set, we know that the load event was already wrapped once + * and we shouldn't wrap it again. + */ + __sentry_wrapped__?: true; +}; + /** * Determines if a thrown "error" is a Redirect object which SvelteKit users can throw to redirect to another route * see: https://kit.svelte.dev/docs/modules#sveltejs-kit-redirect diff --git a/packages/sveltekit/src/server/load.ts b/packages/sveltekit/src/server/load.ts index a5350c6075c2..ab9f9ebdafb3 100644 --- a/packages/sveltekit/src/server/load.ts +++ b/packages/sveltekit/src/server/load.ts @@ -2,12 +2,16 @@ import { trace } from '@sentry/core'; import { captureException } from '@sentry/node'; import type { TransactionContext } from '@sentry/types'; -import { addExceptionMechanism, objectify } from '@sentry/utils'; +import { addExceptionMechanism, addNonEnumerableProperty, objectify } from '@sentry/utils'; import type { HttpError, LoadEvent, ServerLoadEvent } from '@sveltejs/kit'; +import type { SentryWrappedFlag } from '../common/utils'; import { isRedirect } from '../common/utils'; import { getTracePropagationData } from './utils'; +type PatchedLoadEvent = LoadEvent & SentryWrappedFlag; +type PatchedServerLoadEvent = ServerLoadEvent & SentryWrappedFlag; + function isHttpError(err: unknown): err is HttpError { return typeof err === 'object' && err !== null && 'status' in err && 'body' in err; } @@ -59,7 +63,15 @@ export function wrapLoadWithSentry any>(origLoad: T) return new Proxy(origLoad, { apply: (wrappingTarget, thisArg, args: Parameters) => { // Type casting here because `T` cannot extend `Load` (see comment above function signature) - const event = args[0] as LoadEvent; + // Also, this event possibly already has a sentry wrapped flag attached + const event = args[0] as PatchedLoadEvent; + + if (event.__sentry_wrapped__) { + return wrappingTarget.apply(thisArg, args); + } + + addNonEnumerableProperty(event as unknown as Record, '__sentry_wrapped__', true); + const routeId = event.route && event.route.id; const traceLoadContext: TransactionContext = { @@ -102,7 +114,15 @@ export function wrapServerLoadWithSentry any>(origSe return new Proxy(origServerLoad, { apply: (wrappingTarget, thisArg, args: Parameters) => { // Type casting here because `T` cannot extend `ServerLoad` (see comment above function signature) - const event = args[0] as ServerLoadEvent; + // Also, this event possibly already has a sentry wrapped flag attached + const event = args[0] as PatchedServerLoadEvent; + + if (event.__sentry_wrapped__) { + return wrappingTarget.apply(thisArg, args); + } + + addNonEnumerableProperty(event as unknown as Record, '__sentry_wrapped__', true); + const routeId = event.route && event.route.id; const { dynamicSamplingContext, traceparentData } = getTracePropagationData(event); diff --git a/packages/sveltekit/test/client/load.test.ts b/packages/sveltekit/test/client/load.test.ts index a6b5ebcbd278..b07e0c12108f 100644 --- a/packages/sveltekit/test/client/load.test.ts +++ b/packages/sveltekit/test/client/load.test.ts @@ -450,4 +450,17 @@ describe('wrapLoadWithSentry', () => { { handled: false, type: 'sveltekit', data: { function: 'load' } }, ); }); + + it("doesn't wrap load more than once if the wrapper was applied multiple times", async () => { + async function load({ params }: Parameters[0]): Promise> { + return { + post: params.id, + }; + } + + const wrappedLoad = wrapLoadWithSentry(wrapLoadWithSentry(load)); + await wrappedLoad(MOCK_LOAD_ARGS); + + expect(mockTrace).toHaveBeenCalledTimes(1); + }); }); diff --git a/packages/sveltekit/test/server/load.test.ts b/packages/sveltekit/test/server/load.test.ts index 4c57d6245451..fa57dc6b7c46 100644 --- a/packages/sveltekit/test/server/load.test.ts +++ b/packages/sveltekit/test/server/load.test.ts @@ -48,70 +48,80 @@ function getById(_id?: string) { throw new Error('error'); } -const MOCK_LOAD_ARGS: any = { - params: { id: '123' }, - route: { - id: '/users/[id]', - }, - url: new URL('http://localhost:3000/users/123'), -}; - -const MOCK_LOAD_NO_ROUTE_ARGS: any = { - params: { id: '123' }, - url: new URL('http://localhost:3000/users/123'), -}; - -const MOCK_SERVER_ONLY_LOAD_ARGS: any = { - ...MOCK_LOAD_ARGS, - request: { - method: 'GET', - headers: { - get: (key: string) => { - if (key === 'sentry-trace') { - return '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; - } - - if (key === 'baggage') { - return ( - 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' + - 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + - 'sentry-trace_id=1234567890abcdef1234567890abcdef,sentry-sample_rate=1' - ); - } - - return null; +function getLoadArgs() { + return { + params: { id: '123' }, + route: { + id: '/users/[id]', + }, + url: new URL('http://localhost:3000/users/123'), + }; +} + +function getLoadArgsWithoutRoute() { + return { + params: { id: '123' }, + url: new URL('http://localhost:3000/users/123'), + }; +} + +function getServerOnlyArgs() { + return { + ...getLoadArgs(), + request: { + method: 'GET', + headers: { + get: (key: string) => { + if (key === 'sentry-trace') { + return '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; + } + + if (key === 'baggage') { + return ( + 'sentry-environment=production,sentry-release=1.0.0,sentry-transaction=dogpark,' + + 'sentry-user_segment=segmentA,sentry-public_key=dogsarebadatkeepingsecrets,' + + 'sentry-trace_id=1234567890abcdef1234567890abcdef,sentry-sample_rate=1' + ); + } + + return null; + }, }, }, - }, -}; - -const MOCK_SERVER_ONLY_NO_TRACE_LOAD_ARGS: any = { - ...MOCK_LOAD_ARGS, - request: { - method: 'GET', - headers: { - get: (_: string) => { - return null; + }; +} + +function getServerArgsWithoutTracingHeaders() { + return { + ...getLoadArgs(), + request: { + method: 'GET', + headers: { + get: (_: string) => { + return null; + }, }, }, - }, -}; - -const MOCK_SERVER_ONLY_NO_BAGGAGE_LOAD_ARGS: any = { - ...MOCK_LOAD_ARGS, - request: { - method: 'GET', - headers: { - get: (key: string) => { - if (key === 'sentry-trace') { - return '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; - } - - return null; + }; +} + +function getServerArgsWithoutBaggageHeader() { + return { + ...getLoadArgs(), + request: { + method: 'GET', + headers: { + get: (key: string) => { + if (key === 'sentry-trace') { + return '1234567890abcdef1234567890abcdef-1234567890abcdef-1'; + } + + return null; + }, }, }, - }, -}; + }; +} beforeAll(() => { addTracingExtensions(); @@ -136,7 +146,7 @@ describe.each([ } const wrappedLoad = wrapLoadWithSentry(load); - const res = wrappedLoad(MOCK_LOAD_ARGS); + const res = wrappedLoad(getLoadArgs()); await expect(res).rejects.toThrow(); expect(mockCaptureException).toHaveBeenCalledTimes(1); @@ -162,7 +172,7 @@ describe.each([ } const wrappedLoad = wrapLoadWithSentry(load); - const res = wrappedLoad({ ...MOCK_LOAD_ARGS }); + const res = wrappedLoad(getLoadArgs()); await expect(res).rejects.toThrow(); expect(mockCaptureException).toHaveBeenCalledTimes(times); @@ -170,12 +180,12 @@ describe.each([ }); it("doesn't call captureException for thrown `Redirect`s", async () => { - async function load(_: Parameters[0]): Promise> { + async function load(_params: any): Promise> { throw redirect(300, 'other/route'); } const wrappedLoad = wrapLoadWithSentry(load); - const res = wrappedLoad(MOCK_LOAD_ARGS); + const res = wrappedLoad(getLoadArgs()); await expect(res).rejects.toThrow(); expect(mockCaptureException).not.toHaveBeenCalled(); @@ -194,7 +204,7 @@ describe.each([ } const wrappedLoad = sentryLoadWrapperFn.call(this, load); - const res = wrappedLoad(MOCK_SERVER_ONLY_LOAD_ARGS); + const res = wrappedLoad(getServerOnlyArgs()); await expect(res).rejects.toThrow(); expect(addEventProcessorSpy).toBeCalledTimes(1); @@ -206,7 +216,7 @@ describe.each([ }); }); describe('wrapLoadWithSentry calls trace', () => { - async function load({ params }: Parameters[0]): Promise> { + async function load({ params }): Promise> { return { post: params.id, }; @@ -214,7 +224,7 @@ describe('wrapLoadWithSentry calls trace', () => { it('with the context of the universal load function', async () => { const wrappedLoad = wrapLoadWithSentry(load); - await wrappedLoad(MOCK_LOAD_ARGS); + await wrappedLoad(getLoadArgs()); expect(mockTrace).toHaveBeenCalledTimes(1); expect(mockTrace).toHaveBeenCalledWith( @@ -233,7 +243,7 @@ describe('wrapLoadWithSentry calls trace', () => { it('falls back to the raw url if `event.route.id` is not available', async () => { const wrappedLoad = wrapLoadWithSentry(load); - await wrappedLoad(MOCK_LOAD_NO_ROUTE_ARGS); + await wrappedLoad(getLoadArgsWithoutRoute()); expect(mockTrace).toHaveBeenCalledTimes(1); expect(mockTrace).toHaveBeenCalledWith( @@ -249,10 +259,17 @@ describe('wrapLoadWithSentry calls trace', () => { expect.any(Function), ); }); + + it("doesn't wrap load more than once if the wrapper was applied multiple times", async () => { + const wrappedLoad = wrapLoadWithSentry(wrapLoadWithSentry(wrapLoadWithSentry(load))); + await wrappedLoad(getLoadArgs()); + + expect(mockTrace).toHaveBeenCalledTimes(1); + }); }); describe('wrapServerLoadWithSentry calls trace', () => { - async function serverLoad({ params }: Parameters[0]): Promise> { + async function serverLoad({ params }): Promise> { return { post: params.id, }; @@ -260,7 +277,7 @@ describe('wrapServerLoadWithSentry calls trace', () => { it('attaches trace data if available', async () => { const wrappedLoad = wrapServerLoadWithSentry(serverLoad); - await wrappedLoad(MOCK_SERVER_ONLY_LOAD_ARGS); + await wrappedLoad(getServerOnlyArgs()); expect(mockTrace).toHaveBeenCalledTimes(1); expect(mockTrace).toHaveBeenCalledWith( @@ -294,7 +311,7 @@ describe('wrapServerLoadWithSentry calls trace', () => { it("doesn't attach trace data if it's not available", async () => { const wrappedLoad = wrapServerLoadWithSentry(serverLoad); - await wrappedLoad(MOCK_SERVER_ONLY_NO_TRACE_LOAD_ARGS); + await wrappedLoad(getServerArgsWithoutTracingHeaders()); expect(mockTrace).toHaveBeenCalledTimes(1); expect(mockTrace).toHaveBeenCalledWith( @@ -316,7 +333,7 @@ describe('wrapServerLoadWithSentry calls trace', () => { it("doesn't attach the DSC data if the baggage header not available", async () => { const wrappedLoad = wrapServerLoadWithSentry(serverLoad); - await wrappedLoad(MOCK_SERVER_ONLY_NO_BAGGAGE_LOAD_ARGS); + await wrappedLoad(getServerArgsWithoutBaggageHeader()); expect(mockTrace).toHaveBeenCalledTimes(1); expect(mockTrace).toHaveBeenCalledWith( @@ -341,7 +358,8 @@ describe('wrapServerLoadWithSentry calls trace', () => { }); it('falls back to the raw url if `event.route.id` is not available', async () => { - const event = { ...MOCK_SERVER_ONLY_LOAD_ARGS }; + const event = getServerOnlyArgs(); + // @ts-ignore - this is fine (just tests here) delete event.route; const wrappedLoad = wrapServerLoadWithSentry(serverLoad); await wrappedLoad(event); @@ -375,4 +393,11 @@ describe('wrapServerLoadWithSentry calls trace', () => { expect.any(Function), ); }); + + it("doesn't wrap server load more than once if the wrapper was applied multiple times", async () => { + const wrappedLoad = wrapServerLoadWithSentry(wrapServerLoadWithSentry(serverLoad)); + await wrappedLoad(getServerOnlyArgs()); + + expect(mockTrace).toHaveBeenCalledTimes(1); + }); });