apollo-caching: add PrefixingKeyValueCache, TestableKeyValueCache (#2433

)

apollo-server's caching uses the pattern of sharing a single KeyValueCache
object across multiple "features", using a prefix to ensure that the features'
data doesn't conflict. This change makes this pattern explicit rather than
implicit by introducing a PrefixingKeyValueCache class which handles the
prefixing for you.

In addition, it pulls the dangerous reset() operation out of KeyValueCache and
doesn't implement it in PrefixingKeyValueCache. Because reset is typically
implemented by sending a cache server a "drop all data" operation, and servers
may not implement "drop all data with a given prefix", we would not want actual
production code for a particular apollo-server cache-related feature to ever
call reset(). This PR moves that method (and close()) to a TestableKeyValueCache
interface, which is used by the test suite but not by the particular features
that need caches.

packages/apollo-datasource-rest/src/HTTPCache.ts

@@ -2,11 +2,21 @@ import { fetch, Request, Response, Headers } from 'apollo-server-env';
 
 import CachePolicy = require('http-cache-semantics');
 
-import { KeyValueCache, InMemoryLRUCache } from 'apollo-server-caching';
+import {
+  KeyValueCache,
+  InMemoryLRUCache,
+  PrefixingKeyValueCache,
+} from 'apollo-server-caching';
 import { CacheOptions } from './RESTDataSource';
 
 export class HTTPCache {
-  constructor(private keyValueCache: KeyValueCache = new InMemoryLRUCache()) {}
+  private keyValueCache: KeyValueCache;
+  constructor(keyValueCache: KeyValueCache = new InMemoryLRUCache()) {
+    this.keyValueCache = new PrefixingKeyValueCache(
+      keyValueCache,
+      'httpcache:',
+    );
+  }
 
   async fetch(
     request: Request,
@@ -19,7 +29,7 @@ export class HTTPCache {
   ): Promise<Response> {
     const cacheKey = options.cacheKey ? options.cacheKey : request.url;
 
-    const entry = await this.keyValueCache.get(`httpcache:${cacheKey}`);
+    const entry = await this.keyValueCache.get(cacheKey);
     if (!entry) {
       const response = await fetch(request);
 
@@ -122,7 +132,7 @@ export class HTTPCache {
       body,
     });
 
-    await this.keyValueCache.set(`httpcache:${cacheKey}`, entry, {
+    await this.keyValueCache.set(cacheKey, entry, {
       ttl,
     });
 

packages/apollo-server-cache-memcached/src/index.ts

@@ -1,8 +1,8 @@
-import { KeyValueCache } from 'apollo-server-caching';
+import { TestableKeyValueCache } from 'apollo-server-caching';
 import Memcached from 'memcached';
 import { promisify } from 'util';
 
-export class MemcachedCache implements KeyValueCache {
+export class MemcachedCache implements TestableKeyValueCache {
   // FIXME: Replace any with proper promisified type
   readonly client: any;
   readonly defaultSetOptions = {
@@ -37,6 +37,8 @@ export class MemcachedCache implements KeyValueCache {
     return await this.client.del(key);
   }
 
+  // Drops all data from Memcached. This should only be used by test suites ---
+  // production code should never drop all data from an end user cache.
   async flush(): Promise<void> {
     await this.client.flush();
   }

packages/apollo-server-cache-redis/src/index.ts

@@ -1,9 +1,9 @@
-import { KeyValueCache } from 'apollo-server-caching';
+import { TestableKeyValueCache } from 'apollo-server-caching';
 import Redis from 'redis';
 import { promisify } from 'util';
 import DataLoader from 'dataloader';
 
-export class RedisCache implements KeyValueCache<string> {
+export class RedisCache implements TestableKeyValueCache<string> {
   // FIXME: Replace any with proper promisified type
   readonly client: any;
   readonly defaultSetOptions = {
@@ -51,6 +51,8 @@ export class RedisCache implements KeyValueCache<string> {
     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();
   }

packages/apollo-server-caching/src/InMemoryLRUCache.ts

@@ -1,5 +1,5 @@
 import LRU from 'lru-cache';
-import { KeyValueCache } from './KeyValueCache';
+import { TestableKeyValueCache } from './KeyValueCache';
 
 function defaultLengthCalculation(item: any) {
   if (Array.isArray(item) || typeof item === 'string') {
@@ -11,7 +11,7 @@ function defaultLengthCalculation(item: any) {
   return 1;
 }
 
-export class InMemoryLRUCache<V = string> implements KeyValueCache<V> {
+export class InMemoryLRUCache<V = string> implements TestableKeyValueCache<V> {
   private store: LRU.Cache<string, V>;
 
   // FIXME: Define reasonable default max size of the cache
@@ -41,6 +41,9 @@ export class InMemoryLRUCache<V = string> implements KeyValueCache<V> {
   async delete(key: string) {
     this.store.del(key);
   }
+
+  // Drops all data from the cache. This should only be used by test suites ---
+  // production code should never drop all data from an end user cache.
   async flush(): Promise<void> {
     this.store.reset();
   }

packages/apollo-server-caching/src/KeyValueCache.ts

@@ -3,3 +3,12 @@ export interface KeyValueCache<V = string> {
   set(key: string, value: V, options?: { ttl?: number }): Promise<void>;
   delete(key: string): Promise<boolean | void>;
 }
+
+export interface TestableKeyValueCache<V = string> extends KeyValueCache<V> {
+  // Drops all data from the cache. This should only be used by test suites ---
+  // production code should never drop all data from an end user cache (and
+  // notably, PrefixingKeyValueCache intentionally doesn't implement this).
+  flush?(): Promise<void>;
+  // Close connections associated with this cache.
+  close?(): Promise<void>;
+}

packages/apollo-server-caching/src/PrefixingKeyValueCache.ts

@@ -0,0 +1,25 @@
+import { KeyValueCache } from './KeyValueCache';
+
+// PrefixingKeyValueCache wraps another cache and adds a prefix to all keys used
+// by all operations.  This allows multiple features to share the same
+// underlying cache without conflicts.
+//
+// Note that PrefixingKeyValueCache explicitly does not implement
+// TestableKeyValueCache, and notably does not implement the flush()
+// method. Most implementations of TestableKeyValueCache.flush() send a simple
+// command that wipes the entire backend cache system, which wouldn't support
+// "only wipe the part of the cache with this prefix", so trying to provide a
+// flush() method here could be confusingly dangerous.
+export class PrefixingKeyValueCache<V = string> implements KeyValueCache<V> {
+  constructor(private wrapped: KeyValueCache<V>, private prefix: string) {}
+
+  get(key: string) {
+    return this.wrapped.get(this.prefix + key);
+  }
+  set(key: string, value: V, options?: { ttl?: number }) {
+    return this.wrapped.set(this.prefix + key, value, options);
+  }
+  delete(key: string) {
+    return this.wrapped.delete(this.prefix + key);
+  }
+}

packages/apollo-server-caching/src/__tests__/PrefixingKeyValueCache.test.ts

@@ -0,0 +1,14 @@
+import { InMemoryLRUCache } from '../InMemoryLRUCache';
+import { PrefixingKeyValueCache } from '../PrefixingKeyValueCache';
+
+describe('PrefixingKeyValueCache', () => {
+  it('prefixes', async () => {
+    const inner = new InMemoryLRUCache();
+    const prefixing = new PrefixingKeyValueCache(inner, 'prefix:');
+    await prefixing.set('foo', 'bar');
+    expect(await prefixing.get('foo')).toBe('bar');
+    expect(await inner.get('prefix:foo')).toBe('bar');
+    await prefixing.delete('foo');
+    expect(await prefixing.get('foo')).toBe(undefined);
+  });
+});

packages/apollo-server-caching/src/__tests__/testsuite.ts

@@ -1,9 +1,10 @@
 import { advanceTimeBy, mockDate, unmockDate } from '__mocks__/date';
+import { TestableKeyValueCache } from '../';
 
-export function testKeyValueCache_Basics(keyValueCache: any) {
+export function testKeyValueCache_Basics(keyValueCache: TestableKeyValueCache) {
   describe('basic cache functionality', () => {
     beforeEach(() => {
-      keyValueCache.flush();
+      keyValueCache.flush && keyValueCache.flush();
     });
 
     it('can do a basic get and set', async () => {
@@ -21,20 +22,22 @@ export function testKeyValueCache_Basics(keyValueCache: any) {
   });
 }
 
-export function testKeyValueCache_Expiration(keyValueCache: any) {
+export function testKeyValueCache_Expiration(
+  keyValueCache: TestableKeyValueCache,
+) {
   describe('time-based cache expunging', () => {
     beforeAll(() => {
       mockDate();
       jest.useFakeTimers();
     });
 
     beforeEach(() => {
-      keyValueCache.flush();
+      keyValueCache.flush && keyValueCache.flush();
     });
 
     afterAll(() => {
       unmockDate();
-      keyValueCache.close();
+      keyValueCache.close && keyValueCache.close();
     });
 
     it('is able to expire keys based on ttl', async () => {
@@ -54,7 +57,7 @@ export function testKeyValueCache_Expiration(keyValueCache: any) {
   });
 }
 
-export function testKeyValueCache(keyValueCache: any) {
+export function testKeyValueCache(keyValueCache: TestableKeyValueCache) {
   describe('KeyValueCache Test Suite', () => {
     testKeyValueCache_Basics(keyValueCache);
     testKeyValueCache_Expiration(keyValueCache);

packages/apollo-server-caching/src/index.ts

@@ -1,2 +1,3 @@
-export { KeyValueCache } from './KeyValueCache';
+export { KeyValueCache, TestableKeyValueCache } from './KeyValueCache';
 export { InMemoryLRUCache } from './InMemoryLRUCache';
+export { PrefixingKeyValueCache } from './PrefixingKeyValueCache';

packages/apollo-server-core/src/ApolloServer.ts

@@ -16,7 +16,10 @@ import {
   DocumentNode,
 } from 'graphql';
 import { GraphQLExtension } from 'graphql-extensions';
-import { InMemoryLRUCache } from 'apollo-server-caching';
+import {
+  InMemoryLRUCache,
+  PrefixingKeyValueCache,
+} from 'apollo-server-caching';
 import { ApolloServerPlugin } from 'apollo-server-plugin-base';
 import runtimeSupportsUploads from './utils/runtimeSupportsUploads';
 
@@ -54,6 +57,7 @@ import {
   processGraphQLRequest,
   GraphQLRequestContext,
   GraphQLRequest,
+  APQ_CACHE_PREFIX,
 } from './requestPipeline';
 
 import { Headers } from 'apollo-server-env';
@@ -209,11 +213,14 @@ export class ApolloServerBase {
     }
 
     if (requestOptions.persistedQueries !== false) {
-      if (!requestOptions.persistedQueries) {
-        requestOptions.persistedQueries = {
-          cache: requestOptions.cache!,
-        };
-      }
+      requestOptions.persistedQueries = {
+        cache: new PrefixingKeyValueCache(
+          (requestOptions.persistedQueries &&
+            requestOptions.persistedQueries.cache) ||
+            requestOptions.cache!,
+          APQ_CACHE_PREFIX,
+        ),
+      };
     } else {
       // the user does not want to use persisted queries, so we remove the field
       delete requestOptions.persistedQueries;

packages/apollo-server-core/src/requestPipeline.ts

@@ -44,7 +44,11 @@ import {
 import { WithRequired } from 'apollo-server-env';
 
 import { Dispatcher } from './utils/dispatcher';
-import { InMemoryLRUCache, KeyValueCache } from 'apollo-server-caching';
+import {
+  InMemoryLRUCache,
+  KeyValueCache,
+  PrefixingKeyValueCache,
+} from 'apollo-server-caching';
 import { GraphQLParseOptions } from 'graphql-tools';
 
 export {
@@ -56,6 +60,8 @@ export {
 
 import createSHA from './utils/createSHA';
 
+export const APQ_CACHE_PREFIX = 'apq:';
+
 function computeQueryHash(query: string) {
   return createSHA('sha256')
     .update(query)
@@ -128,10 +134,21 @@ export async function processGraphQLRequest<TContext>(
     // do the write at a later point in the request pipeline processing.
     persistedQueryCache = config.persistedQueries.cache;
 
+    // This is a bit hacky, but if `config` came from direct use of the old
+    // apollo-server 1.0-style middleware (graphqlExpress etc, not via the
+    // ApolloServer class), it won't have been converted to
+    // PrefixingKeyValueCache yet.
+    if (!(persistedQueryCache instanceof PrefixingKeyValueCache)) {
+      persistedQueryCache = new PrefixingKeyValueCache(
+        persistedQueryCache,
+        APQ_CACHE_PREFIX,
+      );
+    }
+
     queryHash = extensions.persistedQuery.sha256Hash;
 
     if (query === undefined) {
-      query = await persistedQueryCache.get(`apq:${queryHash}`);
+      query = await persistedQueryCache.get(queryHash);
       if (query) {
         persistedQueryHit = true;
       } else {
@@ -268,7 +285,7 @@ export async function processGraphQLRequest<TContext>(
     // an error) and not actually write, we'll write to the cache if it was
     // determined earlier in the request pipeline that we should do so.
     if (persistedQueryRegister && persistedQueryCache) {
-      Promise.resolve(persistedQueryCache.set(`apq:${queryHash}`, query)).catch(
+      Promise.resolve(persistedQueryCache.set(queryHash, query)).catch(
         console.warn,
       );
     }