Skip to content

fix(express): Make tests match new route name extraction behavior #3103

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 7, 2020
Merged

fix(express): Make tests match new route name extraction behavior #3103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 24 additions & 15 deletions packages/node/src/handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { flush } from './sdk';

const DEFAULT_SHUTDOWN_TIMEOUT = 2000;

interface ExpressRequest extends http.IncomingMessage {
export interface ExpressRequest extends http.IncomingMessage {
[key: string]: any;
baseUrl?: string;
ip?: string;
Expand Down Expand Up @@ -59,7 +59,7 @@ export function tracingHandler(): (
}

const transaction = startTransaction({
name: extractRouteInfo(req, { path: true, method: true }),
name: extractExpressTransactionName(req, { path: true, method: true }),
op: 'http.server',
...traceparentData,
});
Expand All @@ -75,7 +75,8 @@ export function tracingHandler(): (
(res as any).__sentry_transaction = transaction;

res.once('finish', () => {
// We schedule the immediate execution of the `finish` to let all the spans being closed first.
// Push `transaction.finish` to the next event loop so open spans have a chance to finish before the transaction
// closes
setImmediate(() => {
addExpressReqToTransaction(transaction, req);
transaction.setHttpStatus(res.statusCode);
Expand All @@ -93,27 +94,35 @@ export function tracingHandler(): (
*/
function addExpressReqToTransaction(transaction: Transaction | undefined, req: ExpressRequest): void {
if (!transaction) return;
transaction.name = extractRouteInfo(req, { path: true, method: true });
transaction.name = extractExpressTransactionName(req, { path: true, method: true });
transaction.setData('url', req.originalUrl);
transaction.setData('baseUrl', req.baseUrl);
transaction.setData('query', req.query);
}

/**
* Extracts complete generalized path from the request object.
* eg. /mountpoint/user/:id
* Extracts complete generalized path from the request object and uses it to construct transaction name.
*
* eg. GET /mountpoint/user/:id
*
* @param req The ExpressRequest object
* @param options What to include in the transaction name (method, path, or both)
*
* @returns The fully constructed transaction name
*/
function extractRouteInfo(req: ExpressRequest, options: { path?: boolean; method?: boolean } = {}): string {
function extractExpressTransactionName(
req: ExpressRequest,
options: { path?: boolean; method?: boolean } = {},
): string {
const method = req.method?.toUpperCase();
let path;
if (req.baseUrl && req.route) {

let path = '';
if (req.route) {
// if the mountpoint is `/`, req.baseUrl is '' (not undefined), so it's safe to include it here
// see https://github.com/expressjs/express/blob/508936853a6e311099c9985d4c11a4b1b8f6af07/test/req.baseUrl.js#L7
path = `${req.baseUrl}${req.route.path}`;
} else if (req.route) {
path = `${req.route.path}`;
} else if (req.originalUrl || req.url) {
path = stripUrlQueryAndFragment(req.originalUrl || req.url || '');
} else {
path = '';
}

let info = '';
Expand All @@ -136,14 +145,14 @@ type TransactionTypes = 'path' | 'methodPath' | 'handler';
function extractTransaction(req: ExpressRequest, type: boolean | TransactionTypes): string {
switch (type) {
case 'path': {
return extractRouteInfo(req, { path: true });
return extractExpressTransactionName(req, { path: true });
}
case 'handler': {
return req.route?.stack[0].name || '<anonymous>';
}
case 'methodPath':
default: {
return extractRouteInfo(req, { path: true, method: true });
return extractExpressTransactionName(req, { path: true, method: true });
}
}
}
Expand Down
76 changes: 41 additions & 35 deletions packages/node/test/handlers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,29 +8,34 @@ import * as net from 'net';

import { Event, Request, User } from '../src';
import { NodeClient } from '../src/client';
import { parseRequest, tracingHandler } from '../src/handlers';
import { ExpressRequest, parseRequest, tracingHandler } from '../src/handlers';

describe('parseRequest', () => {
let mockReq: { [key: string]: any };

beforeEach(() => {
mockReq = {
baseUrl: '/routerMountPath',
body: 'foo',
cookies: { test: 'test' },
headers: {
host: 'mattrobenolt.com',
},
method: 'POST',
originalUrl: '/some/originalUrl?key=value',
originalUrl: '/routerMountPath/subpath/specificValue?querystringKey=querystringValue',
path: '/subpath/specificValue',
query: {
querystringKey: 'querystringValue',
},
route: {
path: '/path',
path: '/subpath/:parameterName',
stack: [
{
name: 'routeHandler',
name: 'parameterNameRouteHandler',
},
],
},
url: '/some/url?key=value',
url: '/subpath/specificValue?querystringKey=querystringValue',
user: {
custom_property: 'foo',
email: '[email protected]',
Expand All @@ -42,17 +47,17 @@ describe('parseRequest', () => {

describe('parseRequest.contexts runtime', () => {
test('runtime name must contain node', () => {
const parsedRequest: Event = parseRequest({}, mockReq);
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest);
expect((parsedRequest.contexts!.runtime as Runtime).name).toEqual('node');
});

test('runtime version must contain current node version', () => {
const parsedRequest: Event = parseRequest({}, mockReq);
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest);
expect((parsedRequest.contexts!.runtime as Runtime).version).toEqual(process.version);
});

test('runtime disbaled by options', () => {
const parsedRequest: Event = parseRequest({}, mockReq, {
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, {
version: false,
});
expect(parsedRequest).not.toHaveProperty('contexts.runtime');
Expand All @@ -64,12 +69,12 @@ describe('parseRequest', () => {
const CUSTOM_USER_KEYS = ['custom_property'];

test('parseRequest.user only contains the default properties from the user', () => {
const parsedRequest: Event = parseRequest({}, mockReq);
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest);
expect(Object.keys(parsedRequest.user as User)).toEqual(DEFAULT_USER_KEYS);
});

test('parseRequest.user only contains the custom properties specified in the options.user array', () => {
const parsedRequest: Event = parseRequest({}, mockReq, {
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, {
user: CUSTOM_USER_KEYS,
});
expect(Object.keys(parsedRequest.user as User)).toEqual(CUSTOM_USER_KEYS);
Expand All @@ -95,7 +100,7 @@ describe('parseRequest', () => {
{
...mockReq,
ip: '123',
},
} as ExpressRequest,
{
ip: true,
},
Expand All @@ -110,8 +115,8 @@ describe('parseRequest', () => {
...mockReq,
connection: {
remoteAddress: '321',
},
},
} as net.Socket,
} as ExpressRequest,
{
ip: true,
},
Expand All @@ -123,55 +128,56 @@ describe('parseRequest', () => {
describe('parseRequest.request properties', () => {
test('parseRequest.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 = parseRequest({}, mockReq);
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest);
expect(Object.keys(parsedRequest.request as Request)).toEqual(DEFAULT_REQUEST_PROPERTIES);
});

test('parseRequest.request only contains the specified properties in the options.request array', () => {
const INCLUDED_PROPERTIES = ['data', 'headers', 'query_string', 'url'];
const parsedRequest: Event = parseRequest({}, mockReq, {
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, {
request: INCLUDED_PROPERTIES,
});
expect(Object.keys(parsedRequest.request as Request)).toEqual(INCLUDED_PROPERTIES);
});

test('parseRequest.request skips `body` property for GET and HEAD requests', () => {
expect(parseRequest({}, mockReq, {}).request).toHaveProperty('data');
expect(parseRequest({}, { ...mockReq, method: 'GET' }, {}).request).not.toHaveProperty('data');
expect(parseRequest({}, { ...mockReq, method: 'HEAD' }, {}).request).not.toHaveProperty('data');
expect(parseRequest({}, mockReq as ExpressRequest, {}).request).toHaveProperty('data');
expect(parseRequest({}, { ...mockReq, method: 'GET' } as ExpressRequest, {}).request).not.toHaveProperty('data');
expect(parseRequest({}, { ...mockReq, method: 'HEAD' } as ExpressRequest, {}).request).not.toHaveProperty('data');
});
});

describe('parseRequest.transaction property', () => {
test('extracts method and full route path by default from `originalUrl`', () => {
const parsedRequest: Event = parseRequest({}, mockReq);
expect(parsedRequest.transaction).toEqual('POST /some/originalUrl');
test('extracts method and full route path by default`', () => {
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest);
expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/:parameterName');
});

test('extracts method and full route path by default from `url` if `originalUrl` is not present', () => {
delete mockReq.originalUrl;
const parsedRequest: Event = parseRequest({}, mockReq);
expect(parsedRequest.transaction).toEqual('POST /some/url');
test('extracts method and full path by default when mountpoint is `/`', () => {
mockReq.originalUrl = mockReq.originalUrl.replace('/routerMountpath', '');
mockReq.baseUrl = '';
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest);
// "sub"path is the full path here, because there's no router mount path
expect(parsedRequest.transaction).toEqual('POST /subpath/:parameterName');
});

test('fallback to method and `route.path` if previous attempts failed', () => {
delete mockReq.originalUrl;
delete mockReq.url;
const parsedRequest: Event = parseRequest({}, mockReq);
expect(parsedRequest.transaction).toEqual('POST /path');
test('fallback to method and `originalUrl` if route is missing', () => {
delete mockReq.route;
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest);
expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/specificValue');
});

test('can extract path only instead if configured', () => {
const parsedRequest: Event = parseRequest({}, mockReq, { transaction: 'path' });
expect(parsedRequest.transaction).toEqual('/some/originalUrl');
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, { transaction: 'path' });
expect(parsedRequest.transaction).toEqual('/routerMountPath/subpath/:parameterName');
});

test('can extract handler name instead if configured', () => {
const parsedRequest: Event = parseRequest({}, mockReq, { transaction: 'handler' });
expect(parsedRequest.transaction).toEqual('routeHandler');
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, { transaction: 'handler' });
expect(parsedRequest.transaction).toEqual('parameterNameRouteHandler');
});
});
}); // end describe('parseRequest()')
});

describe('tracingHandler', () => {
const urlString = 'http://dogs.are.great:1231/yay/';
Expand Down