revert: set request session on current scope

mydea · mydea · commit fdb6bb51709f · 2024-02-15T13:33:39.000+01:00
diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts
@@ -28,6 +28,7 @@ import {
   normalize,
 } from '@sentry/utils';
 
+import { ServerRuntimeClient } from '@sentry/core';
 import type { NodeClient } from './client';
 import { DEBUG_BUILD } from './debug-build';
 // TODO (v8 / XXX) Remove this import
@@ -165,9 +166,9 @@ export function requestHandler(
     client.initSessionFlusher();
 
     // If Scope contains a Single mode Session, it is removed in favor of using Session Aggregates mode
-    const isolationScope = getIsolationScope();
-    if (isolationScope.getSession()) {
-      isolationScope.setSession();
+    const scope = getCurrentScope();
+    if (scope.getSession()) {
+      scope.setSession();
     }
   }
 
@@ -200,19 +201,17 @@ export function requestHandler(
       const client = getClient<NodeClient>();
       if (isAutoSessionTrackingEnabled(client)) {
         // Set `status` of `RequestSession` to Ok, at the beginning of the request
-        isolationScope.setRequestSession({ status: 'ok' });
+        getCurrentScope().setRequestSession({ status: 'ok' });
       }
 
       res.once('finish', () => {
-        const client = getClient<NodeClient>();
-        if (isAutoSessionTrackingEnabled(client)) {
+        const client = getClient<ServerRuntimeClient>();
+        if (client && isAutoSessionTrackingEnabled(client)) {
           setImmediate(() => {
-            // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-            if (client && (client as any)._captureRequestSession) {
+            if (client['_captureRequestSession']) {
               // Calling _captureRequestSession to capture request session at the end of the request by incrementing
               // the correct SessionAggregates bucket i.e. crashed, errored or exited
-              // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-              (client as any)._captureRequestSession();
+              client['_captureRequestSession']();
             }
           });
         }
@@ -273,7 +272,7 @@ export function errorHandler(options?: {
         // The request should already have been stored in `scope.sdkProcessingMetadata` by `sentryRequestMiddleware`,
         // but on the off chance someone is using `sentryErrorMiddleware` without `sentryRequestMiddleware`, it doesn't
         // hurt to be sure
-        _scope.setSDKProcessingMetadata({ request: _req });
+        getIsolationScope().setSDKProcessingMetadata({ request: _req });
 
         // For some reason we need to set the transaction on the scope again
         // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts
@@ -12,7 +12,7 @@ import {
   mergeScopeData,
   spanToJSON,
 } from '@sentry/core';
-import type { Event, PropagationContext } from '@sentry/types';
+import type { Event, PropagationContext, Scope } from '@sentry/types';
 import { SentryError } from '@sentry/utils';
 
 import { NodeClient } from '../src/client';
@@ -61,18 +61,24 @@ describe('requestHandler', () => {
     jest.restoreAllMocks();
   });
 
-  it('autoSessionTracking is enabled, sets requestSession status to ok, when handling a request', () => {
+  it('autoSessionTracking is enabled, sets requestSession status to ok, when handling a request', done => {
     const options = getDefaultNodeClientOptions({ autoSessionTracking: true, release: '1.2' });
     client = new NodeClient(options);
     // eslint-disable-next-line deprecation/deprecation
     const hub = new Hub(client);
     // eslint-disable-next-line deprecation/deprecation
     makeMain(hub);
 
-    sentryRequestMiddleware(req, res, next);
+    let scope: Scope;
+    sentryRequestMiddleware(req, res, () => {
+      scope = getCurrentScope();
+      return next();
+    });
 
-    const isolationScope = getIsolationScope();
-    expect(isolationScope.getRequestSession()).toEqual({ status: 'ok' });
+    setImmediate(() => {
+      expect(scope.getRequestSession()).toEqual({ status: 'ok' });
+      done();
+    });
   });
 
   it('autoSessionTracking is disabled, does not set requestSession, when handling a request', () => {
@@ -83,13 +89,18 @@ describe('requestHandler', () => {
     // eslint-disable-next-line deprecation/deprecation
     makeMain(hub);
 
-    sentryRequestMiddleware(req, res, next);
+    let scope: Scope;
+    sentryRequestMiddleware(req, res, () => {
+      scope = getCurrentScope();
+      return next();
+    });
 
-    const scope = getCurrentScope();
-    expect(scope?.getRequestSession()).toBeUndefined();
+    setImmediate(() => {
+      expect(scope.getRequestSession()).toBeUndefined();
+    });
   });
 
-  it('autoSessionTracking is enabled, calls _captureRequestSession, on response finish', done => {
+  it('autoSessionTracking is enabled, calls _captureRequestSession, on response finish xxx', done => {
     const options = getDefaultNodeClientOptions({ autoSessionTracking: true, release: '1.2' });
     client = new NodeClient(options);
     // eslint-disable-next-line deprecation/deprecation
@@ -99,13 +110,16 @@ describe('requestHandler', () => {
 
     const captureRequestSession = jest.spyOn<any, any>(client, '_captureRequestSession');
 
-    sentryRequestMiddleware(req, res, next);
+    let scope: Scope;
+    sentryRequestMiddleware(req, res, () => {
+      scope = getCurrentScope();
+      return next();
+    });
 
-    const isolationScope = getIsolationScope();
     res.emit('finish');
 
     setImmediate(() => {
-      expect(isolationScope.getRequestSession()).toEqual({ status: 'ok' });
+      expect(scope.getRequestSession()).toEqual({ status: 'ok' });
       expect(captureRequestSession).toHaveBeenCalled();
       done();
     });
@@ -122,11 +136,10 @@ describe('requestHandler', () => {
     const captureRequestSession = jest.spyOn<any, any>(client, '_captureRequestSession');
 
     sentryRequestMiddleware(req, res, next);
-    const scope = getCurrentScope();
     res.emit('finish');
 
     setImmediate(() => {
-      expect(scope?.getRequestSession()).toBeUndefined();
+      expect(getCurrentScope().getRequestSession()).toBeUndefined();
       expect(captureRequestSession).not.toHaveBeenCalled();
       done();
     });
@@ -479,10 +492,8 @@ describe('tracingHandler', () => {
     const options = getDefaultNodeClientOptions({ tracesSampleRate: 1.0 });
     // eslint-disable-next-line deprecation/deprecation
     const hub = new Hub(new NodeClient(options));
-
-    jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
     // eslint-disable-next-line deprecation/deprecation
-    jest.spyOn(sentryCore, 'getCurrentScope').mockImplementation(() => hub.getScope());
+    makeMain(hub);
 
     sentryTracingMiddleware(req, res, next);
 
@@ -538,31 +549,40 @@ describe('errorHandler()', () => {
     const scope = getCurrentScope();
     // eslint-disable-next-line deprecation/deprecation
     const hub = new Hub(client);
+    // eslint-disable-next-line deprecation/deprecation
+    makeMain(hub);
 
     jest.spyOn<any, any>(client, '_captureRequestSession');
-    jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
 
-    scope?.setRequestSession({ status: 'ok' });
+    scope.setRequestSession({ status: 'ok' });
     sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, next);
-    const requestSession = scope?.getRequestSession();
+    const requestSession = scope.getRequestSession();
     expect(requestSession).toEqual({ status: 'ok' });
   });
 
-  it('autoSessionTracking is enabled + requestHandler is not used -> does not set requestSession status on Crash', () => {
+  it('autoSessionTracking is enabled + requestHandler is not used -> does not set requestSession status on Crash', done => {
     const options = getDefaultNodeClientOptions({ autoSessionTracking: false, release: '3.3' });
     client = new NodeClient(options);
 
-    const scope = getCurrentScope();
     // eslint-disable-next-line deprecation/deprecation
     const hub = new Hub(client);
+    // eslint-disable-next-line deprecation/deprecation
+    makeMain(hub);
 
     jest.spyOn<any, any>(client, '_captureRequestSession');
-    jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
 
-    scope?.setRequestSession({ status: 'ok' });
-    sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, next);
-    const requestSession = scope?.getRequestSession();
-    expect(requestSession).toEqual({ status: 'ok' });
+    getCurrentScope().setRequestSession({ status: 'ok' });
+
+    let scope: Scope;
+    sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, () => {
+      scope = getCurrentScope();
+      return next();
+    });
+
+    setImmediate(() => {
+      expect(scope.getRequestSession()).toEqual({ status: 'ok' });
+      done();
+    });
   });
 
   it('when autoSessionTracking is enabled, should set requestSession status to Crashed when an unhandled error occurs within the bounds of a request', () => {
@@ -580,16 +600,16 @@ describe('errorHandler()', () => {
     jest.spyOn<any, any>(client, '_captureRequestSession');
 
     hub.run(() => {
-      scope?.setRequestSession({ status: 'ok' });
+      scope.setRequestSession({ status: 'ok' });
       sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, () => {
         const scope = getCurrentScope();
-        const requestSession = scope?.getRequestSession();
+        const requestSession = scope.getRequestSession();
         expect(requestSession).toEqual({ status: 'crashed' });
       });
     });
   });
 
-  it('when autoSessionTracking is enabled, should not set requestSession status on Crash when it occurs outside the bounds of a request', () => {
+  it('when autoSessionTracking is enabled, should not set requestSession status on Crash when it occurs outside the bounds of a request', done => {
     const options = getDefaultNodeClientOptions({ autoSessionTracking: true, release: '2.2' });
     client = new NodeClient(options);
     // It is required to initialise SessionFlusher to capture Session Aggregates (it is usually initialised
@@ -598,13 +618,21 @@ describe('errorHandler()', () => {
     const scope = new ScopeClass();
     // eslint-disable-next-line deprecation/deprecation
     const hub = new Hub(client, scope);
+    // eslint-disable-next-line deprecation/deprecation
+    makeMain(hub);
 
     jest.spyOn<any, any>(client, '_captureRequestSession');
-    jest.spyOn(sentryCore, 'getCurrentHub').mockReturnValue(hub);
 
-    sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, next);
-    const requestSession = scope?.getRequestSession();
-    expect(requestSession).toEqual(undefined);
+    let currentScope: Scope;
+    sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, () => {
+      currentScope = getCurrentScope();
+      return next();
+    });
+
+    setImmediate(() => {
+      expect(currentScope.getRequestSession()).toEqual(undefined);
+      done();
+    });
   });
 
   it('stores request in `sdkProcessingMetadata`', () => {
@@ -620,9 +648,8 @@ describe('errorHandler()', () => {
     // `captureException` in order to examine the scope as it exists inside the `withScope` callback
     // eslint-disable-next-line deprecation/deprecation
     hub.captureException = function (this: Hub, _exception: any) {
-      // eslint-disable-next-line deprecation/deprecation
-      const scope = this.getScope();
-      expect((scope as any)._sdkProcessingMetadata.request).toEqual(req);
+      const scope = getIsolationScope();
+      expect(scope.getScopeData().sdkProcessingMetadata.request).toEqual(req);
     } as any;
 
     sentryErrorMiddleware({ name: 'error', message: 'this is an error' }, req, res, next);
