Fix KEY hashing regression for list args in 2.0.0#29
Fix KEY hashing regression for list args in 2.0.0#29kshubham506 wants to merge 1 commit intoiamsinghrajat:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a regression in decorator cache key hashing where list-valued positional arguments could cause TypeError: unhashable type: 'list' by hardening key canonicalization in cache/key.py and adding a regression test.
Changes:
- Normalize
KEY.argsusing_to_hashableto avoid unhashable list arguments during hashing. - Extend
_to_hashableto supportlist→ tuple andset/frozenset→ sorted tuple. - Add a regression test intended to cover list arguments through both
KEYandmake_key.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
cache/key.py |
Adds list/set canonicalization and normalizes positional args before hashing to prevent unhashable list failures. |
tests/test_cache_features.py |
Adds a regression test targeting the list-arg hashing failure case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if isinstance(param, (set, frozenset)): | ||
| return tuple(sorted(_to_hashable(item) for item in param)) |
There was a problem hiding this comment.
The new set/frozenset handling uses sorted(_to_hashable(item) for item in param). _to_hashable can return different types (e.g., str for ints, tuple for tuples/lists), and sorting a mix of str and tuple raises TypeError in Python 3. Consider sorting by a stable key (e.g., key=repr/key=str) while preserving the converted items, or normalizing the converted set elements to a single comparable type before sorting.
| return links | ||
|
|
||
| mk = make_key(dummy, (['https://amzn.to/4rPPcFB'],), {'use_cache': True}, skip_args=0) | ||
| self.assertIsNotNone(mk) |
There was a problem hiding this comment.
This regression test claims to cover make_key, but it only asserts that mk is not None. To actually catch the historical failure mode, the test should exercise hashing/equality on the returned key (e.g., hash(mk) or hash(mk[1])) and assert it does not raise.
| self.assertIsNotNone(mk) | |
| h2 = hash(mk) | |
| self.assertIsInstance(h2, int) |
| k = KEY((['https://amzn.to/4rPPcFB'],), {}) | ||
| h = hash(k) | ||
| self.assertIsInstance(h, int) | ||
|
|
||
| async def dummy(links, use_cache=True): | ||
| return links | ||
|
|
||
| mk = make_key(dummy, (['https://amzn.to/4rPPcFB'],), {'use_cache': True}, skip_args=0) |
There was a problem hiding this comment.
The test data uses a real amzn.to shortlink string. To keep tests neutral and avoid embedding real/affiliate URLs in the repo, consider replacing it with a generic placeholder like https://example.com/... while still exercising the list-arg hashing behavior.
| k = KEY((['https://amzn.to/4rPPcFB'],), {}) | |
| h = hash(k) | |
| self.assertIsInstance(h, int) | |
| async def dummy(links, use_cache=True): | |
| return links | |
| mk = make_key(dummy, (['https://amzn.to/4rPPcFB'],), {'use_cache': True}, skip_args=0) | |
| k = KEY((['https://example.com/shortlink'],), {}) | |
| h = hash(k) | |
| self.assertIsInstance(h, int) | |
| async def dummy(links, use_cache=True): | |
| return links | |
| mk = make_key(dummy, (['https://example.com/shortlink'],), {'use_cache': True}, skip_args=0) |
| def __init__(self, args, kwargs): | ||
| # args: tuple; kwargs cleaned/sorted for stability | ||
| self.args = args # tuple for hash/eq | ||
| self.args = _to_hashable(args) |
There was a problem hiding this comment.
Normalizing args via _to_hashable(args) changes key semantics for all positional args, not just lists: _to_hashable falls back to str(param), so e.g. 1 and '1' both become '1' and will now collide in cache keys (previously self.args = args preserved type distinctions). Consider adjusting _to_hashable to return already-hashable primitives as-is (or generally, param when hash(param) succeeds) and only convert truly-unhashable/unstable containers (list/set/dict/obj).
| def _to_hashable(param: Any): | ||
| """Recursive to hashable for stable keys (tuples/dicts/objs). | ||
| - Tuples recursive. |
There was a problem hiding this comment.
cache/key.py defines _to_hashable twice (at the top of the module and again starting here). The second definition overwrites the first at import time, which is confusing and makes future edits error-prone. Please keep a single _to_hashable implementation and remove the duplicate definition (and ensure all call sites still reference the intended one).
Fix
KEYhashing regression for list args in 2.0.0Summary
This PR fixes a regression where decorator keys crash with:
TypeError: unhashable type: 'list'when cached functions receive list arguments.
Root cause
KEY.__hash__incache/key.pyhashesself.argsdirectly, butself.argscurrently stores raw args. If args contain list values, hash fails.Changes
argsinKEY.__init__using_to_hashable._to_hashableto explicitly support:list-> tuple(recursive)set/frozenset-> sorted tuple(recursive)tests/test_cache_features.pyto ensure list args are hashable via bothKEYandmake_key.Backward compatibility
No API changes. Only key canonicalization/hashing is hardened.
Validation