Repository: ViacomInc/data-point
Pull request #279 (2018-05-04 02:28:29)

feat(data-point-service): consolidate params.cache
<!--
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**:

re: #276

params.[ttl,cacheKey,staleWhileRevalidate] are now consolidated through params.cache, backward
compatibility is provided with a deprecation warning

<!-- 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
- [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 -->


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

```diff
@@          Coverage Diff          @@
##           master   #279   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         128    128           
  Lines        1764   1772    +8     
  Branches      157    158    +1     
=====================================
+ Hits         1764   1772    +8
```


| [Impacted Files](https://codecov.io/gh/ViacomInc/data-point/pull/279?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ackages/data-point-service/lib/cache-middleware.js](https://codecov.io/gh/ViacomInc/data-point/pull/279/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/279?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/279?src=pr&el=footer). Last update [21429f3...5068b73](https://codecov.io/gh/ViacomInc/data-point/pull/279?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/packages/data-point-service/README.md b/packages/data-point-service/README.md
index ddea762..3bce671 100644
--- a/packages/data-point-service/README.md
+++ b/packages/data-point-service/README.md
@@ -89,22 +89,24 @@ createService()
   .then(server)
 ```
 
-## <a name="entity-params-cache">Configure entity.params cache</a>
+## <a name="entity-params-cache">Configure entity's cache settings</a>
 
 To configure an entity's cache settings you must set cache configuration through the params object.
 
 ```js
 '<type>:<entity-name>': {
   params: {
-    ttl: String,
-    staleWhileRevalidate: Boolean
+    cache: {
+      ttl: String,
+      staleWhileRevalidate: Boolean
+    }
   }
 }
 ```
 
-### ttl
+### cache.ttl
 
-Use `ttl` to set an entity's cache entry **Time To Live** value. This value is expected to be written as a string following the format supported by [ms](https://www.npmjs.com/package/ms).
+Use `cache.ttl` to set an entity's cache entry **Time To Live** value. This value is expected to be written as a string following the format supported by [ms](https://www.npmjs.com/package/ms).
 
 **Example:**
 
@@ -117,16 +119,18 @@ DataPointService.create({
     'request:getPlanets': {
       url: 'https://swapi.co/api/planets/'
       params: {
-        ttl: '20m', // 20 minutes
+        cache: {
+          ttl: '20m', // 20 minutes
+        }
       }
     }
   }
 })
 ```
 
-### staleWhileRevalidate
+### cache.staleWhileRevalidate
 
-Use `staleWhileRevalidate` flag in conjunction with a valid `ttl` to use the Stale While Revalidate caching pattern. When this flag is set to `true` your entity will be resolved to a stale value until the ttl expires, revalidation happens in the background and is triggered only upon the key being requested and its ttl being expired.
+Use `cache.staleWhileRevalidate` flag in conjunction with a valid `cache.ttl` to use the Stale While Revalidate caching pattern. When this flag is set to `true` your entity will be resolved to a stale value until the ttl expires, revalidation happens in the background and is triggered only upon the key being requested and its ttl being expired.
 
 **Example:**
 
@@ -139,8 +143,10 @@ DataPointService.create({
     'request:getPlanets': {
       url: 'https://swapi.co/api/planets/'
       params: {
-        ttl: '20m', // 20 minutes
-        staleWhileRevalidate: true
+        cache: {
+          ttl: '20m', // 20 minutes
+          staleWhileRevalidate: true
+        }
       }
     }
   }
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.js
index f3da782..d92f830 100644
--- a/packages/data-point-service/examples/express-cache-stale-while-revalidate.js
+++ b/packages/data-point-service/examples/express-cache-stale-while-revalidate.js
@@ -44,8 +44,10 @@ function createService () {
           return `Hello ${person}!!`
         },
         params: {
-          ttl: '10s',
-          staleWhileRevalidate: true
+          cache: {
+            ttl: '10s',
+            staleWhileRevalidate: true
+          }
         }
       }
     }
diff --git a/packages/data-point-service/examples/express-simple-cache.js b/packages/data-point-service/examples/express-simple-cache.js
index 323e675..76c45a6 100644
--- a/packages/data-point-service/examples/express-simple-cache.js
+++ b/packages/data-point-service/examples/express-simple-cache.js
@@ -44,7 +44,9 @@ function createService () {
           return `Hello ${person}!!`
         },
         params: {
-          ttl: '10s'
+          cache: {
+            ttl: '10s'
+          }
         }
       }
     }
diff --git a/packages/data-point-service/lib/cache-middleware.js b/packages/data-point-service/lib/cache-middleware.js
index 1e91a52..4f3f61f 100644
--- a/packages/data-point-service/lib/cache-middleware.js
+++ b/packages/data-point-service/lib/cache-middleware.js
@@ -1,16 +1,16 @@
 const Promise = require('bluebird')
