Skip to content

Fix KEY hashing regression for list args in 2.0.0 #29

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.

Sign up for GitHub

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

Open
kshubham506 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix KEY hashing regression for list args in 2.0.0 #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion cache/key.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,19 @@
def _to_hashable(param: Any):
"""Recursive to hashable for stable keys (tuples/dicts/objs).
- Tuples recursive.
- Lists: tuple(recursive).
- Sets/frozensets: sorted tuple(recursive).
- Dicts: sorted items tuple.
- Objs: str(sorted vars) (deterministic).
- Else: str (fallback).
Fixes old unstable str(dict.items())/vars.
"""
if isinstance(param, tuple):
return tuple(map(_to_hashable, param))
if isinstance(param, list):
return tuple(map(_to_hashable, param))
if isinstance(param, (set, frozenset)):
return tuple(sorted(_to_hashable(item) for item in param))
Comment on lines +18 to +19
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
if isinstance(param, dict):
return tuple(sorted((k, _to_hashable(v)) for k, v in param.items()))
elif hasattr(param, "__dict__"):
Expand All @@ -32,7 +38,7 @@ class KEY:

def __init__(self, args, kwargs):
# args: tuple; kwargs cleaned/sorted for stability
self.args = args # tuple for hash/eq
self.args = _to_hashable(args)
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.
# copy + remove use_cache (decorator param) + sort for stability
kw = dict(kwargs) # copy to avoid side-effect on caller
kw.pop("use_cache", None)
Expand All @@ -57,13 +63,19 @@ def __repr__(self):
def _to_hashable(param: Any):
"""Recursive to hashable for stable keys (tuples/dicts/objs).
- Tuples recursive.
Comment on lines 63 to 65
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.
- Lists: tuple(recursive).
- Sets/frozensets: sorted tuple(recursive).
- Dicts: sorted items tuple.
- Objs: str(vars) (deterministic repr).
- Else: str (fallback).
Prevents instability vs. old str/dict.items().
"""
if isinstance(param, tuple):
return tuple(map(_to_hashable, param))
if isinstance(param, list):
return tuple(map(_to_hashable, param))
if isinstance(param, (set, frozenset)):
return tuple(sorted(_to_hashable(item) for item in param))
if isinstance(param, dict):
return tuple(sorted((k, _to_hashable(v)) for k, v in param.items()))
elif hasattr(param, "__dict__"):
Expand Down
14 changes: 14 additions & 0 deletions tests/test_cache_features.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,20 @@ async def dummy(a, b, use_cache=True): pass
# used in cache (e.g. herd/batch keys stable)
# (implicit via other tests)

def test_key_with_list_args_is_hashable(self):
"""Regression: list in args should not raise TypeError in KEY hash."""
from cache.key import KEY, make_key

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)
Comment on lines +129 to +136
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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)

Copilot uses AI. Check for mistakes.
self.assertIsNotNone(mk)
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
self.assertIsNotNone(mk)
h2 = hash(mk)
self.assertIsInstance(h2, int)

Copilot uses AI. Check for mistakes.

def test_lru_concurrent_eviction(self):
"""Test for concurrency bug in LRU re-runs with unique keys near maxsize.
Pre-fix (race in move_to_end/evict): hits dropped weirdly (e.g., 3% for size=94 vs ~90% for 95).
Expand Down
Loading