Add cache headers to responses #1390
Conversation
There was a problem hiding this comment.
That looks good to me -- using the private cache directly for 20s keeps a client from totally hammering the server with requests without compromising freshness. Then we expect the request to happen and result in a 304 if there are no changes.
@sadiqkhoja in case you want to take a second look.
| if (req.path.startsWith('/sessions/')) { | ||
| res.set('Cache-Control', 'no-store'); | ||
| } else { | ||
| res.set('Cache-Control', 'private, max-age=20'); |
There was a problem hiding this comment.
20 seconds isn't too much?
And for data endpoints like submissions and entities max-age should be 0
There was a problem hiding this comment.
The idea is to have a low max-age for all dynamic content so that there is some local caching that comes into play for unusual patterns like immediately refreshing. In general, we don't expect immediate refreshes so this eases load on the server a bit without significantly compromising freshness. We could set it to 0 instead but I don't see a big advantage. Whichever we choose we should apply consistently, I think, since all content is equally dynamic.
There was a problem hiding this comment.
I think max-age=0 is likely to provide fewer surprises.
Is immediate refreshing an issue? I'd guess if it's being done, then it's because of an expectation of changed data...
There was a problem hiding this comment.
Fair enough, that works for me!
| @@ -17,24 +17,192 @@ const serverUrl = 'http://localhost:8383'; | |||
| const userEmail = 'x@example.com'; | |||
| const userPassword = 'secret1234'; | |||
|
|
|||
| describe('Cache headers', () => { | |||
| const undici = require('undici'); | |||
There was a problem hiding this comment.
nit: maybe move this to the top of the file with other requires.
Closes getodk/central#662
What has been done to verify that this works as intended?
Added tests.
Why is this the best possible solution? Were any other approaches considered?
It seems simple.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Should prevent future caching surprises.
Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.
No? "cache" is not currently mentioned in docs/
Before submitting this PR, please make sure you have:
make testand confirmed all checks still pass OR confirm CircleCI build passes