Repository: ViacomInc/data-point (js)

Issue:
stale while revalidate only works for registered entities
<!--
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).

Please fill/un-comment out this template with all the relevant information so we can
understand what's going on and fix the issue.
-->

**Problem description**:

Stale while revalidate logic is hooked up in a way that it only works for registered entities, the issue is because when we attempt at revalidating the entity, we do it via its entityId (string) and not by its reference (context)

https://github.com/ViacomInc/data-point/blob/dff25484081317c06d9040f311df3a5a1dbb3bfe/packages/data-point-service/lib/cache-middleware.js#L112-L134

**Suggested solution**:

Use the context instead of the id

<!-- 
REPORTING BUG:

If you are reporting a bug, please provide the full error message/screenshots/anything.
-->

**What happened**:

Using staleWhileRevalidate on an Entity instance does not work and throws the error: InvalidEntity.

Pull request:
fix(data-point-service): fixes revalidation logic
<!--
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**:

- fix bug where revalidation was not being triggered
- provides support registered and instance entities

<!-- Why are these changes necessary? -->
**Why**:

closes #340

<!-- How were these changes implemented? -->
**How**:

passing the entity reducer (accumulator.context) to resolveFromAccumulator, and fixed logic to detect when a remote (redis) entry should revalidate.

I also added more debugging logs since it was getting harder to track the steps, this should help in the future, below is an example of the output of running one of the examples: 

```bash
  data-point:entity:model model:HelloWorld:1 - resolve:start +0ms
  data-point-service:cache:stale-while-revalidate External control exists: false - entity:model:HelloWorld +0ms
  data-point:entity:model model:HelloWorld:1 - resolve +30ms
  data-point-service:cache:stale-while-revalidate Adding entry - entity:model:HelloWorld +4ms
  data-point-service:cache:stale-while-revalidate Setting control to status: SWR-CONTROL-STALE - entity:model:HelloWorld +6ms
  data-point:entity:model model:HelloWorld:1 - resolve:end +9ms
-------------manually expiring control key--------------  data-point:entity:model model:HelloWorld:2 - resolve:start +52s
  data-point-service:cache:stale-while-revalidate External control exists: false - entity:model:HelloWorld +52s
  data-point-service:cache Revalidating entityId: model:HelloWorld with cache key: entity:model:HelloWorld +0ms
  data-point-service:cache:stale-while-revalidate Add revalidation control flags - timeout: 5000ms - entity:model:HelloWorld +8ms
  data-point-service:cache:stale-while-revalidate Setting control to status: SWR-CONTROL-REVALIDATING (5000ms ttl) - entity:model:HelloWorld +0ms
  data-point:entity:model model:HelloWorld:2 - will bypass +14ms
  data-point:entity:model model:HelloWorld:2 - resolve:end +4ms
  data-point-service:cache Resolve entryKey: entity:model:HelloWorld with entityId: model:HelloWorld +11ms
  data-point:entity:model model:HelloWorld:3 - resolve:start +5ms
  data-point:entity:model model:HelloWorld:3 - resolve +2ms
  data-point:entity:model model:HelloWorld:3 - resolve:end +1ms
  data-point-service:cache Updating cache key: entity:model:HelloWorld with new stale value +5ms
  data-point-service:cache:stale-while-revalidate Adding entry - entity:model:HelloWorld +15ms
  data-point-service:cache:stale-while-revalidate Setting control to status: SWR-CONTROL-STALE - entity:model:HelloWorld +4ms
  data-point-service:cache Succesful revalidation entityId: model:HelloWorld with cache key: entity:model:HelloWorld +10ms
```

<!-- 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] Tests
- [X] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->

<!-- feel free to add additional comments -->


Patch:
diff --git a/packages/data-point-service/examples/express-cache-stale-while-revalidate.js b/packages/data-point-service/examples/express-cache-stale-while-revalidate-instance-entity.js
similarity index 98%
rename from packages/data-point-service/examples/express-cache-stale-while-revalidate.js
rename to packages/data-point-service/examples/express-cache-stale-while-revalidate-instance-entity.js
index 7b0720f..e2b26f9 100644
--- a/packages/data-point-service/examples/express-cache-stale-while-revalidate.js
+++ b/packages/data-point-service/examples/express-cache-stale-while-revalidate-instance-entity.js
@@ -45,7 +45,7 @@ function createService () {
         },
         params: {
           cache: {
-            ttl: '1m',
+            ttl: '30s',
             staleWhileRevalidate: '3m'
           }
         }
diff --git a/packages/data-point-service/examples/express-cache-stale-while-revalidate-registered-entity.js b/packages/data-point-service/examples/express-cache-stale-while-revalidate-registered-entity.js
new file mode 100644
index 0000000..c959467
--- /dev/null
+++ b/packages/data-point-service/examples/express-cache-stale-while-revalidate-registered-entity.js
@@ -0,0 +1,58 @@
+const express = require('express')
+const DataPoint = require('data-point')
+const DataPointService = require('../lib')
+
+const Model = DataPoint.Model('HelloWorld', {
+  value: (input, acc) => {
+    const person = acc.locals.query.person
+    if (!person) {
+      throw new Error(
+        'Url query parameter "person" is missing. Try appending ?person=Darek at the end of the URL'
+      )
+    }
+    return `Hello ${person}!!`
+  },
+  params: {
+    cache: {
+      ttl: '30s',
+      staleWhileRevalidate: '3m'
+    }
+  }
+})
+
+function server (dataPoint) {
+  const app = express()
+
+  app.get('/api/hello-world', (req, res) => {
+    const options = {
+      // exposing query to locals will make this object available to all
+      // reducers
+      locals: {
+        query: req.query
+      }
+    }
+    dataPoint
+      .resolve(Model, {}, options)
+      .then(value => {
+        const responseText = `${value} (person = "${req.query.person}")`
+        res.send(responseText)
+      })
+      .catch(error => {
+        res.status(500).send(error.toString())
+      })
+  })
+
+  app.listen(3000, function () {
+    console.log('listening on port 3000!')
+  })
+}
+
+function createService () {
+  return DataPointService.create({
+    DataPoint
+  }).then(service => {
+    return service.dataPoint
+  })
+}
+
+createService().then(server)
diff --git a/packages/data-point-service/lib/cache-middleware.js b/packages/data-point-service/lib/cache-middleware.js
index b38b0c4..6f58143 100644
--- a/packages/data-point-service/lib/cache-middleware.js
+++ b/packages/data-point-service/lib/cache-middleware.js
@@ -102,6 +102,20 @@ function shouldTriggerRevalidate (staleEntry, revalidationState) {
   )
 }
 
