Skip to content

Commit

Permalink
fix(node): Allow ParseRequestOptions to be passed to request handler (
Browse files Browse the repository at this point in the history
#5287)

This fixes an issue introduced by #5257, which was designed to be  backwards-compatible, but which missed a key use case: When users pass options to our Express middleware, they might be (and in fact, for now, almost certainly are) passing them in the form of `ParseRequestOptions`, not `AddRequestDataToEventOptions`. This allows for that possibility, as a backwards-compatibility measure until v8. It also adjusts a spot in the serverless SDK where a similar problem can occur.

Fixes #5282.
  • Loading branch information
lobsterkatie authored Jun 22, 2022
1 parent 45818f3 commit 9b02afb
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 6 deletions.
24 changes: 20 additions & 4 deletions packages/node/src/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ import * as domain from 'domain';
import * as http from 'http';

import { NodeClient } from './client';
// TODO (v8 / XXX) Remove these imports
import type { ParseRequestOptions } from './requestDataDeprecated';
import { parseRequest } from './requestDataDeprecated';
import { addRequestDataToEvent, extractRequestData, flush, isAutoSessionTrackingEnabled } from './sdk';

/**
Expand Down Expand Up @@ -72,9 +75,12 @@ export function tracingHandler(): (
};
}

export type RequestHandlerOptions = AddRequestDataToEventOptions & {
flushTimeout?: number;
};
export type RequestHandlerOptions =
// TODO (v8 / XXX) Remove ParseRequestOptions type and eslint override
// eslint-disable-next-line deprecation/deprecation
| (ParseRequestOptions | AddRequestDataToEventOptions) & {
flushTimeout?: number;
};

/**
* Express compatible request handler.
Expand All @@ -96,11 +102,21 @@ export function requestHandler(
scope.setSession();
}
}

return function sentryRequestMiddleware(
req: http.IncomingMessage,
res: http.ServerResponse,
next: (error?: any) => void,
): void {
// TODO (v8 / XXX) Remove this shim and just use `addRequestDataToEvent`
let backwardsCompatibleEventProcessor: (event: Event) => Event;
if (options && 'include' in options) {
backwardsCompatibleEventProcessor = (event: Event) => addRequestDataToEvent(event, req, options);
} else {
// eslint-disable-next-line deprecation/deprecation
backwardsCompatibleEventProcessor = (event: Event) => parseRequest(event, req, options as ParseRequestOptions);
}

if (options && options.flushTimeout && options.flushTimeout > 0) {
// eslint-disable-next-line @typescript-eslint/unbound-method
const _end = res.end;
Expand All @@ -124,7 +140,7 @@ export function requestHandler(
const currentHub = getCurrentHub();

currentHub.configureScope(scope => {
scope.addEventProcessor((event: Event) => addRequestDataToEvent(event, req, options));
scope.addEventProcessor(backwardsCompatibleEventProcessor);
const client = currentHub.getClient<NodeClient>();
if (isAutoSessionTrackingEnabled(client)) {
const scope = currentHub.getScope();
Expand Down
10 changes: 8 additions & 2 deletions packages/serverless/src/gcpfunction/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,16 @@ import { domainify, getActiveDomain, proxyFunction } from './../utils';
import { 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: AddRequestDataToEventOptions;
parseRequestOptions: ParseRequestOptions;
}
interface NewHttpFunctionWrapperOptions extends WrapperOptions {
addRequestDataToEventOptions: AddRequestDataToEventOptions;
Expand Down Expand Up @@ -58,7 +63,8 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial<HttpFunctionWr

const options: HttpFunctionWrapperOptions = {
flushTimeout: 2000,
addRequestDataToEventOptions: parseRequestOptions ? parseRequestOptions : {},
// TODO (v8 / xxx): Remove this line, since `addRequestDataToEventOptions` will be included in the spread of `wrapOptions`
addRequestDataToEventOptions: parseRequestOptions ? { include: parseRequestOptions } : {},
...wrapOptions,
};
return (req, res) => {
Expand Down

0 comments on commit 9b02afb

Please sign in to comment.