Repository: ViacomInc/data-point
Pull request #339 (2019-01-28 18:44:03)

206 use next instead of resolve
<!--
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!

BREAKING CHANGES:

If your PR includes a breaking change, please submit it with a codemod under
data-point-codemods that will help users upgrade their codebase.

Breaking changes without a codemod will not be accepted unless a codemod is not
viable or does not apply to the specific situation.
-->

<!-- What changes are being made? (What feature/bug is being fixed here?) -->
**What**:

Refactors the middleware API to now use the `next()` method to exit/resolve the middleware(stack) chain. 

<!-- Why are these changes necessary? -->
**Why**:

#206 

<!-- How were these changes implemented? -->
**How**:

Changing data-point and updating data-point-service to use the new API

<!-- 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] 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? -->
- [ ] Added username to **all-contributors** list

<!-- feel free to add additional comments -->

**ADDS BREAKING CHANGE:**

API changed, middleware acc.resolve is no longer available it will be repurposed
later on. To achieve the previous functionality you must pass a second parameter to the `next()` method.

See README for new API and examples on how to use it.


codecov[bot]: # [Codecov](https://codecov.io/gh/ViacomInc/data-point/pull/339?src=pr&el=h1) Report
> Merging [#339](https://codecov.io/gh/ViacomInc/data-point/pull/339?src=pr&el=desc) into [master](https://codecov.io/gh/ViacomInc/data-point/commit/dff25484081317c06d9040f311df3a5a1dbb3bfe?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/ViacomInc/data-point/pull/339/graphs/tree.svg?width=650&token=LYxItnrx4i&height=150&src=pr)](https://codecov.io/gh/ViacomInc/data-point/pull/339?src=pr&el=tree)

```diff
@@          Coverage Diff          @@
##           master   #339   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         139    139           
  Lines        1989   1988    -1     
  Branches      183    182    -1     
=====================================
- Hits         1989   1988    -1
```


| [Impacted Files](https://codecov.io/gh/ViacomInc/data-point/pull/339?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ackages/data-point-service/lib/cache-middleware.js](https://codecov.io/gh/ViacomInc/data-point/pull/339/diff?src=pr&el=tree#diff-cGFja2FnZXMvZGF0YS1wb2ludC1zZXJ2aWNlL2xpYi9jYWNoZS1taWRkbGV3YXJlLmpz) | `100% <100%> (ø)` | :arrow_up: |
| [...es/data-point/lib/middleware-control/middleware.js](https://codecov.io/gh/ViacomInc/data-point/pull/339/diff?src=pr&el=tree#diff-cGFja2FnZXMvZGF0YS1wb2ludC9saWIvbWlkZGxld2FyZS1jb250cm9sL21pZGRsZXdhcmUuanM=) | `100% <100%> (ø)` | :arrow_up: |
| [...kages/data-point/lib/middleware-context/factory.js](https://codecov.io/gh/ViacomInc/data-point/pull/339/diff?src=pr&el=tree#diff-cGFja2FnZXMvZGF0YS1wb2ludC9saWIvbWlkZGxld2FyZS1jb250ZXh0L2ZhY3RvcnkuanM=) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/ViacomInc/data-point/pull/339?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/ViacomInc/data-point/pull/339?src=pr&el=footer). Last update [dff2548...95125d1](https://codecov.io/gh/ViacomInc/data-point/pull/339?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/packages/data-point-service/lib/cache-middleware.js b/packages/data-point-service/lib/cache-middleware.js
index d83b47b..b38b0c4 100644
--- a/packages/data-point-service/lib/cache-middleware.js
+++ b/packages/data-point-service/lib/cache-middleware.js
@@ -260,7 +260,7 @@ function before (service, ctx, next) {
     })
     .then(value => {
       if (value !== undefined) {
-        ctx.resolve(value)
+        next(null, value)
       }
     })
     .asCallback(next)
@@ -310,7 +310,9 @@ function after (service, ctx, next) {
     )
   }
 
-  resolution.asCallback(next)
+  // ensuring we call next only with one parameter to prevent from
+  // exiting the middleware chain
+  resolution.asCallback(error => next(error))
 
   return true
 }
diff --git a/packages/data-point/README.md b/packages/data-point/README.md
index c9066ff..7309ea0 100644
--- a/packages/data-point/README.md
+++ b/packages/data-point/README.md
@@ -3369,28 +3369,84 @@ dataPoint.use(id:String, callback:Function)
 | Argument | Type | Description |
 |:---|:---|:---|
 | *id* | `string` | This ID is a string with the form `<EntityType>:<EventType>`, where `<EntityType>` is any registered entity type and `<EventType>` is either `'before'` or `'after'`. |
-| *callback* | `Function` | This is the callback function that will be executed once an entity event is triggered. The callback has the form `(acc, next)`, where `acc` is the current middleware [Middleware Accumulator](#middleware-accumulator-object) object, and next is a function callback to be executed once the middleware is done executing. The `next` callback uses the form of `(error)`. |
+| *callback* | `Function` | This is the callback function that will be executed once an entity event is triggered. The callback has the form `(acc, next)`, where `acc` is the current middleware [Middleware Accumulator](#middleware-accumulator-object) object, and next is a function callback to be executed once the middleware is done executing. The `next` callback uses the form of `(error, resolvedValue)`. |
 
-### Middleware Accumulator object
+**EXAMPLE:**
 
-This is the current [Accumulator](#accumulator) object with a `resolve(value)` method appended to it. If `acc.resolve(value)` is called inside a middleware function, the entity will resolve to that value without executing any remaining methods. This allows you to skip unnecessary work if, for example, a cached return value was found.
+```js
+const dp = DataPoint.create()
 
-**NOTE**: It's still required to call `next()` after `acc.resolve(value)`; otherwise, the function will hang indefinitely.
+dp.use('before', (acc, next) => {
+  console.log(`Entity ${acc.reducer.id} is being called`)
+  next()
+})
 
-**SYNOPSIS**
+dp.use('after', (acc, next) => {
+  console.log(`Entity ${acc.reducer.id} was called`)
+  next()
+})
 
-```js
-{ // extends Accumulator
-  resolve: Function,
-  ...
-}
+const MyModel = DataPoint.Model('MyModel', {
+  value: () => 'new value'
+})
+
+dp.resolve(MyModel, true)
+  // console output: 
+  //   Entity model:MyModel is being called
+  //   Entity model:MyModel was called
+  .then(() => {
+
+  })
 ```
 
-**API:**
+### Exiting Middleware chain
 
-| Key | Type | Description |
-|:---|:---|:---|
-| `resolve` | `Function` | Will resolve the entire entity with the value passed. This function has the form of: `(value)` |
+To exit the middleware chain with a resolved value you must pass a second parameter to the `next(err, val)` function provided by the middleware handler. By calling the `next` function with `null` as first parameter and a value as a second parameter (eg. `next(null, newValue`) the entity will resolve to that value without executing any remaining methods in the middleware chain. This allows you to skip unnecessary work if, for example, a cached return value was found.
+
+**NOTE**: the `next` method should be called only once per middleware handler, multiple calls will be ignored.
+
+
+<details>
+  <summary>hijacking the value of an entity</summary>
+
+  ```js
+  const dp = DataPoint.create()
+
+  dp.use('before', (acc, next) => {
+    console.log('Entity model:MyModel is being called')
+    next()
+  })
+
+  dp.use('before', (acc, next) => {
+    console.log('hijacking')
+    next(null, 'hijacked')
+  })
+
+  dp.use('before', (acc, next) => {
+    console.log('never got here')
+    next()
+  })
+
+  const MyModel = DataPoint.Model('MyModel', {
+    value: () => {
+      // this will not be executed because the entity was hijacked
+      console.log('processing')
+      return 'hello'
+    }
+  })
+
+  dp.resolve(MyModel, true)
+    // console output:
+    //   Entity model:MyModel is being called
+    //   hijacking
+    .then((value) => {
+      assert.strictEqual(value, 'hijacked')
+    })
+  ```
+
+</details>
+
+Example at: [examples/middleware-exit-chain.js](examples/middleware-exit-chain.js)
 
 ## Custom Entity Types
 
diff --git a/packages/data-point/examples/middleware-exit-chain.js b/packages/data-point/examples/middleware-exit-chain.js
new file mode 100644
index 0000000..2865035
--- /dev/null
+++ b/packages/data-point/examples/middleware-exit-chain.js
@@ -0,0 +1,35 @@
+const assert = require('assert')
+const DataPoint = require('data-point')
+const dp = DataPoint.create()
+
+dp.use('before', (acc, next) => {
+  console.log('Entity model:MyModel is being called')
+  next()
+})
+
+dp.use('before', (acc, next) => {
+  console.log('hijacking')
+  next(null, 'hijacked')
+})
+
+dp.use('before', (acc, next) => {
+  console.log('never got here')
+  next()
+})
+
+const MyModel = DataPoint.Model('MyModel', {
+  value: () => {
+    // this will not be executed because the entity was hijacked
+    console.log('processing')
+    return 'hello'
+  }
+})
+
+dp
+  .resolve(MyModel, true)
+  // console output:
+  // Entity model:MyModel is being called
+  // hijacking
+  .then(value => {
+    assert.strictEqual(value, 'hijacked')
+  })
diff --git a/packages/data-point/lib/middleware-context/factory.js b/packages/data-point/lib/middleware-context/factory.js
index fd03cc1..550640b 100644
--- a/packages/data-point/lib/middleware-context/factory.js
+++ b/packages/data-point/lib/middleware-context/factory.js
@@ -7,29 +7,9 @@ const _ = require('lodash')
  */
 function MiddlewareContext () {
   this.___done = false
-  this.___resolve = false
+  this.___resolve = 0
 }
 
-/**
- * @param {*} value
- * @throws if it's been called more than once
- */
-MiddlewareContext.prototype.resolve = function resolve (value) {
-  if (this.___resolve === true || this.___done === true) {
-    throw new Error('can not execute resolve() more than once per stack chain.')
-  }
-
-  // only assign if an argument is passed, regardless of the value
-  if (arguments.length) {
-    this.value = value
-    this.___resolve = true
-  }
-
-  this.___done = true
-}
-
-module.exports.MiddlewareContext = MiddlewareContext
-
 /**
  * @param {Object} spec
  * @return {Object}
diff --git a/packages/data-point/lib/middleware-control/middleware.js b/packages/data-point/lib/middleware-control/middleware.js
index 64ab178..c1c85ee 100644
--- a/packages/data-point/lib/middleware-control/middleware.js
+++ b/packages/data-point/lib/middleware-control/middleware.js
@@ -1,4 +1,6 @@
-const _ = require('lodash')
+const once = require('lodash/once')
+const attempt = require('lodash/attempt')
+const isError = require('lodash/isError')
 const Promise = require('bluebird')
 
 /**
@@ -15,24 +17,31 @@ function run (accumulator, stackSpec, done) {
 
   const stack = stackSpec.slice(0)
 
-  function next (err) {
+  const onceDone = once(done)
+  function next (err, value) {
     if (err) {
-      return done(err, accumulator)
+      return onceDone(err, accumulator)
+    }
+
+    if (arguments.length === 2) {
+      accumulator.value = value
+      accumulator.___resolve = true
+      accumulator.___done = true
     }
 
     if (accumulator.___done === true) {
-      return done(null, accumulator)
+      return onceDone(null, accumulator)
     }
 
     const middlewareFunc = stack.shift()
 
     if (typeof middlewareFunc === 'undefined') {
-      return done(null, accumulator)
+      return onceDone(null, accumulator)
     }
 
-    const execError = _.attempt(middlewareFunc, accumulator, next)
+    const execError = attempt(middlewareFunc, accumulator, next)
 
-    if (_.isError(execError)) {
+    if (isError(execError)) {
       next(execError)
     }
 


Test patch:
diff --git a/packages/data-point-service/lib/cache-middleware.test.js b/packages/data-point-service/lib/cache-middleware.test.js
index f39c234..79fa548 100644
--- a/packages/data-point-service/lib/cache-middleware.test.js
+++ b/packages/data-point-service/lib/cache-middleware.test.js
@@ -41,8 +41,7 @@ function createContext () {
     context: {
       params: {},
       id: 'model:Foo'
-    },
-    resolve: () => true
+    }
   }
 
   return ctx
@@ -520,14 +519,12 @@ describe('before', () => {
   function createMocks () {
     const mocks = {}
     mocks.ctx = createContext()
-    mocks.ctx.resolve = jest.fn()
     mocks.service = {}
     mocks.cache = {
       ttl: '20m',
       cacheKey: () => true,
       useStaleWhileRevalidate: true
     }
-
     mocks.next = jest.fn()
 
     mocks.getCacheParams = jest
@@ -578,7 +575,7 @@ describe('before', () => {
     const mocks = createMocks()
 
     const next = createNext(done, () => {
-      expect(mocks.ctx.resolve).toBeCalledWith('staleResult')
+      expect(next).toBeCalledWith(null, 'staleResult')
       expect(mocks.resolveStaleWhileRevalidateEntry).toBeCalledWith(
         mocks.service,
         'mockCacheKey',
@@ -598,7 +595,7 @@ describe('before', () => {
     mocks.cache.useStaleWhileRevalidate = false
 
     const next = createNext(done, () => {
-      expect(mocks.ctx.resolve).toBeCalledWith('noTTLEntry')
+      expect(next).toBeCalledWith(null, 'noTTLEntry')
       expect(mocks.resolveStaleWhileRevalidateEntry).not.toBeCalled()
       expect(mocks.getEntry).toBeCalledWith(mocks.service, 'mockCacheKey')
     })
@@ -611,7 +608,7 @@ describe('before', () => {
     const mocks = createMocks()
 
     const next = createNext(done, () => {
-      expect(mocks.ctx.resolve).not.toBeCalled()
+      expect(next).toBeCalledWith(null)
       expect(mocks.resolveStaleWhileRevalidateEntry).toBeCalledWith(
         mocks.service,
         'mockCacheKey',
@@ -631,7 +628,6 @@ describe('after', () => {
     const mocks = {}
     mocks.ctx = createContext()
     mocks.ctx.value = 'resolvedValue'
-    mocks.ctx.resolve = jest.fn()
     mocks.service = {}
     mocks.cache = {
       ttl: '20m',
@@ -696,6 +692,18 @@ describe('after', () => {
     expect(result).toEqual(true)
   })
 
+  it('should call next with only 1 argument to avoid exiting the middleware chain', done => {
+    const mocks = createMocks()
+
+    const next = createNext(done, () => {
+      expect(next).toBeCalledWith(null)
+    })
+
+    mocks.cache.useStaleWhileRevalidate = false
+    const result = CacheMiddleware.after(mocks.service, mocks.ctx, next)
+    expect(result).toEqual(true)
+  })
+
   it('should set basic redis entry value if cache.useStaleWhileRevalidate is false', done => {
     const mocks = createMocks()
 
diff --git a/packages/data-point/lib/entity-types/base-entity/resolve.test.js b/packages/data-point/lib/entity-types/base-entity/resolve.test.js
index 0e74301..18a2ccf 100644
--- a/packages/data-point/lib/entity-types/base-entity/resolve.test.js
+++ b/packages/data-point/lib/entity-types/base-entity/resolve.test.js
@@ -181,8 +181,7 @@ describe('ResolveEntity.resolveMiddleware', () => {
 
   test('It should execute a middleware that forces an error to bypass the promise chain', () => {
     dataPoint.middleware.use('request:before', (acc, next) => {
-      acc.resolve('bar')
-      next(null)
+      next(null, 'bar')
     })
 
     const acc = helpers.createAccumulator('foo')
@@ -237,8 +236,7 @@ describe('ResolveEntity.resolveEntity', () => {
 
   test('It should resolve through bypass', () => {
     dataPoint.middleware.use('hash:before', (acc, next) => {
-      acc.resolve({ data: 'bar' })
-      next(null)
+      next(null, { data: 'bar' })
     })
     return resolveEntity('hash:asIs', 'foo').then(acc => {
       expect(acc.value).toEqual({ data: 'bar' })
@@ -290,8 +288,7 @@ describe('ResolveEntity.resolveEntity outputType', () => {
 
   test('throws error if middleware before returns value that does not pass typeCheck', () => {
     dataPoint.middleware.use('model:before', (acc, next) => {
-      acc.resolve(1)
-      next(null)
+      next(null, 1)
     })
 
     return resolveEntity('model:c.1', 'some string')
@@ -303,8 +300,7 @@ describe('ResolveEntity.resolveEntity outputType', () => {
 
   test('throws error if global before middleware returns value that does not pass typeCheck', () => {
     dataPoint.middleware.use('before', (acc, next) => {
-      acc.resolve(1)
-      next(null)
+      next(null, 1)
     })
 
     return resolveEntity('model:c.1', 'some string')
@@ -324,8 +320,7 @@ describe('ResolveEntity.resolveEntity outputType', () => {
 
   test('throws error if after middleware returns value that does not pass typeCheck', () => {
     dataPoint.middleware.use('model:after', (acc, next) => {
-      acc.resolve(1)
-      next(null)
+      next(null, 1)
     })
 
     return resolveEntity('model:c.1', 'some string')
@@ -337,8 +332,7 @@ describe('ResolveEntity.resolveEntity outputType', () => {
 
   test('throws error if global after middleware returns value that does not pass typeCheck', () => {
     dataPoint.middleware.use('after', (acc, next) => {
-      acc.resolve(1)
-      next(null)
+      next(null, 1)
     })
 
     return resolveEntity('model:c.1', 'some string')
diff --git a/packages/data-point/lib/middleware-context/factory.test.js b/packages/data-point/lib/middleware-context/factory.test.js
index 2b8ff37..911a84e 100644
--- a/packages/data-point/lib/middleware-context/factory.test.js
+++ b/packages/data-point/lib/middleware-context/factory.test.js
@@ -4,37 +4,8 @@ const Factory = require('./factory')
 
 test('middleware-context.create', () => {
   const result = Factory.create({})
-  expect(result.resolve).toBeInstanceOf(Function)
-})
-
-describe('middleware-context.resolve', () => {
-  test('assign value property', () => {
-    const result = Factory.create({})
-    result.resolve('test')
-    expect(result.value).toBe('test')
-  })
-
-  test('throw error if already resolved', () => {
-    const result = Factory.create({})
-    expect(() => {
-      result.___resolve = true
-      result.resolve('test')
-    }).toThrow()
-  })
-
-  test('throw error if done', () => {
-    const result = Factory.create({})
-    expect(() => {
-      result.___done = true
-      result.resolve('test')
-    }).toThrow()
-  })
-
-  test('resolve() with no arguments, should not have side effects', () => {
-    const result = Factory.create({})
-    result.value = 'persist'
-    result.resolve()
-    expect(result.___resolve).toBe(false)
-    expect(result.___done).toBe(true)
+  expect(result).toEqual({
+    ___done: false,
+    ___resolve: 0
   })
 })
diff --git a/packages/data-point/lib/middleware-control/middleware.test.js b/packages/data-point/lib/middleware-control/middleware.test.js
index f035f08..7d2c2d0 100644
--- a/packages/data-point/lib/middleware-control/middleware.test.js
+++ b/packages/data-point/lib/middleware-control/middleware.test.js
@@ -126,6 +126,45 @@ test('middleware#run - exit chain when ___done set to true', () => {
   })
 })
 
+test('middleware#run - exit when next is called with two parameters, value should be second parameter', () => {
+  const stack = [
+    (acc, next) => {
+      next(null)
+    },
+    (acc, next) => {
+      next(null, 'b')
+    },
+    (acc, next) => {
+      /* istanbul ignore next */
+      next(null, 'c')
+    }
+  ]
+
+  const acc = {}
+
+  return middleware.execute(acc, stack).then(context => {
+    expect(context.value).toEqual('b')
+  })
+})
+
+test('middleware#run - only one call to next with resolved value should be used', async () => {
+  const stack = [
+    (acc, next) => {
+      next()
+    },
+    (acc, next) => {
+      next(null, 'a')
+      setInterval(next, null, 'b')
+    }
+  ]
+
+  const acc = {}
+  await expect(middleware.execute(acc, stack)).resolves.toHaveProperty(
+    'value',
+    'a'
+  )
+})
+
 test('middleware#run - exit chain on error', () => {
   const stack = [
     (acc, next) => {