Refactor ManagedEntry serialization with adapter pattern #184
Conversation
…is stores This commit introduces a clean adapter pattern for store-specific serialization: **Key Changes:** - Add SerializationAdapter base class with to_storage() and from_storage() methods - Implement MongoDBAdapter with native BSON storage support - Implement RedisAdapter for JSON string storage - Add estimate_serialized_size() utility function to managed_entry.py - Update CompressionWrapper and LimitSizeWrapper to use estimate_serialized_size() - Add error handling for deserialization failures (return None instead of crashing) - Add type checking for adapter outputs to catch contract violations early **Benefits:** - Consistent serialization interface across stores - Better separation of concerns (serialization logic in adapters) - More efficient size calculations without ManagedEntry instantiation - Easier to test and maintain - Improved error handling Co-authored-by: William Easton <strawgate@users.noreply.github.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code)
- Create shared SerializationAdapter ABC in key_value.shared.utils.serialization - Add common adapter implementations: - FullJsonAdapter: For Redis/Valkey (full JSON strings) - StringifiedDictAdapter: For stores preferring dict with stringified values - MongoDBAdapter: With native BSON datetime support for TTL indexing - ElasticsearchAdapter: With native/string storage mode support - Update MongoDB, Redis, and Elasticsearch stores to use shared adapters - Remove duplicate SerializationAdapter definitions from each store - Remove store-specific helper functions (managed_entry_to_document, etc.) - Run codegen to sync changes to sync library - All linting passes This eliminates code duplication and provides a consistent serialization interface across all stores, making it easier to add new stores or modify serialization behavior. Co-authored-by: William Easton <strawgate@users.noreply.github.com>
- Simplified shared serialization module to contain only SerializationAdapter base class - Moved FullJsonAdapter to Redis store module - Moved MongoDBAdapter to MongoDB store module - Moved ElasticsearchAdapter to Elasticsearch store module - Updated imports in all affected modules - Ran codegen to sync async changes to sync library - Applied linting fixes Co-authored-by: William Easton <strawgate@users.noreply.github.com>
📝 WalkthroughWalkthroughIntroduce a SerializationAdapter abstraction (with BasicSerializationAdapter) and migrate stores/wrappers/tests to use adapter-based (de)serialization; remove stored ttl field in favor of a computed ManagedEntry.ttl property; adjust memory/simple defaults and related tests to assert adapter-produced JSON shapes (created_at / expires_at). Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Store
participant Adapter
participant Backend
rect rgb(220,245,220)
Note over Client,Store: PUT (adapter-based)
Client->>Store: put(collection,key,value,ttl?)
Store->>Store: build ManagedEntry(created_at, expires_at)
Store->>Adapter: dump_json(entry=ManagedEntry)
Adapter->>Adapter: format dates & value (iso/dict or string)
Adapter-->>Store: json_str
Store->>Backend: write(key, json_str, ttl?)
Backend-->>Store: OK
Store-->>Client: OK
end
rect rgb(245,245,220)
Note over Client,Store: GET (adapter-based)
Client->>Store: get(collection,key)
Store->>Backend: read(key)
Backend-->>Store: json_str
Store->>Adapter: load_json(json_str)
Adapter->>Adapter: parse dates & value -> ManagedEntry
Adapter-->>Store: ManagedEntry(created_at, expires_at, value)
Store-->>Client: value
end
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)key-value/key-value-sync/tests/code_gen/stores/base.py (2)
key-value/key-value-aio/tests/stores/base.py (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
🔇 Additional comments (2)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (1)
283-318: Restore collection/key fields before indexing.In both
_put_managed_entryand_put_managed_entrieswe now write whateverSerializationAdapter.dump_dictreturns, which no longer includescollectionorkey. Those fields are still required by_delete_collection,_get_collection_keys,_get_collection_names, and the delete-by-query in_cull. Without them the documents can be created, but nothing can enumerate or purge them, so the store effectively leaks data and breaks key listing. Please reintroduce the two fields before indexing/bulk operations. (elastic.co)- document: dict[str, Any] = self._adapter.dump_dict(entry=managed_entry) + document: dict[str, Any] = self._adapter.dump_dict(entry=managed_entry) + document["collection"] = collection + document["key"] = keyApply the same fix in the bulk path.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
268-305: Same JSON check issue as async store.The synchronous store still calls
managed_entry.value_as_jsonunconditionally, so native-storage callers hit the same serialization error on datetimes/ObjectIds. Please mirror the guard from the async fix so we only stringify when_native_storageis false. (geeksforgeeks.org)- try: - # Ensure that the value is serializable to JSON - _ = managed_entry.value_as_json + try: + if not self._adapter._native_storage: + _ = managed_entry.value_as_jsonkey-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
318-378: Restoringcollection/keyfields on indexed documents.The new adapter flow drops the explicit
collectionandkeyfields when indexing. Our_get_collection_keys,_get_collection_names, and_delete_collectionimplementations all filter oncollection, and the raw docs used in migrations rely onkey. As soon as this ships, those operations will return empty results or fail to delete anything.Please add the metadata back before indexing/bulk indexing, for example:
document: dict[str, Any] = self._adapter.dump_dict(entry=managed_entry) + document["collection"] = collection + document["key"] = key…and the same augmentation inside
_put_managed_entries.key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
66-72: Fix incorrect exception handling indump_to_json.Line 70 catches
json.JSONDecodeError, but this function usesjson.dumps(encoding) which does not raiseJSONDecodeError. Thejson.dumpsfunction can raiseTypeErrororValueErrorfor serialization errors.Apply this diff:
- except (json.JSONDecodeError, TypeError) as e: + except (TypeError, ValueError) as e:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (54)
key-value/key-value-aio/src/key_value/aio/stores/base.py(5 hunks)key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py(4 hunks)key-value/key-value-aio/src/key_value/aio/stores/disk/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py(2 hunks)key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py(8 hunks)key-value/key-value-aio/src/key_value/aio/stores/keyring/store.py(2 hunks)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/memory/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py(8 hunks)key-value/key-value-aio/src/key_value/aio/stores/redis/store.py(8 hunks)key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/simple/store.py(4 hunks)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/vault/store.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/windows_registry/store.py(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/compression/wrapper.py(2 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/limit_size/wrapper.py(2 hunks)key-value/key-value-aio/tests/stores/disk/test_disk.py(2 hunks)key-value/key-value-aio/tests/stores/disk/test_multi_disk.py(2 hunks)key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py(3 hunks)key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py(3 hunks)key-value/key-value-aio/tests/stores/memcached/test_memcached.py(2 hunks)key-value/key-value-aio/tests/stores/mongodb/test_mongodb.py(3 hunks)key-value/key-value-aio/tests/stores/redis/test_redis.py(4 hunks)key-value/key-value-aio/tests/stores/rocksdb/test_rocksdb.py(2 hunks)key-value/key-value-aio/tests/stores/valkey/test_valkey.py(3 hunks)key-value/key-value-aio/tests/stores/wrappers/test_limit_size.py(2 hunks)key-value/key-value-shared/pyproject.toml(0 hunks)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py(4 hunks)key-value/key-value-shared/src/key_value/shared/utils/serialization.py(1 hunks)key-value/key-value-shared/tests/utils/test_serialization.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py(8 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/keyring/store.py(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py(8 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py(8 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/simple/store.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/vault/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/windows_registry/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/compression/wrapper.py(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/limit_size/wrapper.py(2 hunks)key-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py(2 hunks)key-value/key-value-sync/tests/code_gen/stores/disk/test_multi_disk.py(2 hunks)key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py(5 hunks)key-value/key-value-sync/tests/code_gen/stores/mongodb/test_mongodb.py(3 hunks)key-value/key-value-sync/tests/code_gen/stores/redis/test_redis.py(4 hunks)key-value/key-value-sync/tests/code_gen/stores/rocksdb/test_rocksdb.py(2 hunks)key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py(3 hunks)key-value/key-value-sync/tests/code_gen/stores/wrappers/test_limit_size.py(2 hunks)
💤 Files with no reviewable changes (1)
- key-value/key-value-shared/pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (53)
key-value/key-value-aio/src/key_value/aio/stores/keyring/store.py (1)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(129-131)
key-value/key-value-shared/tests/utils/test_serialization.py (2)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (6)
BasicSerializationAdapter(134-146)ValueOnlySerializationAdapter(149-164)dump_dict(109-127)dump_json(129-131)load_dict(64-101)load_json(52-56)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/compression/wrapper.py (1)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
estimate_serialized_size(98-111)
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (2)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(129-131)
key-value/key-value-aio/tests/stores/rocksdb/test_rocksdb.py (2)
key-value/key-value-sync/tests/code_gen/stores/rocksdb/test_rocksdb.py (2)
rocksdb_client(71-72)store(23-29)key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
RocksDBStore(20-194)
key-value/key-value-aio/tests/stores/redis/test_redis.py (4)
key-value/key-value-aio/src/key_value/aio/stores/base.py (5)
put(296-309)get(187-213)ttl(226-235)keys(393-399)put_many(312-340)key-value/key-value-sync/tests/code_gen/stores/redis/test_redis.py (2)
get_client_from_store(42-43)store(61-66)key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (1)
RedisStore(25-220)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ttl(35-38)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (4)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
DeserializationError(14-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (8)
ManagedEntry(15-63)dump_to_json(67-72)load_from_json(76-82)verify_dict(86-95)value_as_dict(46-47)value_as_json(41-43)created_at_isoformat(50-51)expires_at_isoformat(54-55)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (2)
prepare_load(66-88)prepare_dump(53-63)key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (2)
prepare_load(120-131)prepare_dump(107-117)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/keyring/store.py (1)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(129-131)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/simple/store.py (3)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(15-63)load_from_json(76-82)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
ValueOnlySerializationAdapter(149-164)load_json(52-56)dump_json(129-131)key-value/key-value-aio/src/key_value/aio/stores/simple/store.py (1)
SimpleStoreEntry(23-43)
key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
ValkeyStore(25-151)key-value/key-value-aio/tests/stores/valkey/test_valkey.py (3)
get_valkey_client_from_store(39-40)store(82-91)valkey_client(94-95)
key-value/key-value-aio/src/key_value/aio/stores/base.py (3)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
BasicSerializationAdapter(134-146)SerializationAdapter(35-131)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (1)
prepare_entry_timestamps(77-86)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ttl(35-38)ManagedEntry(15-63)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(129-131)
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (4)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (9)
SerializationAdapter(35-131)prepare_dump(104-107)prepare_dump(145-146)prepare_dump(160-164)prepare_load(59-62)prepare_load(142-143)prepare_load(155-158)load_dict(64-101)dump_dict(109-127)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (1)
now_as_epoch(14-16)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (3)
ElasticsearchSerializationAdapter(68-111)prepare_dump(87-97)prepare_load(100-111)
key-value/key-value-aio/tests/stores/memcached/test_memcached.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (3)
put(296-309)get(187-213)ttl(226-235)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
MemcachedStore(20-130)
key-value/key-value-sync/tests/code_gen/stores/redis/test_redis.py (2)
key-value/key-value-aio/tests/stores/redis/test_redis.py (2)
get_client_from_store(42-43)store(61-66)key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (1)
RedisStore(28-211)
key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py (1)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
_connected_client(133-137)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_limit_size.py (1)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
estimate_serialized_size(98-111)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/limit_size/wrapper.py (1)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
estimate_serialized_size(98-111)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (3)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (9)
SerializationAdapter(35-131)prepare_dump(104-107)prepare_dump(145-146)prepare_dump(160-164)prepare_load(59-62)prepare_load(142-143)prepare_load(155-158)load_dict(64-101)dump_dict(109-127)key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (3)
ElasticsearchSerializationAdapter(88-131)prepare_dump(107-117)prepare_load(120-131)
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (3)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(15-63)ttl(35-38)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(129-131)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
ttl(226-235)
key-value/key-value-aio/tests/stores/disk/test_multi_disk.py (2)
key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py (1)
MultiDiskStore(28-150)key-value/key-value-sync/tests/code_gen/stores/disk/test_multi_disk.py (2)
test_value_stored(38-52)store(32-36)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (2)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(129-131)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (2)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
BasicSerializationAdapter(134-146)load_json(52-56)dump_json(129-131)key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (4)
MemoryCacheEntry(26-31)_memory_cache_ttu(34-39)get(68-76)put(78-80)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(129-131)
key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (1)
ElasticsearchSerializationAdapter(68-111)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(109-127)load_dict(64-101)
key-value/key-value-aio/tests/stores/mongodb/test_mongodb.py (2)
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
MongoDBSerializationAdapter(39-88)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(109-127)load_dict(64-101)
key-value/key-value-aio/src/key_value/aio/stores/windows_registry/store.py (2)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(129-131)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (3)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
DeserializationError(14-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (4)
BasicSerializationAdapter(134-146)SerializationAdapter(35-131)load_json(52-56)dump_json(129-131)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/windows_registry/store.py (1)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(129-131)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (3)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
BasicSerializationAdapter(134-146)load_json(52-56)dump_json(129-131)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (5)
MemoryCacheEntry(29-34)_memory_cache_ttu(37-42)_memory_cache_getsizeof(45-47)get(69-77)put(79-81)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)
key-value/key-value-sync/tests/code_gen/stores/mongodb/test_mongodb.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
MongoDBSerializationAdapter(46-95)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(109-127)load_dict(64-101)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (2)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(15-63)ttl(35-38)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(129-131)
key-value/key-value-aio/src/key_value/aio/stores/vault/store.py (2)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(129-131)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/vault/store.py (2)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(129-131)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)
key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (3)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
DeserializationError(14-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (4)
BasicSerializationAdapter(134-146)SerializationAdapter(35-131)load_json(52-56)dump_json(129-131)
key-value/key-value-sync/tests/code_gen/stores/rocksdb/test_rocksdb.py (3)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (4)
collections(404-408)put(282-291)get(193-214)ttl(227-236)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
RocksDBStore(23-185)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
value_as_dict(46-47)ttl(35-38)
key-value/key-value-aio/src/key_value/aio/wrappers/limit_size/wrapper.py (1)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
estimate_serialized_size(98-111)
key-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (3)
put(282-291)get(193-214)ttl(227-236)key-value/key-value-aio/tests/stores/disk/test_disk.py (3)
disk_cache(31-32)disk_store(19-21)test_value_stored(34-47)
key-value/key-value-sync/tests/code_gen/stores/disk/test_multi_disk.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (3)
put(282-291)get(193-214)ttl(227-236)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py (1)
MultiDiskStore(31-147)
key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(129-131)
key-value/key-value-aio/src/key_value/aio/wrappers/compression/wrapper.py (1)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
estimate_serialized_size(98-111)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(129-131)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (3)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (2)
DeserializationError(14-15)SerializationError(10-11)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(15-63)value_as_json(41-43)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (9)
SerializationAdapter(35-131)prepare_dump(104-107)prepare_dump(145-146)prepare_dump(160-164)prepare_load(59-62)prepare_load(142-143)prepare_load(155-158)load_dict(64-101)dump_dict(109-127)
key-value/key-value-aio/tests/stores/valkey/test_valkey.py (2)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
ValkeyStore(23-155)key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py (3)
get_valkey_client_from_store(37-38)store(80-89)valkey_client(92-93)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (3)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
BasicSerializationAdapter(134-146)SerializationAdapter(35-131)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (1)
prepare_entry_timestamps(77-86)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ttl(35-38)ManagedEntry(15-63)
key-value/key-value-aio/tests/stores/disk/test_disk.py (2)
key-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py (3)
disk_cache(34-35)disk_store(22-24)store(28-31)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
value_as_dict(46-47)ttl(35-38)
key-value/key-value-aio/src/key_value/aio/stores/simple/store.py (3)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(15-63)load_from_json(76-82)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
ValueOnlySerializationAdapter(149-164)load_json(52-56)dump_json(129-131)key-value/key-value-sync/src/key_value/sync/code_gen/stores/simple/store.py (1)
SimpleStoreEntry(21-39)
key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py (4)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(15-63)ttl(35-38)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
BasicSerializationAdapter(134-146)load_json(52-56)dump_json(129-131)key-value/key-value-aio/src/key_value/aio/stores/base.py (4)
get(187-213)ttl(226-235)_delete_managed_entry(343-345)delete(360-364)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py (1)
_delete_managed_entry(135-136)
key-value/key-value-aio/tests/stores/wrappers/test_limit_size.py (1)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
estimate_serialized_size(98-111)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py (4)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(15-63)ttl(35-38)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
BasicSerializationAdapter(134-146)load_json(52-56)dump_json(129-131)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (4)
get(69-77)_put_managed_entry(161-163)_delete_managed_entry(166-168)delete(83-84)key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py (2)
_put_managed_entry(128-135)_delete_managed_entry(138-139)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (5)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (2)
DeserializationError(14-15)SerializationError(10-11)key-value/key-value-shared/src/key_value/shared/type_checking/bear_spray.py (1)
bear_enforce(21-23)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (3)
now(19-21)now_plus(29-31)seconds_to(24-26)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
ttl(226-235)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
ttl(227-236)
key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py (2)
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
ElasticsearchSerializationAdapter(88-131)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(109-127)load_dict(64-101)
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (4)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (2)
DeserializationError(14-15)SerializationError(10-11)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(15-63)value_as_json(41-43)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (9)
SerializationAdapter(35-131)prepare_dump(104-107)prepare_dump(145-146)prepare_dump(160-164)prepare_load(59-62)prepare_load(142-143)prepare_load(155-158)load_dict(64-101)dump_dict(109-127)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (3)
MongoDBSerializationAdapter(46-95)prepare_dump(60-70)prepare_load(73-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-shared)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
🔇 Additional comments (52)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (2)
20-20: LGTM! Clean dependency injection pattern.The serialization adapter integration is well-implemented with proper type hints, sensible defaults, and clear documentation. The adapter will be used by subclasses in their implementations of
_get_managed_entryand_put_managed_entry.Also applies to: 77-78, 83-111
289-289: LGTM! Correct usage ofManagedEntry.Both
putandput_manycorrectly constructManagedEntryinstances withexpires_atinstead of storing TTL directly. The TTL will be computed on demand via theManagedEntry.ttlproperty, aligning with the PR's objective to remove TTL as a stored field.Also applies to: 308-308
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (3)
63-65: LGTM: Proper handling of unlimited capacity.The use of
sys.maxsizeas the fallback for unlimited capacity is appropriate and more explicit than a magic number.
75-75: LGTM: Consistent adapter usage for deserialization.The serialization adapter is correctly used to deserialize the cached JSON string.
80-81: LGTM: Consistent adapter usage for serialization.The serialization adapter is correctly used to serialize the
ManagedEntrybefore caching.key-value/key-value-aio/src/key_value/aio/stores/windows_registry/store.py (1)
91-91: LGTM: Proper adapter-based serialization.The migration from
ManagedEntry.from_jsonandmanaged_entry.to_jsonto adapter-basedload_jsonanddump_jsonis consistent with the serialization refactoring pattern across the codebase.Also applies to: 98-98
key-value/key-value-aio/tests/stores/wrappers/test_limit_size.py (1)
142-142: LGTM: More efficient size calculation.Using
estimate_serialized_sizeinstead of constructing aManagedEntryand callingto_json()is more efficient and directly measures what the wrapper checks (the value payload size, not the full entry with metadata).key-value/key-value-aio/src/key_value/aio/wrappers/compression/wrapper.py (1)
59-59: LGTM: Efficient size calculation for compression threshold.Using
estimate_serialized_sizeis more efficient than constructing aManagedEntryand provides an accurate measure of the value's serialized size for the compression decision.key-value/key-value-aio/src/key_value/aio/wrappers/limit_size/wrapper.py (1)
68-68: LGTM: Accurate size measurement for limit checks.Using
estimate_serialized_sizeprovides an accurate measure of the value's serialized size without the overhead of creating a fullManagedEntryobject, which is appropriate for size-based filtering.key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/limit_size/wrapper.py (1)
71-71: LGTM: Consistent refactoring in sync version.The size calculation correctly uses
estimate_serialized_size, consistent with the async version of this wrapper.key-value/key-value-aio/tests/stores/disk/test_disk.py (1)
34-47: LGTM: Comprehensive validation of serialized entry structure.This test properly validates that the serialization adapter correctly persists entries with the expected structure (
created_at,value, and optionallyexpires_at). The use ofsnapshottesting withIsDatetimevalidation is appropriate for verifying the shape while allowing timestamp flexibility.key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/compression/wrapper.py (1)
58-58: LGTM: Consistent refactoring in sync version.The size calculation correctly uses
estimate_serialized_size, consistent with the async version of this wrapper.key-value/key-value-sync/src/key_value/sync/code_gen/stores/windows_registry/store.py (1)
92-92: LGTM! Clean adapter delegation for serialization.The refactoring correctly replaces direct
ManagedEntryserialization methods with adapter-based calls, maintaining consistent behavior while enabling pluggable serialization strategies.Also applies to: 99-99
key-value/key-value-aio/src/key_value/aio/stores/keyring/store.py (1)
82-82: LGTM! Serialization adapter correctly integrated.The keyring store now properly delegates JSON serialization to the centralized adapter, aligning with the project-wide refactoring.
Also applies to: 91-91
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_limit_size.py (1)
6-6: LGTM! More efficient size estimation approach.The test now uses the dedicated
estimate_serialized_sizeutility function instead of creating a fullManagedEntryobject just to calculate serialized size. This is cleaner and more efficient.Also applies to: 145-145
key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
68-68: LGTM! All serialization paths properly updated.The memcached store correctly delegates serialization to the adapter in all code paths: single-entry retrieval, batch retrieval, and storage operations. The refactoring is complete and consistent.
Also applies to: 85-85, 109-109
key-value/key-value-sync/src/key_value/sync/code_gen/stores/vault/store.py (1)
102-102: LGTM! Vault serialization migrated to adapter pattern.The Vault store now uses the centralized serialization adapter for reading and writing secrets, maintaining compatibility with the KV v2 secrets engine while enabling pluggable serialization.
Also applies to: 108-108
key-value/key-value-aio/src/key_value/aio/stores/vault/store.py (1)
105-105: LGTM! Async Vault store correctly uses serialization adapter.The async Vault implementation properly delegates serialization to the adapter, maintaining consistency with the synchronous version and the broader refactoring effort.
Also applies to: 111-111
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
202-202: LGTM! DynamoDB serialization properly refactored.The DynamoDB store correctly uses the serialization adapter for JSON values stored in the 'value' attribute, while maintaining separate TTL handling via the 'ttl' attribute. The refactoring integrates cleanly with DynamoDB's attribute structure.
Also applies to: 213-213
key-value/key-value-sync/src/key_value/sync/code_gen/stores/keyring/store.py (1)
72-72: LGTM! Synchronous keyring store adopts adapter pattern.The sync keyring implementation correctly delegates serialization to the adapter, mirroring the async version and completing the consistent refactoring across both sync and async stores.
Also applies to: 81-81
key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py (3)
2-2: LGTM: Import additions support new test functionality.The new imports (
json,Any,IsDatetime,snapshot, DynamoDB client types) are necessary for the test that validates the stored serialization format and dynamic timestamps.Also applies to: 4-4, 7-8, 10-11
111-114: LGTM: Fixture appropriately exposes internal client for white-box testing.Accessing the private
_connected_clientis intentional here to verify the raw stored representation in DynamoDB.
119-136: LGTM: Test validates serialization adapter output.The test correctly verifies:
- Stored structure includes
created_atandvaluewithout TTL- With TTL, structure includes
expires_at- Dynamic timestamps validated via
IsDatetimekey-value/key-value-aio/tests/stores/rocksdb/test_rocksdb.py (3)
1-1: LGTM: Import additions support new test functionality.The new imports are necessary for validating the stored serialization format in RocksDB.
Also applies to: 7-9
67-69: LGTM: Fixture appropriately exposes internal RocksDB instance.Accessing the private
_dbis intentional for white-box testing of the stored representation.
71-86: LGTM: Test validates serialization adapter output for RocksDB.The test correctly verifies the stored JSON structure includes
created_at,value, andexpires_at(when TTL is provided). Dynamic timestamp validation viaIsDatetimeis appropriate.key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (3)
109-109: LGTM: Deserialization migrated to adapter pattern.The change from direct
ManagedEntry.from_jsontoself._serialization_adapter.load_jsonaligns with the PR objective of centralizing serialization through adapters.
118-118: LGTM: Serialization migrated to adapter pattern.The change from
managed_entry.to_json()toself._serialization_adapter.dump_json(entry=managed_entry)correctly uses the adapter for serialization.
141-141: LGTM: Batch operation serialization migrated to adapter pattern.Consistent use of the serialization adapter in the batch write path.
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (4)
1-1: LGTM: Import updated for timezone-aware datetime handling.The change from time-based functions to
datetimewithtimezonesupports the new approach of populatingexpires_atfrom epoch timestamps.
93-93: LGTM: Deserialization migrated to adapter pattern.Consistent with the broader refactoring to use serialization adapters.
95-96: LGTM: Expiration handling migrated from TTL to expires_at timestamp.The code correctly populates
expires_atfrom the diskcache'sexpire_epochas a UTC-aware datetime. This aligns with the PR's shift from storing TTL to computing it fromexpires_at.
110-110: LGTM: Serialization migrated to adapter pattern.The change to use
self._serialization_adapter.dump_json(entry=managed_entry)is consistent with the refactoring. The code also correctly passesmanaged_entry.ttlto diskcache'sexpireparameter.key-value/key-value-sync/tests/code_gen/stores/rocksdb/test_rocksdb.py (3)
4-4: LGTM: Import additions support new test functionality.The imports are necessary for the new test that validates stored JSON structure.
Also applies to: 10-12
70-72: LGTM: Fixture exposes internal RocksDB instance for testing.Accessing
_dbis intentional for white-box validation of the stored representation.
74-89: LGTM: Test validates serialization format for sync RocksDB store.The test correctly verifies the stored JSON includes
created_at,value, andexpires_at(with TTL), usingIsDatetimefor dynamic timestamp validation.key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (3)
112-112: LGTM: Deserialization migrated to adapter pattern.The change from direct deserialization to
self._serialization_adapter.load_jsonis consistent with the PR objective.
127-127: LGTM: Serialization migrated to adapter pattern.Correctly uses the serialization adapter for writing ManagedEntry to RocksDB.
150-150: LGTM: Batch operation serialization migrated to adapter pattern.Consistent adapter usage in the batch write path.
key-value/key-value-aio/tests/stores/disk/test_multi_disk.py (3)
1-1: LGTM: Import additions support new test functionality.The imports are necessary for the new test. Using
TYPE_CHECKINGfor theCacheimport is appropriate to avoid runtime import overhead.Also applies to: 5-5, 8-9
15-16: LGTM: TYPE_CHECKING block avoids runtime import.Using
TYPE_CHECKINGfor type-only imports is a Python best practice that avoids circular dependencies and reduces runtime overhead.
35-54: LGTM: Test validates serialization format for multi-disk store.The test correctly:
- Accesses the internal cache for white-box testing
- Validates stored JSON structure includes
created_atandvalue- Verifies
expires_atis present when TTL is provided- Uses
IsDatetimefor dynamic timestamp validationkey-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py (3)
4-4: LGTM: Import additions support new test functionality.The imports are necessary for validating the stored JSON structure in the disk cache.
Also applies to: 9-11
33-35: LGTM: Fixture exposes internal disk cache for testing.Accessing
_cacheis intentional for white-box validation of the stored representation.
37-50: LGTM: Test validates serialization format for disk store.The test correctly verifies:
- Uses compound key format
"test::test_key"for retrieval- Stored JSON includes
created_atandvalue- With TTL, includes
expires_at- Dynamic timestamps validated via
IsDatetimekey-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (7)
5-5: LGTM: New imports support the refactored serialization pattern.The imports for
SupportsFloat,bear_enforce, and time utilities align well with the new TTL computation and validation features.Also applies to: 10-11
34-38: LGTM: TTL property correctly computes remaining time.The refactoring from a stored field to a computed property ensures TTL values are always accurate and consistent with
expires_at.
46-47: LGTM: Value validation throughverify_dictimproves data integrity.The change adds proper validation ensuring the value is a Mapping with string keys.
49-55: LGTM: ISO format properties provide convenient timestamp serialization.These helper properties cleanly expose ISO-formatted timestamp strings for use in adapters.
75-82: LGTM: JSON deserialization with proper validation.The error handling correctly catches
JSONDecodeErrorfor deserialization failures, and the use ofverify_dictensures the result is a valid Mapping with string keys.
85-95: LGTM: Robust validation of dictionary structure.The function properly validates that the object is a Mapping with string keys, raising clear TypeError exceptions for invalid inputs.
57-63: The review comment identifies a code smell but overlooks upstream TTL validation.The concern about
if ttltreating zero as falsy is technically correct: ifttl=0reachesfrom_ttl, it would result inexpires_at=None(no expiration) rather than immediate expiration. However, the actual risk is minimal because:
- The
prepare_ttl()function intime_to_live.pyvalidates thatttl <= 0and raisesInvalidTTLError, preventingttl=0from reachingfrom_ttlthrough normal store operations.- The type hint
ttl: SupportsFloat(required, not Optional) contradicts theif ttl else Nonepattern, but this inconsistency is already protected by upstream validation.The real issue is the code smell: The type hint says
ttlis required, but the conditional suggests it could be falsy. This confusion could lead to subtle bugs iffrom_ttlis ever called directly or used in a new context without the upstream validation layer.Consider clarifying the intent: either enforce that
ttlis always truthy at this point, or update the type hint tottl: SupportsFloat | Noneif falsy values are genuinely expected.
key-value/key-value-shared/src/key_value/shared/utils/serialization.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py
Show resolved
Hide resolved
key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py (1)
57-58: Critical:json.loadswill fail on empty dict fallback.If the "S" key is missing,
.get("S", {})returns an empty dict{}, which causesjson.loads({})to raise aTypeErrorsincejson.loadsexpects a string, not a dict.Apply the fix suggested in the previous review:
def get_value_from_response(response: GetItemOutputTypeDef) -> dict[str, Any]: - return json.loads(response.get("Item", {}).get("value", {}).get("S", {})) # pyright: ignore[reportArgumentType] + json_str = response.get("Item", {}).get("value", {}).get("S") + if not json_str: + return {} + return json.loads(json_str)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py (1)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (2)
DynamoDBStore(29-251)_connected_client(133-137)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
🔇 Additional comments (1)
key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py (1)
2-2: LGTM: Import additions support the new test infrastructure.The added imports are appropriate for snapshot testing with datetime assertions and DynamoDB type hints.
Also applies to: 4-4, 7-8, 10-11
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
229-232: Critical: Incorrect TTL timestamp calculation.The computation uses
managed_entry.ttl, which is a property that returns seconds remaining from NOW toexpires_at, not the original TTL duration. This produces incorrect timestamps.From
managed_entry.py:@property def ttl(self) -> float | None: if self.expires_at is None: return None return seconds_to(datetime=self.expires_at)Since
ttlis computed from the current time, adding it tocreated_atyields an incorrect future timestamp.Apply this diff to use
expires_atdirectly:- # Add TTL if present - 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)} + # Add TTL if present + if managed_entry.expires_at is not None: + # DynamoDB TTL expects a Unix timestamp + ttl_timestamp = int(managed_entry.expires_at.timestamp()) + item["ttl"] = {"N": str(ttl_timestamp)}
♻️ Duplicate comments (2)
key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py (2)
58-60: Error handling still missing for malformed responses.This issue was previously flagged but remains unaddressed. When the
"S"key is missing,json.loads({})receives a dict instead of a string, causing aTypeError.Apply the previously suggested diff:
def get_value_from_response(response: GetItemOutputTypeDef) -> dict[str, Any]: - return json.loads(response.get("Item", {}).get("value", {}).get("S", {})) # pyright: ignore[reportArgumentType] + json_str = response.get("Item", {}).get("value", {}).get("S") + if not json_str: + return {} + return json.loads(json_str)
62-63: Documentation still missing for connection requirement.This issue was previously flagged but remains unaddressed. The helper accesses
_connected_client, which raisesValueErrorif not connected.Apply the previously suggested diff:
def get_dynamo_client_from_store(store: DynamoDBStore) -> DynamoDBClient: + """Get the connected DynamoDB client from the store. + + Assumes the store has been properly initialized and connected. + Raises ValueError if the client is not connected. + """ return store._connected_client # pyright: ignore[reportPrivateUsage]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py(3 hunks)key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py(3 hunks)key-value/key-value-aio/tests/stores/valkey/test_valkey.py(3 hunks)key-value/key-value-shared/src/key_value/shared/utils/serialization.py(1 hunks)key-value/key-value-shared/tests/utils/test_serialization.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py (1)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (2)
DynamoDBStore(30-258)_connected_client(134-138)
key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
ValkeyStore(25-151)key-value/key-value-aio/tests/stores/valkey/test_valkey.py (2)
get_valkey_client_from_store(42-43)store(85-94)
key-value/key-value-aio/tests/stores/valkey/test_valkey.py (2)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
ValkeyStore(23-155)key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py (2)
get_valkey_client_from_store(40-41)store(83-92)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (4)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (1)
get(68-76)key-value/key-value-aio/src/key_value/aio/stores/base.py (2)
get(187-213)ttl(226-235)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(131-133)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ttl(35-38)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
DeserializationError(14-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (8)
ManagedEntry(15-63)dump_to_json(67-72)load_from_json(76-82)verify_dict(86-95)value_as_dict(46-47)value_as_json(41-43)created_at_isoformat(50-51)expires_at_isoformat(54-55)
key-value/key-value-shared/tests/utils/test_serialization.py (2)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (6)
BasicSerializationAdapter(136-148)ValueOnlySerializationAdapter(151-166)load_json(52-56)load_dict(64-103)dump_dict(111-129)dump_json(131-133)
🪛 GitHub Actions: Run Tests
key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py
[error] 102-102: Pyright: 'get' is not a known attribute of 'None' (reportOptionalMemberAccess).
key-value/key-value-aio/tests/stores/valkey/test_valkey.py
[error] 104-104: "get" is not a known attribute of "None" (reportOptionalMemberAccess)
🪛 GitHub Check: static_analysis (key-value/key-value-aio)
key-value/key-value-aio/tests/stores/valkey/test_valkey.py
[failure] 111-111:
"get" is not a known attribute of "None" (reportOptionalMemberAccess)
[failure] 104-104:
"get" is not a known attribute of "None" (reportOptionalMemberAccess)
🪛 GitHub Check: static_analysis (key-value/key-value-sync)
key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py
[failure] 109-109:
"get" is not a known attribute of "None" (reportOptionalMemberAccess)
[failure] 102-102:
"get" is not a known attribute of "None" (reportOptionalMemberAccess)
🔇 Additional comments (7)
key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py (2)
5-5: LGTM: Imports support the new test functionality.The added imports (
json,IsDatetime,snapshot,ValkeyStore, and type-onlyBaseClient) are appropriate and correctly structured.Also applies to: 7-8, 10-11, 16-16, 20-22
98-114: Test logic is sound, but depends on fixing the helper.The test correctly validates the new serialization format (with
created_atand conditionalexpires_atfields), but the type annotationvalkey_client: BaseClient | Noneat line 101 reflects the unsafe helper return type. Once the helper is fixed to guarantee a non-Noneclient, update the annotation:- valkey_client: BaseClient | None = get_valkey_client_from_store(store=store) + valkey_client: BaseClient = get_valkey_client_from_store(store=store)This change should be made after fixing the helper as suggested in the previous comment.
key-value/key-value-aio/tests/stores/valkey/test_valkey.py (2)
2-2: LGTM: Imports correctly support async test functionality.The added imports are appropriate for the async variant, with correct package references (
glide.glide_clientfor async vs.glide_sync.glide_clientfor sync).Also applies to: 4-4, 7-8, 13-13, 20-22
100-116: Test logic is sound; update client calls after fixing helper.The async test correctly validates the serialization format with appropriate
awaitstatements. After applying the async helper fix from the previous comment, the test will work correctly. The suggested changes to await the helper and remove the| Nonetype annotation are already included in that fix.key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (2)
1-1: LGTM - Import additions support timezone-aware datetime handling.The additions of
datetimeandtimezoneare appropriate for the TTL timestamp conversions in_get_managed_entryand_put_managed_entry.
203-208: LGTM - TTL handling aligns with DynamoDB's native TTL mechanism.The code correctly deserializes the entry via the adapter and then populates
expires_atfrom DynamoDB's TTL attribute when present. This treats DynamoDB's TTL attribute as the authoritative source for expiration, which aligns with DynamoDB's automatic expiration behavior.key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py (1)
2-12: LGTM - Import additions support test functionality.The added imports for JSON parsing, datetime handling, snapshot testing, and DynamoDB type definitions are appropriate for the new test method.
key-value/key-value-shared/src/key_value/shared/utils/serialization.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py (1)
36-37: Helper function is unused and has unresolved critical issue.This helper is defined but never used in the test file. Additionally, past review comments have already flagged a critical issue: it returns an uninitialized client because ValkeyStore connects lazily. Before using this helper, ensure it calls
store.setup()to establish the connection (as detailed in the existing comments).key-value/key-value-aio/tests/stores/valkey/test_valkey.py (2)
38-40: Helper function is unused and would cause runtime errors if called.This helper returns
store._connected_clientwithout ensuring the async store has been set up. SinceValkeyStoreonly connects when_setup()is awaited (triggered by first operation or explicit setup call), this returnsNonebefore connection, causingAttributeErrorwhen the returned value is used.The helper is currently unused in this test file. Either remove it, or if it's intended for use, make it async and ensure setup is called first:
-def get_valkey_client_from_store(store: ValkeyStore): - return store._connected_client # pyright: ignore[reportPrivateUsage, reportReturnType] +async def get_valkey_client_from_store(store: ValkeyStore): + await store.setup() + client = store._connected_client # pyright: ignore[reportPrivateUsage] + assert client is not None, "Store client not connected after setup" + return clientThen use it in the test below (see separate comment).
Based on learnings
99-101: Fragile pattern: accessing client without explicitly ensuring setup.While this works because
put()on line 97 implicitly triggers_setup(), directly accessingstore._connected_clientwithout explicit setup is fragile and depends on implementation details. If the test is refactored or the put call is moved/removed, line 101'sawait valkey_client.get(...)will raiseAttributeErroronNone.If the helper
get_valkey_client_from_storeabove is fixed (made async and ensuring setup), use it here. Otherwise, explicitly call setup:- valkey_client = store._connected_client # pyright: ignore[reportPrivateUsage] - assert valkey_client is not None + await store.setup() + valkey_client = store._connected_client # pyright: ignore[reportPrivateUsage] + assert valkey_client is not None, "Client should be connected after setup"Or, if the helper is fixed per the previous comment:
- valkey_client = store._connected_client # pyright: ignore[reportPrivateUsage] - assert valkey_client is not None + valkey_client = await get_valkey_client_from_store(store=store)Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
key-value/key-value-aio/tests/stores/valkey/test_valkey.py(3 hunks)key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
key-value/key-value-aio/tests/stores/valkey/test_valkey.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (4)
BaseStore(53-383)put(296-309)get(187-213)ttl(226-235)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
ValkeyStore(23-155)
key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
ValkeyStore(25-151)key-value/key-value-aio/tests/stores/valkey/test_valkey.py (2)
get_valkey_client_from_store(38-39)store(81-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
🔇 Additional comments (2)
key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py (1)
5-5: LGTM! Imports support the new test.The added imports (
json,IsDatetime,snapshot) are appropriate for the snapshot-based verification of stored values with dynamic timestamps.Also applies to: 9-10
key-value/key-value-aio/tests/stores/valkey/test_valkey.py (1)
96-113: Test logic validates serialization format correctly.The test appropriately verifies the stored data structure by:
- Using the raw Valkey client to inspect stored bytes
- Validating the serialized format includes
created_at(always) andexpires_at(when TTL is set)- Using flexible snapshot assertions with
IsDatetimefor timestampsThis aligns well with the PR's serialization adapter refactoring.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py (1)
96-97: Ensure store connection before accessing private client attribute.While the
put()call at line 92 should trigger lazy connection, accessing_connected_clientdirectly is fragile and relies on implicit behavior. For robustness and clarity, explicitly callstore.setup()before accessing the private attribute.Apply this diff to make the connection explicit:
assert isinstance(store, ValkeyStore) +store.setup() valkey_client = store._connected_client # pyright: ignore[reportPrivateUsage] assert valkey_client is not None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
key-value/key-value-aio/src/key_value/aio/stores/simple/store.py(4 hunks)key-value/key-value-aio/tests/stores/valkey/test_valkey.py(2 hunks)key-value/key-value-shared/src/key_value/shared/utils/serialization.py(1 hunks)key-value/key-value-shared/tests/utils/test_serialization.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/simple/store.py(4 hunks)key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
key-value/key-value-aio/src/key_value/aio/stores/simple/store.py (3)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(15-63)load_from_json(76-82)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
BasicSerializationAdapter(136-148)load_json(52-56)dump_json(131-133)key-value/key-value-sync/src/key_value/sync/code_gen/stores/simple/store.py (1)
SimpleStoreEntry(21-39)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (4)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
DeserializationError(14-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (8)
ManagedEntry(15-63)dump_to_json(67-72)load_from_json(76-82)verify_dict(86-95)value_as_dict(46-47)value_as_json(41-43)created_at_isoformat(50-51)expires_at_isoformat(54-55)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (2)
prepare_load(66-88)prepare_dump(53-63)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (2)
prepare_load(100-111)prepare_dump(87-97)
key-value/key-value-shared/tests/utils/test_serialization.py (2)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (4)
BasicSerializationAdapter(136-148)load_json(52-56)load_dict(64-103)dump_dict(111-129)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/simple/store.py (3)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(15-63)load_from_json(76-82)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
BasicSerializationAdapter(136-148)load_json(52-56)dump_json(131-133)key-value/key-value-aio/src/key_value/aio/stores/simple/store.py (1)
SimpleStoreEntry(23-43)
key-value/key-value-aio/tests/stores/valkey/test_valkey.py (2)
key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py (2)
test_value_stored(89-110)store(74-83)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
ValkeyStore(23-155)
key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py (2)
key-value/key-value-aio/tests/stores/valkey/test_valkey.py (2)
test_value_stored(91-112)store(76-85)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
ValkeyStore(25-151)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
🔇 Additional comments (8)
key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py (2)
5-5: LGTM! Imports support the new test functionality.The new imports (
json,IsDatetime,snapshot) are all used in thetest_value_storedmethod to decode and validate the stored JSON structure.Also applies to: 9-10
89-110: LGTM! Test correctly validates serialization format.The test properly validates that the
SerializationAdapterstores data in the expected JSON structure, includingcreated_attimestamps and optionalexpires_atwhen TTL is specified. The use of snapshot testing withIsDatetimeis appropriate for verifying timestamp fields while allowing for dynamic values.key-value/key-value-aio/tests/stores/valkey/test_valkey.py (2)
2-2: LGTM! New imports support serialization validation.The added imports (
json,IsDatetime,snapshot) are necessary for the new test that validates the serialization adapter's JSON output format against snapshots with dynamic timestamp matching.Also applies to: 6-7
100-112: Excellent validation of the serialization adapter pattern.The test correctly validates the new adapter-based serialization approach:
- Compound key format (
test::test_key) matches the ValkeyStore implementation- Snapshot assertions verify the presence of
created_atandvaluefields in all cases- With TTL, the test confirms that
expires_atis properly included- Using
IsDatetimematchers appropriately handles dynamic timestamp valuesThis test effectively validates the PR's serialization refactoring objectives.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/simple/store.py (1)
74-77: Verify the manual timestamp assignment pattern is documented.The store deserializes via the adapter with
date_format=None, then manually assignsexpires_atandcreated_atfromSimpleStoreEntry. This split responsibility is intentional (the adapter handles only the value, while the store manages temporal metadata separately), but the pattern is subtle and could be fragile if refactored.Consider adding an inline comment explaining why timestamps are managed outside the adapter:
# Adapter is configured with date_format=None, so timestamps are managed # separately by SimpleStore rather than serialized in the JSON payload managed_entry.expires_at = store_entry.expires_at managed_entry.created_at = store_entry.created_atkey-value/key-value-aio/src/key_value/aio/stores/simple/store.py (1)
78-81: Verify the manual timestamp assignment pattern is documented.The store deserializes via the adapter with
date_format=None, then manually assignsexpires_atandcreated_atfromSimpleStoreEntry. This split responsibility is intentional (the adapter handles only the value, while the store manages temporal metadata separately), but the pattern is subtle and could be fragile if refactored.Consider adding an inline comment explaining why timestamps are managed outside the adapter:
# Adapter is configured with date_format=None, so timestamps are managed # separately by SimpleStore rather than serialized in the JSON payload managed_entry.expires_at = store_entry.expires_at managed_entry.created_at = store_entry.created_atkey-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
64-103: LGTM: Empty dict handling is correct.The presence check on line 83 (
if "value" not in data) correctly distinguishes between a missing key and a valid empty dict payload ({}), addressing the concerns from past reviews. Empty dicts will pass theisinstance(value, dict)check on line 93 and be accepted byverify_dict.
136-148: LGTM: BasicSerializationAdapter implementation is clean.The no-op
prepare_loadandprepare_dumpmethods correctly implement the pass-through behavior for stores that don't require transformations, making this a solid foundation for the adapter pattern.
key-value/key-value-aio/src/key_value/aio/stores/simple/store.py
Outdated
Show resolved
Hide resolved
| valkey_client = store._connected_client # pyright: ignore[reportPrivateUsage] | ||
| assert valkey_client is not None |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider explicit setup to ensure client initialization.
While await store.put() on line 94 should trigger _setup() and initialize _connected_client, past review comments on this PR flagged concerns about accessing _connected_client without explicit setup. For defensive coding and to align with previous feedback, consider calling await store.setup() before accessing the private client attribute.
Apply this diff to make the setup explicit:
assert isinstance(store, ValkeyStore)
+ await store.setup()
valkey_client = store._connected_client # pyright: ignore[reportPrivateUsage]Based on past review comments.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/valkey/test_valkey.py around lines
98-99, the test accesses the private attribute store._connected_client after
calling await store.put(); update the test to explicitly await store.setup()
before reading _connected_client to ensure initialization (i.e., call await
store.setup() and then assert store._connected_client is not None), keeping the
pyright ignore if still required.
key-value/key-value-shared/src/key_value/shared/utils/serialization.py
Outdated
Show resolved
Hide resolved
| @pytest.fixture | ||
| def serialization_adapter() -> BasicSerializationAdapter: | ||
| return BasicSerializationAdapter() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove the unused module-level fixture.
The serialization_adapter fixture is never used since the test class defines its own adapter fixture at lines 31-33.
Apply this diff to remove the unused fixture:
-@pytest.fixture
-def serialization_adapter() -> BasicSerializationAdapter:
- return BasicSerializationAdapter()
-
-
class TestBasicSerializationAdapter:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.fixture | |
| def serialization_adapter() -> BasicSerializationAdapter: | |
| return BasicSerializationAdapter() |
🤖 Prompt for AI Agents
In key-value/key-value-shared/tests/utils/test_serialization.py around lines 25
to 27, remove the unused module-level fixture definition `serialization_adapter`
(the three-line pytest.fixture block) because the test class provides its own
`adapter` fixture; delete those lines so only the class-level fixture remains
and run tests to confirm there are no regressions.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/simple/store.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-value/key-value-aio/src/key_value/aio/stores/simple/store.py (1)
36-43: Remove unusedto_managed_entrymethod from both aio and sync stores.The
to_managed_entrymethod is dead code—no calls found anywhere. The codebase now usesself._serialization_adapter.load_json()instead (line 78 in aio store, similar pattern in sync).Apply diff to both files:
key-value/key-value-aio/src/key_value/aio/stores/simple/store.py(lines 36-43)key-value/key-value-sync/src/key_value/sync/code_gen/stores/simple/store.py(lines 34-41)- def to_managed_entry(self) -> ManagedEntry: - managed_entry: ManagedEntry = ManagedEntry( - value=load_from_json(json_str=self.json_str), - expires_at=self.expires_at, - created_at=self.created_at, - ) - - return managed_entry - -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/simple/store.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
key-value/key-value-aio/src/key_value/aio/stores/simple/store.py (3)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(15-63)load_from_json(76-82)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
BasicSerializationAdapter(136-148)load_json(52-56)dump_json(131-133)key-value/key-value-sync/src/key_value/sync/code_gen/stores/simple/store.py (1)
SimpleStoreEntry(21-39)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (3)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
BasicSerializationAdapter(136-148)load_json(52-56)dump_json(131-133)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (3)
MemoryCacheEntry(29-34)get(69-77)put(79-81)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)
🔇 Additional comments (4)
key-value/key-value-aio/src/key_value/aio/stores/simple/store.py (4)
1-1: LGTM! Imports are correctly added.The
sysimport is needed forsys.maxsizeon line 61, andBasicSerializationAdapteris needed for the adapter pattern on line 65.Also applies to: 8-8
53-67: LGTM! Correctly addresses the previous review feedback.The explicit
Nonecheck on line 61 correctly handlesmax_entries=0(addressing the past review comment). The adapter initialization withdate_format=Noneis appropriate sinceexpires_atandcreated_atare stored separately inSimpleStoreEntryrather than in the serialized JSON.
78-81: LGTM! Adapter-based deserialization is correctly implemented.The pattern of loading via the adapter and then explicitly setting
expires_atandcreated_atfrom the stored entry ensures that theSimpleStoreEntryfields remain the authoritative source for metadata, which is consistent with the storage pattern.
91-93: LGTM! Adapter-based serialization is correctly implemented.The serialization pattern using
dump_json()and storingexpires_at/created_atseparately is consistent with the deserialization logic and enables efficient metadata access (e.g., thecurrent_ttlproperty on line 29).
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (4)
key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py (3)
58-60: Guard against missing/invalid JSON before json.loads.Current code may raise TypeError when value.S is absent (json.loads({})). Add a safe guard and handle malformed JSON.
Apply this diff:
def get_value_from_response(response: GetItemOutputTypeDef) -> dict[str, Any]: - return json.loads(response.get("Item", {}).get("value", {}).get("S", {})) # pyright: ignore[reportArgumentType] + json_str = response.get("Item", {}).get("value", {}).get("S") + if not json_str: + return {} + return json.loads(json_str)
62-64: Document precondition for accessing the private client.Add a concise docstring noting the store must be connected; otherwise ValueError is raised.
Apply this diff:
def get_dynamo_client_from_store(store: DynamoDBStore) -> DynamoDBClient: + """Return the connected DynamoDB client. + + Assumes the store has been initialized and connected in the test setup. + Raises: + ValueError: If the client is not connected. + """ return store._connected_client # pyright: ignore[reportPrivateUsage]
121-146: Strengthen TTL validation; assert against created_at + configured TTL.Messages are misleading and bounds are weak. Validate ttl precisely relative to created_at with small tolerance.
Apply this diff:
# Verify DynamoDB TTL attribute is set for automatic expiration assert "ttl" in response.get("Item", {}), "DynamoDB TTL attribute should be set when ttl parameter is provided" ttl_value = int(response["Item"]["ttl"]["N"]) # pyright: ignore[reportTypedDictNotRequiredAccess] - now = datetime.now(timezone.utc) - assert ttl_value > now.timestamp(), "TTL timestamp should be a positive integer" - assert ttl_value < now.timestamp() + 10, "TTL timestamp should be less than the expected expiration time" + # TTL should match created_at + 10 seconds (±1s for execution latency) + value_data = get_value_from_response(response=response) + created_at = datetime.fromisoformat(value_data["created_at"]) + expected_ttl = int(created_at.timestamp() + 10) + assert abs(ttl_value - expected_ttl) <= 1, ( + f"TTL should be ~created_at+10s (expected ~{expected_ttl}, got {ttl_value})" + )key-value/key-value-shared/src/key_value/shared/utils/serialization.py (1)
83-98: Clearer error for value=None.Optional: emit a specific message when value is None; helps diagnostics.
if "value" not in data: msg = "Value field not found" raise DeserializationError(message=msg) value = data["value"] + if value is None: + msg = "Value field is None" + raise DeserializationError(message=msg) managed_entry_value: dict[str, Any] = {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py(3 hunks)key-value/key-value-aio/tests/stores/memcached/test_memcached.py(3 hunks)key-value/key-value-aio/tests/stores/mongodb/test_mongodb.py(4 hunks)key-value/key-value-aio/tests/stores/rocksdb/test_rocksdb.py(2 hunks)key-value/key-value-aio/tests/stores/simple/test_store.py(1 hunks)key-value/key-value-aio/tests/stores/vault/test_vault.py(1 hunks)key-value/key-value-aio/tests/stores/windows_registry/test_windows_registry.py(1 hunks)key-value/key-value-shared/src/key_value/shared/utils/serialization.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/mongodb/test_mongodb.py(4 hunks)key-value/key-value-sync/tests/code_gen/stores/rocksdb/test_rocksdb.py(2 hunks)key-value/key-value-sync/tests/code_gen/stores/simple/test_store.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/vault/test_vault.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/windows_registry/test_windows_registry.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
key-value/key-value-aio/tests/stores/memcached/test_memcached.py (2)
key-value/key-value-aio/tests/stores/rocksdb/test_rocksdb.py (2)
store(21-27)test_value_stored(72-87)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
MemcachedStore(20-130)
key-value/key-value-aio/tests/stores/mongodb/test_mongodb.py (2)
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
MongoDBSerializationAdapter(39-88)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(111-129)load_dict(64-103)
key-value/key-value-sync/tests/code_gen/stores/mongodb/test_mongodb.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
MongoDBSerializationAdapter(46-95)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(111-129)load_dict(64-103)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
DeserializationError(14-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (8)
ManagedEntry(15-63)dump_to_json(67-72)load_from_json(76-82)verify_dict(86-95)value_as_dict(46-47)value_as_json(41-43)created_at_isoformat(50-51)expires_at_isoformat(54-55)
key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py (1)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (2)
DynamoDBStore(30-258)_connected_client(134-138)
key-value/key-value-sync/tests/code_gen/stores/rocksdb/test_rocksdb.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
RocksDBStore(23-185)key-value/key-value-sync/tests/code_gen/stores/base.py (1)
ContextManagerStoreTestMixin(263-275)
key-value/key-value-aio/tests/stores/rocksdb/test_rocksdb.py (2)
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
RocksDBStore(20-194)key-value/key-value-sync/tests/code_gen/stores/rocksdb/test_rocksdb.py (3)
rocksdb_client(72-73)store(24-30)test_value_stored(75-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
🔇 Additional comments (14)
key-value/key-value-sync/tests/code_gen/stores/vault/test_vault.py (1)
32-32: LGTM! Warning suppression is appropriate for testing unstable features.The warning suppression is correctly applied and appropriate for test code that explicitly exercises an experimental/unstable store. The decorator syntax is correct and will suppress the warning for all tests in the class.
key-value/key-value-aio/tests/stores/windows_registry/test_windows_registry.py (1)
18-18: This warning filter follows the established pattern for unstable stores and requires no changes.The filterwarnings decorator suppresses an expected UserWarning from the
_warn_about_stability()method in BaseStore. This pattern is applied consistently across all unstable store tests in the codebase (Vault, RocksDB, MongoDB, Simple store, Memcached, DynamoDB), not just Windows Registry. Since these stores are intentionally marked as unstable and still under development, suppressing this warning during testing is the appropriate approach and does not mask issues introduced by the serialization refactor.key-value/key-value-sync/tests/code_gen/stores/simple/test_store.py (1)
11-11: This warning suppression is part of intentional API design and requires no changes.The
_warn_about_stability()warning is not a hidden problem being masked—it's an accepted part of the library's API stability tier system. SimpleStore and other unstable stores intentionally emit this warning at runtime. The_stable_apiflag allows stores to opt into "stable" status (e.g., Redis, Memory, Disk all set_stable_api = True); those that don't remain unstable and warn users appropriately.Test decorators suppress these expected warnings to prevent noise during test execution, while users of unstable stores still see the warning at runtime. This is the correct pattern for managing API maturity across the library.
Likely an incorrect or invalid review comment.
key-value/key-value-aio/tests/stores/memcached/test_memcached.py (3)
2-2: LGTM! Imports support new test functionality.The added imports are necessary for the new
test_value_storedmethod and follow the same pattern used in other store tests (e.g.,test_rocksdb.py).Also applies to: 7-8
48-48: LGTM! Warning filter appropriately scoped.The class-level warning filter correctly suppresses the instability warning for all Memcached tests, which is appropriate given this is testing an experimental store.
72-74: LGTM! Fixture appropriately exposes client for testing.Accessing the private
_clientattribute is acceptable in test code for validating the stored format. The pyright ignore comment properly handles the type checker warning.key-value/key-value-sync/tests/code_gen/stores/windows_registry/test_windows_registry.py (1)
1-20: I need to verify one more detail about the exact location in the auto-generated file to ensure complete consistency.No changes needed — the decorator was correctly applied to the source file.
The key_value.sync module is generated from the async library, so changes must be made to the source in
key-value-aio/tests/stores/windows_registry/test_windows_registry.py. The script confirmed thefilterwarningsdecorator is already present at line 18 of the source file, which is why it appears in this auto-generated file. The warning header about not editing the file directly is standard for generated code and does not indicate a problem with this change.key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py (4)
2-5: Imports look fine; nothing to change.
8-12: Test utilities and typed clients imported appropriately.
67-68: Warning filter addition is reasonable for unstable store notice.
120-120: Skip marker retained intentionally; no change requested.key-value/key-value-aio/tests/stores/mongodb/test_mongodb.py (1)
102-102: Warning filter is appropriate.Okay to suppress instability warning at the class level here.
key-value/key-value-sync/tests/code_gen/stores/rocksdb/test_rocksdb.py (1)
75-90: Stored‑value assertions match adapter behavior.Shapes for both no‑TTL and TTL cases are correct (isoformat strings for datetimes). LGTM.
key-value/key-value-sync/tests/code_gen/stores/mongodb/test_mongodb.py (1)
122-129: Collection name sanitization test is clear and stable.Snapshot matches the expected sanitized name format. LGTM.
| async def test_value_stored(self, store: MemcachedStore, memcached_client: Client): | ||
| await store.put(collection="test", key="test_key", value={"name": "Alice", "age": 30}) | ||
|
|
||
| value = await memcached_client.get(key=b"test::test_key") | ||
| assert value is not None | ||
| value_as_dict = json.loads(value.decode("utf-8")) | ||
| assert value_as_dict == snapshot({"created_at": IsDatetime(iso_string=True), "value": {"age": 30, "name": "Alice"}}) | ||
|
|
||
| await store.put(collection="test", key="test_key", value={"name": "Alice", "age": 30}, ttl=10) | ||
|
|
||
| value = await memcached_client.get(key=b"test::test_key") | ||
| assert value is not None | ||
| value_as_dict = json.loads(value.decode("utf-8")) | ||
| assert value_as_dict == snapshot( | ||
| { | ||
| "created_at": IsDatetime(iso_string=True), | ||
| "expires_at": IsDatetime(iso_string=True), | ||
| "value": {"age": 30, "name": "Alice"}, | ||
| } | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding negative assertion for removed ttl field.
The test validates the stored JSON structure correctly. However, given that this PR removes the stored ttl field in favor of computing it from expires_at, consider adding an explicit assertion that "ttl" is NOT present in the stored JSON to prevent regression.
Apply this diff to add the negative assertion:
value = await memcached_client.get(key=b"test::test_key")
assert value is not None
value_as_dict = json.loads(value.decode("utf-8"))
assert value_as_dict == snapshot({"created_at": IsDatetime(iso_string=True), "value": {"age": 30, "name": "Alice"}})
+ assert "ttl" not in value_as_dict
await store.put(collection="test", key="test_key", value={"name": "Alice", "age": 30}, ttl=10)
value = await memcached_client.get(key=b"test::test_key")
assert value is not None
value_as_dict = json.loads(value.decode("utf-8"))
assert value_as_dict == snapshot(
{
"created_at": IsDatetime(iso_string=True),
"expires_at": IsDatetime(iso_string=True),
"value": {"age": 30, "name": "Alice"},
}
)
+ assert "ttl" not in value_as_dict🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/memcached/test_memcached.py around lines
76 to 95, add explicit negative assertions that the stored JSON does not contain
the "ttl" key to prevent regressions from reintroducing it: after decoding
value_as_dict in both places (the initial put and the put with ttl=10) add
assertions like assert "ttl" not in value_as_dict to ensure ttl is removed and
only expires_at is used; keep placement immediately after value_as_dict is
created and before the snapshot equality assertion.
| adapter = MongoDBSerializationAdapter(native_storage=True) | ||
| document = adapter.dump_dict(entry=managed_entry) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Adapter round‑trip looks good.
Round‑trip via MongoDBSerializationAdapter in native mode is correct and assertions are precise. Consider adding one case where created_at/expires_at are naive datetimes to validate the UTC normalization in prepare_load.
Also applies to: 65-65
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/mongodb/test_mongodb.py around lines
54-56 (and also applies to line 65), add a test case that constructs a
managed_entry with created_at and expires_at as naive datetimes (no tzinfo),
then use MongoDBSerializationAdapter(native_storage=True) to dump and
subsequently load the document, and assert that after prepare_load the returned
datetimes have tzinfo set to UTC and represent the same instants (i.e.,
converted correctly to UTC); ensure assertions check both tzinfo is timezone.utc
and that the timestamps match expected UTC values.
| adapter = MongoDBSerializationAdapter(native_storage=False) | ||
| document = adapter.dump_dict(entry=managed_entry) | ||
|
|
||
| assert document == snapshot( |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Legacy mode coverage is solid.
Dump/load assertions for string storage are accurate. Same optional naive‑datetime check would harden the test matrix.
Also applies to: 90-90
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/mongodb/test_mongodb.py around lines
78-81 (and also line 90), the test covers dump/load for string storage but
misses an assertion for optional naive-datetime handling; update the test to
include an additional case where managed_entry contains a naive datetime (no
tzinfo), call adapter.dump_dict and adapter.load_dict (or round-trip) for that
entry, and assert the resulting document/value matches the expected snapshot or
equals the original naive datetime representation so the matrix verifies
naive‑datetime behavior in legacy/native string storage.
| import json | ||
| from collections.abc import AsyncGenerator | ||
| from pathlib import Path | ||
| from tempfile import TemporaryDirectory | ||
|
|
||
| import pytest | ||
| from dirty_equals import IsDatetime | ||
| from inline_snapshot import snapshot | ||
| from rocksdict import Rdict |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Imports are appropriate for the new assertions; optional de-dup.
json, IsDatetime, snapshot, and Rdict are all used below. Minor nit: Rdict is also imported inside test_rocksdb_db_connection; consider relying on the top-level import only.
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/rocksdb/test_rocksdb.py lines 1-9, the
module imports are fine but Rdict is duplicated later inside
test_rocksdb_db_connection; remove the redundant local import and use the
top-level Rdict import instead to avoid duplication and minor confusion. Ensure
any test that referenced the local import still uses Rdict from the module-level
import and run the tests to confirm no namespace regressions.
| from tests.stores.base import BaseStoreTests, ContextManagerStoreTestMixin | ||
|
|
||
|
|
||
| @pytest.mark.filterwarnings("ignore:A configured store is unstable and may change in a backwards incompatible way. Use at your own risk.") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make the warning filter less brittle.
Match with a regex prefix so message tweaks don’t break the filter.
-@pytest.mark.filterwarnings("ignore:A configured store is unstable and may change in a backwards incompatible way. Use at your own risk.")
+@pytest.mark.filterwarnings(r"ignore:^A configured store is unstable.*")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.filterwarnings("ignore:A configured store is unstable and may change in a backwards incompatible way. Use at your own risk.") | |
| @pytest.mark.filterwarnings(r"ignore:^A configured store is unstable.*") |
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/rocksdb/test_rocksdb.py around line 17,
the pytest warning filter matches the full warning text and is brittle; change
it to use a regex prefix (start-anchor) that matches the beginning of the
warning message so small wording changes won’t break the filter (e.g. use an
expression that begins with ^A configured store is unstable and match the rest
more loosely).
| def parse_datetime_str(value: str) -> datetime: | ||
| try: | ||
| return datetime.fromisoformat(value) | ||
| except ValueError: | ||
| msg = f"Invalid datetime string: {value}" | ||
| raise DeserializationError(message=msg) from None | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Harden ISO datetime parsing for 'Z' and naive inputs.
fromisoformat may not accept 'Z' in all runtimes and returns naive datetimes for naive strings. Normalize both.
-from datetime import datetime
+from datetime import datetime, timezone
@@
-def parse_datetime_str(value: str) -> datetime:
- try:
- return datetime.fromisoformat(value)
+def parse_datetime_str(value: str) -> datetime:
+ try:
+ # Accept trailing 'Z' and normalize naive datetimes to UTC
+ normalized = value[:-1] + "+00:00" if value.endswith("Z") else value
+ dt = datetime.fromisoformat(normalized)
+ return dt if dt.tzinfo is not None else dt.replace(tzinfo=timezone.utc)
except ValueError:
msg = f"Invalid datetime string: {value}"
raise DeserializationError(message=msg) from NoneCheck whether now()/now_plus() are timezone‑aware to avoid naive/aware comparisons:
🏁 Script executed:
#!/bin/bash
rg -nP 'def\s+now\(|def\s+now_plus\(' -C2 key-valueLength of output: 2385
Harden ISO datetime parsing for 'Z' and naive inputs to match timezone-aware datetime functions in codebase.
All now() and now_plus() functions return timezone-aware UTC datetimes. The current parse_datetime_str can return naive datetimes or fail on 'Z' suffix, creating comparison incompatibilities. Apply the suggested normalization to ensure consistent timezone-aware returns across the codebase.
-from datetime import datetime
+from datetime import datetime, timezone
@@
-def parse_datetime_str(value: str) -> datetime:
- try:
- return datetime.fromisoformat(value)
+def parse_datetime_str(value: str) -> datetime:
+ try:
+ # Accept trailing 'Z' and normalize naive datetimes to UTC
+ normalized = value[:-1] + "+00:00" if value.endswith("Z") else value
+ dt = datetime.fromisoformat(normalized)
+ return dt if dt.tzinfo is not None else dt.replace(tzinfo=timezone.utc)
except ValueError:
msg = f"Invalid datetime string: {value}"
raise DeserializationError(message=msg) from None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def parse_datetime_str(value: str) -> datetime: | |
| try: | |
| return datetime.fromisoformat(value) | |
| except ValueError: | |
| msg = f"Invalid datetime string: {value}" | |
| raise DeserializationError(message=msg) from None | |
| def parse_datetime_str(value: str) -> datetime: | |
| try: | |
| # Accept trailing 'Z' and normalize naive datetimes to UTC | |
| normalized = value[:-1] + "+00:00" if value.endswith("Z") else value | |
| dt = datetime.fromisoformat(normalized) | |
| return dt if dt.tzinfo is not None else dt.replace(tzinfo=timezone.utc) | |
| except ValueError: | |
| msg = f"Invalid datetime string: {value}" | |
| raise DeserializationError(message=msg) from None |
🤖 Prompt for AI Agents
In key-value/key-value-shared/src/key_value/shared/utils/serialization.py around
lines 27 to 33, parse_datetime_str currently returns naive datetimes or fails on
strings ending with 'Z'; update it to first normalize a trailing 'Z' to
'+00:00', then parse with datetime.fromisoformat, and if the resulting datetime
is naive attach timezone.utc so the function always returns a timezone-aware UTC
datetime; preserve the existing DeserializationError raising on invalid input.
| adapter = MongoDBSerializationAdapter(native_storage=True) | ||
| document = adapter.dump_dict(entry=managed_entry) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Adapter round‑trip (native) is correct.
Assertions align with MongoDBSerializationAdapter semantics. Consider adding a naive‑datetime case to verify UTC attachment.
Also applies to: 66-66
🤖 Prompt for AI Agents
In key-value/key-value-sync/tests/code_gen/stores/mongodb/test_mongodb.py around
lines 55-57 (also apply similar change at line 66), add a naive-datetime test
case: create a managed_entry containing a datetime without tzinfo, pass it
through MongoDBSerializationAdapter.dump_dict (native_storage=True), then assert
the resulting document has the datetime converted/annotated to UTC (tzinfo set
to UTC or ISO string with Z) and optionally round-trip via adapter.load_dict to
confirm original semantics; implement assertions consistent with existing
adapter behavior for attaching UTC to naive datetimes.
| adapter = MongoDBSerializationAdapter(native_storage=False) | ||
| document = adapter.dump_dict(entry=managed_entry) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Adapter round‑trip (legacy) is correct.
String mode expectations look right. Optional: add a malformed value test to ensure DeserializationError is raised as expected.
Also applies to: 90-90
🤖 Prompt for AI Agents
In key-value/key-value-sync/tests/code_gen/stores/mongodb/test_mongodb.py around
lines 79-81 (and similarly at line 90), add a negative test that supplies a
malformed serialized value to the MongoDBSerializationAdapter and asserts that
DeserializationError is raised; specifically, construct the adapter as in the
existing test, create an entry with an invalid/garbled string payload (or an
incorrect type) and call the adapter.load_* method inside a
pytest.raises(DeserializationError) context to verify the adapter fails cleanly.
|
@claude why is dynamo ttl not working in the older version test? The latest workflow runs are failing only for the lowest dependency versions |
|
Claude finished @strawgate's task —— View job Investigating DynamoDB TTL Test FailuresI've identified and fixed the root cause of the DynamoDB TTL test failures in older dependency versions. AnalysisThe Bug: Lines 229-232 in 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)}Why it fails: @property
def ttl(self) -> float | None:
if self.expires_at is None:
return None
return seconds_to(datetime=self.expires_at) # Returns: (expires_at - now()).total_seconds()This calculates the remaining seconds from the current moment to
Why older dependencies fail more often: Slower test infrastructure or dependencies can introduce timing delays between entry creation and the TTL property access, making the remaining TTL shorter and causing the timestamp to be consistently incorrect. The timing-dependent nature of the bug makes it appear intermittently, especially in slower environments. The FixChanged the code to use if managed_entry.expires_at is not None:
# DynamoDB TTL expects a Unix timestamp
ttl_timestamp = int(managed_entry.expires_at.timestamp())
item["ttl"] = {"N": str(ttl_timestamp)}This ensures the TTL timestamp is always correct, regardless of when the property is accessed. Tasks
Fixed in commit: |
The managed_entry.ttl property returns remaining seconds from NOW, not the original TTL duration. This caused incorrect TTL timestamps when there was any delay between entry creation and property access. Using expires_at directly ensures the timestamp is always correct. Fixes test failures in older dependency versions where timing delays were more common. Co-authored-by: William Easton <strawgate@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (4)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (1)
get(68-76)key-value/key-value-aio/src/key_value/aio/stores/base.py (2)
get(187-213)ttl(226-235)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(52-56)dump_json(131-136)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ttl(35-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
🔇 Additional comments (2)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (2)
1-1: LGTM: Import added for TTL timestamp conversion.The
datetimeandtimezoneimports are correctly added to support the TTL timestamp conversion in_get_managed_entry.
220-232: LGTM: Serialization adapter integration is correct.The
_put_managed_entrymethod correctly:
- Uses the serialization adapter to convert the
ManagedEntryto JSON (which includesexpires_at)- Separately sets the DynamoDB
ttlfield fromexpires_atfor DynamoDB's automatic expiration mechanismThis dual storage ensures both application-level expiration checking and DynamoDB's built-in TTL work correctly.
| managed_entry = self._serialization_adapter.load_json(json_str=json_value) | ||
|
|
||
| ttl = item.get("ttl", {}).get("N") | ||
| if ttl: | ||
| managed_entry.expires_at = datetime.fromtimestamp(int(ttl), tz=timezone.utc) | ||
|
|
||
| return managed_entry |
There was a problem hiding this comment.
Potential TTL handling bug: unconditional overwrite of expires_at.
After deserializing the JSON (which should include expires_at if present), the code unconditionally overwrites expires_at with the value from DynamoDB's ttl field. This creates several issues:
- Redundancy: If
expires_atis already in the JSON (as expected with the new serialization adapter), reading from thettlfield is unnecessary. - Inconsistency risk: The
ttlfield and JSONexpires_atcould drift out of sync, leading to incorrect expiration behavior. - Backwards compatibility unclear: If this is for backwards compatibility with older data that lacks
expires_atin JSON, it should conditionally setexpires_atonly when it'sNone.
This may be related to the TTL failures mentioned in the PR comments for older dependency versions.
Consider this approach for backwards compatibility:
managed_entry = self._serialization_adapter.load_json(json_str=json_value)
-ttl = item.get("ttl", {}).get("N")
-if ttl:
- managed_entry.expires_at = datetime.fromtimestamp(int(ttl), tz=timezone.utc)
+# For backwards compatibility: only set expires_at from ttl if not already present
+if managed_entry.expires_at is None:
+ ttl = item.get("ttl", {}).get("N")
+ if ttl:
+ managed_entry.expires_at = datetime.fromtimestamp(int(ttl), tz=timezone.utc)
return managed_entryAlternatively, if the DynamoDB ttl field is the source of truth, remove expires_at from the JSON serialization entirely to avoid duplication.
🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py around
lines 203-209, the code unconditionally overwrites managed_entry.expires_at with
the DynamoDB ttl value after deserializing JSON, which can incorrectly replace a
valid expires_at from the JSON; update the logic to only set
managed_entry.expires_at from the DynamoDB ttl when managed_entry.expires_at is
None (i.e., absent from the JSON) and ttl exists, converting ttl to an int and
using datetime.fromtimestamp(..., tz=timezone.utc); keep the JSON value as
authoritative otherwise (or, if you intend DynamoDB ttl to be source-of-truth,
remove expires_at from JSON serialization instead).
|
Adapt S3Store to match the serialization refactoring from PR #184: - Replace ManagedEntry.from_json() with self._serialization_adapter.load_json() - Replace managed_entry.to_json() with self._serialization_adapter.dump_json() - Leverage BasicSerializationAdapter automatically initialized by BaseStore Co-authored-by: William Easton <strawgate@users.noreply.github.com>
Updates FileTreeStore to work with the new SerializationAdapter pattern introduced in main branch PR #184: - Uses self._serialization_adapter.load_json() instead of ManagedEntry.from_json() - Uses self._serialization_adapter.dump_json() instead of .to_json() - Passes serialization_adapter parameter to BaseStore.__init__() - Maintains all original functionality (path traversal protection, etc.) - Includes comprehensive tests and documentation Co-authored-by: William Easton <strawgate@users.noreply.github.com>
- Add DuckDBSerializationAdapter class following MongoDB/Elasticsearch patterns - Move inline imports (json, timezone) to module level - Update _get_managed_entry() to use adapter.load_dict() - Update _put_managed_entry() to use adapter.dump_dict() - Fix floating point precision issue in SQL queryability test - All tests passing (405 passed, 7 skipped) Aligns DuckDB store with SerializationAdapter refactoring from PR #184 Co-authored-by: William Easton <strawgate@users.noreply.github.com>
Adapt S3Store to match the serialization refactoring from PR #184: - Replace ManagedEntry.from_json() with self._serialization_adapter.load_json() - Replace managed_entry.to_json() with self._serialization_adapter.dump_json() - Leverage BasicSerializationAdapter automatically initialized by BaseStore Co-authored-by: William Easton <strawgate@users.noreply.github.com>
Adapt S3Store to match the serialization refactoring from PR #184: - Replace ManagedEntry.from_json() with self._serialization_adapter.load_json() - Replace managed_entry.to_json() with self._serialization_adapter.dump_json() - Leverage BasicSerializationAdapter automatically initialized by BaseStore Co-authored-by: William Easton <strawgate@users.noreply.github.com>
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests