* Use [node-]redis as a direct dependency

* Extract Redis client creation to its own module

* Attach extensive logging in the Redis client creation module

* Allow the rate limiter to pass requests when Redis is disconnected

* Update rate-limit-redis

* Default error input to empty object for formatRedisError method

* Provide a name for the rate limiter's Redis client

* Include redis-mock, exclude ioredis/ioredis-mock

* Remove unused RedisAccessor#exists method

* Switch RedisAccessor to use redis/redis-mock

* Provide a name for logging on the Redis page cache

* Remove extraneous trailing space from Redis logging prefix

Our updated use of console.* will already be adding a space after the prefix

* Replace ioredis-mock with redis-mock in tests

* Revert removal of ioredis dependency

* Bind Redis client to async promisified methods

* Extract former RedisAccessor constructor tests to new create-client tests

* Update RedisAccessor tests to work with the callback-based redis client

* Handle formatting Redis errors (or not) with more resiliency
This commit is contained in:
James M. Greene 2021-03-29 12:34:22 -05:00 коммит произвёл GitHub
Родитель c1e3348f2e
Коммит 84547e54c7
Не найден ключ, соответствующий данной подписи
Идентификатор ключа GPG: 4AEE18F83AFDEB23
7 изменённых файлов: 97 добавлений и 229 удалений

Просмотреть файл

@ -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

Просмотреть файл

@ -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') })

Просмотреть файл

@ -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

140
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",

Просмотреть файл

@ -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",

Просмотреть файл

@ -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')
})
})
})

Просмотреть файл

@ -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')
)
})
})