Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 15 additions & 4 deletions packages/node/src/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ function extractTransaction(req: { [key: string]: any }, type: boolean | Transac
stack: [
{
name: string;
}
},
];
};
};
Expand All @@ -51,7 +51,7 @@ function extractTransaction(req: { [key: string]: any }, type: boolean | Transac
}

/** JSDoc */
function extractRequestData(req: { [key: string]: any }): { [key: string]: string } {
function extractRequestData(req: { [key: string]: any }, keys: boolean | string[]): { [key: string]: string } {
// headers:
// node, express: req.headers
// koa: req.header
Expand Down Expand Up @@ -112,6 +112,17 @@ function extractRequestData(req: { [key: string]: any }): { [key: string]: strin
url: absoluteUrl,
};

const attributes = Array.isArray(keys) ? keys : [];

if (attributes.length) {
Copy link
Contributor Author

@ThatTobMate ThatTobMate Jul 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using slightly different logic here than the extractUserData function, as you are already running some custom logic to set/manipulate keys on the request interface.
In this PR we build the request interface as before then remove any of the properties not specified in the optional array (if it exists).

This behaviour is slightly different to the extractUserData fn as it allows users to request any of the properties from the request interface rather than any properties of the req argument itself.

Copy link
Contributor Author

@ThatTobMate ThatTobMate Jul 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's a bit too ambiguous we could do something like below which would allow users to extract any property on the req object:

  1. Set the requested properties
  2. Run the rest of the current logic to overwrite the values
  3. Merge the requested properties with the default values
  4. Delete any keys that don't match the requested properties from the array.
function extractRequestData(...) {
  // 1.
  const request: { [key: string]: string } = {};
  const attributes = Array.isArray(keys) ? keys : [];

  attributes.forEach(key => {
    if ({}.hasOwnProperty.call(req, key)) {
      request[key] = (req as { [key: string]: string })[key];
    }
  });

  ----
  // 2.
  // set values
  const headers = ...
  const method = ...
  ----

  // 3.
  // request interface
  const requestInterface: {
    [key: string]: any;
  } = {
    ...request
    cookies,
    data,
    headers,
    method,
    query_string: query,
    url: absoluteUrl,
  };

 ----
 // 4.
 // Delete any of the default keys not specified in the array
  if (attributes.length) {
    Object.keys(requestInterface).forEach(key => {
      /** Remove any of the unspecified keys in the options from the request interface */
      if (!attributes.includes(key)) {
        delete requestInterface[key];
      }
    });
  }

 return requestInterface
}

Object.keys(request).forEach(key => {
/** Remove any of the unspecified keys in the options from the request interface */
if (!attributes.includes(key)) {
delete request[key];
}
});
}

return request;
}

Expand Down Expand Up @@ -161,7 +172,7 @@ export function parseRequest(
[key: string]: any;
},
options?: {
request?: boolean;
request?: boolean | string[];
serverName?: boolean;
transaction?: boolean | TransactionTypes;
user?: boolean | string[];
Expand All @@ -188,7 +199,7 @@ export function parseRequest(
if (options.request) {
event.request = {
...event.request,
...extractRequestData(req),
...extractRequestData(req, options.request),
};
}

Expand Down
73 changes: 73 additions & 0 deletions packages/node/test/handlers.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { parseRequest } from '../src/handlers';
import { Event } from '../src';

describe('parseRequest', () => {
const mockReq = {
method: 'GET',
url: '/some/path?key=value',
headers: {
host: 'mattrobenolt.com',
},
cookies: { test: 'test' },
body: '',
user: {
id: 123,
username: 'tobias',
email: '[email protected]',
custom_property: 'foo',
},
};

describe('parseRequest.user properties', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought i'd add a bit of test coverage for some of this logic for both the user and request properties

const DEFAULT_USER_KEYS = ['id', 'username', 'email'];
const CUSTOM_USER_KEYS = ['custom_property'];

test('parseRequest.user only contains the default properties from the user', done => {
const fakeEvent: Event = {};
const parsedRequest: Event = parseRequest(fakeEvent, mockReq);
const userKeys = Object.keys(parsedRequest.user);

expect(userKeys).toEqual(DEFAULT_USER_KEYS);
expect(userKeys).not.toEqual(expect.arrayContaining(CUSTOM_USER_KEYS));
done();
});

test('parseRequest.user only contains the custom properties specified in the options.user array', done => {
const options = {
user: CUSTOM_USER_KEYS,
};
const fakeEvent: Event = {};
const parsedRequest: Event = parseRequest(fakeEvent, mockReq, options);
const userKeys = Object.keys(parsedRequest.user);

expect(userKeys).toEqual(CUSTOM_USER_KEYS);
expect(userKeys).not.toEqual(expect.arrayContaining(DEFAULT_USER_KEYS));
done();
});
});

describe('parseRequest.request properties', () => {
test('parseRequest.request only contains the default set of properties from the request', done => {
const DEFAULT_REQUEST_PROPERTIES = ['cookies', 'data', 'headers', 'method', 'query_string', 'url'];
const fakeEvent: Event = {};
const parsedRequest: Event = parseRequest(fakeEvent, mockReq, {});
expect(Object.keys(parsedRequest.request)).toEqual(DEFAULT_REQUEST_PROPERTIES);
done();
});

test('parseRequest.request only contains the specified properties in the options.request array', done => {
const EXCLUDED_PROPERTIES = ['cookies', 'method'];
const INCLUDED_PROPERTIES = ['data', 'headers', 'query_string', 'url'];
const options = {
request: INCLUDED_PROPERTIES,
};
const fakeEvent: Event = {};
const parsedRequest: Event = parseRequest(fakeEvent, mockReq, options);
const requestKeys = Object.keys(parsedRequest.request);

expect(requestKeys).toEqual(INCLUDED_PROPERTIES);
expect(requestKeys).not.toEqual(expect.arrayContaining(EXCLUDED_PROPERTIES));
done();
});
});
});