From 4aff9f14bee54a65105c2ec65170ecef19c05b33 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 3 Nov 2022 13:36:42 +0000 Subject: [PATCH] fix(remix): Prevent capturing pending promises as exceptions. --- packages/remix/src/utils/instrumentServer.ts | 24 ++- .../app/routes/action-json-response/$id.tsx | 16 ++ .../integration/test/server/action.test.ts | 200 ++++++++++++++++++ 3 files changed, 235 insertions(+), 5 deletions(-) diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index deb822c51c51..1e769c50d452 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -57,7 +57,7 @@ function isCatchResponse(response: Response): boolean { // Based on Remix Implementation // https://github.com/remix-run/remix/blob/7688da5c75190a2e29496c78721456d6e12e3abe/packages/remix-server-runtime/data.ts#L131-L145 -function extractData(response: Response): Promise { +async function extractData(response: Response): Promise { const contentType = response.headers.get('Content-Type'); // Cloning the response to avoid consuming the original body stream @@ -70,14 +70,28 @@ function extractData(response: Response): Promise { return responseClone.text(); } -function captureRemixServerException(err: Error, name: string, request: Request): void { +async function extractResponseError(response: Response): Promise { + const responseData = await extractData(response); + + if (typeof responseData === 'string') { + return responseData; + } + + if (response.statusText) { + return response.statusText; + } + + return responseData; +} + +async function captureRemixServerException(err: Error, name: string, request: Request): Promise { // Skip capturing if the thrown error is not a 5xx response // https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders if (isResponse(err) && err.status < 500) { return; } - captureException(isResponse(err) ? extractData(err) : err, scope => { + captureException(isResponse(err) ? await extractResponseError(err) : err, scope => { scope.setSDKProcessingMetadata({ request }); scope.addEventProcessor(event => { addExceptionMechanism(event, { @@ -128,7 +142,7 @@ function makeWrappedDocumentRequestFunction( span?.finish(); } catch (err) { - captureRemixServerException(err, 'documentRequest', request); + await captureRemixServerException(err, 'documentRequest', request); throw err; } @@ -165,7 +179,7 @@ function makeWrappedDataFunction(origFn: DataFunction, id: string, name: 'action currentScope.setSpan(activeTransaction); span?.finish(); } catch (err) { - captureRemixServerException(err, name, args.request); + await captureRemixServerException(err, name, args.request); throw err; } diff --git a/packages/remix/test/integration/app/routes/action-json-response/$id.tsx b/packages/remix/test/integration/app/routes/action-json-response/$id.tsx index 588c340b2c23..2c7a19059596 100644 --- a/packages/remix/test/integration/app/routes/action-json-response/$id.tsx +++ b/packages/remix/test/integration/app/routes/action-json-response/$id.tsx @@ -17,6 +17,22 @@ export const action: ActionFunction = async ({ params: { id } }) => { throw redirect('/action-json-response/-1'); } + if (id === '-3') { + throw json({}, { status: 500, statusText: 'Sentry Test Error' }); + } + + if (id === '-4') { + throw json({ data: 1234 }, { status: 500 }); + } + + if (id === '-5') { + throw json('Sentry Test Error [string body]', { status: 500 }); + } + + if (id === '-6') { + throw json({}, { status: 500 }); + } + return json({ test: 'test' }); }; diff --git a/packages/remix/test/integration/test/server/action.test.ts b/packages/remix/test/integration/test/server/action.test.ts index d7a611c45ea8..32e1d26f4a2d 100644 --- a/packages/remix/test/integration/test/server/action.test.ts +++ b/packages/remix/test/integration/test/server/action.test.ts @@ -185,4 +185,204 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada }, }); }); + + it('handles a thrown `json()` error response with `statusText`', async () => { + const env = await RemixTestEnv.init(adapter); + const url = `${env.url}/action-json-response/-3`; + + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + count: 2, + method: 'post', + envelopeType: ['transaction', 'event'], + }); + + const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction'); + const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); + + assertSentryTransaction(transaction[2], { + contexts: { + trace: { + op: 'http.server', + status: 'internal_error', + tags: { + method: 'POST', + 'http.status_code': '500', + }, + }, + }, + tags: { + transaction: 'routes/action-json-response/$id', + }, + }); + + assertSentryEvent(event[2], { + exception: { + values: [ + { + type: 'Error', + value: 'Sentry Test Error', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: 'action', + }, + handled: true, + type: 'instrument', + }, + }, + ], + }, + }); + }); + + it('handles a thrown `json()` error response without `statusText`', async () => { + const env = await RemixTestEnv.init(adapter); + const url = `${env.url}/action-json-response/-4`; + + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + count: 2, + method: 'post', + envelopeType: ['transaction', 'event'], + }); + + const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction'); + const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); + + assertSentryTransaction(transaction[2], { + contexts: { + trace: { + op: 'http.server', + status: 'internal_error', + tags: { + method: 'POST', + 'http.status_code': '500', + }, + }, + }, + tags: { + transaction: 'routes/action-json-response/$id', + }, + }); + + assertSentryEvent(event[2], { + exception: { + values: [ + { + type: 'Error', + value: 'Non-Error exception captured with keys: data', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: 'action', + }, + handled: true, + type: 'instrument', + }, + }, + ], + }, + }); + }); + + it('handles a thrown `json()` error response with string body', async () => { + const env = await RemixTestEnv.init(adapter); + const url = `${env.url}/action-json-response/-5`; + + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + count: 2, + method: 'post', + envelopeType: ['transaction', 'event'], + }); + + const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction'); + const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); + + assertSentryTransaction(transaction[2], { + contexts: { + trace: { + op: 'http.server', + status: 'internal_error', + tags: { + method: 'POST', + 'http.status_code': '500', + }, + }, + }, + tags: { + transaction: 'routes/action-json-response/$id', + }, + }); + + assertSentryEvent(event[2], { + exception: { + values: [ + { + type: 'Error', + value: 'Sentry Test Error [string body]', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: 'action', + }, + handled: true, + type: 'instrument', + }, + }, + ], + }, + }); + }); + + it('handles a thrown `json()` error response with an empty object', async () => { + const env = await RemixTestEnv.init(adapter); + const url = `${env.url}/action-json-response/-6`; + + const envelopes = await env.getMultipleEnvelopeRequest({ + url, + count: 2, + method: 'post', + envelopeType: ['transaction', 'event'], + }); + + const [transaction] = envelopes.filter(envelope => envelope[1].type === 'transaction'); + const [event] = envelopes.filter(envelope => envelope[1].type === 'event'); + + assertSentryTransaction(transaction[2], { + contexts: { + trace: { + op: 'http.server', + status: 'internal_error', + tags: { + method: 'POST', + 'http.status_code': '500', + }, + }, + }, + tags: { + transaction: 'routes/action-json-response/$id', + }, + }); + + assertSentryEvent(event[2], { + exception: { + values: [ + { + type: 'Error', + value: 'Non-Error exception captured with keys: [object has no keys]', + stacktrace: expect.any(Object), + mechanism: { + data: { + function: 'action', + }, + handled: true, + type: 'instrument', + }, + }, + ], + }, + }); + }); });