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

feat: on-get callback #38

Merged
merged 3 commits into from
Nov 13, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
9 changes: 9 additions & 0 deletions src/cacheout/cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ class Cache:
default: Default value or function to use in :meth:`get` when key is not found. If callable,
it will be passed a single argument, ``key``, and its return value will be set for that
cache key.
on_get: Callback which will be executed when a cache entry is got.
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of is got, maybe is retrieved.

on_delete: Callback which will be executed when a cache entry is removed.
Copy link
Owner

Choose a reason for hiding this comment

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

Would be good to include the excepted method signatures for on_get and on_delete in terms of what the positional arguments are since just the types don't give enough context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any example?

Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
on_get: Callback which will be executed when a cache entry is got.
on_delete: Callback which will be executed when a cache entry is removed.
on_get: Callback that will be executed when a cache entry is retrieved. It is called with arguments ``(key, value, exists)`` where `key` is the cache key, `value` is the value retrieved (could be the default), and `exists` is whether the cache key exists or not.
on_delete: Callback that will be executed when a cache entry is removed. It is called with arguments ``(key, value, cause)`` where `key` is the cache key, `value` is the cached value at the time of deletion, and `cause` is the reason the key was removed (see :class:`RemovalCause` for enumerated causes).

stats: Cache statistics.
"""
Expand All @@ -90,12 +91,14 @@ def __init__(
timer: t.Callable[[], T_TTL] = time.time,
default: t.Any = None,
enable_stats: bool = False,
on_get: t.Optional[t.Callable[[t.Hashable, t.Any, bool], None]] = None,
on_delete: t.Optional[t.Callable[[t.Hashable, t.Any, RemovalCause], None]] = None,
):
self.maxsize = maxsize
self.ttl = ttl
self.timer = timer
self.default = default
self.on_get = on_get
self.on_delete = on_delete
self.stats = CacheStatsTracker(self, enable=enable_stats)

Expand Down Expand Up @@ -262,6 +265,9 @@ def _get(self, key: t.Hashable, default: t.Any = None) -> t.Any:
self._delete(key, RemovalCause.EXPIRED)
raise KeyError
self.stats.inc_hit_count()

if self.on_get:
self.on_get(key, value, True)
except KeyError:
Copy link
Owner

Choose a reason for hiding this comment

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

There's an edge-case here where if self.on_get() throws a KeyError, then we end up calling it twice. Maybe we should defer calling self.on_get until after the try/except block. That way we'd only have a single place where it's called and if it happens to raise a KeyError, we won't mistakenly catch it.

self.stats.inc_miss_count()
if default is None:
Expand All @@ -273,6 +279,9 @@ def _get(self, key: t.Hashable, default: t.Any = None) -> t.Any:
else:
value = default

if self.on_get:
self.on_get(key, value, False)

return value

def get_many(self, iteratee: T_FILTER) -> dict:
Expand Down
30 changes: 24 additions & 6 deletions tests/test_cache.py
Original file line number Diff line number Diff line change
Expand Up @@ -719,32 +719,50 @@ def test_cache_on_delete(cache: Cache, timer: Timer):

def on_delete(key, value, cause):
nonlocal log
log = f"{key}:{value} {cause.value}"
log = f"{key}={value}, RemovalCause={cause.value}"

cache.on_delete = on_delete
cache.set("DELETE", 1)
cache.delete("DELETE")
assert log == f"DELETE:1 {RemovalCause.DELETE.value}"
assert log == f"DELETE=1, RemovalCause={RemovalCause.DELETE.value}"

cache.set("SET", 1)
cache.set("SET", 2)
assert log == f"SET:1 {RemovalCause.SET.value}"
assert log == f"SET=1, RemovalCause={RemovalCause.SET.value}"

cache.clear()
cache.set("POPITEM", 1)
cache.popitem()
assert log == f"POPITEM:1 {RemovalCause.POPITEM.value}"
assert log == f"POPITEM=1, RemovalCause={RemovalCause.POPITEM.value}"

cache.set("EXPIRED", 1, ttl=1)
timer.time = 1
cache.delete_expired()
assert log == f"EXPIRED:1 {RemovalCause.EXPIRED.value}"
assert log == f"EXPIRED=1, RemovalCause={RemovalCause.EXPIRED.value}"

cache.clear()
cache.maxsize = 1
cache.set("FULL", 1)
cache.set("OVERFLOW", 2)
assert log == f"FULL:1 {RemovalCause.FULL.value}"
assert log == f"FULL=1, RemovalCause={RemovalCause.FULL.value}"


def test_cache_on_get(cache: Cache):
"""Test that on_get(cache) callback."""
log = ""

def on_get(key, value, existed):
nonlocal log
log = f"{key}={value}, existed={existed}"

cache.on_get = on_get
cache.set("hit", 1)

cache.get("hit")
assert log == "hit=1, existed=True"

cache.get("miss")
assert log == "miss=None, existed=False"


def test_cache_stats__disabled_by_default(cache: Cache):
Expand Down