fixup: remove unpatch method

Author: isaacs

packages/core/src/index.ts

@@ -116,12 +116,7 @@ export { linkedErrorsIntegration } from './integrations/linkederrors';
 export { moduleMetadataIntegration } from './integrations/moduleMetadata';
 export { requestDataIntegration } from './integrations/requestdata';
 export { captureConsoleIntegration } from './integrations/captureconsole';
-export {
-  unpatchExpressModule,
-  patchExpressModule,
-  setupExpressErrorHandler,
-  expressErrorHandler,
-} from './integrations/express/index';
+export { patchExpressModule, setupExpressErrorHandler, expressErrorHandler } from './integrations/express/index';
 export type {
   ExpressIntegrationOptions,
   ExpressHandlerOptions,

packages/core/src/integrations/express/index.ts

@@ -56,7 +56,7 @@ import {
   isExpressWithoutRouterPrototype,
   isExpressWithRouterPrototype,
 } from './utils';
-import { unwrapMethod, wrapMethod } from '../../utils/object';
+import { wrapMethod } from '../../utils/object';
 import { patchLayer } from './patch-layer';
 
 const getExpressExport = (express: ExpressModuleExport): ExpressExport =>
@@ -156,40 +156,6 @@ export const patchExpressModule = (options: ExpressIntegrationOptions) => {
   return express;
 };
 
-export const unpatchExpressModule = (options: ExpressIntegrationOptions) => {
-  const express = getExpressExport(options.express);
-  const routerProto: ExpressRouterv5 | ExpressRouterv4 | undefined = isExpressWithRouterPrototype(express)
-    ? express.Router.prototype // Express v5
-    : isExpressWithoutRouterPrototype(express)
-      ? express.Router // Express v4
-      : undefined;
-
-  if (!routerProto) {
-    throw new TypeError('no valid Express route function to deinstrument');
-  }
-
-  try {
-    unwrapMethod(routerProto, 'route');
-  } catch (e) {
-    DEBUG_BUILD && debug.error('Failed to unpatch express route method:', e);
-  }
-
-  try {
-    unwrapMethod(routerProto, 'use');
-  } catch (e) {
-    DEBUG_BUILD && debug.error('Failed to unpatch express use method:', e);
-  }
-
-  const { application } = express;
-  try {
-    unwrapMethod(application, 'use');
-  } catch (e) {
-    DEBUG_BUILD && debug.error('Failed to unpatch express application.use method:', e);
-  }
-
-  return express;
-};
-
 /**
  * An Express-compatible error handler, used by setupExpressErrorHandler
  */

packages/core/src/integrations/express/utils.ts

@@ -6,7 +6,6 @@ import type {
   ExpressLayer,
   ExpressLayerType,
   ExpressRequest,
-  IgnoreMatcher,
   LayerPathSegment,
   MiddlewareError,
   ExpressRouterv4,

packages/core/test/lib/integrations/express/index.test.ts

@@ -1,11 +1,11 @@
 import {
   patchExpressModule,
-  unpatchExpressModule,
   expressErrorHandler,
   setupExpressErrorHandler,
 } from '../../../../src/integrations/express/index';
 
-import { describe, it, expect, vi } from 'vitest';
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+import type { Mock } from 'vitest';
 import type {
   ExpressIntegrationOptions,
   ExpressExportv5,
@@ -22,7 +22,6 @@ import type {
   ExpressHandlerOptions,
 } from '../../../../src/integrations/express/types';
 import type { WrappedFunction } from '../../../../src/types-hoist/wrappedfunction';
-import type { IncomingMessage, ServerResponse } from 'http';
 
 const sdkProcessingMetadata: unknown[] = [];
 const isolationScope = {
@@ -57,6 +56,7 @@ vi.mock('../../../../src/utils/debug-logger', () => ({
   },
 }));
 
+beforeEach(() => (patchLayerCalls.length = 0));
 const patchLayerCalls: [options: ExpressIntegrationOptions, layer: ExpressLayer, layerPath?: string][] = [];
 
 vi.mock('../../../../src/integrations/express/patch-layer', () => ({
@@ -67,53 +67,78 @@ vi.mock('../../../../src/integrations/express/patch-layer', () => ({
   },
 }));
 
-describe('(un)patchExpressModule', () => {
-  it('throws trying to patch/unpatch the wrong thing', () => {
-    expect(() => {
-      patchExpressModule({
-        express: {} as unknown as ExpressModuleExport,
-      } as unknown as ExpressIntegrationOptions);
-    }).toThrowError('no valid Express route function to instrument');
-    expect(() => {
-      unpatchExpressModule({
-        express: {},
-      } as unknown as ExpressIntegrationOptions);
-    }).toThrowError('no valid Express route function to deinstrument');
-  });
+type ExpressSpies = Record<'routerUse' | 'routerRoute' | 'appUse', Mock<() => void>>;
 
-  // these are called in the unit tests below, so set spies
-  const routerv4route = vi.fn();
-  const routerv4use = vi.fn();
-  const appv4use = vi.fn();
-  const expressv4 = Object.assign(function express() {}, {
-    application: { use: appv4use },
+// get a fresh copy of a mock Express version 4 export
+function getExpress4(): ExpressExportv4 & { spies: ExpressSpies } {
+  const routerRoute = vi.fn();
+  const routerUse = vi.fn();
+  const appUse = vi.fn();
+  const spies = {
+    routerRoute,
+    routerUse,
+    appUse,
+  } as const;
+  const express = Object.assign(function express() {}, {
+    spies,
+    application: { use: appUse },
     Router: Object.assign(function Router() {}, {
-      route: routerv4route,
-      use: routerv4use,
+      route: routerRoute,
+      use: routerUse,
       stack: [{ name: 'layer0' }, { name: 'layer1' }, { name: 'layerFinal' }],
     }),
-  }) as unknown as ExpressExportv4;
-  Object.assign(expressv4.application, { _router: expressv4.Router });
+  }) as unknown as ExpressExportv4 & { spies: ExpressSpies };
+  Object.assign(express.application, { _router: express.Router });
+
+  return express;
+}
 
-  const appv5use = vi.fn();
+// get a fresh copy of a mock Express version 5 export
+function getExpress5(): ExpressExportv5 & { spies: ExpressSpies } {
+  const routerRoute = vi.fn();
+  const routerUse = vi.fn();
+  const appUse = vi.fn();
+  const spies = {
+    routerRoute,
+    routerUse,
+    appUse,
+  } as const;
   const expressv5 = Object.assign(function express() {}, {
-    application: { use: appv5use },
+    spies,
+    application: { use: appUse },
     Router: class Router {
       stack: ExpressLayer[] = [];
-      route() {}
-      use() {}
+      route(...args: unknown[]) {
+        return routerRoute(...args);
+      }
+      use(...args: unknown[]) {
+        return routerUse(...args);
+      }
     },
-  }) as unknown as ExpressExportv5;
+  }) as unknown as ExpressExportv5 & { spies: ExpressSpies };
+  const stack = [{ name: 'layer0' }, { name: 'layer1' }, { name: 'layerFinal' }];
   Object.assign(expressv5.application, {
-    router: {
-      stack: [{ name: 'layer0' }, { name: 'layer1' }, { name: 'layerFinal' }],
-    },
+    router: { stack },
+  });
+
+  return expressv5;
+}
+
+describe('patchExpressModule', () => {
+  it('throws trying to patch/unpatch the wrong thing', () => {
+    expect(() => {
+      patchExpressModule({
+        express: {} as unknown as ExpressModuleExport,
+      } as unknown as ExpressIntegrationOptions);
+    }).toThrowError('no valid Express route function to instrument');
   });
 
   it('can patch and restore expressv4 style module', () => {
-    const r = expressv4.Router as ExpressRouterv4;
-    const a = expressv4.application;
-    for (const module of [expressv4, { default: expressv4 }]) {
+    for (const useDefault of [false, true]) {
+      const express = getExpress4();
+      const module = useDefault ? { default: express } : express;
+      const r = express.Router as ExpressRouterv4;
+      const a = express.application;
       const options = { express: module } as unknown as ExpressIntegrationOptions;
       expect((r.use as WrappedFunction).__sentry_original__).toBe(undefined);
       expect((r.route as WrappedFunction).__sentry_original__).toBe(undefined);
@@ -124,19 +149,15 @@ describe('(un)patchExpressModule', () => {
       expect(typeof (r.use as WrappedFunction).__sentry_original__).toBe('function');
       expect(typeof (r.route as WrappedFunction).__sentry_original__).toBe('function');
       expect(typeof (a.use as WrappedFunction).__sentry_original__).toBe('function');
-
-      unpatchExpressModule(options);
-
-      expect((r.use as WrappedFunction).__sentry_original__).toBe(undefined);
-      expect((r.route as WrappedFunction).__sentry_original__).toBe(undefined);
-      expect((a.use as WrappedFunction).__sentry_original__).toBe(undefined);
     }
   });
 
   it('can patch and restore expressv5 style module', () => {
-    const r = expressv5.Router as ExpressRouterv5;
-    const a = expressv5.application;
-    for (const module of [expressv5, { default: expressv5 }]) {
+    for (const useDefault of [false, true]) {
+      const express = getExpress5();
+      const r = express.Router as ExpressRouterv5;
+      const a = express.application;
+      const module = useDefault ? { default: express } : express;
       const options = { express: module } as unknown as ExpressIntegrationOptions;
       expect((r.prototype.use as WrappedFunction).__sentry_original__).toBe(undefined);
       expect((r.prototype.route as WrappedFunction).__sentry_original__).toBe(undefined);
@@ -147,74 +168,64 @@ describe('(un)patchExpressModule', () => {
       expect(typeof (r.prototype.use as WrappedFunction).__sentry_original__).toBe('function');
       expect(typeof (r.prototype.route as WrappedFunction).__sentry_original__).toBe('function');
       expect(typeof (a.use as WrappedFunction).__sentry_original__).toBe('function');
-
-      unpatchExpressModule(options);
-
-      expect((r.prototype.use as WrappedFunction).__sentry_original__).toBe(undefined);
-      expect((r.prototype.route as WrappedFunction).__sentry_original__).toBe(undefined);
-      expect((a.use as WrappedFunction).__sentry_original__).toBe(undefined);
     }
   });
 
   it('calls patched and original Router.route', () => {
+    const expressv4 = getExpress4();
+    const { spies } = expressv4;
     const options = { express: expressv4 };
     patchExpressModule(options);
     expressv4.Router.route('a');
-    unpatchExpressModule(options);
-    expect(patchLayerCalls).toStrictEqual([[options, { name: 'layerFinal' }, 'a']]);
-    patchLayerCalls.length = 0;
-    expect(routerv4route).toHaveBeenCalledExactlyOnceWith('a');
-    routerv4route.mockReset();
+    expect(spies.routerRoute).toHaveBeenCalledExactlyOnceWith('a');
   });
 
   it('calls patched and original Router.use', () => {
+    const expressv4 = getExpress4();
+    const { spies } = expressv4;
     const options = { express: expressv4 };
     patchExpressModule(options);
     expressv4.Router.use('a');
-    unpatchExpressModule(options);
     expect(patchLayerCalls).toStrictEqual([[options, { name: 'layerFinal' }, 'a']]);
-    patchLayerCalls.length = 0;
-    expect(routerv4use).toHaveBeenCalledExactlyOnceWith('a');
-    routerv4use.mockReset();
+    expect(spies.routerUse).toHaveBeenCalledExactlyOnceWith('a');
   });
 
   it('skips patchLayer call in Router.use if no layer in the stack', () => {
+    const expressv4 = getExpress4();
+    const { spies } = expressv4;
     const options = { express: expressv4 };
     patchExpressModule(options);
     const { stack } = expressv4.Router;
     expressv4.Router.stack = [];
     expressv4.Router.use('a');
     expressv4.Router.stack = stack;
-    unpatchExpressModule(options);
     expect(patchLayerCalls).toStrictEqual([]);
-    patchLayerCalls.length = 0;
-    expect(routerv4use).toHaveBeenCalledExactlyOnceWith('a');
-    routerv4use.mockReset();
+    expect(spies.routerUse).toHaveBeenCalledExactlyOnceWith('a');
   });
 
   it('calls patched and original application.use', () => {
+    const expressv4 = getExpress4();
+    const { spies } = expressv4;
     const options = { express: expressv4 };
     patchExpressModule(options);
     expressv4.application.use('a');
-    unpatchExpressModule(options);
     expect(patchLayerCalls).toStrictEqual([[options, { name: 'layerFinal' }, 'a']]);
-    patchLayerCalls.length = 0;
-    expect(appv4use).toHaveBeenCalledExactlyOnceWith('a');
-    appv4use.mockReset();
+    expect(spies.appUse).toHaveBeenCalledExactlyOnceWith('a');
   });
 
   it('calls patched and original application.use on express v5', () => {
+    const expressv5 = getExpress5();
+    const { spies } = expressv5;
     const options = { express: expressv5 };
     patchExpressModule(options);
     expressv5.application.use('a');
-    unpatchExpressModule(options);
     expect(patchLayerCalls).toStrictEqual([[options, { name: 'layerFinal' }, 'a']]);
-    patchLayerCalls.length = 0;
-    expect(appv5use).toHaveBeenCalledExactlyOnceWith('a');
-    appv5use.mockReset();
+    expect(spies.appUse).toHaveBeenCalledExactlyOnceWith('a');
   });
 
   it('skips patchLayer on application.use if no router found', () => {
+    const expressv4 = getExpress4();
+    const { spies } = expressv4;
     const options = { express: expressv4 };
     patchExpressModule(options);
     const app = expressv4.application as {
@@ -223,29 +234,14 @@ describe('(un)patchExpressModule', () => {
     const { _router } = app;
     delete app._router;
     expressv4.application.use('a');
-    unpatchExpressModule(options);
     app._router = _router;
     // no router, so no layers to patch!
     expect(patchLayerCalls).toStrictEqual([]);
-    patchLayerCalls.length = 0;
-    expect(appv4use).toHaveBeenCalledExactlyOnceWith('a');
-    appv4use.mockReset();
-  });
-
-  it('debug error when unpatching fails', () => {
-    unpatchExpressModule({ express: expressv5 });
-    expect(debugErrors).toStrictEqual([
-      ['Failed to unpatch express route method:', new Error('Method route is not wrapped, and cannot be unwrapped')],
-      ['Failed to unpatch express use method:', new Error('Method use is not wrapped, and cannot be unwrapped')],
-      [
-        'Failed to unpatch express application.use method:',
-        new Error('Method use is not wrapped, and cannot be unwrapped'),
-      ],
-    ]);
-    debugErrors.length = 0;
+    expect(spies.appUse).toHaveBeenCalledExactlyOnceWith('a');
   });
 
   it('debug error when patching fails', () => {
+    const expressv5 = getExpress5();
     patchExpressModule({ express: expressv5 });
     patchExpressModule({ express: expressv5 });
     expect(debugErrors).toStrictEqual([
@@ -333,7 +329,7 @@ describe('setupExpressErrorHandler', () => {
     const reqHandler = appUseCalls[0];
     expect(typeof reqHandler).toBe('function');
     const next = vi.fn();
-    (reqHandler as (request: IncomingMessage, _res: ServerResponse, next: () => void) => void)(
+    (reqHandler as (request: ExpressRequest, _res: ExpressResponse, next: () => void) => void)(
       {
         method: 'GET',
         headers: { request: 'headers' },