fix: remove null postData, add support for nested express routes (#337)

* fix(node): no requestbody removes key, support nested express objects * fix(node): fix some comments Co-authored-by: Jon Ursenbach <[email protected]> * fix(node): fix test name Co-authored-by: Jon Ursenbach <[email protected]> * style: commenting out the default-param-last rule in tests * fix: shallow copying the requst destroys req.headers on node 16 Co-authored-by: Jon Ursenbach <[email protected]> Co-authored-by: Jon Ursenbach <[email protected]>
readmeio · Dec 21, 2021 · 6433718 · 6433718
1 parent d7ea298
commit 6433718
Show file tree

Hide file tree

Showing 5 changed files with 66 additions and 20 deletions.
diff --git a/packages/node/__tests__/.eslintrc b/packages/node/__tests__/.eslintrc
@@ -1,6 +1,7 @@
 {
   "extends": "@readme/eslint-config/testing",
   "rules": {
+    "default-param-last": "off",
     "jest/expect-expect": [
       "error",
       {

diff --git a/packages/node/__tests__/index.test.ts b/packages/node/__tests__/index.test.ts
@@ -108,18 +108,6 @@ describe('#metrics', () => {
     rimraf.sync(cacheDir);
   });
 
-  /* it('should error if missing apiKey', () => {
-    expect(() => {
-      expressMiddleware();
-    }).toThrow('You must provide your ReadMe API key');
-  });
-
-  it('should error if missing grouping function', () => {
-    expect(() => {
-      expressMiddleware('api-key');
-    }).toThrow('You must provide a grouping function');
-  }); */
-
   it('should send a request to the metrics server', () => {
     const apiMock = getReadMeApiMock(1);
     const mock = nock(config.host, {
@@ -150,6 +138,45 @@ describe('#metrics', () => {
       });
   });
 
+  it('express should log the full request url with nested express apps', () => {
+    const apiMock = getReadMeApiMock(1);
+    const mock = nock(config.host, {
+      reqheaders: {
+        'Content-Type': 'application/json',
+        'User-Agent': `${pkg.name}/${pkg.version}`,
+      },
+    })
+      .post('/v1/request', ([body]) => {
+        expect(body.group).toStrictEqual(outgoingGroup);
+        expect(body.request.log.entries[0].request.url).toContain('/test/nested');
+        return true;
+      })
+      .basicAuth({ user: apiKey })
+      .reply(200);
+
+    const app = express();
+    const appNest = express();
+
+    appNest.use(expressMiddleware(apiKey, () => incomingGroup));
+    appNest.get('/nested', (req, res) => {
+      // We're asserting `req.url` to be `/nested` here because the way that Express does contextual route loading
+      // `req.url` won't include the `/test`. The `/test` is only added later internally in Express with `req.originalUrl`.
+      expect(req.url).toBe('/nested');
+      res.sendStatus(200);
+    });
+
+    app.use('/test', appNest);
+
+    return request(app)
+      .get('/test/nested')
+      .expect(200)
+      .expect(res => expect(res).toHaveDocumentationHeader())
+      .then(() => {
+        apiMock.done();
+        mock.done();
+      });
+  });
+
   it('should have access to group(req,res) objects', () => {
     const apiMock = getReadMeApiMock(1);
     const mock = nock(config.host, {
@@ -168,6 +195,7 @@ describe('#metrics', () => {
       .reply(200);
 
     const app = express();
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
     app.use((req: any, res: any, next) => {
       req.a = 'a';
       res.b = 'b';

diff --git a/packages/node/__tests__/lib/process-request.test.ts b/packages/node/__tests__/lib/process-request.test.ts
@@ -4,7 +4,6 @@ import * as http from 'http';
 import processRequest from '../../src/lib/process-request';
 import FormData from 'form-data';
 
-// eslint-disable-next-line default-param-last
 function createApp(reqOptions?: LogOptions, shouldPreParse = false, bodyOverride?) {
   const requestListener = function (req: http.IncomingMessage, res: http.ServerResponse) {
     let body = '';
@@ -470,15 +469,15 @@ describe('#postData', () => {
   });
 });
 
-test('should be an empty object if request is a GET', () =>
+test('should be undefined if request has no postData', () =>
   request(createApp())
     .get('/')
-    .expect(({ body }) => expect(body.postData).toBeNull()));
+    .expect(({ body }) => expect(body.postData).toBeUndefined()));
 
-test('should be null if req.body is empty', () =>
+test('should be missing if req.body is empty', () =>
   request(createApp())
     .post('/')
-    .expect(({ body }) => expect(body.postData).toBeNull()));
+    .expect(({ body }) => expect(body.postData).toBeUndefined()));
 
 test('#text should contain stringified body', () => {
   const body = { a: 1, b: 2 };

diff --git a/packages/node/src/lib/express-middleware.ts b/packages/node/src/lib/express-middleware.ts
@@ -97,7 +97,17 @@ export function expressMiddleware(readmeApiKey: string, group: GroupingFunction,
       const groupData = group(req, res);
 
       const payload = constructPayload(
-        req,
+        {
+          ...req,
+
+          // Shallow copying `req` destroys `req.headers` on Node 16 so we're re-adding it.
+          headers: req.headers,
+
+          // If you're using route nesting with `express.use()` then `req.url` is contextual to that route. So say
+          // you have an `/api` route that loads `/v1/upload`, `req.url` within the `/v1/upload` controller will be
+          // `/v1/upload`. Calling `req.originalUrl` ensures that we also capture the `/api` prefix.
+          url: req.originalUrl,
+        },
         res,
         {
           ...groupData,

diff --git a/packages/node/src/lib/process-request.ts b/packages/node/src/lib/process-request.ts
@@ -37,7 +37,8 @@ export function fixHeader(header: string | number | Array<string>): string | und
  * @returns A redacted string potentially containing the length of the original value, if it was a string
  */
 function redactValue(value: string) {
-  return `[REDACTED${typeof value === 'string' ? ` ${value.length}` : ''}]`;
+  const redactedVal = typeof value === 'string' ? ` ${value.length}` : '';
+  return `[REDACTED${redactedVal}]`;
 }
 
 /**
@@ -179,7 +180,7 @@ export default function processRequest(
   // We only ever use this reqUrl with the fake hostname for the pathname and querystring.
   const reqUrl = new URL(req.url, `${protocol}://readme.io`);
 
-  return {
+  const requestData = {
     method: req.method,
     url: url.format({
       protocol,
@@ -197,4 +198,11 @@ export default function processRequest(
     headersSize: 0,
     bodySize: 0,
   };
+
+  // At the moment the server doesn't accept null for request bodies. We're opening up support soon, but getting this fix out will be faster.
+  if (requestData.postData === null) {
+    delete requestData.postData;
+  }
+
+  return requestData;
 }