fix(middleware/cache): avoid caching according to RFC7231

[miyamo2]

This pull request updates the cache middleware to avoid caching when it is defined as uncacheable in RFC 7231.
Caching will no longer occur under the following conditions.

Conditions for Avoiding Caching

1. Status codes defined as uncacheable by default

Responses with status codes that are defined as cacheable by default
(e.g., 200, 203, 204, 206, 300, 301, 404, 405, 410, 414, and 501 in this specification)
https://datatracker.ietf.org/doc/html/rfc7231#section-6.1

Additionally, It introducing an optional argument to allow caching of arbitrary status codes.

// In this case, only 412 will be cached.
app.use(
  '/*',
  cache({
    cacheName: 'foo',
    wait: true,
    cacheControl: 'max-age=10',
    cacheableStatusCodes: [412],
  })
)

2. Request methods defined as uncacheable

Only GET, HEAD, POST, and PATCH will be cached, otherwise it is no longer cached.

this specification defines GET, HEAD, and POST as
cacheable

https://datatracker.ietf.org/doc/html/rfc7231#section-4.2.3

A response to this method is only cacheable if it contains explicit freshness information (such as an Expires header or "Cache-Control: max-age" directive) as well as the Content-Location header matching the Request-URI, indicating that the PATCH response body is a resource representation.

https://datatracker.ietf.org/doc/html/rfc5789#section-2

See also

• https://developer.mozilla.org/en-US/docs/Glossary/Cacheable

Thank you.

The author should do the following, if applicable

• Add tests
• Run tests
• bun run format:fix && bun run lint:fix to format the code
• Add TSDoc/JSDoc to document the code

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.33%. Comparing base (a2ec848) to head (368c857).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3943   +/-   ##
=======================================
  Coverage   91.32%   91.33%           
=======================================
  Files         168      168           
  Lines       10688    10698   +10     
  Branches     3070     3077    +7     
=======================================
+ Hits         9761     9771   +10     
  Misses        926      926           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[usualoma]

Hi, @miyamo2. Thank you for creating PR!

cacheable status

404

When we apply this change, the cache middleware will cache the 404 status code by default. I think it depends on the application whether or not you want to cache 404.

I think it's a good idea to refer to the RFC, but the RFC talks about ‘whether or not it is cacheable?’ and not ‘whether or not it should be cached?’ , so I think it's up to the ‘application (or framework)’ to decide whether or not middleware should cache 404s.

In my experience, when caching 404s, you often want to set a shorter TTL than for other status codes. Therefore, if 404s are to be cached, I think a system that allows you to set a TTL for each status code is necessary. If such a system does not exist, a simple system that only caches 2xx is fine, as it is now.

201, 202, 205, etc

201 and 202 are currently cached, and I think it would be more appropriate not to cache them. (I don't think Hono users would apply cache middleware to APIs that return 201 or 202, though.

cacheable method

I think that for almost all apps, we only want to cache GET and HEAD requests, so if we make this change, I think the default should be only GET and HEAD. (POST and PATCH should be excluded.)

If we do that, I think it will be easier to specify using app.use() than now when you want to cache a particular area or below roughly. (I think it will be easier than specifying with app.get())

Depending on the application, you might want to cache DELETE method like this (this is a deviation from the standard, but in some cases it is a reasonable design to deviate from the standard depending on the application's requirements). In such cases, the existing code will no longer work, but in that case I think it would be fine to specify the method explicitly.

app.delete(
  '/api/*',
  cache()
)

[usualoma]

That said, I don't think there are any security issues with the current implementation. I don't think there is anything particularly problematic about the DELETE method also being cached, if you think of it as ‘middleware for a low-level cache layer’.

Therefore, although it is a very thought-provoking pull request, I think it is also possible to leave the current simple implementation as it is. (It might be a good idea to exclude 201, 202, 205, etc.)

[usualoma]

In terms of implementation, we should use the combination of Set<number> and set.has() here, rather than the combination of Map<number, boolean> and map.get(). It is more straightforward for what we want to do, and also improves performance.

[usualoma]

I'd also like to hear the opinions of other committers.