Generalize PydanticAdapter to support any pydantic-serializable type #250
Conversation
Support list[int], tuple, set, dict, datetime, and other types beyond just BaseModel. Uses TypeForm[T] for proper type checker support.
|
Warning Rate limit exceeded@jlowin has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 13 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughPydanticAdapter now supports any pydantic-serializable type (BaseModel, dataclass, TypedDict, dict, sequences, primitives). Non-dict-serializable values are stored wrapped as {"items": ...}. Internal helpers and state were added and Changes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.pykey-value/key-value-aio/src/key_value/aio/adapters/pydantic/base.pykey-value/key-value-aio/tests/adapters/test_pydantic.py
🧰 Additional context used
🧬 Code graph analysis (3)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/base.py (5)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (1)
_get_model_type_name(97-99)key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/base.py (1)
_get_model_type_name(36-42)key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (1)
_get_model_type_name(53-55)key-value/key-value-sync/src/key_value/sync/code_gen/adapters/dataclass/adapter.py (1)
_get_model_type_name(69-71)key-value/key-value-shared/src/key_value/shared/errors/key_value.py (1)
DeserializationError(14-15)
key-value/key-value-aio/tests/adapters/test_pydantic.py (3)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (1)
PydanticAdapter(16-99)key-value/key-value-sync/src/key_value/sync/code_gen/adapters/pydantic/adapter.py (1)
PydanticAdapter(17-55)key-value/key-value-aio/src/key_value/aio/adapters/pydantic/base.py (4)
put(199-208)get(126-126)get(129-129)get(131-158)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (2)
key-value/key-value-aio/src/key_value/aio/protocols/key_value.py (1)
AsyncKeyValue(185-190)key-value/key-value-aio/src/key_value/aio/adapters/pydantic/base.py (1)
BasePydanticAdapter(18-268)
🪛 GitHub Actions: Run Tests
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py
[error] 57-57: PLR0911 Too many return statements (8 > 6)
[error] 80-80: SIM103 Return the negated condition directly
🔇 Additional comments (13)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/base.py (3)
27-27: LGTM: Field rename correctly generalizes wrapping logic.The rename from
_is_list_modelto_needs_wrappingaccurately reflects the expanded type support. All references consistently use the new name.Also applies to: 59-59, 117-117
44-46: LGTM: Docstrings accurately describe the wrapping strategy.The updated docstrings clearly explain the distinction between dict-serializing types (stored directly) and wrapped types (stored as
{"items": value}).Also applies to: 102-105
62-62: LGTM: Error messaging generalized for broader type support.Removing "list" from error messages and adding structured logging context correctly supports the expanded type coverage.
Also applies to: 67-74
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (5)
1-8: LGTM: Type system changes enable generic alias support.The shift from
type[T]toTypeForm[T]and the unboundTypeVarcorrectly enable parameterization with generic aliases likelist[int]. The new imports support the expanded type introspection logic.Also applies to: 13-13, 36-36
17-28: LGTM: Documentation clearly describes expanded type support.The docstring accurately lists all supported type categories and explains the wrapping convention.
Also applies to: 44-50
52-55: LGTM: Initialization correctly delegates wrapping decision.Creating the
TypeAdapterand computing_needs_wrappingvia the helper method is the right approach.
64-68: LGTM: Recursive Annotated handling is correct.Unwrapping
Annotated[T, ...]to the underlying type ensures proper wrapping detection for annotated types.
71-95: LGTM: Wrapping logic correctly distinguishes dict-serializable types.The method correctly identifies:
- Dict-serializable (no wrap): BaseModel, dataclass, TypedDict, dict, Mapping
- Needs wrapping: sequences, primitives, and all other types
The defensive
isinstanceandtry/exceptguards prevent TypeErrors from introspection edge cases.key-value/key-value-aio/tests/adapters/test_pydantic.py (5)
124-128: LGTM: Test validates list[int] support.Correctly verifies round-trip behavior for a simple list of primitives.
227-232: LGTM: Test validates tuple support.Correctly tests fixed-length tuple serialization with mixed types.
234-239: LGTM: Test validates set support.Correctly verifies set round-trip behavior.
241-247: LGTM: Test validates datetime support.Correctly tests primitive type wrapping with a timezone-aware datetime.
249-262: LGTM: Test validates dict storage without wrapping.Excellent test that not only verifies round-trip behavior but also inspects raw storage to confirm dict types are stored directly without the
{"items": ...}wrapper.
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py
Outdated
Show resolved
Hide resolved
Test Failure AnalysisSummary: The workflow failed due to 2 Ruff linting violations in the Root Cause: The new
Suggested Solution: Fix SIM103 ViolationIn # Current code (lines 80-83):
if is_typeddict(pydantic_model): # pyright: ignore[reportUnknownArgumentType]
return False
# Everything else: int, str, datetime, UUID, Enum, etc.
return TrueWith: # Simplified version:
# TypedDict serializes to dict; everything else needs wrapping
return not is_typeddict(pydantic_model) # pyright: ignore[reportUnknownArgumentType]Fix PLR0911 Violation (Too Many Return Statements)You have two options: Option 1: Disable the rule for this specific method by adding a comment: def _check_needs_wrapping(self, pydantic_model: Any) -> bool: # noqa: PLR0911
"""Determine if this type serializes to a non-dict and needs wrapping.Option 2: Refactor to reduce return statements (more complex, but cleaner): Important Notes
Detailed AnalysisThe static analysis job found these violations: The codegen check failed because after generating the sync library, Relevant log excerpt: Related Files
|
Test Failure Analysis - UpdatedSummary: The codegen_check job failed because the sync library wasn't regenerated after the latest changes to the async library. Root Cause: After fixing the linting issues (PLR0911, SIM103) by refactoring the code, the author committed the changes to the async library () but forgot to run Suggested Solution: Run the following commands locally and commit the generated changes: make codegen
git add key-value/key-value-sync/
git commit -m "Regenerate sync library after pydantic adapter changes"
git pushImportant: The async-first development workflow requires:
Alternatively, run Detailed AnalysisThe CI workflow runs these steps:
After codegen ran in CI, it detected uncommitted changes in:
The changes include:
These are the exact changes made to the async library in the latest commit, properly converted to sync code. Related FilesSource files (async library):
Generated files (sync library):
Build script:
|
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/adapters/pydantic/adapter.py (1)
94-96: Consider more accurate type name for error messages.Now that the adapter supports more than BaseModel subclasses (dataclasses, TypedDict, primitives, etc.), returning "Pydantic model" is slightly misleading. Consider deriving a more specific name from
self._type_adapterfor clearer error messages, e.g., usingtype(self._type_adapter).__name__or similar.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py
🧰 Additional context used
🧬 Code graph analysis (1)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (1)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/base.py (1)
BasePydanticAdapter(18-268)
🔇 Additional comments (5)
key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py (5)
1-9: LGTM: Imports support expanded type handling.All new imports (contextlib, Mapping, is_dataclass, type introspection utilities, TypeForm) are necessary for the generalized adapter and properly sourced from standard library or typing_extensions.
14-14: LGTM: Unbound TypeVar enables broader type support.Removing the BaseModel bound is necessary to support the expanded range of pydantic-serializable types (dataclasses, TypedDict, primitives, sequences).
31-56: LGTM: Initialization correctly sets up TypeAdapter and wrapping logic.The
TypeForm[T]parameter type properly supports generic aliases, and all required base class fields are initialized correctly. The@bear_spraydecorator usage is appropriately documented.
58-72: LGTM: Correct wrapping detection with Annotated unwrapping.The logic properly handles
Annotatedtypes by recursively unwrapping them and correctly returns the negation of_serializes_to_dictto determine wrapping needs.
74-92: Logic is correct; past SIM103 feedback appears addressed.The type-checking logic correctly identifies dict-serializable types (BaseModel, dataclass, TypedDict, dict, Mapping). The current implementation at line 83 (
return is_typeddict(pydantic_model)) is already in simplified form, addressing the past SIM103 warning mentioned in previous reviews.The method has 6 return statements, meeting the PLR0911 threshold, but the early-return pattern is clear and appropriate for type discrimination logic.
| """Adapter around a KVStore-compliant Store that allows type-safe persistence of any pydantic-serializable type. | ||
| # Beartype cannot handle the parameterized type annotation (type[T]) used here for this generic adapter. | ||
| # Using @bear_spray to bypass beartype's runtime checks for this specific method. | ||
| Supports: | ||
| - Pydantic models (BaseModel subclasses) | ||
| - Dataclasses and TypedDicts | ||
| - Dict types (dict[K, V]) | ||
| - Sequences (list[T], tuple[T, ...], set[T]) | ||
| - Primitives and other types (int, str, datetime, UUID, etc.) | ||
| Types that serialize to dicts (BaseModel, dataclass, TypedDict, dict) are stored directly. | ||
| All other types are wrapped in {"items": <value>} for consistent dict-based storage. | ||
| """ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clear docstring documenting expanded capabilities.
The docstring comprehensively lists supported types and explains the wrapping logic. One optional enhancement: consider mentioning that Annotated[T, ...] types are unwrapped to T before processing.
🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/adapters/pydantic/adapter.py around
lines 18 to 29, update the module docstring to mention that
typing.Annotated[...] types are unwrapped to their underlying T before
processing; explicitly state that Annotated[T, ...] will be treated the same as
T for the purposes of the type checks and wrapping logic (i.e., treated as
model/dict types when T is such, or wrapped in {"items": value} when T is a
primitive/other), keeping the rest of the listed supported types and behavior
unchanged.
Add required spaces to table separators to pass MD060 linting.
Run codegen to sync PydanticAdapter changes.
|
Fixes type errors when upgrading to ty 0.0.5: - Add base_url validation to auth providers (AWS, Azure, Discord, GitHub, Google, WorkOS) - Add partial[] to tool/prompt decorator return types - Add type ignores for PydanticAdapter generic aliases (pending strawgate/py-key-value#250) - Add type ignores for Depends() in tests (type checker doesn't understand DI unwrapping)
1 participant
The
PydanticAdapterwas previously limited toBaseModelsubclasses andlist[BaseModel]. This change generalizes it to support any type that Pydantic can serialize, including:list[int],tuple[str, ...],set[float]dict[str, int]datetime,UUID,int,strTypes that naturally serialize to dicts (
BaseModel,dataclass,TypedDict,dict) are stored directly. All other types are wrapped in{"items": <value>}for consistent dict-based storage.Uses
TypeForm[T]fromtyping_extensionsto properly type thepydantic_modelparameter, enabling type checkers to accept generic aliases likelist[Tool].This change is required for fastmcp's caching middleware, which uses
PydanticAdapter[list[Tool]],PydanticAdapter[list[Resource]], etc.Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.