Repository: ViacomInc/data-point
Pull request #111 (2018-01-04 15:58:39)

feat(data-point/request-entity): improves request error message
<!--
Thanks for your interest in the project. We appreciate bugs filed and PRs submitted!

Please make sure that you are familiar with and follow the Code of Conduct for
this project (found in the CODE_OF_CONDUCT.md file).

Also, please make sure you're familiar with and follow the instructions in the
contributing guidelines (found in the CONTRIBUTING.md file).

Please fill out the information below to expedite the review and (hopefully)
merge of your pull request!
-->

<!-- What changes are being made? (What feature/bug is being fixed here?) -->
**What**: #110 

<!-- Why are these changes necessary? -->
**Why**:  #110

<!-- How were these changes implemented? -->
**How**: logs request critical info and attaches it to error.message

<!-- 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" -->
- [N/A] Documentation
- [X] Tests
- [X] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
- [N/A] Added username to **all-contributors** list

<!-- feel free to add additional comments -->


coveralls: 
[![Coverage Status](https://coveralls.io/builds/14901132/badge)](https://coveralls.io/builds/14901132)

Coverage decreased (-0.06%) to 99.939% when pulling **fe4d271b8ac6aad1004de86734d531277931ffc9 on acatl:request-entity-inspect-error** into **73a2527ab1958fee695642cc0acd625bbc98aa7b on ViacomInc:master**.

acatl: wow taking care of that coverage... 
coveralls: 
[![Coverage Status](https://coveralls.io/builds/14901671/badge)](https://coveralls.io/builds/14901671)

Coverage remained the same at 100.0% when pulling **3aa1276d09c965ab273e2b9b9b08bc6fb0a2fb69 on acatl:request-entity-inspect-error** into **73a2527ab1958fee695642cc0acd625bbc98aa7b on ViacomInc:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14901955/badge)](https://coveralls.io/builds/14901955)

Coverage remained the same at 100.0% when pulling **9d24908c1c1457d7e36d32f28d79d5001796a224 on acatl:request-entity-inspect-error** into **73a2527ab1958fee695642cc0acd625bbc98aa7b on ViacomInc:master**.


Patch:
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 78186c5..0326ec8 100644
--- a/packages/data-point/lib/entity-types/entity-request/resolve.js
+++ b/packages/data-point/lib/entity-types/entity-request/resolve.js
@@ -106,7 +106,31 @@ module.exports.resolveBeforeRequest = resolveBeforeRequest
 
 function resolveRequest (acc, resolveTransform) {
   inspect(acc)
-  return rp(acc.options).then(result => utils.set(acc, 'value', result))
+  return rp(acc.options)
+    .then(result => utils.set(acc, 'value', result))
+    .catch(error => {
+      const message = [
+        'Entity info:',
+        '\n  - Id: ',
+        _.get(acc, 'reducer.spec.id'),
+        '\n',
+        utils.inspectProperties(acc, ['options', 'params', 'value'], '  '),
+        '\n  Request:\n',
+        utils.inspectProperties(
+          error,
+          ['error', 'message', 'statusCode', 'options', 'body'],
+          '  '
+        )
+      ].join('')
+
+      // this is useful in the case the error itself is not logged by the
+      // implementation
+      console.info(error.toString(), message)
+
+      // 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
diff --git a/packages/data-point/lib/utils/index.js b/packages/data-point/lib/utils/index.js
index cc8fe59..155cdbb 100644
--- a/packages/data-point/lib/utils/index.js
+++ b/packages/data-point/lib/utils/index.js
@@ -1,6 +1,7 @@
 'use strict'
 
 const _ = require('lodash')
+const util = require('util')
 
 /**
  * sets key to value of a copy of target. target stays untouched, if key is
@@ -69,6 +70,25 @@ function inspect (acc, data) {
 
 module.exports.inspect = inspect
 
+/**
+ * @param {*} obj - object to inspect
+ * @param {Array} props - properties to inspect
+ * @param {string} indent - indent to be used on each key
+ */
+function inspectProperties (obj, props, indent = '') {
+  return props.reduce((acc, key) => {
+    const val = obj[key]
+    if (typeof val !== 'undefined') {
+      return `${acc}${indent}- ${key}: ${util.inspect(obj[key], {
+        breakLength: 0
+      })}\n`
+    }
+    return acc
+  }, '')
+}
+
+module.exports.inspectProperties = inspectProperties
+
 function isFalsy (val) {
   return val === null || val === false || typeof val === 'undefined'
 }


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
new file mode 100644
index 0000000..8d4575f
--- /dev/null
+++ b/packages/data-point/lib/entity-types/entity-request/__snapshots__/resolve.test.js.snap
@@ -0,0 +1,24 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`resolveRequest log errors when request fails 1`] = `
+"404 - \\"not found\\"
+
+Entity info:
+  - Id: test:test
+  - options: { json: true,
+  url: 'http://remote.test/source1' }
+  - value: 'foo'
+
+  Request:
+  - error: 'not found'
+  - message: '404 - \\"not found\\"'
+  - statusCode: 404
+  - options: { json: true,
+  url: 'http://remote.test/source1',
+  callback: [Function: RP$callback],
+  transform: undefined,
+  simple: true,
+  resolveWithFullResponse: false,
+  transform2xxOnly: false }
+"
+`;
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 1916b7e..ee44c09 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
@@ -175,6 +175,13 @@ describe('getRequestOptions', () => {
 })
 
 describe('resolveRequest', () => {
+  let consoleInfo
+  beforeAll(() => {
+    consoleInfo = console.info
+  })
+  afterEach(() => {
+    console.info = consoleInfo
+  })
   test('resolve reducer locals', () => {
     nock('http://remote.test')
       .get('/source1')
@@ -195,6 +202,28 @@ describe('resolveRequest', () => {
       })
     })
   })
+
+  test('log errors when request fails', () => {
+    nock('http://remote.test')
+      .get('/source1')
+      .reply(404, 'not found')
+
+    const acc = {
+      options: {
+        json: true,
+        url: 'http://remote.test/source1'
+      },
+      value: 'foo'
+    }
+    _.set(acc, 'reducer.spec.id', 'test:test')
+    console.info = jest.fn()
+    return Resolve.resolveRequest(acc)
+      .catch(e => e)
+      .then(result => {
+        expect(console.info).toBeCalled()
+        expect(result.message).toMatchSnapshot()
+      })
+  })
 })
 
 describe('inspect', () => {
diff --git a/packages/data-point/lib/utils/__snapshots__/utils.test.js.snap b/packages/data-point/lib/utils/__snapshots__/utils.test.js.snap
new file mode 100644
index 0000000..3db7626
--- /dev/null
+++ b/packages/data-point/lib/utils/__snapshots__/utils.test.js.snap
@@ -0,0 +1,14 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`inspectProperties It should add indent to keys 1`] = `
+"  - a: 1
+  - b: true
+"
+`;
+
+exports[`inspectProperties It should inspect each property 1`] = `
+"- a: 1
+- b: { a: true,
+  b: false }
+"
+`;
diff --git a/packages/data-point/lib/utils/utils.test.js b/packages/data-point/lib/utils/utils.test.js
index ab558f1..be12f26 100644
--- a/packages/data-point/lib/utils/utils.test.js
+++ b/packages/data-point/lib/utils/utils.test.js
@@ -101,6 +101,26 @@ describe('inspect', () => {
   })
 })
 
+describe('inspectProperties', () => {
+  test('It should inspect each property', () => {
+    const obj = {
+      a: 1,
+      b: { a: true, b: false }
+    }
+    expect(utils.inspectProperties(obj, ['a', 'b', 'c'])).toMatchSnapshot()
+  })
+
+  test('It should add indent to keys', () => {
+    const obj = {
+      a: 1,
+      b: true
+    }
+    expect(
+      utils.inspectProperties(obj, ['a', 'b', 'c'], '  ')
+    ).toMatchSnapshot()
+  })
+})
+
 describe('isEmpty', () => {
   test('It should test for empty', () => {
     expect(utils.isFalsy(null)).toEqual(true)