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

Use strings for cache keys #226

Merged
merged 1 commit into from Feb 23, 2019
Merged

Conversation

adamchainz
Copy link

As discovered in adamchainz/django-perf-rec#21, django-waffle is using bytes objects for cache keys. This seems to work with most of the caching infrastructure, but it's not documented as supported in the Django cache docs nor does it seem to be tested for in its test suite. Thus some cache backends may not support bytes, and django-perf-rec currently doesn't support them either.

@@ -22,7 +22,7 @@ def keyfmt(k, v=None):
key = prefix + k
else:
key = prefix + hashlib.md5((k % v).encode('utf-8')).hexdigest()
return key.encode('utf-8')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this was added specifically because unencoded utf8 characters break multiple cache backends (in particular memcache). The Python code is OK with it, but memcached itself was not. If the CACHE_PREFIX setting is set to an unsafe string, this will break. k should only come from within the library, so that should be OK, and v gets encoded and digested, so it should be safe.

At the very least, this would need docs added to the settings reference that made it clear CACHE_PREFIX needs to be plain ascii.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plain ASCII restrict exists on memcache but afaik all other Django core cache backends support unicode, or can support it in the right conditions, e.g. correct database setup for dbcache. I can document it, but Django does already emit warnings when forming cache keys that won't work on memcached: https://github.com/django/django/blob/master/django/core/cache/backends/base.py#L226

As discovered in adamchainz/django-perf-rec#21, django-waffle is using `bytes` objects for cache keys. This seems to work with most of the caching infrastructure, but it's not documented as supported in the [Django cache docs](https://docs.djangoproject.com/en/1.10/topics/cache/) nor does it seem to be tested for in its test suite. Thus some cache backends may not support `bytes`, and `django-perf-rec` currently doesn't support them either.
@adamchainz
Copy link
Author

Added documentation

@jsocol jsocol mentioned this pull request Jun 24, 2017
@stale stale bot added the stale label Aug 26, 2017
@stale
Copy link

stale bot commented Aug 26, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot closed this Sep 9, 2017
@stale
Copy link

stale bot commented Sep 9, 2017

This issue has been closed as stale because it has not had any recent activity. Please feel free to re-open if the issue is still relevant. Thank you for your contributions!

@jeffbr13
Copy link

Hi there! Just wanted to note that I'm having the same issue using django-perf-rec as well! Since there are new maintainers (@rlucioni and @ahsan-ul-haq) perhaps we could rescue this from stale issues, since it's a simple (although potentially breaking) fix?

@clintonb clintonb merged commit 776f461 into jazzband:master Feb 23, 2019
@adamchainz adamchainz deleted the cache_keys_strings branch February 23, 2019 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants