feat(middleware/cache): add cacheableStatusCodes option

[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.29%. Comparing base (a2ec848) to head (c2e4a38).
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3943      +/-   ##
==========================================
- Coverage   91.32%   91.29%   -0.04%     
==========================================
  Files         168      168              
  Lines       10688    10775      +87     
  Branches     3070     3105      +35     
==========================================
+ Hits         9761     9837      +76     
- Misses        926      937      +11     
  Partials        1        1              

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

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[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.

[miyamo2]

Hi, @usualoma. Thanks for checking and pointing it out.

RFC talks about ‘whether or not it is cacheable?’ and not ‘whether or not it should be cached?’

My Apologies, I misunderstood what they meant.

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.

After reading your comments, I am beginning to feel that it is not reasonable to exclude DELETE, OPTIONS, etc. from the cache.
I found that there was infinitely less benefit to be gained by make breaking changes.

we should use the combination of Set and set.has() here

I corrected to use Set instead of Map. Thank you.

[miyamo2]

@usualoma
I now understand that there is no definitive way to determine whether caching is required based on the status code or method—it ultimately depends on the application.

Given this, how about allowing users to configure the status code and method at the userland?
While this is not the original intent of the PR, but it would provide users with greater flexibility to implement caching behavior as needed while maintaining backward compatibility.

[usualoma]

Hi, @miyamo2. I appreciate your consideration.

method

As mentioned above, hono can specify the method to apply middleware in the framework semantics. (app.get(‘*’, cache())) With that in mind, I think specifying it in the cache middleware is unnecessary. (There may be opposing opinions.)

status

Regarding the status code, currently, response.ok is used, i.e., 200-299 is cached, but I think it should be narrowed down more.

For example, 206 is cacheable, as you know from the HTTP semantics, but to cache it, we must include the Range request header in the cache key, etc., and Hono's cache middleware does not support this by default.

From the perspective of preventing accidents (or certain types of vulnerability) caused by careless caching, the default should be 200 only.

In addition, as you say, it might be a good idea to allow users to specify the status code to be cached explicitly.

[yusukebe]

Hi @miyamo2 Thank you for the PR. @usualoma Thank you for the comments.

Regarding handling the method, I also think the middleware should not specify the method, but for example, you can use app.get() to support only GET. We don't have to implement that in the middleware.

Regarding the status code, it's a good idea not to use 200 - 299 defined in Response.ok, but narrow down the codes. The. 200 as default is suitable. If the user wants to add additional code, using the cacheableStatusCodes option is good.

Either way, making the default status code not 200 - 299 will cause a breaking change. We can't merge this immediately.

[miyamo2]

Hi @yusukebe, @usualoma.
Thank you for checking.

I modified the default cached response to only 200: OK.
Also, the cacheability of the request method has been removed.
Could you please check these?

Either way, making the default status code not 200 - 299 will cause a breaking change. We can't merge this immediately.

Sure. In my correspondence with @usualoma , I understood that this PR should have been kept as a proposal, not a fix.

[usualoma]

LGTM!

[yusukebe]

LGTM!

[yusukebe]

Hey @miyamo2

Looks good!

I fixed the title to "add cacheableStatusCodes option" since the purpose was changed from when you created the PR. This is a new feature, so this may be included in the next minor version. So, I left it does not merge immediately.

I appreciate your great work!