Repository: ViacomInc/data-point
Pull request #268 (2018-04-24 02:30:57)

refactor(data-point): Do not try to execute undefined entity lifecycle methods
**What**: Refactors the base-entity resolve function.

**Why**: Performance. I tested a simple entity with bench-trial, and the resolve time went from 19,530 ops/sec to 25,353 ops/sec, which is a ~30% improvement.


```js
'model:test': {
  value: () => true,
  outputType: 'boolean'
}
```

**How**:

Added conditionals in `base-entity/resolve.js` to make sure lifecycle methods / middleware are defined before trying to execute them (which extends the promise chain).

<!-- 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
- [ ] Documentation n/a
- [x] Tests
- [x] Ready to be merged

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

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

```diff
@@            Coverage Diff             @@
##           master     #268      +/-   ##
==========================================
- Coverage     100%   99.94%   -0.06%     
==========================================
  Files         127      127              
  Lines        1695     1711      +16     
  Branches      147      153       +6     
==========================================
+ Hits         1695     1710      +15     
- Misses          0        1       +1
```


| [Impacted Files](https://codecov.io/gh/ViacomInc/data-point/pull/268?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...data-point/lib/entity-types/base-entity/resolve.js](https://codecov.io/gh/ViacomInc/data-point/pull/268/diff?src=pr&el=tree#diff-cGFja2FnZXMvZGF0YS1wb2ludC9saWIvZW50aXR5LXR5cGVzL2Jhc2UtZW50aXR5L3Jlc29sdmUuanM=) | `100% <100%> (ø)` | :arrow_up: |
| [packages/data-point/lib/middleware/resolve.js](https://codecov.io/gh/ViacomInc/data-point/pull/268/diff?src=pr&el=tree#diff-cGFja2FnZXMvZGF0YS1wb2ludC9saWIvbWlkZGxld2FyZS9yZXNvbHZlLmpz) | `90.9% <0%> (-9.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/ViacomInc/data-point/pull/268?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/268?src=pr&el=footer). Last update [541983d...b1b47a1](https://codecov.io/gh/ViacomInc/data-point/pull/268?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

acatl: @raingerber would u like me add the fix so codeclimate passes? i can spend some time on it so we can this merged
acatl: probably you could configure your fork to sync with code climate? so when
you commit and push you can get a branch analysis before sending a PR
On Tue, Apr 24, 2018 at 9:13 PM Matthew Armstrong <notifications@github.com>
wrote:

> please feel free to fix that, but is there a way to run codeclimate
> locally? It's kind of annoying when each commit has 30 seconds worth of
> linting, but the PR is still rejected
>
> —
> You are receiving this because you commented.
>
>
> Reply to this email directly, view it on GitHub
> <https://github.com/ViacomInc/data-point/pull/268#issuecomment-384128852>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAOWZ9Rp_sraMogCU3x-Hc3r5dJu8M9cks5tr82dgaJpZM4Tg6CR>
> .
>
-- 
Acatl


Patch:
diff --git a/packages/data-point/lib/entity-types/base-entity/resolve.js b/packages/data-point/lib/entity-types/base-entity/resolve.js
index 2a61783..53ffd96 100644
--- a/packages/data-point/lib/entity-types/base-entity/resolve.js
+++ b/packages/data-point/lib/entity-types/base-entity/resolve.js
@@ -32,6 +32,34 @@ function resolveErrorReducers (manager, error, accumulator, resolveReducer) {
 
 module.exports.resolveErrorReducers = resolveErrorReducers
 
+/**
+ * @param {Promise} promise
+ * @param {*} condition
+ * @param {Function} callback
+ * @returns {Promise}
+ */
+function addToPromiseChain (promise, condition, callback) {
+  if (!condition) {
+    return promise
+  }
+
+  return promise.then(callback)
+}
+
+/**
+ * @param {Error} error
+ * @returns {*} bypass value
+ * @throws rethrows error if bypass was not found
+ */
+function handleByPassError (error) {
+  // checking if this is an error to bypass the `then` chain
+  if (error.bypass === true) {
+    return error.bypassValue
+  }
+
+  throw error
+}
+
 /**
  * @param {Object} manager
  * @param {Accumulator} accumulator
@@ -71,35 +99,47 @@ module.exports.createCurrentAccumulator = createCurrentAccumulator
  * then pls let me know and send a PR
  *
  * @param {Object} manager - dataPoint instance
+ * @param {Promise} promise - should resolve to the current accumulator
  * @param {string} name - name of middleware to execute
- * @param {Accumulator} acc - current accumulator
+ * @returns {Promise}
  */
-function resolveMiddleware (manager, name, acc) {
-  return middleware.resolve(manager, name, acc).then(middlewareResult => {
-    const reqCtx = utils.assign(acc, {
-      value: middlewareResult.value,
-      locals: middlewareResult.locals
-    })
+function resolveMiddleware (manager, promise, name) {
+  if (!manager.middleware.store.has(name)) {
+    return promise
+  }
 
-    if (middlewareResult.___resolve === true) {
-      // doing this until proven wrong :)
-      const err = new Error('bypassing middleware')
-      err.name = 'bypass'
-      err.bypass = true
-      err.bypassValue = reqCtx
-      return Promise.reject(err)
-    }
+  return promise
+    .then(accumulator => {
+      return middleware.resolve(manager, name, accumulator)
+    })
+    .then(middlewareResult => {
+      if (middlewareResult.___resolve === true) {
+        // doing this until proven wrong :)
+        const err = new Error('bypassing middleware')
+        err.name = 'bypass'
+        err.bypass = true
+        err.bypassValue = middlewareResult
+        throw err
+      }
 
-    return reqCtx
-  })
+      return middlewareResult
+    })
 }
 
 module.exports.resolveMiddleware = resolveMiddleware
 
-function typeCheck (manager, acc, reducer, resolveReducer) {
-  // if no error returns original accumulator
-  // this prevents typeCheckTransform from mutating the value
-  return resolveReducer(manager, acc, reducer).return(acc)
+/**
+ * @param {Object} manager
+ * @param {Accumulator} accumulator
+ * @param {Reducer} reducer
+ * @param {Function} resolveReducer
+ * @returns {Promise}
+ */
+function typeCheck (manager, accumulator, reducer, resolveReducer) {
+  // returns original accumulator if there's no error
+  // this helps prevent type check reducers from mutating the value, but
+  // it's still possible to modify the value by reference when it's an object
+  return resolveReducer(manager, accumulator, reducer).return(accumulator)
 }
 
 /**
@@ -127,52 +167,70 @@ function resolveEntity (
     currentAccumulator.trace === true ||
     currentAccumulator.reducer.spec.params.trace === true
 
-  let accUid = currentAccumulator
   let timeId
+  let accUid = currentAccumulator
   if (trace === true) {
     accUid = utils.set(currentAccumulator, 'euid', utils.getUID())
     timeId = `⧖ ${accUid.context.id}(${accUid.euid})`
     console.time(timeId)
   }
 
-  const resolveReducerBound = resolveReducer.bind(null, manager)
+  const {
+    inputType,
+    before,
+    after,
+    outputType
+  } = currentAccumulator.reducer.spec
 
-  return Promise.resolve(accUid)
-    .then(acc =>
-      typeCheck(manager, acc, acc.reducer.spec.inputType, resolveReducer)
-    )
-    .then(acc => resolveMiddleware(manager, `before`, acc))
-    .then(acc =>
-      resolveMiddleware(manager, `${reducer.entityType}:before`, acc)
-    )
-    .then(acc => resolveReducer(manager, acc, acc.reducer.spec.before))
-    .then(acc => mainResolver(acc, resolveReducerBound))
-    .then(acc => resolveReducer(manager, acc, acc.reducer.spec.after))
-    .then(acc => resolveMiddleware(manager, `${reducer.entityType}:after`, acc))
-    .then(acc => resolveMiddleware(manager, `after`, acc))
-    .catch(error => {
-      // checking if this is an error to bypass the `then` chain
-      if (error.bypass === true) {
-        return error.bypassValue
-      }
+  let result = Promise.resolve(accUid)
 
-      throw error
-    })
-    .then(acc =>
-      typeCheck(manager, acc, acc.reducer.spec.outputType, resolveReducer)
-    )
+  result = addToPromiseChain(result, inputType, acc =>
+    typeCheck(manager, acc, inputType, resolveReducer)
+  )
+
+  result = resolveMiddleware(manager, result, 'before')
+
+  result = resolveMiddleware(manager, result, `${reducer.entityType}:before`)
+
+  result = addToPromiseChain(result, before, acc =>
+    resolveReducer(manager, acc, before)
+  )
+
+  result = result.then(acc => {
+    return mainResolver(acc, resolveReducer.bind(null, manager))
+  })
+
+  result = addToPromiseChain(result, after, acc =>
+    resolveReducer(manager, acc, after)
+  )
+
+  result = resolveMiddleware(manager, result, `${reducer.entityType}:after`)
+
+  result = resolveMiddleware(manager, result, 'after')
+
+  result = result.catch(handleByPassError)
+
+  result = addToPromiseChain(result, outputType, acc =>
+    typeCheck(manager, acc, outputType, resolveReducer)
+  )
+
+  return result
     .catch(error => {
       // attach entity information to help debug
       error.entityId = currentAccumulator.reducer.spec.id
 
-      return resolveErrorReducers(
+      let errorResult = resolveErrorReducers(
         manager,
         error,
         currentAccumulator,
         resolveReducer
-      ).then(acc =>
-        typeCheck(manager, acc, acc.reducer.spec.outputType, resolveReducer)
       )
+
+      errorResult = addToPromiseChain(errorResult, outputType, acc =>
+        typeCheck(manager, acc, outputType, resolveReducer)
+      )
+
+      return errorResult
     })
     .then(resultContext => {
       if (trace === true) {
diff --git a/packages/data-point/lib/middleware-control/middleware.js b/packages/data-point/lib/middleware-control/middleware.js
index 7141eaf..64ab178 100644
--- a/packages/data-point/lib/middleware-control/middleware.js
+++ b/packages/data-point/lib/middleware-control/middleware.js
@@ -9,7 +9,7 @@ const Promise = require('bluebird')
  * @return {*}
  */
 function run (accumulator, stackSpec, done) {
-  if (stackSpec.length === 0) {
+  if (!stackSpec || stackSpec.length === 0) {
     return done(null, accumulator)
   }
 
diff --git a/packages/data-point/lib/middleware/resolve.js b/packages/data-point/lib/middleware/resolve.js
index 52e8eb1..36313c4 100644
--- a/packages/data-point/lib/middleware/resolve.js
+++ b/packages/data-point/lib/middleware/resolve.js
@@ -1,5 +1,3 @@
-const Promise = require('bluebird')
-
 const middlewareContextFactory = require('../middleware-context')
 const middlewareControl = require('../middleware-control')
 
@@ -9,7 +7,7 @@ const middlewareControl = require('../middleware-control')
  * @return {Array}
  */
 function getStack (store, middlewareName) {
-  return store.get(middlewareName) || []
+  return store.get(middlewareName)
 }
 
 module.exports.getStack = getStack
@@ -21,13 +19,9 @@ module.exports.getStack = getStack
  * @return {Promise}
  */
 function resolve (manager, middlewareName, accumulator) {
+  const middlewareContext = middlewareContextFactory.create(accumulator)
   const stack = getStack(manager.middleware.store, middlewareName)
-  if (stack.length === 0) {
-    return Promise.resolve(accumulator)
-  }
-
-  accumulator = middlewareContextFactory.create(accumulator)
-  return middlewareControl(accumulator, stack)
+  return middlewareControl(middlewareContext, stack)
 }
 
 module.exports.resolve = resolve


Test patch:
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 dce44c9..6bbf74b 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
@@ -1,5 +1,7 @@
 /* eslint-env jest */
 
+const Promise = require('bluebird')
+
 const ResolveEntity = require('./resolve')
 const createReducer = require('../../reducer-types').create
 const resolveReducer = require('../../reducer-types').resolve
@@ -102,11 +104,11 @@ describe('ResolveEntity.resolveMiddleware', () => {
       next(null)
     })
 
-    const racc = helpers.createAccumulator('foo')
+    const acc = helpers.createAccumulator('foo')
     return ResolveEntity.resolveMiddleware(
       dataPoint,
-      'request:before',
-      racc
+      Promise.resolve(acc),
+      'request:before'
     ).then(acc => {
       expect(acc.value).toEqual('bar')
     })
@@ -118,8 +120,12 @@ describe('ResolveEntity.resolveMiddleware', () => {
       next(null)
     })
 
-    const racc = helpers.createAccumulator('foo')
-    return ResolveEntity.resolveMiddleware(dataPoint, 'request:before', racc)
+    const acc = helpers.createAccumulator('foo')
+    return ResolveEntity.resolveMiddleware(
+      dataPoint,
+      Promise.resolve(acc),
+      'request:before'
+    )
       .catch(reason => reason)
       .then(reason => {
         expect(reason).toBeInstanceOf(Error)