-
Notifications
You must be signed in to change notification settings - Fork 21
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
Use core django for caching signed urls. #13
base: master
Are you sure you want to change the base?
Conversation
Use the caching feature of django core to cache urls. Using this feature we can create multi-day urls and keep them cached in whatever caching backend is configured. May only help in certain use cases, but one of those use cases is mine :)
Just in case it's not throwing an exception.
Hey, thanks for the PR! I'm a bit busy because of Easter holidays, but I'll try to look into this soon!
This should also be included in the unit tests, especially for covering multiple Django versions. I should update those for recent Django versions anyway, now that I think about it.
Am 30. März 2018 12:11:33 MESZ schrieb Matt Clark <[email protected]>:
…Use the caching feature of django core to cache urls. Using this
feature we can create multi-day urls and keep them cached in
whatever caching backend is configured. May only help in certain
use cases, but one of those use cases is mine :)
You can view, comment on, or merge this pull request online at:
#13
-- Commit Summary --
* Use core django for caching signed urls.
-- File Changes --
M django_gcloud_storage/__init__.py (43)
-- Patch Links --
https://github.com/Strayer/django-gcloud-storage/pull/13.patch
https://github.com/Strayer/django-gcloud-storage/pull/13.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#13
|
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.
Hey @scattym,
sorry for taking so long… thanks again for the PR!
The code itself looks fine, but this really needs tests. I think there should be at least three tests:
test_should_cache_urls_when_enabled
Basically check wether a second call returns the identical URL. This may generate false positives if the gcloud API itself generates an identical URL, but I this would come up with the other test.
test_should_not_cache_urls_by_default
As before, but simply checking a second call returns a different URL.
test_should_not_return_cached_urls_when_cache_disabled
This is a little more complicated. This should test that a different URL is returned when caching is disabled but this cache_key does have an entry in the cache.
For reference: Changing settings within a test is already done here: https://github.com/Strayer/django-gcloud-storage/blob/db78650/tests/test_class.py#L256-L258
I don't think testing cache expiry and such is required, that should be handled by Djangos core tests already.
Use the caching feature of django core to cache urls. Using this
feature we can create multi-day urls and keep them cached in
whatever caching backend is configured. May only help in certain
use cases, but one of those use cases is mine :)