From beded079dbdb4b538ba1c15f568274bba014acb9 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 12 Feb 2024 14:13:41 -0500 Subject: [PATCH 1/6] feat(v8): Remove requestData deprecations --- packages/core/src/integrations/requestdata.ts | 8 +- packages/node/src/handlers.ts | 49 +- packages/node/src/requestDataDeprecated.ts | 54 -- packages/node/src/sdk.ts | 1 - packages/node/test/handlers.test.ts | 1 - .../test/integrations/requestdata.test.ts | 3 +- packages/node/test/requestdata.test.ts | 582 ------------------ packages/serverless/src/gcpfunction/http.ts | 37 +- packages/serverless/test/gcpfunction.test.ts | 1 - packages/types/src/transaction.ts | 6 - packages/utils/src/requestdata.ts | 7 +- packages/utils/test/requestdata.test.ts | 493 +++++++++++++++ 12 files changed, 502 insertions(+), 740 deletions(-) delete mode 100644 packages/node/src/requestDataDeprecated.ts delete mode 100644 packages/node/test/requestdata.test.ts create mode 100644 packages/utils/test/requestdata.test.ts diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts index d3aa6bb7b850..e1db10390434 100644 --- a/packages/core/src/integrations/requestdata.ts +++ b/packages/core/src/integrations/requestdata.ts @@ -95,13 +95,7 @@ const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) = return event; } - // The Express request handler takes a similar `include` option to that which can be passed to this integration. - // If passed there, we store it in `sdkProcessingMetadata`. TODO(v8): Force express and GCP people to use this - // integration, so that all of this passing and conversion isn't necessary - const addRequestDataOptions = - sdkProcessingMetadata.requestDataOptionsFromExpressHandler || - sdkProcessingMetadata.requestDataOptionsFromGCPWrapper || - convertReqDataIntegrationOptsToAddReqDataOpts(_options); + const addRequestDataOptions = convertReqDataIntegrationOptsToAddReqDataOpts(_options); const processedEvent = _addRequestData(event, req, addRequestDataOptions); diff --git a/packages/node/src/handlers.ts b/packages/node/src/handlers.ts index b1140d0d9c28..89bbb85cc91a 100644 --- a/packages/node/src/handlers.ts +++ b/packages/node/src/handlers.ts @@ -18,7 +18,6 @@ import type { Span } from '@sentry/types'; import type { AddRequestDataToEventOptions } from '@sentry/utils'; import { addRequestDataToTransaction, - dropUndefinedKeys, extractPathForTransaction, extractRequestData, isString, @@ -29,8 +28,6 @@ import { import type { NodeClient } from './client'; import { DEBUG_BUILD } from './debug-build'; -// TODO (v8 / XXX) Remove this import -import type { ParseRequestOptions } from './requestDataDeprecated'; import { isAutoSessionTrackingEnabled } from './sdk'; /** @@ -115,37 +112,9 @@ export function tracingHandler(): ( }; } -export type RequestHandlerOptions = - // TODO (v8 / XXX) Remove ParseRequestOptions type and eslint override - // eslint-disable-next-line deprecation/deprecation - (ParseRequestOptions | AddRequestDataToEventOptions) & { - flushTimeout?: number; - }; - -/** - * Backwards compatibility shim which can be removed in v8. Forces the given options to follow the - * `AddRequestDataToEventOptions` interface. - * - * TODO (v8): Get rid of this, and stop passing `requestDataOptionsFromExpressHandler` to `setSDKProcessingMetadata`. - */ -function convertReqHandlerOptsToAddReqDataOpts( - reqHandlerOptions: RequestHandlerOptions = {}, -): AddRequestDataToEventOptions | undefined { - let addRequestDataOptions: AddRequestDataToEventOptions | undefined; - - if ('include' in reqHandlerOptions) { - addRequestDataOptions = { include: reqHandlerOptions.include }; - } else { - // eslint-disable-next-line deprecation/deprecation - const { ip, request, transaction, user } = reqHandlerOptions as ParseRequestOptions; - - if (ip || request || transaction || user) { - addRequestDataOptions = { include: dropUndefinedKeys({ ip, request, transaction, user }) }; - } - } - - return addRequestDataOptions; -} +export type RequestHandlerOptions = AddRequestDataToEventOptions & { + flushTimeout?: number; +}; /** * Express compatible request handler. @@ -154,9 +123,6 @@ function convertReqHandlerOptsToAddReqDataOpts( export function requestHandler( options?: RequestHandlerOptions, ): (req: http.IncomingMessage, res: http.ServerResponse, next: (error?: any) => void) => void { - // TODO (v8): Get rid of this - const requestDataOptions = convertReqHandlerOptsToAddReqDataOpts(options); - const client = getClient(); // Initialise an instance of SessionFlusher on the client when `autoSessionTracking` is enabled and the // `requestHandler` middleware is used indicating that we are running in SessionAggregates mode @@ -193,8 +159,6 @@ export function requestHandler( const scope = getCurrentScope(); scope.setSDKProcessingMetadata({ request: req, - // TODO (v8): Stop passing this - requestDataOptionsFromExpressHandler: requestDataOptions, }); const client = getClient(); @@ -372,7 +336,6 @@ export function trpcMiddleware(options: SentryTrpcMiddlewareOptions = {}) { } if (isThenable(maybePromiseResult)) { - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access Promise.resolve(maybePromiseResult).then( nextResult => { captureIfError(nextResult as any); @@ -389,9 +352,3 @@ export function trpcMiddleware(options: SentryTrpcMiddlewareOptions = {}) { return maybePromiseResult; }; } - -// TODO (v8 / #5257): Remove this -// eslint-disable-next-line deprecation/deprecation -export type { ParseRequestOptions, ExpressRequest } from './requestDataDeprecated'; -// eslint-disable-next-line deprecation/deprecation -export { parseRequest, extractRequestData } from './requestDataDeprecated'; diff --git a/packages/node/src/requestDataDeprecated.ts b/packages/node/src/requestDataDeprecated.ts deleted file mode 100644 index 74e0a9c98666..000000000000 --- a/packages/node/src/requestDataDeprecated.ts +++ /dev/null @@ -1,54 +0,0 @@ -/** - * Deprecated functions which are slated for removal in v8. When the time comes, this entire file can be deleted. - * - * See https://github.com/getsentry/sentry-javascript/pull/5257. - */ - -/* eslint-disable deprecation/deprecation */ -/* eslint-disable @typescript-eslint/no-explicit-any */ -import type { Event, ExtractedNodeRequestData, PolymorphicRequest } from '@sentry/types'; -import type { AddRequestDataToEventOptions } from '@sentry/utils'; -import { addRequestDataToEvent, extractRequestData as _extractRequestData } from '@sentry/utils'; - -/** - * @deprecated `Handlers.ExpressRequest` is deprecated and will be removed in v8. Use `PolymorphicRequest` instead. - */ -export type ExpressRequest = PolymorphicRequest; - -/** - * Normalizes data from the request object, accounting for framework differences. - * - * @deprecated `Handlers.extractRequestData` is deprecated and will be removed in v8. Use `extractRequestData` instead. - * - * @param req The request object from which to extract data - * @param keys An optional array of keys to include in the normalized data. - * @returns An object containing normalized request data - */ -export function extractRequestData(req: { [key: string]: any }, keys?: string[]): ExtractedNodeRequestData { - return _extractRequestData(req, { include: keys }); -} - -/** - * Options deciding what parts of the request to use when enhancing an event - * - * @deprecated `Handlers.ParseRequestOptions` is deprecated and will be removed in v8. Use - * `AddRequestDataToEventOptions` in `@sentry/utils` instead. - */ -export type ParseRequestOptions = AddRequestDataToEventOptions['include'] & { - serverName?: boolean; - version?: boolean; -}; - -/** - * Enriches passed event with request data. - * - * @deprecated `Handlers.parseRequest` is deprecated and will be removed in v8. Use `addRequestDataToEvent` instead. - * - * @param event Will be mutated and enriched with req data - * @param req Request object - * @param options object containing flags to enable functionality - * @hidden - */ -export function parseRequest(event: Event, req: ExpressRequest, options: ParseRequestOptions = {}): Event { - return addRequestDataToEvent(event, req, { include: options }); -} diff --git a/packages/node/src/sdk.ts b/packages/node/src/sdk.ts index 6aa6b0499ca2..f6f83aef9352 100644 --- a/packages/node/src/sdk.ts +++ b/packages/node/src/sdk.ts @@ -1,4 +1,3 @@ -/* eslint-disable max-lines */ import { endSession, functionToStringIntegration, diff --git a/packages/node/test/handlers.test.ts b/packages/node/test/handlers.test.ts index 34e00f06b9c6..56682446a1d8 100644 --- a/packages/node/test/handlers.test.ts +++ b/packages/node/test/handlers.test.ts @@ -179,7 +179,6 @@ describe('requestHandler', () => { const scope = getCurrentScope(); expect((scope as any)._sdkProcessingMetadata).toEqual({ request: req, - requestDataOptionsFromExpressHandler: requestHandlerOptions, }); }); }); diff --git a/packages/node/test/integrations/requestdata.test.ts b/packages/node/test/integrations/requestdata.test.ts index e73fe2fbda88..3f58f8779de6 100644 --- a/packages/node/test/integrations/requestdata.test.ts +++ b/packages/node/test/integrations/requestdata.test.ts @@ -82,11 +82,10 @@ describe('`RequestData` integration', () => { it('uses options from GCP wrapper', async () => { type GCPHandler = (req: PolymorphicRequest, res: http.ServerResponse) => void; - const mockGCPWrapper = (origHandler: GCPHandler, options: Record): GCPHandler => { + const mockGCPWrapper = (origHandler: GCPHandler, _options: Record): GCPHandler => { const wrappedHandler: GCPHandler = (req, res) => { getCurrentScope().setSDKProcessingMetadata({ request: req, - requestDataOptionsFromGCPWrapper: options, }); origHandler(req, res); }; diff --git a/packages/node/test/requestdata.test.ts b/packages/node/test/requestdata.test.ts deleted file mode 100644 index 6b4e5e13101a..000000000000 --- a/packages/node/test/requestdata.test.ts +++ /dev/null @@ -1,582 +0,0 @@ -/* eslint-disable deprecation/deprecation */ - -// TODO (v8 / #5257): Remove everything related to the deprecated functions and move tests into `@sentry/utils` - -import type * as net from 'net'; -import type { Event, PolymorphicRequest, TransactionSource, User } from '@sentry/types'; -import type { AddRequestDataToEventOptions } from '@sentry/utils'; -import { - addRequestDataToEvent, - extractPathForTransaction, - extractRequestData as newExtractRequestData, -} from '@sentry/utils'; - -import type { ExpressRequest } from '../src/requestDataDeprecated'; -import { extractRequestData as oldExtractRequestData, parseRequest } from '../src/requestDataDeprecated'; - -// TODO (v8 / #5257): Remove `describe.each` wrapper, remove `formatArgs` wrapper, reformat args in tests, and use only -// `addRequestDataToEvent` -describe.each([parseRequest, addRequestDataToEvent])( - 'backwards compatibility of `parseRequest` rename and move', - fn => { - /** Rearrage and cast args correctly for each version of the function */ - function formatArgs( - fn: typeof parseRequest | typeof addRequestDataToEvent, - event: Event, - req: any, - include?: AddRequestDataToEventOptions['include'], - ): Parameters | Parameters { - if (fn.name === 'parseRequest') { - return [event, req as ExpressRequest, include]; - } else { - return [event, req as PolymorphicRequest, { include }]; - } - } - - describe(fn, () => { - let mockEvent: Event; - let mockReq: { [key: string]: any }; - - beforeEach(() => { - mockEvent = {}; - mockReq = { - baseUrl: '/routerMountPath', - body: 'foo', - cookies: { test: 'test' }, - headers: { - host: 'mattrobenolt.com', - }, - method: 'POST', - originalUrl: '/routerMountPath/subpath/specificValue?querystringKey=querystringValue', - path: '/subpath/specificValue', - query: { - querystringKey: 'querystringValue', - }, - route: { - path: '/subpath/:parameterName', - stack: [ - { - name: 'parameterNameRouteHandler', - }, - ], - }, - url: '/subpath/specificValue?querystringKey=querystringValue', - user: { - custom_property: 'foo', - email: 'tobias@mail.com', - id: 123, - username: 'tobias', - }, - }; - }); - - describe(`${fn.name}.user properties`, () => { - const DEFAULT_USER_KEYS = ['id', 'username', 'email']; - const CUSTOM_USER_KEYS = ['custom_property']; - - test(`${fn.name}.user only contains the default properties from the user`, () => { - const [event, req, options] = formatArgs(fn, mockEvent, mockReq); - const parsedRequest: Event = fn(event, req, options); - - expect(Object.keys(parsedRequest.user as User)).toEqual(DEFAULT_USER_KEYS); - }); - - test(`${fn.name}.user only contains the custom properties specified in the options.user array`, () => { - const optionsWithCustomUserKeys = { - user: CUSTOM_USER_KEYS, - }; - - const [event, req, options] = formatArgs(fn, mockEvent, mockReq, optionsWithCustomUserKeys); - const parsedRequest: Event = fn(event, req, options); - - expect(Object.keys(parsedRequest.user as User)).toEqual(CUSTOM_USER_KEYS); - }); - - test(`${fn.name}.user doesnt blow up when someone passes non-object value`, () => { - const reqWithUser = { - ...mockReq, - user: 'wat', - }; - - const [event, req, options] = formatArgs(fn, mockEvent, reqWithUser); - const parsedRequest: Event = fn(event, req, options); - - expect(parsedRequest.user).toBeUndefined(); - }); - }); - - describe(`${fn.name}.ip property`, () => { - test('can be extracted from req.ip', () => { - const mockReqWithIP = { - ...mockReq, - ip: '123', - }; - const optionsWithIP = { - ip: true, - }; - - const [event, req, options] = formatArgs(fn, mockEvent, mockReqWithIP, optionsWithIP); - const parsedRequest: Event = fn(event, req, options); - - expect(parsedRequest.user!.ip_address).toEqual('123'); - }); - - test('can extract from req.socket.remoteAddress', () => { - const reqWithIPInSocket = { - ...mockReq, - socket: { - remoteAddress: '321', - } as net.Socket, - }; - const optionsWithIP = { - ip: true, - }; - - const [event, req, options] = formatArgs(fn, mockEvent, reqWithIPInSocket, optionsWithIP); - const parsedRequest: Event = fn(event, req, options); - - expect(parsedRequest.user!.ip_address).toEqual('321'); - }); - }); - - describe(`${fn.name}.request properties`, () => { - test(`${fn.name}.request only contains the default set of properties from the request`, () => { - const DEFAULT_REQUEST_PROPERTIES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url']; - - const [event, req, options] = formatArgs(fn, mockEvent, mockReq); - const parsedRequest: Event = fn(event, req, options); - - expect(Object.keys(parsedRequest.request!)).toEqual(DEFAULT_REQUEST_PROPERTIES); - }); - - test(`${fn.name}.request only contains the specified properties in the options.request array`, () => { - const INCLUDED_PROPERTIES = ['data', 'headers', 'query_string', 'url']; - const optionsWithRequestIncludes = { - request: INCLUDED_PROPERTIES, - }; - - const [event, req, options] = formatArgs(fn, mockEvent, mockReq, optionsWithRequestIncludes); - const parsedRequest: Event = fn(event, req, options); - - expect(Object.keys(parsedRequest.request!)).toEqual(INCLUDED_PROPERTIES); - }); - - test.each([ - [undefined, true], - ['GET', false], - ['HEAD', false], - ])( - `${fn.name}.request skips \`body\` property for GET and HEAD requests - %s method`, - (method, shouldIncludeBodyData) => { - const reqWithMethod = { ...mockReq, method }; - - const [event, req, options] = formatArgs(fn, mockEvent, reqWithMethod); - const parsedRequest: Event = fn(event, req, options); - - if (shouldIncludeBodyData) { - expect(parsedRequest.request).toHaveProperty('data'); - } else { - expect(parsedRequest.request).not.toHaveProperty('data'); - } - }, - ); - }); - - describe(`${fn.name}.transaction property`, () => { - test('extracts method and full route path by default`', () => { - const [event, req, options] = formatArgs(fn, mockEvent, mockReq); - const parsedRequest: Event = fn(event, req, options); - - expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/:parameterName'); - }); - - test('extracts method and full path by default when mountpoint is `/`', () => { - mockReq.originalUrl = mockReq.originalUrl.replace('/routerMountpath', ''); - mockReq.baseUrl = ''; - - const [event, req, options] = formatArgs(fn, mockEvent, mockReq); - const parsedRequest: Event = fn(event, req, options); - - // `subpath/` is the full path here, because there's no router mount path - expect(parsedRequest.transaction).toEqual('POST /subpath/:parameterName'); - }); - - test('fallback to method and `originalUrl` if route is missing', () => { - delete mockReq.route; - - const [event, req, options] = formatArgs(fn, mockEvent, mockReq); - const parsedRequest: Event = fn(event, req, options); - - expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/specificValue'); - }); - - test('can extract path only instead if configured', () => { - const optionsWithPathTransaction = { transaction: 'path' } as const; - - const [event, req, options] = formatArgs(fn, mockEvent, mockReq, optionsWithPathTransaction); - const parsedRequest: Event = fn(event, req, options); - - expect(parsedRequest.transaction).toEqual('/routerMountPath/subpath/:parameterName'); - }); - - test('can extract handler name instead if configured', () => { - const optionsWithHandlerTransaction = { transaction: 'handler' } as const; - - const [event, req, options] = formatArgs(fn, mockEvent, mockReq, optionsWithHandlerTransaction); - const parsedRequest: Event = fn(event, req, options); - - expect(parsedRequest.transaction).toEqual('parameterNameRouteHandler'); - }); - }); - }); - }, -); - -// TODO (v8 / #5257): Remove `describe.each` wrapper, remove `formatArgs` wrapper, reformat args in tests, use only -// `newExtractRequestData`, and rename `newExtractRequestData` to just `extractRequestData` -Object.defineProperty(oldExtractRequestData, 'name', { - value: 'oldExtractRequestData', -}); -Object.defineProperty(newExtractRequestData, 'name', { - value: 'newExtractRequestData', -}); -describe.each([oldExtractRequestData, newExtractRequestData])( - 'backwards compatibility of `extractRequestData` move', - fn => { - /** Rearrage and cast args correctly for each version of the function */ - function formatArgs( - fn: typeof oldExtractRequestData | typeof newExtractRequestData, - req: any, - include?: string[], - ): Parameters | Parameters { - if (fn.name === 'oldExtractRequestData') { - return [req as ExpressRequest, include] as Parameters; - } else { - return [req as PolymorphicRequest, { include }] as Parameters; - } - } - - describe(fn, () => { - describe('default behaviour', () => { - test('node', () => { - const mockReq = { - headers: { host: 'example.com' }, - method: 'GET', - socket: { encrypted: true }, - originalUrl: '/', - }; - - const [req, options] = formatArgs(fn, mockReq); - - expect(fn(req, options as any)).toEqual({ - cookies: {}, - headers: { - host: 'example.com', - }, - method: 'GET', - query_string: undefined, - url: 'https://example.com/', - }); - }); - - test('degrades gracefully without request data', () => { - const mockReq = {}; - - const [req, options] = formatArgs(fn, mockReq); - - expect(fn(req, options as any)).toEqual({ - cookies: {}, - headers: {}, - method: undefined, - query_string: undefined, - url: 'http://', - }); - }); - }); - - describe('headers', () => { - it('removes the `Cookie` header from requestdata.headers, if `cookies` is not set in the options', () => { - const mockReq = { - cookies: { foo: 'bar' }, - headers: { cookie: 'foo=bar', otherHeader: 'hello' }, - }; - const optionsWithCookies = ['headers']; - - const [req, options] = formatArgs(fn, mockReq, optionsWithCookies); - - expect(fn(req, options as any)).toStrictEqual({ - headers: { otherHeader: 'hello' }, - }); - }); - - it('includes the `Cookie` header in requestdata.headers, if `cookies` is set in the options', () => { - const mockReq = { - cookies: { foo: 'bar' }, - headers: { cookie: 'foo=bar', otherHeader: 'hello' }, - }; - const optionsWithCookies = ['headers', 'cookies']; - - const [req, options] = formatArgs(fn, mockReq, optionsWithCookies); - - expect(fn(req, options as any)).toStrictEqual({ - headers: { otherHeader: 'hello', cookie: 'foo=bar' }, - cookies: { foo: 'bar' }, - }); - }); - }); - - describe('cookies', () => { - it('uses `req.cookies` if available', () => { - const mockReq = { - cookies: { foo: 'bar' }, - }; - const optionsWithCookies = ['cookies']; - - const [req, options] = formatArgs(fn, mockReq, optionsWithCookies); - - expect(fn(req, options as any)).toEqual({ - cookies: { foo: 'bar' }, - }); - }); - - it('parses the cookie header', () => { - const mockReq = { - headers: { - cookie: 'foo=bar;', - }, - }; - const optionsWithCookies = ['cookies']; - - const [req, options] = formatArgs(fn, mockReq, optionsWithCookies); - - expect(fn(req, options as any)).toEqual({ - cookies: { foo: 'bar' }, - }); - }); - - it('falls back if no cookies are defined', () => { - const mockReq = {}; - const optionsWithCookies = ['cookies']; - - const [req, options] = formatArgs(fn, mockReq, optionsWithCookies); - - expect(fn(req, options as any)).toEqual({ - cookies: {}, - }); - }); - }); - - describe('data', () => { - it('includes data from `req.body` if available', () => { - const mockReq = { - method: 'POST', - headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, - body: 'foo=bar', - }; - const optionsWithData = ['data']; - - const [req, options] = formatArgs(fn, mockReq, optionsWithData); - - expect(fn(req, options as any)).toEqual({ - data: 'foo=bar', - }); - }); - - it('encodes JSON body contents back to a string', () => { - const mockReq = { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: { foo: 'bar' }, - }; - const optionsWithData = ['data']; - - const [req, options] = formatArgs(fn, mockReq, optionsWithData); - - expect(fn(req, options as any)).toEqual({ - data: '{"foo":"bar"}', - }); - }); - }); - - describe('query_string', () => { - it('parses the query parms from the url', () => { - const mockReq = { - headers: { host: 'example.com' }, - secure: true, - originalUrl: '/?foo=bar', - }; - const optionsWithQueryString = ['query_string']; - - const [req, options] = formatArgs(fn, mockReq, optionsWithQueryString); - - expect(fn(req, options as any)).toEqual({ - query_string: 'foo=bar', - }); - }); - - it('gracefully degrades if url cannot be determined', () => { - const mockReq = {}; - const optionsWithQueryString = ['query_string']; - - const [req, options] = formatArgs(fn, mockReq, optionsWithQueryString); - - expect(fn(req, options as any)).toEqual({ - query_string: undefined, - }); - }); - }); - - describe('url', () => { - test('express/koa', () => { - const mockReq = { - host: 'example.com', - protocol: 'https', - url: '/', - }; - const optionsWithURL = ['url']; - - const [req, options] = formatArgs(fn, mockReq, optionsWithURL); - - expect(fn(req, options as any)).toEqual({ - url: 'https://example.com/', - }); - }); - - test('node', () => { - const mockReq = { - headers: { host: 'example.com' }, - socket: { encrypted: true }, - originalUrl: '/', - }; - const optionsWithURL = ['url']; - - const [req, options] = formatArgs(fn, mockReq, optionsWithURL); - - expect(fn(req, options as any)).toEqual({ - url: 'https://example.com/', - }); - }); - }); - - describe('custom key', () => { - it('includes the custom key if present', () => { - const mockReq = { - httpVersion: '1.1', - }; - const optionsWithCustomKey = ['httpVersion']; - - const [req, options] = formatArgs(fn, mockReq, optionsWithCustomKey); - - expect(fn(req, options as any)).toEqual({ - httpVersion: '1.1', - }); - }); - - it('gracefully degrades if the custom key is missing', () => { - const mockReq = {}; - const optionsWithCustomKey = ['httpVersion']; - - const [req, options] = formatArgs(fn, mockReq, optionsWithCustomKey); - - expect(fn(req, options as any)).toEqual({}); - }); - }); - }); - }, -); - -describe('extractPathForTransaction', () => { - it.each([ - [ - 'extracts a parameterized route and method if available', - { - method: 'get', - baseUrl: '/api/users', - route: { path: '/:id/details' }, - originalUrl: '/api/users/123/details', - } as PolymorphicRequest, - { path: true, method: true }, - 'GET /api/users/:id/details', - 'route' as TransactionSource, - ], - [ - 'ignores the method if specified', - { - method: 'get', - baseUrl: '/api/users', - route: { path: '/:id/details' }, - originalUrl: '/api/users/123/details', - } as PolymorphicRequest, - { path: true, method: false }, - '/api/users/:id/details', - 'route' as TransactionSource, - ], - [ - 'ignores the path if specified', - { - method: 'get', - baseUrl: '/api/users', - route: { path: '/:id/details' }, - originalUrl: '/api/users/123/details', - } as PolymorphicRequest, - { path: false, method: true }, - 'GET', - 'route' as TransactionSource, - ], - [ - 'returns an empty string if everything should be ignored', - { - method: 'get', - baseUrl: '/api/users', - route: { path: '/:id/details' }, - originalUrl: '/api/users/123/details', - } as PolymorphicRequest, - { path: false, method: false }, - '', - 'route' as TransactionSource, - ], - [ - 'falls back to the raw URL if no parameterized route is available', - { - method: 'get', - baseUrl: '/api/users', - originalUrl: '/api/users/123/details', - } as PolymorphicRequest, - { path: true, method: true }, - 'GET /api/users/123/details', - 'url' as TransactionSource, - ], - ])( - '%s', - ( - _: string, - req: PolymorphicRequest, - options: { path?: boolean; method?: boolean }, - expectedRoute: string, - expectedSource: TransactionSource, - ) => { - const [route, source] = extractPathForTransaction(req, options); - - expect(route).toEqual(expectedRoute); - expect(source).toEqual(expectedSource); - }, - ); - - it('overrides the requests information with a custom route if specified', () => { - const req = { - method: 'get', - baseUrl: '/api/users', - route: { path: '/:id/details' }, - originalUrl: '/api/users/123/details', - } as PolymorphicRequest; - - const [route, source] = extractPathForTransaction(req, { - path: true, - method: true, - customRoute: '/other/path/:id/details', - }); - - expect(route).toEqual('GET /other/path/:id/details'); - expect(source).toEqual('route'); - }); -}); diff --git a/packages/serverless/src/gcpfunction/http.ts b/packages/serverless/src/gcpfunction/http.ts index e02093acf438..d04c00a62521 100644 --- a/packages/serverless/src/gcpfunction/http.ts +++ b/packages/serverless/src/gcpfunction/http.ts @@ -5,7 +5,6 @@ import { handleCallbackErrors, setHttpStatus, } from '@sentry/core'; -import type { AddRequestDataToEventOptions } from '@sentry/node'; import { continueTrace, startSpanManual } from '@sentry/node'; import { getCurrentScope } from '@sentry/node'; import { captureException, flush } from '@sentry/node'; @@ -15,24 +14,6 @@ import { DEBUG_BUILD } from '../debug-build'; import { domainify, markEventUnhandled, proxyFunction } from './../utils'; import type { HttpFunction, WrapperOptions } from './general'; -// TODO (v8 / #5257): Remove this whole old/new business and just use the new stuff -type ParseRequestOptions = AddRequestDataToEventOptions['include'] & { - serverName?: boolean; - version?: boolean; -}; - -interface OldHttpFunctionWrapperOptions extends WrapperOptions { - /** - * @deprecated Use `addRequestDataToEventOptions` instead. - */ - parseRequestOptions: ParseRequestOptions; -} -interface NewHttpFunctionWrapperOptions extends WrapperOptions { - addRequestDataToEventOptions: AddRequestDataToEventOptions; -} - -export type HttpFunctionWrapperOptions = OldHttpFunctionWrapperOptions | NewHttpFunctionWrapperOptions; - /** * Wraps an HTTP function handler adding it error capture and tracing capabilities. * @@ -40,10 +21,7 @@ export type HttpFunctionWrapperOptions = OldHttpFunctionWrapperOptions | NewHttp * @param options Options * @returns HTTP handler */ -export function wrapHttpFunction( - fn: HttpFunction, - wrapOptions: Partial = {}, -): HttpFunction { +export function wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial = {}): HttpFunction { const wrap = (f: HttpFunction): HttpFunction => domainify(_wrapHttpFunction(f, wrapOptions)); let overrides: Record | undefined; @@ -59,17 +37,7 @@ export function wrapHttpFunction( } /** */ -function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial = {}): HttpFunction { - // TODO (v8 / #5257): Switch to using `addRequestDataToEventOptions` - // eslint-disable-next-line deprecation/deprecation - const { parseRequestOptions } = wrapOptions as OldHttpFunctionWrapperOptions; - - const options: HttpFunctionWrapperOptions = { - flushTimeout: 2000, - // TODO (v8 / xxx): Remove this line, since `addRequestDataToEventOptions` will be included in the spread of `wrapOptions` - addRequestDataToEventOptions: parseRequestOptions ? { include: parseRequestOptions } : {}, - ...wrapOptions, - }; +function _wrapHttpFunction(fn: HttpFunction, options: Partial = { flushTimeout: 2000 }): HttpFunction { return (req, res) => { const reqMethod = (req.method || '').toUpperCase(); const reqUrl = stripUrlQueryAndFragment(req.originalUrl || req.url || ''); @@ -90,7 +58,6 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial { getCurrentScope().setSDKProcessingMetadata({ request: req, - requestDataOptionsFromGCPWrapper: options.addRequestDataToEventOptions, }); if (span instanceof Transaction) { diff --git a/packages/serverless/test/gcpfunction.test.ts b/packages/serverless/test/gcpfunction.test.ts index cde69e6b22d2..ab9781bdc6f1 100644 --- a/packages/serverless/test/gcpfunction.test.ts +++ b/packages/serverless/test/gcpfunction.test.ts @@ -247,7 +247,6 @@ describe('GCPFunction', () => { headers: { host: 'hostname', 'content-type': 'application/json' }, body: { foo: 'bar' }, }, - requestDataOptionsFromGCPWrapper: { include: { ip: true } }, }); }); diff --git a/packages/types/src/transaction.ts b/packages/types/src/transaction.ts index fbcf8b38f02d..49b92b2e218a 100644 --- a/packages/types/src/transaction.ts +++ b/packages/types/src/transaction.ts @@ -205,12 +205,6 @@ export interface TransactionMetadata { /** For transactions tracing server-side request handling, the request being tracked. */ request?: PolymorphicRequest; - /** Compatibility shim for transitioning to the `RequestData` integration. The options passed to our Express request - * handler controlling what request data is added to the event. - * TODO (v8): This should go away - */ - requestDataOptionsFromExpressHandler?: { [key: string]: unknown }; - /** For transactions tracing server-side request handling, the path of the request being tracked. */ /** TODO: If we rm -rf `instrumentServer`, this can go, too */ requestPath?: string; diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index 85b748dadaba..7b8c61d27750 100644 --- a/packages/utils/src/requestdata.ts +++ b/packages/utils/src/requestdata.ts @@ -189,8 +189,6 @@ export function extractRequestData( req: PolymorphicRequest, options?: { include?: string[]; - // TODO(v8): Remove this paramater - deps?: InjectedNodeDeps; }, ): ExtractedNodeRequestData { const { include = DEFAULT_REQUEST_INCLUDES } = options || {}; @@ -258,7 +256,6 @@ export function extractRequestData( // query string: // node: req.url (raw) // express, koa, nextjs: req.query - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access requestData.query_string = extractQueryParams(req); break; } @@ -309,8 +306,8 @@ export function addRequestDataToEvent( if (include.request) { const extractedRequestData = Array.isArray(include.request) - ? extractRequestData(req, { include: include.request, deps: options && options.deps }) - : extractRequestData(req, { deps: options && options.deps }); + ? extractRequestData(req, { include: include.request }) + : extractRequestData(req); event.request = { ...event.request, diff --git a/packages/utils/test/requestdata.test.ts b/packages/utils/test/requestdata.test.ts new file mode 100644 index 000000000000..3bd5ac507268 --- /dev/null +++ b/packages/utils/test/requestdata.test.ts @@ -0,0 +1,493 @@ +import type * as net from 'net'; +import type { Event, PolymorphicRequest, TransactionSource, User } from '@sentry/types'; +import { addRequestDataToEvent, extractPathForTransaction, extractRequestData } from '@sentry/utils'; + +describe('addRequestDataToEvent', () => { + let mockEvent: Event; + let mockReq: { [key: string]: any }; + + beforeEach(() => { + mockEvent = {}; + mockReq = { + baseUrl: '/routerMountPath', + body: 'foo', + cookies: { test: 'test' }, + headers: { + host: 'example.org', + }, + method: 'POST', + originalUrl: '/routerMountPath/subpath/specificValue?querystringKey=querystringValue', + path: '/subpath/specificValue', + query: { + querystringKey: 'querystringValue', + }, + route: { + path: '/subpath/:parameterName', + stack: [ + { + name: 'parameterNameRouteHandler', + }, + ], + }, + url: '/subpath/specificValue?querystringKey=querystringValue', + user: { + custom_property: 'foo', + email: 'tobias@mail.com', + id: 123, + username: 'tobias', + }, + }; + }); + + describe('addRequestDataToEvent user properties', () => { + const DEFAULT_USER_KEYS = ['id', 'username', 'email']; + const CUSTOM_USER_KEYS = ['custom_property']; + + test('user only contains the default properties from the user', () => { + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); + expect(Object.keys(parsedRequest.user as User)).toEqual(DEFAULT_USER_KEYS); + }); + + test('user only contains the custom properties specified in the options.user array', () => { + const optionsWithCustomUserKeys = { + include: { + user: CUSTOM_USER_KEYS, + }, + }; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithCustomUserKeys); + + expect(Object.keys(parsedRequest.user as User)).toEqual(CUSTOM_USER_KEYS); + }); + + test('setting user doesnt blow up when someone passes non-object value', () => { + const reqWithUser = { + ...mockReq, + // intentionally setting user to a non-object value, hence the as any cast + user: 'wat', + } as any; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithUser); + + expect(parsedRequest.user).toBeUndefined(); + }); + }); + + describe('addRequestDataToEvent ip property', () => { + test('can be extracted from req.ip', () => { + const mockReqWithIP = { + ...mockReq, + ip: '123', + }; + const optionsWithIP = { + include: { + ip: true, + }, + }; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReqWithIP, optionsWithIP); + + expect(parsedRequest.user!.ip_address).toEqual('123'); + }); + + test('can extract from req.socket.remoteAddress', () => { + const reqWithIPInSocket = { + ...mockReq, + socket: { + remoteAddress: '321', + } as net.Socket, + }; + const optionsWithIP = { + include: { + ip: true, + }, + }; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithIPInSocket, optionsWithIP); + + expect(parsedRequest.user!.ip_address).toEqual('321'); + }); + }); + + describe('request properties', () => { + test('request only contains the default set of properties from the request', () => { + const DEFAULT_REQUEST_PROPERTIES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url']; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); + + expect(Object.keys(parsedRequest.request!)).toEqual(DEFAULT_REQUEST_PROPERTIES); + }); + + test('request only contains the specified properties in the options.request array', () => { + const INCLUDED_PROPERTIES = ['data', 'headers', 'query_string', 'url']; + const optionsWithRequestIncludes = { + include: { + request: INCLUDED_PROPERTIES, + }, + }; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithRequestIncludes); + + expect(Object.keys(parsedRequest.request!)).toEqual(INCLUDED_PROPERTIES); + }); + + test.each([ + [undefined, true], + ['GET', false], + ['HEAD', false], + ])('request skips `body` property for GET and HEAD requests - %s method', (method, shouldIncludeBodyData) => { + const reqWithMethod = { ...mockReq, method }; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, reqWithMethod); + + if (shouldIncludeBodyData) { + expect(parsedRequest.request).toHaveProperty('data'); + } else { + expect(parsedRequest.request).not.toHaveProperty('data'); + } + }); + }); + + describe('transaction property', () => { + test('extracts method and full route path by default`', () => { + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); + + expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/:parameterName'); + }); + + test('extracts method and full path by default when mountpoint is `/`', () => { + mockReq.originalUrl = mockReq.originalUrl.replace('/routerMountpath', ''); + mockReq.baseUrl = ''; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); + + // `subpath/` is the full path here, because there's no router mount path + expect(parsedRequest.transaction).toEqual('POST /subpath/:parameterName'); + }); + + test('fallback to method and `originalUrl` if route is missing', () => { + delete mockReq.route; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq); + + expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/specificValue'); + }); + + test('can extract path only instead if configured', () => { + const optionsWithPathTransaction = { + include: { + transaction: 'path', + }, + } as const; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithPathTransaction); + + expect(parsedRequest.transaction).toEqual('/routerMountPath/subpath/:parameterName'); + }); + + test('can extract handler name instead if configured', () => { + const optionsWithHandlerTransaction = { + include: { + transaction: 'handler', + }, + } as const; + + const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithHandlerTransaction); + + expect(parsedRequest.transaction).toEqual('parameterNameRouteHandler'); + }); + }); +}); + +describe('extractRequestData', () => { + describe('default behaviour', () => { + test('node', () => { + const mockReq = { + headers: { host: 'example.com' }, + method: 'GET', + socket: { encrypted: true }, + originalUrl: '/', + }; + + expect(extractRequestData(mockReq)).toEqual({ + cookies: {}, + headers: { + host: 'example.com', + }, + method: 'GET', + query_string: undefined, + url: 'https://example.com/', + }); + }); + + test('degrades gracefully without request data', () => { + const mockReq = {}; + + expect(extractRequestData(mockReq)).toEqual({ + cookies: {}, + headers: {}, + method: undefined, + query_string: undefined, + url: 'http://', + }); + }); + }); + + describe('headers', () => { + it('removes the `Cookie` header from requestdata.headers, if `cookies` is not set in the options', () => { + const mockReq = { + cookies: { foo: 'bar' }, + headers: { cookie: 'foo=bar', otherHeader: 'hello' }, + }; + const options = { include: ['headers'] }; + + expect(extractRequestData(mockReq, options)).toStrictEqual({ + headers: { otherHeader: 'hello' }, + }); + }); + + it('includes the `Cookie` header in requestdata.headers, if `cookies` is set in the options', () => { + const mockReq = { + cookies: { foo: 'bar' }, + headers: { cookie: 'foo=bar', otherHeader: 'hello' }, + }; + const optionsWithCookies = { include: ['headers', 'cookies'] }; + + expect(extractRequestData(mockReq, optionsWithCookies)).toStrictEqual({ + headers: { otherHeader: 'hello', cookie: 'foo=bar' }, + cookies: { foo: 'bar' }, + }); + }); + }); + + describe('cookies', () => { + it('uses `req.cookies` if available', () => { + const mockReq = { + cookies: { foo: 'bar' }, + }; + const optionsWithCookies = { include: ['cookies'] }; + + expect(extractRequestData(mockReq, optionsWithCookies)).toEqual({ + cookies: { foo: 'bar' }, + }); + }); + + it('parses the cookie header', () => { + const mockReq = { + headers: { + cookie: 'foo=bar;', + }, + }; + const optionsWithCookies = { include: ['cookies'] }; + + expect(extractRequestData(mockReq, optionsWithCookies)).toEqual({ + cookies: { foo: 'bar' }, + }); + }); + + it('falls back if no cookies are defined', () => { + const mockReq = {}; + const optionsWithCookies = { include: ['cookies'] }; + + expect(extractRequestData(mockReq, optionsWithCookies)).toEqual({ + cookies: {}, + }); + }); + }); + + describe('data', () => { + it('includes data from `req.body` if available', () => { + const mockReq = { + method: 'POST', + headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + body: 'foo=bar', + }; + const optionsWithData = { include: ['data'] }; + + expect(extractRequestData(mockReq, optionsWithData)).toEqual({ + data: 'foo=bar', + }); + }); + + it('encodes JSON body contents back to a string', () => { + const mockReq = { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: { foo: 'bar' }, + }; + const optionsWithData = { include: ['data'] }; + + expect(extractRequestData(mockReq, optionsWithData)).toEqual({ + data: '{"foo":"bar"}', + }); + }); + }); + + describe('query_string', () => { + it('parses the query parms from the url', () => { + const mockReq = { + headers: { host: 'example.com' }, + secure: true, + originalUrl: '/?foo=bar', + }; + const optionsWithQueryString = { include: ['query_string'] }; + + expect(extractRequestData(mockReq, optionsWithQueryString)).toEqual({ + query_string: 'foo=bar', + }); + }); + + it('gracefully degrades if url cannot be determined', () => { + const mockReq = {}; + const optionsWithQueryString = { include: ['query_string'] }; + + expect(extractRequestData(mockReq, optionsWithQueryString)).toEqual({ + query_string: undefined, + }); + }); + }); + + describe('url', () => { + test('express/koa', () => { + const mockReq = { + host: 'example.com', + protocol: 'https', + url: '/', + }; + const optionsWithURL = { include: ['url'] }; + + expect(extractRequestData(mockReq, optionsWithURL)).toEqual({ + url: 'https://example.com/', + }); + }); + + test('node', () => { + const mockReq = { + headers: { host: 'example.com' }, + socket: { encrypted: true }, + originalUrl: '/', + }; + const optionsWithURL = { include: ['url'] }; + + expect(extractRequestData(mockReq, optionsWithURL)).toEqual({ + url: 'https://example.com/', + }); + }); + }); + + describe('custom key', () => { + it('includes the custom key if present', () => { + const mockReq = { + httpVersion: '1.1', + } as any; + const optionsWithCustomKey = { include: ['httpVersion'] }; + + expect(extractRequestData(mockReq, optionsWithCustomKey)).toEqual({ + httpVersion: '1.1', + }); + }); + + it('gracefully degrades if the custom key is missing', () => { + const mockReq = {} as any; + const optionsWithCustomKey = { include: ['httpVersion'] }; + + expect(extractRequestData(mockReq, optionsWithCustomKey)).toEqual({}); + }); + }); +}); + +describe('extractPathForTransaction', () => { + it.each([ + [ + 'extracts a parameterized route and method if available', + { + method: 'get', + baseUrl: '/api/users', + route: { path: '/:id/details' }, + originalUrl: '/api/users/123/details', + } as PolymorphicRequest, + { path: true, method: true }, + 'GET /api/users/:id/details', + 'route' as TransactionSource, + ], + [ + 'ignores the method if specified', + { + method: 'get', + baseUrl: '/api/users', + route: { path: '/:id/details' }, + originalUrl: '/api/users/123/details', + } as PolymorphicRequest, + { path: true, method: false }, + '/api/users/:id/details', + 'route' as TransactionSource, + ], + [ + 'ignores the path if specified', + { + method: 'get', + baseUrl: '/api/users', + route: { path: '/:id/details' }, + originalUrl: '/api/users/123/details', + } as PolymorphicRequest, + { path: false, method: true }, + 'GET', + 'route' as TransactionSource, + ], + [ + 'returns an empty string if everything should be ignored', + { + method: 'get', + baseUrl: '/api/users', + route: { path: '/:id/details' }, + originalUrl: '/api/users/123/details', + } as PolymorphicRequest, + { path: false, method: false }, + '', + 'route' as TransactionSource, + ], + [ + 'falls back to the raw URL if no parameterized route is available', + { + method: 'get', + baseUrl: '/api/users', + originalUrl: '/api/users/123/details', + } as PolymorphicRequest, + { path: true, method: true }, + 'GET /api/users/123/details', + 'url' as TransactionSource, + ], + ])( + '%s', + ( + _: string, + req: PolymorphicRequest, + options: { path?: boolean; method?: boolean }, + expectedRoute: string, + expectedSource: TransactionSource, + ) => { + const [route, source] = extractPathForTransaction(req, options); + + expect(route).toEqual(expectedRoute); + expect(source).toEqual(expectedSource); + }, + ); + + it('overrides the requests information with a custom route if specified', () => { + const req = { + method: 'get', + baseUrl: '/api/users', + route: { path: '/:id/details' }, + originalUrl: '/api/users/123/details', + } as PolymorphicRequest; + + const [route, source] = extractPathForTransaction(req, { + path: true, + method: true, + customRoute: '/other/path/:id/details', + }); + + expect(route).toEqual('GET /other/path/:id/details'); + expect(source).toEqual('route'); + }); +}); From dccbb3e4346fbd80ad1dd74db501b6338e75e1b1 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Mon, 12 Feb 2024 16:07:57 -0500 Subject: [PATCH 2/6] fix bad test --- packages/core/src/integrations/requestdata.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts index e1db10390434..3c13a4ad3f5a 100644 --- a/packages/core/src/integrations/requestdata.ts +++ b/packages/core/src/integrations/requestdata.ts @@ -50,7 +50,7 @@ const DEFAULT_OPTIONS = { email: true, }, }, - transactionNamingScheme: 'methodPath', + transactionNamingScheme: 'methodPath' as const, }; const INTEGRATION_NAME = 'RequestData'; @@ -61,9 +61,6 @@ const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) = ...DEFAULT_OPTIONS, ...options, include: { - // @ts-expect-error It's mad because `method` isn't a known `include` key. (It's only here and not set by default in - // `addRequestDataToEvent` for legacy reasons. TODO (v8): Change that.) - method: true, ...DEFAULT_OPTIONS.include, ...options.include, user: From 38c8d07d3002b67697eb980c5cfe19a11307186b Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 15 Feb 2024 11:51:33 -0500 Subject: [PATCH 3/6] use method --- packages/core/src/integrations/requestdata.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/core/src/integrations/requestdata.ts b/packages/core/src/integrations/requestdata.ts index 3c13a4ad3f5a..1835fda4ec4a 100644 --- a/packages/core/src/integrations/requestdata.ts +++ b/packages/core/src/integrations/requestdata.ts @@ -56,7 +56,6 @@ const DEFAULT_OPTIONS = { const INTEGRATION_NAME = 'RequestData'; const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) => { - const _addRequestData = addRequestDataToEvent; const _options: Required = { ...DEFAULT_OPTIONS, ...options, @@ -94,7 +93,7 @@ const _requestDataIntegration = ((options: RequestDataIntegrationOptions = {}) = const addRequestDataOptions = convertReqDataIntegrationOptsToAddReqDataOpts(_options); - const processedEvent = _addRequestData(event, req, addRequestDataOptions); + const processedEvent = addRequestDataToEvent(event, req, addRequestDataOptions); // Transaction events already have the right `transaction` value if (event.type === 'transaction' || transactionNamingScheme === 'handler') { @@ -176,7 +175,7 @@ function convertReqDataIntegrationOptsToAddReqDataOpts( include: { ip, user, ...requestOptions }, } = integrationOptions; - const requestIncludeKeys: string[] = []; + const requestIncludeKeys: string[] = ['method']; for (const [key, value] of Object.entries(requestOptions)) { if (value) { requestIncludeKeys.push(key); From 9a188a9d6a9cd9b6c80d3ac0ada41c4ece34aa3c Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 15 Feb 2024 12:16:52 -0500 Subject: [PATCH 4/6] delete test --- .../test/integrations/requestdata.test.ts | 111 ------------------ 1 file changed, 111 deletions(-) delete mode 100644 packages/node/test/integrations/requestdata.test.ts diff --git a/packages/node/test/integrations/requestdata.test.ts b/packages/node/test/integrations/requestdata.test.ts deleted file mode 100644 index 3f58f8779de6..000000000000 --- a/packages/node/test/integrations/requestdata.test.ts +++ /dev/null @@ -1,111 +0,0 @@ -import * as http from 'http'; -import type { RequestDataIntegrationOptions } from '@sentry/core'; -import { setCurrentClient } from '@sentry/core'; -import { applyScopeDataToEvent } from '@sentry/core'; -import { getCurrentScope } from '@sentry/core'; -import { RequestData } from '@sentry/core'; -import type { Event, EventProcessor, PolymorphicRequest } from '@sentry/types'; -import * as sentryUtils from '@sentry/utils'; - -import { NodeClient } from '../../src/client'; -import { requestHandler } from '../../src/handlers'; -import { getDefaultNodeClientOptions } from '../helper/node-client-options'; - -const addRequestDataToEventSpy = jest.spyOn(sentryUtils, 'addRequestDataToEvent'); - -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): EventProcessor { - // eslint-disable-next-line deprecation/deprecation - const requestDataIntegration = new RequestData({ - ...integrationOptions, - }); - - const client = new NodeClient( - getDefaultNodeClientOptions({ - dsn: 'https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012', - integrations: [requestDataIntegration], - }), - ); - setCurrentClient(client); - client.init(); - - const eventProcessors = client['_eventProcessors'] as EventProcessor[]; - const eventProcessor = eventProcessors.find(processor => processor.id === 'RequestData'); - - expect(eventProcessor).toBeDefined(); - - return eventProcessor!; -} - -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('usage with express request handler and GCP wrapper', () => { - it('uses options from Express request handler', async () => { - const sentryRequestMiddleware = requestHandler({ include: { transaction: 'methodPath' } }); - const res = new http.ServerResponse(req); - const next = jest.fn(); - - const requestDataEventProcessor = initWithRequestDataIntegrationOptions({ transactionNamingScheme: 'path' }); - - sentryRequestMiddleware(req, res, next); - - applyScopeDataToEvent(event, getCurrentScope().getScopeData()); - void requestDataEventProcessor(event, {}); - - const passedOptions = addRequestDataToEventSpy.mock.calls[0][2]; - - // `transaction` matches the request middleware's option, not the integration's option - expect(passedOptions?.include).toEqual(expect.objectContaining({ transaction: 'methodPath' })); - }); - - it('uses options from GCP wrapper', async () => { - type GCPHandler = (req: PolymorphicRequest, res: http.ServerResponse) => void; - const mockGCPWrapper = (origHandler: GCPHandler, _options: Record): GCPHandler => { - const wrappedHandler: GCPHandler = (req, res) => { - getCurrentScope().setSDKProcessingMetadata({ - request: req, - }); - origHandler(req, res); - }; - return wrappedHandler; - }; - - const wrappedGCPFunction = mockGCPWrapper(jest.fn(), { include: { transaction: 'methodPath' } }); - const res = new http.ServerResponse(req); - - const requestDataEventProcessor = initWithRequestDataIntegrationOptions({ transactionNamingScheme: 'path' }); - - wrappedGCPFunction(req, res); - - applyScopeDataToEvent(event, getCurrentScope().getScopeData()); - void requestDataEventProcessor(event, {}); - - const passedOptions = addRequestDataToEventSpy.mock.calls[0][2]; - - // `transaction` matches the GCP wrapper's option, not the integration's option - expect(passedOptions?.include).toEqual(expect.objectContaining({ transaction: 'methodPath' })); - }); - }); -}); From f1b8cc80fa12c805a6c39ae2e7cfece9c8045644 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 15 Feb 2024 13:08:00 -0500 Subject: [PATCH 5/6] fix wrap function --- packages/serverless/test/gcpfunction.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/serverless/test/gcpfunction.test.ts b/packages/serverless/test/gcpfunction.test.ts index ab9781bdc6f1..798284a6d334 100644 --- a/packages/serverless/test/gcpfunction.test.ts +++ b/packages/serverless/test/gcpfunction.test.ts @@ -231,7 +231,7 @@ describe('GCPFunction', () => { const handler: HttpFunction = (_req, res) => { res.end(); }; - const wrappedHandler = wrapHttpFunction(handler, { addRequestDataToEventOptions: { include: { ip: true } } }); + const wrappedHandler = wrapHttpFunction(handler); await handleHttp(wrappedHandler); From 2837da192c365a5ac9c363cb745337bd6ca7184e Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 15 Feb 2024 16:03:09 -0500 Subject: [PATCH 6/6] use timeout --- packages/serverless/src/gcpfunction/http.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/serverless/src/gcpfunction/http.ts b/packages/serverless/src/gcpfunction/http.ts index d04c00a62521..fe5279ddc6cf 100644 --- a/packages/serverless/src/gcpfunction/http.ts +++ b/packages/serverless/src/gcpfunction/http.ts @@ -37,7 +37,8 @@ export function wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial = { flushTimeout: 2000 }): HttpFunction { +function _wrapHttpFunction(fn: HttpFunction, options: Partial): HttpFunction { + const flushTimeout = options.flushTimeout || 2000; return (req, res) => { const reqMethod = (req.method || '').toUpperCase(); const reqUrl = stripUrlQueryAndFragment(req.originalUrl || req.url || ''); @@ -78,7 +79,7 @@ function _wrapHttpFunction(fn: HttpFunction, options: Partial = } // eslint-disable-next-line @typescript-eslint/no-floating-promises - flush(options.flushTimeout) + flush(flushTimeout) .then(null, e => { DEBUG_BUILD && logger.error(e); })