fix: canStale should return false if Cache-Control: must-revalidate i…

…s present #507
arthurfiorette · Apr 9, 2023 · 22870d7 · 22870d7
1 parent 9bb2918
commit 22870d7
Show file tree

Hide file tree

Showing 5 changed files with 92 additions and 44 deletions.
diff --git a/src/header/interpreter.ts b/src/header/interpreter.ts
@@ -8,8 +8,9 @@ export const defaultHeaderInterpreter: HeaderInterpreter = (headers) => {
   const cacheControl: unknown = headers[Header.CacheControl];
 
   if (cacheControl) {
-    const { noCache, noStore, mustRevalidate, maxAge, immutable, staleWhileRevalidate } =
-      parse(String(cacheControl));
+    const { noCache, noStore, maxAge, immutable, staleWhileRevalidate } = parse(
+      String(cacheControl)
+    );
 
     // Header told that this response should not be cached.
     if (noCache || noStore) {
@@ -19,23 +20,22 @@ export const defaultHeaderInterpreter: HeaderInterpreter = (headers) => {
     if (immutable) {
       // 1 year is sufficient, as Infinity may cause problems with certain storages.
       // It might not be the best way, but a year is better than none.
-      return { cache: 1000 * 60 * 60 * 24 * 365 };
-    }
-
-    // Already out of date, for cache can be saved, but must be requested again
-    const stale = staleWhileRevalidate !== undefined ? staleWhileRevalidate * 1000 : 0;
-    if (mustRevalidate) {
-      return { cache: 0, stale };
+      return {
+        cache: 1000 * 60 * 60 * 24 * 365
+      };
     }
 
     if (maxAge !== undefined) {
       const age: unknown = headers[Header.Age];
 
-      if (!age) {
-        return { cache: maxAge * 1000, stale };
-      }
-
-      return { cache: (maxAge - Number(age)) * 1000, stale };
+      return {
+        cache: age
+          ? // If age is present, we must subtract it from maxAge
+            (maxAge - Number(age)) * 1000
+          : maxAge * 1000,
+        // Already out of date, for cache can be saved, but must be requested again
+        stale: staleWhileRevalidate !== undefined ? staleWhileRevalidate * 1000 : 0
+      };
     }
   }
 

diff --git a/src/storage/build.ts b/src/storage/build.ts
@@ -27,6 +27,15 @@ function hasUniqueIdentifierHeader(
 
 /** Returns true if this has sufficient properties to stale instead of expire. */
 export function canStale(value: CachedStorageValue): boolean {
+  // Must revalidate is a special case and should not be staled
+  if (
+    String(value.data.headers[Header.CacheControl])
+      // We could use cache-control's parse function, but this is way faster and simpler
+      .includes('must-revalidate')
+  ) {
+    return false;
+  }
+
   if (hasUniqueIdentifierHeader(value)) {
     return true;
   }

diff --git a/test/header/cache-control.test.ts b/test/header/cache-control.test.ts
@@ -19,7 +19,7 @@ describe('test Cache-Control header', () => {
       [Header.CacheControl]: 'must-revalidate'
     });
 
-    expect(mustRevalidate).toEqual({ cache: 0, stale: 0 });
+    expect(mustRevalidate).toEqual('not enough headers');
   });
 
   it('tests with maxAge header for 10 seconds', () => {
@@ -38,4 +38,13 @@ describe('test Cache-Control header', () => {
 
     expect(result).toEqual({ cache: 0, stale: 0 });
   });
+
+  it('tests stale values with age', () => {
+    const result = defaultHeaderInterpreter({
+      [Header.CacheControl]: 'max-age=10, stale-while-revalidate=5',
+      [Header.Age]: '5'
+    });
+
+    expect(result).toEqual({ cache: 5 * 1000, stale: 5 * 1000 });
+  });
 });
diff --git a/test/interceptors/last-modified.test.ts b/test/interceptors/last-modified.test.ts
@@ -1,3 +1,4 @@
+import type { CacheRequestConfig } from '../../src';
 import { Header } from '../../src/header/headers';
 import { mockAxios, XMockRandom } from '../mocks/axios';
 import { sleep } from '../utils';
@@ -12,18 +13,21 @@ describe('Last-Modified handling', () => {
       }
     );
 
-    const config = { cache: { interpretHeader: true, modifiedSince: true } };
+    const config: CacheRequestConfig = {
+      id: 'same request',
+      cache: { interpretHeader: true, modifiedSince: true }
+    };
 
-    await axios.get('http://test.com', config);
+    await axios.get('url', config);
 
-    const response = await axios.get('http://test.com', config);
+    const response = await axios.get('url', config);
     expect(response.cached).toBe(true);
     expect(response.data).toBe(true);
 
     // Sleep entire max age time.
     await sleep(1000);
 
-    const response2 = await axios.get('http://test.com', config);
+    const response2 = await axios.get('url', config);
     // from revalidation
     expect(response2.cached).toBe(true);
     expect(response2.status).toBe(200);
@@ -38,16 +42,16 @@ describe('Last-Modified handling', () => {
       }
     );
 
-    await axios.get('http://test.com');
+    await axios.get('url');
 
-    const response = await axios.get('http://test.com');
+    const response = await axios.get('url');
     expect(response.cached).toBe(true);
     expect(response.data).toBe(true);
 
     // Sleep entire max age time.
     await sleep(1000);
 
-    const response2 = await axios.get('http://test.com');
+    const response2 = await axios.get('url');
     // from revalidation
     expect(response2.cached).toBe(true);
     expect(response2.status).toBe(200);
@@ -56,17 +60,18 @@ describe('Last-Modified handling', () => {
   it('tests modifiedSince as date', async () => {
     const axios = mockAxios({ ttl: 0 });
 
-    const config = {
+    const config: CacheRequestConfig = {
+      id: 'same request',
       cache: { modifiedSince: new Date(2014, 1, 1) }
     };
 
-    const response = await axios.get('http://test.com', config);
+    const response = await axios.get('url', config);
     expect(response.cached).toBe(false);
     expect(response.data).toBe(true);
     expect(response.config.headers?.[Header.IfModifiedSince]).toBeUndefined();
     expect(response.headers?.[Header.XAxiosCacheLastModified]).toBeDefined();
 
-    const response2 = await axios.get('http://test.com', config);
+    const response2 = await axios.get('url', config);
     expect(response2.cached).toBe(true);
     expect(response2.data).toBe(true);
     expect(response2.config.headers?.[Header.IfModifiedSince]).toBeDefined();
@@ -77,22 +82,25 @@ describe('Last-Modified handling', () => {
     const axios = mockAxios(
       {},
       {
-        'cache-control': 'must-revalidate'
+        [Header.CacheControl]: 'max-age=0',
+        // etag is a header to makes a response able to stale
+        [Header.ETag]: 'W/123'
       }
     );
 
-    const config = {
+    const config: CacheRequestConfig = {
+      id: 'same request',
       cache: { interpretHeader: true, modifiedSince: true }
     };
 
-    await axios.get('http://test.com', config);
-    const response = await axios.get('http://test.com', config);
+    // pre caches
+    await axios.get('url', config);
 
+    const response = await axios.get('url', config);
     const modifiedSince = response.config.headers?.[Header.IfModifiedSince] as string;
 
-    if (!modifiedSince) {
-      throw new Error('modifiedSince is not defined');
-    }
+    expect(modifiedSince).toBeDefined();
+
     const milliseconds = Date.parse(modifiedSince);
 
     expect(typeof milliseconds).toBe('number');
@@ -103,14 +111,14 @@ describe('Last-Modified handling', () => {
     const axios = mockAxios();
 
     // First request, return x-my-header. Ttl 1 to make the cache stale
-    const firstResponse = await axios.get('http://test.com', { cache: { ttl: -1 } });
+    const firstResponse = await axios.get('url', { cache: { ttl: -1 } });
     const firstMyHeader: unknown = firstResponse.headers?.[XMockRandom];
 
     expect(firstMyHeader).toBeDefined();
     expect(Number(firstMyHeader)).not.toBeNaN();
 
     // Second request with 304 Not Modified
-    const secondResponse = await axios.get('http://test.com', {
+    const secondResponse = await axios.get('url', {
       cache: { modifiedSince: true }
     });
     const secondMyHeader: unknown = secondResponse.headers?.[XMockRandom];

diff --git a/test/interceptors/request.test.ts b/test/interceptors/request.test.ts
@@ -100,7 +100,7 @@ describe('test request interceptor', () => {
   it('test cache expiration', async () => {
     const axios = mockAxios(
       {},
-      { 'cache-control': 'max-age=1,stale-while-revalidate=10' }
+      { [Header.CacheControl]: 'max-age=1,stale-while-revalidate=10' }
     );
 
     await axios.get('http://test.com', { cache: { interpretHeader: true } });
@@ -115,17 +115,39 @@ describe('test request interceptor', () => {
     expect(response2.cached).toBe(false);
   });
 
-  it('tests "must revalidate" handling without any headers to do so', async () => {
-    const axios = mockAxios({}, { 'cache-control': 'must-revalidate' });
-    const config = { cache: { interpretHeader: true } };
-    await axios.get('http://test.com', config);
+  test('"must revalidate" does not allows stale', async () => {
+    const axios = mockAxios(
+      {},
+      {
+        [Header.CacheControl]: 'must-revalidate, max-age=1',
+        // etag is a header that should make the cache stale
+        [Header.ETag]: 'W/123'
+      }
+    );
 
-    // 0ms cache
-    await sleep(1);
+    const config: CacheRequestConfig = {
+      id: 'req-id',
+      cache: {
+        interpretHeader: true,
+        etag: true
+      }
+    };
 
-    const response = await axios.get('http://test.com', config);
-    // nothing to use for revalidation
-    expect(response.cached).toBe(false);
+    const res1 = await axios.get('url', config);
+    const res2 = await axios.get('url', config);
+    const res3 = await axios.get('url', config);
+
+    expect(res1.cached).toBeFalsy();
+    expect(res2.cached).toBeTruthy();
+    expect(res3.cached).toBeTruthy();
+
+    // waits one second
+    await sleep(1000);
+
+    const res4 = await axios.get('url', config);
+
+    // Should be false because the cache couldn't be stale
+    expect(res4.cached).toBeFalsy();
   });
 
   it("expect two requests with different body aren't cached", async () => {