From 419dc680a9ed2fe6394d2a558477e42605f11edd Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Mon, 27 Oct 2025 00:31:41 +0000 Subject: [PATCH 1/3] fix: address critical and high-severity issues from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit addresses 8 out of 10 critical/high-severity issues identified in the comprehensive code review for the 0.3.0 release. Critical fixes: - Fix DynamoDB TTL crash when created_at is None - Fix Passthrough Cache race condition by deleting cache after primary ops - Fix Statistics wrapper falsiness bug for empty dict values - Add documentation to TTL Clamp wrapper clarifying behavior High-severity fixes: - Fix Memory Store collection access with proper error handling - Fix Encryption wrapper exception type (TypeError not JSONDecodeError) - Fix Limit Size wrapper empty list edge case - Add missing super().__init__() calls in Routing and DefaultValue wrappers Not addressed in this commit: - DynamoDB bulk operations (requires significant implementation effort) - DynamoDB client initialization (already correct on review) Resolves #137 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: William Easton --- .../key_value/aio/stores/dynamodb/store.py | 2 +- .../src/key_value/aio/stores/memory/store.py | 24 +++++++++++++++---- .../aio/wrappers/default_value/wrapper.py | 2 ++ .../key_value/aio/wrappers/encryption/base.py | 2 +- .../aio/wrappers/limit_size/wrapper.py | 3 ++- .../aio/wrappers/passthrough_cache/wrapper.py | 16 ++++++++----- .../key_value/aio/wrappers/routing/wrapper.py | 2 ++ .../aio/wrappers/statistics/wrapper.py | 6 +++-- .../aio/wrappers/ttl_clamp/wrapper.py | 7 +++++- 9 files changed, 48 insertions(+), 16 deletions(-) diff --git a/key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py b/key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py index a89c641f..142a66ab 100644 --- a/key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py +++ b/key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py @@ -203,7 +203,7 @@ async def _put_managed_entry( } # Add TTL if present - if managed_entry.ttl is not None and managed_entry.created_at: + if managed_entry.ttl is not None and managed_entry.created_at is not None: # DynamoDB TTL expects a Unix timestamp ttl_timestamp = int(managed_entry.created_at.timestamp() + managed_entry.ttl) item["ttl"] = {"N": str(ttl_timestamp)} diff --git a/key-value/key-value-aio/src/key_value/aio/stores/memory/store.py b/key-value/key-value-aio/src/key_value/aio/stores/memory/store.py index 06e7dc1f..fddd7dd7 100644 --- a/key-value/key-value-aio/src/key_value/aio/stores/memory/store.py +++ b/key-value/key-value-aio/src/key_value/aio/stores/memory/store.py @@ -155,7 +155,11 @@ async def _setup_collection(self, *, collection: str) -> None: @override async def _get_managed_entry(self, *, key: str, collection: str) -> ManagedEntry | None: - collection_cache: MemoryCollection = self._cache[collection] + collection_cache: MemoryCollection | None = self._cache.get(collection) + + if collection_cache is None: + msg = f"Collection '{collection}' has not been setup. Call setup_collection() first." + raise KeyError(msg) return collection_cache.get(key=key) @@ -167,19 +171,31 @@ async def _put_managed_entry( collection: str, managed_entry: ManagedEntry, ) -> None: - collection_cache: MemoryCollection = self._cache[collection] + collection_cache: MemoryCollection | None = self._cache.get(collection) + + if collection_cache is None: + msg = f"Collection '{collection}' has not been setup. Call setup_collection() first." + raise KeyError(msg) collection_cache.put(key=key, value=managed_entry) @override async def _delete_managed_entry(self, *, key: str, collection: str) -> bool: - collection_cache: MemoryCollection = self._cache[collection] + collection_cache: MemoryCollection | None = self._cache.get(collection) + + if collection_cache is None: + msg = f"Collection '{collection}' has not been setup. Call setup_collection() first." + raise KeyError(msg) return collection_cache.delete(key=key) @override async def _get_collection_keys(self, *, collection: str, limit: int | None = None) -> list[str]: - collection_cache: MemoryCollection = self._cache[collection] + collection_cache: MemoryCollection | None = self._cache.get(collection) + + if collection_cache is None: + msg = f"Collection '{collection}' has not been setup. Call setup_collection() first." + raise KeyError(msg) return collection_cache.keys(limit=limit) diff --git a/key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py b/key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py index 81536d28..2e025f11 100644 --- a/key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py +++ b/key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py @@ -40,6 +40,8 @@ def __init__( self._default_value_json = dump_to_json(obj=dict(default_value)) self._default_ttl = None if default_ttl is None else float(default_ttl) + super().__init__() + def _new_default_value(self) -> dict[str, Any]: return load_from_json(json_str=self._default_value_json) diff --git a/key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py b/key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py index e1d83c2d..363febce 100644 --- a/key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py +++ b/key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py @@ -69,7 +69,7 @@ def _encrypt_value(self, value: dict[str, Any]) -> dict[str, Any]: json_str: str = json.dumps(value, separators=(",", ":")) json_bytes: bytes = json_str.encode(encoding="utf-8") - except (json.JSONDecodeError, TypeError) as e: + except TypeError as e: msg: str = f"Failed to serialize object to JSON: {e}" raise SerializationError(msg) from e diff --git a/key-value/key-value-aio/src/key_value/aio/wrappers/limit_size/wrapper.py b/key-value/key-value-aio/src/key_value/aio/wrappers/limit_size/wrapper.py index fa5974fd..c537f949 100644 --- a/key-value/key-value-aio/src/key_value/aio/wrappers/limit_size/wrapper.py +++ b/key-value/key-value-aio/src/key_value/aio/wrappers/limit_size/wrapper.py @@ -101,4 +101,5 @@ async def put_many( filtered_keys.append(k) filtered_values.append(v) - await self.key_value.put_many(keys=filtered_keys, values=filtered_values, collection=collection, ttl=ttl) + if filtered_keys: + await self.key_value.put_many(keys=filtered_keys, values=filtered_values, collection=collection, ttl=ttl) diff --git a/key-value/key-value-aio/src/key_value/aio/wrappers/passthrough_cache/wrapper.py b/key-value/key-value-aio/src/key_value/aio/wrappers/passthrough_cache/wrapper.py index ffdab81a..7aef58f7 100644 --- a/key-value/key-value-aio/src/key_value/aio/wrappers/passthrough_cache/wrapper.py +++ b/key-value/key-value-aio/src/key_value/aio/wrappers/passthrough_cache/wrapper.py @@ -130,10 +130,10 @@ async def ttl_many(self, keys: Sequence[str], *, collection: str | None = None) @override async def put(self, key: str, value: Mapping[str, Any], *, collection: str | None = None, ttl: SupportsFloat | None = None) -> None: - _ = await self.cache_key_value.delete(collection=collection, key=key) - await self.primary_key_value.put(collection=collection, key=key, value=value, ttl=ttl) + _ = await self.cache_key_value.delete(collection=collection, key=key) + @override async def put_many( self, @@ -143,18 +143,22 @@ async def put_many( collection: str | None = None, ttl: SupportsFloat | None = None, ) -> None: - _ = await self.cache_key_value.delete_many(collection=collection, keys=keys) - await self.primary_key_value.put_many(keys=keys, values=values, collection=collection, ttl=ttl) + _ = await self.cache_key_value.delete_many(collection=collection, keys=keys) + @override async def delete(self, key: str, *, collection: str | None = None) -> bool: + result = await self.primary_key_value.delete(collection=collection, key=key) + _ = await self.cache_key_value.delete(collection=collection, key=key) - return await self.primary_key_value.delete(collection=collection, key=key) + return result @override async def delete_many(self, keys: Sequence[str], *, collection: str | None = None) -> int: + result = await self.primary_key_value.delete_many(collection=collection, keys=keys) + _ = await self.cache_key_value.delete_many(collection=collection, keys=keys) - return await self.primary_key_value.delete_many(collection=collection, keys=keys) + return result diff --git a/key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py b/key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py index dbc75459..1d596fd3 100644 --- a/key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py +++ b/key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py @@ -48,6 +48,8 @@ def __init__( self._routing_function = routing_function self._default_store = default_store + super().__init__() + def _get_store(self, collection: str | None) -> AsyncKeyValue: """Get the appropriate store for the given collection. diff --git a/key-value/key-value-aio/src/key_value/aio/wrappers/statistics/wrapper.py b/key-value/key-value-aio/src/key_value/aio/wrappers/statistics/wrapper.py index 8bb0e9a2..8326f9c0 100644 --- a/key-value/key-value-aio/src/key_value/aio/wrappers/statistics/wrapper.py +++ b/key-value/key-value-aio/src/key_value/aio/wrappers/statistics/wrapper.py @@ -113,7 +113,9 @@ def statistics(self) -> KVStoreStatistics: async def get(self, key: str, *, collection: str | None = None) -> dict[str, Any] | None: collection = collection or DEFAULT_COLLECTION_NAME - if value := await self.key_value.get(collection=collection, key=key): + value = await self.key_value.get(collection=collection, key=key) + + if value is not None: self.statistics.get_collection(collection=collection).get.increment_hit() return value @@ -127,7 +129,7 @@ async def ttl(self, key: str, *, collection: str | None = None) -> tuple[dict[st value, ttl = await self.key_value.ttl(collection=collection, key=key) - if value: + if value is not None: self.statistics.get_collection(collection=collection).ttl.increment_hit() return value, ttl diff --git a/key-value/key-value-aio/src/key_value/aio/wrappers/ttl_clamp/wrapper.py b/key-value/key-value-aio/src/key_value/aio/wrappers/ttl_clamp/wrapper.py index 163c560a..82a1ebac 100644 --- a/key-value/key-value-aio/src/key_value/aio/wrappers/ttl_clamp/wrapper.py +++ b/key-value/key-value-aio/src/key_value/aio/wrappers/ttl_clamp/wrapper.py @@ -9,7 +9,12 @@ class TTLClampWrapper(BaseWrapper): - """Wrapper that enforces a maximum TTL for puts into the store.""" + """Wrapper that enforces a maximum TTL for puts into the store. + + This wrapper only modifies write operations (put, put_many). All read operations + (get, get_many, ttl, ttl_many, delete, delete_many) pass through unchanged to + the underlying store. + """ def __init__( self, key_value: AsyncKeyValue, min_ttl: SupportsFloat, max_ttl: SupportsFloat, missing_ttl: SupportsFloat | None = None From d13ff16d2902c13b0acdf5b1bef660250f6c8fc3 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Mon, 27 Oct 2025 00:51:58 +0000 Subject: [PATCH 2/3] refactor: extract collection guard pattern into helper method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract repeated collection validation logic into a private helper method _get_collection_or_raise() to reduce code duplication in MemoryStore. - Created _get_collection_or_raise() helper with proper docstring - Refactored 4 methods to use the helper: - _get_managed_entry - _put_managed_entry - _delete_managed_entry - _get_collection_keys - Reduces 20 lines of duplicated code - Maintains identical behavior and error messages Addresses CodeRabbit review feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: William Easton --- .../src/key_value/aio/stores/memory/store.py | 40 +++++++++---------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/key-value/key-value-aio/src/key_value/aio/stores/memory/store.py b/key-value/key-value-aio/src/key_value/aio/stores/memory/store.py index fddd7dd7..e10721be 100644 --- a/key-value/key-value-aio/src/key_value/aio/stores/memory/store.py +++ b/key-value/key-value-aio/src/key_value/aio/stores/memory/store.py @@ -153,14 +153,27 @@ async def _setup_collection(self, *, collection: str) -> None: collection_cache = MemoryCollection(max_entries=self.max_entries_per_collection) self._cache[collection] = collection_cache - @override - async def _get_managed_entry(self, *, key: str, collection: str) -> ManagedEntry | None: - collection_cache: MemoryCollection | None = self._cache.get(collection) + def _get_collection_or_raise(self, collection: str) -> MemoryCollection: + """Get a collection or raise KeyError if not setup. + + Args: + collection: The collection name. + Returns: + The MemoryCollection instance. + + Raises: + KeyError: If the collection has not been setup via setup_collection(). + """ + collection_cache: MemoryCollection | None = self._cache.get(collection) if collection_cache is None: msg = f"Collection '{collection}' has not been setup. Call setup_collection() first." raise KeyError(msg) + return collection_cache + @override + async def _get_managed_entry(self, *, key: str, collection: str) -> ManagedEntry | None: + collection_cache = self._get_collection_or_raise(collection) return collection_cache.get(key=key) @override @@ -171,32 +184,17 @@ async def _put_managed_entry( collection: str, managed_entry: ManagedEntry, ) -> None: - collection_cache: MemoryCollection | None = self._cache.get(collection) - - if collection_cache is None: - msg = f"Collection '{collection}' has not been setup. Call setup_collection() first." - raise KeyError(msg) - + collection_cache = self._get_collection_or_raise(collection) collection_cache.put(key=key, value=managed_entry) @override async def _delete_managed_entry(self, *, key: str, collection: str) -> bool: - collection_cache: MemoryCollection | None = self._cache.get(collection) - - if collection_cache is None: - msg = f"Collection '{collection}' has not been setup. Call setup_collection() first." - raise KeyError(msg) - + collection_cache = self._get_collection_or_raise(collection) return collection_cache.delete(key=key) @override async def _get_collection_keys(self, *, collection: str, limit: int | None = None) -> list[str]: - collection_cache: MemoryCollection | None = self._cache.get(collection) - - if collection_cache is None: - msg = f"Collection '{collection}' has not been setup. Call setup_collection() first." - raise KeyError(msg) - + collection_cache = self._get_collection_or_raise(collection) return collection_cache.keys(limit=limit) @override From e9d1422b3a59f4828378eb29d0e8de8fd72718ef Mon Sep 17 00:00:00 2001 From: "claude[bot]" <41898282+claude[bot]@users.noreply.github.com> Date: Mon, 27 Oct 2025 01:12:09 +0000 Subject: [PATCH 3/3] revert: restore fail-safe cache invalidation order in PassthroughCacheWrapper Revert cache invalidation to occur BEFORE primary store operations to ensure fail-safe behavior. Even if the primary operation fails, the cache will not serve stale data. This addresses feedback from PR review discussion. Co-authored-by: William Easton --- .../aio/wrappers/passthrough_cache/wrapper.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/key-value/key-value-aio/src/key_value/aio/wrappers/passthrough_cache/wrapper.py b/key-value/key-value-aio/src/key_value/aio/wrappers/passthrough_cache/wrapper.py index 7aef58f7..ffdab81a 100644 --- a/key-value/key-value-aio/src/key_value/aio/wrappers/passthrough_cache/wrapper.py +++ b/key-value/key-value-aio/src/key_value/aio/wrappers/passthrough_cache/wrapper.py @@ -130,10 +130,10 @@ async def ttl_many(self, keys: Sequence[str], *, collection: str | None = None) @override async def put(self, key: str, value: Mapping[str, Any], *, collection: str | None = None, ttl: SupportsFloat | None = None) -> None: - await self.primary_key_value.put(collection=collection, key=key, value=value, ttl=ttl) - _ = await self.cache_key_value.delete(collection=collection, key=key) + await self.primary_key_value.put(collection=collection, key=key, value=value, ttl=ttl) + @override async def put_many( self, @@ -143,22 +143,18 @@ async def put_many( collection: str | None = None, ttl: SupportsFloat | None = None, ) -> None: - await self.primary_key_value.put_many(keys=keys, values=values, collection=collection, ttl=ttl) - _ = await self.cache_key_value.delete_many(collection=collection, keys=keys) + await self.primary_key_value.put_many(keys=keys, values=values, collection=collection, ttl=ttl) + @override async def delete(self, key: str, *, collection: str | None = None) -> bool: - result = await self.primary_key_value.delete(collection=collection, key=key) - _ = await self.cache_key_value.delete(collection=collection, key=key) - return result + return await self.primary_key_value.delete(collection=collection, key=key) @override async def delete_many(self, keys: Sequence[str], *, collection: str | None = None) -> int: - result = await self.primary_key_value.delete_many(collection=collection, keys=keys) - _ = await self.cache_key_value.delete_many(collection=collection, keys=keys) - return result + return await self.primary_key_value.delete_many(collection=collection, keys=keys)