-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Cache collections with new CacheablePathsProcessor
#9069
Cache collections with new CacheablePathsProcessor
#9069
Conversation
The new processor attempts to return cached copies of `GET` requests to paths defined in the `paths_and_expiries` dictionary.
from openlibrary.core.processors import ReadableUrlProcessor | ||
from openlibrary.plugins.openlibrary.home import caching_prethread |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewer: Is it okay to use this caching_prethread
?
I more or less re-implemented the homepage caching in this new processor, which uses caching_prethread
.
The following are not cached: - Pages with special encodings (json, yml, etc.) - Pages where the mode is not `view` (edit pages, etc.) - Pages where `debug` is included in the query parameters Language is now taken into consideration when caching. Cache key includes the language code and a sorted query string.
4c90b79
to
c354ec2
Compare
mc(_cache='delete') | ||
|
||
return _page | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewer: Please check the order of the conditionals that return handler()
.
My goal is to order these such that we exit this method as quickly as possible if the page isn't being cached by this processor.
In addition to languages, print-disabled patron status and potentially safe-for-work mode may be useful to consider here. It may be nice to codify what these are e.g. language, region, safe for work, print disabled, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At minimum, cache should take into consideration lang
and printdisabled
(in he future sfw
48d9103
to
2e0416c
Compare
Caching localized pages was already in place (we both missed that during our quick review). Have added additional caching for print-disabled page requests, and have included commented-out code for |
731ec1f
to
bde02c1
Compare
for more information, see https://pre-commit.ci
Closes #9058
Creates new
CacheablePathsProcessor
that can be used to cache Infogami pages without overriding the default Infogami handler for such pages.This processor attempts to fetch pages from cache when the following are true:
GET
json
,yml
, etc.)paths_and_expiries
dictionarydebug
is not in the request's query stringview
The processor has been set up to cache
/collections
and/collections/{collection_identifier}
pages.Technical
Cache keys will take the form:
{web.ctx.path}.{web.ctx.lang}{query_string}
...where
query_string
is either an empty string or an ordered query string (e.g.?a=1&b=1&c=1
)Adding a new path pattern and expiry time to
paths_and_expiries
will enable caching for pages matching to path pattern.Testing
/collections
pageThe load time on refresh should be noticeably faster than the initial page load.
Screenshot
Stakeholders
@mekarpeles