From 68107da3fdd6cd0397f4800a2bc9630c802eda58 Mon Sep 17 00:00:00 2001 From: Aras Abbasi Date: Tue, 15 Oct 2024 23:54:56 +0200 Subject: [PATCH] [http-cache] follow up (#3733) * followup http-cache * fix methods check * avoid instantiation in case of error * improve * rename assertCacheStoreType to assertCacheStore * remove validation * remove fallback * fix * fix --- lib/core/util.js | 2 +- lib/handler/cache-handler.js | 62 +++++++++++------------ lib/handler/cache-revalidation-handler.js | 4 +- lib/interceptor/cache.js | 42 +++++++-------- lib/util/cache.js | 37 ++++++++++---- types/cache-interceptor.d.ts | 4 +- 6 files changed, 82 insertions(+), 69 deletions(-) diff --git a/lib/core/util.js b/lib/core/util.js index c37f213349b..fb311a7d36b 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -924,6 +924,6 @@ module.exports = { isHttpOrHttpsPrefixed, nodeMajor, nodeMinor, - safeHTTPMethods: ['GET', 'HEAD', 'OPTIONS', 'TRACE'], + safeHTTPMethods: Object.freeze(['GET', 'HEAD', 'OPTIONS', 'TRACE']), wrapRequestBody } diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index 2eaf0fa2417..3c4044a653e 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -2,24 +2,35 @@ const util = require('../core/util') const DecoratorHandler = require('../handler/decorator-handler') -const { parseCacheControlHeader, parseVaryHeader, UNSAFE_METHODS, assertCacheStoreType } = require('../util/cache') +const { + parseCacheControlHeader, + parseVaryHeader +} = require('../util/cache') /** * Writes a response to a CacheStore and then passes it on to the next handler */ class CacheHandler extends DecoratorHandler { /** - * @type {import('../../types/cache-interceptor.d.ts').default.CacheOptions | null} - */ - #opts = null + * @type {import('../../types/cache-interceptor.d.ts').default.CacheStore} + */ + #store + + /** + * @type {import('../../types/cache-interceptor.d.ts').default.CacheMethods} + */ + #methods + /** - * @type {import('../../types/dispatcher.d.ts').default.RequestOptions | null} + * @type {import('../../types/dispatcher.d.ts').default.RequestOptions} */ - #req = null + #requestOptions + /** - * @type {import('../../types/dispatcher.d.ts').default.DispatchHandlers | null} + * @type {import('../../types/dispatcher.d.ts').default.DispatchHandlers} */ - #handler = null + #handler + /** * @type {import('../../types/cache-interceptor.d.ts').default.CacheStoreWriteable | undefined} */ @@ -27,29 +38,18 @@ class CacheHandler extends DecoratorHandler { /** * @param {import('../../types/cache-interceptor.d.ts').default.CacheOptions} opts - * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} req + * @param {import('../../types/dispatcher.d.ts').default.RequestOptions} requestOptions * @param {import('../../types/dispatcher.d.ts').default.DispatchHandlers} handler */ - constructor (opts, req, handler) { - super(handler) + constructor (opts, requestOptions, handler) { + const { store, methods } = opts - if (typeof opts !== 'object') { - throw new TypeError(`expected opts to be an object, got type ${typeof opts}`) - } - - assertCacheStoreType(opts.store) - - if (typeof req !== 'object') { - throw new TypeError(`expected req to be an object, got type ${typeof opts}`) - } - - if (typeof handler !== 'object') { - throw new TypeError(`expected handler to be an object, got type ${typeof opts}`) - } + super(handler) - this.#opts = opts - this.#req = req + this.#store = store + this.#requestOptions = requestOptions this.#handler = handler + this.#methods = methods } /** @@ -75,14 +75,14 @@ class CacheHandler extends DecoratorHandler { ) if ( - UNSAFE_METHODS.includes(this.#req.method) && + !this.#methods.includes(this.#requestOptions.method) && statusCode >= 200 && statusCode <= 399 ) { // https://www.rfc-editor.org/rfc/rfc9111.html#name-invalidating-stored-respons // Try/catch for if it's synchronous try { - const result = this.#opts.store.deleteByOrigin(this.#req.origin) + const result = this.#store.deleteByOrigin(this.#requestOptions.origin) if ( result && typeof result.catch === 'function' && @@ -103,7 +103,7 @@ class CacheHandler extends DecoratorHandler { const cacheControlHeader = headers['cache-control'] const contentLengthHeader = headers['content-length'] - if (!cacheControlHeader || !contentLengthHeader || this.#opts.store.isFull) { + if (!cacheControlHeader || !contentLengthHeader || this.#store.isFull) { // Don't have the headers we need, can't cache return downstreamOnHeaders() } @@ -122,7 +122,7 @@ class CacheHandler extends DecoratorHandler { const staleAt = determineStaleAt(now, headers, cacheControlDirectives) if (staleAt) { const varyDirectives = headers.vary - ? parseVaryHeader(headers.vary, this.#req.headers) + ? parseVaryHeader(headers.vary, this.#requestOptions.headers) : undefined const deleteAt = determineDeleteAt(now, cacheControlDirectives, staleAt) @@ -132,7 +132,7 @@ class CacheHandler extends DecoratorHandler { cacheControlDirectives ) - this.#writeStream = this.#opts.store.createWriteStream(this.#req, { + this.#writeStream = this.#store.createWriteStream(this.#requestOptions, { statusCode, statusMessage, rawHeaders: strippedHeaders, diff --git a/lib/handler/cache-revalidation-handler.js b/lib/handler/cache-revalidation-handler.js index d5293cdb166..53c58773022 100644 --- a/lib/handler/cache-revalidation-handler.js +++ b/lib/handler/cache-revalidation-handler.js @@ -32,12 +32,12 @@ class CacheRevalidationHandler extends DecoratorHandler { * @param {import('../../types/dispatcher.d.ts').default.DispatchHandlers} handler */ constructor (successCallback, handler) { - super(handler) - if (typeof successCallback !== 'function') { throw new TypeError('successCallback must be a function') } + super(handler) + this.#successCallback = successCallback this.#handler = handler } diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index e726ec2ba47..b03734aa886 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -4,40 +4,32 @@ const util = require('../core/util') const CacheHandler = require('../handler/cache-handler') const MemoryCacheStore = require('../cache/memory-cache-store') const CacheRevalidationHandler = require('../handler/cache-revalidation-handler') -const { UNSAFE_METHODS, assertCacheStoreType } = require('../util/cache.js') +const { assertCacheStore, assertCacheMethods } = require('../util/cache.js') const AGE_HEADER = Buffer.from('age') /** - * @param {import('../../types/cache-interceptor.d.ts').default.CacheOptions | undefined} globalOpts + * @param {import('../../types/cache-interceptor.d.ts').default.CacheOptions} [opts] * @returns {import('../../types/dispatcher.d.ts').default.DispatcherComposeInterceptor} */ -module.exports = globalOpts => { - if (!globalOpts) { - globalOpts = {} +module.exports = (opts = {}) => { + const { + store = new MemoryCacheStore(), + methods = ['GET'] + } = opts + + if (typeof opts !== 'object' || opts === null) { + throw new TypeError(`expected type of opts to be an Object, got ${opts === null ? 'null' : typeof opts}`) } - if (globalOpts.store) { - assertCacheStoreType(globalOpts.store) - } else { - globalOpts.store = new MemoryCacheStore() - } - - if (globalOpts.methods) { - if (!Array.isArray(globalOpts.methods)) { - throw new TypeError(`methods needs to be an array, got ${typeof globalOpts.methods}`) - } + assertCacheStore(store, 'opts.store') + assertCacheMethods(methods, 'opts.methods') - if (globalOpts.methods.length === 0) { - throw new Error('methods must have at least one method in it') - } - } else { - globalOpts.methods = ['GET'] + const globalOpts = { + store, + methods } - // Safe methods the user wants and unsafe methods - const methods = [...globalOpts.methods, ...UNSAFE_METHODS] - return dispatch => { return (opts, handler) => { if (!opts.origin || !methods.includes(opts.method)) { @@ -45,7 +37,7 @@ module.exports = globalOpts => { return dispatch(opts, handler) } - const stream = globalOpts.store.createReadStream(opts) + const stream = store.createReadStream(opts) if (!stream) { // Request isn't cached return dispatch(opts, new CacheHandler(globalOpts, opts, handler)) @@ -169,7 +161,7 @@ module.exports = globalOpts => { respondWithCachedValue(stream, value) } - Promise.resolve(stream).then(handleStream).catch(err => handler.onError(err)) + Promise.resolve(stream).then(handleStream).catch(handler.onError) return true } diff --git a/lib/util/cache.js b/lib/util/cache.js index 17ba5a31dae..48a91da3e74 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -1,8 +1,8 @@ 'use strict' -const UNSAFE_METHODS = /** @type {const} */ ([ - 'POST', 'PUT', 'PATCH', 'DELETE' -]) +const { + safeHTTPMethods +} = require('../core/util') /** * @see https://www.rfc-editor.org/rfc/rfc9111.html#name-cache-control @@ -181,25 +181,44 @@ function parseVaryHeader (varyHeader, headers) { * @param {unknown} store * @returns {asserts store is import('../../types/cache-interceptor.d.ts').default.CacheStore} */ -function assertCacheStoreType (store) { +function assertCacheStore (store, name = 'CacheStore') { if (typeof store !== 'object' || store === null) { - throw new TypeError(`expected type to be an store, got ${typeof store}`) + throw new TypeError(`expected type of ${name} to be a CacheStore, got ${store === null ? 'null' : typeof store}`) } for (const fn of ['createReadStream', 'createWriteStream', 'deleteByOrigin']) { if (typeof store[fn] !== 'function') { - throw new TypeError(`CacheStore needs a \`${fn}()\` function`) + throw new TypeError(`${name} needs to have a \`${fn}()\` function`) } } if (typeof store.isFull !== 'boolean') { - throw new TypeError(`CacheStore needs a isFull getter with type boolean, current type: ${typeof store.isFull}`) + throw new TypeError(`${name} needs a isFull getter with type boolean, current type: ${typeof store.isFull}`) + } +} +/** + * @param {unknown} methods + * @returns {asserts methods is import('../../types/cache-interceptor.d.ts').default.CacheMethods[]} + */ +function assertCacheMethods (methods, name = 'CacheMethods') { + if (!Array.isArray(methods)) { + throw new TypeError(`expected type of ${name} needs to be an array, got ${methods === null ? 'null' : typeof methods}`) + } + + if (methods.length === 0) { + throw new TypeError(`${name} needs to have at least one method`) + } + + for (const method of methods) { + if (!safeHTTPMethods.includes(method)) { + throw new TypeError(`element of ${name}-array needs to be one of following values: ${safeHTTPMethods.join(', ')}, got ${method}`) + } } } module.exports = { - UNSAFE_METHODS, parseCacheControlHeader, parseVaryHeader, - assertCacheStoreType + assertCacheMethods, + assertCacheStore } diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index 45af1a8a5f5..7fa1aeab0ed 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -4,6 +4,8 @@ import Dispatcher from './dispatcher' export default CacheHandler declare namespace CacheHandler { + export type CacheMethods = 'GET' | 'HEAD' | 'OPTIONS' | 'TRACE' + export interface CacheOptions { store?: CacheStore @@ -14,7 +16,7 @@ declare namespace CacheHandler { * @see https://www.rfc-editor.org/rfc/rfc9111.html#name-invalidating-stored-respons * @see https://www.rfc-editor.org/rfc/rfc9110#section-9.2.1 */ - methods?: ('GET' | 'HEAD' | 'OPTIONS' | 'TRACE')[] + methods?: CacheMethods[] } /**