Prevent multiple wrappings when using a manual wrapper.

Author: onurtemizkan

packages/remix/src/utils/instrumentServer.ts

@@ -1,6 +1,7 @@
 /* eslint-disable max-lines */
 import { captureException, getCurrentHub } from '@sentry/node';
 import { getActiveTransaction, hasTracingEnabled, Span } from '@sentry/tracing';
+import { WrappedFunction } from '@sentry/types';
 import { addExceptionMechanism, fill, isNodeEnv, loadModule, logger, serializeBaggage } from '@sentry/utils';
 
 import {
@@ -17,6 +18,9 @@ import {
   ServerRouteManifest,
 } from './types';
 
+// Flag to track if the core request handler is instrumented.
+export let isRequestHandlerWrapped = false;
+
 // Taken from Remix Implementation
 // https://github.com/remix-run/remix/blob/32300ec6e6e8025602cea63e17a2201989589eab/packages/remix-server-runtime/responses.ts#L60-L77
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
@@ -365,16 +369,22 @@ export function instrumentBuild(build: ServerBuild): ServerBuild {
 
   const wrappedEntry = { ...build.entry, module: { ...build.entry.module } };
 
-  fill(wrappedEntry.module, 'default', makeWrappedDocumentRequestFunction);
+  // Not keeping boolean flags like it's done for `requestHandler` functions,
+  // Because the build can change between build and runtime.
+  // So if there is a new `loader` or`action` or `documentRequest` after build.
+  // We should be able to wrap them, as they may not be wrapped before.
+  if (!(wrappedEntry.module.default as WrappedFunction).__sentry_original__) {
+    fill(wrappedEntry.module, 'default', makeWrappedDocumentRequestFunction);
+  }
 
   for (const [id, route] of Object.entries(build.routes)) {
     const wrappedRoute = { ...route, module: { ...route.module } };
 
-    if (wrappedRoute.module.action) {
+    if (wrappedRoute.module.action && !(wrappedRoute.module.action as WrappedFunction).__sentry_original__) {
       fill(wrappedRoute.module, 'action', makeWrappedAction(id));
     }
 
-    if (wrappedRoute.module.loader) {
+    if (wrappedRoute.module.loader && !(wrappedRoute.module.loader as WrappedFunction).__sentry_original__) {
       fill(wrappedRoute.module, 'loader', makeWrappedLoader(id));
     }
 
@@ -385,6 +395,7 @@ export function instrumentBuild(build: ServerBuild): ServerBuild {
         wrappedRoute.module.loader = () => ({});
       }
 
+      // We want to wrap the root loader regardless of whether it's already wrapped before.
       fill(wrappedRoute.module, 'loader', makeWrappedRootLoader);
     }
 
@@ -397,6 +408,10 @@ export function instrumentBuild(build: ServerBuild): ServerBuild {
 function makeWrappedCreateRequestHandler(
   origCreateRequestHandler: CreateRequestHandlerFunction,
 ): CreateRequestHandlerFunction {
+  // To track if this wrapper has been applied, before other wrappers.
+  // Can't track `__sentry_original__` because it's not the same function as the potentially manually wrapped one.
+  isRequestHandlerWrapped = true;
+
   return function (this: unknown, build: ServerBuild, ...args: unknown[]): RequestHandler {
     const newBuild = instrumentBuild(build);
     const requestHandler = origCreateRequestHandler.call(this, newBuild, ...args);

packages/remix/src/utils/serverAdapters/express.ts

@@ -1,6 +1,11 @@
 import { extractRequestData, loadModule } from '@sentry/utils';
 
-import { createRoutes, instrumentBuild, startRequestHandlerTransaction } from '../instrumentServer';
+import {
+  createRoutes,
+  instrumentBuild,
+  isRequestHandlerWrapped,
+  startRequestHandlerTransaction,
+} from '../instrumentServer';
 import {
   ExpressCreateRequestHandler,
   ExpressCreateRequestHandlerOptions,
@@ -19,6 +24,11 @@ function wrapExpressRequestHandler(
   const routes = createRoutes(build.routes);
   const pkg = loadModule<ReactRouterDomPkg>('react-router-dom');
 
+  // If the core request handler is already wrapped, don't wrap Express handler which uses it.
+  if (isRequestHandlerWrapped) {
+    return origRequestHandler;
+  }
+
   return async function (
     this: unknown,
     req: ExpressRequest,

packages/remix/test/integration/app/routes/action-json-response/$id.tsx

@@ -13,7 +13,7 @@ export const action: ActionFunction = async ({ params: { id } }) => {
   }
 
   if (id === '-2') {
-    // Note: This GET request triggers to the `Loader` of the URL, not the `Action`.
+    // Note: This GET request triggers the `Loader` of the URL, not the `Action`.
     throw redirect('/action-json-response/-1');
   }
 

packages/remix/test/integration/test/server/action.test.ts

@@ -8,9 +8,10 @@ import {
 
 jest.spyOn(console, 'error').mockImplementation();
 
-describe('Remix API Actions', () => {
+// Repeat tests for each adapter
+describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', adapter => {
   it('correctly instruments a parameterized Remix API action', async () => {
-    const baseURL = await runServer();
+    const baseURL = await runServer(adapter);
     const url = `${baseURL}/action-json-response/123123`;
     const envelope = await getEnvelopeRequest(url, 'post');
     const transaction = envelope[2];
@@ -39,7 +40,7 @@ describe('Remix API Actions', () => {
   });
 
   it('reports an error thrown from the action', async () => {
-    const baseURL = await runServer();
+    const baseURL = await runServer(adapter);
     const url = `${baseURL}/action-json-response/-1`;
 
     const [transaction, event] = await getMultipleEnvelopeRequest(url, 2, 'post');
@@ -76,7 +77,7 @@ describe('Remix API Actions', () => {
   });
 
   it('handles a thrown 500 response', async () => {
-    const baseURL = await runServer();
+    const baseURL = await runServer(adapter);
     const url = `${baseURL}/action-json-response/-2`;
 
     const [transaction_1, event, transaction_2] = await getMultipleEnvelopeRequest(url, 3, 'post');

packages/remix/test/integration/test/server/loader.test.ts

@@ -8,12 +8,17 @@ import {
 
 jest.spyOn(console, 'error').mockImplementation();
 
-describe('Remix API Loaders', () => {
+// Repeat tests for each adapter
+describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', adapter => {
   it('reports an error thrown from the loader', async () => {
-    const baseURL = await runServer();
+    const baseURL = await runServer(adapter);
     const url = `${baseURL}/loader-json-response/-2`;
 
-    const [transaction, event] = await getMultipleEnvelopeRequest(url, 2);
+    let [transaction, event] = await getMultipleEnvelopeRequest(url, 2);
+
+    // The event envelope is returned before the transaction envelope when using express adapter.
+    // We can update this when we merge the envelope filtering utility.
+    adapter === 'express' && ([event, transaction] = [transaction, event]);
 
     assertSentryTransaction(transaction[2], {
       contexts: {
@@ -47,7 +52,7 @@ describe('Remix API Loaders', () => {
   });
 
   it('correctly instruments a parameterized Remix API loader', async () => {
-    const baseURL = await runServer();
+    const baseURL = await runServer(adapter);
     const url = `${baseURL}/loader-json-response/123123`;
     const envelope = await getEnvelopeRequest(url);
     const transaction = envelope[2];
@@ -75,7 +80,7 @@ describe('Remix API Loaders', () => {
   });
 
   it('handles a thrown 500 response', async () => {
-    const baseURL = await runServer();
+    const baseURL = await runServer(adapter);
     const url = `${baseURL}/loader-json-response/-1`;
 
     const [transaction_1, event, transaction_2] = await getMultipleEnvelopeRequest(url, 3);

packages/remix/test/integration/test/server/utils/helpers.ts

@@ -1,18 +1,22 @@
 import express from 'express';
 import { createRequestHandler } from '@remix-run/express';
 import { getPortPromise } from 'portfinder';
+import { wrapExpressCreateRequestHandler } from '@sentry/remix';
 
 export * from '../../../../../../node-integration-tests/utils';
 
 /**
  * Runs a test server
  * @returns URL
  */
-export async function runServer(): Promise<string> {
+export async function runServer(adapter: string = 'builtin'): Promise<string> {
+  const requestHandlerFactory =
+    adapter === 'express' ? wrapExpressCreateRequestHandler(createRequestHandler) : createRequestHandler;
+
   const app = express();
   const port = await getPortPromise();
 
-  app.all('*', createRequestHandler({ build: require('../../../build') }));
+  app.all('*', requestHandlerFactory({ build: require('../../../build') }));
 
   const server = app.listen(port, () => {
     setTimeout(() => {