Skip to content
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

Implement HTTP browser caching for blobs #137

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jbethune
Copy link
Collaborator

@jbethune jbethune commented Sep 23, 2023

fix #113

I also checked Starlette's code, but they don't use the Cache-Control header at all.

This PR does not implement caching for language strings because there are no etags for them (yet).
Edit: Basic caching without ETags is now implemented as part of this PR for localization strings.

I tested the code in Firebug and the caching works nicely.

@jbethune jbethune requested a review from scy September 23, 2023 20:33
@t-muehlberger
Copy link
Collaborator

t-muehlberger commented Sep 23, 2023

@jbethune maybe we should not yet close #113 since, as you mentioned, language strings are not yet cached.

Edit: Regarding the missing ETags. As you know you can compute the ETag using any hashing algorithm of your choice, they do not need to be stored in the DB or the YAML-Config. But you should probably do it only once over the lifetime of the backend-process rather than for each request as the hashing can be an expensive computation. You can probably just hold the computed ETags in memory as the number of languages will be relatively limited any you only need a few bytes of memory per ETag. @scy please correct me if I am wrong.

@jbethune
Copy link
Collaborator Author

jbethune commented Sep 24, 2023

I can also implement basic caching instructions for language strings without etag is part of this PR. That's actually easy to do. Then each set of language strings can have an expiration of e.g. 1 day.

I would also like to point out that there is #60 but that is in a later milestone. I suggest I implement caching for language strings as described in the previous paragraph and then we implement etag-based caching once we have migrated the translations into a separate location. Then we can calculate a new etag for each version of the translations whenever there is a change.

@scy
Copy link
Collaborator

scy commented Sep 25, 2023

No time for a full review right now, sorry, but about the translation strings caching:

The Config is a singleton and will not change during the application’s lifetime. You could simply use something like

strings = all_frontend_strings(language)
etag = str(id(strings))

because all_frontend_strings employs a lru_cache and should thus respond with the same dict instance per language (please double-check).

If you want to be super careful about id reuse between application runs, feel free to add an application startup timestamp or something to the etag.

@jbethune
Copy link
Collaborator Author

jbethune commented Sep 30, 2023

strings = all_frontend_strings(language)
etag = str(id(strings))

because all_frontend_strings employs a lru_cache and should thus respond with the same dict instance per language (please double-check).

Implemented as suggested. This should still be improved when #60 is implemented, because it's just a hack that works most of the time.

@scy scy added the prio:C Nice to have, but optional label Oct 11, 2023
joern added 3 commits October 18, 2023 17:00
This should later be replaced by the implementation that will fix #60
because using id() is just a hack.
@jbethune jbethune force-pushed the feature/browser-cache-api-response branch from 648741d to c64ae63 Compare October 18, 2023 15:02
@jbethune
Copy link
Collaborator Author

I have rebased this public branch to catch up with other changes and fixed all merge conflicts.

@scy
Copy link
Collaborator

scy commented Oct 18, 2023

Thanks! We might not merge this until after the initial go-live, simply to reduce the number of moving parts, but I appreciate the rebase and will look at it as soon as I can.

@jbethune
Copy link
Collaborator Author

@scy Please let me know when you want to review this, so that I can rebase it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio:C Nice to have, but optional
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add caching headers to HTTP endpoints that benefit from it
3 participants