Repository navigation
Make cache keys memcached-safe - #11
willgearty merged 5 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The changes appear correct and well-covered by the new warning-as-error regression tests, with only a minor docstring capitalization nit noted.
Pull request overview
This PR makes cache keys memcached-safe by hashing the variable/argument portion of cache keys while keeping the human-recognizable cache/token name prefix intact, and adds regression tests that fail on CacheKeyWarning.
Changes:
- Hash
ArgCache.key()andToken.key_filt()argument components with SHA-256 to avoid memcached-invalid characters and unbounded key length. - Introduce
marinade_dish_str()to normalizemarinade_dish()output tostrwithout repeated conversions. - Add
CacheKeySafetyTestthat promotesCacheKeyWarningto an error and exercises key-building paths (awkward args, model invalidation, cached inclusion tag).
File summaries
| File | Description |
|---|---|
| tests/tests.py | Adds regression tests that error on CacheKeyWarning and exercise key construction/invalidation paths. |
| src/marinade.py | Adds marinade_dish_str() helper and uses it to avoid repeated marinading/decoding in list handling. |
| src/cache_token.py | Hashes the filtered-args portion of token keys while preserving TOKEN__ and token name prefixes. |
| src/argcache.py | Hashes the argument portion of ArgCache keys while preserving the cache name prefix. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ArgCache.key() and Token.key_filt() built keys by stringifying arguments through marinade_dish() with no sanitization or length bound, so keys routinely contained spaces, brackets, braces and quotes, and could exceed memcached's 250-byte limit. The locmem backend used by the tests only warns (CacheKeyWarning), but a memcached backend would either error out or silently drop the cached value. Hash the variable portion of the key with SHA-256, keeping the cache name (and the TOKEN__ prefix) in front so keys stay recognizable when debugging. pretty_name is built from the name and params rather than from per-argument keys, so the view_all page is unaffected. While normalizing the marinaded arguments, collapse the repeated "marinade_dish(arg) if isinstance(marinade_dish(arg), bytes)" idiom into marinade_dish_str(). It is still needed, because force_str() returns bytes for anything that is not already a str, but calling marinade_dish() twice per argument meant nested lists cost 2^depth marinades and a QuerySet argument was evaluated twice. Add CacheKeySafetyTest, which promotes CacheKeyWarning to an error with warnings.catch_warnings() and then exercises the key-building paths: awkward argument types, cached model methods along with the token keys used to expire them, and the cached inclusion tag that produced the longest keys. The tests clear the cache and reset the shared counters around themselves, since both outlive an individual test. Fixes learning-unlimited#8.
53600be to
9c709f9
Compare
force_str() still returned str(x).encode('utf8') for anything that was
not already a str, a leftover from the Python 3 migration. That is why
every caller needed an "isinstance(..., bytes)" check, which the previous
commit collected into marinade_dish_str(). Fix it at the source instead,
so marinade_dish() always returns a str and the wrapper can go.
The marinaded strings are unchanged (only their type is), so cache keys
are the same as before this commit.
Hashing only the arguments left the name unbounded, and names can get long: the cached inclusion tag's render cache is named after both the node class and the tag function (111 characters in the test suite). Hash the whole thing instead, so every value key is 64 hex characters and every token key is 71. The hashed string is exactly what the key used to be before this PR, name + '|' + marinaded arguments, so no two calls that had different keys before now share one. Both ArgCache.key() and Token.key_filt() go through the new hash_key() helper so the two can't drift apart. Token keys keep their TOKEN__ prefix, which is now what separates them from value keys: the global token has the same name as its cache, so for a function with no arguments the two hashes would otherwise be identical.
cache.clear() is not implemented by every cache backend. Use registry.dump_all_caches() instead, which calls argcache's own delete_all() on every registered cache and so works on any backend. Also spell GitHub with a capital H in the class docstring.
The existing jobs use the locmem backend, which only warns about keys memcached can't store, so a key bug like learning-unlimited#8 passes CI. Add a job that runs the same suite with PyMemcacheCache against a memcached service container; that backend raises InvalidCacheKey for such keys, so they fail the build instead. tests/settings.py switches to memcached when ARGCACHE_TEST_MEMCACHED is set to its address, and leaves the default locmem setup alone otherwise. Run locally against memcached:1.6, the new job passes on this branch; on django5 without the key fix, 13 of 19 tests fail with InvalidCacheKey.
willgearty
left a comment
willgearty
left a comment
There was a problem hiding this comment.
Thanks for putting this together! I've left some comments below. Also, as you note, we don't actually test this with memcached. If you're up for it, a matrix entry with a memcached service and CACHES set to PyMemcacheCache would turn this class of bug from advisory into a hard failure.
|
Thanks for the review! I took you up on the memcached suggestion. bd200ca adds a I made it a separate job rather than a matrix entry so the memcached container only starts where it's needed. Happy to fold it into the matrix if you'd prefer that. Before pushing, I ran it locally against The workflow run is waiting for approval again. |
willgearty
left a comment
willgearty
left a comment
There was a problem hiding this comment.
Looks good, thanks for addressing my comments and for your contribution!
Fixes #8. Built off
django5, as requested in the issue thread.What changed
ArgCache.key()andToken.key_filt()now hash the variable portion of the key with SHA-256, keeping the cache name — and theTOKEN__prefix — in front so keys stay recognizable:pretty_nameis built fromself.nameandself.params, not from per-argument keys, soview_all.htmlrenders exactly as before.One deviation from the proposed patch
The proposal carried over the existing
marinade_dish(arg).decode('UTF-8') if isinstance(marinade_dish(arg), bytes) else marinade_dish(arg)idiom. That normalization is genuinely needed —force_str()returnsstr(x).encode('utf8'), i.e. bytes, for anything that is not already astr— but it callsmarinade_dish()twice per argument. This PR collapses it into a singlemarinade_dish_str()helper inmarinade.pyand uses that at all three call sites.That also fixes a latent bug in
marinade_dish()itself, which used the same doubled idiom in its list branch: nested lists cost 2^depth marinades, and aQuerySetargument was fully evaluated to a list twice.Tests
New
CacheKeySafetyTestpromotesCacheKeyWarningto an error viawarnings.catch_warnings()/simplefilter('error', CacheKeyWarning), then exercises the paths that build keys:frozenset,dict, nested list,None, a 500-character string, non-ASCII; each called twice, for the miss path and the hit pathdelete_all()through the global tokenThese tests clear the cache and reset the shared module-level counters around themselves, because both outlive an individual test and these cases deliberately cache values under arguments that other tests also use.
Verified on Python 3.11 / Django 5.2:
@unittest.skipfor Upgrade to Django 3.x and Python 3.10 #2), and zeroCacheKeyWarninglines, down from 34 ondjango5src/reverted: all three new tests fail, so the regression test actually bitesNotes for review
argcache.extras.template.CachedInclusionNode.render_given_args*<...>plus Django's:1:prefix and the 64-character digest — could still approach memcached's 250-byte cap. Happy to bound the name too if you would rather close that off in this PR.validate_key.