apollo-server-cache-redis: follow-up to #5034

Add docs and changelog. As part of doing this, realize that the implementation in #5034 makes it annoying to inject a cluster client, so switch the new API to decide about get vs mget based on which kind of client is passed to the base class.
apollographql · Apr 6, 2021 · b3a829b · b3a829b
1 parent 1a7f470
commit b3a829b
Show file tree

Hide file tree

Showing 8 changed files with 151 additions and 89 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,7 @@ The version headers in this history reflect the versions of Apollo Server itself
 > The changes noted within this `vNEXT` section have not been released yet.  New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development.  With few exceptions, the format of the entry should follow convention (i.e., prefix with package name, use markdown `backtick formatting` for package names and code, suffix with a link to the change-set à la `[PR #YYY](https://link/pull/YYY)`, etc.).  When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.
 
 - `apollo-server-core`: Add optional argument to `ApolloServer.executeOperation` allowing the caller to manually specify an argument to the `config` function analogous to that provided by integration packages. [PR #4166](https://github.com/apollographql/apollo-server/pull/4166) [Issue #2886](https://github.com/apollographql/apollo-server/issues/2886)
+- `apollo-server-cache-redis`: New `BaseRedisCache` class which takes an `ioredis`-compatible Redis client as an argument. The existing classes `RedisCache` and `RedisClusterCache` (which pass their arguments to `ioredis` constructors) are now implemented in terms of this class. This allows you to use any of the `ioredis` constructor forms rather than just the ones recognized by our classes. This also fixes a long-standing bug where the Redis cache implementations returned a number from `delete()`; it now returns a number, matching what the `KeyValueCache` interface and the TypeScript types expect. [PR #5034](https://github.com/apollographql/apollo-server/pull/5034) [PR #5088](https://github.com/apollographql/apollo-server/pull/5088) [Issue #4870](https://github.com/apollographql/apollo-server/issues/4870) [Issue #5006](https://github.com/apollographql/apollo-server/issues/5006)
 
 ## v2.22.2
 

diff --git a/docs/source/data/data-sources.md b/docs/source/data/data-sources.md
@@ -257,14 +257,16 @@ const server = new ApolloServer({
 For documentation of the options you can pass to the underlying Memcached client, look [here](https://github.com/3rd-Eden/memcached).
 
 ```js
-const { RedisCache } = require('apollo-server-cache-redis');
+const { BaseRedisCache } = require('apollo-server-cache-redis');
+const Redis = require('ioredis');
 
 const server = new ApolloServer({
   typeDefs,
   resolvers,
-  cache: new RedisCache({
-    host: 'redis-server',
-    // Options are passed through to the Redis client
+  cache: new BaseRedisCache({
+    client: new Redis({
+      host: 'redis-server',
+    }),
   }),
   dataSources: () => ({
     moviesAPI: new MoviesAPI(),

diff --git a/docs/source/performance/apq.md b/docs/source/performance/apq.md
@@ -217,20 +217,22 @@ const server = new ApolloServer({
 ### Redis (single instance)
 
 ```shell
-$ npm install apollo-server-cache-redis
+$ npm install apollo-server-cache-redis ioredis
 ```
 
 ```javascript
-const { RedisCache } = require('apollo-server-cache-redis');
+const { BaseRedisCache } = require('apollo-server-cache-redis');
+const Redis = require('ioredis');
 
 const server = new ApolloServer({
   typeDefs,
   resolvers,
   // highlight-start
   persistedQueries: {
-    cache: new RedisCache({
-      host: 'redis-server',
-      // Options are passed through to the Redis client
+    cache: new BaseRedisCache({
+      client: new Redis({
+        host: 'redis-server',
+      }),
     }),
   },
   // highlight-end
@@ -240,25 +242,27 @@ const server = new ApolloServer({
 ### Redis (Sentinel)
 
 ```shell
-$ npm install apollo-server-cache-redis
+$ npm install apollo-server-cache-redis ioredis
 ```
 
 ```javascript
-const { RedisCache } = require('apollo-server-cache-redis');
+const { BaseRedisCache } = require('apollo-server-cache-redis');
+const Redis = require('ioredis');
 
 const server = new ApolloServer({
   typeDefs,
   resolvers,
   // highlight-start
   persistedQueries: {
-    cache: new RedisCache({
-      sentinels: [{
-        host: 'sentinel-host-01',
-        port: 26379
-      }],
-      password: 'my_password',
-      name: 'service_name',
-      // Options are passed through to the Redis client
+    cache: new BaseRedisCache({
+      client: new Redis({
+        sentinels: [{
+          host: 'sentinel-host-01',
+          port: 26379
+         }],
+        password: 'my_password',
+        name: 'service_name',
+      }),
     }),
   },
   // highlight-end
@@ -268,26 +272,30 @@ const server = new ApolloServer({
 ### Redis Cluster
 
 ```shell
-$ npm install apollo-server-cache-redis
+$ npm install apollo-server-cache-redis ioredis
 ```
 
 ```javascript
-const { RedisClusterCache } = require('apollo-server-cache-redis');
+const { BaseRedisCache } = require('apollo-server-cache-redis');
+const Redis = require('ioredis');
 
 const server = new ApolloServer({
   typeDefs,
   resolvers,
   // highlight-start
   persistedQueries: {
-    cache: new RedisClusterCache(
-      [{
-        host: 'redis-node-01-host',
-        // Options are passed through to the Redis cluster client
-      }],
-      {
-        // Cluster options are passed through to the Redis cluster client
-      }
-    ),
+    cache: new BaseRedisCache({
+      // Note that this uses the "clusterClient" option rather than "client",
+      // which avoids using the mget command which doesn't work in cluster mode.
+      clusterClient: new Redis.Cluster(
+        [{
+          host: 'redis-node-01-host',
+        }],
+        {
+          // Other Redis cluster client options
+        }
+      ),
+    }),
   },
   // highlight-end
 });

diff --git a/packages/apollo-server-cache-redis/README.md b/packages/apollo-server-cache-redis/README.md
@@ -9,17 +9,22 @@ It currently supports a single instance of Redis, [Cluster](http://redis.io/topi
 
 ## Usage
 
+This package is built to be compatible with the [ioredis](https://www.npmjs.com/package/ioredis) Redis client. The recommended usage is to use the `BaseRedisCache` class which takes either a `client` option (a client that talks to a single server) or a `clusterClient` option (a client that talks to Redis Cluster). (The difference is that ioredis [only supports the `mget` multi-get command in non-cluster mode](https://github.com/luin/ioredis/issues/811), so using `clusterClient` tells `BaseRedisCache` to use parallel `get` commands instead.)
+
+You may also use the older `RedisCache` and `RedisClusterCache` classes, which allow you to pass the ioredis constructor arguments directly to the cache class's constructor.
 ### Single instance
 
 ```js
-const { RedisCache } = require('apollo-server-cache-redis');
+const { BaseRedisCache } = require('apollo-server-cache-redis');
+const Redis = require('ioredis');
 
 const server = new ApolloServer({
   typeDefs,
   resolvers,
-  cache: new RedisCache({
-    host: 'redis-server',
-    // Options are passed through to the Redis client
+  cache: new BaseRedisCache({
+    client: new Redis({
+      host: 'redis-server',
+    }),
   }),
   dataSources: () => ({
     moviesAPI: new MoviesAPI(),
@@ -30,19 +35,21 @@ const server = new ApolloServer({
 ### Sentinels
 
 ```js
-const { RedisCache } = require('apollo-server-cache-redis');
+const { BaseRedisCache } = require('apollo-server-cache-redis');
+const Redis = require('ioredis');
 
 const server = new ApolloServer({
   typeDefs,
   resolvers,
-  cache: new RedisCache({
-    sentinels: [{
-      host: 'sentinel-host-01',
-      port: 26379
-    }],
-    password: 'my_password',
-    name: 'service_name',
-    // Options are passed through to the Redis client
+  cache: new BaseRedisCache({
+    client: new Redis({
+      sentinels: [{
+        host: 'sentinel-host-01',
+        port: 26379
+      }],
+      password: 'my_password',
+      name: 'service_name',
+    }),
   }),
   dataSources: () => ({
     moviesAPI: new MoviesAPI(),
@@ -53,20 +60,23 @@ const server = new ApolloServer({
 ### Cluster
 
 ```js
-const { RedisClusterCache } = require('apollo-server-cache-redis');
+const { BaseRedisCache } = require('apollo-server-cache-redis');
+const Redis = require('ioredis');
 
 const server = new ApolloServer({
   typeDefs,
   resolvers,
-  cache: new RedisClusterCache(
-    [{
-      host: 'redis-node-01-host',
-      // Options are passed through to the Redis cluster client
-    }],
-    {
-      // Cluster options are passed through to the Redis cluster client
-    }
-  ),
+  cache: new BaseRedisCache({
+    clusterClient: new Redis.Cluster(
+      [{
+        host: 'redis-node-01-host',
+        // Options are passed through to the Redis cluster client
+      }],
+      {
+        // Redis cluster client options
+      }
+    ),
+  }),
   dataSources: () => ({
     moviesAPI: new MoviesAPI(),
   }),

diff --git a/packages/apollo-server-cache-redis/src/BaseRedisCache.ts b/packages/apollo-server-cache-redis/src/BaseRedisCache.ts
@@ -4,28 +4,70 @@ import {
 } 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>
+interface BaseRedisClient {
+  set: (
+    key: string,
+    value: string,
+    option?: string,
+    optionValue?: number,
+  ) => Promise<any>;
+  flushdb: () => Promise<any>;
+  del: (key: string) => Promise<number>;
+  quit: () => Promise<any>;
+}
+
+export interface RedisClient extends BaseRedisClient {
+  mget: (...key: Array<string>) => Promise<Array<string | null>>;
+}
+
+export interface RedisClusterClient extends BaseRedisClient {
+  get: (key: string) => Promise<string | null>;
+}
+
+/**
+ * Provide exactly one of the options `client` and `clusterClient`. `client` is
+ * a client that supports the `mget` multiple-get command.
+ *
+ * ioredis does not support `mget` for cluster mode (see
+ * https://github.com/luin/ioredis/issues/811), so if you're using cluster mode,
+ * pass `clusterClient` instead, which has a `get` method instead of `mget`;
+ * this package will issue parallel `get` commands instead of a single `mget`
+ * command if `clusterClient` is provided.
+ */
+export interface BaseRedisCacheOptions {
+  client?: RedisClient;
+  clusterClient?: RedisClusterClient;
 }
 
 export class BaseRedisCache implements TestableKeyValueCache<string> {
-  readonly client: RedisClient;
+  readonly client: BaseRedisClient;
   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,
-    });
+  constructor(options: BaseRedisCacheOptions) {
+    const { client, clusterClient } = options;
+    if (client && clusterClient) {
+      throw Error('You may only provide one of `client` and `clusterClient`');
+    } else if (client) {
+      this.client = client;
+      this.loader = new DataLoader((keys) => client.mget(...keys), {
+        cache: false,
+      });
+    } else if (clusterClient) {
+      this.client = clusterClient;
+      this.loader = new DataLoader(
+        (keys) =>
+          Promise.all(keys.map((key) => clusterClient.get(key).catch(() => null))),
+        {
+          cache: false,
+        },
+      );
+    } else {
+      throw Error('You must provide `client` or `clusterClient`');
+    }
   }
 
   async set(
@@ -52,7 +94,7 @@ export class BaseRedisCache implements TestableKeyValueCache<string> {
   }
 
   async delete(key: string): Promise<boolean> {
-    return await this.client.del(key) > 0;
+    return (await this.client.del(key)) > 0;
   }
 
   // Drops all data from Redis. This should only be used by test suites ---

diff --git a/packages/apollo-server-cache-redis/src/RedisCache.ts b/packages/apollo-server-cache-redis/src/RedisCache.ts
@@ -3,6 +3,6 @@ import { BaseRedisCache } from './BaseRedisCache';
 
 export class RedisCache extends BaseRedisCache {
   constructor(options?: RedisOptions) {
-    super(new Redis(options));
+    super({ client: new Redis(options) });
   }
 }
diff --git a/packages/apollo-server-cache-redis/src/RedisClusterCache.ts b/packages/apollo-server-cache-redis/src/RedisClusterCache.ts
@@ -9,16 +9,8 @@ export class RedisClusterCache extends BaseRedisCache {
   private readonly clusterClient: Redis.Cluster;
 
   constructor(nodes: ClusterNode[], options?: ClusterOptions) {
-    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),
-    });
+    const clusterClient = new Redis.Cluster(nodes, options);
+    super({ clusterClient });
     this.clusterClient = clusterClient;
   }
 

diff --git a/packages/apollo-server-cache-redis/src/__tests__/BaseRedisCache.test.ts b/packages/apollo-server-cache-redis/src/__tests__/BaseRedisCache.test.ts
@@ -1,31 +1,38 @@
-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 timeouts: NodeJS.Timer[] = [];
+  afterAll(() => {
+    timeouts.forEach((t) => clearTimeout(t));
+  });
   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]))),
+    set: jest.fn(
+      (key: string, value: string, option?: string, ttl?: number) => {
+        store[key] = value;
+        if (option === 'EX' && ttl) {
+          timeouts.push(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);
+  const cache = new BaseRedisCache({ client: testRedisClient });
   testKeyValueCache_Basics(cache);
   testKeyValueCache_Expiration(cache);
 });