+const defaultTo = require('lodash/defaultTo')
 const set = require('lodash/fp/set')
 const logger = require('./logger')
+const { deprecate } = require('util')
 
 /**
- * @param {String} prefix cache ky prefix
+ * @param {Function} cacheKey function to generate a cache key
  * @param {DataPoint.Accumulator} ctx DataPoint Accumulator object
  * @returns {String} generated cache key
  */
-function generateKey (ctx) {
-  return ctx.context.params.cacheKey
-    ? ctx.context.params.cacheKey(ctx)
-    : `entity:${ctx.context.id}`
+function generateKey (cacheKey, ctx) {
+  return cacheKey ? cacheKey(ctx) : `entity:${ctx.context.id}`
 }
 
 /**
@@ -234,27 +234,57 @@ function resolveStaleWhileRevalidateEntry (service, entryKey, ttl, ctx) {
   })
 }
 
+const looseCacheParamsDeprecationWarning = deprecate(() => {},
+'Usage of params.ttl, params.cacheKey and params.staleWhileRevalidate will be deprecated. Please configure through params.cache object instead')
+
+/**
+ * Logs deprecation warning if loose cache params were used
+ * @param {Object} params entity's custom params
+ */
+function warnLooseParamsCacheDeprecation (params) {
+  if (params.ttl || params.cacheKey || params.staleWhileRevalidate) {
+    module.exports.looseCacheParamsDeprecationWarning()
+  }
+}
+
+/**
+ * @param {Object} params entity's custom params
+ * @returns {Object} normalized values
+ */
+function getCacheParams (params) {
+  warnLooseParamsCacheDeprecation(params)
+  const cache = defaultTo(params.cache, {})
+  return {
+    ttl: defaultTo(cache.ttl, params.ttl),
+    cacheKey: defaultTo(cache.cacheKey, params.cacheKey),
+    staleWhileRevalidate: defaultTo(
+      cache.staleWhileRevalidate,
+      params.staleWhileRevalidate
+    )
+  }
+}
+
 /**
  * @param {Service} service Service instance
  * @param {DataPoint.Accumulator} ctx DataPoint Accumulator object
  * @param {Function} next Middleware callback
  */
 function before (service, ctx, next) {
-  const { ttl, staleWhileRevalidate } = ctx.context.params
+  const cache = getCacheParams(ctx.context.params)
 
-  if (!ttl || ctx.locals.resetCache === true) {
+  if (!cache.ttl || ctx.locals.resetCache === true) {
     return next()
   }
 
-  const entryKey = generateKey(ctx)
+  const entryKey = generateKey(cache.cacheKey, ctx)
 
-  Promise.resolve(staleWhileRevalidate)
+  Promise.resolve(cache.staleWhileRevalidate)
     .then(isStaleWhileRevalidate => {
       return isStaleWhileRevalidate
         ? module.exports.resolveStaleWhileRevalidateEntry(
             service,
             entryKey,
-            ttl,
+            cache.ttl,
             ctx
           )
         : getEntry(service, entryKey)
@@ -277,17 +307,17 @@ function before (service, ctx, next) {
  * @param {Function} next Middleware callback
  */
 function after (service, ctx, next) {
-  const { ttl, staleWhileRevalidate } = ctx.context.params
+  const cache = getCacheParams(ctx.context.params)
 
-  if (!ttl) {
+  if (!cache.ttl) {
     // do nothing
     return next()
   }
 
   // from here below ttl is assumed to be true
-  const entryKey = generateKey(ctx)
+  const entryKey = generateKey(cache.cacheKey, ctx)
 
-  if (staleWhileRevalidate) {
+  if (cache.staleWhileRevalidate) {
     // if its at the process of revalidating, then lets skip any further calls
     if (ctx.locals.revalidatingCache) {
       return next()
@@ -295,14 +325,14 @@ function after (service, ctx, next) {
 
     // adds (or updates) the stale cache entry with the latest value
     return module.exports
-      .setStaleWhileRevalidateEntry(service, entryKey, ctx.value, ttl)
+      .setStaleWhileRevalidateEntry(service, entryKey, ctx.value, cache.ttl)
       .then(() => {
         next()
       })
   }
 
   // adds a cache entry
-  return setEntry(service, entryKey, ctx.value, ttl).then(() => next())
+  return setEntry(service, entryKey, ctx.value, cache.ttl).then(() => next())
 }
 
 module.exports = {
@@ -317,6 +347,9 @@ module.exports = {
   resolveStaleWhileRevalidateEntry,
   revalidateEntry,
   setStaleWhileRevalidateEntry,
+  looseCacheParamsDeprecationWarning,
+  warnLooseParamsCacheDeprecation,
+  getCacheParams,
   before,
   after
 }


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 0e86eee..816fb39 100644
--- a/packages/data-point-service/lib/cache-middleware.test.js
+++ b/packages/data-point-service/lib/cache-middleware.test.js
@@ -1,7 +1,10 @@
 /* eslint-env jest */
 
-const CacheMiddleware = require('./cache-middleware')
 const winston = require('winston')
+const util = require('util')
+util.deprecate = jest.fn(fn => fn)
+
+const CacheMiddleware = require('./cache-middleware')
 
 function createContext () {
   const ctx = {
@@ -42,18 +45,18 @@ function createMocks () {
 describe('genrateKey', () => {
   it('should generate default id', () => {
     const ctx = createContext()
-    const result = CacheMiddleware.generateKey(ctx)
+    const result = CacheMiddleware.generateKey(null, ctx)
     expect(result).toEqual('entity:model:Foo')
   })
 
   it('should generate a key using cacheKey parameter', () => {
     const ctx = createContext()
 
-    ctx.context.params.cacheKey = acc => {
+    const cacheKey = acc => {
       return `custom:${acc.context.id}`
     }
 
-    const result = CacheMiddleware.generateKey(ctx)
+    const result = CacheMiddleware.generateKey(cacheKey, ctx)
     expect(result).toEqual('custom:model:Foo')
   })
 })
@@ -387,6 +390,97 @@ describe('resolveStaleWhileRevalidateEntry', () => {
   })
 })
 
+describe('warnLooseParamsCacheDeprecation', () => {
+  const looseCacheParamsDeprecationWarning =
+    CacheMiddleware.looseCacheParamsDeprecationWarning
+  afterAll(() => {
+    CacheMiddleware.looseCacheParamsDeprecationWarning = looseCacheParamsDeprecationWarning
+  })
+  it('should call deprecate if params.ttl is set', () => {
+    CacheMiddleware.looseCacheParamsDeprecationWarning = jest.fn()
+    expect(
+      CacheMiddleware.warnLooseParamsCacheDeprecation({
+        ttl: true
+      })
+    )
+    expect(CacheMiddleware.looseCacheParamsDeprecationWarning).toBeCalled()
+  })
+
+  it('should call deprecate if params.cacheKey is set', () => {
+    CacheMiddleware.looseCacheParamsDeprecationWarning = jest.fn()
+
+    expect(
+      CacheMiddleware.warnLooseParamsCacheDeprecation({
+        cacheKey: true
+      })
+    )
+
+    expect(CacheMiddleware.looseCacheParamsDeprecationWarning).toBeCalled()
+  })
+
+  it('should call deprecate if params.staleWhileRevalidate is set', () => {
+    CacheMiddleware.looseCacheParamsDeprecationWarning = jest.fn()
+
+    expect(
+      CacheMiddleware.warnLooseParamsCacheDeprecation({
+        staleWhileRevalidate: true
+      })
+    )
+
+    expect(CacheMiddleware.looseCacheParamsDeprecationWarning).toBeCalled()
+  })
+})
+
+describe('getCacheParams', () => {
+  it('should have backwards compatability with params loose properties', () => {
+    const params = {
+      ttl: '20s',
+      cacheKey: () => true,
+      staleWhileRevalidate: () => true
+    }
+    const cache = CacheMiddleware.getCacheParams(params)
+    expect(cache).toEqual({
+      ttl: params.ttl,
+      cacheKey: params.cacheKey,
+      staleWhileRevalidate: params.staleWhileRevalidate
+    })
+  })
+  it('should have get values from params.cache property', () => {
+    const params = {
+      cache: {
+        ttl: '20s',
+        cacheKey: () => true,
+        staleWhileRevalidate: () => true
+      }
+    }
+    const cache = CacheMiddleware.getCacheParams(params)
+    expect(cache).toEqual({
+      ttl: params.cache.ttl,
+      cacheKey: params.cache.cacheKey,
+      staleWhileRevalidate: params.cache.staleWhileRevalidate
+    })
+  })
+  it('should have params.cache priority over loose param cache settings', () => {
+    const params = {
+      ttl: '10s',
+      cacheKey: () => true,
+      staleWhileRevalidate: () => true,
+      cache: {
+        ttl: '20s',
+        cacheKey: () => true
+      }
+    }
+    const cache = CacheMiddleware.getCacheParams(params)
+    expect(cache).toEqual({
+      ttl: params.cache.ttl,
+      cacheKey: params.cache.cacheKey,
+      // this key will use the loose value since its not being set through
+      // params.cache
+      staleWhileRevalidate: params.staleWhileRevalidate
+    })
+  })
+})
+
 describe('before', () => {
   it('should call next if no  ttl', () => {
     const mocks = createMocks()