change Payload type to Mapping instead of dict to allow passing TypedDict#1145
change Payload type to Mapping instead of dict to allow passing TypedDict#1145Fogapod 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. |
📝 WalkthroughWalkthroughThis change modifies the public Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@qdrant_client/http/models/models.py`:
- Line 1: The Payload type alias is too permissive (currently Payload =
Mapping[str, Any]) yet code uses dict-only methods (e.g., .pop()); change the
alias to Payload = Dict[str, Any] and update imports (use typing.Dict, Any) so
all references (the Payload alias in models.py and any usage sites like
local_collection logic that expects dict semantics) reflect concrete dict
behavior; alternatively, if you prefer to keep Mapping, add explicit runtime
conversion/validation at API boundaries to coerce/raise on non-dict payloads —
choose one approach and apply it consistently where Payload is used.
| @@ -1,3 +1,4 @@ | |||
| from collections.abc import Mapping | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find likely in-place mutations of payload objects
rg -n --type=py -C2 '\bpayload\b\s*\[|\bpayload\b\.update\(|\bpayload\b\.setdefault\(|\bpayload\b\.pop\('Repository: qdrant/qdrant-client
Length of output: 13966
🏁 Script executed:
# Also check the actual file to see the Payload definition and imports
head -50 qdrant_client/http/models/models.pyRepository: qdrant/qdrant-client
Length of output: 2067
🏁 Script executed:
# Search for Payload type usage and assignments
rg -n --type=py 'Payload\s*=' | head -20Repository: qdrant/qdrant-client
Length of output: 1290
🏁 Script executed:
# Search for where Payload is actually mutated or modified
rg -n --type=py 'payload\[.*\]\s*=' qdrant_client/ | head -30Repository: qdrant/qdrant-client
Length of output: 470
🏁 Script executed:
# Check the context around the .pop() call and the method signature
sed -n '2690,2710p' qdrant_client/local/local_collection.pyRepository: qdrant/qdrant-client
Length of output: 657
🏁 Script executed:
# Find the method name and its purpose
sed -n '2670,2705p' qdrant_client/local/local_collection.pyRepository: qdrant/qdrant-client
Length of output: 1123
🏁 Script executed:
# Check what self.payload is declared as
rg -n 'self\.payload\s*[:=]' qdrant_client/local/local_collection.py | head -5Repository: qdrant/qdrant-client
Length of output: 115
🏁 Script executed:
# Check if Payload type is actually used as an annotation in constructors/methods that accept user input
rg -n 'payload\s*:\s*Payload' qdrant_client/http/models/models.py | head -10Repository: qdrant/qdrant-client
Length of output: 46
🏁 Script executed:
# Find the import and definition of to_jsonable_python
rg -n 'to_jsonable_python' qdrant_client/local/local_collection.py | head -3Repository: qdrant/qdrant-client
Length of output: 293
🏁 Script executed:
# Check if there are other places where self.payload[idx] is mutated or used with dict-only methods
rg -n 'self\.payload\[.*\]\.' qdrant_client/local/local_collection.pyRepository: qdrant/qdrant-client
Length of output: 115
🏁 Script executed:
# Verify if Payload annotation is used in any other method signature where user input is directly mutated
rg -n 'def.*payload.*Payload' qdrant_client/local/local_collection.pyRepository: qdrant/qdrant-client
Length of output: 46
Type annotation Payload = Mapping[str, Any] is too permissive for the implementation's actual requirements.
The code calls .pop() on payload objects (line 2699 in qdrant_client/local/local_collection.py), which only exists on dict, not on all Mapping implementations. While user-provided payloads are safely converted to dict before storage via deepcopy(to_jsonable_python(...)), the type annotation misleads about what the code actually accepts. Either revert Payload to Dict[str, Any] or ensure payloads that don't support dict-only methods are rejected at input boundaries.
🤖 Prompt for AI Agents
In `@qdrant_client/http/models/models.py` at line 1, The Payload type alias is too
permissive (currently Payload = Mapping[str, Any]) yet code uses dict-only
methods (e.g., .pop()); change the alias to Payload = Dict[str, Any] and update
imports (use typing.Dict, Any) so all references (the Payload alias in models.py
and any usage sites like local_collection logic that expects dict semantics)
reflect concrete dict behavior; alternatively, if you prefer to keep Mapping,
add explicit runtime conversion/validation at API boundaries to coerce/raise on
non-dict payloads — choose one approach and apply it consistently where Payload
is used.
There was a problem hiding this comment.
Unsure whats the course of actions here: call dict() in local_collection code for better typing in main lib or leave it dict. btw pyright reports 8820 type errors after poetry install
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for other payload mutations in local_collection.py
rg -n 'self\.payload\[.*\]\s*=' qdrant_client/local/local_collection.py
# Also check for other dict-specific methods being called on payload
rg -n 'payload\.(pop|popitem|setdefault|clear|update)\(' qdrant_client/local/local_collection.py
# Check if Payload type is used in method signatures in local_collection
rg -n 'def.*payload.*:.*Payload' qdrant_client/local/local_collection.py
Length of output: 246
@Fogapod, I recommend keeping Payload = Mapping[str, Any] for the public API. Here's why:
The current implementation is already correct:
- Your change allows users to pass
TypedDictinstances without casting, which is the goal - The internal code in
local_collection.pyalready converts payloads todictviadeepcopy(to_jsonable_python(payload))at API boundaries (lines 2256, 2681) - The
.pop()call on line 2699 operates onself.payload[idx], which is internal storage that's always a concretedict, not on user-provided payloads
Why this separation is good:
Mapping[str, Any]in the public API accepts any mapping (includingTypedDict,dict, etc.)- Internal storage can be typed more precisely if needed (e.g.,
self.payload: Dict[int, Dict[str, Any]]) - The conversion layer protects internal code from non-dict mappings
Regarding the 8820 pyright errors:
That indicates the codebase isn't fully typed yet, so your PR is an incremental improvement for user-facing ergonomics. You don't need to fix all typing issues at once.
Bottom line: The bot's concern is misplaced—keep Mapping for the API. The internal dict conversion is already in place and working correctly.
🧬 Code Graph Analysis Results
[qdrant_client/http/models/models.py] lines 1-3
-
from collections.abc import Mapping
-
Payload = Mapping[str, Any]
-
This shows the Payload type alias is a Mapping[str, Any], not a plain dict. This is relevant to typing decisions in local_collection code (e.g., whether to rely on Mapping vs Dict) and may inform whether to wrap/convert to dict() for typing consistency.
🧠 Learnings used
Learnt from: joein
Repo: qdrant/qdrant-client PR: 977
File: tests/congruence_tests/test_payload.py:645-651
Timestamp: 2025-05-14T20:18:46.258Z
Learning: In the Qdrant client, there is no separate `models.OverwritePayload` class. The `models.OverwritePayloadOperation` class is designed to accept a `models.SetPayload` object as its `overwrite_payload` parameter.
Currently type checkers (pyright and ty) do not allow this:
Because TypedDict is incompatible with dict for complicated reasons. It works if payload type is Mapping.
Unsure if there are
Mappings which would break serialization and should not be allowedAll 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: