Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

return no-cache on 404 #2609

Merged
merged 4 commits into from
Feb 12, 2025
Merged

return no-cache on 404 #2609

merged 4 commits into from
Feb 12, 2025

Conversation

taoeffect
Copy link
Member

Partially addresses #2608.

Copy link

cypress bot commented Feb 11, 2025

group-income    Run #3928

Run Properties:  status check passed Passed #3928  •  git commit c2335d374a ℹ️: Merge a44b1e93d1ec00156919df11420137f79c5672fb into 4680441e5ef89a418159ac9941a9...
Project group-income
Branch Review 2608-disable-cache-404
Run status status check passed Passed #3928
Run duration 11m 34s
Commit git commit c2335d374a ℹ️: Merge a44b1e93d1ec00156919df11420137f79c5672fb into 4680441e5ef89a418159ac9941a9...
Committer Greg Slepak
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 10
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 114
View all changes introduced in this branch ↗︎

function notFoundNoCache (h) {
return h.response()
.code(404)
.header('Cache-Control', 'no-store, must-revalidate, proxy-revalidate')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*-revalidate is redundant as well, since no-store already means that the response should not be put in a cache to begin with.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think the issue here is this, since by default 404 responses will not be cached.

Most commonly, caches store the successful result of a retrieval request: i.e., a 200 (OK) response to a GET request, which contains a representation of the target resource (Section 9.3.1 of [HTTP]). However, it is also possible to store redirects, negative results (e.g., 404 (Not Found)), incomplete results (e.g., 206 (Partial Content)), and responses to methods other than GET if the method's definition allows such caching and defines something suitable for use as a cache key.

  cache: {
    // Do not set other cache options here, to make sure the 'otherwise' option
    // will be used so that the 'immutable' directive gets included.
    otherwise: 'public,max-age=31536000,immutable'
  }

I'm not too familiar with HAPI, and not with the old version used in GI, but, instead of setting a cache policy and then 'undoing it' with this new notFoundNoCache wouldn't it be better to simply tell HAPI not to apply the immutable (or a custom) cache policy to errors?

.code(404)
.header('Cache-Control', 'no-store, must-revalidate, proxy-revalidate')
.header('Pragma', 'no-cache')
.header('Expires', '0')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expires is supposed to have a timestamp, so 0 isn't a valid value. However, it's not really relevant if cache-control is present, except for clients that don't support HTTP/1.1. I don't think any such clients exist (today) to any significant, and even if they did, they can be ignored.

return h.response()
.code(404)
.header('Cache-Control', 'no-store, must-revalidate, proxy-revalidate')
.header('Pragma', 'no-cache')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pragma: no-cache has never been standard for responses, and is also largely irrelevant for clients that understand cache-control.

@corrideat
Copy link
Member

I suggest either of these solutions:

  1. Set cache-control: immutable,... only for successful responses. From the HAPI docs, it seems like removing the cache: otherwise option would do this, plus adding the header to the response. This requires no additional changes because 404, etc. isn't supposed to be cached unless it says it should.
  2. Leave things mostly are they currently are, but on 404 return only cache-control: no-store without pragma or expires.

corrideat
corrideat previously approved these changes Feb 12, 2025
@taoeffect taoeffect merged commit b35c12b into master Feb 12, 2025
4 checks passed
@taoeffect taoeffect deleted the 2608-disable-cache-404 branch February 12, 2025 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants