Repository: ViacomInc/data-point (js)

Issue:
Change order of execution for entity type checking
**Problem description**:

The `inputType` and `outputType` entity methods are not the first/last to be executed, because middleware can be executed before and after those type checks occur. Here's an example of where that becomes a problem:

```js
const dataPoint = DataPoint.create({
  entities: {
    'model:thing': {
      value: () => 'THIS IS A STRING',
      after: input => input.toLowerCase(),
      outputType: 'string'
    }
  }
})

dataPoint.use('model:after', (acc, next) => {
  acc.resolve(52)
  next(null)
})

// this returns a number despite the outputType: 'string' type check
// resolves in this order: value -> after -> outputType -> 'model:after'
dataPoint.resolve('model:thing', {}) // => 52
```

**Suggested solution**:

`inputType` should be first entity method that's executed; `outputType` should be the last. There should not be any middleware executed before `inputType` or after `outputType`.

Pull request:
Change execution order to have type checks as bookends
<!--
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**: Moves the `inputType` and `outputType` resolution to the beginning/end of the entity resolution.

<!-- Why are these changes necessary? -->
**Why**: Closes #202

<!-- How were these changes implemented? -->
**How**: just moved the lines really.

<!-- 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
- [ ] 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 -->

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 9eff06a..3476c04 100644
--- a/packages/data-point/lib/entity-types/base-entity/resolve.js
+++ b/packages/data-point/lib/entity-types/base-entity/resolve.js
@@ -139,19 +139,16 @@ function resolveEntity (
   const resolveReducerBound = _.partial(resolveReducer, manager)
 
   return Promise.resolve(accUid)
-    .then(acc => resolveMiddleware(manager, `before`, acc))
     .then(acc =>
-      resolveMiddleware(manager, `${reducer.entityType}:before`, acc)
+      typeCheck(manager, acc, acc.reducer.spec.inputType, resolveReducer)
     )
+    .then(acc => resolveMiddleware(manager, `before`, acc))
     .then(acc =>
-      typeCheck(manager, acc, acc.reducer.spec.inputType, resolveReducer)
+      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 =>
-      typeCheck(manager, acc, acc.reducer.spec.outputType, resolveReducer)
-    )
     .then(acc => resolveMiddleware(manager, `${reducer.entityType}:after`, acc))
     .then(acc => resolveMiddleware(manager, `after`, acc))
     .catch(error => {
@@ -159,6 +156,13 @@ function resolveEntity (
       if (error.bypass === true) {
         return error.bypassValue
       }
+
+      throw error
+    })
+    .then(acc =>
+      typeCheck(manager, acc, acc.reducer.spec.outputType, resolveReducer)
+    )
+    .catch(error => {
       // attach entity information to help debug
       error.entityId = currentAccumulator.reducer.spec.id
 
@@ -167,6 +171,8 @@ function resolveEntity (
         error,
         currentAccumulator,
         resolveReducer
+      ).then(acc =>
+        typeCheck(manager, acc, acc.reducer.spec.outputType, resolveReducer)
       )
     })
     .then(resultContext => {


Test patch:
diff --git a/packages/data-point/lib/entity-types/base-entity/__snapshots__/resolve.test.js.snap b/packages/data-point/lib/entity-types/base-entity/__snapshots__/resolve.test.js.snap
index d66e03d..593d71a 100644
--- a/packages/data-point/lib/entity-types/base-entity/__snapshots__/resolve.test.js.snap
+++ b/packages/data-point/lib/entity-types/base-entity/__snapshots__/resolve.test.js.snap
@@ -1,7 +1,25 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`ResolveEntity.resolveEntity if custom typeCheck throws then fail 1`] = `[Error: custom type error]`;
-
 exports[`ResolveEntity.resolveEntity inputType - throws error if inputType does not pass 1`] = `[Error: value did not pass type check, type expected: number but recevied: string. The value received is: 'foo']`;
 
-exports[`ResolveEntity.resolveEntity outputType - throws error if outputType does not pass 1`] = `[Error: value did not pass type check, type expected: string but recevied: number. The value received is: 1]`;
+exports[`ResolveEntity.resolveEntity outputType fails if error method catches typeCheck errors and returns bad value 1`] = `[Error: value did not pass type check, type expected: string but recevied: number. The value received is: 2]`;
+
+exports[`ResolveEntity.resolveEntity outputType is bypassed if error throws error 1`] = `[Error: error from before method]`;
+
+exports[`ResolveEntity.resolveEntity outputType throws error if after method returns value that does not pass typeCheck 1`] = `[Error: value did not pass type check, type expected: string but recevied: number. The value received is: 1]`;
+
+exports[`ResolveEntity.resolveEntity outputType throws error if after middleware returns value that does not pass typeCheck 1`] = `[Error: value did not pass type check, type expected: string but recevied: number. The value received is: 1]`;
+
+exports[`ResolveEntity.resolveEntity outputType throws error if before method returns value that does not pass typeCheck 1`] = `[Error: value did not pass type check, type expected: string but recevied: number. The value received is: 1]`;
+
+exports[`ResolveEntity.resolveEntity outputType throws error if custom typeCheck fails 1`] = `[Error: custom type error]`;
+
+exports[`ResolveEntity.resolveEntity outputType throws error if global after middleware returns value that does not pass typeCheck 1`] = `[Error: value did not pass type check, type expected: string but recevied: number. The value received is: 1]`;
+
+exports[`ResolveEntity.resolveEntity outputType throws error if global before middleware returns value that does not pass typeCheck 1`] = `[Error: value did not pass type check, type expected: string but recevied: number. The value received is: 1]`;
+
+exports[`ResolveEntity.resolveEntity outputType throws error if middleware before returns value that does not pass typeCheck 1`] = `[Error: value did not pass type check, type expected: string but recevied: number. The value received is: 1]`;
+
+exports[`ResolveEntity.resolveEntity outputType throws error if value does not pass typeCheck 1`] = `[Error: value did not pass type check, type expected: string but recevied: number. The value received is: 1]`;
+
+exports[`ResolveEntity.resolveEntity outputType throws if error method does not return value with correct type 1`] = `[Error: value did not pass type check, type expected: string but recevied: number. The value received is: 1]`;
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 b30ef78..734bb03 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
@@ -211,8 +211,24 @@ describe('ResolveEntity.resolveEntity', () => {
       expect(ac.value).toEqual(1)
     })
   })
+})
+
+describe('ResolveEntity.resolveEntity outputType', () => {
+  const defaultResolver = (acc, resolveReducer) => Promise.resolve(acc)
+
+  const resolveEntity = (entityId, input, options, resolver) => {
+    const racc = helpers.createAccumulator.call(null, input, options)
+    const reducer = createReducerEntity(createReducer, entityId)
+    return ResolveEntity.resolveEntity(
+      dataPoint,
+      resolveReducer,
+      racc,
+      reducer,
+      resolver || defaultResolver
+    )
+  }
 
-  test('outputType - throws error if outputType does not pass', () => {
+  test('throws error if value does not pass typeCheck', () => {
     return resolveEntity('model:c.1', 1)
       .catch(e => e)
       .then(e => {
@@ -220,19 +236,123 @@ describe('ResolveEntity.resolveEntity', () => {
       })
   })
 
-  test('outputType - if typeCheck passes then resolve normal', () => {
-    return resolveEntity('model:c.1', 'foo').then(ac => {
-      expect(ac.value).toEqual('foo')
+  test('throws error if before method returns value that does not pass typeCheck', () => {
+    return resolveEntity('model:c.5', 'some string')
+      .catch(e => e)
+      .then(e => {
+        expect(e).toMatchSnapshot()
+      })
+  })
+
+  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)
+    })
+
+    return resolveEntity('model:c.1', 'some string')
+      .catch(e => e)
+      .then(e => {
+        expect(e).toMatchSnapshot()
+      })
+  })
+
+  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)
+    })
+
+    return resolveEntity('model:c.1', 'some string')
+      .catch(e => e)
+      .then(e => {
+        expect(e).toMatchSnapshot()
+      })
+  })
+
+  test('throws error if after method returns value that does not pass typeCheck', () => {
+    return resolveEntity('model:c.4', 'some string')
+      .catch(e => e)
+      .then(e => {
+        expect(e).toMatchSnapshot()
+      })
+  })
+
+  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)
+    })
+
+    return resolveEntity('model:c.1', 'some string')
+      .catch(e => e)
+      .then(e => {
+        expect(e).toMatchSnapshot()
+      })
+  })
+
+  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)
+    })
+
+    return resolveEntity('model:c.1', 'some string')
+      .catch(e => e)
+      .then(e => {
+        expect(e).toMatchSnapshot()
+      })
+  })
+
+  test('passes if error method returns value with correct type', () => {
+    return resolveEntity('model:c.6', 'string').then(acc => {
+      expect(acc.value).toBe('error string')
+    })
+  })
+
+  test('throws if error method does not return value with correct type', () => {
+    return resolveEntity('model:c.7', 'string')
+      .catch(e => e)
+      .then(e => {
+        expect(e).toMatchSnapshot()
+      })
+  })
+
+  test('is bypassed if error throws error', () => {
+    return resolveEntity('model:c.8', 'string')
+      .catch(e => e)
+      .then(e => {
+        expect(e).toMatchSnapshot()
+      })
+  })
+
+  test('passes if error method catches typeCheck errors and returns value', () => {
+    return resolveEntity('model:c.9', 'string').then(acc => {
+      expect(acc.value).toBe('string from error')
+    })
+  })
+
+  test('fails if error method catches typeCheck errors and returns bad value', () => {
+    return resolveEntity('model:c.10', 'string')
+      .catch(e => e)
+      .then(e => {
+        expect(e).toMatchSnapshot()
+      })
+  })
+
+  test('resolves normally if typeCheck passes', () => {
+    return resolveEntity('model:c.1', 'foo').then(acc => {
+      expect(acc.value).toEqual('foo')
     })
   })
 
-  test('typeCheck should not be able to change acc.value', () => {
+  test('does not change acc.value', () => {
     return resolveEntity('model:c.2', 'my string').then(result => {
       expect(result.value).toEqual('my string')
     })
   })
 
-  test('if custom typeCheck throws then fail', () => {
+  test('throws error if custom typeCheck fails', () => {
     return resolveEntity('model:c.3', 123)
       .catch(e => e)
       .then(result => {
diff --git a/packages/data-point/test/definitions/model.js b/packages/data-point/test/definitions/model.js
index 3b84801..8ccfcc7 100644
--- a/packages/data-point/test/definitions/model.js
+++ b/packages/data-point/test/definitions/model.js
@@ -33,5 +33,53 @@ module.exports = {
 
       return value
     }
+  },
+
+  'model:c.4': {
+    after: input => 1,
+    outputType: 'string'
+  },
+
+  'model:c.5': {
+    before: input => 1,
+    outputType: 'string'
+  },
+
+  'model:c.6': {
+    before: () => {
+      throw new Error()
+    },
+    error: () => 'error string',
+    outputType: 'string'
+  },
+
+  'model:c.7': {
+    before: () => {
+      throw new Error()
+    },
+    error: () => 1,
+    outputType: 'string'
+  },
+
+  'model:c.8': {
+    before: () => {
+      throw new Error('error from before method')
+    },
+    error: error => {
+      throw error
+    },
+    outputType: 'string'
+  },
+
+  'model:c.9': {
+    before: () => 1,
+    error: () => 'string from error',
+    outputType: 'string'
+  },
+
+  'model:c.10': {
+    before: () => 1,
+    error: () => 2,
+    outputType: 'string'
   }
 }