Refactor ManagedEntry serialization with adapter pattern #174
Conversation
|
@claude please fix the merge conflicts and put the store-specific adapters in the store implementations |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@claude this PR needs to have conflicts fixed and review the coderabbit feedback, its probably wrong in a bunch of spots so please be thorough |
|
Claude finished @strawgate's task —— View job ✅ PR #174: Conflicts Fixed and Adapter Pattern AppliedSummarySuccessfully fixed the branch history issues and applied the serialization adapter pattern refactoring from scratch on a clean main branch. What Was Done1. Fixed Branch History
2. Implemented Adapter Pattern
3. Performance Improvements
4. Error Handling
5. Code Quality
What Was NOT Done
Key Differences from Original PRThis implementation:
Next StepsThe maintainer should:
|
…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 <[email protected]> 🤖 Generated with [Claude Code](https://claude.com/claude-code)
claude
bot
force-pushed
the
claude/issue-165-20251029-0155
branch
from
36aa82e to
796e8ec
Compare
|
@claude Each store shouldn't have it's own ABC for SerializationAdapter and we should have a couple common serialization adapters as described so that each store doesn't need to implement basic ones We also need to lint and codegen |
- 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 <[email protected]>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@claude let's put the store-specific adapters in the store's code and let's move as much of the serialization/validation logic out of managedentry as we can into the adapter base class maybe? |
This comment was marked as resolved.
This comment was marked as resolved.
- 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 <[email protected]>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
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-shared/src/key_value/shared/utils/managed_entry.py (1)
132-137: Stop passing the removedttlkwarg and restore TTL fallback logic
ManagedEntryno longer accepts attl=parameter after the field was deleted, yetfrom_dictstill callscls(..., ttl=ttl_seconds, …). This is exactly what basedpyright is flagging (“No parameter named 'ttl'”), and it will raise aTypeErrorat runtime. We need to drop that kwarg and, if a TTL override is supplied, translate it into an expiration timestamp before instantiating the dataclass.- ttl_seconds: float | None = prepare_ttl(t=ttl) - - return cls( - created_at=created_at, - expires_at=expires_at, - ttl=ttl_seconds, - value=value, - ) + ttl_seconds: float | None = prepare_ttl(t=ttl) + + if ttl_seconds is not None and expires_at is None: + expires_at = now() + timedelta(seconds=ttl_seconds) + + return cls( + created_at=created_at, + expires_at=expires_at, + value=value, + )(Remember to import
timedeltaalongsidedatetime.) Without this fix we can’t ship—both the type checker and runtime will fail here.
♻️ Duplicate comments (2)
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
66-69: Critical: Mapping type mismatch will cause indexing failures.This issue was flagged in a previous review:
value.stringis mapped astype: "object"withenabled: False, but the adapter writes a JSON string to this field (line 115 inprepare_dump). Elasticsearch will reject documents during indexing with a mapper_parsing_exception.The fix is to map
value.stringas a non-indexed text field:"string": { - "type": "object", - "enabled": False, + "type": "text", + "index": False },Update the comment above accordingly and test with both native_storage modes.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
72-81: Fix empty dict handling in prepare_load.The walrus operator at line 75 evaluates falsy when
value_objectis an empty dict ({}), causing valid entries with empty values to skip native deserialization and fall through to the error case. This was flagged in a previous review.Apply this diff:
@override def prepare_load(self, data: dict[str, Any]) -> dict[str, Any]: value = data.pop("value") - if value_object := value.get("object"): - data["value"] = value_object - elif value_string := value.get("string"): - data["value"] = value_string + if "object" in value: + data["value"] = value["object"] + elif "string" in value: + data["value"] = value["string"] else: msg = "Value field not found in MongoDB document" raise DeserializationError(message=msg)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
key-value/key-value-aio/src/key_value/aio/stores/base.py(5 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/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/tests/stores/elasticsearch/test_elasticsearch.py(3 hunks)key-value/key-value-aio/tests/stores/mongodb/test_mongodb.py(5 hunks)key-value/key-value-aio/tests/stores/redis/test_redis.py(4 hunks)key-value/key-value-shared/pyproject.toml(0 hunks)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py(3 hunks)key-value/key-value-shared/src/key_value/shared/utils/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/elasticsearch/store.py(8 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/tests/code_gen/stores/elasticsearch/test_elasticsearch.py(5 hunks)key-value/key-value-sync/tests/code_gen/stores/mongodb/test_mongodb.py(5 hunks)key-value/key-value-sync/tests/code_gen/stores/redis/test_redis.py(4 hunks)
💤 Files with no reviewable changes (1)
- key-value/key-value-shared/pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (16)
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(121-133)SerializationAdapter(28-118)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(34-37)ManagedEntry(14-143)
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(102-114)load_dict(57-94)
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(45-94)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(102-114)load_dict(57-94)
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (3)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(14-143)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (7)
SerializationAdapter(28-118)prepare_dump(97-100)prepare_dump(132-133)prepare_load(52-55)prepare_load(129-130)load_dict(57-94)dump_dict(102-114)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/mongodb/test_mongodb.py (2)
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
MongoDBSerializationAdapter(38-87)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(102-114)load_dict(57-94)
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 (1)
DeserializationError(14-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(14-143)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (7)
SerializationAdapter(28-118)prepare_dump(97-100)prepare_dump(132-133)prepare_load(52-55)prepare_load(129-130)load_dict(57-94)dump_dict(102-114)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (3)
MongoDBSerializationAdapter(45-94)prepare_dump(59-69)prepare_load(72-94)
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(121-133)SerializationAdapter(28-118)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(34-37)ManagedEntry(14-143)
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(102-114)load_dict(57-94)
key-value/key-value-aio/tests/stores/redis/test_redis.py (2)
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/serialization.py (5)
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(14-143)dump_to_json(146-151)load_from_json(154-160)verify_dict(163-172)value_as_dict(45-46)value_as_json(40-42)created_at_isoformat(49-50)expires_at_isoformat(53-54)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (1)
try_parse_datetime_str(34-41)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-aio/src/key_value/aio/stores/mongodb/store.py (2)
prepare_load(65-87)prepare_dump(52-62)
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/utils/managed_entry.py (1)
ManagedEntry(14-143)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (7)
SerializationAdapter(28-118)prepare_dump(97-100)prepare_dump(132-133)prepare_load(52-55)prepare_load(129-130)load_dict(57-94)dump_dict(102-114)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (3)
MongoDBSerializationAdapter(38-87)prepare_dump(52-62)prepare_load(65-87)
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(14-143)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (4)
BasicSerializationAdapter(121-133)SerializationAdapter(28-118)load_json(45-49)dump_json(116-118)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (5)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(14-143)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (7)
SerializationAdapter(28-118)prepare_dump(97-100)prepare_dump(132-133)prepare_load(52-55)prepare_load(129-130)load_dict(57-94)dump_dict(102-114)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (1)
now_as_epoch(14-16)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-shared/src/key_value/shared/errors/key_value.py (1)
DeserializationError(14-15)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.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/compound.py (2)
compound_key(32-34)get_keys_from_compound_keys(74-77)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(14-143)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (4)
BasicSerializationAdapter(121-133)SerializationAdapter(28-118)load_json(45-49)dump_json(116-118)
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-shared/src/key_value/shared/utils/managed_entry.py (3)
key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (6)
now(19-21)prepare_ttl(45-45)prepare_ttl(49-49)prepare_ttl(52-74)seconds_to(24-26)try_parse_datetime_str(34-41)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
ttl(219-228)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
ttl(220-229)
🪛 GitHub Actions: Run Tests
key-value/key-value-sync/tests/code_gen/stores/redis/test_redis.py
[error] 99-99: basedpyright: Argument type is unknown. Argument corresponds to parameter "s" in function "loads" (reportUnknownArgumentType).
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py
[error] 135-135: basedpyright: No parameter named 'ttl' (reportCallIssue). Command: uv run basedpyright .
🪛 GitHub Check: static_analysis (key-value/key-value-sync)
key-value/key-value-sync/tests/code_gen/stores/redis/test_redis.py
[failure] 124-124:
Argument of type "ResponseT" cannot be assigned to parameter "s" of type "str | bytes | bytearray" in function "loads"
Type "ResponseT" is not assignable to type "str | bytes | bytearray"
Type "Awaitable[Any]" is not assignable to type "str | bytes | bytearray"
"Awaitable[Any]" is not assignable to "str"
"Awaitable[Any]" is not assignable to "bytes"
"Awaitable[Any]" is not assignable to "bytearray" (reportArgumentType)
[failure] 114-114:
"Awaitable[Any]" is not iterable
"iter" method not defined (reportGeneralTypeIssues)
[failure] 114-114:
Type of "raw_document" is unknown (reportUnknownVariableType)
[failure] 114-114:
Argument type is unknown
Argument corresponds to parameter "s" in function "loads" (reportUnknownArgumentType)
[failure] 99-99:
"Awaitable[Any]" is not iterable
"iter" method not defined (reportGeneralTypeIssues)
[failure] 99-99:
Type of "raw_document" is unknown (reportUnknownVariableType)
[failure] 99-99:
Argument type is unknown
Argument corresponds to parameter "s" in function "loads" (reportUnknownArgumentType)
🔇 Additional comments (24)
key-value/key-value-sync/tests/code_gen/stores/redis/test_redis.py (2)
42-44: LGTM: Test helper for accessing client.The helper appropriately exposes the private client for test validation while acknowledging the private usage with a type ignore.
68-71: LGTM: Clean fixture design.The fixture appropriately provides Redis client access for raw document validation tests.
key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py (2)
53-75: LGTM: Solid adapter round-trip test.The test correctly validates the adapter's non-native storage mode with proper round-trip verification. Document structure and metadata preservation are thoroughly checked.
77-99: LGTM: Native storage mode validated.The test appropriately verifies native storage mode with flattened value structure and complete round-trip validation.
key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py (1)
54-91: LGTM: Consistent adapter test coverage.Both non-native and native storage modes are properly tested with round-trip validation. The sync version correctly mirrors the async implementation.
key-value/key-value-aio/src/key_value/aio/stores/redis/store.py (3)
28-88: LGTM: Adapter initialization is appropriate.The store correctly initializes
BasicSerializationAdapterwithisoformatdates anddictvalue format, which aligns with Redis's JSON string storage model.
90-123: LGTM: Robust deserialization with error handling.The get operations properly handle
DeserializationErrorby returningNonefor corrupt documents, preventing failures from propagating and affecting other entries in batch operations.
125-182: LGTM: Unified serialization through adapter.Put operations correctly delegate to
dump_json, and bulk operations appropriately handle both TTL and non-TTL paths with proper use ofmsetvs. pipelinedsetex.key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (3)
127-183: LGTM: Adapter properly initialized and stored.The adapter is correctly instantiated with the
native_storageparameter and stored for use across all serialization operations.
217-326: LGTM: Comprehensive adapter integration with robust error handling.All get/put operations correctly delegate to the adapter, and deserialization errors are properly caught with appropriate logging for bulk operations.
68-111: Verify whether Elasticsearch adapter should be auto-generated or intentionally duplicated.The duplication is confirmed:
ElasticsearchSerializationAdapterappears identically in both sync and async store files, and the same pattern exists forMongoDBSerializationAdapter. However, the sync adapter lacks the auto-generated header present in other code_gen files, making it unclear whether this duplication is intentional per your PR design or an oversight.Per your PR objectives ("move store-specific adapters into the store implementations"), the per-store implementations may be by design. However, given that the sync code is in the
code_gendirectory andbuild_sync_library.pyexists to transform async→sync, consider whether adapters should be auto-generated from the async versions instead of maintained manually across both codebases.key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
19-26: LGTM: Type-safe dictionary validator.The
key_must_behelper provides a clean, type-safe way to validate dictionary fields with proper error messages.
102-118: LGTM: Clean serialization with configurable formats.The dump methods appropriately handle value and date format variations, with proper hook points for store-specific transformations.
121-133: LGTM: Clean identity adapter implementation.
BasicSerializationAdapterappropriately provides no-op transformations for stores like Redis that don't require special field handling.key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (2)
90-176: LGTM: Adapter properly initialized.The MongoDB adapter is correctly instantiated with the
native_storageflag and integrated into the store's initialization sequence.
227-317: LGTM: Complete adapter integration with metadata preservation.All serialization operations correctly use the adapter, and put operations appropriately include
collectionandkeyfields in stored documents for query support. Error handling ensures corrupt documents don't break batch operations.key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (2)
147-147: Adapter integration looks good.The
_adapterfield is properly declared and initialized with the store'snative_storageconfiguration.Also applies to: 212-212
265-267: LGTM: Adapter methods properly integrated.The adapter's
load_dictanddump_dictmethods are correctly used in get/put operations, with appropriate error handling for deserialization failures.Also applies to: 298-299, 329-329, 367-367
key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (6)
45-57: LGTM: Adapter initialization.The MongoDB adapter is correctly initialized with appropriate date and value formats for BSON storage.
58-69: LGTM: prepare_dump implementation.The method correctly transforms the flat value structure into MongoDB's nested document format.
83-92: LGTM: Timezone handling.The datetime fields are correctly converted to UTC timezone-aware datetimes for consistency.
103-103: Adapter integration looks good.The
_adapterfield is properly declared and initialized with the store'snative_storageconfiguration.Also applies to: 181-181
238-241: LGTM: Error handling in deserialization.The adapter's
load_dictcalls are properly wrapped with exception handling, returningNonefor corrupted or missing entries.Also applies to: 259-262
268-274: LGTM: Adapter usage in put operations.The
dump_dictresults are correctly spread into MongoDB update operations, with collection and key metadata properly added.Also applies to: 294-298
There was a problem hiding this comment.
Actionable comments posted: 2
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-shared/src/key_value/shared/utils/managed_entry.py (1)
141-144: Also remove unusedttlparameter fromfrom_json.Since the
ttlparameter is unused infrom_dict(see previous comment), it should also be removed fromfrom_jsonfor consistency.Apply this diff:
@classmethod - def from_json(cls, json_str: str, includes_metadata: bool = True, ttl: SupportsFloat | None = None) -> Self: + def from_json(cls, json_str: str, includes_metadata: bool = True) -> Self: data: dict[str, Any] = load_from_json(json_str=json_str) - return cls.from_dict(data=data, includes_metadata=includes_metadata, ttl=ttl) + return cls.from_dict(data=data, includes_metadata=includes_metadata)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (5)
key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (3)
now(19-21)seconds_to(24-26)try_parse_datetime_str(34-41)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
ttl(219-228)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
ttl(220-229)key-value/key-value-aio/src/key_value/aio/wrappers/compression/wrapper.py (1)
ttl(124-126)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/compression/wrapper.py (1)
ttl(119-121)
🪛 GitHub Actions: Run Tests
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py
[error] 88-88: ARG003 Unused class method argument: ttl
🔇 Additional comments (3)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (3)
33-37: LGTM! TTL property correctly derives from expires_at.The computed property implementation correctly derives TTL from
expires_atusingseconds_to, which aligns with the PR's objective to establish consistent TTL handling across stores.
48-54: LGTM! Convenient ISO format properties.These properties provide a clean interface for serialization adapters to access ISO-formatted timestamps, improving consistency across the codebase.
176-189: Thedict(value)conversion is necessary for type consistency.Regarding the previous review comment: the
dict(value)conversion should be kept because:
dump_to_jsonexpectsdict[str, Any]per its type signaturejson.dumpswithsort_keys=Truerequires a concrete dict for consistent behavior- Converting
Mappingtodictensures predictable size calculationThe conversion is appropriate for this size estimation utility.
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py (1)
137-139: Verify inconsistentexclude_noneparameter usage.This code passes
exclude_none=Falsetodump_json, but the aioDiskStoreat Line 110 ofstore.pydoesn't specify this parameter (defaults toTrue). This creates inconsistent serialization behavior betweenDiskStoreandMultiDiskStore.Ensure this difference is intentional and aligned with the sync version.
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
46-46: Critical: verify_dict will fail for non-dict Mappings.
self.valueis typed asMapping[str, Any], butverify_dicton Line 172 checksisinstance(obj, dict), which will raise aDeserializationErrorfor valid Mapping types likeMappingProxyTypeorOrderedDict. This is the same issue flagged in previous reviews.Apply this diff to convert to dict before verification:
@property def value_as_dict(self) -> dict[str, Any]: - return verify_dict(obj=self.value) + return verify_dict(obj=dict(self.value))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py(3 hunks)key-value/key-value-aio/src/key_value/aio/stores/disk/store.py(3 hunks)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
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(14-151)ttl(34-37)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(45-49)dump_json(116-118)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
ttl(219-228)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
ttl(220-229)
key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (2)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(14-151)ttl(34-37)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(45-49)dump_json(116-118)
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(14-151)ttl(34-37)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(45-49)dump_json(116-118)key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py (1)
_put_managed_entry(128-139)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
_put_managed_entry(104-107)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (3)
key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (4)
now(19-21)now_plus(29-31)seconds_to(24-26)try_parse_datetime_str(34-41)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
ttl(219-228)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
ttl(220-229)
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(14-151)ttl(34-37)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(45-49)dump_json(116-118)
🪛 GitHub Actions: Run Tests
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py
[error] 61-61: pyright: Argument of type 'SupportsFloat' cannot be assigned to parameter 'seconds' of type 'float' in function 'now_plus'. 'SupportsFloat' is not assignable to 'float' (reportArgumentType).
🪛 GitHub Check: static_analysis (key-value/key-value-shared)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py
[failure] 61-61:
Argument of type "SupportsFloat" cannot be assigned to parameter "seconds" of type "float" in function "now_plus"
"SupportsFloat" is not assignable to "float" (reportArgumentType)
🔇 Additional comments (8)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (2)
4-4: LGTM: Adapter-based deserialization with timezone-aware expiration.The migration to
_serialization_adapter.load_jsonand explicit timezone-awareexpires_atassignment is correct and aligns with the adapter pattern.Also applies to: 96-99
107-107: LGTM: Adapter-based serialization.The use of
_serialization_adapter.dump_jsoncorrectly centralizes serialization logic.key-value/key-value-aio/src/key_value/aio/stores/disk/store.py (2)
1-1: LGTM: Consistent with sync implementation.The async version correctly mirrors the sync implementation with adapter-based deserialization and timezone-aware expiration handling.
Also applies to: 93-96
110-110: LGTM: Adapter-based serialization.Correctly uses the serialization adapter for persistence.
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (3)
33-37: LGTM: TTL property correctly derives from expires_at.The new
ttlproperty properly computes TTL dynamically fromexpires_at, maintaining backward compatibility while removing the stored ttl field.
48-54: LGTM: ISO format properties for timestamps.The
created_at_isoformatandexpires_at_isoformatproperties provide convenient access to ISO-formatted timestamp strings.
183-196: LGTM: Efficient size estimation utility.The
estimate_serialized_sizefunction provides an efficient way to estimate JSON size without creating a fullManagedEntry, addressing wrapper performance concerns mentioned in the PR objectives.Note: The
dict(value)conversion on Line 196 was previously flagged as potentially unnecessary, but it ensures consistency with JSON serialization behavior (especially forsort_keys=True).key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py (1)
134-136: Verify inconsistentexclude_noneparameter usage.The inconsistency is confirmed:
MultiDiskStorepassesexclude_none=FalsewhileDiskStoreomits the parameter (defaults toTrue). This pattern is consistent across both sync and async implementations, but there is no documentation or test validation explaining why they differ.Since
exclude_none=Falseexplicitly includesNonevalues in serialized JSON, this could affect data integrity across collections. Verify whether this difference is intentional and required forMultiDiskStore, or if both stores should use consistent serialization behavior.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (1)
66-69: Critical: Mapping type mismatch for value.string remains unfixed.Despite being marked as addressed in a previous review, the DEFAULT_MAPPING still declares
value.stringastype: "object"withenabled: False. Whennative_storage=False, the adapter'sprepare_dumpmethod (line 115) writes a JSON string to this field, causing Elasticsearch to reject documents with amapper_parsing_exceptionbecause the field expects an object, not a string.Apply this fix to DEFAULT_MAPPING:
"string": { - "type": "object", - "enabled": False, + "type": "text", + "index": False },Update the comment above this field (lines 64-65) to reflect the change. Then run integration tests with
native_storage=Falseto verify documents index successfully.key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (1)
38-87: CRITICAL: TTL index will fail because dates are stored as ISO strings instead of datetime objects.The base
SerializationAdapter.dump_dict()method always convertscreated_atandexpires_atto ISO format strings (viaentry.created_at_isoformatandentry.expires_at_isoformat). However, MongoDB's TTL index (created at line 222) requires these fields to contain BSON datetime objects, not strings. With the current implementation, documents will never be automatically expired.Apply this diff to convert ISO strings back to datetime objects in
prepare_dump:@override def prepare_dump(self, data: dict[str, Any]) -> dict[str, Any]: + from datetime import datetime + value = data.pop("value") data["value"] = {} if self._native_storage: data["value"]["object"] = value else: data["value"]["string"] = value + + # Convert ISO format strings back to datetime objects for MongoDB TTL index + if created_at_str := data.get("created_at"): + if isinstance(created_at_str, str): + from key_value.shared.utils.time import try_parse_datetime_str + data["created_at"] = try_parse_datetime_str(value=created_at_str) + + if expires_at_str := data.get("expires_at"): + if isinstance(expires_at_str, str): + from key_value.shared.utils.time import try_parse_datetime_str + data["expires_at"] = try_parse_datetime_str(value=expires_at_str) return dataAlternatively, override
dump_dictto produce datetime objects directly:+@override +def dump_dict(self, entry: ManagedEntry, exclude_none: bool = True) -> dict[str, Any]: + """Convert a ManagedEntry to a dictionary with BSON datetime objects.""" + data: dict[str, Any] = { + "value": entry.value_as_dict if self._value_format == "dict" else entry.value_as_json, + "created_at": entry.created_at, + "expires_at": entry.expires_at, + } + + if exclude_none: + data = {k: v for k, v in data.items() if v is not None} + + return self.prepare_dump(data=data)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
183-196: Consider whetherdict(value)conversion is necessary.Line 196 converts the
Mappingtodictbefore serialization. Sincedump_to_jsoncallsjson.dumps(), which can handleMappingtypes directly, this conversion might be redundant.If the conversion is not necessary for correctness or
sort_keys=Truebehavior, consider simplifying to:- return len(dump_to_json(obj=dict(value))) + return len(dump_to_json(obj=value))However, keep the conversion if it's needed for consistency with other parts of the codebase or to ensure predictable serialization behavior. Based on learnings
56-62: Logic error: TTL of 0 will not set expiration.Line 61 uses
if ttlwhich means a TTL of 0 will be treated as falsy, resulting inexpires_at=Noneinstead of an immediately-expired entry. This breaks the semantics of zero-TTL entries.Apply this diff to always compute
expires_atfrom the TTL:@classmethod def from_ttl(cls, *, value: Mapping[str, Any], created_at: datetime | None = None, ttl: SupportsFloat) -> Self: return cls( value=value, created_at=created_at, - expires_at=(now_plus(seconds=float(ttl)) if ttl else None), + expires_at=now_plus(seconds=float(ttl)), )Note: If you need to support
Noneas a TTL value for "no expiration," change the type signature tottl: SupportsFloat | Noneand keep the conditional, but check explicitly forNonerather than relying on truthiness:expires_at=(now_plus(seconds=float(ttl)) if ttl is not None else None).key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py (1)
7-7: Fix incorrect import ofdatetime.Line 7 imports
datetimefromkey_value.shared.utils.managed_entry, but that module importsdatetimefrom the standard library and does not re-export it. This import will fail or behave unexpectedly.Apply this diff to import
datetimedirectly from the standard library:-from key_value.shared.utils.managed_entry import ManagedEntry, datetime +from datetime import datetime + +from key_value.shared.utils.managed_entry import ManagedEntry
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py(3 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/mongodb/store.py(8 hunks)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_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/mongodb/store.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
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 (1)
DeserializationError(14-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(14-151)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (7)
SerializationAdapter(28-118)prepare_dump(97-100)prepare_dump(132-133)prepare_load(52-55)prepare_load(129-130)load_dict(57-94)dump_dict(102-114)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (3)
MongoDBSerializationAdapter(45-94)prepare_dump(59-69)prepare_load(72-94)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py (2)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(14-151)ttl(34-37)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(45-49)dump_json(116-118)
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(14-151)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (7)
SerializationAdapter(28-118)prepare_dump(97-100)prepare_dump(132-133)prepare_load(52-55)prepare_load(129-130)load_dict(57-94)dump_dict(102-114)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/src/key_value/aio/stores/disk/multi_store.py (3)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
ManagedEntry(14-151)ttl(34-37)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(45-49)dump_json(116-118)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
ttl(219-228)
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/utils/managed_entry.py (1)
ManagedEntry(14-151)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (7)
SerializationAdapter(28-118)prepare_dump(97-100)prepare_dump(132-133)prepare_load(52-55)prepare_load(129-130)load_dict(57-94)dump_dict(102-114)key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (4)
MongoDBSerializationAdapter(38-87)prepare_dump(52-62)prepare_load(65-87)_put_managed_entry(260-281)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.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/time_to_live.py (4)
now(19-21)now_plus(29-31)seconds_to(24-26)try_parse_datetime_str(34-41)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
ttl(219-228)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
ttl(220-229)
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(14-151)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (7)
SerializationAdapter(28-118)prepare_dump(97-100)prepare_dump(132-133)prepare_load(52-55)prepare_load(129-130)load_dict(57-94)dump_dict(102-114)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)
🔇 Additional comments (19)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py (3)
126-127: Timezone fix correctly applied.The previous review concern about naive datetime has been resolved. The code now correctly uses
tz=timezone.utcwhen creating the datetime from the timestamp, ensuring timezone-aware datetimes throughout.
135-137: Correct adapter usage and TTL handling.The serialization via
dump_jsonwithexclude_none=Falseensures complete metadata persistence, and usingmanaged_entry.ttl(computed fromexpires_at) correctly provides the diskcache expiration value.
124-124: No issues found. The_serialization_adapteris properly initialized in the base class.The verification confirms that
self._serialization_adapteris initialized inkey-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py:103asBasicSerializationAdapter(). The disk store classes inherit this attribute from the base class and use it correctly. The adapter type isBasicSerializationAdapter, which is the standard serialization adapter for these stores.key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py (3)
88-131: LGTM: Adapter correctly implements dual storage modes.The
ElasticsearchSerializationAdapterproperly extends theSerializationAdapterbase class and supports both native (flattened) and string storage modes. Theprepare_loadmethod correctly uses key presence checks ("flattened" in value) instead of truthiness checks, which prevents false negatives when deserializing empty dicts or empty strings—addressing a previously flagged issue.
147-148: LGTM: Adapter properly integrated into store.The
_adapterfield is correctly declared and initialized with theElasticsearchSerializationAdapterinstance, passing through thenative_storageconfiguration. This follows good dependency injection practices.Also applies to: 212-212
265-265: LGTM: Adapter usage is consistent and type-safe.The store correctly delegates serialization to
self._adapter.dump_dict()(lines 329, 367) and deserialization toself._adapter.load_dict()(lines 265, 298). The refactor from the previousto_storage()/from_storage()methods todump_dict()/load_dict()resolves prior type checking issues, asdump_dict()returns a concretedict[str, Any]rather than a union type. The pattern is consistently applied across both single-document and batch operations.Also applies to: 298-298, 329-329, 367-367
key-value/key-value-aio/src/key_value/aio/stores/mongodb/store.py (3)
96-96: LGTM: Adapter initialization is clean.The adapter is properly initialized with the
native_storageparameter and stored as an instance field. While injection would increase testability, the current approach is sufficient for the store's needs.Also applies to: 174-174
231-234: LGTM: Robust error handling for corrupted documents.Catching
DeserializationErrorand returningNoneprovides graceful degradation when individual documents are corrupted. This prevents a single bad document from failing entire queries.Also applies to: 252-255
267-281: LGTM: Adapter integration in put operations is clean.The adapter's
dump_dict()output is correctly spread alongside collection and key metadata. The update pattern with$setandupsert=Trueis appropriate for both single and bulk operations.Also applies to: 301-315
key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (7)
12-15: LGTM! Imports correctly updated for adapter pattern.The imports have been properly refactored to support the new serialization adapter approach. The previous circular import concern with
try_parse_datetime_strhas been resolved by delegating datetime parsing to the base adapter class.
68-111: Well-designed adapter implementation with backward compatibility.The
ElasticsearchSerializationAdaptercorrectly implements the serialization pattern for Elasticsearch's dual storage modes. The implementation is particularly robust becauseprepare_load(lines 100-111) checks for both "flattened" and "string" keys, enabling seamless handling of documents stored in either mode—valuable for migrations or mixed storage scenarios.The nested structure created by
prepare_dump(lines 87-97) correctly matches theDEFAULT_MAPPINGschema defined at lines 48-56.
181-181: LGTM! Adapter correctly initialized.The adapter is properly initialized with the
native_storageflag, ensuring consistent serialization behavior throughout the store's lifecycle.
228-231: LGTM! Proper adapter usage with appropriate error handling.The deserialization correctly delegates to
self._adapter.load_dict()and handles failures gracefully by returningNone, which is appropriate behavior for a get operation.
261-269: LGTM! Robust batch deserialization with enhanced error handling.The batch operation properly uses the adapter for deserialization and includes informative error logging (lines 264-268) while gracefully handling failures. The logging includes useful context (collection, document_id, error message) for debugging corrupted documents.
283-289: LGTM! Proper adapter usage for serialization.The serialization correctly delegates to
self._adapter.dump_dict()and appropriately wraps Elasticsearch's serialization errors in the store'sSerializationErrortype for consistent error handling across stores.
311-326: LGTM! Consistent adapter usage in bulk operations.The bulk put operation correctly serializes each entry individually using the adapter (line 316) and builds the bulk operations list properly. Error handling is consistent with the single-put operation, converting Elasticsearch serialization errors to store-level
SerializationErrorexceptions.key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py (1)
123-124: Timezone handling correctly implemented.The code now properly creates timezone-aware datetimes using
tz=timezone.utc, which addresses the previous review concern about naive datetimes.key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
33-37: TTL property correctly implemented.The conversion of
ttlfrom a stored field to a computed property that derives its value fromexpires_atis correct and aligns with the PR's goal of consistent TTL handling.
172-180: Good fix:verify_dictnow handles all Mapping types.The change from checking
isinstance(obj, dict)toisinstance(obj, Mapping)and converting todictproperly addresses the previous review concern. This ensures thatself.value(typed asMapping[str, Any]) can be validated without errors, including non-dict Mapping implementations likeMappingProxyType.
| # DO NOT CHANGE! Change the original file instead. | ||
| import time | ||
| from collections.abc import Callable | ||
| from datetime import timezone |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider importing datetime directly from the datetime module for consistency.
The code imports timezone directly from the datetime module (line 5) but imports datetime from managed_entry (line 10). While this works, importing both from the same source improves clarity.
Apply this diff to make imports consistent:
-from datetime import timezone
+from datetime import datetime, timezone
from pathlib import Path
from typing import overload
from key_value.shared.utils.compound import compound_key
-from key_value.shared.utils.managed_entry import ManagedEntry, datetime
+from key_value.shared.utils.managed_entry import ManagedEntryAlso applies to: 10-10
🤖 Prompt for AI Agents
In
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py
around lines 5 and 10, the imports are inconsistent: timezone is imported from
the datetime module (line 5) while datetime is imported from managed_entry (line
10); update imports to import both datetime and timezone from the datetime
module (e.g., from datetime import datetime, timezone) and remove the datetime
import from managed_entry so all datetime objects come from the same source,
ensuring any references to managed_entry.datetime are updated to use the
standard datetime import.
|
|
||
| _index_prefix: str | ||
|
|
||
| _native_storage: bool |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider whether store-level _native_storage is needed.
The _native_storage field is set in __init__ (line 179) but appears to only be used to initialize the adapter (line 181). Since the adapter maintains its own _native_storage field, this store-level field may be redundant.
However, keeping it could be intentional for API consistency or future use, and removing it would be a breaking change to the constructor signature.
🤖 Prompt for AI Agents
key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py
lines 125 (and refer to __init__ around 179-181): the store-level
_native_storage attribute appears redundant because it's only assigned in
__init__ and immediately passed to the adapter which already tracks its own
_native_storage; either remove the store-level field and constructor parameter
and reference adapter._native_storage everywhere the store would need it, or
keep the field but document its purpose and use it explicitly (or forward it
consistently) to avoid redundancy — if removing, update the constructor
signature and call sites (noting this is a breaking change), and if keeping, add
a brief comment explaining API consistency/future use and ensure it is actually
referenced beyond initializing the adapter.
| def prepare_load(self, data: dict[str, Any]) -> dict[str, Any]: | ||
| value = data.pop("value") | ||
|
|
||
| # Store in appropriate field based on mode | ||
| if native_storage: | ||
| document["value"]["object"] = managed_entry.value_as_dict | ||
| else: | ||
| document["value"]["string"] = json_str | ||
| if "object" in value: | ||
| data["value"] = value["object"] | ||
| elif "string" in value: | ||
| data["value"] = value["string"] | ||
| else: | ||
| msg = "Value field not found in MongoDB document" | ||
| raise DeserializationError(message=msg) | ||
|
|
||
| # Add metadata fields | ||
| if managed_entry.created_at: | ||
| document["created_at"] = managed_entry.created_at | ||
| if managed_entry.expires_at: | ||
| document["expires_at"] = managed_entry.expires_at | ||
| if date_created := data.get("created_at"): | ||
| if not isinstance(date_created, datetime): | ||
| msg = "Expected `created_at` field to be a datetime" | ||
| raise DeserializationError(message=msg) | ||
| data["created_at"] = date_created.replace(tzinfo=timezone.utc) | ||
| if date_expires := data.get("expires_at"): | ||
| if not isinstance(date_expires, datetime): | ||
| msg = "Expected `expires_at` field to be a datetime" | ||
| raise DeserializationError(message=msg) | ||
| data["expires_at"] = date_expires.replace(tzinfo=timezone.utc) |
There was a problem hiding this comment.
Restore backwards compatibility for legacy documents.
Before this refactor, MongoDBStore wrote documents with value stored directly (dict in native mode, JSON string otherwise) and timestamps serialized via ManagedEntry.to_dict() (ISO strings). The new adapter only accepts the nested {"value": {"object"| "string"}} shape and requires datetime instances, so every legacy record triggers DeserializationError and is dropped, effectively hiding all existing data. Please add a fallback path in prepare_load() that recognizes the old shapes (plain dict/str payloads and ISO timestamp strings) and normalizes them into the new canonical form before delegating to load_dict(), or provide a migration that rewrites stored documents. Example adjustment:
@@
- value = data.pop("value")
-
- if "object" in value:
- data["value"] = value["object"]
- elif "string" in value:
- data["value"] = value["string"]
+ value = data.pop("value")
+
+ if "object" in value:
+ data["value"] = value["object"]
+ elif "string" in value:
+ data["value"] = value["string"]
+ elif self._native_storage and isinstance(value, dict):
+ data["value"] = value # legacy native document layout
+ elif not self._native_storage and isinstance(value, str):
+ data["value"] = value # legacy stringified layout
else:
msg = "Value field not found in MongoDB document"
raise DeserializationError(message=msg)
- if date_created := data.get("created_at"):
- if not isinstance(date_created, datetime):
- msg = "Expected `created_at` field to be a datetime"
- raise DeserializationError(message=msg)
- data["created_at"] = date_created.replace(tzinfo=timezone.utc)
- if date_expires := data.get("expires_at"):
- if not isinstance(date_expires, datetime):
- msg = "Expected `expires_at` field to be a datetime"
- raise DeserializationError(message=msg)
- data["expires_at"] = date_expires.replace(tzinfo=timezone.utc)
+ if date_created := data.get("created_at"):
+ if isinstance(date_created, str):
+ parsed = try_parse_datetime_str(date_created)
+ if parsed is None:
+ msg = "Unable to parse `created_at` legacy ISO string"
+ raise DeserializationError(message=msg)
+ date_created = parsed
+ if not isinstance(date_created, datetime):
+ msg = "Expected `created_at` field to be a datetime"
+ raise DeserializationError(message=msg)
+ data["created_at"] = date_created.replace(tzinfo=timezone.utc)
+ if date_expires := data.get("expires_at"):
+ if isinstance(date_expires, str):
+ parsed = try_parse_datetime_str(date_expires)
+ if parsed is None:
+ msg = "Unable to parse `expires_at` legacy ISO string"
+ raise DeserializationError(message=msg)
+ date_expires = parsed
+ if not isinstance(date_expires, datetime):
+ msg = "Expected `expires_at` field to be a datetime"
+ raise DeserializationError(message=msg)
+ data["expires_at"] = date_expires.replace(tzinfo=timezone.utc)(Import try_parse_datetime_str from key_value.shared.utils.datetime.)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py
around lines 72-92, prepare_load currently only accepts the new nested {"value":
{"object"| "string"}} shape and datetime instances causing legacy records to
raise DeserializationError; update prepare_load to detect legacy shapes: if the
popped value is a dict treat it as the old plain-object payload and set
data["value"] = {"object": that_dict}, if it's a str set data["value"] =
{"string": that_str}; for created_at and expires_at, if the fields are strings
attempt to parse them with try_parse_datetime_str (import from
key_value.shared.utils.datetime) and then replace tzinfo=timezone.utc on the
parsed datetime, otherwise if parsing fails raise DeserializationError; keep
existing checks for datetime instances and normalize all legacy forms into the
canonical new form before returning so load_dict() can proceed as before.
📝 WalkthroughPre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used🧬 Code graph analysis (5)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (2)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (6)
⏰ 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)
🔇 Additional comments (4)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
key-value/key-value-aio/src/key_value/aio/stores/dynamodb/store.py (1)
77-115: Critical: Missing serialization adapter initialization.The
__init__method does not initializeself._serialization_adapter, but lines 202 and 213 reference it. This will cause anAttributeErrorat runtime when_get_managed_entryor_put_managed_entryis called.Based on the PR's adapter pattern and DynamoDB's full-JSON storage approach, add the missing import and initialization:
+from key_value.shared.utils.serialization import BasicSerializationAdapter + from key_value.aio.stores.base import ( BaseContextManagerStore, BaseStore, )super().__init__(default_collection=default_collection) + self._serialization_adapter = BasicSerializationAdapter()key-value/key-value-aio/src/key_value/aio/stores/simple/store.py (1)
36-43: Remove unused method.The
to_managed_entry()method is no longer called—deserialization now uses the adapter (line 78). This method has become dead code.Apply this diff to remove the unused method:
- 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 - -key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
108-110: Inconsistent adapter usage for deserialization.Line 109 still uses
ManagedEntry.from_json()directly, while the serialization paths (lines 118, 141) correctly useself._serialization_adapter.dump_json(). This inconsistency defeats the purpose of the adapter pattern refactoring. The pipeline failure (reportUnknownVariableType) is likely caused by this direct method call.Apply this diff to use the adapter consistently:
- managed_entry_str: str = value.decode("utf-8") - managed_entry: ManagedEntry = ManagedEntry.from_json(json_str=managed_entry_str) - - return managed_entry + json_str: str = value.decode("utf-8") + return self._serialization_adapter.load_json(json_str=json_str)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
100-100: Critical inconsistency: deserialization not using adapter.Line 100 still uses
ManagedEntry.from_json()directly, while line 115 in_get_managed_entriescorrectly usesself._serialization_adapter.load_json(). This inconsistency within the same store means single-entry and batch operations use different serialization logic, violating the adapter pattern's goal of centralizing serialization behavior.Apply this diff to use the adapter consistently:
- decoded_response: str = response.decode("utf-8") - - return ManagedEntry.from_json(json_str=decoded_response) + json_str: str = response.decode("utf-8") + return self._serialization_adapter.load_json(json_str=json_str)key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
111-113: Inconsistent adapter usage for deserialization.Line 112 still uses
ManagedEntry.from_json()directly, while the serialization paths (lines 127, 150) correctly useself._serialization_adapter.dump_json(). This inconsistency defeats the purpose of the adapter pattern refactoring. The pipeline failure (reportUnknownVariableType) is likely caused by this direct method call.Apply this diff to use the adapter consistently:
- managed_entry_str: str = value.decode("utf-8") - managed_entry: ManagedEntry = ManagedEntry.from_json(json_str=managed_entry_str) - - return managed_entry + json_str: str = value.decode("utf-8") + return self._serialization_adapter.load_json(json_str=json_str)key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
88-98: Inconsistent serialization:_get_managed_entrybypasses the adapter.Line 98 uses
ManagedEntry.from_json(json_str=decoded_response)directly, while_get_managed_entries(line 113) correctly usesself._serialization_adapter.load_json(). This inconsistency defeats the purpose of the adapter pattern refactoring.Apply this diff to use the adapter consistently:
- return ManagedEntry.from_json(json_str=decoded_response) + return self._serialization_adapter.load_json(json_str=decoded_response)
♻️ Duplicate comments (2)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
98-111: Consider removing redundant dict(value) conversion.Passing Mapping directly to dump_to_json may be sufficient; converting to dict might be unnecessary overhead. If other parts rely on a concrete dict, keep it and add a brief comment.
- return len(dump_to_json(obj=dict(value))) + return len(dump_to_json(obj=value)) # value is a Mapping; dump_to_json will serialize itkey-value/key-value-shared/src/key_value/shared/utils/serialization.py (1)
76-89: Don’t treat empty dict values as missing. Use explicit key presence check.- if not (value := data.get("value")): - msg = "Value field not found" - raise DeserializationError(message=msg) + if "value" not in data: + msg = "Value field not found" + raise DeserializationError(message=msg) + + value = data["value"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (32)
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/dynamodb/store.py(2 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(5 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/rocksdb/store.py(2 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(2 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/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/memcached/test_memcached.py(2 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(2 hunks)key-value/key-value-aio/tests/stores/wrappers/test_limit_size.py(2 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-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/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/rocksdb/store.py(2 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(2 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/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/rocksdb/test_rocksdb.py(2 hunks)key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py(2 hunks)key-value/key-value-sync/tests/code_gen/stores/wrappers/test_limit_size.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (32)
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 (3)
rocksdb_client(71-72)store(23-29)test_value_stored(74-89)key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
RocksDBStore(20-194)
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(45-49)dump_json(116-118)key-value/key-value-sync/src/key_value/sync/code_gen/stores/vault/store.py (1)
_put_managed_entry(105-113)
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(45-49)dump_json(116-118)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/keyring/store.py (1)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(45-49)dump_json(116-118)
key-value/key-value-aio/src/key_value/aio/stores/disk/multi_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 (3)
BasicSerializationAdapter(121-133)load_json(45-49)dump_json(116-118)
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/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-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)
valkey_client(88-89)store(76-85)test_value_stored(95-110)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (1)
dump_json(116-118)
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)
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 (3)
put(275-284)get(186-207)ttl(220-229)key-value/key-value-aio/tests/stores/rocksdb/test_rocksdb.py (3)
rocksdb_client(68-69)store(20-26)test_value_stored(71-86)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
RocksDBStore(23-185)
key-value/key-value-aio/tests/stores/disk/test_disk.py (1)
key-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py (4)
disk_cache(34-35)disk_store(22-24)test_value_stored(37-50)store(28-31)
key-value/key-value-aio/tests/stores/memcached/test_memcached.py (2)
key-value/key-value-aio/tests/stores/disk/test_disk.py (2)
store(25-28)test_value_stored(34-47)key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
MemcachedStore(21-133)
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (1)
dump_json(116-118)
key-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py (2)
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/src/key_value/sync/code_gen/stores/memory/store.py (2)
put(79-81)get(69-77)
key-value/key-value-sync/tests/code_gen/stores/disk/test_multi_disk.py (2)
key-value/key-value-aio/tests/stores/disk/test_multi_disk.py (1)
test_value_stored(35-54)key-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py (3)
test_value_stored(37-50)store(28-31)disk_cache(34-35)
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 (3)
BasicSerializationAdapter(121-133)load_json(45-49)dump_json(116-118)
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(136-148)load_json(45-49)dump_json(116-118)key-value/key-value-aio/src/key_value/aio/stores/simple/store.py (1)
SimpleStoreEntry(23-43)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (6)
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/src/key_value/shared/utils/time_to_live.py (1)
try_parse_datetime_str(34-41)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-aio/src/key_value/aio/stores/mongodb/store.py (2)
prepare_load(65-87)prepare_dump(52-62)key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (1)
get(68-76)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.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/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-sync/src/key_value/sync/code_gen/stores/base.py (1)
ttl(220-229)
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/base.py (4)
BaseStore(59-348)put(275-284)get(186-207)ttl(220-229)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
ValkeyStore(25-151)
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(45-49)dump_json(116-118)
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(121-133)load_json(45-49)dump_json(116-118)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-sync/src/key_value/sync/code_gen/stores/disk/store.py (2)
_put_managed_entry(104-107)_delete_managed_entry(110-113)
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(45-49)dump_json(116-118)
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(45-49)dump_json(116-118)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/memory/store.py (2)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
BasicSerializationAdapter(121-133)load_json(45-49)dump_json(116-118)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/valkey/store.py (1)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(45-49)dump_json(116-118)
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(45-49)dump_json(116-118)
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(136-148)load_json(45-49)dump_json(116-118)key-value/key-value-sync/src/key_value/sync/code_gen/stores/simple/store.py (1)
SimpleStoreEntry(21-39)
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/windows_registry/store.py (3)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
load_json(45-49)dump_json(116-118)key-value/key-value-aio/src/key_value/aio/stores/windows_registry/store.py (3)
_put_managed_entry(94-100)_sanitize_key(65-70)_get_registry_path(72-75)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/memory/store.py (3)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
BasicSerializationAdapter(121-133)load_json(45-49)dump_json(116-118)key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (3)
MemoryCacheEntry(26-31)get(68-76)put(78-80)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)
🪛 GitHub Actions: Run Tests
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py
[error] 109-109: pyright: Type of 'managed_entry' is unknown (reportUnknownVariableType).
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py
[error] 112-112: Step 'uv run basedpyright .' failed. Pyright error: Type of "managed_entry" is unknown (reportUnknownVariableType).
key-value/key-value-shared/src/key_value/shared/utils/serialization.py
[error] 148-148: pyright: Type 'Any | None' is not assignable to return type 'dict[str, Any]'. (reportReturnType)
🪛 GitHub Check: static_analysis (key-value/key-value-shared)
key-value/key-value-shared/src/key_value/shared/utils/serialization.py
[failure] 148-148:
Type "Any | None" is not assignable to return type "dict[str, Any]"
Type "Any | None" is not assignable to type "dict[str, Any]"
"None" is not assignable to "dict[str, Any]" (reportReturnType)
🔇 Additional comments (54)
key-value/key-value-aio/tests/stores/rocksdb/test_rocksdb.py (3)
1-1: LGTM: Imports support low-level serialization validation.The new imports are appropriately scoped to the test's objective: validating the raw serialization format using snapshot testing with flexible datetime matching.
Also applies to: 7-9
67-69: Appropriate fixture for adapter format validation.Exposing the internal
_dbclient allows the test to validate the raw serialization format introduced by the adapter pattern. Thepyright: ignorecomment correctly acknowledges intentional private access for testing purposes.
71-86: LGTM: Effective validation of adapter serialization format.The test appropriately validates the new adapter-based serialization format for both TTL and non-TTL cases. The structure mirrors the sync version for consistency, and the use of snapshot testing with
IsDatetimeprovides flexible yet rigorous format validation.key-value/key-value-aio/tests/stores/disk/test_multi_disk.py (2)
1-1: LGTM: Imports are appropriate and well-organized.The new imports support the serialization format testing introduced in this PR. The
TYPE_CHECKINGguard for theCachetype import is a good practice to avoid runtime import overhead.Also applies to: 5-5, 8-9, 15-17
35-54: Excellent test for validating serialization format.This test appropriately validates the raw JSON structure stored by the
BasicSerializationAdapterused inMultiDiskStore. Key strengths:
- Tests both scenarios (with and without TTL) to ensure
expires_atis correctly included when needed.- Uses snapshot testing to catch unintended serialization format changes, which is valuable given this PR's focus on refactoring serialization.
- Direct access to the underlying disk cache (line 37) is intentional and appropriate here—it allows verification of the actual persisted format rather than just the public API behavior.
Note: The dictionary key ordering in the second snapshot (line 53:
"age"before"name") is consistent with the sync version and will be caught by snapshot updates if the serialization behavior changes.key-value/key-value-aio/src/key_value/aio/stores/simple/store.py (4)
1-1: LGTM! Imports support the adapter pattern.The
sysimport is used forsys.maxsizeandValueOnlySerializationAdapterimplements the new serialization strategy.Also applies to: 8-8
53-63: Verify the handling ofmax_entries=0.Line 63 uses
max_entries or sys.maxsize, which treats0as falsy and converts it tosys.maxsize(unlimited). Confirm this is the intended behavior, as passing0might reasonably mean "no entries allowed" rather than "unlimited entries."Consider using explicit None-checking for clarity:
- self.max_entries = max_entries or sys.maxsize + self.max_entries = max_entries if max_entries is not None else sys.maxsize
78-81: LGTM! Adapter-based deserialization with separate metadata handling.The pattern correctly uses
ValueOnlySerializationAdapterto deserialize the value, then manually restores metadata from the store entry. This separation is by design since the adapter only handles the value field.
91-93: LGTM! Adapter-based serialization with metadata preservation.The code correctly uses the adapter to serialize the value while preserving metadata fields in the store entry.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/vault/store.py (3)
108-108: Serialization adapter usage is correct.The call to
self._serialization_adapter.dump_json(entry=managed_entry)correctly uses the new adapter pattern. The storage pattern (full JSON string in Vault'svaluefield) aligns withFullJsonAdapterbehavior mentioned in the PR summary.
1-3: Confirm changes were made to the original async source file.This file is auto-generated from an async source file. Ensure that the corresponding changes were properly made to the original async
store.pyfile in the aio directory, and that this sync version was regenerated usingbuild_sync_library.py.Run the following script to verify the original async file has matching changes:
102-102: Verify adapter initialization and type in BaseStore.The code now delegates to
self._serialization_adapter.load_json(), but_serialization_adapteris not initialized in this file. Ensure that:
BaseStoreproperly initializes_serialization_adapterbefore any store operations- VaultStore uses the correct adapter (likely
FullJsonAdaptersince it stores full JSON strings in thevaluefield)Run the following script to verify the adapter initialization:
key-value/key-value-aio/tests/stores/wrappers/test_limit_size.py (1)
3-3: LGTM: Efficient size estimation without ManagedEntry overhead.The change to use
estimate_serialized_sizeavoids the overhead of creating a fullManagedEntryobject just to calculate size. This aligns with the PR's objective to improve wrapper efficiency.Also applies to: 142-142
key-value/key-value-aio/tests/stores/disk/test_disk.py (2)
1-1: LGTM: Appropriate test dependencies added.The imports support the new test that validates internal storage format using snapshot assertions.
Also applies to: 6-8
30-47: LGTM: Well-structured test validates adapter serialization.The test appropriately validates that the serialization adapter correctly persists
created_at,value, andexpires_atfields to the underlying disk cache. Using snapshot assertions withIsDatetimeis a good approach for validating the structure while allowing timestamp flexibility.key-value/key-value-aio/tests/stores/dynamodb/test_dynamodb.py (4)
2-2: LGTM: Required imports added for DynamoDB testing.The imports support the new test infrastructure for validating DynamoDB storage format.
Also applies to: 4-4, 7-8, 10-11
57-59: LGTM: Helper function extracts value from DynamoDB response.The helper appropriately navigates the DynamoDB response structure to extract the stored JSON value.
111-113: LGTM: Fixture exposes DynamoDB client for testing.The fixture appropriately exposes the internal DynamoDB client to enable direct storage validation in tests.
119-136: LGTM: Test validates adapter serialization to DynamoDB.The test properly validates that the serialization adapter correctly persists entries to DynamoDB, including metadata fields. The snapshot-based assertions with
IsDatetimeare appropriate for validating structure while handling timestamp variability.key-value/key-value-aio/src/key_value/aio/stores/disk/multi_store.py (5)
2-2: LGTM: Required imports added for adapter pattern.The imports provide timezone support and the serialization adapter needed for the refactored implementation.
Also applies to: 6-7
103-103: LGTM: Serialization adapter properly initialized.The initialization of
BasicSerializationAdapteraddresses the previous critical issue where_serialization_adapterwas referenced but never initialized.
115-125: LGTM: Adapter-based deserialization with timezone-aware datetime.The method correctly uses the serialization adapter for deserialization and properly creates timezone-aware
expires_atusingtz=timezone.utc, addressing the previous issue with naive datetimes.
127-135: LGTM: Adapter-based serialization.The method correctly uses the serialization adapter for serialization, consistent with the refactored pattern.
137-139: LGTM: Simplified deletion logic.The deletion now correctly uses the key directly instead of compound_key, consistent with the updated storage approach.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/simple/store.py (3)
63-63: LGTM: Serialization adapter initialized.The
ValueOnlySerializationAdapteris appropriate for SimpleStore, which stores only the value without metadata in the JSON.
74-77: LGTM: Adapter deserialization with metadata restoration.The deserialization correctly uses the adapter for the JSON value and then restores
expires_atandcreated_atfrom theSimpleStoreEntry, which stores metadata separately.
86-90: LGTM: Adapter serialization with metadata preservation.The serialization correctly uses the adapter to dump only the value to JSON while preserving
expires_atandcreated_atin theSimpleStoreEntryseparately.key-value/key-value-sync/tests/code_gen/stores/wrappers/test_limit_size.py (1)
6-6: LGTM: Efficient size estimation (code-generated).The code-generated change correctly mirrors the async version, using
estimate_serialized_sizeto avoid unnecessaryManagedEntryinstantiation.Also applies to: 145-145
key-value/key-value-aio/src/key_value/aio/stores/vault/store.py (2)
105-105: LGTM: Adapter-based deserialization.The deserialization correctly uses
self._serialization_adapter.load_jsoninstead of the deprecatedManagedEntry.from_json, consistent with the refactored adapter pattern.
111-111: LGTM: Adapter-based serialization.The serialization correctly uses
self._serialization_adapter.dump_jsoninstead ofmanaged_entry.to_json(), consistent with the refactored adapter pattern.key-value/key-value-aio/src/key_value/aio/stores/windows_registry/store.py (2)
91-91: LGTM: Adapter-based deserialization.The deserialization correctly uses
self._serialization_adapter.load_jsoninstead of the deprecatedManagedEntry.from_json, consistent with the refactored adapter pattern.
98-98: LGTM: Adapter-based serialization.The serialization correctly uses
self._serialization_adapter.dump_jsoninstead ofmanaged_entry.to_json(), consistent with the refactored adapter pattern.key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (2)
118-118: LGTM: Serialization correctly uses adapter.The serialization path correctly delegates to the adapter, consistent with the PR's refactoring goals.
141-141: LGTM: Batch serialization correctly uses adapter.The batch serialization path correctly delegates to the adapter, maintaining consistency with single-entry operations.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/windows_registry/store.py (1)
92-92: LGTM: Consistent adapter usage for both serialization directions.Both deserialization (line 92) and serialization (line 99) correctly delegate to the adapter, providing a consistent interface as intended by the refactoring.
Also applies to: 99-99
key-value/key-value-sync/tests/code_gen/stores/rocksdb/test_rocksdb.py (1)
70-89: LGTM: Test validates stored JSON format through direct client access.The test appropriately verifies the serialized format by accessing the underlying RocksDB client. Using
store._dbwith a pyright ignore directive is acceptable in test context for validating storage format, including metadata fields (created_at, expires_at). The snapshot assertions ensure the stored structure remains consistent.key-value/key-value-aio/tests/stores/memcached/test_memcached.py (1)
71-94: LGTM: Test validates stored JSON format with TTL handling.The test follows the same pattern as other store tests, appropriately accessing
store._client(with pyright ignore) to verify the serialized format. The test validates both non-TTL and TTL scenarios, ensuring metadata fields are correctly included in the stored JSON.key-value/key-value-aio/src/key_value/aio/stores/keyring/store.py (1)
82-82: LGTM: Consistent adapter usage for keyring serialization.Both deserialization (line 82) and serialization (line 91) correctly use the adapter, ensuring consistent behavior across the keyring store.
Also applies to: 91-91
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
115-115: Note: Partial adapter adoption creates inconsistency.While these lines correctly use the adapter, line 100 in
_get_managed_entrystill uses the oldManagedEntry.from_json()method. This inconsistency should be resolved to ensure uniform serialization behavior across all operations.Also applies to: 125-125
key-value/key-value-aio/src/key_value/aio/stores/rocksdb/store.py (1)
127-127: LGTM: Serialization paths correctly use adapter.Both single-entry and batch serialization correctly delegate to the adapter. However, note that the deserialization path (line 112) still needs to be updated for consistency.
Also applies to: 150-150
key-value/key-value-aio/tests/stores/valkey/test_valkey.py (1)
89-112: LGTM: Test validates stored format via direct Valkey client access.The test follows the established pattern of accessing the underlying client (
store._connected_clientwith pyright ignore) to validate serialized format. The test appropriately verifies both non-TTL and TTL scenarios with snapshot assertions.key-value/key-value-sync/tests/code_gen/stores/disk/test_disk.py (1)
33-50: LGTM! Test validates serialized format correctly.The test properly verifies that:
- Stored values include
created_attimestamp- Values with TTL include
expires_attimestamp- The structure matches expected JSON format via snapshots
The use of
IsDatetimefor dynamic timestamp fields is appropriate.key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py (1)
71-71: Serialization updated correctly.The migration from
ManagedEntry.from_json()toself._serialization_adapter.load_json()is correct and consistent with the adapter pattern.key-value/key-value-sync/tests/code_gen/stores/disk/test_multi_disk.py (1)
38-52: LGTM! Test validates multi-disk serialization correctly.The test properly verifies the JSON structure stored in the disk cache for
MultiDiskStore, consistent with the singleDiskStoretest pattern.key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py (3)
106-108: Correct initialization order.The adapter is properly initialized before calling
super().__init__(), ensuring any parent class setup that might depend on the adapter will work correctly.
118-126: Serialization adapter integration looks good.The code correctly:
- Uses
adapter.load_json()for deserialization- Populates
expires_atwith timezone-aware datetime (UTC) fromexpire_epoch- Addresses the previous review comment about timezone awareness
132-132: Adapter usage is correct.The serialization path properly delegates to
self._serialization_adapter.dump_json().key-value/key-value-sync/tests/code_gen/stores/valkey/test_valkey.py (1)
87-110: LGTM! Test validates Valkey serialization format.The test correctly:
- Exposes the underlying Valkey client for direct storage inspection
- Validates the JSON structure includes
created_atandvalue- Verifies TTL-based entries include
expires_at- Uses snapshot testing appropriately
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (3)
54-66: Verify the default behavior change is intentional.The
max_entriesparameter default changed from10000toNone(unlimited). This is a breaking change in behavior - previously collections were bounded by default, now they're unbounded.Confirm this change aligns with the PR objectives. If bounded collections are desired by default, consider:
- def __init__(self, max_entries: int | None = None): + def __init__(self, max_entries: int | None = 10000):
66-80: Adapter initialization and usage is correct.The adapter is properly initialized in
__init__()and used consistently inget()andput()methods.
100-122: Correct initialization order; verify default change.The adapter is properly initialized before
super().__init__()(line 122), ensuring correct initialization order. However, the default formax_entries_per_collectionchanged from10000toNone(unlimited), which is a behavior change.Confirm this default change is intentional and document it in the PR description if it's a breaking change.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/keyring/store.py (1)
72-72: Verify that_serialization_adapteris initialized before use.The code uses
self._serialization_adapter.load_json()but the adapter initialization is not visible in this file. Ensure that eitherBaseStore.__init__()orKeyringStore.__init__()initializes_serialization_adapterbefore any_get_managed_entrycalls.Run this script to verify the adapter is properly initialized:
key-value/key-value-aio/src/key_value/aio/stores/valkey/store.py (1)
113-113: Verify that_serialization_adapteris initialized.The adapter is used here but its initialization is not visible in the provided code. Ensure
BaseStoreorValkeyStore.__init__()properly initializes the adapter.key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (1)
75-82: LGTM on adapter-based (de)serialization path.Using BasicSerializationAdapter.load_json/dump_json here keeps MemoryStore consistent with other backends. No issues spotted.
| super().__init__(default_collection=default_collection) | ||
|
|
||
| self._client = client or Client(host=host, port=port) | ||
|
|
||
| super().__init__(default_collection=default_collection) | ||
| self._serialization_adapter = BasicSerializationAdapter(value_format="dict") |
There was a problem hiding this comment.
Critical initialization order issue.
The code calls super().__init__(default_collection=default_collection) on line 48 before initializing self._serialization_adapter on line 52. If the parent class initialization triggers any methods that depend on the adapter (e.g., seed data processing, setup hooks), this will cause an AttributeError.
Apply this diff to initialize the adapter before calling the parent constructor:
- super().__init__(default_collection=default_collection)
-
self._client = client or Client(host=host, port=port)
self._serialization_adapter = BasicSerializationAdapter(value_format="dict")
+
+ super().__init__(default_collection=default_collection)📝 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.
| super().__init__(default_collection=default_collection) | |
| self._client = client or Client(host=host, port=port) | |
| super().__init__(default_collection=default_collection) | |
| self._serialization_adapter = BasicSerializationAdapter(value_format="dict") | |
| self._client = client or Client(host=host, port=port) | |
| self._serialization_adapter = BasicSerializationAdapter(value_format="dict") | |
| super().__init__(default_collection=default_collection) |
🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/memcached/store.py around
lines 48 to 52, the adapter is initialized after calling super().__init__, which
can cause AttributeError if the parent constructor calls methods that use the
adapter; move the line that sets self._serialization_adapter =
BasicSerializationAdapter(value_format="dict") to before the
super().__init__(default_collection=default_collection) call so the adapter is
ready during base-class initialization.
| from datetime import datetime | ||
|
|
||
| from key_value.shared.utils.compound import compound_key, get_collections_from_compound_keys, get_keys_from_compound_keys | ||
| from key_value.shared.utils.managed_entry import ManagedEntry, load_from_json |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused import after cleaning up dead code.
Once SimpleStoreEntry.to_managed_entry() is removed (as suggested above), the load_from_json import will no longer be needed.
Apply this diff:
-from key_value.shared.utils.managed_entry import ManagedEntry, load_from_json
+from key_value.shared.utils.managed_entry import ManagedEntry📝 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.
| from key_value.shared.utils.managed_entry import ManagedEntry, load_from_json | |
| from key_value.shared.utils.managed_entry import ManagedEntry |
🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/simple/store.py around line
7, the import load_from_json from key_value.shared.utils.managed_entry is no
longer used after removing SimpleStoreEntry.to_managed_entry(); remove
load_from_json from the import list (leave ManagedEntry if still used) or delete
the entire import line if neither symbol is used to eliminate the unused import.
| self._data = defaultdict[str, SimpleStoreEntry]() | ||
|
|
||
| super().__init__(default_collection=default_collection) | ||
| self._serialization_adapter = ValueOnlySerializationAdapter() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type annotation for _serialization_adapter.
The instance variable should have a type annotation for consistency and better type checking.
Add the annotation to the class attributes section:
class SimpleStore(BaseEnumerateCollectionsStore, BaseEnumerateKeysStore, BaseDestroyStore, BaseStore):
"""Simple managed dictionary-based key-value store for testing and development."""
max_entries: int
_data: dict[str, SimpleStoreEntry]
+ _serialization_adapter: ValueOnlySerializationAdapterCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/simple/store.py around line
67, add a type annotation for the instance variable _serialization_adapter in
the class attributes section (e.g. declare _serialization_adapter:
ValueOnlySerializationAdapter) so the attribute has an explicit type; if
ValueOnlySerializationAdapter is not already imported at the top of the file,
add the import for it.
| class ValueOnlySerializationAdapter(SerializationAdapter): | ||
| """Serialization adapter that only serializes the value.""" | ||
|
|
||
| def __init__(self, *, value_format: Literal["string", "dict"] | None = "dict") -> None: | ||
| super().__init__(date_format=None, value_format=value_format) | ||
|
|
||
| def prepare_load(self, data: dict[str, Any]) -> dict[str, Any]: | ||
| return { | ||
| "value": data, | ||
| } | ||
|
|
||
| def prepare_dump(self, data: dict[str, Any]) -> dict[str, Any]: | ||
| return data.get("value") |
There was a problem hiding this comment.
Fix return type and behavior of ValueOnlySerializationAdapter; resolve pyright error.
prepare_dump must return a dict, and dump_json should emit only the value. This also fixes the pipeline failure at Line 148.
class ValueOnlySerializationAdapter(SerializationAdapter):
@@
- def prepare_load(self, data: dict[str, Any]) -> dict[str, Any]:
+ def prepare_load(self, data: dict[str, Any]) -> dict[str, Any]:
return {
"value": data,
}
- def prepare_dump(self, data: dict[str, Any]) -> dict[str, Any]:
- return data.get("value")
+ def prepare_dump(self, data: dict[str, Any]) -> dict[str, Any]:
+ # Ensure dict return type for static analysis; dump_json is overridden to emit only the value.
+ return {"value": data["value"]}
+
+ def dump_dict(self, entry: ManagedEntry, exclude_none: bool = True) -> dict[str, Any]:
+ # Keep contract but only carry the value forward.
+ return {"value": entry.value_as_dict if self._value_format == "dict" else entry.value_as_json}
+
+ def dump_json(self, entry: ManagedEntry, exclude_none: bool = True) -> str:
+ # Value-only JSON emission.
+ if self._value_format == "dict":
+ return dump_to_json(obj=entry.value_as_dict)
+ return entry.value_as_jsonCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Run Tests
[error] 148-148: pyright: Type 'Any | None' is not assignable to return type 'dict[str, Any]'. (reportReturnType)
🪛 GitHub Check: static_analysis (key-value/key-value-shared)
[failure] 148-148:
Type "Any | None" is not assignable to return type "dict[str, Any]"
Type "Any | None" is not assignable to type "dict[str, Any]"
"None" is not assignable to "dict[str, Any]" (reportReturnType)
🤖 Prompt for AI Agents
key-value/key-value-shared/src/key_value/shared/utils/serialization.py lines
136-148: prepare_dump currently returns the raw value (possibly non-dict) which
violates the expected dict[str, Any] return type and causes pyright/pipeline
failure; change prepare_dump to always return a dict like {"value":
data.get("value")} (ensuring the value key exists and respects typing), and
add/override a dump_json method that serializes and returns only the inner value
(not the wrapper dict) so external JSON output remains the value-only form; keep
prepare_load as-is (it should wrap incoming value into {"value": ...}) and
ensure function and return type annotations reflect dict[str, Any] for
prepare_dump and appropriate str/bytes return for dump_json.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py
Show resolved
Hide resolved
| self.max_entries_per_collection = max_entries_per_collection or sys.maxsize | ||
|
|
||
| self._cache = {} | ||
|
|
||
| self._stable_api = True | ||
|
|
||
| self._serialization_adapter = BasicSerializationAdapter() | ||
|
|
There was a problem hiding this comment.
Minor: Remove or use unused store-level adapter.
MemoryStore sets self._serialization_adapter but doesn't use it in this module. Either pass it into MemoryCollection (as above) or drop it.
🤖 Prompt for AI Agents
In key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py
around lines 111 to 118, the MemoryStore sets self._serialization_adapter but
never uses it; either remove the unused attribute or wire it into
MemoryCollection instances. Fix by passing self._serialization_adapter to
MemoryCollection constructors wherever collections are created in this module
(or to the Collection factory call used), or if collections already accept an
adapter ensure the adapter parameter is forwarded; alternatively delete the
self._serialization_adapter assignment and any related unused imports to
eliminate dead code.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/simple/store.py
Show resolved
Hide resolved
|
Summary
This PR implements a comprehensive refactoring of ManagedEntry serialization to address inconsistencies across stores and wrappers, as identified in issue #165.
Changes
1. Created Serialization Adapter Pattern:
SerializationAdapterbase class with three implementations:FullJsonAdapter: For Redis/Valkey (full JSON string)StringifiedDictAdapter: For MongoDB (dict with stringified value)ElasticsearchAdapter: For Elasticsearch (native or stringified)2. Updated Stores:
3. Fixed Wrapper Inefficiencies:
estimate_serialized_size()utility function4. Removed Deprecated Functions:
Benefits
Closes #165
🤖 Generated with Claude Code
Summary by CodeRabbit