Repository: ViacomInc/data-point
Pull request #282 (2018-05-10 03:33:51)

fix(data-point-service): fix revalidation logic and key creation
it now checks for revalidation cache key vs current cache key to prevent multiple revalidations

<!--
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**:

im sorry :) two bugs included in one single PR

- fixes revalidation logic
- changes key generation prefix to postfix

<!-- Why are these changes necessary? -->
**Why**:

- revalidation logic was not checking against being the same cache key, so it would essentially re-revalidate every single entity
- stale-while-revalidate key prefixes were not helping with the taxonomy of the key

<!-- How were these changes implemented? -->
**How**:

- we check against the cache key being revalidated and the current one, only if it matches
- moving the stale-while-revalidate key from prefix to postfix allows us to filter by anything preceding it

<!-- 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
- [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 -->


codecov[bot]: # [Codecov](https://codecov.io/gh/ViacomInc/data-point/pull/282?src=pr&el=h1) Report
> Merging [#282](https://codecov.io/gh/ViacomInc/data-point/pull/282?src=pr&el=desc) into [master](https://codecov.io/gh/ViacomInc/data-point/commit/56a73d7556214ded08da9e25f30c531d7d79c5f3?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/282/graphs/tree.svg?height=150&width=650&token=LYxItnrx4i&src=pr)](https://codecov.io/gh/ViacomInc/data-point/pull/282?src=pr&el=tree)

```diff
@@          Coverage Diff          @@
##           master   #282   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         128    128           
  Lines        1782   1784    +2     
  Branches      162    163    +1     
=====================================
+ Hits         1782   1784    +2
```


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

------

[Continue to review full report at Codecov](https://codecov.io/gh/ViacomInc/data-point/pull/282?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/282?src=pr&el=footer). Last update [56a73d7...a2acd7d](https://codecov.io/gh/ViacomInc/data-point/pull/282?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 557b056..cd5a998 100644
--- a/packages/data-point-service/lib/cache-middleware.js
+++ b/packages/data-point-service/lib/cache-middleware.js
@@ -5,6 +5,8 @@ const logger = require('./logger')
 const { deprecate } = require('util')
 const ms = require('ms')
 
+const SWR_CONTROL = 'SWR-CONTROL'
+
 /**
  * @param {Function} cacheKey function to generate a cache key
  * @param {DataPoint.Accumulator} ctx DataPoint Accumulator object
@@ -16,18 +18,18 @@ function generateKey (cacheKey, ctx) {
 
 /**
  * @param {String} key cache key
- * @returns {String} key prefixed with "SWI-STALE"
+ * @returns {String} key postfixed with "swr.stale"
  */
-function createSWIStaleKey (key) {
-  return `SWI-STALE:${key}`
+function createSWRStaleKey (key) {
+  return `${key}:swr.stale`
 }
 
 /**
  * @param {String} key cache key
- * @returns {String} key prefixed with "SWI-CONTROL"
+ * @returns {String} key postfixed with "swr.control"
  */
-function createSWIControlKey (key) {
-  return `SWI-CONTROL:${key}`
+function createSWRControlKey (key) {
+  return `${key}:swr.control`
 }
 
 /**
@@ -44,8 +46,8 @@ function getEntry (service, key) {
  * @param {String} key entry key
  * @returns {Promise<Object|undefined>} entry value
  */
-function getSWIStaleEntry (service, key) {
-  return service.cache.get(createSWIStaleKey(key))
+function getSWRStaleEntry (service, key) {
+  return service.cache.get(createSWRStaleKey(key))
 }
 
 /**
@@ -53,9 +55,9 @@ function getSWIStaleEntry (service, key) {
  * @param {String} key entry key
  * @returns {Promise<Object|undefined>} entry value
  */
-function getSWIControlEntry (service, key) {
-  return getEntry(service, createSWIControlKey(key)).then(
-    result => result === 'SWI-CONTROL'
+function getSWRControlEntry (service, key) {
+  return getEntry(service, createSWRControlKey(key)).then(
+    result => result === SWR_CONTROL
   )
 }
 
@@ -78,8 +80,8 @@ function setEntry (service, key, value, ttl) {
  * @param {Number|String} ttl time to live value supported by https://github.com/zeit/ms
  * @returns {Promise}
  */
-function setSWIStaleEntry (service, key, value, ttl) {
-  return setEntry(service, createSWIStaleKey(key), value, ttl)
+function setSWRStaleEntry (service, key, value, ttl) {
+  return setEntry(service, createSWRStaleKey(key), value, ttl)
 }
 
 /**
@@ -88,8 +90,8 @@ function setSWIStaleEntry (service, key, value, ttl) {
  * @param {String} ttl time to live value supported by https://github.com/zeit/ms
  * @returns {Promise}
  */
-function setSWIControlEntry (service, key, ttl) {
-  return setEntry(service, createSWIControlKey(key), 'SWI-CONTROL', ttl)
+function setSWRControlEntry (service, key, ttl) {
+  return setEntry(service, createSWRControlKey(key), SWR_CONTROL, ttl)
 }
 
 /**
@@ -100,12 +102,12 @@ function setSWIControlEntry (service, key, ttl) {
  * @returns {Promise}
  */
 function setStaleWhileRevalidateEntry (service, entryKey, value, cache) {
-  return setSWIStaleEntry(
+  return setSWRStaleEntry(
     service,
     entryKey,
     value,
     cache.staleWhileRevalidateTtl
-  ).then(() => setSWIControlEntry(service, entryKey, cache.ttl))
+  ).then(() => setSWRControlEntry(service, entryKey, cache.ttl))
 }
 
 /**
@@ -130,7 +132,7 @@ function revalidateSuccess (entityId, entryKey) {
  * @param {Object} cache cache configuration
  * @returns {Function}
  */
-function updateSWIEntry (service, entryKey, cache) {
+function updateSWREntry (service, entryKey, cache) {
   /**
    * @param {Accumulator} acc
    */
@@ -197,11 +199,22 @@ function revalidateEntry (service, entryKey, cache, ctx) {
 
   return service.dataPoint
     .resolveFromAccumulator(entityId, revalidateContext)
-    .then(updateSWIEntry(service, entryKey, cache))
+    .then(updateSWREntry(service, entryKey, cache))
     .then(revalidateSuccess(entityId, entryKey))
     .catch(handleRevalidateError(entityId, entryKey))
 }
 
+/**
+ * Checks if the current entry key is the one being revalidated
+ * @param {DataPoint.Accumulator} ctx DataPoint Accumulator object
+ * @param {String} currentEntryKey entry key
+ * @returns {Boolean} true if revalidating entryKey matches current key
+ */
+function isRevalidatingCacheKey (ctx, currentEntryKey) {
+  const revalidatingCache = ctx.locals.revalidatingCache
+  return (revalidatingCache && revalidatingCache.entryKey) === currentEntryKey
+}
+
 /**
  * If stale entry exists and control entry has expired it will fire a new thread
  * to resolve the entity, this thread is not meant to be chained to the main
@@ -214,16 +227,17 @@ function revalidateEntry (service, entryKey, cache, ctx) {
  * @returns {Promise<Object|undefined>} cached stale value
  */
 function resolveStaleWhileRevalidateEntry (service, entryKey, cache, ctx) {
-  const revalidatingCache = ctx.locals.revalidatingCache
-
-  if (revalidatingCache) {
-    // bypass the rest so entity gets resolved
+  // IMPORTANT: we only want to bypass an entity that is being revalidated and
+  // that matches the same cache entry key, otherwise all child entities will
+  // be needlesly resolved
+  if (isRevalidatingCacheKey(ctx, entryKey)) {
+    // bypass the rest forces entity to get resolved
     return undefined
   }
 
   const tasks = [
-    getSWIControlEntry(service, entryKey),
-    getSWIStaleEntry(service, entryKey)
+    getSWRControlEntry(service, entryKey),
+    getSWRStaleEntry(service, entryKey)
   ]
 
   return Promise.all(tasks).then(results => {
@@ -398,13 +412,14 @@ function after (service, ctx, next) {
 
 module.exports = {
   generateKey,
-  createSWIStaleKey,
-  createSWIControlKey,
+  createSWRStaleKey,
+  createSWRControlKey,
   getEntry,
-  getSWIControlEntry,
+  getSWRControlEntry,
   setEntry,
-  setSWIStaleEntry,
-  setSWIControlEntry,
+  setSWRStaleEntry,
+  setSWRControlEntry,
+  isRevalidatingCacheKey,
   resolveStaleWhileRevalidateEntry,
   revalidateEntry,
   setStaleWhileRevalidateEntry,


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 2c171da..24068f5 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
@@ -30,13 +30,13 @@ Array [
 exports[`revalidateEntry should set cache 1`] = `
 Array [
   Array [
-    "SWI-STALE:key",
+    "key:swr.stale",
     undefined,
     400,
   ],
   Array [
-    "SWI-CONTROL:key",
-    "SWI-CONTROL",
+    "key:swr.control",
+    "SWR-CONTROL",
     200,
   ],
 ]
diff --git a/packages/data-point-service/lib/cache-middleware.test.js b/packages/data-point-service/lib/cache-middleware.test.js
index 073928a..741ded5 100644
--- a/packages/data-point-service/lib/cache-middleware.test.js
+++ b/packages/data-point-service/lib/cache-middleware.test.js
@@ -62,16 +62,16 @@ describe('genrateKey', () => {
   })
 })
 
-describe('createSWIStaleKey', () => {
+describe('createSWRStaleKey', () => {
   it('should create stale key', () => {
-    expect(CacheMiddleware.createSWIStaleKey('key')).toEqual('SWI-STALE:key')
+    expect(CacheMiddleware.createSWRStaleKey('key')).toEqual('key:swr.stale')
   })
 })
 
-describe('createSWIStaleKey', () => {
+describe('createSWRStaleKey', () => {
   it('should create control key', () => {
-    expect(CacheMiddleware.createSWIControlKey('key')).toEqual(
-      'SWI-CONTROL:key'
+    expect(CacheMiddleware.createSWRControlKey('key')).toEqual(
+      'key:swr.control'
     )
   })
 })
@@ -89,15 +89,15 @@ describe('getEntry', () => {
   })
 })
 
-describe('getSWIControlEntry', () => {
+describe('getSWRControlEntry', () => {
   it('should return true if key exists with valid wrapper ', () => {
     const service = {
       cache: {
-        get: jest.fn(key => Promise.resolve('SWI-CONTROL'))
+        get: jest.fn(key => Promise.resolve('SWR-CONTROL'))
       }
     }
-    return CacheMiddleware.getSWIControlEntry(service, 'test').then(result => {
-      expect(service.cache.get).toBeCalledWith('SWI-CONTROL:test')
+    return CacheMiddleware.getSWRControlEntry(service, 'test').then(result => {
+      expect(service.cache.get).toBeCalledWith('test:swr.control')
       expect(result).toEqual(true)
     })
   })
@@ -108,8 +108,8 @@ describe('getSWIControlEntry', () => {
         get: jest.fn(key => Promise.resolve(undefined))
       }
     }
-    return CacheMiddleware.getSWIControlEntry(service, 'test').then(result => {
-      expect(service.cache.get).toBeCalledWith('SWI-CONTROL:test')
+    return CacheMiddleware.getSWRControlEntry(service, 'test').then(result => {
+      expect(service.cache.get).toBeCalledWith('test:swr.control')
       expect(result).toEqual(false)
     })
   })
@@ -127,52 +127,52 @@ describe('setEntry', () => {
   })
 })
 
-describe('setSWIStaleEntry', () => {
+describe('setSWRStaleEntry', () => {
   it('should create a key that has no ttl', () => {
     const service = {
       cache: {
         set: jest.fn()
       }
     }
-    CacheMiddleware.setSWIStaleEntry(service, 'key', 'value', 100)
-    expect(service.cache.set).toBeCalledWith('SWI-STALE:key', 'value', 100)
+    CacheMiddleware.setSWRStaleEntry(service, 'key', 'value', 100)
+    expect(service.cache.set).toBeCalledWith('key:swr.stale', 'value', 100)
   })
 })
 
-describe('setSWIControlEntry', () => {
+describe('setSWRControlEntry', () => {
   it('should create a key that has no ttl', () => {
     const service = {
       cache: {
         set: jest.fn()
       }
     }
-    CacheMiddleware.setSWIControlEntry(service, 'key', 100)
+    CacheMiddleware.setSWRControlEntry(service, 'key', 100)
     expect(service.cache.set).toBeCalledWith(
-      'SWI-CONTROL:key',
-      'SWI-CONTROL',
+      'key:swr.control',
+      'SWR-CONTROL',
       100
     )
   })
 })
 
-describe('setSWIControlEntry', () => {
+describe('setSWRControlEntry', () => {
   it('should create a key that has no ttl', () => {
     const service = {
       cache: {
         set: jest.fn()
       }
     }
-    CacheMiddleware.setSWIControlEntry(service, 'key', 200)
+    CacheMiddleware.setSWRControlEntry(service, 'key', 200)
     expect(service.cache.set).toBeCalledWith(
-      'SWI-CONTROL:key',
-      'SWI-CONTROL',
+      'key:swr.control',
+      'SWR-CONTROL',
       200
     )
   })
 })
 
 describe('setStaleWhileRevalidateEntry', () => {
-  it('should store stale entry and SWI control entry', () => {
+  it('should store stale entry and SWR control entry', () => {
     const set = jest.fn(() => Promise.resolve(true))
     const service = {
       cache: {
@@ -191,8 +191,8 @@ describe('setStaleWhileRevalidateEntry', () => {
       'value',
       cache
     ).then(() => {
-      expect(set.mock.calls[0]).toEqual(['SWI-STALE:key', 'value', 400])
-      expect(set.mock.calls[1]).toEqual(['SWI-CONTROL:key', 'SWI-CONTROL', 200])
+      expect(set.mock.calls[0]).toEqual(['key:swr.stale', 'value', 400])
+      expect(set.mock.calls[1]).toEqual(['key:swr.control', 'SWR-CONTROL', 200])
     })
   })
 })
@@ -287,6 +287,28 @@ describe('revalidateEntry', () => {
   })
 })
 
+describe('isRevalidatingCacheKey', () => {
+  it('should return false if revalidating property does not exists', () => {
+    const ctx = createContext()
+    ctx.locals.revalidatingCache = undefined
+    expect(CacheMiddleware.isRevalidatingCacheKey(ctx, 'key')).toEqual(false)
+  })
+  it('should return false if revalidating key does not match current key', () => {
+    const ctx = createContext()
+    ctx.locals.revalidatingCache = {
+      entryKey: 'otherKey'
+    }
+    expect(CacheMiddleware.isRevalidatingCacheKey(ctx, 'key')).toEqual(false)
+  })
+  it('should return true if revalidating key matches current key', () => {
+    const ctx = createContext()
+    ctx.locals.revalidatingCache = {
+      entryKey: 'key'
+    }
+    expect(CacheMiddleware.isRevalidatingCacheKey(ctx, 'key')).toEqual(true)
+  })
+})
+
 describe('resolveStaleWhileRevalidateEntry', () => {
   let logDebug
   let logError
@@ -310,7 +332,23 @@ describe('resolveStaleWhileRevalidateEntry', () => {
 
   it('should exit if revalidation flag is set', () => {
     const mocks = createMocks()
-    mocks.ctx.locals.revalidatingCache = true
+    mocks.ctx.locals.revalidatingCache = {
+      entryKey: 'key'
+    }
+    const result = CacheMiddleware.resolveStaleWhileRevalidateEntry(
+      mocks.service,
+      'key',
+      200,
+      mocks.ctx
+    )
+    expect(result).toBeUndefined()
+  })
+
+  it('should return undefined if revalidation flag is set', () => {
+    const mocks = createMocks()
+    mocks.ctx.locals.revalidatingCache = {
+      entryKey: 'key'
+    }
     const result = CacheMiddleware.resolveStaleWhileRevalidateEntry(
       mocks.service,
       'key',
@@ -324,12 +362,12 @@ describe('resolveStaleWhileRevalidateEntry', () => {
     const mocks = createMocks()
     mocks.service.cache.get = jest.fn(key => {
       return Promise.resolve().then(() => {
-        if (key === 'SWI-STALE:key') {
+        if (key === 'key:swr.stale') {
           return 'STALE'
         }
-        // this forces getSWIControlEntry to return true
-        if (key === 'SWI-CONTROL:key') {
-          return 'SWI-CONTROL'
+        // this forces getSWRControlEntry to return true
+        if (key === 'key:swr.control') {
+          return 'SWR-CONTROL'
         }
       })
     })
@@ -349,13 +387,13 @@ describe('resolveStaleWhileRevalidateEntry', () => {
     const spyRevalidateEntry = jest.spyOn(CacheMiddleware, 'revalidateEntry')
     mocks.service.cache.get = jest.fn(key => {
       return Promise.resolve().then(() => {
-        if (key === 'SWI-STALE:key') {
+        if (key === 'key:swr.stale') {
           return 'STALE'
         }
-        // this forces getSWIControlEntry to return true, simulating that key's ttl
+        // this forces getSWRControlEntry to return true, simulating that key's ttl
         // has not expired
-        if (key === 'SWI-CONTROL:key') {
-          return 'SWI-CONTROL'
+        if (key === 'key:swr.control') {
+          return 'SWR-CONTROL'
         }
       })
     })
@@ -378,12 +416,12 @@ describe('resolveStaleWhileRevalidateEntry', () => {
     spyRevalidateEntry.mockImplementation(() => true)
     mocks.service.cache.get = jest.fn(key => {
       return Promise.resolve().then(() => {
-        if (key === 'SWI-STALE:key') {
+        if (key === 'key:swr.stale') {
           return 'STALE'
         }
-        // this forces getSWIControlEntry to return false, simulating that key's ttl
+        // this forces getSWRControlEntry to return false, simulating that key's ttl
         // has now expired
-        if (key === 'SWI-CONTROL:key') {
+        if (key === 'key:swr.control') {
           return undefined
         }
       })