Skip to content

Commit

Permalink
fix(sveltekit): Don't capture thrown Redirects as exceptions (#7731)
Browse files Browse the repository at this point in the history
As outlined in the [SvelteKit docs](https://kit.svelte.dev/docs/load#redirects), users can `throw redirect(300, 'route/to/redirect')` in `load` functions. We don't want to capture these as errors and send them to Sentry.
  • Loading branch information
Lms24 committed Apr 4, 2023
1 parent 95e51a2 commit 52b764e
Show file tree
Hide file tree
Showing 6 changed files with 81 additions and 2 deletions.
7 changes: 7 additions & 0 deletions packages/sveltekit/src/client/load.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,18 @@ import { captureException } from '@sentry/svelte';
import { addExceptionMechanism, objectify } from '@sentry/utils';
import type { LoadEvent } from '@sveltejs/kit';

import { isRedirect } from '../common/utils';

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.
const objectifiedErr = objectify(e);

// We don't want to capture thrown `Redirect`s as these are not errors but expected behaviour
if (isRedirect(objectifiedErr)) {
return objectifiedErr;
}

captureException(objectifiedErr, scope => {
scope.addEventProcessor(event => {
addExceptionMechanism(event, {
Expand Down
16 changes: 16 additions & 0 deletions packages/sveltekit/src/common/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import type { Redirect } from '@sveltejs/kit';

/**
* 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
* @param error the potential redirect error
*/
export function isRedirect(error: unknown): error is Redirect {
if (error == null || typeof error !== 'object') {
return false;
}
const hasValidLocation = 'location' in error && typeof error.location === 'string';
const hasValidStatus =
'status' in error && typeof error.status === 'number' && error.status >= 300 && error.status <= 308;
return hasValidLocation && hasValidStatus;
}
8 changes: 7 additions & 1 deletion packages/sveltekit/src/server/load.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type { TransactionContext } from '@sentry/types';
import { addExceptionMechanism, objectify } from '@sentry/utils';
import type { HttpError, LoadEvent, ServerLoadEvent } from '@sveltejs/kit';

import { isRedirect } from '../common/utils';
import { getTracePropagationData } from './utils';

function isHttpError(err: unknown): err is HttpError {
Expand All @@ -19,7 +20,12 @@ function sendErrorToSentry(e: unknown): unknown {
// The error() helper is commonly used to throw errors in load functions: https://kit.svelte.dev/docs/modules#sveltejs-kit-error
// If we detect a thrown error that is an instance of HttpError, we don't want to capture 4xx errors as they
// could be noisy.
if (isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400) {
// Also the `redirect(...)` helper is used to redirect users from one page to another. We don't want to capture thrown
// `Redirect`s as they're not errors but expected behaviour
if (
isRedirect(objectifiedErr) ||
(isHttpError(objectifiedErr) && objectifiedErr.status < 500 && objectifiedErr.status >= 400)
) {
return objectifiedErr;
}

Expand Down
13 changes: 13 additions & 0 deletions packages/sveltekit/test/client/load.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { addTracingExtensions, Scope } from '@sentry/svelte';
import type { Load } from '@sveltejs/kit';
import { redirect } from '@sveltejs/kit';
import { vi } from 'vitest';

import { wrapLoadWithSentry } from '../../src/client/load';
Expand Down Expand Up @@ -99,6 +100,18 @@ describe('wrapLoadWithSentry', () => {
expect(mockCaptureException).toHaveBeenCalledTimes(1);
});

it("doesn't call captureException for thrown `Redirect`s", async () => {
async function load(_: Parameters<Load>[0]): Promise<ReturnType<Load>> {
throw redirect(300, 'other/route');
}

const wrappedLoad = wrapLoadWithSentry(load);
const res = wrappedLoad(MOCK_LOAD_ARGS);
await expect(res).rejects.toThrow();

expect(mockCaptureException).not.toHaveBeenCalled();
});

it('calls trace function', async () => {
async function load({ params }: Parameters<Load>[0]): Promise<ReturnType<Load>> {
return {
Expand Down
25 changes: 25 additions & 0 deletions packages/sveltekit/test/common/utils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { isRedirect } from '../../src/common/utils';

describe('isRedirect', () => {
it.each([
{ location: '/users/id', status: 300 },
{ location: '/users/id', status: 304 },
{ location: '/users/id', status: 308 },
{ location: '', status: 308 },
])('returns `true` for valid Redirect objects', redirectObject => {
expect(isRedirect(redirectObject)).toBe(true);
});

it.each([
300,
'redirect',
{ location: { route: { id: 'users/id' } }, status: 300 },
{ status: 308 },
{ location: '/users/id' },
{ location: '/users/id', status: 201 },
{ location: '/users/id', status: 400 },
{ location: '/users/id', status: 500 },
])('returns `false` for invalid Redirect objects', redirectObject => {
expect(isRedirect(redirectObject)).toBe(false);
});
});
14 changes: 13 additions & 1 deletion packages/sveltekit/test/server/load.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { addTracingExtensions } from '@sentry/core';
import { Scope } from '@sentry/node';
import type { Load, ServerLoad } from '@sveltejs/kit';
import { error } from '@sveltejs/kit';
import { error, redirect } from '@sveltejs/kit';
import { vi } from 'vitest';

import { wrapLoadWithSentry, wrapServerLoadWithSentry } from '../../src/server/load';
Expand Down Expand Up @@ -166,6 +166,18 @@ describe.each([
});
});

it("doesn't call captureException for thrown `Redirect`s", async () => {
async function load(_: Parameters<Load>[0]): Promise<ReturnType<Load>> {
throw redirect(300, 'other/route');
}

const wrappedLoad = wrapLoadWithSentry(load);
const res = wrappedLoad(MOCK_LOAD_ARGS);
await expect(res).rejects.toThrow();

expect(mockCaptureException).not.toHaveBeenCalled();
});

it('adds an exception mechanism', async () => {
const addEventProcessorSpy = vi.spyOn(mockScope, 'addEventProcessor').mockImplementationOnce(callback => {
void callback({}, { event_id: 'fake-event-id' });
Expand Down

0 comments on commit 52b764e

Please sign in to comment.