Extend cache TTL for responses with last-modified (#106)

Some cache entries can be used even after they "expire", if they have a
response header like "etag" or "last-modified" that lets you run a
special HTTP request that can get a 304 response (to mean "reuse the
data you have" rather than needing to fully send it again). So we have
special logic to keep those responses in our cache for longer... but
that logic only looks for etag, not last-modified. So now we look for
both.

As part of this, add a bunch of related comments and documentation.

In order to make the test of this functionality fail without the fix, it
needs to use a KeyValueCache that actually observes TTL, so switch to
the one we use in apollo-server. Note that this reveals that some
existing tests were actually reading cache values after TTL which
resulted in some minor changes like `age: 0` instead of missing `age`.

Fixes #34.

.changeset/eight-moles-beam.md

@@ -0,0 +1,5 @@
+---
+'@apollo/datasource-rest': minor
+---
+
+Previously, RESTDataSource doubled the TTL used with its shared header-sensitive cache when it may be able to use the cache entry after it goes stale because it contained the `ETag` header; for these cache entries, RESTDataSource can set the `If-None-Match` header when sending the REST request and the server can return a 304 response telling RESTDataSource to reuse the old response from its cache. Now, RESTDataSource also extends the TTL for responses with the `Last-Modified` header (which it can validate with `If-Modified-Since`).

README.md

@@ -144,7 +144,7 @@ will wait until the promise is completed to continue executing the request. See
 the [intercepting fetches](#intercepting-fetches) section for usage examples.
 
 ##### `cacheOptionsFor`
-Allows setting the `CacheOptions` to be used for each request/response in the HTTPCache. This is separate from the request-only cache. You can use this to set the TTL.
+Allows setting the `CacheOptions` to be used for each request/response in the HTTPCache. This is separate from the request-only cache. You can use this to set the TTL to a value in seconds. If you return `{ttl: 0}`, the response will not be stored. If you return a positive number for `ttl` and the operation returns a 2xx status code, then the response *will* be cached, regardless of HTTP headers or method: make sure this is what you intended! (There is currently no way to say "only cache responses that should be cached according to HTTP headers, but change the TTL to something specific".) Note that if you do not specify `ttl` here, only `GET` requests are cached.
 
 ```javascript
 override cacheOptionsFor() {

cspell-dict.txt

@@ -10,6 +10,7 @@ codesandbox
 concat
 datasource
 direnv
+Fakeable
 falsey
 httpcache
 isplainobject

src/HTTPCache.ts

@@ -47,6 +47,8 @@ export class HTTPCache {
 
     const entry = await this.keyValueCache.get(cacheKey);
     if (!entry) {
+      // There's nothing in our cache. Fetch the URL and save it to the cache if
+      // we're allowed.
       const response = await this.httpFetch(urlString, requestOpts);
 
       const policy = new CachePolicy(
@@ -80,13 +82,32 @@ export class HTTPCache {
           policyRequestFrom(urlString, requestOpts),
         ))
     ) {
+      // Either the cache entry was created with an explicit TTL override (ie,
+      // `ttl` returned from `cacheOptionsFor`) and we're within that TTL, or
+      // the cache entry was not created with an explicit TTL override and the
+      // header-based cache policy says we can safely use the cached response.
       const headers = policy.responseHeaders();
       return new Response(body, {
         url: urlFromPolicy,
         status: policy._status,
         headers,
       });
     } else {
+      // We aren't sure that we're allowed to use the cached response, so we are
+      // going to actually do a fetch. However, we may have one extra trick up
+      // our sleeve. If the cached response contained an `etag` or
+      // `last-modified` header, then we can add an appropriate `if-none-match`
+      // or `if-modified-since` header to the request. If what we're fetching
+      // hasn't changed, then the server can return a small 304 response instead
+      // of a large 200, and we can use the body from our cache. This logic is
+      // implemented inside `policy.revalidationHeaders`; we support it by
+      // setting a larger KeyValueCache TTL for responses with these headers
+      // (see `canBeRevalidated`).  (If the cached response doesn't have those
+      // headers, we'll just end up fetching the normal request here.)
+      //
+      // Note that even if we end up able to reuse the cached body here, we
+      // still re-write to the cache, because we might need to update the TTL or
+      // other aspects of the cache policy based on the headers we got back.
       const revalidationHeaders = policy.revalidationHeaders(
         policyRequestFrom(urlString, requestOpts),
       );
@@ -154,8 +175,10 @@ export class HTTPCache {
         : ttlOverride;
     if (ttl <= 0) return response;
 
-    // If a response can be revalidated, we don't want to remove it from the cache right after it expires.
-    // We may be able to use better heuristics here, but for now we'll take the max-age times 2.
+    // If a response can be revalidated, we don't want to remove it from the
+    // cache right after it expires. (See the comment above the call to
+    // `revalidationHeaders` for details.) We may be able to use better
+    // heuristics here, but for now we'll take the max-age times 2.
     if (canBeRevalidated(response)) {
       ttl *= 2;
     }
@@ -185,7 +208,7 @@ export class HTTPCache {
 }
 
 function canBeRevalidated(response: FetcherResponse): boolean {
-  return response.headers.has('ETag');
+  return response.headers.has('ETag') || response.headers.has('Last-Modified');
 }
 
 function policyRequestFrom(url: string, request: RequestOptions) {

src/__tests__/FakeableTTLTestingCache.ts

@@ -0,0 +1,50 @@
+// The default AS cache (`InMemoryLRUCache`) uses `lru-cache` internally, which
+// we've had issues mocking timers for. Presumably this has something to do with
+// the way that `lru-cache` grabs its `perf` function:
+// https://github.com/isaacs/node-lru-cache/blob/118a078cc0ea3a17f7b2ff4caf04e6aa3a33b136/index.js#L1-L6
+// This test suite already mocks `Date` (because
+// `ApolloServerPluginResponseCache` uses it internally), so we used that for
+// time calculation in this mock cache as well.
+//
+// (Borrowed from apollo-server.)
+export class FakeableTTLTestingCache {
+  constructor(
+    private cache: Map<
+      string,
+      { value: string; deadline: number | null }
+    > = new Map(),
+  ) {}
+
+  async get(key: string) {
+    const entry = this.cache.get(key);
+    if (!entry) return undefined;
+    if (entry.deadline && entry.deadline <= Date.now()) {
+      await this.delete(key);
+      return undefined;
+    }
+    return entry.value;
+  }
+
+  async set(
+    key: string,
+    value: string,
+    { ttl }: { ttl: number | null } = { ttl: null },
+  ) {
+    this.cache.set(key, {
+      value,
+      deadline: ttl ? Date.now() + ttl * 1000 : null,
+    });
+  }
+
+  async delete(key: string) {
+    this.cache.delete(key);
+  }
+
+  isEmpty() {
+    // Trigger the get()-time TTL cleanup.
+    for (const key of this.cache.keys()) {
+      this.get(key);
+    }
+    return this.cache.size === 0;
+  }
+}

src/__tests__/HTTPCache.test.ts

@@ -1,16 +1,16 @@
 import fetch from 'node-fetch';
 import nock from 'nock';
 import { HTTPCache } from '../HTTPCache';
-import { MapKeyValueCache } from './MapKeyValueCache';
 import { nockAfterEach, nockBeforeEach } from './nockAssertions';
+import { FakeableTTLTestingCache } from './FakeableTTLTestingCache';
 
 describe('HTTPCache', () => {
-  let store: MapKeyValueCache<string>;
+  let store: FakeableTTLTestingCache;
   let httpCache: HTTPCache;
 
   beforeEach(() => {
     nockBeforeEach();
-    store = new MapKeyValueCache<string>();
+    store = new FakeableTTLTestingCache();
     httpCache = new HTTPCache(store, fetch);
   });
 
@@ -92,7 +92,7 @@ describe('HTTPCache', () => {
     );
 
     expect(await response.json()).toEqual({ name: 'Alan Turing' });
-    expect(response.headers.get('age')).toEqual('0');
+    expect(response.headers.get('age')).toBeNull();
   });
 
   describe('overriding TTL', () => {
@@ -148,7 +148,7 @@ describe('HTTPCache', () => {
       );
 
       expect(await response.json()).toEqual({ name: 'Alan Turing' });
-      expect(response.headers.get('age')).toEqual('0');
+      expect(response.headers.get('age')).toBeNull();
     });
 
     it('fetches a fresh response from the origin when the overridden TTL expired even if a longer max-age has been specified', async () => {
@@ -175,7 +175,7 @@ describe('HTTPCache', () => {
       );
 
       expect(await response.json()).toEqual({ name: 'Alan Turing' });
-      expect(response.headers.get('age')).toEqual('0');
+      expect(response.headers.get('age')).toBeNull();
     });
 
     it('does not store a response with an overridden TTL and a non-success status code', async () => {
@@ -191,7 +191,7 @@ describe('HTTPCache', () => {
         },
       );
 
-      expect(store.size).toEqual(0);
+      expect(store.isEmpty()).toBe(true);
     });
 
     it('allows overriding the TTL dynamically', async () => {
@@ -231,7 +231,7 @@ describe('HTTPCache', () => {
         },
       );
 
-      expect(store.size).toEqual(0);
+      expect(store.isEmpty()).toBe(true);
     });
   });
 
@@ -263,31 +263,31 @@ describe('HTTPCache', () => {
 
     await httpCache.fetch(adaUrl, { method: 'POST' });
 
-    expect(store.size).toEqual(0);
+    expect(store.isEmpty()).toBe(true);
   });
 
   it('does not store a response with a non-success status code', async () => {
     mockInternalServerError({ 'cache-control': 'max-age=30' });
 
     await httpCache.fetch(adaUrl);
 
-    expect(store.size).toEqual(0);
+    expect(store.isEmpty()).toBe(true);
   });
 
   it('does not store a response without cache-control header', async () => {
     mockGetAdaLovelace();
 
     await httpCache.fetch(adaUrl);
 
-    expect(store.size).toEqual(0);
+    expect(store.isEmpty()).toBe(true);
   });
 
   it('does not store a private response', async () => {
     mockGetAdaLovelace({ 'cache-control': 'private, max-age: 60' });
 
     await httpCache.fetch(adaUrl);
 
-    expect(store.size).toEqual(0);
+    expect(store.isEmpty()).toBe(true);
   });
 
   it('returns a cached response when vary header fields match', async () => {
@@ -360,15 +360,25 @@ describe('HTTPCache', () => {
     storeSet.mockRestore();
   });
 
-  it('revalidates a cached response when expired and returns the cached response when not modified', async () => {
+  it('revalidates a cached response when expired and returns the cached response when not modified via etag', async () => {
     mockGetAdaLovelace({
       'cache-control': 'public, max-age=30',
       etag: 'foo',
     });
 
-    await httpCache.fetch(adaUrl);
+    const response0 = await httpCache.fetch(adaUrl);
+    expect(response0.status).toEqual(200);
+    expect(await response0.json()).toEqual({ name: 'Ada Lovelace' });
+    expect(response0.headers.get('age')).toBeNull();
 
-    jest.advanceTimersByTime(30000);
+    jest.advanceTimersByTime(10000);
+
+    const response1 = await httpCache.fetch(adaUrl);
+    expect(response1.status).toEqual(200);
+    expect(await response1.json()).toEqual({ name: 'Ada Lovelace' });
+    expect(response1.headers.get('age')).toEqual('10');
+
+    jest.advanceTimersByTime(21000);
 
     nock(apiUrl)
       .get(adaPath)
@@ -393,6 +403,49 @@ describe('HTTPCache', () => {
     expect(response2.headers.get('age')).toEqual('10');
   });
 
+  it('revalidates a cached response when expired and returns the cached response when not modified via last-modified', async () => {
+    mockGetAdaLovelace({
+      'cache-control': 'public, max-age=30',
+      'last-modified': 'Wed, 21 Oct 2015 07:28:00 GMT',
+    });
+
+    const response0 = await httpCache.fetch(adaUrl);
+    expect(response0.status).toEqual(200);
+    expect(await response0.json()).toEqual({ name: 'Ada Lovelace' });
+    expect(response0.headers.get('age')).toBeNull();
+
+    jest.advanceTimersByTime(10000);
+
+    const response1 = await httpCache.fetch(adaUrl);
+    expect(response1.status).toEqual(200);
+    expect(await response1.json()).toEqual({ name: 'Ada Lovelace' });
+    expect(response1.headers.get('age')).toEqual('10');
+
+    jest.advanceTimersByTime(21000);
+
+    nock(apiUrl)
+      .get(adaPath)
+      .matchHeader('if-modified-since', 'Wed, 21 Oct 2015 07:28:00 GMT')
+      .reply(304, undefined, {
+        'cache-control': 'public, max-age=30',
+        'last-modified': 'Wed, 21 Oct 2015 07:28:00 GMT',
+      });
+
+    const response = await httpCache.fetch(adaUrl);
+
+    expect(response.status).toEqual(200);
+    expect(await response.json()).toEqual({ name: 'Ada Lovelace' });
+    expect(response.headers.get('age')).toEqual('0');
+
+    jest.advanceTimersByTime(10000);
+
+    const response2 = await httpCache.fetch(adaUrl);
+
+    expect(response2.status).toEqual(200);
+    expect(await response2.json()).toEqual({ name: 'Ada Lovelace' });
+    expect(response2.headers.get('age')).toEqual('10');
+  });
+
   it('revalidates a cached response when expired and returns and caches a fresh response when modified', async () => {
     mockGetAdaLovelace({
       'cache-control': 'public, max-age=30',
@@ -430,7 +483,7 @@ describe('HTTPCache', () => {
     mockGetAdaLovelace();
 
     const customFetch = jest.fn(fetch);
-    const customHttpCache = new HTTPCache(store as any, customFetch);
+    const customHttpCache = new HTTPCache(store, customFetch);
 
     const response = await customHttpCache.fetch(adaUrl);