Skip to content

Commit 322028f

Browse files
lobsterkatielobsterkatie
authored
fix tests, make TS happy (#3103)
1 parent b90dfd6 commit 322028f

File tree

2 files changed

+65
-50
lines changed

2 files changed

+65
-50
lines changed

‎packages/node/src/handlers.ts

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import { flush } from './sdk';
2020

2121
const DEFAULT_SHUTDOWN_TIMEOUT = 2000;
2222

23-
interface ExpressRequest extends http.IncomingMessage {
23+
export interface ExpressRequest extends http.IncomingMessage {
2424
[key: string]: any;
2525
baseUrl?: string;
2626
ip?: string;
@@ -59,7 +59,7 @@ export function tracingHandler(): (
5959
}
6060

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

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

102103
/**
103-
* Extracts complete generalized path from the request object.
104-
* eg. /mountpoint/user/:id
104+
* Extracts complete generalized path from the request object and uses it to construct transaction name.
105+
*
106+
* eg. GET /mountpoint/user/:id
107+
*
108+
* @param req The ExpressRequest object
109+
* @param options What to include in the transaction name (method, path, or both)
110+
*
111+
* @returns The fully constructed transaction name
105112
*/
106-
function extractRouteInfo(req: ExpressRequest, options: { path?: boolean; method?: boolean } = {}): string {
113+
function extractExpressTransactionName(
114+
req: ExpressRequest,
115+
options: { path?: boolean; method?: boolean } = {},
116+
): string {
107117
const method = req.method?.toUpperCase();
108-
let path;
109-
if (req.baseUrl && req.route) {
118+
119+
let path = '';
120+
if (req.route) {
121+
// if the mountpoint is `/`, req.baseUrl is '' (not undefined), so it's safe to include it here
122+
// see https://github.com/expressjs/express/blob/508936853a6e311099c9985d4c11a4b1b8f6af07/test/req.baseUrl.js#L7
110123
path = `${req.baseUrl}${req.route.path}`;
111-
} else if (req.route) {
112-
path = `${req.route.path}`;
113124
} else if (req.originalUrl || req.url) {
114125
path = stripUrlQueryAndFragment(req.originalUrl || req.url || '');
115-
} else {
116-
path = '';
117126
}
118127

119128
let info = '';
@@ -136,14 +145,14 @@ type TransactionTypes = 'path' | 'methodPath' | 'handler';
136145
function extractTransaction(req: ExpressRequest, type: boolean | TransactionTypes): string {
137146
switch (type) {
138147
case 'path': {
139-
return extractRouteInfo(req, { path: true });
148+
return extractExpressTransactionName(req, { path: true });
140149
}
141150
case 'handler': {
142151
return req.route?.stack[0].name || '<anonymous>';
143152
}
144153
case 'methodPath':
145154
default: {
146-
return extractRouteInfo(req, { path: true, method: true });
155+
return extractExpressTransactionName(req, { path: true, method: true });
147156
}
148157
}
149158
}

‎packages/node/test/handlers.test.ts

Lines changed: 41 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -8,29 +8,34 @@ import * as net from 'net';
88

99
import { Event, Request, User } from '../src';
1010
import { NodeClient } from '../src/client';
11-
import { parseRequest, tracingHandler } from '../src/handlers';
11+
import { ExpressRequest, parseRequest, tracingHandler } from '../src/handlers';
1212

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

1616
beforeEach(() => {
1717
mockReq = {
18+
baseUrl: '/routerMountPath',
1819
body: 'foo',
1920
cookies: { test: 'test' },
2021
headers: {
2122
host: 'mattrobenolt.com',
2223
},
2324
method: 'POST',
24-
originalUrl: '/some/originalUrl?key=value',
25+
originalUrl: '/routerMountPath/subpath/specificValue?querystringKey=querystringValue',
26+
path: '/subpath/specificValue',
27+
query: {
28+
querystringKey: 'querystringValue',
29+
},
2530
route: {
26-
path: '/path',
31+
path: '/subpath/:parameterName',
2732
stack: [
2833
{
29-
name: 'routeHandler',
34+
name: 'parameterNameRouteHandler',
3035
},
3136
],
3237
},
33-
url: '/some/url?key=value',
38+
url: '/subpath/specificValue?querystringKey=querystringValue',
3439
user: {
3540
custom_property: 'foo',
3641
email: 'tobias@mail.com',
@@ -42,17 +47,17 @@ describe('parseRequest', () => {
4247

4348
describe('parseRequest.contexts runtime', () => {
4449
test('runtime name must contain node', () => {
45-
const parsedRequest: Event = parseRequest({}, mockReq);
50+
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest);
4651
expect((parsedRequest.contexts!.runtime as Runtime).name).toEqual('node');
4752
});
4853

4954
test('runtime version must contain current node version', () => {
50-
const parsedRequest: Event = parseRequest({}, mockReq);
55+
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest);
5156
expect((parsedRequest.contexts!.runtime as Runtime).version).toEqual(process.version);
5257
});
5358

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

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

7176
test('parseRequest.user only contains the custom properties specified in the options.user array', () => {
72-
const parsedRequest: Event = parseRequest({}, mockReq, {
77+
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, {
7378
user: CUSTOM_USER_KEYS,
7479
});
7580
expect(Object.keys(parsedRequest.user as User)).toEqual(CUSTOM_USER_KEYS);
@@ -95,7 +100,7 @@ describe('parseRequest', () => {
95100
{
96101
...mockReq,
97102
ip: '123',
98-
},
103+
} as ExpressRequest,
99104
{
100105
ip: true,
101106
},
@@ -110,8 +115,8 @@ describe('parseRequest', () => {
110115
...mockReq,
111116
connection: {
112117
remoteAddress: '321',
113-
},
114-
},
118+
} as net.Socket,
119+
} as ExpressRequest,
115120
{
116121
ip: true,
117122
},
@@ -123,55 +128,56 @@ describe('parseRequest', () => {
123128
describe('parseRequest.request properties', () => {
124129
test('parseRequest.request only contains the default set of properties from the request', () => {
125130
const DEFAULT_REQUEST_PROPERTIES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url'];
126-
const parsedRequest: Event = parseRequest({}, mockReq);
131+
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest);
127132
expect(Object.keys(parsedRequest.request as Request)).toEqual(DEFAULT_REQUEST_PROPERTIES);
128133
});
129134

130135
test('parseRequest.request only contains the specified properties in the options.request array', () => {
131136
const INCLUDED_PROPERTIES = ['data', 'headers', 'query_string', 'url'];
132-
const parsedRequest: Event = parseRequest({}, mockReq, {
137+
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, {
133138
request: INCLUDED_PROPERTIES,
134139
});
135140
expect(Object.keys(parsedRequest.request as Request)).toEqual(INCLUDED_PROPERTIES);
136141
});
137142

138143
test('parseRequest.request skips `body` property for GET and HEAD requests', () => {
139-
expect(parseRequest({}, mockReq, {}).request).toHaveProperty('data');
140-
expect(parseRequest({}, { ...mockReq, method: 'GET' }, {}).request).not.toHaveProperty('data');
141-
expect(parseRequest({}, { ...mockReq, method: 'HEAD' }, {}).request).not.toHaveProperty('data');
144+
expect(parseRequest({}, mockReq as ExpressRequest, {}).request).toHaveProperty('data');
145+
expect(parseRequest({}, { ...mockReq, method: 'GET' } as ExpressRequest, {}).request).not.toHaveProperty('data');
146+
expect(parseRequest({}, { ...mockReq, method: 'HEAD' } as ExpressRequest, {}).request).not.toHaveProperty('data');
142147
});
143148
});
144149

145150
describe('parseRequest.transaction property', () => {
146-
test('extracts method and full route path by default from `originalUrl`', () => {
147-
const parsedRequest: Event = parseRequest({}, mockReq);
148-
expect(parsedRequest.transaction).toEqual('POST /some/originalUrl');
151+
test('extracts method and full route path by default`', () => {
152+
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest);
153+
expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/:parameterName');
149154
});
150155

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

157-
test('fallback to method and `route.path` if previous attempts failed', () => {
158-
delete mockReq.originalUrl;
159-
delete mockReq.url;
160-
const parsedRequest: Event = parseRequest({}, mockReq);
161-
expect(parsedRequest.transaction).toEqual('POST /path');
164+
test('fallback to method and `originalUrl` if route is missing', () => {
165+
delete mockReq.route;
166+
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest);
167+
expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/specificValue');
162168
});
163169

164170
test('can extract path only instead if configured', () => {
165-
const parsedRequest: Event = parseRequest({}, mockReq, { transaction: 'path' });
166-
expect(parsedRequest.transaction).toEqual('/some/originalUrl');
171+
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, { transaction: 'path' });
172+
expect(parsedRequest.transaction).toEqual('/routerMountPath/subpath/:parameterName');
167173
});
168174

169175
test('can extract handler name instead if configured', () => {
170-
const parsedRequest: Event = parseRequest({}, mockReq, { transaction: 'handler' });
171-
expect(parsedRequest.transaction).toEqual('routeHandler');
176+
const parsedRequest: Event = parseRequest({}, mockReq as ExpressRequest, { transaction: 'handler' });
177+
expect(parsedRequest.transaction).toEqual('parameterNameRouteHandler');
172178
});
173179
});
174-
}); // end describe('parseRequest()')
180+
});
175181

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

0 commit comments

Comments
 (0)