feat: add seed support, default values, and JSON serialization wrappers #122
Conversation
- Add seed parameter to MemoryStore for pre-populating data on init - Add DefaultValueWrapper to return default values for missing keys - Add default parameter to PydanticAdapter get() and get_many() methods - Add PydanticJsonWrapper to ensure JSON-serializable value conversion All features updated to use Sequence[str] for key parameters per PR #119. Fixes #115 Co-authored-by: William Easton <strawgate@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds seed support for stores, a DefaultValueWrapper that returns materialized defaults for missing keys, and optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BaseStore
participant MemoryStore
User->>BaseStore: __init__(..., seed=seed_map)
BaseStore->>BaseStore: _seed = _seed_to_frozen_seed_data(seed_map)
Note over BaseStore: setup() invoked during store init
BaseStore->>BaseStore: _setup/_setup_store()
BaseStore->>BaseStore: _seed_store()
loop per collection in _seed
BaseStore->>MemoryStore: ensure collection exists (lazy create)
loop per key in collection
BaseStore->>MemoryStore: put(key, value)
end
end
sequenceDiagram
participant Client
participant DefaultValueWrapper
participant UnderlyingStore
Client->>DefaultValueWrapper: get(key)
DefaultValueWrapper->>UnderlyingStore: get(key)
alt found
UnderlyingStore-->>DefaultValueWrapper: value, ttl
DefaultValueWrapper-->>Client: value, ttl
else missing
UnderlyingStore-->>DefaultValueWrapper: None
DefaultValueWrapper->>DefaultValueWrapper: materialize default from _default_value_json
DefaultValueWrapper-->>Client: default_value, _default_ttl
end
sequenceDiagram
participant Caller
participant PydanticAdapter
participant UnderlyingStore
Caller->>PydanticAdapter: get(key, default=?)
PydanticAdapter->>UnderlyingStore: get(key)
UnderlyingStore-->>PydanticAdapter: raw_dict or None
alt raw_dict and validates
PydanticAdapter-->>Caller: validated model T
else missing or invalid
PydanticAdapter-->>Caller: provided default (or None)
end
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@claude i think we should overload the get on pydanticwrapper so that it type checks correctly when you pass a default and maybe worth a light refactor of the memorystore to make seeding a little easier with less copy pasta |
|
Claude finished @strawgate's task —— View job Tasks completedTasks
Changes MadePydanticAdapter (
MemoryStore (
Verification
--- | Branch |
…moryStore seeding - Add @overload decorator to PydanticAdapter.get() for better type inference when default is provided - Refactor MemoryStore._seed_store_sync() to extract collection_cache variable and reduce duplication Co-authored-by: William Easton <strawgate@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (2)
77-88: Clarify default behavior when validation fails.The docstring describes what happens when "the key doesn't exist," but it doesn't explain the behavior when a key exists but validation fails (and
raise_on_validation_error=False). In that case,Noneis returned from_validate_modelrather than the provided default.Update the docstring to clarify:
Args: key: The key to retrieve. collection: The collection to use. If not provided, uses the default collection. - default: The default value to return if the key doesn't exist. + default: The default value to return if the key doesn't exist. Note: if the key exists but validation + fails (when raise_on_validation_error=False), None is returned instead of the default. Returns: - The parsed model instance, the default value if provided and key doesn't exist, or None. + The parsed model instance if found and valid, the default value if the key doesn't exist, or None if + validation fails.
96-115: Consider overloads for get_many to match get's pattern.The
getmethod has overload signatures to narrow the return type when a default is provided, butget_manydoes not. This creates an API inconsistency—callers might expect similar type-narrowing behavior.If you retain the overloads for
get, consider adding matching overloads forget_many:@overload async def get_many(self, keys: Sequence[str], *, collection: str | None = None, default: T) -> list[T]: ... @overload async def get_many(self, keys: Sequence[str], *, collection: str | None = None, default: None = None) -> list[T | None]: ...However, note that this would inherit the same type-safety issue discussed for
get—validation failures can still produceNoneeven when a default is provided. If you choose to remove or modify the overloads ongetto address that issue, you may want to skip adding them here as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py(4 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/wrappers/default_value/__init__.py(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/__init__.py(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/wrapper.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/wrapper.py (2)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (3)
put(117-126)ttl(150-168)put_many(128-136)
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/__init__.py (1)
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py (1)
DefaultValueWrapper(10-81)
key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/__init__.py (1)
key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/wrapper.py (1)
PydanticJsonWrapper(11-69)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (3)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (3)
get(9-24)get_many(60-70)keys(132-139)key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (3)
get(68-83)get_many(85-99)_validate_model(46-56)key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py (2)
get(28-39)get_many(42-53)
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py (2)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (6)
get(69-69)get(72-72)get(74-94)get_many(96-115)ttl(150-168)ttl_many(170-176)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (2)
MemoryCollection(68-97)put(89-90)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(14-84)
🔇 Additional comments (9)
key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/wrapper.py (3)
11-25: LGTM! Clean wrapper initialization.The class structure and initialization are well-designed. The TypeAdapter is appropriately instantiated once during initialization for performance, and the docstring clearly explains the wrapper's purpose of ensuring JSON-serializable storage.
38-49: LGTM! Clean implementation.The
putmethod correctly converts values to JSON-safe format before delegating to the wrapped store. The implementation properly preserves all parameters and follows the wrapper pattern.
52-69: LGTM! Correct bulk operation implementation.The
put_manymethod correctly converts multiple values to JSON-safe format before delegating to the wrapped store. The implementation efficiently processes all values and maintains proper alignment with keys and TTL values.Note: Only write operations (
put/put_many) are overridden in this wrapper. Read operations (get/get_many) pass through to the wrapped store without conversion, which is appropriate since the wrapper's purpose is to ensure JSON-safe storage, not to transform retrieved data.key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (3)
2-2: LGTM: Import addition is appropriate.The
Mappingimport is correctly added for type-hinting the seed parameter.
113-119: LGTM: Method signature is well-structured.The seed parameter type correctly represents the nested structure and follows the existing pattern of keyword-only arguments.
136-138: LGTM: Seed application logic is correct.The conditional seeding after base class initialization is appropriate, and the synchronous approach is suitable for initialization-time data loading.
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (1)
2-2: LGTM: Required import for overload support.The addition of
overloadto the typing imports is necessary for the new overload decorators used below.key-value/key-value-aio/src/key_value/aio/wrappers/default_value/__init__.py (1)
1-3: Re-export looks goodImport path and all are correct. No further action.
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py (1)
41-53: Minor: keep type, but note None won’t appearThe signature allows
None, but this implementation always returns a dict (due to default). Fine to keep (matches protocol); just noting for callers.If protocol allows narrowing, we could change return type to
list[dict[str, Any]]. If not, keep as-is. Please confirm protocol constraints.
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/__init__.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/wrapper.py
Outdated
Show resolved
Hide resolved
|
@claude lint and in DefaultValueWrapper i think we need a |
|
Claude finished @strawgate's task —— View job Tasks CompletedTasks
Changes MadeDefaultValueWrapper (
MemoryStore (
Verification
--- | Branch |
- Add default_ttl parameter to DefaultValueWrapper for configurable TTL on default values - Fix ttl() and ttl_many() to return default_ttl instead of underlying store's TTL for missing keys - Extract _create_collection() helper in MemoryStore to eliminate code duplication - Refactor _seed_store_sync() and _setup_collection() to use shared helper Co-authored-by: William Easton <strawgate@users.noreply.github.com>
|
@claude tests are failing please fix and also do a line by line self code review and fix any issues |
|
Claude finished @strawgate's task —— View job Tasks Completed ✅SummaryI've completed a comprehensive line-by-line code review and fixed all identified issues. All tests are now passing! Issues Found and FixedCritical Type Error (causing test failure) ✅
Type Safety Issues ✅
Code Quality Issues ✅
Test Results
Changes Committed
|
- Fix PydanticJsonWrapper.put_many ttl parameter type to match protocol - Update PydanticAdapter get/get_many to return default on validation failure - Standardize attribute naming in wrappers (_key_value as private, key_value as public alias) - Improve MemoryStore seed docstring clarity - Add module docstrings to wrapper __init__.py files - Add type annotation to get_many result list All tests passing: - Ruff linting: passed - Ruff formatting: passed - Type checking (basedpyright strict): passed Co-authored-by: William Easton <strawgate@users.noreply.github.com>
|
@claude perhaps seeding could occur when we call setup on the store which should only be called once, that would work nicely right? And then this could be a base class feature |
|
Claude finished @strawgate's task —— View job Tasks Completed ✅
SummaryI've successfully refactored the MemoryStore seeding to occur during the Changes MadeMemoryStore Refactoring (
Why This Approach is Better
Verification
The CI will run the full test suite including type checking with all dependencies properly installed. |
…alization - Store seed data as instance variable instead of seeding synchronously in __init__ - Move seeding logic into _setup_collection which is called once per collection - Remove _seed_store_sync method entirely - Update docstring to clarify seeding occurs lazily when collection is first accessed This provides cleaner separation of concerns and leverages the existing setup infrastructure, ensuring collections are only seeded when actually used. Co-authored-by: William Easton <strawgate@users.noreply.github.com>
|
@coderabbitai please review the latest on this pr |
|
@strawgate, I'll review the latest changes on this PR right away. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (3)
key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/wrapper.py (1)
31-41: Replace ignore with explicit cast or document why it’s safeRepeat of earlier nitpick: prefer
typing.cast(dict[str, Any], ...)or add an explanatory comment for the suppression.-from pydantic import TypeAdapter +from pydantic import TypeAdapter +from typing import cast @@ - def _to_json_safe(self, value: Mapping[str, Any]) -> dict[str, Any]: + def _to_json_safe(self, value: Mapping[str, Any]) -> dict[str, Any]: @@ - return self._adapter.dump_python(value, mode="json") # type: ignore[return-value] + # TypeAdapter.dump_python returns Any; for dict input this is a JSON-safe dict[str, Any]. + return cast(dict[str, Any], self._adapter.dump_python(value, mode="json"))key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (1)
68-96: Overload + default path now sound for get().Validation failure now falls back to default, making the overload contract accurate.
key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (1)
68-96: Sync get(): contract aligned with default semantics.Matches aio fix; returns default on validation failure or miss.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py(4 hunks)key-value/key-value-aio/src/key_value/aio/stores/memory/store.py(2 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/default_value/__init__.py(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/__init__.py(1 hunks)key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/wrapper.py(1 hunks)key-value/key-value-aio/tests/adapters/test_pydantic.py(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/__init__.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/wrapper.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/pydantic_json/__init__.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/pydantic_json/wrapper.py(1 hunks)key-value/key-value-sync/src/key_value/sync/wrappers/default_value/__init__.py(1 hunks)key-value/key-value-sync/src/key_value/sync/wrappers/pydantic_json/__init__.py(1 hunks)key-value/key-value-sync/tests/code_gen/adapters/test_pydantic.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python code must follow Ruff formatting and linting with a configured maximum line length of 140 characters (per pyproject.toml)
Use Basedpyright in strict mode for static type checking across the Python codebase
Files:
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/pydantic_json/__init__.pykey-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/__init__.pykey-value/key-value-aio/tests/adapters/test_pydantic.pykey-value/key-value-sync/tests/code_gen/adapters/test_pydantic.pykey-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/wrapper.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/wrapper.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/__init__.pykey-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.pykey-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.pykey-value/key-value-aio/src/key_value/aio/stores/memory/store.pykey-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.pykey-value/key-value-sync/src/key_value/sync/wrappers/pydantic_json/__init__.pykey-value/key-value-aio/src/key_value/aio/wrappers/default_value/__init__.pykey-value/key-value-sync/src/key_value/sync/wrappers/default_value/__init__.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/pydantic_json/wrapper.pykey-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py
key-value/key-value-sync/**
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit files in key-value/key-value-sync/ directly; this package is generated and will be overwritten by make codegen
Files:
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/pydantic_json/__init__.pykey-value/key-value-sync/tests/code_gen/adapters/test_pydantic.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/wrapper.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/__init__.pykey-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.pykey-value/key-value-sync/src/key_value/sync/wrappers/pydantic_json/__init__.pykey-value/key-value-sync/src/key_value/sync/wrappers/default_value/__init__.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/pydantic_json/wrapper.pykey-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py
key-value/key-value-aio/src/key_value/aio/stores/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
key-value/key-value-aio/src/key_value/aio/stores/**/*.py: Async store implementations must implement the AsyncKeyValue protocol from key_value/aio/protocols/key_value.py
Store backends must store and retrieve ManagedEntry wrappers (with value, creation timestamp, and TTL), not raw values
Files:
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py
🧬 Code graph analysis (16)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/pydantic_json/__init__.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/pydantic_json/wrapper.py (1)
PydanticJsonWrapper(14-71)
key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/__init__.py (1)
key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/wrapper.py (1)
PydanticJsonWrapper(11-73)
key-value/key-value-aio/tests/adapters/test_pydantic.py (2)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (8)
PydanticAdapter(15-191)get(69-69)get(72-72)get(74-96)put(132-141)get_many(99-99)get_many(102-102)get_many(104-130)key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (4)
get(9-24)put(39-49)get_many(60-70)keys(132-139)
key-value/key-value-sync/tests/code_gen/adapters/test_pydantic.py (2)
key-value/key-value-aio/tests/adapters/test_pydantic.py (3)
User(12-15)test_simple_adapter_with_default(85-92)user_adapter(56-57)key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (8)
PydanticAdapter(18-191)get(69-69)get(72-72)get(74-96)put(132-141)get_many(99-99)get_many(102-102)get_many(104-130)
key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/wrapper.py (3)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (3)
put(132-141)ttl(165-183)put_many(143-151)key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py (1)
ttl(67-80)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/wrapper.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (8)
get(69-69)get(72-72)get(74-96)get_many(99-99)get_many(102-102)get_many(104-130)ttl(165-183)ttl_many(185-191)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/__init__.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/wrapper.py (1)
DefaultValueWrapper(13-94)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (7)
get(69-69)get(72-72)get(74-96)_validate_model(46-56)get_many(99-99)get_many(102-102)get_many(104-130)key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (3)
get(9-24)get_many(60-70)keys(132-139)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (4)
_create_collection(140-151)MemoryCollection(70-103)_setup_collection(154-168)put(95-96)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(14-84)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (4)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
put(39-49)key-value/key-value-aio/src/key_value/aio/stores/base.py (2)
_setup_collection(74-75)put(193-204)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (4)
_create_collection(135-146)MemoryCollection(69-98)_setup_collection(149-163)put(90-91)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(14-84)
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py (2)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (8)
get(69-69)get(72-72)get(74-96)get_many(99-99)get_many(102-102)get_many(104-130)ttl(165-183)ttl_many(185-191)
key-value/key-value-sync/src/key_value/sync/wrappers/pydantic_json/__init__.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/pydantic_json/wrapper.py (1)
PydanticJsonWrapper(14-71)
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/__init__.py (1)
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py (1)
DefaultValueWrapper(10-96)
key-value/key-value-sync/src/key_value/sync/wrappers/default_value/__init__.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/wrapper.py (1)
DefaultValueWrapper(13-94)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/pydantic_json/wrapper.py (3)
key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
KeyValue(169-174)key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (3)
put(132-141)ttl(165-183)put_many(143-151)key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (3)
put(132-141)ttl(165-183)put_many(143-151)
key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (2)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (7)
get(69-69)get(72-72)get(74-96)_validate_model(46-56)get_many(99-99)get_many(102-102)get_many(104-130)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/wrapper.py (2)
get(37-48)get_many(51-62)
⏰ 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). (6)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
🔇 Additional comments (12)
key-value/key-value-sync/src/key_value/sync/wrappers/pydantic_json/__init__.py (1)
1-8: Auto-generated file correctly re-exports PydanticJsonWrapper from code_gen.This file properly serves as the public API entry point for the PydanticJsonWrapper, following the standard pattern of re-exporting from the code_gen module. The import path aligns with the implementation, and all correctly exposes the wrapper class.
Note: Per coding guidelines, do not edit files in
key-value/key-value-sync/directly—this package is auto-generated and will be overwritten bymake codegen.key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/__init__.py (1)
1-5: LGTM! Module docstring properly added.The file correctly implements the standard Python package initialization pattern with proper re-export of
PydanticJsonWrapper. The module docstring clearly describes the package's purpose, addressing the previous review feedback.key-value/key-value-sync/src/key_value/sync/wrappers/default_value/__init__.py (1)
1-8: LGTM! Auto-generated code is correct.This auto-generated sync wrapper package initialization correctly:
- Includes proper auto-generation warnings
- Provides a clear module docstring
- Imports and re-exports
DefaultValueWrapperfrom the generated implementation- Follows the established pattern for other sync wrappers
As per coding guidelines, any changes to this functionality should be made to the source aio version in
key-value-aio/, not this file, as it will be regenerated bymake codegen.key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/__init__.py (1)
6-8: Re-export looks good
DefaultValueWrapperis correctly exposed via package init.key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/pydantic_json/wrapper.py (1)
58-71: Validatekeysandvalueslength in put_manyAdd a simple guard to catch mismatch early and fail fast.
Apply in the original source (this file is generated):
def put_many( self, keys: Sequence[str], values: Sequence[Mapping[str, Any]], *, collection: str | None = None, ttl: SupportsFloat | None = None ) -> None: """Store multiple values after converting them to JSON-safe format. @@ - json_safe_values = [self._to_json_safe(value) for value in values] + if len(keys) != len(values): + raise ValueError("keys and values must have the same length") + json_safe_values = [self._to_json_safe(value) for value in values] self._key_value.put_many(keys=keys, values=json_safe_values, collection=collection, ttl=ttl)Based on learnings.
Likely an incorrect or invalid review comment.
key-value/key-value-sync/tests/code_gen/adapters/test_pydantic.py (1)
44-45: LGTM on new test data.SAMPLE_USER_2 and TEST_KEY_2 additions look correct and consistent.
Also applies to: 50-50
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/__init__.py (1)
1-5: Public API export looks good.Clean re-export; consistent with other wrappers.
key-value/key-value-aio/tests/adapters/test_pydantic.py (1)
41-41: LGTM on added test fixtures.SAMPLE_USER_2 and TEST_KEY_2 are fine.
Also applies to: 47-47
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (1)
98-131: get_many() correctly applies defaults per element.Preserves order and substitutes default on missing/invalid entries. Good.
key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (1)
98-131: Sync get_many(): per-item defaulting with order preserved.Implementation mirrors aio and is correct.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (1)
149-164: No changes required — idempotency guarantees already exist in base class.The base class
BaseStore(lines 92–102 ofbase.py) implements a double-checked locking pattern that guarantees_setup_collectionruns exactly once per collection:
- Per-collection lock:
_setup_collection_locks[collection]- Per-collection completion flag:
_setup_collection_complete[collection]- Two checks before and after lock acquisition to prevent concurrent re-entry and re-seeding
The
MemoryStore._setup_collectionmethod is called only by the base class'ssetup_collection()method, which enforces these guarantees. No additional guards are needed in the implementation.Additionally, this file is generated code and cannot be edited directly per the
key-value/key-value-sync/constraint—it will be overwritten bymake codegen.Likely an incorrect or invalid review comment.
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (1)
154-169: Base class guarantees one-time execution per collection; double-seed concern unfounded. JSON-serialization documentation is valid.The base class (base.py lines 92-99) uses a double-check locking pattern with per-collection locks and flags (
_setup_collection_locks[collection]and_setup_collection_complete[collection]), ensuring_setup_collectionruns exactly once per collection. No double-seeding risk exists.However, the JSON-serialization suggestion is valid. During seeding,
ManagedEntry.to_json()is invoked viaMemoryCacheEntry.from_managed_entry()when items are stored. Seed values must be JSON-serializable orSerializationErroris raised.Suggested doc tweak:
- seed: Optional seed data to pre-populate the store. Format: {collection: {key: {field: value, ...}}}. - Each value must be a mapping (dict) that will be stored as the entry's value. + seed: Optional seed data to pre-populate the store. Format: {collection: {key: {field: value, ...}}}. + Each value must be a mapping (dict) stored as the entry's value and must be JSON-serializable.
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py
Outdated
Show resolved
Hide resolved
key-value/key-value-aio/src/key_value/aio/wrappers/pydantic_json/wrapper.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/wrapper.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/pydantic_json/__init__.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/pydantic_json/wrapper.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/tests/code_gen/adapters/test_pydantic.py
Outdated
Show resolved
Hide resolved
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-aio/src/key_value/aio/adapters/pydantic/adapter.py (1)
64-72: Replace pyright ignore with a precise cast.Avoid
# pyright: ignore[reportAny]by casting the dump to the declared return type.- return self._type_adapter.dump_python(value, mode="json") # pyright: ignore[reportAny] + # TypeAdapter.dump_python returns Any; constrain to our declared shape. + from typing import cast + return cast(dict[str, Any], self._type_adapter.dump_python(value, mode="json"))
♻️ Duplicate comments (5)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (2)
107-129: Previously flagged: Fix strict typing for_seedassignment in source file.The typing issue at line 129 (
self._seed = seed or {}) has already been identified in a previous review. The expressionseed or {}returns an emptydictwhenseedisNone, butdictis incompatible with the declared typeMapping[str, Mapping[str, Mapping[str, Any]]]under strict type checking.Refer to the existing comment for the recommended fix using
castin the source (aio) file.
135-147: Previously flagged: Make collection creation idempotent.The idempotency concern for
_create_collectionhas already been identified in a previous review. While the base class guards_setup_collectionto run once per collection, making_create_collectionidempotent by checking for existing collections first is a defensive best practice.Refer to the existing comment for the recommended implementation in the source (aio) file.
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (1)
97-103: Good: default applied on validation failure.This closes the previously noted type/behavior gap when
raise_on_validation_error=False.key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (2)
134-134: Strict typing bug:_seed = seed or {}violates Basedpyright strict.
{}is inferred asdict[str, Any], notMapping[str, Mapping[str, Mapping[str, Any]]]]. Usecastfor a typed empty mapping.-from typing import Any, SupportsFloat +from typing import Any, SupportsFloat, cast @@ - self._seed = seed or {} + self._seed = seed if seed is not None else cast(Mapping[str, Mapping[str, Mapping[str, Any]]], {})
140-152: Make collection creation idempotent to prevent accidental overwrite.Avoid replacing an existing collection if called twice (re-entrancy or concurrent setup).
def _create_collection(self, collection: str) -> MemoryCollection: @@ - collection_cache = MemoryCollection(max_entries=self.max_entries_per_collection) - self._cache[collection] = collection_cache - return collection_cache + existing = self._cache.get(collection) + if existing is not None: + return existing + collection_cache = MemoryCollection(max_entries=self.max_entries_per_collection) + self._cache[collection] = collection_cache + return collection_cache
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py(4 hunks)key-value/key-value-aio/src/key_value/aio/stores/memory/store.py(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py(4 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
key-value/key-value-sync/**
📄 CodeRabbit inference engine (AGENTS.md)
Never edit files in key-value/key-value-sync/ directly; regenerate from async via make codegen
Files:
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.pykey-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use Ruff for Python formatting and linting (configured in pyproject.toml)
Python line length is 140 characters
Use Basedpyright in strict mode for static type checking
Files:
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.pykey-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.pykey-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.pykey-value/key-value-aio/src/key_value/aio/stores/memory/store.py
key-value/key-value-sync/src/key_value/sync/code_gen/stores/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Sync store files are generated mirrors of async stores; do not hand-edit implementations here
Files:
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py
key-value/key-value-aio/src/key_value/aio/{wrappers,adapters}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add functionality via wrappers/adapters in their respective directories rather than modifying core store behavior directly
Files:
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py
key-value/key-value-aio/src/key_value/aio/stores/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
key-value/key-value-aio/src/key_value/aio/stores/**/*.py: Store implementations must persist ManagedEntry wrappers (not raw values) with value, creation timestamp, and TTL metadata
Async store implementations must implement the AsyncKeyValue protocol (key_value/aio/protocols/key_value.py)
Files:
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py
🧬 Code graph analysis (4)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (3)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (2)
_setup_collection(77-78)put(182-189)key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (4)
_create_collection(140-151)MemoryCollection(70-103)_setup_collection(154-168)put(95-96)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(14-84)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (7)
get(75-75)get(78-78)get(80-102)_validate_model(52-62)get_many(105-105)get_many(108-108)get_many(110-136)key-value/key-value-aio/src/key_value/aio/stores/base.py (3)
get(111-137)get_many(140-145)keys(290-296)
key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (1)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (7)
get(75-75)get(78-78)get(80-102)_validate_model(52-62)get_many(105-105)get_many(108-108)get_many(110-136)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (3)
key-value/key-value-aio/src/key_value/aio/stores/base.py (2)
_setup_collection(74-75)put(193-204)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (4)
_create_collection(135-146)MemoryCollection(69-98)_setup_collection(149-163)put(90-91)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(14-84)
🔇 Additional comments (9)
key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (3)
74-102: Excellent type-safe overload design.The overloads correctly narrow the return type based on whether a non-None default is provided, and the implementation properly validates before returning the default. This matches the async version and provides good type safety for callers.
104-136: Correct implementation of batch default handling.The
get_manyoverloads and implementation correctly handle defaults for both missing keys and validation failures, preserving the order of results. The type narrowing ensures callers getlist[T]when providing a non-None default.
1-5: Manual verification required: Confirm this file was regenerated viamake codegen.The sandbox environment doesn't have
makeavailable, so I cannot verify if this file was auto-generated. However, the file has an explicit warning header stating it's auto-generated from the async source, and the coding guidelines are clear: files inkey-value/key-value-sync/must never be edited directly—they must be regenerated viamake codegen.Please confirm that:
- The changes were made to the async source file (
key-value/src/key_value/adapters/pydantic/adapter.py)- The sync version was regenerated by running
make codegen- This PR's changes did not involve manually editing files in the
key-value/key-value-sync/directorykey-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (2)
5-5: LGTM: Import required for seed type annotations.The
Mappingimport is correctly added to support the new_seedfield type annotation.
148-164: LGTM: Seeding logic is correct for the stated requirements.The implementation correctly:
- Creates the collection lazily on first access
- Seeds the collection if seed data exists for it
- Creates
ManagedEntryinstances with the provided values- Stores entries via the collection's
putmethodNote: Seeded entries are created without TTL (they won't expire automatically). This aligns with the PR objectives, which don't specify TTL support for seed data. If time-limited seed data is needed in the future, the seed structure could be extended to include optional TTL values per entry.
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (1)
74-90: Overloads and default semantics look correct.Signatures and docs align with behavior and preserve type safety when defaults are provided.
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (3)
2-2: Import change is fine.
112-112: Typed _seed annotation looks good.
126-129: Docstring vs PR summary mismatch: lazy vs synchronous seeding.Docstring says “Seeding occurs lazily…”, while the PR summary states synchronous initialization. Confirm intended behavior and update either implementation or docs accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (1)
113-136: Fix type safety for _seed assignment.Line 136 has a type safety violation under Basedpyright strict mode. The pattern
seed or {}infers the empty dict as an incompatible type.Apply this diff:
-from typing import Any, SupportsFloat +from typing import Any, SupportsFloat, cast- super().__init__(default_collection=default_collection, seed=seed) + from key_value.aio.stores.base import SEED_DATA_TYPE + super().__init__( + default_collection=default_collection, + seed=seed if seed is not None else cast(SEED_DATA_TYPE, {}) + )Wait, looking more carefully - the issue is in the base class initialization at line 69 of base.py, not here. The seed parameter is passed through correctly. Let me reconsider...
Actually, the memory store passes
seeddirectly tosuper().__init__(), so the fix should be in the base class only. However, if we want to be extra safe and the base doesn't handle it, we could validate here.But checking the base.py again - line 69 does
self._seed = seed or {}which IS the problem. So the fix should be in base.py, not here. This file just passes through the parameter correctly.Let me revise this comment.
♻️ Duplicate comments (4)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (1)
108-131: Type safety fix needed in source file (aio variant).The assignment at line 131 has the same type safety issue as the aio base. Since this is an auto-generated file, the fix must be applied to
key-value/key-value-aio/src/key_value/aio/stores/memory/store.pyand then regenerated.See the review comment on the aio source file for the required changes.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
55-72: Type safety fix needed in source file.Line 72 has the same type safety issue as the aio source. Since this is auto-generated, the fix must be applied to
key-value/key-value-aio/src/key_value/aio/stores/base.pyline 69, then regenerate withmake codegen.The fix is to use
castto provide a properly typed empty fallback:-from typing import Any, SupportsFloat +from typing import Any, SupportsFloat, cast- self._seed = seed or {} + self._seed = seed if seed is not None else cast(SEED_DATA_TYPE, {})Apply this in the aio source and regenerate.
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/wrapper.py (1)
1-102: Auto-generated file correctly mirrors async source.This file is auto-generated from the async implementation. The
_new_default_value()helper method already centralizes the default-value copying logic via JSON deserialization, which provides deep-copy safety. Any changes should be made to the async source file inkey-value-aio/src/key_value/aio/wrappers/default_value/wrapper.pyand regenerated.Based on coding guidelines.
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py (1)
43-44: Helper method centralizes default-value copying.The
_new_default_value()helper consolidates the default-value copying logic, addressing past suggestions to extract this behavior. The JSON round-trip provides deep-copy semantics, preventing callers from mutating the default value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
key-value/key-value-aio/src/key_value/aio/stores/base.py(4 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/wrappers/default_value/wrapper.py(1 hunks)key-value/key-value-aio/tests/stores/memory/test_memory.py(1 hunks)key-value/key-value-aio/tests/stores/wrappers/test_default_value.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py(4 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/wrappers/default_value/wrapper.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/memory/test_memory.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/wrappers/test_default_value.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use Ruff for Python formatting and linting (configured in pyproject.toml)
Python line length is 140 characters
Use Basedpyright in strict mode for static type checking
Files:
key-value/key-value-aio/tests/stores/memory/test_memory.pykey-value/key-value-aio/tests/stores/wrappers/test_default_value.pykey-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/wrapper.pykey-value/key-value-sync/tests/code_gen/stores/wrappers/test_default_value.pykey-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.pykey-value/key-value-sync/tests/code_gen/stores/memory/test_memory.pykey-value/key-value-aio/src/key_value/aio/stores/memory/store.pykey-value/key-value-aio/src/key_value/aio/stores/base.pykey-value/key-value-sync/src/key_value/sync/code_gen/stores/base.pykey-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py
key-value/key-value-sync/**
📄 CodeRabbit inference engine (AGENTS.md)
Never edit files in key-value/key-value-sync/ directly; regenerate from async via make codegen
Files:
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/wrapper.pykey-value/key-value-sync/tests/code_gen/stores/wrappers/test_default_value.pykey-value/key-value-sync/tests/code_gen/stores/memory/test_memory.pykey-value/key-value-sync/src/key_value/sync/code_gen/stores/base.pykey-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py
key-value/key-value-aio/src/key_value/aio/{wrappers,adapters}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Add functionality via wrappers/adapters in their respective directories rather than modifying core store behavior directly
Files:
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py
key-value/key-value-aio/src/key_value/aio/stores/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
key-value/key-value-aio/src/key_value/aio/stores/**/*.py: Store implementations must persist ManagedEntry wrappers (not raw values) with value, creation timestamp, and TTL metadata
Async store implementations must implement the AsyncKeyValue protocol (key_value/aio/protocols/key_value.py)
Files:
key-value/key-value-aio/src/key_value/aio/stores/memory/store.pykey-value/key-value-aio/src/key_value/aio/stores/base.py
key-value/key-value-sync/src/key_value/sync/code_gen/stores/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Sync store files are generated mirrors of async stores; do not hand-edit implementations here
Files:
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.pykey-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py
🧬 Code graph analysis (10)
key-value/key-value-aio/tests/stores/memory/test_memory.py (2)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (2)
MemoryStore(106-204)get(85-93)key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
get(130-156)
key-value/key-value-aio/tests/stores/wrappers/test_default_value.py (4)
key-value/key-value-aio/src/key_value/aio/stores/base.py (6)
get(130-156)ttl(167-176)get_many(159-164)keys(309-315)ttl_many(179-194)put(212-223)key-value/key-value-aio/tests/stores/memory/test_memory.py (1)
store(11-12)key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (4)
MemoryStore(106-204)get(85-93)keys(101-103)put(95-96)key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py (5)
DefaultValueWrapper(11-104)get(47-58)ttl(75-88)get_many(61-72)ttl_many(91-104)
key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/wrapper.py (3)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (5)
get(133-154)get_many(157-162)keys(289-295)ttl(165-174)ttl_many(177-187)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
dump_to_json(87-92)load_from_json(95-111)key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py (6)
DefaultValueWrapper(11-104)_new_default_value(43-44)get(47-58)get_many(61-72)ttl(75-88)ttl_many(91-104)
key-value/key-value-sync/tests/code_gen/stores/wrappers/test_default_value.py (3)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (7)
BaseStore(33-279)get(133-154)ttl(165-174)get_many(157-162)keys(289-295)ttl_many(177-187)put(201-208)key-value/key-value-sync/tests/code_gen/stores/memory/test_memory.py (1)
store(14-15)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/wrapper.py (5)
DefaultValueWrapper(14-102)get(45-56)ttl(73-86)get_many(59-70)ttl_many(89-102)
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py (4)
key-value/key-value-aio/src/key_value/aio/stores/base.py (5)
get(130-156)get_many(159-164)keys(309-315)ttl(167-176)ttl_many(179-194)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (2)
dump_to_json(87-92)load_from_json(95-111)key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(176-181)key-value/key-value-sync/src/key_value/sync/code_gen/wrappers/default_value/wrapper.py (6)
DefaultValueWrapper(14-102)_new_default_value(41-42)get(45-56)get_many(59-70)ttl(73-86)ttl_many(89-102)
key-value/key-value-sync/tests/code_gen/stores/memory/test_memory.py (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (2)
MemoryStore(101-193)get(80-88)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
get(133-154)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (2)
_setup(81-82)_setup_collection(84-85)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (3)
_setup(134-136)_setup_collection(139-149)MemoryCollection(69-98)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (3)
_seed_store(90-95)setup_collection(111-121)put(201-208)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (3)
_seed_store(87-92)setup_collection(108-118)put(212-223)key-value/key-value-sync/src/key_value/sync/code_gen/protocols/key_value.py (1)
put(37-47)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (2)
key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (3)
_setup(139-141)_setup_collection(144-154)MemoryCollection(70-103)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (2)
_setup(84-85)_setup_collection(87-88)
🔇 Additional comments (21)
key-value/key-value-sync/tests/code_gen/stores/memory/test_memory.py (1)
17-19: LGTM! Test validates basic seed functionality.The test correctly verifies that a MemoryStore initialized with seed data returns the expected value on retrieval. The test is simple and effective.
key-value/key-value-aio/tests/stores/memory/test_memory.py (1)
14-16: LGTM! Async seed test is correct.The async test properly validates seed functionality with correct use of
awaitfor the asyncgetmethod.key-value/key-value-sync/tests/code_gen/stores/wrappers/test_default_value.py (3)
26-42: LGTM! Comprehensive test for default value behavior.The test thoroughly validates:
- Default values returned for missing keys
- Default TTL applied correctly
- Batch operations (get_many, ttl_many) with defaults
- Put operations override defaults
- Mixed results after updates
The use of
IsFloat(approx=...)for TTL assertions is appropriate for handling floating-point precision.
44-94: Skipped tests are appropriate for wrapper behavior.The skipped base class tests are intentional since the DefaultValueWrapper changes the expected behavior for missing keys (returns defaults instead of None). This is the correct approach.
20-24: No issues found -memory_storefixture is properly defined.The
memory_storefixture is defined inkey-value/key-value-sync/tests/code_gen/stores/conftest.pyand is automatically discovered by pytest for all tests in that directory hierarchy. The fixture will be correctly injected into thestoremethod.key-value/key-value-aio/src/key_value/aio/stores/base.py (3)
27-27: LGTM! Type alias clearly defines seed data structure.The
SEED_DATA_TYPEalias properly captures the three-level nested mapping structure: collection → key → field/value pairs.
94-107: LGTM! Setup flow ensures one-time seeding.The double-checked locking pattern ensures
_setup()and_seed_store()are called only once when_setup_completeis False. The lock prevents race conditions in async contexts.
87-92: No actionable issues found—seeding is properly guarded by double-checked locking.The verification confirms the concerns are resolved. The
_setup_completeflag and double-checked locking pattern (lines 95-97) ensure that_seed_store()runs exactly once per store instance. On subsequentsetup()calls, the outer condition at line 95 prevents re-entry entirely. Theput()calls correctly include metadata as required by coding guidelines. No changes needed.key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (1)
134-149: LGTM! Idempotent collection creation prevents data loss.The
_setup_collectionmethod now checks if the collection exists before creating it (line 145), making it idempotent. This addresses the concern from past reviews about accidental overwrites.key-value/key-value-aio/tests/stores/wrappers/test_default_value.py (3)
23-39: LGTM! Comprehensive async test for default values.The test correctly validates default value behavior with proper use of
awaitfor all async operations. Coverage includes:
- Default values for missing keys
- Default TTL handling
- Batch operations
- Updates overriding defaults
41-91: Skipped tests are correct for wrapper semantics.The skipped base tests are appropriate since the DefaultValueWrapper fundamentally changes the contract for missing keys.
17-21: memory_store fixture is properly available and defined.The verification confirms that the
memory_storefixture is defined inkey-value/key-value-aio/tests/stores/conftest.pyand will be automatically discovered and injected by pytest. The fixture is accessible to all test classes in subdirectories, includingTestDefaultValueWrapper.key-value/key-value-aio/src/key_value/aio/stores/memory/store.py (2)
113-136: LGTM! Constructor correctly documents and passes seed parameter.The constructor properly:
- Documents the seed format with explicit nested structure
- Describes lazy seeding behavior
- Passes the seed parameter to the base class
The type safety issue with
_seedassignment is in the base class (key_value/aio/stores/base.pyline 69), not this file.
138-154: LGTM! Idempotent collection creation with proper seeding setup.The implementation correctly:
- Iterates seed collections in
_setupto ensure they're initialized- Makes
_setup_collectionidempotent by checking cache first (line 150)- Creates collections lazily on first access
The actual seeding (putting key-value pairs) happens via
BaseStore._seed_storewhich callsput(), ensuring entries get propercreated_attimestamps as required by coding guidelines.key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (3)
30-30: LGTM! Type alias properly defines seed structure.The
SEED_DATA_TYPEclearly defines the three-level nested mapping for collections, keys, and values.
90-95: LGTM! Seeding logic ensures proper metadata.The
_seed_storemethod correctly:
- Iterates all seed collections
- Calls
setup_collectionto ensure collection exists- Uses
put()which createsManagedEntrywithcreated_at=now()(line 206)This satisfies the coding guideline requirement to persist ManagedEntry wrappers with value, creation timestamp, and TTL metadata.
97-110: LGTM! One-time seeding protected by double-checked locking.The setup method ensures:
_setup()is called exactly once_seed_store()is called exactly once within the setup lock- Thread-safe initialization
key-value/key-value-aio/src/key_value/aio/wrappers/default_value/wrapper.py (4)
11-24: Well-designed wrapper with clear documentation.The class appropriately uses JSON string storage (
_default_value_json) to enable deep copying via the_new_default_value()helper, preventing mutation of the default value across multiple retrievals. The docstring correctly documents the behavior and notes important TTL considerations for wrapper composition.
26-41: Initialization correctly prepares immutable default value.The JSON serialization approach (line 40) ensures the default value is stored immutably and can be deeply copied on each retrieval via
_new_default_value(). Thedict(default_value)conversion handles arbitrary Mapping types before serialization.
46-72: get and get_many correctly apply default values.Both methods properly delegate to the underlying store and return fresh default copies for missing keys. The ternary expressions (lines 58, 72) correctly handle None results without writing to the store.
74-104: ttl and ttl_many correctly handle default values and TTLs.Both methods properly return
_default_ttlfor missing keys (lines 87, 103), matching the docstring promise of "default TTL". The implementation correctly preserves actual TTL values for existing keys while applying defaults only whenresult is None.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
77-77: Address type safety issue with seed initialization.Line 77 assigns
seed or {}toself._seed, but the helper function expectsSEED_DATA_TYPE. In Basedpyright strict mode, the empty dict literal{}is inferred asdict[Never, Never], which is incompatible withMapping[str, Mapping[str, Mapping[str, Any]]].Apply this diff to fix the type incompatibility:
+from typing import Any, SupportsFloat, cast- self._seed = _seed_to_frozen_seed_data(seed=seed or {}) + self._seed = _seed_to_frozen_seed_data(seed=seed if seed is not None else cast(SEED_DATA_TYPE, {}))Based on past review comments.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
83-83: Same type safety issue as async version.Line 83 has the identical
seed or {}type incompatibility issue as the async version (line 77 in async base.py). Since this file is auto-generated, fix the async version first and regenerate.The async version should use:
self._seed = _seed_to_frozen_seed_data(seed=seed if seed is not None else cast(SEED_DATA_TYPE, {}))Then run
make codegento regenerate this file. As per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py(5 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Use Ruff for Python formatting and linting (configured in pyproject.toml)
Python line length is 140 characters
Use Basedpyright in strict mode for static type checking
Files:
key-value/key-value-aio/src/key_value/aio/stores/base.pykey-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py
key-value/key-value-aio/src/key_value/aio/stores/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
key-value/key-value-aio/src/key_value/aio/stores/**/*.py: Store implementations must persist ManagedEntry wrappers (not raw values) with value, creation timestamp, and TTL metadata
Async store implementations must implement the AsyncKeyValue protocol (key_value/aio/protocols/key_value.py)
Files:
key-value/key-value-aio/src/key_value/aio/stores/base.py
key-value/key-value-sync/**
📄 CodeRabbit inference engine (AGENTS.md)
Never edit files in key-value/key-value-sync/ directly; regenerate from async via make codegen
Files:
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py
key-value/key-value-sync/src/key_value/sync/code_gen/stores/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Sync store files are generated mirrors of async stores; do not hand-edit implementations here
Files:
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py
🧬 Code graph analysis (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (4)
_seed_to_frozen_seed_data(35-41)_seed_store(101-106)setup_collection(122-132)put(212-219)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
key-value/key-value-aio/src/key_value/aio/stores/base.py (4)
_seed_to_frozen_seed_data(32-35)_seed_store(95-100)setup_collection(116-126)put(220-231)
🔇 Additional comments (2)
key-value/key-value-aio/src/key_value/aio/stores/base.py (1)
27-35: LGTM: Seed type definitions and helper function are well-structured.The type aliases clearly define the seed data structure (3 levels: collection → key → value fields), and
_seed_to_frozen_seed_datacorrectly converts mutable mappings into immutableMappingProxyTypewrappers at each level.key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py (1)
30-41: LGTM: Sync version of seed infrastructure matches async implementation.The type definitions and
_seed_to_frozen_seed_datahelper correctly mirror the async version, converting nested mappings to immutableMappingProxyTypestructures.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/base.py
Outdated
Show resolved
Hide resolved
|
Summary
This PR implements four new features requested in issue #115:
Changes
MemoryStore.__init__to acceptseedparameter with synchronous initializationDefaultValueWrapperinkey-value-aio/src/key_value/aio/wrappers/default_value/PydanticAdapter.get()andget_many()withdefaultparameter