fix(nestjs): wrap @catch and @Injectable with 2 flags

Author: isaacs

packages/server-utils/src/integrations/tracing-channel/nestjs-decorators.ts

@@ -18,7 +18,7 @@ const ORIGIN_MIDDLEWARE = 'auto.middleware.orchestrion.nestjs';
 /** The class an `@Injectable` decorator is applied to (`ctx.arguments[0]`). */
 export interface InjectableTarget {
   name?: string;
-  sentryPatched?: boolean;
+  sentryPatchedInjectable?: boolean;
   __SENTRY_INTERNAL__?: boolean;
   prototype: {
     use?: AnyFn;
@@ -31,7 +31,7 @@ export interface InjectableTarget {
 /** The class a `@Catch` decorator is applied to (an exception filter). */
 export interface CatchTarget {
   name?: string;
-  sentryPatched?: boolean;
+  sentryPatchedCatch?: boolean;
   __SENTRY_INTERNAL__?: boolean;
   prototype: { catch?: AnyFn };
 }
@@ -49,14 +49,20 @@ interface ObservableLike {
 }
 
 /**
- * Mark a target class as patched so it's instrumented only once (mirrors the
- * vendored `isPatched`). Also give idempotency across repeated subscriptions.
+ * Mark a target class as patched (for the given pass) so it's instrumented only
+ * once, and to stay idempotent across repeated subscriptions.
+ *
+ * The `@Injectable` and `@Catch` passes use *separate* flags on purpose: they
+ * wrap disjoint method sets (use/canActivate/transform/intercept vs catch), and
+ * a class can be decorated with both (an exception filter that also uses DI).
+ * A single shared flag would let whichever channel fired first latch it and
+ * block the other pass, dropping that pass's spans regardless of ordering.
  */
-function isTargetPatched(target: { sentryPatched?: boolean }): boolean {
-  if (target.sentryPatched) {
+function isTargetPatched(target: object, flag: 'sentryPatchedInjectable' | 'sentryPatchedCatch'): boolean {
+  if ((target as Record<string, unknown>)[flag]) {
     return true;
   }
-  addNonEnumerableProperty(target as object, 'sentryPatched', true);
+  addNonEnumerableProperty(target, flag, true);
   return false;
 }
 
@@ -201,7 +207,7 @@ function patchInterceptor(target: InjectableTarget, intercept: AnyFn, seenContex
  */
 export function patchInjectableTarget(target: InjectableTarget, seenContexts: WeakSet<object>): void {
   const proto = target?.prototype;
-  if (!proto || target.__SENTRY_INTERNAL__ || isTargetPatched(target)) {
+  if (!proto || target.__SENTRY_INTERNAL__ || isTargetPatched(target, 'sentryPatchedInjectable')) {
     return;
   }
 
@@ -264,7 +270,12 @@ export function patchInjectableTarget(target: InjectableTarget, seenContexts: We
  */
 export function patchCatchTarget(target: CatchTarget): void {
   const proto = target?.prototype;
-  if (!proto || typeof proto.catch !== 'function' || target.__SENTRY_INTERNAL__ || isTargetPatched(target)) {
+  if (
+    !proto ||
+    typeof proto.catch !== 'function' ||
+    target.__SENTRY_INTERNAL__ ||
+    isTargetPatched(target, 'sentryPatchedCatch')
+  ) {
     return;
   }
   proto.catch = new Proxy(proto.catch, {

packages/server-utils/test/orchestrion/nestjs.test.ts

@@ -519,6 +519,104 @@ describe('nestjsChannelIntegration: @Catch (exception filter)', () => {
     new HttpExceptionFilter().catch('boom', undefined);
     expect(spanInside).toBeUndefined();
   });
+
+  // A class can be decorated with both `@Injectable` and `@Catch` (an exception
+  // filter that uses DI). Which channel fires first depends on decorator
+  // stacking order (decorators apply inner-first): `@Catch` over `@Injectable`
+  // fires @Injectable first; `@Injectable` over `@Catch` fires @Catch first.
+  // Because the two passes use separate patched-flags, both must wrap their own
+  // methods regardless of which channel fires first.
+  function fireInjectable(target: object): void {
+    tracingChannel<{ arguments: unknown[] }>(CHANNELS.NESTJS_INJECTABLE).traceSync(() => undefined, {
+      arguments: [target],
+    });
+  }
+
+  it('still wraps `catch` when the @Injectable channel fired first (dual @Injectable @Catch filter)', () => {
+    installTestAsyncContextStrategy();
+    initTestClient();
+    nestjsChannelIntegration().setupOnce!();
+
+    let spanInside: ReturnType<typeof getActiveSpan>;
+    class HttpExceptionFilter {
+      public catch(exception: unknown, _host: unknown): string {
+        spanInside = getActiveSpan();
+        return `handled:${String(exception)}`;
+      }
+    }
+    fireInjectable(HttpExceptionFilter);
+    applyCatch(HttpExceptionFilter);
+
+    const ret = new HttpExceptionFilter().catch('boom', { switchToHttp: () => ({}) });
+    expect(ret).toBe('handled:boom');
+
+    const json = spanToJSON(spanInside!);
+    expect(json.description).toBe('HttpExceptionFilter');
+    expect(json.op).toBe('middleware.nestjs');
+    expect(json.origin).toBe('auto.middleware.orchestrion.nestjs.exception_filter');
+  });
+
+  it('still wraps `catch` when the @Catch channel fired first (dual @Injectable @Catch filter)', () => {
+    installTestAsyncContextStrategy();
+    initTestClient();
+    nestjsChannelIntegration().setupOnce!();
+
+    let spanInside: ReturnType<typeof getActiveSpan>;
+    class HttpExceptionFilter {
+      public catch(exception: unknown, _host: unknown): string {
+        spanInside = getActiveSpan();
+        return `handled:${String(exception)}`;
+      }
+    }
+    applyCatch(HttpExceptionFilter);
+    fireInjectable(HttpExceptionFilter);
+
+    const ret = new HttpExceptionFilter().catch('boom', { switchToHttp: () => ({}) });
+    expect(ret).toBe('handled:boom');
+
+    const json = spanToJSON(spanInside!);
+    expect(json.description).toBe('HttpExceptionFilter');
+    expect(json.op).toBe('middleware.nestjs');
+    expect(json.origin).toBe('auto.middleware.orchestrion.nestjs.exception_filter');
+  });
+
+  // A (contrived) class that is BOTH a guard (`canActivate`) and an exception
+  // filter (`catch`) proves the two passes are independent: neither ordering may
+  // let one pass's patched-flag block the other. Both spans must appear either way.
+  for (const order of ['injectable-first', 'catch-first'] as const) {
+    it(`wraps BOTH canActivate and catch when the ${order} channel fired first`, () => {
+      installTestAsyncContextStrategy();
+      initTestClient();
+      nestjsChannelIntegration().setupOnce!();
+
+      let guardSpan: ReturnType<typeof getActiveSpan>;
+      let filterSpan: ReturnType<typeof getActiveSpan>;
+      class GuardAndFilter {
+        public canActivate(_ctx: unknown): boolean {
+          guardSpan = getActiveSpan();
+          return true;
+        }
+        public catch(exception: unknown, _host: unknown): string {
+          filterSpan = getActiveSpan();
+          return `handled:${String(exception)}`;
+        }
+      }
+
+      if (order === 'injectable-first') {
+        fireInjectable(GuardAndFilter);
+        applyCatch(GuardAndFilter);
+      } else {
+        applyCatch(GuardAndFilter);
+        fireInjectable(GuardAndFilter);
+      }
+
+      expect(new GuardAndFilter().canActivate({ ctx: true })).toBe(true);
+      expect(new GuardAndFilter().catch('boom', { switchToHttp: () => ({}) })).toBe('handled:boom');
+
+      expect(spanToJSON(guardSpan!).origin).toBe('auto.middleware.orchestrion.nestjs.guard');
+      expect(spanToJSON(filterSpan!).origin).toBe('auto.middleware.orchestrion.nestjs.exception_filter');
+    });
+  }
 });
 
 describe('nestjsChannelIntegration: schedule / event / bullmq', () => {