From c2c1ea30ee629f6d1d1940022e60adc58892fca3 Mon Sep 17 00:00:00 2001 From: allen0099 Date: Fri, 2 Oct 2026 13:04:04 +0000 Subject: [PATCH] feat(backends)!: make delete() return whether a key was removed (#71) BaseCacheBackend.delete() now returns bool on memory, Redis and Memcached. The base delete_many() fallback counts only keys that existed, and the get_and_delete() / delete_if_equals() fallbacks no longer let a caller win after its delete found the key gone. --- changelog.d/71.changed.md | 9 +++++++++ fastapi_cachex/backends/base.py | 28 ++++++++++++++++------------ fastapi_cachex/backends/memcached.py | 21 ++++++++++++--------- fastapi_cachex/backends/memory.py | 10 ++++++---- fastapi_cachex/backends/redis.py | 9 +++++---- tests/backends/test_base.py | 24 +++++++++++++++++++++--- tests/backends/test_memcached.py | 15 ++++++++++++++- tests/backends/test_memory.py | 6 ++++-- tests/backends/test_redis.py | 3 ++- tests/session/test_manager.py | 4 ++-- 10 files changed, 91 insertions(+), 38 deletions(-) create mode 100644 changelog.d/71.changed.md diff --git a/changelog.d/71.changed.md b/changelog.d/71.changed.md new file mode 100644 index 0000000..9ee52ae --- /dev/null +++ b/changelog.d/71.changed.md @@ -0,0 +1,9 @@ +**`BaseCacheBackend.delete()` returns whether a key was removed.** The memory, +Redis and Memcached backends return `True` when the key held an entry and +`False` otherwise; on memory an entry that had already expired counts as +absent, as it does on Redis. The base `delete_many()` fallback now counts only +the keys that existed, and the non-atomic `get_and_delete()` and +`delete_if_equals()` fallbacks use the result, so a caller whose delete found +the key already gone no longer wins as well. A third-party backend that declares `-> None` fails type +checking and should return the result instead. See the +[migration guide](https://fastapi-cachex.readthedocs.io/en/stable/MIGRATING_0_4/#backend-delete). diff --git a/fastapi_cachex/backends/base.py b/fastapi_cachex/backends/base.py index d05f09f..26c5140 100644 --- a/fastapi_cachex/backends/base.py +++ b/fastapi_cachex/backends/base.py @@ -140,21 +140,24 @@ async def set(self, key: str, value: CacheEntry, ttl: int | None = None) -> None """ @abstractmethod - async def delete(self, key: str) -> None: - """Remove a response from the cache.""" + async def delete(self, key: str) -> bool: + """Remove a response from the cache. + + Returns: + Whether ``key`` held an entry that had not expired yet + """ async def delete_many(self, keys: Iterable[str]) -> int: """Remove every key in ``keys``; returns how many were removed. - The base implementation deletes one key at a time and reports how - many were attempted, since ``delete`` does not say whether the key - existed. The built-in backends override it and count what was - actually removed. + The base implementation deletes one key at a time and counts the + deletes that found an entry. The built-in backends override it with + batched deletes. """ count = 0 for key in keys: - await self.delete(key) - count += 1 + if await self.delete(key): + count += 1 return count async def get_and_delete(self, key: str) -> CacheEntry | None: @@ -172,8 +175,10 @@ async def get_and_delete(self, key: str) -> CacheEntry | None: The entry that was stored under ``key``, or ``None`` if there was none """ value = await self.get(key) - if value is not None: - await self.delete(key) + if value is None or not await self.delete(key): + # Absent, or another caller removed it between the get and the + # delete: that caller got the entry. + return None return value async def set_if_absent( @@ -226,8 +231,7 @@ async def delete_if_equals(self, key: str, expected: CacheEntry) -> bool: """ if await self.get(key) != expected: return False - await self.delete(key) - return True + return await self.delete(key) async def expire_if_equals(self, key: str, expected: CacheEntry, ttl: int) -> bool: """Update expiry on ``key`` to ``ttl`` seconds only while it still holds ``expected``. diff --git a/fastapi_cachex/backends/memcached.py b/fastapi_cachex/backends/memcached.py index a31af95..1f8eec9 100644 --- a/fastapi_cachex/backends/memcached.py +++ b/fastapi_cachex/backends/memcached.py @@ -443,15 +443,21 @@ async def increment(self, key: str, delta: int = 1, ttl: int | None = None) -> i logger.debug("Memcached INCREMENT; key=%s value=%s ttl=%s", key, value, ttl) return value - async def delete(self, key: str) -> None: + async def delete(self, key: str) -> bool: """Delete value from cache. Args: key: Cache key to delete + + Returns: + Whether the server held ``key`` """ prefixed = self._make_key(key) - await asyncio.to_thread(self.client.delete, prefixed) - logger.debug("Memcached DELETE; key=%s", key) + removed = bool( + await asyncio.to_thread(self.client.delete, prefixed, noreply=False) + ) + logger.debug("Memcached DELETE; key=%s removed=%s", key, removed) + return removed async def delete_many(self, keys: Iterable[str]) -> int: """Remove every key in ``keys`` in one worker call; returns how many existed. @@ -525,17 +531,14 @@ async def clear_path(self, path: str, include_params: bool = False) -> int: ) # Try to delete the prefixed key (exact match only) - prefixed_key = self._make_key(path) - result = await asyncio.to_thread( - self.client.delete, prefixed_key, noreply=False - ) + removed = int(await self.delete(path)) logger.debug( "Memcached CLEAR_PATH; path=%s include_params=%s removed=%s", path, include_params, - 1 if result else 0, + removed, ) - return 1 if result else 0 + return removed async def clear_pattern(self, pattern: str) -> int: """Clear cached responses matching a pattern. diff --git a/fastapi_cachex/backends/memory.py b/fastapi_cachex/backends/memory.py index be350ae..25b9aa8 100644 --- a/fastapi_cachex/backends/memory.py +++ b/fastapi_cachex/backends/memory.py @@ -166,11 +166,13 @@ async def set(self, key: str, value: CacheEntry, ttl: int | None = None) -> None self.cache[key] = CacheItem(value=value, expiry=expiry) logger.debug("Memory cache SET; key=%s ttl=%s", key, ttl) - async def delete(self, key: str) -> None: - """Remove a response from the cache.""" + async def delete(self, key: str) -> bool: + """Remove a response from the cache; returns whether it had not expired.""" async with self.lock: - self.cache.pop(key, None) - logger.debug("Memory cache DELETE; key=%s", key) + item = self.cache.pop(key, None) + removed = item is not None and _is_live(item, time.time()) + logger.debug("Memory cache DELETE; key=%s removed=%s", key, removed) + return removed async def delete_many(self, keys: Iterable[str]) -> int: """Remove every key in ``keys`` under one lock; returns how many existed.""" diff --git a/fastapi_cachex/backends/redis.py b/fastapi_cachex/backends/redis.py index f092d99..be4b246 100644 --- a/fastapi_cachex/backends/redis.py +++ b/fastapi_cachex/backends/redis.py @@ -309,10 +309,11 @@ async def set(self, key: str, value: CacheEntry, ttl: int | None = None) -> None await self.client.set(self._make_key(key), encode_entry(value), ex=ttl) logger.debug("Redis SET; key=%s ttl=%s", key, ttl) - async def delete(self, key: str) -> None: - """Remove a response from the cache.""" - await self.client.delete(self._make_key(key)) - logger.debug("Redis DELETE; key=%s", key) + async def delete(self, key: str) -> bool: + """Remove a response from the cache; returns whether the key existed.""" + removed = bool(await self.client.delete(self._make_key(key))) + logger.debug("Redis DELETE; key=%s removed=%s", key, removed) + return removed async def delete_many(self, keys: Iterable[str]) -> int: """Remove every key in ``keys`` with batched DELs; returns how many existed.""" diff --git a/tests/backends/test_base.py b/tests/backends/test_base.py index a000a21..3d0c31d 100644 --- a/tests/backends/test_base.py +++ b/tests/backends/test_base.py @@ -24,8 +24,8 @@ async def get(self, key: str) -> CacheEntry | None: async def set(self, key: str, value: CacheEntry, ttl: int | None = None) -> None: self.store[key] = (value, ttl) - async def delete(self, key: str) -> None: - self.store.pop(key, None) + async def delete(self, key: str) -> bool: + return self.store.pop(key, None) is not None async def clear(self) -> None: self.store.clear() @@ -76,11 +76,29 @@ async def test_get_and_delete_fallback_returns_then_removes( assert await backend.get_and_delete("once") is None +async def test_fallbacks_lose_to_a_delete_between_get_and_delete( + backend: DictBackend, monkeypatch: pytest.MonkeyPatch +) -> None: + """Another caller removed the key after the get: only it got the entry.""" + value = CacheEntry(fingerprint="e", content=b"once") + await backend.set("once", value) + + async def lost_the_race(key: str) -> bool: + backend.store.pop(key, None) + return False + + monkeypatch.setattr(backend, "delete", lost_the_race) + + assert await backend.get_and_delete("once") is None + await backend.set("once", value) + assert await backend.delete_if_equals("once", value) is False + + async def test_delete_many_fallback_deletes_one_by_one(backend: DictBackend) -> None: await backend.set("a", CacheEntry(fingerprint="e", content=b"1")) await backend.set("b", CacheEntry(fingerprint="e", content=b"2")) - assert await backend.delete_many(["a", "b", "missing"]) == 3 + assert await backend.delete_many(["a", "b", "missing"]) == 2 assert backend.store == {} diff --git a/tests/backends/test_memcached.py b/tests/backends/test_memcached.py index 34b48f8..8282ac3 100644 --- a/tests/backends/test_memcached.py +++ b/tests/backends/test_memcached.py @@ -113,10 +113,11 @@ async def test_memcached_delete(memcached_backend: MemcachedBackend): value = CacheEntry(fingerprint="test_etag", content=b"test_content") await memcached_backend.set(key, value) - await memcached_backend.delete(key) + assert await memcached_backend.delete(key) is True retrieved_value = await memcached_backend.get(key) assert retrieved_value is None + assert await memcached_backend.delete(key) is False @requires_memcached @@ -1079,6 +1080,18 @@ async def test_memcached_delete_many_sends_each_namespaced_key_once() -> None: ) +@pytest.mark.parametrize("present", [True, False]) +async def test_memcached_delete_reports_what_the_server_answered( + present: bool, +) -> None: + backend = stubbed_backend() + backend.client.delete.return_value = present + + assert await backend.delete("k") is present + + backend.client.delete.assert_called_once_with(backend._make_key("k"), noreply=False) + + async def test_memcached_delete_many_with_no_keys_does_no_io() -> None: backend = stubbed_backend() diff --git a/tests/backends/test_memory.py b/tests/backends/test_memory.py index 303a6d4..02c96b1 100644 --- a/tests/backends/test_memory.py +++ b/tests/backends/test_memory.py @@ -69,10 +69,11 @@ async def test_memory_backend_delete(memory_backend: MemoryBackend): ttl = 60 await memory_backend.set(key, value, ttl) - await memory_backend.delete(key) + assert await memory_backend.delete(key) is True retrieved_value = await memory_backend.get(key) assert retrieved_value is None + assert await memory_backend.delete(key) is False async def test_memory_backend_clear(memory_backend: MemoryBackend): @@ -599,8 +600,9 @@ async def test_memory_backend_enumeration_skips_expired_entries( lambda backend: backend.clear_pattern("stale"), lambda backend: backend.clear_path("stale"), lambda backend: backend.delete_many(["stale"]), + lambda backend: backend.delete("stale"), ], - ids=["clear_pattern", "clear_path", "delete_many"], + ids=["clear_pattern", "clear_path", "delete_many", "delete"], ) async def test_memory_backend_does_not_count_expired_entries_as_cleared( memory_backend: MemoryBackend, clear: Callable[[MemoryBackend], Awaitable[int]] diff --git a/tests/backends/test_redis.py b/tests/backends/test_redis.py index 458b9db..7c04860 100644 --- a/tests/backends/test_redis.py +++ b/tests/backends/test_redis.py @@ -353,8 +353,9 @@ async def test_set_with_ttl(self, async_redis_backend: AsyncRedisCacheBackend): async def test_delete(self, async_redis_backend: AsyncRedisCacheBackend): value = CacheEntry(fingerprint="test-etag", content=b"test-content") await async_redis_backend.set("test-key", value) - await async_redis_backend.delete("test-key") + assert await async_redis_backend.delete("test-key") is True assert await async_redis_backend.get("test-key") is None + assert await async_redis_backend.delete("test-key") is False @requires_redis async def test_clear(self, async_redis_backend: AsyncRedisCacheBackend): diff --git a/tests/session/test_manager.py b/tests/session/test_manager.py index 9c400fd..119600d 100644 --- a/tests/session/test_manager.py +++ b/tests/session/test_manager.py @@ -755,9 +755,9 @@ def __init__(self) -> None: self.deletes = 0 self.batches: list[int] = [] - async def delete(self, key: str) -> None: + async def delete(self, key: str) -> bool: self.deletes += 1 - await super().delete(key) + return await super().delete(key) async def delete_many(self, keys: Iterable[str]) -> int: keys = list(keys)