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.

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<any>
+  mget: (...key: Array<string>) => Promise<Array<string | null>>
+  flushdb: () => Promise<any>
+  del: (key: string) => Promise<number>
+  quit: () => Promise<any>
+}
+
+export class BaseRedisCache implements TestableKeyValueCache<string> {
+  readonly client: RedisClient;
+  readonly defaultSetOptions: KeyValueCacheSetOptions = {
+    ttl: 300,
+  };
+
+  private loader: DataLoader<string, string | null>;
+
+  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<void> {
+    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<string | undefined> {
+    const reply = await this.loader.load(key);
+    if (reply !== null) {
+      return reply;
+    }
+    return;
+  }
+
+  async delete(key: string): Promise<boolean> {
+    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<void> {
+    await this.client.flushdb();
+  }
+
+  async close(): Promise<void> {
+    await this.client.quit();
+    return;
+  }
+}

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<string> {
-  readonly client: any;
-  readonly defaultSetOptions: KeyValueCacheSetOptions = {
-    ttl: 300,
-  };
-
-  private loader: DataLoader<string, string | null>;
+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<void> {
-    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<string | undefined> {
-    const reply = await this.loader.load(key);
-    if (reply !== null) {
-      return reply;
-    }
-    return;
-  }
-
-  async delete(key: string): Promise<boolean> {
-    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<void> {
-    await this.client.flushdb();
-  }
-
-  async close(): Promise<void> {
-    await this.client.quit();
-    return;
+    super(new Redis(options));
   }
 }

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<string, string | null>;
+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<void> {
-    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<string | undefined> {
-    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<boolean> {
-    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<string>): Promise<Array<string | null>> {
+        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<void> {
-    const masters = this.client.nodes('master') || [];
+    const masters = this.clusterClient.nodes('master') || [];
     await Promise.all(masters.map((node: RedisInstance) => node.flushdb()));
   }
-
-  async close(): Promise<void> {
-    await this.client.quit();
-    return;
-  }
 }

packages/apollo-server-cache-redis/src/__mocks__/ioredis.ts

@@ -3,8 +3,9 @@ class Redis {
   private timeouts = new Set<NodeJS.Timer>();
 
   async del(key: string) {
+    const keysDeleted = this.keyValue.hasOwnProperty(key) ? 1 : 0;
     delete this.keyValue[key];
-    return true;
+    return keysDeleted;
   }
 
   async get(key: string) {

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);
+});

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';