Repository: ViacomInc/data-point
Pull request #118 (2018-01-04 22:18:28)

Entities skip if empty should error
<!--
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**:  #45

<!-- Why are these changes necessary? -->
**Why**: to remove unexpected behaviours when entites where being resolved if value was not right type

<!-- How were these changes implemented? -->
**How**:s

remove premature check and delegate to entity validation after value is resolved

<!-- 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" -->
- [X] Documentation
- [X] Tests
- [X] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
- [ ] Added username to **all-contributors** list - N/A

<!-- feel free to add additional comments -->


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

Coverage remained the same at 100.0% when pulling **f3afb1576f5f9574f0c9459967a5933d7463504d on acatl:entities-skip-if-empty-should-error** into **ff00ba87f4b5d43ed84b69df3bc5b3fb3b99f4f0 on ViacomInc:master**.

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

Coverage remained the same at 100.0% when pulling **f3afb1576f5f9574f0c9459967a5933d7463504d on acatl:entities-skip-if-empty-should-error** into **ff00ba87f4b5d43ed84b69df3bc5b3fb3b99f4f0 on ViacomInc:master**.

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

Coverage remained the same at 100.0% when pulling **14567403ae57c9891e89510c0745c3a8446a9984 on acatl:entities-skip-if-empty-should-error** into **ff00ba87f4b5d43ed84b69df3bc5b3fb3b99f4f0 on ViacomInc:master**.


Patch:
diff --git a/packages/data-point/README.md b/packages/data-point/README.md
index c92aa7c..c070455 100644
--- a/packages/data-point/README.md
+++ b/packages/data-point/README.md
@@ -1663,7 +1663,7 @@ If `params.inspect` is a `function`, you may execute custom debugging code to be
 
 A Hash entity transforms a _Hash_ like data structure. It enables you to manipulate the keys within a Hash. 
 
