Skip to content

Commit

Permalink
fix tests, make TS happy (#3103)
Browse files Browse the repository at this point in the history
  • Loading branch information
@lobsterkatie
lobsterkatie authored Dec 7, 2020
1 parent b90dfd6 commit 322028f
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 50 deletions.
39 changes: 24 additions & 15 deletions packages/node/src/handlers.ts
View file
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
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: 'tobias@mail.com',
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

0 comments on commit 322028f

Please sign in to comment.