config/public: cache blob content#1810
Conversation
|
|
||
| if (!frame.isPublic) return Problem.user.insufficientRights(); | ||
|
|
||
| response.set('Cache-Control', request.query.ts ? 'max-age=31536000' : 'no-cache'); |
There was a problem hiding this comment.
What do you think about moving this logic to getConfig()? To me, the only difference between /v1/config/:key and /v1/config/public/:key is how auth works. I'd expect caching to work the same between the two.
Practically, moving the logic wouldn't have any user-facing effect, since Frontend is only using /v1/config/public/:key for these images; Frontend doesn't need /v1/config/:key to do caching. But to me, it'd be a little clearer to handle the logic in getConfig(). Doing so would require passing request and response to getConfig().
I don't have strong feelings about this one. Mostly I just wanted to throw it out there to see what you thought.
There was a problem hiding this comment.
Or is the idea that /v1/config/public/:key is already public, so there's no concern about sensitive data ending up in a cache somewhere?
There was a problem hiding this comment.
On that point, I'm now realizing that even when request.query.ts is falsey, the endpoint sets the header to no-cache, which is different from the default of private, no-cache that's set by lib/http/service.js. To me, the absence of private is confirmation of the intention that only the public endpoint should have these differences in caching. 👍
There was a problem hiding this comment.
Now I'm wondering whether a code comment explaining some of this reasoning would be helpful. 🤔
(Sorry for all the comments here!)
There was a problem hiding this comment.
no-cache is to allow for public caching, and revalidation via ETag. I've refactored cache header handling to be more descriptive - please take a look.
There was a problem hiding this comment.
I think this change also makes it clearer why cache headers would be handled differently for /config/public/:key to /config/:key.
2 participants
Closes getodk/central#1822
What has been done to verify that this works as intended?
Why is this the best possible solution? Were any other approaches considered?
Alternatives
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 make the login page load faster.
Does this change require updates to the API documentation? If so, please update docs/api.yaml as part of this PR.
304 responses have been added to the API documentation.
Before submitting this PR, please make sure you have:
make testand confirmed all checks still pass, or witnessed Github completing all checks with success