Skip to content

Commit 419dc68

Browse files
fix: address critical and high-severity issues from code review
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 <[email protected]>
1 parent d58fe24 commit 419dc68

File tree

9 files changed

+48
-16
lines changed

9 files changed

+48
-16
lines changed

key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,7 @@ async def _put_managed_entry(
203203
}
204204

205205
# Add TTL if present
206-
if managed_entry.ttl is not None and managed_entry.created_at:
206+
if managed_entry.ttl is not None and managed_entry.created_at is not None:
207207
# DynamoDB TTL expects a Unix timestamp
208208
ttl_timestamp = int(managed_entry.created_at.timestamp() + managed_entry.ttl)
209209
item["ttl"] = {"N": str(ttl_timestamp)}

key-value/key-value-aio/src/key_value/aio/stores/memory/store.py

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,11 @@ async def _setup_collection(self, *, collection: str) -> None:
155155

156156
@override
157157
async def _get_managed_entry(self, *, key: str, collection: str) -> ManagedEntry | None:
158-
collection_cache: MemoryCollection = self._cache[collection]
158+
collection_cache: MemoryCollection | None = self._cache.get(collection)
159+
160+
if collection_cache is None:
161+
msg = f"Collection '{collection}' has not been setup. Call setup_collection() first."
162+
raise KeyError(msg)
159163

160164
return collection_cache.get(key=key)
161165

@@ -167,19 +171,31 @@ async def _put_managed_entry(
167171
collection: str,
168172
managed_entry: ManagedEntry,
169173
) -> None:
170-
collection_cache: MemoryCollection = self._cache[collection]
174+
collection_cache: MemoryCollection | None = self._cache.get(collection)
175+
176+
if collection_cache is None:
177+
msg = f"Collection '{collection}' has not been setup. Call setup_collection() first."
178+
raise KeyError(msg)
171179

172180
collection_cache.put(key=key, value=managed_entry)
173181

174182
@override
175183
async def _delete_managed_entry(self, *, key: str, collection: str) -> bool:
176-
collection_cache: MemoryCollection = self._cache[collection]
184+
collection_cache: MemoryCollection | None = self._cache.get(collection)
185+
186+
if collection_cache is None:
187+
msg = f"Collection '{collection}' has not been setup. Call setup_collection() first."
188+
raise KeyError(msg)
177189

178190
return collection_cache.delete(key=key)
179191

180192
@override
181193
async def _get_collection_keys(self, *, collection: str, limit: int | None = None) -> list[str]:
182-
collection_cache: MemoryCollection = self._cache[collection]
194+
collection_cache: MemoryCollection | None = self._cache.get(collection)
195+
196+
if collection_cache is None:
197+
msg = f"Collection '{collection}' has not been setup. Call setup_collection() first."
198+
raise KeyError(msg)
183199

184200
return collection_cache.keys(limit=limit)
185201

