Merged
Conversation
lognaturel
commented
May 13, 2025
| map $cache_strategy $cache_header_cache_control { | ||
| "immutable" "max-age=31536000"; | ||
| "revalidate" "no-cache"; | ||
| "passthrough" ""; |
Member
Author
There was a problem hiding this comment.
I verified that if there's no value set for a header, that header isn't set by nginx.
Contributor
There was a problem hiding this comment.
I see a PR to update nginx docs: "The add_header directive will not generate a header if the value provided is an empty string." except if it is multi-value header
lognaturel
commented
May 13, 2025
| @@ -41,16 +44,19 @@ map "$request_method::$uri$is_args$args" $cache_strategy { | |||
| map $cache_strategy $cache_header_cache_control { | |||
| "immutable" "max-age=31536000"; | |||
Member
Author
There was a problem hiding this comment.
I think this should be public, max-age=31536000, immutable otherwise we don't know if intermediary caches will cache.
lognaturel
commented
May 13, 2025
| @@ -41,16 +44,19 @@ map "$request_method::$uri$is_args$args" $cache_strategy { | |||
| map $cache_strategy $cache_header_cache_control { | |||
| "immutable" "max-age=31536000"; | |||
| "revalidate" "no-cache"; | |||
Member
Author
There was a problem hiding this comment.
Same as above, I think we can/should add public.
sadiqkhoja
approved these changes
May 13, 2025
Contributor
sadiqkhoja
left a comment
sadiqkhoja
left a comment
There was a problem hiding this comment.
Looks good to me. However, we should add one or two tests in test-nginx.js
| @@ -41,16 +44,19 @@ map "$request_method::$uri$is_args$args" $cache_strategy { | |||
| map $cache_strategy $cache_header_cache_control { | |||
| "immutable" "max-age=31536000"; | |||
Member
Author
|
@sadiqkhoja please verify the tests and either open a PR to fix or send me some comments if they're not right/complete! |
sadiqkhoja
added a commit
to sadiqkhoja/central
that referenced
this pull request
May 13, 2025
* Update assertions: Header object doesn't have headers as properties, we have to use .has() * Update mock-http-server to return cache headers for a sample API * Add test to assert that backend cache headers are passed through
sadiqkhoja
added a commit
to sadiqkhoja/central
that referenced
this pull request
May 13, 2025
* Update assertions: Header object doesn't have headers as properties, we have to use .has() * Update mock-http-server to return cache headers for a sample API * Add test to assert that backend cache headers are passed through
sadiqkhoja
added a commit
that referenced
this pull request
May 13, 2025
* Update assertions: Header object doesn't have headers as properties, we have to use .has() * Update mock-http-server to return cache headers for a sample API * Add test to assert that backend cache headers are passed through
yanokwa
pushed a commit
to yanokwa/odk-central
that referenced
this pull request
Jun 11, 2025
yanokwa
pushed a commit
to yanokwa/odk-central
that referenced
this pull request
Jun 11, 2025
* Update assertions: Header object doesn't have headers as properties, we have to use .has() * Update mock-http-server to return cache headers for a sample API * Add test to assert that backend cache headers are passed through
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1048
What has been done to verify that this works as intended?
Put it on the dev server. Verified that there's exactly one
Cache-Controlheader and that it's set as expected.Why is this the best possible solution? Were any other approaches considered?
We could also set the headers here to
private, no-cache, etc. But then we'd be setting duplicate headers which could drift between nginx and backend. I think it's clearer to make caching headers clearly the backend's responsibility.Note that this does not set the
Pragmaheader for backend. I think we could do away with it altogether because it has been deprecated for a long time.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?
Backend responses should be cached in private caches. We should get 304s.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
nextbranch OR only changed documentation/infrastructure (masteris stable and used in production)