Repository: ViacomInc/data-point (js)

Issue:
Add option of using the request-debug library
**Proposed Feature:**

Add the option of using [request-debug](https://www.npmjs.com/package/request-debug) for debugging request entities.

When `locals.requestDebug` is a function, it should be passed to `request-debug` like this:

```js
const requestDebug = _.get(acc, 'locals.requestDebug')
if (typeof requestDebug === 'function') {
  require('request-debug')(rp, requestDebug)
}
```

It could also be passed the current `accumulator` as an extra parameter. 

**EDIT:**

It probably makes more sense to extend the current `inspect` option, instead of adding a whole new function. When `inspect` is a function, it currently receives the `accumulator` as the only parameter; this could be refactored, so that it also receives the output from `request-debug`. The new inspect function would look like this:

```js
const inspect = (acc, type, data, r) => {/*...*/}
```

where `type, data, r` comes from `request-debug`.







Pull request:
feat(data-point): Add more functionality to the inspect function for request entities
<!-- What changes are being made? (What feature/bug is being fixed here?) -->
**What**: fix #324

<!-- Why are these changes necessary? -->
**Why**: This adds more functionality to the `inspect` function for request entities. The function now has two parameters: `accumulator` and `data`, where the data object contains information like the fully-formed URI and the response status code. This replaces #328, which introduced the `request-debug` library as a dependency.

<!-- Have you done all of these things?  -->
**Checklist**:
<!-- add "N/A" to the end of each line that's irrelevant to your changes -->
<!-- to check an item, place an "x" in the box like so: "- [x] Documentation" -->
- [ ] Has Breaking changes
- [x] Documentation
- [x] Tests
- [x] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
- [x] Added username to **all-contributors** list



Patch:
diff --git a/packages/data-point/README.md b/packages/data-point/README.md
index e56161e..55c4534 100644
--- a/packages/data-point/README.md
+++ b/packages/data-point/README.md
@@ -2254,7 +2254,7 @@ Example at: [examples/entity-request-options.js](examples/entity-request-options
 
 For more examples of request entities, see the [Examples](examples), the [Integration Examples](test/definitions/integrations.js), and the unit tests: [Request Definitions](test/definitions/requests.js).
 
-#### Inspecting Request
+#### Inspecting Request Entities
 
 You may inspect a Request entity through the `params.inspect` property.
 
@@ -2272,10 +2272,41 @@ dataPoint.addEntities({
 })
 ```
 
-If `params.inspect` is `true` it will output the entity's information to the console.
+**Boolean**
 
-If `params.inspect` is a `function`, you may execute custom debugging code to be executed before the actual request gets made. The function receives the current accumulator value as its only parameter.
+If `params.inspect` is `true`, it will output the entity's information to the console.
 
+**Function**
+
+If `params.inspect` is a function, it will be called twice: once before the request is made, and once when the request is resolved. It should have the signature ```(accumulator: Object, data: Object)```.
+
+The `inspect` function is first called just before initiating the request. The first argument is the `accumulator`, and the second is a `data` object with these properties:
+
+```js
+{
+  type: 'request',
+  // unique ID that is shared with the 'response' object
+  debugId: Number,
+  // ex: 'GET'
+  method: String,
+  // fully-formed URI
+  uri: String,
+  // the value of request.body (or undefined)
+  [body]: String
+}
+```
+
+It's then called when the request succeeds or fails. The `data` object will have a `type` property of either `'response'` or `'error'`. The `debugId` can be used to match the response with the corresponding request.
+
+```js
+{
+  type: 'response|error',
+  // unique ID that is shared with the 'request' object
+  debugId: Number,
+  // http status code
+  statusCode: Number,
+}
+```
 
 ### Hash
 
diff --git a/packages/data-point/lib/entity-types/entity-request/resolve.js b/packages/data-point/lib/entity-types/entity-request/resolve.js
index cf6fe20..d4815c0 100644
--- a/packages/data-point/lib/entity-types/entity-request/resolve.js
+++ b/packages/data-point/lib/entity-types/entity-request/resolve.js
@@ -5,6 +5,8 @@ const rp = require('request-promise')
 
 const utils = require('../../utils')
 
+let debugIdCounter = 0
+
 /**
  * request's default options
  * @type {Object}
@@ -93,14 +95,8 @@ module.exports.resolveUrl = resolveUrl
 /**
  * @param {Accumulator} acc
  */
-function inspect (acc) {
+function inspect (acc, request) {
   const paramInspect = acc.params && acc.params.inspect
-
-  if (typeof paramInspect === 'function') {
-    paramInspect(acc)
-    return true
-  }
-
   if (paramInspect === true) {
     utils.inspect(acc, {
       options: acc.options,
@@ -109,6 +105,43 @@ function inspect (acc) {
     return true
   }
 
+  if (typeof paramInspect === 'function') {
+    // some of this logic borrows from https://github.com/request/request-debug
+    const debugId = ++debugIdCounter
+    const data = {
+      debugId,
+      type: 'request',
+      uri: request.uri.href,
+      method: request.method,
+      headers: _.cloneDeep(request.headers)
+    }
+    if (request.body) {
+      data.body = request.body.toString('utf8')
+    }
+    paramInspect(acc, data)
+    // This promise chain should not be returned,
+    // because it is only being used to trigger
+    // the paramInspect callback
+    request
+      .then(res => {
+        _.attempt(paramInspect, acc, {
+          debugId,
+          type: 'response',
+          statusCode: res.statusCode,
+          headers: res.headers
+        })
+      })
+      .catch(error => {
+        _.attempt(paramInspect, acc, {
+          debugId,
+          type: 'error',
+          statusCode: error.statusCode,
+          headers: error.headers
+        })
+      })
+    return true
+  }
+
   return false
 }
 
@@ -120,36 +153,39 @@ module.exports.inspect = inspect
  * @return {Promise<Accumulator>}
  */
 function resolveRequest (acc, resolveReducer) {
-  inspect(acc)
-  return rp(acc.options)
-    .then(result => utils.set(acc, 'value', result))
-    .catch(error => {
-      // remove auth objects from acc and error for printing to console
-      const redactedAcc = fp.set('options.auth', '[omitted]', acc)
-      const redactedError = fp.set('options.auth', '[omitted]', error)
-
-      const message = [
-        'Entity info:',
-        '\n  - Id: ',
-        _.get(redactedAcc, 'reducer.spec.id'),
-        '\n',
-        utils.inspectProperties(
-          redactedAcc,
-          ['options', 'params', 'value'],
-          '  '
-        ),
-        '\n  Request:\n',
-        utils.inspectProperties(
-          redactedError,
-          ['error', 'message', 'statusCode', 'options', 'body'],
-          '  '
-        )
-      ].join('')
-
-      // attaching to error so it can be exposed by a handler outside datapoint
-      error.message = `${error.message}\n\n${message}`
-      throw error
-    })
+  const options = Object.assign({}, acc.options, {
+    resolveWithFullResponse: true
+  })
+
+  const request = rp(options)
+  inspect(acc, request)
+  return request.then(res => utils.set(acc, 'value', res.body)).catch(error => {
+    // remove auth objects from acc and error for printing to console
+    const redactedAcc = fp.set('options.auth', '[omitted]', acc)
+    const redactedError = fp.set('options.auth', '[omitted]', error)
+
+    const message = [
+      'Entity info:',
+      '\n  - Id: ',
+      _.get(redactedAcc, 'reducer.spec.id'),
+      '\n',
+      utils.inspectProperties(
+        redactedAcc,
+        ['options', 'params', 'value'],
+        '  '
+      ),
+      '\n  Request:\n',
+      utils.inspectProperties(
+        redactedError,
+        ['error', 'message', 'statusCode', 'options', 'body'],
+        '  '
+      )
+    ].join('')
+
+    // attaching to error so it can be exposed by a handler outside datapoint
+    error.message = `${error.message}\n\n${message}`
+    throw error
+  })
 }
 
 module.exports.resolveRequest = resolveRequest


Test patch:
diff --git a/packages/data-point/lib/entity-types/entity-request/__snapshots__/resolve.test.js.snap b/packages/data-point/lib/entity-types/entity-request/__snapshots__/resolve.test.js.snap
index 6c5dd33..8d6efc3 100644
--- a/packages/data-point/lib/entity-types/entity-request/__snapshots__/resolve.test.js.snap
+++ b/packages/data-point/lib/entity-types/entity-request/__snapshots__/resolve.test.js.snap
@@ -18,10 +18,10 @@ Entity info:
   method: 'GET',
   json: true,
   url: 'http://remote.test/source1',
+  resolveWithFullResponse: true,
   callback: [Function: RP$callback],
   transform: undefined,
   simple: true,
-  resolveWithFullResponse: false,
   transform2xxOnly: false }
 "
 `;
@@ -42,10 +42,10 @@ Entity info:
   - statusCode: 404
   - options: { json: true,
   url: 'http://remote.test/source1',
+  resolveWithFullResponse: true,
   callback: [Function: RP$callback],
   transform: undefined,
   simple: true,
-  resolveWithFullResponse: false,
   transform2xxOnly: false,
   auth: '[omitted]' }
 "
diff --git a/packages/data-point/lib/entity-types/entity-request/resolve.test.js b/packages/data-point/lib/entity-types/entity-request/resolve.test.js
index 22fd8ed..0264f7d 100644
--- a/packages/data-point/lib/entity-types/entity-request/resolve.test.js
+++ b/packages/data-point/lib/entity-types/entity-request/resolve.test.js
@@ -1,8 +1,9 @@
 /* eslint-env jest */
 
 const _ = require('lodash')
+const rp = require('request-promise')
 const nock = require('nock')
-const Resolve = require('./resolve')
+let Resolve = require('./resolve')
 
 const AccumulatorFactory = require('../../accumulator/factory')
 const ReducerFactory = require('../../reducer-types/factory')
@@ -12,7 +13,7 @@ const ResolveEntity = require('../base-entity/resolve')
 const FixtureStore = require('../../../test/utils/fixture-store')
 
 const helpers = require('../../helpers')
-const utils = require('../../utils')
+let utils = require('../../utils')
 
 let dataPoint
 let resolveReducerBound
@@ -55,6 +56,10 @@ beforeEach(() => {
   dataPoint.middleware.clear()
 })
 
+afterEach(() => {
+  nock.cleanAll()
+})
+
 describe('resolveUrlInjections', () => {
   test('url with no reducer', () => {
     const value = {
@@ -311,54 +316,196 @@ describe('resolveRequest', () => {
 })
 
 describe('inspect', () => {
-  let consoleInfo
-  function getAcc () {
-    const acc = {}
-    _.set(acc, 'reducer.spec.id', 'test:test')
-    _.set(acc, 'params.inspect', true)
-    return acc
-  }
-  beforeAll(() => {
-    consoleInfo = console.info
+  let utilsInspectSpy
+  beforeEach(() => {
+    // debugIdCounter is a local variable in
+    // resolve.js, so this resets it to zero
+    jest.resetModules()
+    Resolve = require('./resolve')
+    utils = require('../../utils')
+    utilsInspectSpy = jest.spyOn(utils, 'inspect').mockReturnValue(undefined)
   })
   afterEach(() => {
-    console.info = consoleInfo
+    utilsInspectSpy.mockClear()
   })
-  test('It should not execute custom inspect or utils.inspect', () => {
-    const utilsIinspectSpy = jest.spyOn(utils, 'inspect')
-    const acc = getAcc()
-    acc.params.inspect = undefined
-
-    Resolve.inspect(acc)
-    expect(utilsIinspectSpy).not.toBeCalled()
-
-    utilsIinspectSpy.mockReset()
-    utilsIinspectSpy.mockRestore()
+  afterAll(() => {
+    utilsInspectSpy.mockRestore()
   })
-  test('It should execute custom inspect when provided, not execute utils.inspect', () => {
-    const utilsIinspectSpy = jest.spyOn(utils, 'inspect')
-    const acc = getAcc()
-    acc.params.inspect = jest.fn()
 
-    Resolve.inspect(acc)
-    expect(utilsIinspectSpy).not.toBeCalled()
-    expect(acc.params.inspect).toBeCalledWith(acc)
+  function createAcc ({ inspect }) {
+    return {
+      value: 'boomerang',
+      options: {},
+      params: {
+        inspect
+      },
+      reducer: _.set({}, 'spec.id', 'test:test')
+    }
+  }
+  function createMockRequest (options) {
+    let { statusCode, requestType, rpOptions } = options
+    const nockInstance = nock('http://remote.test')
+    nockInstance[requestType]('/').reply(statusCode, { statusCode })
+    return rp[requestType]({
+      uri: 'http://remote.test',
+      resolveWithFullResponse: true,
+      ...rpOptions
+    })
+  }
 
-    utilsIinspectSpy.mockReset()
-    utilsIinspectSpy.mockRestore()
+  test('It should ignore params.inspect and utils.inspect when params.inspect === undefined', async () => {
+    const acc = createAcc({ inspect: undefined })
+    const request = createMockRequest({ statusCode: 200, requestType: 'get' })
+    await expect(request).resolves.toBeTruthy()
+    Resolve.inspect(acc, request)
+    expect(utilsInspectSpy).not.toBeCalled()
+  })
+  test('It should ignore params.inspect and utils.inspect when params.inspect === false', async () => {
+    const acc = createAcc({ inspect: false })
+    const request = createMockRequest({ statusCode: 200, requestType: 'get' })
+    await expect(request).resolves.toBeTruthy()
+    Resolve.inspect(acc, request)
+    expect(utilsInspectSpy).not.toBeCalled()
+  })
+  test('It should execute utils.inspect when params.inspect === true', async () => {
+    const acc = createAcc({ inspect: true })
+    const request = createMockRequest({ statusCode: 200, requestType: 'get' })
+    Resolve.inspect(acc, request)
+    await expect(request).resolves.toBeTruthy()
+    expect(utilsInspectSpy).toBeCalledWith(
+      acc,
+      expect.objectContaining({
+        options: acc.options,
+        value: acc.value
+      })
+    )
   })
-  test('It should execute utils.inspect when params.inspect === true', () => {
-    const utilsIinspectSpy = jest
-      .spyOn(utils, 'inspect')
-      .mockImplementation(() => true)
-    const acc = getAcc()
-    acc.params.inspect = true
-
-    Resolve.inspect(acc)
-    expect(utilsIinspectSpy).toBeCalled()
-
-    utilsIinspectSpy.mockReset()
-    utilsIinspectSpy.mockRestore()
+  test('It should execute params.inspect when rp.then is called', async () => {
+    const acc = createAcc({ inspect: jest.fn() })
+    const request = createMockRequest({
+      statusCode: 200,
+      requestType: 'get'
+    })
+    Resolve.inspect(acc, request)
+    await expect(request).resolves.toBeTruthy()
+    expect(utilsInspectSpy).not.toBeCalled()
+    expect(acc.params.inspect.mock.calls).toEqual([
+      [
+        acc,
+        expect.objectContaining({
+          debugId: 1,
+          type: 'request',
+          method: 'GET',
+          uri: expect.stringMatching('http://remote.test')
+        })
+      ],
+      [
+        acc,
+        expect.objectContaining({
+          debugId: 1,
+          statusCode: 200,
+          type: 'response'
+        })
+      ]
+    ])
+  })
+  test('It should execute params.inspect when rp.catch is called', async () => {
+    const acc = createAcc({ inspect: jest.fn() })
+    const request = createMockRequest({
+      statusCode: 404,
+      requestType: 'get'
+    })
+    Resolve.inspect(acc, request)
+    await expect(request).rejects.toBeTruthy()
+    expect(utilsInspectSpy).not.toBeCalled()
+    expect(acc.params.inspect.mock.calls).toEqual([
+      [
+        acc,
+        expect.objectContaining({
+          debugId: 1,
+          type: 'request',
+          method: 'GET',
+          uri: expect.stringMatching('http://remote.test')
+        })
+      ],
+      [
+        acc,
+        expect.objectContaining({
+          debugId: 1,
+          statusCode: 404,
+          type: 'error'
+        })
+      ]
+    ])
+  })
+  test('It should pass the body option to params.inspect', async () => {
+    const acc = createAcc({ inspect: jest.fn() })
+    const bodyData = JSON.stringify({ test: true })
+    const request = createMockRequest({
+      statusCode: 200,
+      requestType: 'post',
+      rpOptions: {
+        body: bodyData
+      }
+    })
+    Resolve.inspect(acc, request)
+    await expect(request).resolves.toBeTruthy()
+    const mockArguments = acc.params.inspect.mock.calls.slice(0, 2)
+    expect(utilsInspectSpy).not.toBeCalled()
+    expect(mockArguments).toEqual([
+      [
+        acc,
+        expect.objectContaining({
+          debugId: 1,
+          type: 'request',
+          method: 'POST',
+          uri: expect.stringMatching('http://remote.test'),
+          body: expect.stringMatching(bodyData)
+        })
+      ],
+      [
+        acc,
+        expect.objectContaining({
+          debugId: 1,
+          statusCode: 200,
+          type: 'response'
+        })
+      ]
+    ])
+  })
+  test('It should use incrementing debugId values', async () => {
+    const executeRequest = async (resolveWithSuccess, expectedDebugId) => {
+      const acc = createAcc({ inspect: jest.fn() })
+      const request = createMockRequest({
+        statusCode: resolveWithSuccess ? 200 : 404,
+        requestType: 'get'
+      })
+      Resolve.inspect(acc, request)
+      if (resolveWithSuccess) {
+        await expect(request).resolves.toBeTruthy()
+      } else {
+        await expect(request).rejects.toBeTruthy()
+      }
+      expect(utilsInspectSpy).not.toBeCalled()
+      expect(acc.params.inspect.mock.calls).toEqual([
+        [
+          acc,
+          expect.objectContaining({
+            debugId: expectedDebugId
+          })
+        ],
+        [
+          acc,
+          expect.objectContaining({
+            debugId: expectedDebugId
+          })
+        ]
+      ])
+    }
+    await executeRequest(true, 1)
+    await executeRequest(false, 2)
+    await executeRequest(false, 3)
+    await executeRequest(true, 4)
   })
 })