Load JSON using pydantic directly in HTTP client#1122
Load JSON using pydantic directly in HTTP client#1122Fogapod wants to merge 1 commit intoqdrant:devfrom
Conversation
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe changes refactor the response parsing mechanism in the API client by replacing dynamic Pydantic model creation with TypeAdapter-based validation. The implementation shifts response deserialization from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
qdrant_client/http/api_client.py (2)
111-130: Missing handler fortype_=Nonecase.The method signature allows
type_to beNone(see overloads at lines 73-79), but line 127 unconditionally callsparse_as_type(response.content, type_). Whentype_isNone, this would attempt to createTypeAdapter(None), which is invalid.Apply this diff to handle the
Nonecase:if response.status_code in [200, 201, 202]: + if type_ is None: + return None try: return parse_as_type(response.content, type_) except ValidationError as e:
200-219: Missing handler fortype_=Nonecase in async path.Same issue as the synchronous
sendmethod: line 216 unconditionally callsparse_as_typeeven whentype_isNone(see overloads at lines 160-169).Apply this diff to handle the
Nonecase:if response.status_code in [200, 201, 202]: + if type_ is None: + return None try: return parse_as_type(response.content, type_) except ValidationError as e:
🧹 Nitpick comments (1)
qdrant_client/http/api_client.py (1)
253-255: Consider optimization for BaseModel types.The caching approach with
lru_cacheis good for performance. However, as noted in the PR description, most target types are alreadyBaseModel. For these cases, you could potentially optimize further by usingmodel_validate_jsondirectly, which may be faster than going throughTypeAdapter.Consider this optimization:
@lru_cache(maxsize=None) def _wrap_in_pydantic_model(type_: T) -> TypeAdapter[T]: return TypeAdapter(type_) def parse_as_type(data: bytes, type_: Type[T]) -> T: # Optimize for BaseModel subclasses try: if isinstance(type_, type) and issubclass(type_, BaseModel): return type_.model_validate_json(data) except TypeError: pass return _wrap_in_pydantic_model(type_).validate_json(data)You would need to import
BaseModel:from pydantic import BaseModel, TypeAdapter, ValidationError
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
qdrant_client/http/api_client.py(4 hunks)
⏰ 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). (3)
- GitHub Check: Redirect rules - poetic-froyo-8baba7
- GitHub Check: Header rules - poetic-froyo-8baba7
- GitHub Check: Pages changed - poetic-froyo-8baba7
🔇 Additional comments (2)
qdrant_client/http/api_client.py (2)
258-259: No issues found - TypeAdapter implementation is correct.Pydantic's TypeAdapter.validate_json() is the recommended approach for validating arbitrary types in pydantic v2. The code properly handles all edge cases mentioned:
- Union types: Fully supported (the codebase contains 75+ Union types including
Union[datetime, date]andUnion[List[X], X])- Optional types: Fully supported (extensively used in response models)
- Custom types: Fully supported (all Pydantic BaseModel subclasses are handled correctly)
The change from
response.json()with dynamiccreate_model()wrapper toresponse.contentwith directTypeAdapter.validate_json()is a correct performance optimization using pydantic v2's intended pattern.
7-7: The original review comment is incorrect.The dependency constraint
pydantic = ">=1.10.8,!=2.0.*,!=2.1.*,!=2.2.0"permits pydantic 2.2.1 and above. The poetry.lock file confirms pydantic 2.12.4 is currently in use, and TypeAdapter is available in this version. The import is valid and compatible.Likely an incorrect or invalid review comment.
|
Looks like |
Partially addresses #714
There were 2 attempts at this: #756 and #902 but they add extra dependency.
Pydantic json parsing is pretty fast. They claim being faster than orjson. Even if they are not there is a benefit of not crossing pyo3 bridge twice.
I used
TypeAdapterinstead of hoops withobjproperty although I'm not sure why this extra layer is needed at all. I checked a few types this is called with and they are allBaseModelalready so you could justmodel_validate_jsonon them. If there are some non-pydantic types i guess something like this could be done:My local pyright shows
8738 errors, 0 warnings, 0 informationsnot sure what to think of that. I did runpoetry installAll Submissions:
devbranch. Did you create your branch fromdev?New Feature Submissions:
pre-commitwithpip3 install pre-commitand set up hooks withpre-commit install?Changes to Core Features: