fix: cache fixes (#3871)

* fix: cache fixes * Update cache-interceptor.d.ts * fixup * rfixupo * fixup * fixup * fixup * fixup
nodejs · Nov 23, 2024 · 79f9d2f · 79f9d2f
1 parent a781473
commit 79f9d2f
Show file tree

Hide file tree

Showing 6 changed files with 83 additions and 45 deletions.
diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js
@@ -42,6 +42,17 @@ class MemoryCacheStore {
         this.#maxCount = opts.maxCount
       }
 
+      if (opts.maxSize !== undefined) {
+        if (
+          typeof opts.maxSize !== 'number' ||
+          !Number.isInteger(opts.maxSize) ||
+          opts.maxSize < 0
+        ) {
+          throw new TypeError('MemoryCacheStore options.maxSize must be a non-negative integer')
+        }
+        this.#maxSize = opts.maxSize
+      }
+
       if (opts.maxEntrySize !== undefined) {
         if (
           typeof opts.maxEntrySize !== 'number' ||
@@ -126,7 +137,7 @@ class MemoryCacheStore {
         store.#size += entry.size
         store.#count += 1
 
-        if (store.#size > store.#maxEntrySize || store.#count > store.#maxCount) {
+        if (store.#size > store.#maxSize || store.#count > store.#maxCount) {
           for (const [key, entries] of store.#entries) {
             for (const entry of entries.splice(0, entries.length / 2)) {
               store.#size -= entry.size

diff --git a/lib/cache/sqlite-cache-store.js b/lib/cache/sqlite-cache-store.js
@@ -248,7 +248,7 @@ class SqliteCacheStore {
     const body = []
     const store = this
 
-    const writable = new Writable({
+    return new Writable({
       write (chunk, encoding, callback) {
         if (typeof chunk === 'string') {
           chunk = Buffer.from(chunk, encoding)
@@ -267,9 +267,6 @@ class SqliteCacheStore {
       final (callback) {
         store.prune()
 
-        /**
-         * @type {SqliteStoreValue | undefined}
-         */
         const existingValue = store.#findValue(key, true)
         if (existingValue) {
           // Updating an existing response, let's overwrite it
@@ -306,8 +303,6 @@ class SqliteCacheStore {
         callback()
       }
     })
-
-    return writable
   }
 
   /**
@@ -373,14 +368,14 @@ class SqliteCacheStore {
    */
   #findValue (key, canBeExpired = false) {
     const url = this.#makeValueUrl(key)
+    const { headers, method } = key
 
     /**
      * @type {SqliteStoreValue[]}
      */
-    const values = this.#getValuesQuery.all(url, key.method)
+    const values = this.#getValuesQuery.all(url, method)
 
     if (values.length === 0) {
-      // No responses, let's just return early
       return undefined
     }
 
@@ -393,16 +388,14 @@ class SqliteCacheStore {
       let matches = true
 
       if (value.vary) {
-        if (!key.headers) {
-          // Request doesn't have headers so it can't fulfill the vary
-          //  requirements no matter what, let's return early
+        if (!headers) {
           return undefined
         }
 
-        value.vary = JSON.parse(value.vary)
+        const vary = JSON.parse(value.vary)
 
-        for (const header in value.vary) {
-          if (key.headers[header] !== value.vary[header]) {
+        for (const header in vary) {
+          if (headerValueEquals(headers[header], vary[header])) {
             matches = false
             break
           }
@@ -418,6 +411,29 @@ class SqliteCacheStore {
   }
 }
 
+/**
+ * @param {string|string[]|null|undefined} lhs
+ * @param {string|string[]|null|undefined} rhs
+ * @returns {boolean}
+ */
+function headerValueEquals (lhs, rhs) {
+  if (Array.isArray(lhs) && Array.isArray(rhs)) {
+    if (lhs.length !== rhs.length) {
+      return false
+    }
+
+    for (let i = 0; i < lhs.length; i++) {
+      if (rhs.includes(lhs[i])) {
+        return false
+      }
+    }
+
+    return true
+  }
+
+  return lhs === rhs
+}
+
 /**
  * @param {Buffer[]} buffers
  * @returns {string[]}

diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js
@@ -35,7 +35,7 @@ class CacheHandler extends DecoratorHandler {
   #writeStream
 
   /**
-   * @param {import('../../types/cache-interceptor.d.ts').default.CacheOptions} opts
+   * @param {import('../../types/cache-interceptor.d.ts').default.CacheHandlerOptions} opts
    * @param {import('../../types/cache-interceptor.d.ts').default.CacheKey} cacheKey
    * @param {import('../../types/dispatcher.d.ts').default.DispatchHandlers} handler
    */
@@ -106,17 +106,11 @@ class CacheHandler extends DecoratorHandler {
     const headers = util.parseHeaders(parsedRawHeaders)
 
     const cacheControlHeader = headers['cache-control']
-    const isCacheFull = typeof this.#store.isFull !== 'undefined'
-      ? this.#store.isFull
-      : false
-
-    if (
-      !cacheControlHeader ||
-      isCacheFull
-    ) {
+    if (!cacheControlHeader) {
       // Don't have the cache control header or the cache is full
       return downstreamOnHeaders()
     }
+
     const cacheControlDirectives = parseCacheControlHeader(cacheControlHeader)
     if (!canCacheResponse(statusCode, headers, cacheControlDirectives)) {
       return downstreamOnHeaders()
@@ -236,10 +230,7 @@ class CacheHandler extends DecoratorHandler {
  * @param {import('../util/cache.js').CacheControlDirectives} cacheControlDirectives
  */
 function canCacheResponse (statusCode, headers, cacheControlDirectives) {
-  if (
-    statusCode !== 200 &&
-    statusCode !== 307
-  ) {
+  if (statusCode !== 200 && statusCode !== 307) {
     return false
   }
 
@@ -309,7 +300,7 @@ function determineStaleAt (now, headers, cacheControlDirectives) {
   if (headers.expire && typeof headers.expire === 'string') {
     // https://www.rfc-editor.org/rfc/rfc9111.html#section-5.3
     const expiresDate = new Date(headers.expire)
-    if (expiresDate instanceof Date && !isNaN(expiresDate)) {
+    if (expiresDate instanceof Date && Number.isFinite(expiresDate.valueOf())) {
       return now + (Date.now() - expiresDate.getTime())
     }
   }
@@ -391,9 +382,7 @@ function stripNecessaryHeaders (rawHeaders, parsedRawHeaders, cacheControlDirect
     strippedHeaders.length -= offset
   }
 
-  return strippedHeaders
-    ? util.encodeRawHeaders(strippedHeaders)
-    : rawHeaders
+  return strippedHeaders ? util.encodeRawHeaders(strippedHeaders) : rawHeaders
 }
 
 module.exports = CacheHandler
diff --git a/lib/util/cache.js b/lib/util/cache.js
@@ -13,21 +13,38 @@ function makeCacheKey (opts) {
     throw new Error('opts.origin is undefined')
   }
 
-  /**
-   * @type {import('../../types/cache-interceptor.d.ts').default.CacheKey}
-   */
-  const cacheKey = {
+  /** @type {Record<string, string[] | string>} */
+  let headers
+  if (opts.headers == null) {
+    headers = {}
+  } else if (typeof opts.headers[Symbol.iterator] === 'function') {
+    headers = {}
+    for (const x of opts.headers) {
+      if (!Array.isArray(x)) {
+        throw new Error('opts.headers is not a valid header map')
+      }
+      const [key, val] = x
+      if (typeof key !== 'string' || typeof val !== 'string') {
+        throw new Error('opts.headers is not a valid header map')
+      }
+      headers[key] = val
+    }
+  } else if (typeof opts.headers === 'object') {
+    headers = opts.headers
+  } else {
+    throw new Error('opts.headers is not an object')
+  }
+
+  return {
     origin: opts.origin.toString(),
     method: opts.method,
     path: opts.path,
-    headers: opts.headers
+    headers
   }
-
-  return cacheKey
 }
 
 /**
- * @param {any} value
+ * @param {any} key
  */
 function assertCacheKey (key) {
   if (typeof key !== 'object') {
@@ -299,10 +316,6 @@ function assertCacheStore (store, name = 'CacheStore') {
       throw new TypeError(`${name} needs to have a \`${fn}()\` function`)
     }
   }
-
-  if (typeof store.isFull !== 'undefined' && typeof store.isFull !== 'boolean') {
-    throw new TypeError(`${name} needs a isFull getter with type boolean or undefined, current type: ${typeof store.isFull}`)
-  }
 }
 /**
  * @param {unknown} methods

diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts
@@ -5,6 +5,10 @@ export default CacheHandler
 declare namespace CacheHandler {
   export type CacheMethods = 'GET' | 'HEAD' | 'OPTIONS' | 'TRACE'
 
+  export interface CacheHandlerOptions {
+    store: CacheStore
+  }
+
   export interface CacheOptions {
     store?: CacheStore
 
@@ -75,6 +79,11 @@ declare namespace CacheHandler {
      */
     maxSize?: number
 
+    /**
+     * @default Infinity
+     */
+    maxEntrySize?: number
+
     errorCallback?: (err: Error) => void
   }
 

diff --git a/types/dispatcher.d.ts b/types/dispatcher.d.ts
@@ -103,7 +103,7 @@ declare namespace Dispatcher {
     /** Default: `null` */
     body?: string | Buffer | Uint8Array | Readable | null | FormData;
     /** Default: `null` */
-    headers?: IncomingHttpHeaders | string[] | Iterable<[string, string | string[] | undefined]> | null;
+    headers?: Record<string, string | string[]> | IncomingHttpHeaders | string[] | Iterable<[string, string | string[] | undefined]> | null;
     /** Query string params to be embedded in the request URL. Default: `null` */
     query?: Record<string, any>;
     /** Whether the requests can be safely retried or not. If `false` the request won't be sent until all preceding requests in the pipeline have completed. Default: `true` if `method` is `HEAD` or `GET`. */