ref(node): Small RequestData integration tweaks (#5979)

This makes a few small revisions to the new `RequestData` integration:

- Switch to using booleans rather than an array of keys when specifying what user data to include. This makes it match all of the other options, and is something I should have just done from the get-go. Given that the integration is new and thus far entirely undocumented, IMHO it feels safe to make what is technically a breaking change here.

- Rename the integration's internal `RequestDataOptions` type to `RequestDataIntegrationOptions`, to help distinguish it from the many other flavors of request data functions and types floating around.

- Make all properties in `RequestDataIntegrationOptions` optional, rather than using the `Partial` helper type.

- Switch the callback which actually does the data adding from being an option to being a protected property, in order to make it less public but still leave open the option of subclassing and setting it to a different value if we ever get around to using this in browser-based SDKs. Because I also made the property's type slightly more generic and used an index signature to do it, I also had to switch `AddRequestDataToEventOptions` from being an interface to being a type. See microsoft/TypeScript#15300.

- Rename the helper function which formats the `include` option for use in `addRequestDataToEvent` to more specifically indicate that it's converting from integration-style options to `addRequestDataToEvent`-style options.

- Refactor the aforementioned helper function to act upon and return an entire options object rather than just the `include` property, in order to have access to the `transactionNamingScheme` option.

- Add missing `transaction` property in helper function's output.

- Add tests for the helper function.

Author: lobsterkatie

packages/node/src/integrations/requestdata.ts

@@ -1,52 +1,48 @@
 // TODO (v8 or v9): Whenever this becomes a default integration for `@sentry/browser`, move this to `@sentry/core`. For
 // now, we leave it in `@sentry/integrations` so that it doesn't contribute bytes to our CDN bundles.
 
-import { EventProcessor, Hub, Integration, Transaction } from '@sentry/types';
+import { Event, EventProcessor, Hub, Integration, PolymorphicRequest, Transaction } from '@sentry/types';
 import { extractPathForTransaction } from '@sentry/utils';
 
-import {
-  addRequestDataToEvent,
-  AddRequestDataToEventOptions,
-  DEFAULT_USER_INCLUDES,
-  TransactionNamingScheme,
-} from '../requestdata';
+import { addRequestDataToEvent, AddRequestDataToEventOptions, TransactionNamingScheme } from '../requestdata';
 
-type RequestDataOptions = {
+export type RequestDataIntegrationOptions = {
   /**
    * Controls what data is pulled from the request and added to the event
    */
-  include: {
+  include?: {
     cookies?: boolean;
     data?: boolean;
     headers?: boolean;
     ip?: boolean;
     query_string?: boolean;
     url?: boolean;
-    user?: boolean | Array<typeof DEFAULT_USER_INCLUDES[number]>;
+    user?:
+      | boolean
+      | {
+          id?: boolean;
+          username?: boolean;
+          email?: boolean;
+        };
   };
 
   /** Whether to identify transactions by parameterized path, parameterized path with method, or handler name */
-  transactionNamingScheme: TransactionNamingScheme;
-
-  /**
-   * Function for adding request data to event. Defaults to `addRequestDataToEvent` from `@sentry/node` for now, but
-   * left injectable so this integration can be moved to `@sentry/core` and used in browser-based SDKs in the future.
-   *
-   * @hidden
-   */
-  addRequestData: typeof addRequestDataToEvent;
+  transactionNamingScheme?: TransactionNamingScheme;
 };
 
 const DEFAULT_OPTIONS = {
-  addRequestData: addRequestDataToEvent,
   include: {
     cookies: true,
     data: true,
     headers: true,
     ip: false,
     query_string: true,
     url: true,
-    user: DEFAULT_USER_INCLUDES,
+    user: {
+      id: true,
+      username: true,
+      email: true,
+    },
   },
   transactionNamingScheme: 'methodpath',
 };
@@ -64,12 +60,20 @@ export class RequestData implements Integration {
    */
   public name: string = RequestData.id;
 
-  private _options: RequestDataOptions;
+  /**
+   * Function for adding request data to event. Defaults to `addRequestDataToEvent` from `@sentry/node` for now, but
+   * left as a property so this integration can be moved to `@sentry/core` as a base class in case we decide to use
+   * something similar in browser-based SDKs in the future.
+   */
+  protected _addRequestData: (event: Event, req: PolymorphicRequest, options?: { [key: string]: unknown }) => Event;
+
+  private _options: Required<RequestDataIntegrationOptions>;
 
   /**
    * @inheritDoc
    */
-  public constructor(options: Partial<RequestDataOptions> = {}) {
+  public constructor(options: RequestDataIntegrationOptions = {}) {
+    this._addRequestData = addRequestDataToEvent;
     this._options = {
       ...DEFAULT_OPTIONS,
       ...options,
@@ -79,6 +83,14 @@ export class RequestData implements Integration {
         method: true,
         ...DEFAULT_OPTIONS.include,
         ...options.include,
+        user:
+          options.include && typeof options.include.user === 'boolean'
+            ? options.include.user
+            : {
+                ...DEFAULT_OPTIONS.include.user,
+                // Unclear why TS still thinks `options.include.user` could be a boolean at this point
+                ...((options.include || {}).user as Record<string, boolean>),
+              },
       },
     };
   }
@@ -91,7 +103,7 @@ export class RequestData implements Integration {
     // the moment it lives here, though, until https://github.com/getsentry/sentry-javascript/issues/5718 is addressed.
     // (TL;DR: Those functions touch many parts of the repo in many different ways, and need to be clened up. Once
     // that's happened, it will be easier to add this logic in without worrying about unexpected side effects.)
-    const { include, addRequestData, transactionNamingScheme } = this._options;
+    const { transactionNamingScheme } = this._options;
 
     addGlobalEventProcessor(event => {
       const hub = getCurrentHub();
@@ -104,7 +116,9 @@ export class RequestData implements Integration {
         return event;
       }
 
-      const processedEvent = addRequestData(event, req, { include: formatIncludeOption(include) });
+      const addRequestDataOptions = convertReqDataIntegrationOptsToAddReqDataOpts(this._options);
+
+      const processedEvent = this._addRequestData(event, req, addRequestDataOptions);
 
       // Transaction events already have the right `transaction` value
       if (event.type === 'transaction' || transactionNamingScheme === 'handler') {
@@ -138,12 +152,15 @@ export class RequestData implements Integration {
   }
 }
 
-/** Convert `include` option to match what `addRequestDataToEvent` expects */
+/** Convert this integration's options to match what `addRequestDataToEvent` expects */
 /** TODO: Can possibly be deleted once https://github.com/getsentry/sentry-javascript/issues/5718 is fixed */
-function formatIncludeOption(
-  integrationInclude: RequestDataOptions['include'] = {},
-): AddRequestDataToEventOptions['include'] {
-  const { ip, user, ...requestOptions } = integrationInclude;
+function convertReqDataIntegrationOptsToAddReqDataOpts(
+  integrationOptions: Required<RequestDataIntegrationOptions>,
+): AddRequestDataToEventOptions {
+  const {
+    transactionNamingScheme,
+    include: { ip, user, ...requestOptions },
+  } = integrationOptions;
 
   const requestIncludeKeys: string[] = [];
   for (const [key, value] of Object.entries(requestOptions)) {
@@ -152,10 +169,28 @@ function formatIncludeOption(
     }
   }
 
+  let addReqDataUserOpt;
+  if (user === undefined) {
+    addReqDataUserOpt = true;
+  } else if (typeof user === 'boolean') {
+    addReqDataUserOpt = user;
+  } else {
+    const userIncludeKeys: string[] = [];
+    for (const [key, value] of Object.entries(user)) {
+      if (value) {
+        userIncludeKeys.push(key);
+      }
+    }
+    addReqDataUserOpt = userIncludeKeys;
+  }
+
   return {
-    ip,
-    user,
-    request: requestIncludeKeys.length !== 0 ? requestIncludeKeys : undefined,
+    include: {
+      ip,
+      user: addReqDataUserOpt,
+      request: requestIncludeKeys.length !== 0 ? requestIncludeKeys : undefined,
+      transaction: transactionNamingScheme,
+    },
   };
 }
 

packages/node/src/requestdata.ts

@@ -15,15 +15,15 @@ export const DEFAULT_USER_INCLUDES = ['id', 'username', 'email'];
 /**
  * Options deciding what parts of the request to use when enhancing an event
  */
-export interface AddRequestDataToEventOptions {
+export type AddRequestDataToEventOptions = {
   /** Flags controlling whether each type of data should be added to the event */
   include?: {
     ip?: boolean;
     request?: boolean | Array<typeof DEFAULT_REQUEST_INCLUDES[number]>;
     transaction?: boolean | TransactionNamingScheme;
     user?: boolean | Array<typeof DEFAULT_USER_INCLUDES[number]>;
   };
-}
+};
 
 export type TransactionNamingScheme = 'path' | 'methodPath' | 'handler';
 

packages/node/test/integrations/requestdata.test.ts

@@ -0,0 +1,103 @@
+import { getCurrentHub, Hub, makeMain } from '@sentry/core';
+import { Event, EventProcessor } from '@sentry/types';
+import * as http from 'http';
+
+import { NodeClient } from '../../src/client';
+import { RequestData, RequestDataIntegrationOptions } from '../../src/integrations/requestdata';
+import * as requestDataModule from '../../src/requestdata';
+import { getDefaultNodeClientOptions } from '../helper/node-client-options';
+
+const addRequestDataToEventSpy = jest.spyOn(requestDataModule, 'addRequestDataToEvent');
+const requestDataEventProcessor = jest.fn();
+
+const headers = { ears: 'furry', nose: 'wet', tongue: 'spotted', cookie: 'favorite=zukes' };
+const method = 'wagging';
+const protocol = 'mutualsniffing';
+const hostname = 'the.dog.park';
+const path = '/by/the/trees/';
+const queryString = 'chase=me&please=thankyou';
+
+function initWithRequestDataIntegrationOptions(integrationOptions: RequestDataIntegrationOptions): void {
+  const setMockEventProcessor = (eventProcessor: EventProcessor) =>
+    requestDataEventProcessor.mockImplementationOnce(eventProcessor);
+
+  const requestDataIntegration = new RequestData({
+    ...integrationOptions,
+  });
+
+  const client = new NodeClient(
+    getDefaultNodeClientOptions({
+      dsn: 'https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012',
+      integrations: [requestDataIntegration],
+    }),
+  );
+  client.setupIntegrations = () => requestDataIntegration.setupOnce(setMockEventProcessor, getCurrentHub);
+  client.getIntegration = () => requestDataIntegration as any;
+
+  const hub = new Hub(client);
+
+  makeMain(hub);
+}
+
+describe('`RequestData` integration', () => {
+  let req: http.IncomingMessage, event: Event;
+
+  beforeEach(() => {
+    req = {
+      headers,
+      method,
+      protocol,
+      hostname,
+      originalUrl: `${path}?${queryString}`,
+    } as unknown as http.IncomingMessage;
+    event = { sdkProcessingMetadata: { request: req } };
+  });
+
+  afterEach(() => {
+    jest.clearAllMocks();
+  });
+
+  describe('option conversion', () => {
+    it('leaves `ip` and `user` at top level of `include`', () => {
+      initWithRequestDataIntegrationOptions({ include: { ip: false, user: true } });
+
+      requestDataEventProcessor(event);
+
+      const passedOptions = addRequestDataToEventSpy.mock.calls[0][2];
+
+      expect(passedOptions?.include).toEqual(expect.objectContaining({ ip: false, user: true }));
+    });
+
+    it('moves `transactionNamingScheme` to `transaction` include', () => {
+      initWithRequestDataIntegrationOptions({ transactionNamingScheme: 'path' });
+
+      requestDataEventProcessor(event);
+
+      const passedOptions = addRequestDataToEventSpy.mock.calls[0][2];
+
+      expect(passedOptions?.include).toEqual(expect.objectContaining({ transaction: 'path' }));
+    });
+
+    it('moves `true` request keys into `request` include, but omits `false` ones', async () => {
+      initWithRequestDataIntegrationOptions({ include: { data: true, cookies: false } });
+
+      requestDataEventProcessor(event);
+
+      const passedOptions = addRequestDataToEventSpy.mock.calls[0][2];
+
+      expect(passedOptions?.include?.request).toEqual(expect.arrayContaining(['data']));
+      expect(passedOptions?.include?.request).not.toEqual(expect.arrayContaining(['cookies']));
+    });
+
+    it('moves `true` user keys into `user` include, but omits `false` ones', async () => {
+      initWithRequestDataIntegrationOptions({ include: { user: { id: true, email: false } } });
+
+      requestDataEventProcessor(event);
+
+      const passedOptions = addRequestDataToEventSpy.mock.calls[0][2];
+
+      expect(passedOptions?.include?.user).toEqual(expect.arrayContaining(['id']));
+      expect(passedOptions?.include?.user).not.toEqual(expect.arrayContaining(['email']));
+    });
+  });
+});