fix: redirect paginated requests that pollute cache #83
Conversation
This file contains 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
Fixes lobsters/lobsters#1219 Requests that attempt to use pagination via ?page=N for anything but search will pollute the rails cache because rails ignores the parameters when writing to the file cache: see eg rails/actionpack-page_caching#52 There are two sides to this bug: we should not read from the cached parameterless page if there is a page parameter present, and we should not write to the cache for the parameterless page if there is a page parameter present. Both cases can be avoided by redirecting requests with the page parameter to the path format that lobste.rs prefers, so /hottest.json?page=10 becomes /hottest/page/10.json. There is one page which does use the page parameter instead of path parameters for pagination, and that is /search. However, search does not use the full page cache at all: https://github.com/search?q=repo%3Alobsters%2Flobsters%20%20caches_page&type=code Hence we can skip the redirect for /search, but also safely skip the other cache checks. This logic would be simpler if nginx supported nested ifs but it does not: http://nginx.org/en/docs/http/ngx_http_rewrite_module.html#if Note that the if directive is not allowed in an 'if' context (whereas the rewrite directive is)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes lobsters/lobsters#1219
Requests that attempt to use pagination via ?page=N for anything but search will pollute the rails cache because rails ignores the parameters when writing to the file cache: see eg
rails/actionpack-page_caching#52
There are two sides to this bug: we should not read from the cached parameterless page if there is a page parameter present, and we should not write to the cache for the parameterless page if there is a page parameter present.
Both cases can be avoided by redirecting requests with the page parameter to the path format that lobste.rs prefers, so /hottest.json?page=10 becomes /hottest/page/10.json.
There is one page which does use the page parameter instead of path parameters for pagination, and that is /search. However, search does not use the full page cache at all:
https://github.com/search?q=repo%3Alobsters%2Flobsters%20%20caches_page&type=code
Hence we can skip the redirect for /search, but also safely skip the other cache checks. This logic would be simpler if nginx supported nested ifs but it does not: http://nginx.org/en/docs/http/ngx_http_rewrite_module.html#if Note that the if directive is not allowed in an 'if' context (whereas the rewrite directive is)