key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ def __init__(
4040
self._default_value_json = dump_to_json(obj=dict(default_value))
4141
self._default_ttl = None if default_ttl is None else float(default_ttl)
4242

43+
super().__init__()
44+
4345
def _new_default_value(self) -> dict[str, Any]:
4446
return load_from_json(json_str=self._default_value_json)
4547

key-value/key-value-aio/src/key_value/aio/wrappers/encryption/base.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ def _encrypt_value(self, value: dict[str, Any]) -> dict[str, Any]:
6969
json_str: str = json.dumps(value, separators=(",", ":"))
7070

7171
json_bytes: bytes = json_str.encode(encoding="utf-8")
72-
except (json.JSONDecodeError, TypeError) as e:
72+
except TypeError as e:
7373
msg: str = f"Failed to serialize object to JSON: {e}"
7474
raise SerializationError(msg) from e
7575

key-value/key-value-aio/src/key_value/aio/wrappers/limit_size/wrapper.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,4 +101,5 @@ async def put_many(
101101
filtered_keys.append(k)
102102
filtered_values.append(v)
103103

104-
await self.key_value.put_many(keys=filtered_keys, values=filtered_values, collection=collection, ttl=ttl)
104+
if filtered_keys:
105+
await self.key_value.put_many(keys=filtered_keys, values=filtered_values, collection=collection, ttl=ttl)

key-value/key-value-aio/src/key_value/aio/wrappers/passthrough_cache/wrapper.py

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -130,10 +130,10 @@ async def ttl_many(self, keys: Sequence[str], *, collection: str | None = None)
130130

131131
@override
132132
async def put(self, key: str, value: Mapping[str, Any], *, collection: str | None = None, ttl: SupportsFloat | None = None) -> None:
133-
_ = await self.cache_key_value.delete(collection=collection, key=key)
134-
135133
await self.primary_key_value.put(collection=collection, key=key, value=value, ttl=ttl)
136134

135+
_ = await self.cache_key_value.delete(collection=collection, key=key)
136+
137137
@override
138138
async def put_many(
139139
self,
@@ -143,18 +143,22 @@ async def put_many(
143143
collection: str | None = None,
144144
ttl: SupportsFloat | None = None,
145145
) -> None:
146-
_ = await self.cache_key_value.delete_many(collection=collection, keys=keys)
147-
148146
await self.primary_key_value.put_many(keys=keys, values=values, collection=collection, ttl=ttl)
149147

148+
_ = await self.cache_key_value.delete_many(collection=collection, keys=keys)
149+
150150
@override
151151
async def delete(self, key: str, *, collection: str | None = None) -> bool:
152+
result = await self.primary_key_value.delete(collection=collection, key=key)
153+
152154
_ = await self.cache_key_value.delete(collection=collection, key=key)
153155

154-
return await self.primary_key_value.delete(collection=collection, key=key)
156+
return result
155157

156158
@override
157159
async def delete_many(self, keys: Sequence[str], *, collection: str | None = None) -> int:
160+
result = await self.primary_key_value.delete_many(collection=collection, keys=keys)
161+
158162
_ = await self.cache_key_value.delete_many(collection=collection, keys=keys)
159163

160-
return await self.primary_key_value.delete_many(collection=collection, keys=keys)
164+
return result

key-value/key-value-aio/src/key_value/aio/wrappers/routing/wrapper.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ def __init__(
4848
self._routing_function = routing_function
4949
self._default_store = default_store
5050

51+
super().__init__()
52+
5153
def _get_store(self, collection: str | None) -> AsyncKeyValue:
5254
"""Get the appropriate store for the given collection.
5355

key-value/key-value-aio/src/key_value/aio/wrappers/statistics/wrapper.py

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,9 @@ def statistics(self) -> KVStoreStatistics:
113113
async def get(self, key: str, *, collection: str | None = None) -> dict[str, Any] | None:
114114
collection = collection or DEFAULT_COLLECTION_NAME
115115

116-
if value := await self.key_value.get(collection=collection, key=key):
116+
value = await self.key_value.get(collection=collection, key=key)
117+
118+
if value is not None:
117119
self.statistics.get_collection(collection=collection).get.increment_hit()
118120
return value
119121

@@ -127,7 +129,7 @@ async def ttl(self, key: str, *, collection: str | None = None) -> tuple[dict[st
127129

128130
value, ttl = await self.key_value.ttl(collection=collection, key=key)
129131

130-
if value:
132+
if value is not None:
131133
self.statistics.get_collection(collection=collection).ttl.increment_hit()
132134
return value, ttl
133135

key-value/key-value-aio/src/key_value/aio/wrappers/ttl_clamp/wrapper.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,12 @@
99

1010

1111
class TTLClampWrapper(BaseWrapper):
12-
"""Wrapper that enforces a maximum TTL for puts into the store."""
12+
"""Wrapper that enforces a maximum TTL for puts into the store.
13+
14+
This wrapper only modifies write operations (put, put_many). All read operations
15+
(get, get_many, ttl, ttl_many, delete, delete_many) pass through unchanged to
16+
the underlying store.
17+
"""
1318

1419
def __init__(
1520
self, key_value: AsyncKeyValue, min_ttl: SupportsFloat, max_ttl: SupportsFloat, missing_ttl: SupportsFloat | None = None

0 commit comments

Comments
 (0)