+function addRevalidationFlags (service, entryKey, revalidateTimeout) {
+  return service.staleWhileRevalidate.addRevalidationFlags(
+    entryKey,
+    revalidateTimeout
+  )
+}
+
+function resolveFromAccumulator (service, entryKey, reducer, accumulator) {
+  return () => {
+    debug('Resolve entryKey: %s with entityId: %s', entryKey, reducer.id)
+    return service.dataPoint.resolveFromAccumulator(reducer, accumulator)
+  }
+}
+
 /**
  * @param {Service} service Service instance
  * @param {String} entryKey entry key
@@ -126,22 +140,16 @@ function revalidateEntry (service, entryKey, cache, ctx) {
 
   debug('Revalidating entityId: %s with cache key: %s', entityId, entryKey)
 
-  const tasks = [
-    service.staleWhileRevalidate.addRevalidationFlags(
-      entryKey,
-      cache.revalidateTimeout
-    ),
-    service.dataPoint.resolveFromAccumulator(entityId, revalidateContext)
-  ]
-
   const {
     updateSWREntry,
     revalidateSuccess,
     catchRevalidateError
   } = module.exports
 
-  return Promise.all(tasks)
-    .then(results => results[1])
+  return addRevalidationFlags(service, entryKey, cache.revalidateTimeout)
+    .then(
+      resolveFromAccumulator(service, entryKey, ctx.context, revalidateContext)
+    )
     .then(updateSWREntry(service, entryKey, cache))
     .then(revalidateSuccess(service, entityId, entryKey))
     .catch(catchRevalidateError(service, entityId, entryKey))
diff --git a/packages/data-point-service/lib/data-point-factory.js b/packages/data-point-service/lib/data-point-factory.js
index d012c74..3fed057 100644
--- a/packages/data-point-service/lib/data-point-factory.js
+++ b/packages/data-point-service/lib/data-point-factory.js
@@ -1,14 +1,8 @@
-const _ = require('lodash')
-
 function verify (options) {
   if (!options.DataPoint) {
     throw new Error('DataPoint module must be provided')
   }
 
-  if (_.isEmpty(options.entities)) {
-    throw new Error('DataPoint options.entities should not be Empty')
-  }
-
   return options
 }
 
diff --git a/packages/data-point-service/lib/stale-while-revalidate.js b/packages/data-point-service/lib/stale-while-revalidate.js
index b2b518e..2ec6415 100644
--- a/packages/data-point-service/lib/stale-while-revalidate.js
+++ b/packages/data-point-service/lib/stale-while-revalidate.js
@@ -1,6 +1,9 @@
 const revalidationStoreFactory = require('./revalidation-store').create
 
 const RedisController = require('./redis-controller')
+const debug = require('debug')(
+  'data-point-service:cache:stale-while-revalidate'
+)
 
 /**
  * Flag for redis control entry flag, when set it means the stored result should
@@ -22,6 +25,12 @@ const SWR_CONTROL_REVALIDATING = 'SWR-CONTROL-REVALIDATING'
 function revalidationExternalFactory (service) {
   return {
     add: (entryKey, ttl) => {
+      debug(
+        'Setting control to status: %s (%sms ttl) - %s',
+        SWR_CONTROL_REVALIDATING,
+        ttl,
+        entryKey
+      )
       return RedisController.setSWRControlEntry(
         service,
         entryKey,
@@ -30,11 +39,16 @@ function revalidationExternalFactory (service) {
       )
     },
     remove: entryKey => {
+      debug('Removing external Control - %s', entryKey)
       return RedisController.deleteSWRControlEntry(service, entryKey)
     },
     exists: entryKey => {
       return RedisController.getSWRControlEntry(service, entryKey).then(
-        value => typeof value !== 'undefined'
+        controlEntryValue => {
+          const entryExists = typeof controlEntryValue !== 'undefined'
+          debug('External control exists: %s - %s', entryExists, entryKey)
+          return entryExists
+        }
       )
     }
   }
@@ -48,19 +62,21 @@ function revalidationExternalFactory (service) {
  * @returns {Promise}
  */
 function addEntry (service, entryKey, value, cache) {
+  debug('Adding entry - %s', entryKey)
   return RedisController.setSWRStaleEntry(
     service,
     entryKey,
     value,
     cache.staleWhileRevalidateTtl
-  ).then(() =>
-    RedisController.setSWRControlEntry(
+  ).then(() => {
+    debug('Setting control to status: %s - %s', SWR_CONTROL_STALE, entryKey)
+    return RedisController.setSWRControlEntry(
       service,
       entryKey,
       cache.ttl,
       SWR_CONTROL_STALE
     )
-  )
+  })
 }
 
 /**
@@ -74,13 +90,18 @@ function getEntry (service, entryKey) {
 
 /**
  * Adds revalidation flags locally (node instance) and external (redis) to
- * prevent duplicated revalidations of same key.
+ * prevent duplicated revalidation of same key.
  * @param {Object} revalidation revalidation API
  * @param {String} entryKey cache entry key
  * @param {Number} revalidateTimeout time to timeout revalidation in milliseconds
  * @returns {Promise}
  */
 function addRevalidationFlags (revalidation, entryKey, revalidateTimeout) {
+  debug(
+    'Add revalidation control flags - timeout: %sms - %s',
+    revalidateTimeout,
+    entryKey
+  )
   // local (node instance) flag is set to immediately prevent concurrent calls
   revalidation.local.add(entryKey, revalidateTimeout)
   // external (redis) flag is set to prevent multiple instances from duplicating
@@ -93,6 +114,7 @@ function addRevalidationFlags (revalidation, entryKey, revalidateTimeout) {
  * @param {String} entryKey cache entry key
  */
 function clearAllRevalidationFlags (revalidation, entryKey) {
+  debug('Clear revalidation control flags - %s', entryKey)
   revalidation.local.remove(entryKey)
   return revalidation.external.remove(entryKey)
 }
@@ -110,14 +132,13 @@ function clearAllRevalidationFlags (revalidation, entryKey) {
  * @returns {Promise<RevalidationState>} Object with revalidation state
  */
 function getRevalidationState (revalidation, entryKey) {
-  return revalidation.external
-    .exists(entryKey)
-    .then(hasExternalEntryExpired => {
-      return {
-        hasExternalEntryExpired,
-        isRevalidatingLocally: () => revalidation.local.exists(entryKey)
-      }
-    })
+  return revalidation.external.exists(entryKey).then(externalEntryExists => {
+    const hasExternalEntryExpired = externalEntryExists === false
+    return {
+      hasExternalEntryExpired,
+      isRevalidatingLocally: () => revalidation.local.exists(entryKey)
+    }
+  })
 }
 
 /**


Test patch:
diff --git a/packages/data-point-service/lib/__snapshots__/cache-middleware.test.js.snap b/packages/data-point-service/lib/__snapshots__/cache-middleware.test.js.snap
index 389861e..76cf0bd 100644
--- a/packages/data-point-service/lib/__snapshots__/cache-middleware.test.js.snap
+++ b/packages/data-point-service/lib/__snapshots__/cache-middleware.test.js.snap
@@ -36,6 +36,11 @@ Array [
     "model:Foo",
     "entryKey",
   ],
+  Array [
+    "Resolve entryKey: %s with entityId: %s",
+    "entryKey",
+    "model:Foo",
+  ],
 ]
 `;
 
diff --git a/packages/data-point-service/lib/__snapshots__/data-point-factory.test.js.snap b/packages/data-point-service/lib/__snapshots__/data-point-factory.test.js.snap
deleted file mode 100644
index c77de97..0000000
--- a/packages/data-point-service/lib/__snapshots__/data-point-factory.test.js.snap
+++ /dev/null
@@ -1,3 +0,0 @@
-// Jest Snapshot v1, https://goo.gl/fbAQLP
-
-exports[`verify It should throw error if entities not provided 1`] = `"DataPoint options.entities should not be Empty"`;
diff --git a/packages/data-point-service/lib/cache-middleware.test.js b/packages/data-point-service/lib/cache-middleware.test.js
index 79fa548..c93d2cd 100644
--- a/packages/data-point-service/lib/cache-middleware.test.js
+++ b/packages/data-point-service/lib/cache-middleware.test.js
@@ -196,7 +196,7 @@ describe('revalidateEntry', () => {
       .spyOn(CacheMiddleware, 'catchRevalidateError')
       .mockImplementation(() => () => Promise.resolve('catchRevalidateError'))
 
-    mocks.addRevalidationFlags = jest.fn(() => () => Promise.resolve('flags'))
+    mocks.addRevalidationFlags = jest.fn(() => Promise.resolve('flags'))
     mocks.service = {}
     _.set(
       mocks.service,
@@ -268,7 +268,7 @@ describe('revalidateEntry', () => {
       const resolveFromAccumulatorArgs =
         mocks.resolveFromAccumulator.mock.calls[0]
 
-      expect(resolveFromAccumulatorArgs[0]).toEqual('model:Foo')
+      expect(resolveFromAccumulatorArgs[0]).toEqual(ctx.context)
       expect(resolveFromAccumulatorArgs[1]).toHaveProperty(
         'locals.revalidatingCache',
         {
diff --git a/packages/data-point-service/lib/data-point-factory.test.js b/packages/data-point-service/lib/data-point-factory.test.js
index 88bd24b..4506868 100644
--- a/packages/data-point-service/lib/data-point-factory.test.js
+++ b/packages/data-point-service/lib/data-point-factory.test.js
@@ -23,14 +23,6 @@ describe('verify', () => {
       })
     }).toThrowError(/provided/)
   })
-
-  test('It should throw error if entities not provided', () => {
-    expect(() => {
-      DataPointFactory.verify({
-        DataPoint
-      })
-    }).toThrowErrorMatchingSnapshot()
-  })
 })
 
 describe('createDataPoint', () => {
diff --git a/packages/data-point-service/lib/stale-while-revalidate.test.js b/packages/data-point-service/lib/stale-while-revalidate.test.js
index e770601..d614739 100644
--- a/packages/data-point-service/lib/stale-while-revalidate.test.js
+++ b/packages/data-point-service/lib/stale-while-revalidate.test.js
@@ -187,14 +187,29 @@ describe('getRevalidationState', () => {
     _.set(
       revalidation,
       'external.exists',
-      jest.fn(() => Promise.resolve('externalState'))
+      jest.fn(() => Promise.resolve(false))
     )
 
     return StaleWhileRevalidate.getRevalidationState(
       revalidation,
       'entryKey'
     ).then(result => {
-      expect(result.hasExternalEntryExpired).toEqual('externalState')
+      expect(result.hasExternalEntryExpired).toEqual(true)
+      expect(result.isRevalidatingLocally).toBeInstanceOf(Function)
+      expect(result.isRevalidatingLocally()).toEqual('localState')
+    })
+  })
+
+  it('should return hasExternalEntryExpired === true if exists == false', () => {
+    const revalidation = {}
+    _.set(revalidation, 'local.exists', jest.fn(() => 'localState'))
+    _.set(revalidation, 'external.exists', jest.fn(() => Promise.resolve(true)))
+
+    return StaleWhileRevalidate.getRevalidationState(
+      revalidation,
+      'entryKey'
+    ).then(result => {
+      expect(result.hasExternalEntryExpired).toEqual(false)
       expect(result.isRevalidatingLocally).toBeInstanceOf(Function)
       expect(result.isRevalidatingLocally()).toEqual('localState')
     })