-To prevent unexpected results, **Hash** can only process **Plain Objects**, which are objects created by the Object constructor. 
+To prevent unexpected results, **Hash** can only process **Plain Objects**, which are objects created by the Object constructor. If [Hash.value](#hash-value) does not resolve to a Plain Object it will **throw** an error. 
 
 Hash entities expose a set of reducers: [mapKeys](#hash-mapKeys), [omitKeys](#hash-omitKeys), [pickKeys](#hash-pickKeys), [addKeys](#hash-addKeys), [addValues](#hash-addValues). You may apply one or more of these available reducers to a Hash entity. Keep in mind that those reducers will always be executed in a specific order:
 
@@ -2097,6 +2097,8 @@ A Collection entity enables you to operate over an array. Its API provides basic
 
 Collection entities expose a set of reducers that you may apply to them: [map](#collection-map), [find](#collection-find), [filter](#collection-filter). These reducers are executed in a [specific order](#collection-reducers-order). If you want to have more control over the order of execution, use the [compose](#compose-reducer) reducer.
 
+To prevent unexpected results, a **Collection Entity** can only process **Arrays**, if Collection.value does not resolve to an Array it will **throw** an error. 
+
 **IMPORTANT:** Keep in mind that in DataPoint, **all** operations are asynchronous. If your operations do NOT need to be asynchronous, iterating over a large array might result in slower execution. In such cases, consider using a reducer function where you can implement a synchronous solution.
 
 **SYNOPSIS**
diff --git a/packages/data-point/lib/entity-types/entity-collection/resolve.js b/packages/data-point/lib/entity-types/entity-collection/resolve.js
index 5ed1b33..05874e1 100644
--- a/packages/data-point/lib/entity-types/entity-collection/resolve.js
+++ b/packages/data-point/lib/entity-types/entity-collection/resolve.js
@@ -3,6 +3,7 @@
 const _ = require('lodash')
 const Promise = require('bluebird')
 const utils = require('../../utils')
+const Util = require('util')
 
 function resolveMapReducer (accumulator, reducer, resolveReducer) {
   if (utils.reducerIsEmpty(reducer)) {
@@ -95,18 +96,23 @@ function validateAsArray (acc) {
   return acc.value instanceof Array
     ? acc
     : Promise.reject(
-      new Error(`Entity ${entity.id}.value resolved to a non Array value`)
+      new Error(
+        Util.format(
+          '%s received acc.value = %s of type %s,',
+          entity.id,
+          _.truncate(Util.inspect(acc.value, { breakLength: Infinity }), {
+            length: 30
+          }),
+          utils.typeOf(acc.value),
+          'this entity only resolves Array values. More info https://github.com/ViacomInc/data-point/tree/master/packages/data-point#collection-entity'
+        )
+      )
     )
 }
 
 function resolve (accumulator, resolveReducer) {
   const entity = accumulator.reducer.spec
 
-  // if there is nothing to do, lets just move on
-  if (_.isEmpty(accumulator.value)) {
-    return Promise.resolve(accumulator)
-  }
-
   return resolveReducer(accumulator, entity.value)
     .then(acc => validateAsArray(acc))
     .then(acc => resolveCompose(acc, entity.compose, resolveReducer))
diff --git a/packages/data-point/lib/entity-types/entity-hash/resolve.js b/packages/data-point/lib/entity-types/entity-hash/resolve.js
index 90b93ee..45fa4bd 100644
--- a/packages/data-point/lib/entity-types/entity-hash/resolve.js
+++ b/packages/data-point/lib/entity-types/entity-hash/resolve.js
@@ -3,6 +3,7 @@
 const _ = require('lodash')
 const Promise = require('bluebird')
 const utils = require('../../utils')
+const Util = require('util')
 
 function resolveMapKeys (accumulator, reducer, resolveReducer) {
   return resolveReducer(accumulator, reducer).then(acc => {
@@ -61,12 +62,15 @@ function validateAsObject (acc) {
   }
   return Promise.reject(
     new Error(
-      `"${entity.id}" received acc.value = ${JSON.stringify(acc.value).substr(
-        0,
-        15
-      )} of type ${utils.typeOf(
-        acc.value
-      )} this entity only processes plain Objects. More info https://github.com/ViacomInc/data-point#hash-entity`
+      Util.format(
+        '%s received acc.value = %s of type %s,',
+        entity.id,
+        _.truncate(Util.inspect(acc.value, { breakLength: Infinity }), {
+          length: 30
+        }),
+        utils.typeOf(acc.value),
+        'this entity only resolves plain Objects. More info https://github.com/ViacomInc/data-point/tree/master/packages/data-point#hash-entity'
+      )
     )
   )
 }
@@ -101,11 +105,6 @@ function resolveCompose (accumulator, composeReducers, resolveReducer) {
 function resolve (acc, resolveReducer) {
   const entity = acc.reducer.spec
 
-  // if there is nothing to do, lets just move on
-  if (typeof acc.value === 'undefined' || acc.value === null) {
-    return Promise.resolve(acc)
-  }
-
   return resolveReducer(acc, entity.value)
     .then(itemContext => validateAsObject(itemContext))
     .then(acc => resolveCompose(acc, entity.compose, resolveReducer))


Test patch:
diff --git a/packages/data-point/lib/entity-types/entity-collection/__snapshots__/resolve.test.js.snap b/packages/data-point/lib/entity-types/entity-collection/__snapshots__/resolve.test.js.snap
new file mode 100644
index 0000000..2394d7a
--- /dev/null
+++ b/packages/data-point/lib/entity-types/entity-collection/__snapshots__/resolve.test.js.snap
@@ -0,0 +1,3 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`CollectionReducer.resolve entity.collection - throw error if value not array 1`] = `[Error: collection:ObjectsNotAllowed received acc.value = { c: [ 1, 2, 3 ] } of type object, this entity only resolves Array values. More info https://github.com/ViacomInc/data-point/tree/master/packages/data-point#collection-entity]`;
diff --git a/packages/data-point/lib/entity-types/entity-collection/resolve.test.js b/packages/data-point/lib/entity-types/entity-collection/resolve.test.js
index bfd5abd..21f8d33 100644
--- a/packages/data-point/lib/entity-types/entity-collection/resolve.test.js
+++ b/packages/data-point/lib/entity-types/entity-collection/resolve.test.js
@@ -45,10 +45,12 @@ describe('CollectionReducer.resolve', () => {
       })
   })
 
-  test('entity.collection - do nothing if empty', () => {
-    return transform('collection:noValue', null).then(result => {
-      expect(result.value).toEqual(null)
-    })
+  test('entity.collection - throw error if value not array', () => {
+    return transform('collection:ObjectsNotAllowed', testData)
+      .catch(err => err)
+      .then(result => {
+        expect(result).toMatchSnapshot()
+      })
   })
 })
 
diff --git a/packages/data-point/lib/entity-types/entity-hash/__snapshots__/resolve.test.js.snap b/packages/data-point/lib/entity-types/entity-hash/__snapshots__/resolve.test.js.snap
new file mode 100644
index 0000000..8a53480
--- /dev/null
+++ b/packages/data-point/lib/entity-types/entity-hash/__snapshots__/resolve.test.js.snap
@@ -0,0 +1,5 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`entity.hash.resolve entity.hash - only process Plain Objects 1`] = `"hash:asIs received acc.value = [ { foo: 1, a: { b: [Object... of type array, this entity only resolves plain Objects. More info https://github.com/ViacomInc/data-point/tree/master/packages/data-point#hash-entity"`;
+
+exports[`entity.hash.resolve entity.hash - throw error if value is not object 1`] = `[Error: hash:noValue received acc.value = 'invalid' of type string, this entity only resolves plain Objects. More info https://github.com/ViacomInc/data-point/tree/master/packages/data-point#hash-entity]`;
diff --git a/packages/data-point/lib/entity-types/entity-hash/resolve.test.js b/packages/data-point/lib/entity-types/entity-hash/resolve.test.js
index 28d0bf9..3a7dff9 100644
--- a/packages/data-point/lib/entity-types/entity-hash/resolve.test.js
+++ b/packages/data-point/lib/entity-types/entity-hash/resolve.test.js
@@ -32,21 +32,22 @@ beforeAll(() => {
 
 describe('entity.hash.resolve', () => {
   test('entity.hash - only process Plain Objects', () => {
-    return transform('hash:arraysNotAllowed', testData)
+    return transform('hash:asIs', [testData])
       .catch(result => {
         return result
       })
       .then(result => {
         expect(result).toBeInstanceOf(Error)
-        expect(result.message).toContain('[1,2,3] of type array')
-        expect(result.message).toContain('More info https://')
+        expect(result.message).toMatchSnapshot()
       })
   })
 
-  test('entity.hash - do nothing if empty', () => {
-    return transform('hash:noValue', null).then(result => {
-      expect(result.value).toEqual(null)
-    })
+  test('entity.hash - throw error if value is not object', () => {
+    return transform('hash:noValue', null)
+      .catch(e => e)
+      .then(result => {
+        expect(result).toMatchSnapshot()
+      })
   })
 })
 
diff --git a/packages/data-point/lib/entity-types/integrations.test.js b/packages/data-point/lib/entity-types/integrations.test.js
index 4f5cefa..2eb8a28 100644
--- a/packages/data-point/lib/entity-types/integrations.test.js
+++ b/packages/data-point/lib/entity-types/integrations.test.js
@@ -268,3 +268,39 @@ describe('trace feature', () => {
       })
   })
 })
+
+describe('handle undefined value', () => {
+  test('HashEntity - should throw error', () => {
+    return dataPoint
+      .transform('hash:noValue')
+      .catch(e => e)
+      .then(res => {
+        expect(res).toBeInstanceOf(Error)
+      })
+  })
+
+  test('CollectionEntity - should throw error', () => {
+    return dataPoint
+      .transform('collection:noValue')
+      .catch(e => e)
+      .then(res => {
+        expect(res).toBeInstanceOf(Error)
+      })
+  })
+
+  test('CollectionEntity - should ignore input', () => {
+    nock('http://remote.test')
+      .get('/source1')
+      .reply(200, {
+        ok: true
+      })
+    return dataPoint
+      .transform('request:a1')
+      .catch(e => e)
+      .then(res => {
+        expect(res.value).toEqual({
+          ok: true
+        })
+      })
+  })
+})