From 1a7f47042f61a29f9a99e7323f9ca8dff1ed0e0f Mon Sep 17 00:00:00 2001 From: Adam Noakes Date: Tue, 6 Apr 2021 21:51:43 +0100 Subject: [PATCH] apollo-servercache-redis: Allow Redis client to be injected (#5034) - Allow Redis client to be injected by using the `BaseRedisCache` class which contains the existing implementation logic without the client initialisation. - Removes duplication between `RedisCache` and `RedisClusterCache` - Fixes a bug where the result of delete is returned as a number, rather than a boolean. (Our TypeScript types have always declared that this function returns a boolean as `KeyValueCache.delete` should, but the actual implementation returned a number. This was not noticed because of the use of an `any` type.) In my opinion, `BaseRedisCache` should really be `RedisCache` and the existing `RedisCache` + `RedisCacheCluster` should either be renamed to identify that they are using the ioredis client, or removed and left for the consumer of the library to implement/ inject the client using this method. But this would obviously be a breaking change so maybe one to think about for the future? I've created an interface for the `RedisClient` rather than rely on the ioredis type as i do not believe apollo should be tied to the ioredis interface. Fixes #4871. Fixes #4870. Fixes #5006. --- .../src/BaseRedisCache.ts | 68 +++++++++++++++++++ .../src/RedisCache.ts | 60 +--------------- .../src/RedisClusterCache.ts | 65 ++++-------------- .../src/__mocks__/ioredis.ts | 3 +- .../src/__tests__/BaseRedisCache.test.ts | 31 +++++++++ .../apollo-server-cache-redis/src/index.ts | 1 + 6 files changed, 120 insertions(+), 108 deletions(-) create mode 100644 packages/apollo-server-cache-redis/src/BaseRedisCache.ts create mode 100644 packages/apollo-server-cache-redis/src/__tests__/BaseRedisCache.test.ts diff --git a/packages/apollo-server-cache-redis/src/BaseRedisCache.ts b/packages/apollo-server-cache-redis/src/BaseRedisCache.ts new file mode 100644 index 00000000000..116302f92c3 --- /dev/null +++ b/packages/apollo-server-cache-redis/src/BaseRedisCache.ts @@ -0,0 +1,68 @@ +import { + TestableKeyValueCache, + KeyValueCacheSetOptions, +} from 'apollo-server-caching'; +import DataLoader from 'dataloader'; + +export interface RedisClient { + set: (key: string, value: string, option?: string, optionValue?: number) => Promise + mget: (...key: Array) => Promise> + flushdb: () => Promise + del: (key: string) => Promise + quit: () => Promise +} + +export class BaseRedisCache implements TestableKeyValueCache { + readonly client: RedisClient; + readonly defaultSetOptions: KeyValueCacheSetOptions = { + ttl: 300, + }; + + private loader: DataLoader; + + constructor(client: RedisClient) { + this.client = client; + + this.loader = new DataLoader(keys => client.mget(...keys), { + cache: false, + }); + } + + async set( + key: string, + value: string, + options?: KeyValueCacheSetOptions, + ): Promise { + const { ttl } = Object.assign({}, this.defaultSetOptions, options); + if (typeof ttl === 'number') { + await this.client.set(key, value, 'EX', ttl); + } else { + // We'll leave out the EXpiration when no value is specified. Of course, + // it may be purged from the cache for other reasons as deemed necessary. + await this.client.set(key, value); + } + } + + async get(key: string): Promise { + const reply = await this.loader.load(key); + if (reply !== null) { + return reply; + } + return; + } + + async delete(key: string): Promise { + return await this.client.del(key) > 0; + } + + // Drops all data from Redis. This should only be used by test suites --- + // production code should never drop all data from an end user Redis cache! + async flush(): Promise { + await this.client.flushdb(); + } + + async close(): Promise { + await this.client.quit(); + return; + } +} diff --git a/packages/apollo-server-cache-redis/src/RedisCache.ts b/packages/apollo-server-cache-redis/src/RedisCache.ts index 22fe9a1977f..4732327efde 100644 --- a/packages/apollo-server-cache-redis/src/RedisCache.ts +++ b/packages/apollo-server-cache-redis/src/RedisCache.ts @@ -1,62 +1,8 @@ -import { - TestableKeyValueCache, - KeyValueCacheSetOptions, -} from 'apollo-server-caching'; import Redis, { RedisOptions } from 'ioredis'; -import DataLoader from 'dataloader'; - -export class RedisCache implements TestableKeyValueCache { - readonly client: any; - readonly defaultSetOptions: KeyValueCacheSetOptions = { - ttl: 300, - }; - - private loader: DataLoader; +import { BaseRedisCache } from './BaseRedisCache'; +export class RedisCache extends BaseRedisCache { constructor(options?: RedisOptions) { - const client = new Redis(options); - this.client = client; - - this.loader = new DataLoader(keys => client.mget(...keys), { - cache: false, - }); - } - - async set( - key: string, - value: string, - options?: KeyValueCacheSetOptions, - ): Promise { - const { ttl } = Object.assign({}, this.defaultSetOptions, options); - if (typeof ttl === 'number') { - await this.client.set(key, value, 'EX', ttl); - } else { - // We'll leave out the EXpiration when no value is specified. Of course, - // it may be purged from the cache for other reasons as deemed necessary. - await this.client.set(key, value); - } - } - - async get(key: string): Promise { - const reply = await this.loader.load(key); - if (reply !== null) { - return reply; - } - return; - } - - async delete(key: string): Promise { - return await this.client.del(key); - } - - // Drops all data from Redis. This should only be used by test suites --- - // production code should never drop all data from an end user Redis cache! - async flush(): Promise { - await this.client.flushdb(); - } - - async close(): Promise { - await this.client.quit(); - return; + super(new Redis(options)); } } diff --git a/packages/apollo-server-cache-redis/src/RedisClusterCache.ts b/packages/apollo-server-cache-redis/src/RedisClusterCache.ts index 401799a9d52..26ccc2b63a9 100644 --- a/packages/apollo-server-cache-redis/src/RedisClusterCache.ts +++ b/packages/apollo-server-cache-redis/src/RedisClusterCache.ts @@ -1,64 +1,29 @@ -import { KeyValueCache, KeyValueCacheSetOptions } from 'apollo-server-caching'; import Redis, { ClusterOptions, ClusterNode, Redis as RedisInstance, } from 'ioredis'; -import DataLoader from 'dataloader'; +import { BaseRedisCache } from './BaseRedisCache'; -export class RedisClusterCache implements KeyValueCache { - readonly client: any; - readonly defaultSetOptions: KeyValueCacheSetOptions = { - ttl: 300, - }; - - private loader: DataLoader; +export class RedisClusterCache extends BaseRedisCache { + private readonly clusterClient: Redis.Cluster; constructor(nodes: ClusterNode[], options?: ClusterOptions) { - const client = this.client = new Redis.Cluster(nodes, options); - - this.loader = new DataLoader( - (keys = []) => - Promise.all(keys.map(key => client.get(key).catch(() => null))), - { cache: false }, - ); - } - - async set( - key: string, - data: string, - options?: KeyValueCacheSetOptions, - ): Promise { - const { ttl } = Object.assign({}, this.defaultSetOptions, options); - if (typeof ttl === 'number') { - await this.client.set(key, data, 'EX', ttl); - } else { - // We'll leave out the EXpiration when no value is specified. Of course, - // it may be purged from the cache for other reasons as deemed necessary. - await this.client.set(key, data); - } - } - - async get(key: string): Promise { - const reply = await this.loader.load(key); - // reply is null if key is not found - if (reply !== null) { - return reply; - } - return; - } - - async delete(key: string): Promise { - return await this.client.del(key); + const clusterClient = new Redis.Cluster(nodes, options) + super({ + del: clusterClient.del.bind(clusterClient), + flushdb: clusterClient.flushdb.bind(clusterClient), + mget(...keys: Array): Promise> { + return Promise.all(keys.map(key => clusterClient.get(key).catch(() => null))) + }, + quit: clusterClient.quit.bind(clusterClient), + set: clusterClient.set.bind(clusterClient), + }); + this.clusterClient = clusterClient; } async flush(): Promise { - const masters = this.client.nodes('master') || []; + const masters = this.clusterClient.nodes('master') || []; await Promise.all(masters.map((node: RedisInstance) => node.flushdb())); } - - async close(): Promise { - await this.client.quit(); - return; - } } diff --git a/packages/apollo-server-cache-redis/src/__mocks__/ioredis.ts b/packages/apollo-server-cache-redis/src/__mocks__/ioredis.ts index dd56966b0de..e46a9e11e29 100644 --- a/packages/apollo-server-cache-redis/src/__mocks__/ioredis.ts +++ b/packages/apollo-server-cache-redis/src/__mocks__/ioredis.ts @@ -3,8 +3,9 @@ class Redis { private timeouts = new Set(); async del(key: string) { + const keysDeleted = this.keyValue.hasOwnProperty(key) ? 1 : 0; delete this.keyValue[key]; - return true; + return keysDeleted; } async get(key: string) { diff --git a/packages/apollo-server-cache-redis/src/__tests__/BaseRedisCache.test.ts b/packages/apollo-server-cache-redis/src/__tests__/BaseRedisCache.test.ts new file mode 100644 index 00000000000..978f18fff5c --- /dev/null +++ b/packages/apollo-server-cache-redis/src/__tests__/BaseRedisCache.test.ts @@ -0,0 +1,31 @@ +jest.mock('ioredis'); + +import { BaseRedisCache, RedisClient } from '../index'; +import { + testKeyValueCache_Basics, + testKeyValueCache_Expiration, +} from '../../../apollo-server-caching/src/__tests__/testsuite'; + + +describe('BaseRedisCacheTest', () => { + const store: { [key: string]: string } = {}; + const testRedisClient: RedisClient = { + set: jest.fn((key: string, value: string, option?: string, ttl?: number) => { + store[key] = value; + option === 'EX' && ttl && setTimeout(() => delete store[key], ttl * 1000); + return Promise.resolve(); + }), + mget: jest.fn((...keys) => Promise.resolve(keys.map((key: string) => store[key]))), + flushdb: jest.fn(() => Promise.resolve()), + del: jest.fn((key: string) => { + const keysDeleted = store.hasOwnProperty(key) ? 1 : 0; + delete store[key]; + return Promise.resolve(keysDeleted); + }), + quit: jest.fn(() => Promise.resolve()), + } + + const cache = new BaseRedisCache(testRedisClient); + testKeyValueCache_Basics(cache); + testKeyValueCache_Expiration(cache); +}); diff --git a/packages/apollo-server-cache-redis/src/index.ts b/packages/apollo-server-cache-redis/src/index.ts index 0c22eee2f0b..34782f2d488 100644 --- a/packages/apollo-server-cache-redis/src/index.ts +++ b/packages/apollo-server-cache-redis/src/index.ts @@ -1,2 +1,3 @@ export { RedisCache } from './RedisCache'; export { RedisClusterCache } from './RedisClusterCache'; +export { RedisClient, BaseRedisCache } from './BaseRedisCache';