[must-revalidate] properly revalidate based on eTag (#824)

* [must-revalidate] implement logic to properly revalidate requests with must-revalidate cache-control header * [lint] fix sorting of imports * [pull 824] apply feedback --------- Co-authored-by: Edwin Veldhuizen <[email protected]>
arthurfiorette · Apr 23, 2024 · 6cba59c · 6cba59c
1 parent b8f30b5
commit 6cba59c
Show file tree

Hide file tree

Showing 7 changed files with 102 additions and 62 deletions.
diff --git a/src/cache/cache.ts b/src/cache/cache.ts
@@ -207,7 +207,9 @@ export interface CacheProperties<R = unknown, D = unknown> {
     | undefined
     | ((
         cache:
-          | (LoadingStorageValue & { previous: 'stale' })
+          | (LoadingStorageValue & {
+              previous: 'stale' | 'must-revalidate';
+            })
           | CachedStorageValue
           | StaleStorageValue
       ) => void | Promise<void>);

diff --git a/src/interceptors/request.ts b/src/interceptors/request.ts
@@ -85,7 +85,12 @@ export function defaultRequestInterceptor(axios: AxiosCacheInstance) {
 
     // Not cached, continue the request, and mark it as fetching
     // biome-ignore lint/suspicious/noConfusingLabels: required to break condition in simultaneous accesses
-    ignoreAndRequest: if (cache.state === 'empty' || cache.state === 'stale' || overrideCache) {
+    ignoreAndRequest: if (
+      cache.state === 'empty' ||
+      cache.state === 'stale' ||
+      cache.state === 'must-revalidate' ||
+      overrideCache
+    ) {
       // This checks for simultaneous access to a new key. The js event loop jumps on the
       // first await statement, so the second (asynchronous call) request may have already
       // started executing.
@@ -98,7 +103,7 @@ export function defaultRequestInterceptor(axios: AxiosCacheInstance) {
         // say by a `axios.storage.delete(key)` and has a concurrent loading request.
         // Because in this case, the cache will be empty and may still has a pending key
         // on waiting map.
-        if (cache.state !== 'empty') {
+        if (cache.state !== 'empty' && cache.state !== 'must-revalidate') {
           if (__ACI_DEV__) {
             axios.debug({
               id: config.id,
@@ -128,7 +133,7 @@ export function defaultRequestInterceptor(axios: AxiosCacheInstance) {
               ? 'stale'
               : 'empty'
             : // Typescript doesn't know that cache.state here can only be 'empty' or 'stale'
-              (cache.state as 'stale'),
+              (cache.state as 'stale' | 'must-revalidate'),
 
           data: cache.data as any,
 
@@ -138,7 +143,7 @@ export function defaultRequestInterceptor(axios: AxiosCacheInstance) {
         config
       );
 
-      if (cache.state === 'stale') {
+      if (cache.state === 'stale' || cache.state === 'must-revalidate') {
         updateStaleRequest(cache, config as ConfigWithCache<unknown>);
 
         if (__ACI_DEV__) {
@@ -163,7 +168,7 @@ export function defaultRequestInterceptor(axios: AxiosCacheInstance) {
       }
 
       // Hydrates any UI temporarily, if cache is available
-      if (cache.state === 'stale' || cache.data) {
+      if (cache.state === 'stale' || (cache.data && cache.state !== 'must-revalidate')) {
         await config.cache.hydrate?.(cache);
       }
 

diff --git a/src/interceptors/util.ts b/src/interceptors/util.ts
@@ -2,7 +2,11 @@ import type { Method } from 'axios';
 import type { CacheAxiosResponse, CacheRequestConfig } from '../cache/axios.js';
 import type { CacheProperties } from '../cache/cache.js';
 import { Header } from '../header/headers.js';
-import type { CachedResponse, StaleStorageValue } from '../storage/types.js';
+import type {
+  CachedResponse,
+  MustRevalidateStorageValue,
+  StaleStorageValue
+} from '../storage/types.js';
 
 /**
  * Creates a new validateStatus function that will use the one already used and also
@@ -33,7 +37,10 @@ export interface ConfigWithCache<D> extends CacheRequestConfig<unknown, D> {
  * This function updates the cache when the request is stale. So, the next request to the
  * server will be made with proper header / settings.
  */
-export function updateStaleRequest<D>(cache: StaleStorageValue, config: ConfigWithCache<D>): void {
+export function updateStaleRequest<D>(
+  cache: StaleStorageValue | MustRevalidateStorageValue,
+  config: ConfigWithCache<D>
+): void {
   config.headers ||= {};
 
   const { etag, modifiedSince } = config.cache;

diff --git a/src/storage/build.ts b/src/storage/build.ts
@@ -18,17 +18,15 @@ function hasUniqueIdentifierHeader(value: CachedStorageValue | StaleStorageValue
   );
 }
 
+/** Returns true if value must be revalidated */
+export function mustRevalidate(value: CachedStorageValue | StaleStorageValue): boolean {
+  // Must revalidate is a special case and should not serve stale values
+  // We could use cache-control's parse function, but this is way faster and simpler
+  return String(value.data.headers[Header.CacheControl]).includes('must-revalidate');
+}
+
 /** 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;
   }
@@ -48,7 +46,7 @@ export function canStale(value: CachedStorageValue): boolean {
 
 /**
  * Checks if the provided cache is expired. You should also check if the cache
- * {@link canStale}
+ * {@link canStale} and {@link mayUseStale}
  */
 export function isExpired(value: CachedStorageValue | StaleStorageValue): boolean {
   return value.ttl !== undefined && value.createdAt + value.ttl <= Date.now();
@@ -105,7 +103,11 @@ export function buildStorage({ set, find, remove }: BuildStorage): AxiosStorage
         return { state: 'empty' };
       }
 
-      if (value.state === 'empty' || value.state === 'loading') {
+      if (
+        value.state === 'empty' ||
+        value.state === 'loading' ||
+        value.state === 'must-revalidate'
+      ) {
         return value;
       }
 
@@ -129,6 +131,11 @@ export function buildStorage({ set, find, remove }: BuildStorage): AxiosStorage
         };
 
         await set(key, value, config);
+
+        // Must revalidate is a special case and should not serve stale values
+        if (mustRevalidate(value)) {
+          return { ...value, state: 'must-revalidate' };
+        }
       }
 
       // A second check in case the new stale value was created already expired.

diff --git a/src/storage/types.ts b/src/storage/types.ts
@@ -13,7 +13,8 @@ export type StorageValue =
   | StaleStorageValue
   | CachedStorageValue
   | LoadingStorageValue
-  | EmptyStorageValue;
+  | EmptyStorageValue
+  | MustRevalidateStorageValue;
 
 export type NotEmptyStorageValue = Exclude<StorageValue, EmptyStorageValue>;
 
@@ -25,6 +26,14 @@ export interface StaleStorageValue {
   state: 'stale';
 }
 
+export interface MustRevalidateStorageValue {
+  data: CachedResponse;
+  ttl?: number;
+  staleTtl?: undefined;
+  createdAt: number;
+  state: 'must-revalidate';
+}
+
 export interface CachedStorageValue {
   data: CachedResponse;
   /**
@@ -37,7 +46,10 @@ export interface CachedStorageValue {
   state: 'cached';
 }
 
-export type LoadingStorageValue = LoadingEmptiedStorageValue | LoadingStaledStorageValue;
+export type LoadingStorageValue =
+  | LoadingEmptiedStorageValue
+  | LoadingStaledStorageValue
+  | LoadingRevalidateStorageValue;
 
 export interface LoadingEmptiedStorageValue {
   data?: undefined;
@@ -57,6 +69,15 @@ export interface LoadingStaledStorageValue {
   previous: 'stale';
 }
 
+export interface LoadingRevalidateStorageValue {
+  state: 'loading';
+  data: CachedResponse;
+  ttl?: undefined;
+  staleTtl?: undefined;
+  createdAt: number;
+  previous: 'must-revalidate';
+}
+
 export interface EmptyStorageValue {
   data?: undefined;
   ttl?: undefined;

diff --git a/test/interceptors/request.test.ts b/test/interceptors/request.test.ts
@@ -89,17 +89,23 @@ describe('Request Interceptor', () => {
     const response2 = await axios.get('http://test.com');
     assert.ok(response2.cached);
 
-    const response3 = await axios.get('http://test.com', { id: 'random-id' });
+    const response3 = await axios.get('http://test.com', {
+      id: 'random-id'
+    });
     assert.equal(response3.cached, false);
 
-    const response4 = await axios.get('http://test.com', { id: 'random-id' });
+    const response4 = await axios.get('http://test.com', {
+      id: 'random-id'
+    });
     assert.ok(response4.cached);
   });
 
   it('Cache expiration', async () => {
     const axios = mockAxios({}, { [Header.CacheControl]: 'max-age=1,stale-while-revalidate=10' });
 
-    await axios.get('http://test.com', { cache: { interpretHeader: true } });
+    await axios.get('http://test.com', {
+      cache: { interpretHeader: true }
+    });
 
     const resultCache = await axios.get('http://test.com');
     assert.ok(resultCache.cached);
@@ -134,6 +140,9 @@ describe('Request Interceptor', () => {
     const res3 = await axios.get('url', config);
 
     assert.equal(res1.cached, false);
+    const headers1 = res1.headers as Record<string, string>;
+    const headers2 = res2.headers as Record<string, string>;
+    assert.equal(headers1['x-mock-random'], headers2['x-mock-random']);
     assert.ok(res2.cached);
     assert.ok(res3.cached);
 
@@ -142,8 +151,9 @@ describe('Request Interceptor', () => {
 
     const res4 = await axios.get('url', config);
 
-    // Should be false because the cache couldn't be stale
-    assert.equal(res4.cached, false);
+    // Should be different because the call it may not serve stale
+    const headers4 = res4.headers as Record<string, string>;
+    assert.notEqual(headers1['x-mock-random'], headers4['x-mock-random']);
   });
 
   it('Expects two requests with different body are not cached', async () => {

diff --git a/test/storage/storages.test.ts b/test/storage/storages.test.ts
@@ -1,9 +1,9 @@
 import assert from 'node:assert';
 import { describe, it } from 'node:test';
 import { Axios } from 'axios';
-import { buildStorage, canStale, isStorage } from '../../src/storage/build.js';
+import { buildStorage, canStale, isStorage, mustRevalidate } from '../../src/storage/build.js';
 import { buildMemoryStorage } from '../../src/storage/memory.js';
-import type { AxiosStorage, StorageValue } from '../../src/storage/types.js';
+import type { AxiosStorage, CachedStorageValue, StorageValue } from '../../src/storage/types.js';
 import { buildWebStorage } from '../../src/storage/web-api.js';
 import { localStorage } from '../dom.js';
 import { mockAxios } from '../mocks/axios.js';
@@ -150,41 +150,29 @@ describe('General storage functions', () => {
   });
 
   it('canStale() function with MustRevalidate', () => {
-    // Normal request, but without must-revalidate
-    assert.ok(
-      canStale({
-        data: {
-          headers: {
-            'Cache-Control': 'max-age=1'
-          },
-          data: true,
-          status: 200,
-          statusText: 'OK'
+    // Normal request, without must-revalidate
+    const entry: CachedStorageValue = {
+      data: {
+        headers: {
+          'Cache-Control': 'max-age=1'
         },
-        createdAt: Date.now(),
-        state: 'cached',
-        ttl: 1000,
-        staleTtl: 1000
-      })
-    );
+        data: true,
+        status: 200,
+        statusText: 'OK'
+      },
+      createdAt: Date.now(),
+      state: 'cached',
+      ttl: 1000,
+      staleTtl: 1000
+    };
 
-    // Normal request, but with must-revalidate
-    assert.equal(
-      canStale({
-        data: {
-          headers: {
-            'cache-control': 'must-revalidate, max-age=1'
-          },
-          data: true,
-          status: 200,
-          statusText: 'OK'
-        },
-        createdAt: Date.now(),
-        state: 'cached',
-        ttl: 1000,
-        staleTtl: 1000
-      }),
-      false
-    );
+    assert.ok(canStale(entry));
+    assert.equal(mustRevalidate(entry), false);
+
+    // Now with must-revalidate
+    entry.data.headers['cache-control'] = 'must-revalidate, max-age=1';
+
+    assert.equal(canStale(entry), true);
+    assert.equal(mustRevalidate(entry), true);
   });
 });