Repository: ViacomInc/data-point
Pull request #270 (2018-05-03 13:34:06)

feat(data-point-service): deprecates cache.prefix
use cache.redis.keyPrefix instead

BREAKING CHANGE: cache.prefix is no longer used, if set; an error is thrown to alert the user of the
change

<!--
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**:

deprecated `cache.prefix` in favor or `cache.redis.keyPrefix`

<!-- Why are these changes necessary? -->
**Why**:

There were two places where this was possible to be set, and the cache.prefix was not the right approach, prefixing should be delegated to native ioredis options.

<!-- How were these changes implemented? -->
**How**:

Removed the feature, added a check if the value is set, then throw an error to alert the user of the change.

<!-- 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] 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 -->


acatl: FYI I will be publishing a new release but will change to release strategy to independent, so only packages with changes get published
codecov[bot]: # [Codecov](https://codecov.io/gh/ViacomInc/data-point/pull/270?src=pr&el=h1) Report
> Merging [#270](https://codecov.io/gh/ViacomInc/data-point/pull/270?src=pr&el=desc) into [master](https://codecov.io/gh/ViacomInc/data-point/commit/ce127f35f30958b0728eb6f96e1d44c646376faf?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/270/graphs/tree.svg?height=150&width=650&src=pr&token=LYxItnrx4i)](https://codecov.io/gh/ViacomInc/data-point/pull/270?src=pr&el=tree)

```diff
@@          Coverage Diff          @@
##           master   #270   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         128    128           
  Lines        1756   1763    +7     
  Branches      154    157    +3     
=====================================
+ Hits         1756   1763    +7
```


| [Impacted Files](https://codecov.io/gh/ViacomInc/data-point/pull/270?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [packages/data-point-service/lib/factory.js](https://codecov.io/gh/ViacomInc/data-point/pull/270/diff?src=pr&el=tree#diff-cGFja2FnZXMvZGF0YS1wb2ludC1zZXJ2aWNlL2xpYi9mYWN0b3J5Lmpz) | `100% <100%> (ø)` | :arrow_up: |
| [...ackages/data-point-service/lib/cache-middleware.js](https://codecov.io/gh/ViacomInc/data-point/pull/270/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/270?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/270?src=pr&el=footer). Last update [ce127f3...8d9c2b6](https://codecov.io/gh/ViacomInc/data-point/pull/270?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 eb23135..ddea762 100644
--- a/packages/data-point-service/README.md
+++ b/packages/data-point-service/README.md
@@ -30,8 +30,7 @@ Properties of the `options` argument:
 |:---|:---|:---|
 | **cache** | `Object` | cache specific settings |
 | **cache.isRequired** | `Boolean` | false by default, if true it will throw an error |
-| **cache.prefix** | `string` | by default [os.hostname()](https://nodejs.org/api/os.html#os_os_hostname), this will be the prefix of every cache key added to redis |
-| **cache.redis** | `Object` | redis settings passed to the [IORedis constructor](https://github.com/luin/ioredis/blob/master/API.md#new-redisport-host-options) |
+| **cache.redis** | `Object` | **redis** settings passed to the [ioredis](https://github.com/luin/ioredis/blob/master/API.md#new-redisport-host-options) constructor. For key prefixing, set [keyPrefix](https://github.com/luin/ioredis#transparent-key-prefixing) through `cache.redis.keyPrefix`; if the value is not provided then [os.hostname()](https://nodejs.org/api/os.html#os_os_hostname) will be used. |
 
 This method returns a Promise that resolves to a Service Object. 
 
diff --git a/packages/data-point-service/lib/cache-middleware.js b/packages/data-point-service/lib/cache-middleware.js
index 211f41b..1e91a52 100644
--- a/packages/data-point-service/lib/cache-middleware.js
+++ b/packages/data-point-service/lib/cache-middleware.js
@@ -7,12 +7,10 @@ const logger = require('./logger')
  * @param {DataPoint.Accumulator} ctx DataPoint Accumulator object
  * @returns {String} generated cache key
  */
-function generateKey (prefix, ctx) {
-  const cacheKey = ctx.context.params.cacheKey
+function generateKey (ctx) {
+  return ctx.context.params.cacheKey
     ? ctx.context.params.cacheKey(ctx)
     : `entity:${ctx.context.id}`
-
-  return `${prefix}:${cacheKey}`
 }
 
 /**
@@ -248,7 +246,7 @@ function before (service, ctx, next) {
     return next()
   }
 
-  const entryKey = generateKey(service.cachePrefix, ctx)
+  const entryKey = generateKey(ctx)
 
   Promise.resolve(staleWhileRevalidate)
     .then(isStaleWhileRevalidate => {
@@ -287,7 +285,7 @@ function after (service, ctx, next) {
   }
 
   // from here below ttl is assumed to be true
-  const entryKey = generateKey(service.cachePrefix, ctx)
+  const entryKey = generateKey(ctx)
 
   if (staleWhileRevalidate) {
     // if its at the process of revalidating, then lets skip any further calls
diff --git a/packages/data-point-service/lib/factory.js b/packages/data-point-service/lib/factory.js
index 7d48076..1dc3503 100644
--- a/packages/data-point-service/lib/factory.js
+++ b/packages/data-point-service/lib/factory.js
@@ -8,26 +8,51 @@ const { setupMiddleware } = require('./setup-middleware')
 
 const Cache = require('data-point-cache')
 
-const settingsDefault = {
-  cache: {
-    isRequired: false,
-    prefix: os.hostname()
+/**
+ * @returns {Object} Default module's options
+ */
+function getDefaultSettings () {
+  return {
+    cache: {
+      isRequired: false
+    }
   }
 }
 
-function createServiceObject (options) {
-  const settings = _.merge({}, settingsDefault, options)
+/**
+ * @param {Object} options module's options
+ * @returns {undefined}
+ */
+function prefixDeprecationError (options) {
+  const prefix = _.get(options, 'cache.prefix')
+  if (typeof prefix !== 'undefined') {
+    throw new Error(
+      'options.cache.prefix is now deprecated, please use options.cache.redis.keyPrefix instead.'
+    )
+  }
+}
 
-  const cachePrefix = _.defaultTo(
-    _.get(settings, 'cache.prefix'),
-    os.hostname()
-  )
+/**
+ * @param {Object} settings module's settings object
+ * @returns {String} cache prefix defaults to os.hostname()
+ */
+function getCachePrefix (settings) {
+  const keyPrefix = _.get(settings, 'cache.redis.keyPrefix')
+  const prefix = keyPrefix || os.hostname()
+  const separator = prefix.endsWith(':') ? '' : ':'
+  return `${prefix}${separator}`
+}
+
+function createServiceObject (options) {
+  prefixDeprecationError(options)
 
+  const settings = _.merge({}, getDefaultSettings(), options)
   const isCacheRequired = _.defaultTo(_.get(settings, 'cache.isRequired'), true)
 
+  _.set(settings, 'cache.redis.keyPrefix', getCachePrefix(settings))
+
   return {
     isCacheRequired,
-    cachePrefix,
     settings,
     isCacheAvailable: false,
     cache: null,
@@ -97,6 +122,9 @@ function create (options) {
 }
 
 module.exports = {
+  getDefaultSettings,
+  prefixDeprecationError,
+  getCachePrefix,
   create,
   createServiceObject,
   successCreateCache,


Test patch:
diff --git a/packages/data-point-service/lib/__snapshots__/factory.test.js.snap b/packages/data-point-service/lib/__snapshots__/factory.test.js.snap
new file mode 100644
index 0000000..913969e
--- /dev/null
+++ b/packages/data-point-service/lib/__snapshots__/factory.test.js.snap
@@ -0,0 +1,11 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`getDefaultSettings should return default settings 1`] = `
+Object {
+  "cache": Object {
+    "isRequired": false,
+  },
+}
+`;
+
+exports[`prefixDeprecationError should warn when cache.prefix is set 1`] = `"options.cache.prefix is now deprecated, please use options.cache.redis.keyPrefix instead."`;
diff --git a/packages/data-point-service/lib/cache-middleware.test.js b/packages/data-point-service/lib/cache-middleware.test.js
index 93172a8..0e86eee 100644
--- a/packages/data-point-service/lib/cache-middleware.test.js
+++ b/packages/data-point-service/lib/cache-middleware.test.js
@@ -21,7 +21,6 @@ function createMocks () {
   const set = jest.fn(() => Promise.resolve(true))
   const get = jest.fn(() => Promise.resolve(true))
   const service = {
-    cachePrefix: 'prefix',
     dataPoint: {
       resolveFromAccumulator
     },
@@ -41,10 +40,10 @@ function createMocks () {
 }
 
 describe('genrateKey', () => {
-  it('should generate default id with prefix', () => {
+  it('should generate default id', () => {
     const ctx = createContext()
-    const result = CacheMiddleware.generateKey('prefix', ctx)
-    expect(result).toEqual('prefix:entity:model:Foo')
+    const result = CacheMiddleware.generateKey(ctx)
+    expect(result).toEqual('entity:model:Foo')
   })
 
   it('should generate a key using cacheKey parameter', () => {
@@ -54,8 +53,8 @@ describe('genrateKey', () => {
       return `custom:${acc.context.id}`
     }
 
-    const result = CacheMiddleware.generateKey('prefix', ctx)
-    expect(result).toEqual('prefix:custom:model:Foo')
+    const result = CacheMiddleware.generateKey(ctx)
+    expect(result).toEqual('custom:model:Foo')
   })
 })
 
@@ -419,7 +418,7 @@ describe('before', () => {
     const next = () => {
       expect(spyResolveStaleWhileRevalidateEntry).toHaveBeenCalledWith(
         mocks.service,
-        'prefix:entity:model:Foo',
+        'entity:model:Foo',
         '20m',
         mocks.ctx
       )
@@ -435,9 +434,7 @@ describe('before', () => {
     mocks.service.cache.get = jest.fn()
 
     const next = jest.fn(() => {
-      expect(mocks.service.cache.get).toHaveBeenCalledWith(
-        'prefix:entity:model:Foo'
-      )
+      expect(mocks.service.cache.get).toHaveBeenCalledWith('entity:model:Foo')
       expect(next).toBeCalled()
       done()
     })
@@ -520,7 +517,7 @@ describe('after', () => {
     const next = () => {
       expect(setStaleWhileRevalidateEntry).toBeCalledWith(
         mocks.service,
-        'prefix:entity:model:Foo',
+        'entity:model:Foo',
         'VALUE',
         '20m'
       )
@@ -548,7 +545,7 @@ describe('after', () => {
     const next = () => {
       expect(setStaleWhileRevalidateEntry).not.toBeCalled()
       expect(mocks.service.cache.set).toBeCalledWith(
-        'prefix:entity:model:Foo',
+        'entity:model:Foo',
         'VALUE',
         '20m'
       )
diff --git a/packages/data-point-service/lib/factory.test.js b/packages/data-point-service/lib/factory.test.js
index 841c35f..586ed2b 100644
--- a/packages/data-point-service/lib/factory.test.js
+++ b/packages/data-point-service/lib/factory.test.js
@@ -8,6 +8,7 @@ jest.mock('data-point-cache', () => {
   }
 })
 
+let os = require('os')
 let Factory = require('./factory')
 const DataPoint = require('data-point')
 const _ = require('lodash')
@@ -16,20 +17,92 @@ const logger = require('./logger')
 
 logger.clear()
 
+describe('getDefaultSettings', () => {
+  it('should return default settings', () => {
+    expect(Factory.getDefaultSettings()).toMatchSnapshot()
+  })
+})
+
+describe('prefixDeprecationError', () => {
+  it('should do nothing if cache.prefix is undefined', () => {
+    expect(() => {
+      Factory.prefixDeprecationError({})
+    }).not.toThrowError()
+  })
+
+  it('should warn when cache.prefix is set', () => {
+    expect(() => {
+      Factory.prefixDeprecationError({
+        cache: {
+          prefix: 'something'
+        }
+      })
+    }).toThrowErrorMatchingSnapshot()
+  })
+})
+
+describe('getCachePrefix', () => {
+  const hostname = os.hostname
+  const warn = logger.warn
+  afterEach(() => {
+    os.hostname = hostname
+    logger.warn = warn
+  })
+
+  it('should return os.hostname if not set', () => {
+    const options = {}
+    os.hostname = () => 'test'
+    expect(Factory.getCachePrefix(options)).toEqual('test:')
+  })
+  it('should use cache.redis.keyPrefix ', () => {
+    const options = {
+      cache: {
+        redis: {
+          keyPrefix: 'keyPrefix'
+        }
+      }
+    }
+    expect(Factory.getCachePrefix(options)).toEqual('keyPrefix:')
+  })
+  it('should use cache.redis.keyPrefix over cache.prefix ', () => {
+    const options = {
+      cache: {
+        redis: {
+          keyPrefix: 'keyPrefix'
+        }
+      }
+    }
+    logger.warn = () => {}
+    expect(Factory.getCachePrefix(options)).toEqual('keyPrefix:')
+  })
+  it('should use not add colon(:) twice', () => {
+    const options = {
+      cache: {
+        redis: {
+          keyPrefix: 'keyPrefix:'
+        }
+      }
+    }
+    logger.warn = () => {}
+    expect(Factory.getCachePrefix(options)).toEqual('keyPrefix:')
+  })
+})
+
 describe('createServiceObject', () => {
   test('It should create a default ServiceObject', () => {
     const os = require('os')
     const Service = Factory.createServiceObject()
     expect(Service).toEqual({
       cache: null,
-      cachePrefix: os.hostname(),
       dataPoint: null,
       isCacheAvailable: false,
       isCacheRequired: false,
       settings: {
         cache: {
           isRequired: false,
-          prefix: os.hostname()
+          redis: {
+            keyPrefix: `${os.hostname()}:`
+          }
         }
       }
     })
@@ -44,14 +117,15 @@ describe('createServiceObject', () => {
     })
     expect(Service).toEqual({
       cache: null,
-      cachePrefix: os.hostname(),
       dataPoint: null,
       isCacheAvailable: false,
       isCacheRequired: true,
       settings: {
         cache: {
           isRequired: true,
-          prefix: os.hostname()
+          redis: {
+            keyPrefix: `${os.hostname()}:`
+          }
         }
       }
     })