diff --git a/lib/redis-accessor.js b/lib/redis-accessor.js index cca44d54ba..3cd533f845 100644 --- a/lib/redis-accessor.js +++ b/lib/redis-accessor.js @@ -1,42 +1,21 @@ -const Redis = require('ioredis') -const InMemoryRedis = require('ioredis-mock') +const createRedisClient = require('./redis/create-client') +const InMemoryRedis = require('redis-mock') +const { promisify } = require('util') -const { CI, NODE_ENV, REDIS_URL, REDIS_MAX_DB } = process.env +const { CI, NODE_ENV, REDIS_URL } = process.env // Do not use real a Redis client for CI, tests, or if the REDIS_URL is not provided const useRealRedis = !CI && NODE_ENV !== 'test' && !!REDIS_URL -// By default, every Redis instance supports database numbers 0 - 15 -const redisMaxDb = REDIS_MAX_DB || 15 - -// Enable better stack traces in non-production environments -const redisBaseOptions = { - showFriendlyErrorStack: NODE_ENV !== 'production' -} - class RedisAccessor { - constructor ({ databaseNumber = 0, prefix = null, allowSetFailures = false } = {}) { - if (!Number.isInteger(databaseNumber) || databaseNumber < 0 || databaseNumber > redisMaxDb) { - throw new TypeError( - `Redis database number must be an integer between 0 and ${redisMaxDb} but was: ${JSON.stringify(databaseNumber)}` - ) - } - + constructor ({ databaseNumber = 0, prefix = null, allowSetFailures = false, name = null } = {}) { const redisClient = useRealRedis - ? new Redis(REDIS_URL, { - ...redisBaseOptions, + ? createRedisClient({ + url: REDIS_URL, db: databaseNumber, - - // Only add this configuration for TLS-enabled REDIS_URL values. - // Otherwise, it breaks for local Redis instances without TLS enabled. - ...REDIS_URL.startsWith('rediss://') && { - tls: { - // Required for production Heroku Redis - rejectUnauthorized: false - } - } + name: name || 'redis-accessor' }) - : new InMemoryRedis() + : InMemoryRedis.createClient() this._client = redisClient @@ -96,6 +75,7 @@ class RedisAccessor { } async set (key, value, options = {}) { + const setAsync = promisify(this._client.set).bind(this._client) const fullKey = this.prefix(key) if (typeof value !== 'string' || !value) { @@ -106,7 +86,7 @@ class RedisAccessor { const setArgs = this.constructor.translateSetArguments(options) try { - const result = await this._client.set(fullKey, value, ...setArgs) + const result = await setAsync(fullKey, value, ...setArgs) return result === 'OK' } catch (err) { const errorText = `Failed to set value in Redis. @@ -124,14 +104,10 @@ Error: ${err.message}` } async get (key) { - const value = await this._client.get(this.prefix(key)) + const getAsync = promisify(this._client.get).bind(this._client) + const value = await getAsync(this.prefix(key)) return value } - - async exists (key) { - const result = await this._client.exists(this.prefix(key)) - return result === 1 - } } module.exports = RedisAccessor diff --git a/lib/redis/create-client.js b/lib/redis/create-client.js index 45025aeb0d..36e9d60345 100644 --- a/lib/redis/create-client.js +++ b/lib/redis/create-client.js @@ -6,10 +6,12 @@ const { REDIS_MIN_DB, REDIS_MAX_DB } = process.env const redisMinDb = REDIS_MIN_DB || 0 const redisMaxDb = REDIS_MAX_DB || 15 -function formatRedisError (error = {}) { - const { code } = error - const preamble = error.constructor.name + (code ? ` with code "${code}"` : '') - return preamble + error.toString() +function formatRedisError (error) { + const errorCode = error ? error.code : null + const errorName = error ? error.constructor.name : 'Error' + const errorMsg = error ? error.toString() : 'unknown error' + const preamble = errorName + (errorCode ? ` with code "${errorCode}"` : '') + return preamble + ': ' + errorMsg } module.exports = function createClient (options = {}) { @@ -45,7 +47,7 @@ module.exports = function createClient (options = {}) { }) // If a `name` was provided, use it in the prefix for logging event messages - const logPrefix = '[redis' + (name ? ` (${name})` : '') + '] ' + const logPrefix = '[redis' + (name ? ` (${name})` : '') + ']' // Add event listeners for basic logging client.on('connect', () => { console.log(logPrefix, 'Connection opened') }) diff --git a/middleware/render-page.js b/middleware/render-page.js index 0bb4e62774..207984e941 100644 --- a/middleware/render-page.js +++ b/middleware/render-page.js @@ -16,7 +16,8 @@ const pageCache = new RedisAccessor({ databaseNumber: pageCacheDatabaseNumber, prefix: (HEROKU_RELEASE_VERSION ? HEROKU_RELEASE_VERSION + ':' : '') + 'rp', // Allow for graceful failures if a Redis SET operation fails - allowSetFailures: true + allowSetFailures: true, + name: 'page-cache' }) // a list of query params that *do* alter the rendered page, and therefore should be cached separately diff --git a/package-lock.json b/package-lock.json index e9db6da16f..3ec158f9e4 100644 --- a/package-lock.json +++ b/package-lock.json @@ -5711,11 +5711,6 @@ "resolved": "https://registry.npmjs.org/array-flatten/-/array-flatten-1.1.1.tgz", "integrity": "sha1-ml9pkFGx5wczKPKgCJaLZOopVdI=" }, - "array-from": { - "version": "2.1.1", - "resolved": "https://registry.npmjs.org/array-from/-/array-from-2.1.1.tgz", - "integrity": "sha1-z+nYwmYoudxa7MYqn12PHzUsEZU=" - }, "array-includes": { "version": "3.1.1", "resolved": "https://registry.npmjs.org/array-includes/-/array-includes-3.1.1.tgz", @@ -9898,19 +9893,6 @@ "es6-symbol": "^3.1.1" } }, - "es6-map": { - "version": "0.1.5", - "resolved": "https://registry.npmjs.org/es6-map/-/es6-map-0.1.5.tgz", - "integrity": "sha1-kTbgUD3MBqMBaQ8LsU/042TpSfA=", - "requires": { - "d": "1", - "es5-ext": "~0.10.14", - "es6-iterator": "~2.0.1", - "es6-set": "~0.1.5", - "es6-symbol": "~3.1.1", - "event-emitter": "~0.3.5" - } - }, "es6-promise": { "version": "4.2.8", "resolved": "https://registry.npmjs.org/es6-promise/-/es6-promise-4.2.8.tgz", @@ -9925,29 +9907,6 @@ "es6-promise": "^4.0.3" } }, - "es6-set": { - "version": "0.1.5", - "resolved": "https://registry.npmjs.org/es6-set/-/es6-set-0.1.5.tgz", - "integrity": "sha1-0rPsXU2ADO2BjbU40ol02wpzzLE=", - "requires": { - "d": "1", - "es5-ext": "~0.10.14", - "es6-iterator": "~2.0.1", - "es6-symbol": "3.1.1", - "event-emitter": "~0.3.5" - }, - "dependencies": { - "es6-symbol": { - "version": "3.1.1", - "resolved": "https://registry.npmjs.org/es6-symbol/-/es6-symbol-3.1.1.tgz", - "integrity": "sha1-vwDvT9q2uhtG7Le2KbTH7VcVzHc=", - "requires": { - "d": "1", - "es5-ext": "~0.10.14" - } - } - } - }, "es6-symbol": { "version": "3.1.3", "resolved": "https://registry.npmjs.org/es6-symbol/-/es6-symbol-3.1.3.tgz", @@ -10566,15 +10525,6 @@ "resolved": "https://registry.npmjs.org/etag/-/etag-1.8.1.tgz", "integrity": "sha1-Qa4u62XvpiJorr/qg6x9eSmbCIc=" }, - "event-emitter": { - "version": "0.3.5", - "resolved": "https://registry.npmjs.org/event-emitter/-/event-emitter-0.3.5.tgz", - "integrity": "sha1-34xp7vFkeSPHFXuc6DhAYQsCzDk=", - "requires": { - "d": "1", - "es5-ext": "~0.10.14" - } - }, "event-stream": { "version": "3.3.4", "resolved": "https://registry.npmjs.org/event-stream/-/event-stream-3.3.4.tgz", @@ -11242,28 +11192,6 @@ "resolved": "https://registry.npmjs.org/feature-policy/-/feature-policy-0.3.0.tgz", "integrity": "sha512-ZtijOTFN7TzCujt1fnNhfWPFPSHeZkesff9AXZj+UEjYBynWNUIYpC87Ve4wHzyexQsImicLu7WsC2LHq7/xrQ==" }, - "fengari": { - "version": "0.1.4", - "resolved": "https://registry.npmjs.org/fengari/-/fengari-0.1.4.tgz", - "integrity": "sha512-6ujqUuiIYmcgkGz8MGAdERU57EIluGGPSUgGPTsco657EHa+srq0S3/YUl/r9kx1+D+d4rGfYObd+m8K22gB1g==", - "requires": { - "readline-sync": "^1.4.9", - "sprintf-js": "^1.1.1", - "tmp": "^0.0.33" - }, - "dependencies": { - "sprintf-js": { - "version": "1.1.2", - "resolved": "https://registry.npmjs.org/sprintf-js/-/sprintf-js-1.1.2.tgz", - "integrity": "sha512-VE0SOVEHCk7Qc8ulkWw3ntAzXuqf7S2lvwQaDLRnUeIEaKNQJzV6BwmLKhOqT61aGhfUMrXeaBk+oDGCzvhcug==" - } - } - }, - "fengari-interop": { - "version": "0.1.2", - "resolved": "https://registry.npmjs.org/fengari-interop/-/fengari-interop-0.1.2.tgz", - "integrity": "sha512-8iTvaByZVoi+lQJhHH9vC+c/Yaok9CwOqNQZN6JrVpjmWwW4dDkeblBXhnHC+BoI6eF4Cy5NKW3z6ICEjvgywQ==" - }, "figgy-pudding": { "version": "3.5.2", "resolved": "https://registry.npmjs.org/figgy-pudding/-/figgy-pudding-3.5.2.tgz", @@ -13511,20 +13439,20 @@ } }, "ioredis": { - "version": "4.19.4", - "resolved": "https://registry.npmjs.org/ioredis/-/ioredis-4.19.4.tgz", - "integrity": "sha512-3haQWw9dpEjcfVcRktXlayVNrrqvvc2io7Q/uiV2UsYw8/HC2YwwJr78Wql7zu5bzwci0x9bZYA69U7KkevAvw==", + "version": "4.24.4", + "resolved": "https://registry.npmjs.org/ioredis/-/ioredis-4.24.4.tgz", + "integrity": "sha512-v28xxBENyTmReC6lVTL7EkAPjVF8cuGtDEjGDi1Z2n7htsC2WdiDceZrCIPeuPiLa6kDFWHb1Y8O0upZROsMgA==", "requires": { "cluster-key-slot": "^1.1.0", - "debug": "^4.1.1", + "debug": "^4.3.1", "denque": "^1.1.0", "lodash.defaults": "^4.2.0", "lodash.flatten": "^4.4.0", "p-map": "^2.1.0", - "redis-commands": "1.6.0", + "redis-commands": "1.7.0", "redis-errors": "^1.2.0", "redis-parser": "^3.0.0", - "standard-as-callback": "^2.0.1" + "standard-as-callback": "^2.1.0" }, "dependencies": { "debug": { @@ -13539,28 +13467,11 @@ "version": "2.1.2", "resolved": "https://registry.npmjs.org/ms/-/ms-2.1.2.tgz", "integrity": "sha512-sGkPx+VjMtmA6MX27oA4FBFELFCZZ4S4XqeGOXCv68tT+jb3vk/RyaKWP0PTKyWtmLSM0b+adUTEvbs1PEaH2w==" - } - } - }, - "ioredis-mock": { - "version": "5.2.1", - "resolved": "https://registry.npmjs.org/ioredis-mock/-/ioredis-mock-5.2.1.tgz", - "integrity": "sha512-Cc9KcZvt1e8KUIkTq669n6EdheveQVwloORpUmMQonxTnI2wn5mjQ9kSrsKX0Obv1TUUwz+nRDcFecfo+h9koA==", - "requires": { - "array-from": "^2.1.1", - "es6-map": "^0.1.5", - "es6-set": "^0.1.5", - "fengari": "^0.1.4", - "fengari-interop": "^0.1.2", - "lodash": "^4.17.20", - "minimatch": "^3.0.4", - "standard-as-callback": "^2.0.1" - }, - "dependencies": { - "lodash": { - "version": "4.17.20", - "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz", - "integrity": "sha512-PlhdFcillOINfeV7Ni6oF1TAEayyZBoZ8bcshTHqOYJYlrqzRK5hagpagky5o4HfCzzd1TRkXPMFq6cKk9rGmA==" + }, + "redis-commands": { + "version": "1.7.0", + "resolved": "https://registry.npmjs.org/redis-commands/-/redis-commands-1.7.0.tgz", + "integrity": "sha512-nJWqw3bTFy21hX/CPKHth6sfhZbdiHP6bTawSgQBlKOVRG7EZkfHbbHwQJnrE4vsQf0CMNE+3gJ4Fmm16vdVlQ==" } } }, @@ -19726,11 +19637,6 @@ "windows-release": "^3.1.0" } }, - "os-tmpdir": { - "version": "1.0.2", - "resolved": "https://registry.npmjs.org/os-tmpdir/-/os-tmpdir-1.0.2.tgz", - "integrity": "sha1-u+Z0BseaqFxc/sdm/lc0VV36EnQ=" - }, "p-cancelable": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/p-cancelable/-/p-cancelable-1.1.0.tgz", @@ -21100,11 +21006,6 @@ "readable-stream": "^2.0.2" } }, - "readline-sync": { - "version": "1.4.10", - "resolved": "https://registry.npmjs.org/readline-sync/-/readline-sync-1.4.10.tgz", - "integrity": "sha512-gNva8/6UAe8QYepIQH/jQ2qn91Qj0B9sYjMBBs3QOB8F2CXcKgLxQaJRP76sWVRQt+QU+8fAkCbCvjjMFu7Ycw==" - }, "redent": { "version": "3.0.0", "resolved": "https://registry.npmjs.org/redent/-/redent-3.0.0.tgz", @@ -21135,6 +21036,11 @@ "resolved": "https://registry.npmjs.org/redis-errors/-/redis-errors-1.2.0.tgz", "integrity": "sha1-62LSrbFeTq9GEMBK/hUpOEJQq60=" }, + "redis-mock": { + "version": "0.56.3", + "resolved": "https://registry.npmjs.org/redis-mock/-/redis-mock-0.56.3.tgz", + "integrity": "sha512-ynaJhqk0Qf3Qajnwvy4aOjS4Mdf9IBkELWtjd+NYhpiqu4QCNq6Vf3Q7c++XRPGiKiwRj9HWr0crcwy7EiPjYQ==" + }, "redis-parser": { "version": "3.0.0", "resolved": "https://registry.npmjs.org/redis-parser/-/redis-parser-3.0.0.tgz", @@ -22570,9 +22476,9 @@ "dev": true }, "standard-as-callback": { - "version": "2.0.1", - "resolved": "https://registry.npmjs.org/standard-as-callback/-/standard-as-callback-2.0.1.tgz", - "integrity": "sha512-NQOxSeB8gOI5WjSaxjBgog2QFw55FV8TkS6Y07BiB3VJ8xNTvUYm0wl0s8ObgQ5NhdpnNfigMIKjgPESzgr4tg==" + "version": "2.1.0", + "resolved": "https://registry.npmjs.org/standard-as-callback/-/standard-as-callback-2.1.0.tgz", + "integrity": "sha512-qoRRSyROncaz1z0mvYqIE4lCd9p2R90i6GxW3uZv5ucSu8tU7B5HXUP1gG8pVZsYNVaXjk8ClXHPttLyxAL48A==" }, "start-server-and-test": { "version": "1.12.0", @@ -23530,14 +23436,6 @@ "upper-case": "^1.0.3" } }, - "tmp": { - "version": "0.0.33", - "resolved": "https://registry.npmjs.org/tmp/-/tmp-0.0.33.tgz", - "integrity": "sha512-jRCJlojKnZ3addtTOjdIqoRuPEKBvNXcGYqzO6zWZX8KfKEpnGY5jfggJQ3EjKuu8D4bJRr0y+cYJFmYbImXGw==", - "requires": { - "os-tmpdir": "~1.0.2" - } - }, "tmpl": { "version": "1.0.4", "resolved": "https://registry.npmjs.org/tmpl/-/tmpl-1.0.4.tgz", diff --git a/package.json b/package.json index ab86347013..57dab2f197 100644 --- a/package.json +++ b/package.json @@ -58,8 +58,7 @@ "hot-shots": "^8.2.0", "html-entities": "^1.2.1", "imurmurhash": "^0.1.4", - "ioredis": "^4.19.4", - "ioredis-mock": "^5.2.0", + "ioredis": "^4.24.4", "is-url": "^1.2.4", "js-cookie": "^2.2.1", "js-yaml": "^3.14.0", @@ -79,6 +78,7 @@ "react": "^17.0.1", "react-dom": "^17.0.1", "redis": "^3.0.2", + "redis-mock": "^0.56.3", "rehype-autolink-headings": "^2.0.5", "rehype-highlight": "^3.1.0", "rehype-raw": "^4.0.2", diff --git a/tests/unit/redis-accessor.js b/tests/unit/redis-accessor.js index 3e8d77995c..7b673a13a7 100644 --- a/tests/unit/redis-accessor.js +++ b/tests/unit/redis-accessor.js @@ -1,4 +1,4 @@ -const InMemoryRedis = require('ioredis-mock') +const { RedisClient: InMemoryRedis } = require('redis-mock') const RedisAccessor = require('../../lib/redis-accessor') describe('RedisAccessor', () => { @@ -59,32 +59,6 @@ describe('RedisAccessor', () => { }) }) - describe('constructor', () => { - test('throws if databaseNumber is provided but is not a number', async () => { - expect(() => new RedisAccessor({ databaseNumber: 'dbName' })).toThrowError( - new TypeError('Redis database number must be an integer between 0 and 15 but was: "dbName"') - ) - }) - - test('throws if databaseNumber is provided but is not an integer', async () => { - expect(() => new RedisAccessor({ databaseNumber: 1.5 })).toThrowError( - new TypeError('Redis database number must be an integer between 0 and 15 but was: 1.5') - ) - }) - - test('throws if databaseNumber is provided but is less than 0', async () => { - expect(() => new RedisAccessor({ databaseNumber: -1 })).toThrowError( - new TypeError('Redis database number must be an integer between 0 and 15 but was: -1') - ) - }) - - test('throws if databaseNumber is provided but is greater than max allowed', async () => { - expect(() => new RedisAccessor({ databaseNumber: 16 })).toThrowError( - new TypeError('Redis database number must be an integer between 0 and 15 but was: 16') - ) - }) - }) - describe('#prefix method', () => { test('returns prefixed key', async () => { const prefix = 'myPrefix' @@ -203,7 +177,7 @@ describe('RedisAccessor', () => { test('resolves to false if value was not set', async () => { const instance = new RedisAccessor() - instance._client.set = jest.fn(async () => 'NOT_OK') + instance._client.set = jest.fn((...args) => args.pop()(null, 'NOT_OK')) expect(await instance.set('myKey', 'myValue')).toBe(false) }) @@ -213,7 +187,8 @@ describe('RedisAccessor', () => { const setSpy = jest.spyOn(instance._client, 'set') await instance.set('myKey', 'myValue') - expect(setSpy).toBeCalledWith('myKey', 'myValue') + expect(setSpy.mock.calls.length).toBe(1) + expect(setSpy.mock.calls[0].slice(0, 2)).toEqual(['myKey', 'myValue']) }) test('resolves to false if Redis replies with an error and `allowSetFailures` option is set to true', async () => { @@ -221,7 +196,7 @@ describe('RedisAccessor', () => { const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation() const instance = new RedisAccessor({ prefix: 'myPrefix', allowSetFailures: true }) - instance._client.set = jest.fn(async () => { throw new Error('Redis ReplyError') }) + instance._client.set = jest.fn((...args) => args.pop()(new Error('Redis ReplyError'))) const result = await instance.set('myKey', 'myValue') @@ -241,7 +216,7 @@ Error: Redis ReplyError` const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation() const instance = new RedisAccessor({ prefix: 'myPrefix' }) - instance._client.set = jest.fn(async () => { throw new Error('Redis ReplyError') }) + instance._client.set = jest.fn((...args) => args.pop()(new Error('Redis ReplyError'))) await expect(instance.set('myKey', 'myValue')).rejects.toThrowError( new Error(`Failed to set value in Redis. @@ -279,7 +254,8 @@ Error: Redis ReplyError` await instance.set('myKey', 'myValue', { expireIn: 20 }) expect(argSpy).toBeCalled() - expect(setSpy).toBeCalledWith('myKey', 'myValue', 'PX', 20) + expect(setSpy.mock.calls.length).toBe(1) + expect(setSpy.mock.calls[0].slice(0, 4)).toEqual(['myKey', 'myValue', 'PX', 20]) argSpy.mockRestore() }) @@ -304,42 +280,24 @@ Error: Redis ReplyError` test('retrieves matching entry from Redis with #_client.get', async () => { const instance = new RedisAccessor() - const getSpy = jest.spyOn(instance._client, 'get') + let callbackSpy + const originalGet = instance._client.get.bind(instance._client) + instance._client.get = jest.fn((...args) => { + const realCallback = args.pop() + callbackSpy = jest.fn((error, value) => { + realCallback(error, value) + }) + + return originalGet(...args, callbackSpy) + }) await instance.set('myKey', 'myValue') await instance.get('myKey') - expect(getSpy).toBeCalledWith('myKey') - expect(getSpy).toHaveReturnedWith(Promise.resolve('myValue')) - }) - }) + expect(instance._client.get.mock.calls.length).toBe(1) + expect(instance._client.get.mock.calls[0].slice(0, 1)).toEqual(['myKey']) - describe('#exists method', () => { - test('resolves to true if matching entry exists in Redis', async () => { - const instance = new RedisAccessor() - - await instance.set('myKey', 'myValue') - - const result = await instance.exists('myKey') - expect(result).toBe(true) - }) - - test('resolves to false if no matching entry exists in Redis', async () => { - const instance = new RedisAccessor() - - const result = await instance.exists('fakeKey') - expect(result).toBe(false) - }) - - test('checks for matching entry from Redis with #_client.exists', async () => { - const instance = new RedisAccessor() - const existsSpy = jest.spyOn(instance._client, 'exists') - - await instance.set('myKey', 'myValue') - await instance.exists('myKey') - - expect(existsSpy).toBeCalledWith('myKey') - expect(existsSpy).toHaveReturnedWith(Promise.resolve(true)) + expect(callbackSpy).toHaveBeenCalledWith(null, 'myValue') }) }) }) diff --git a/tests/unit/redis/create-client.js b/tests/unit/redis/create-client.js new file mode 100644 index 0000000000..e01848cc3f --- /dev/null +++ b/tests/unit/redis/create-client.js @@ -0,0 +1,33 @@ +const createRedisClient = require('../../../lib/redis/create-client') + +const redisUrl = 'http://localhost:6379' + +describe('create-client', () => { + test('returns null if no URL is provided', async () => { + expect(createRedisClient({})).toBe(null) + }) + + test('throws if database number is provided but is not a number', async () => { + expect(() => createRedisClient({ url: redisUrl, db: 'dbName' })).toThrowError( + new TypeError('Redis database number must be an integer between 0 and 15 but was: "dbName"') + ) + }) + + test('throws if database number is provided but is not an integer', async () => { + expect(() => createRedisClient({ url: redisUrl, db: 1.5 })).toThrowError( + new TypeError('Redis database number must be an integer between 0 and 15 but was: 1.5') + ) + }) + + test('throws if database number is provided but is less than 0', async () => { + expect(() => createRedisClient({ url: redisUrl, db: -1 })).toThrowError( + new TypeError('Redis database number must be an integer between 0 and 15 but was: -1') + ) + }) + + test('throws if database number is provided but is greater than max allowed', async () => { + expect(() => createRedisClient({ url: redisUrl, db: 16 })).toThrowError( + new TypeError('Redis database number must be an integer between 0 and 15 but was: 16') + ) + }) +})