Skip to content

Comments

change Payload type to Mapping instead of dict to allow passing TypedDict#1145

Open
Fogapod wants to merge 1 commit intoqdrant:devfrom
Fogapod:payload-mapping
Open

change Payload type to Mapping instead of dict to allow passing TypedDict#1145
Fogapod wants to merge 1 commit intoqdrant:devfrom
Fogapod:payload-mapping

Conversation

@Fogapod
Copy link

@Fogapod Fogapod commented Jan 27, 2026

Currently type checkers (pyright and ty) do not allow this:

class Payload(TypedDict):
    customer_id: str
    ki_id: str
    tokens: int

await qdrant.upsert(
    "my_coll",
    points=[
        PointStruct(
            id=uuid.uuid4().hex,
            vector=vector,
            # works if you do dict(Payload(...))
            payload=Payload(
                customer_id=str(customer_id),
                ki_id=str(ki_id),
                tokens=tokens,
            ),
        )
    ],
)

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 allowed

All Submissions:

  • Contributions should target the dev branch. Did you create your branch from dev?
  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Have you installed pre-commit with pip3 install pre-commit and set up hooks with pre-commit install?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@netlify
Copy link

netlify bot commented Jan 27, 2026

Deploy Preview for poetic-froyo-8baba7 ready!

Name Link
🔨 Latest commit 19c20cc
🔍 Latest deploy log https://app.netlify.com/projects/poetic-froyo-8baba7/deploys/6978ea32e2c1a200086e6a51
😎 Deploy Preview https://deploy-preview-1145--poetic-froyo-8baba7.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai
Copy link

coderabbitai bot commented Jan 27, 2026

📝 Walkthrough

Walkthrough

This change modifies the public Payload type alias in qdrant_client/http/models/models.py from Dict[str, Any] to Mapping[str, Any], and adds the corresponding import from collections.abc. The modification broadens the type contract for payload parameters to accept any mapping-like object rather than strictly dictionary instances.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: converting the Payload type from Dict to Mapping to support TypedDict compatibility.
Description check ✅ Passed The description is directly related to the changeset, providing a concrete example of the problem (TypedDict incompatibility with dict) that the Payload type change solves.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

@coderabbitai coderabbitai bot Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 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.py

Repository: qdrant/qdrant-client

Length of output: 2067


🏁 Script executed:

# Search for Payload type usage and assignments
rg -n --type=py 'Payload\s*=' | head -20

Repository: 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 -30

Repository: 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.py

Repository: 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.py

Repository: 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 -5

Repository: 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 -10

Repository: 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 -3

Repository: 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.py

Repository: 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.py

Repository: 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 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 TypedDict instances without casting, which is the goal
  • The internal code in local_collection.py already converts payloads to dict via deepcopy(to_jsonable_python(payload)) at API boundaries (lines 2256, 2681)
  • The .pop() call on line 2699 operates on self.payload[idx], which is internal storage that's always a concrete dict, not on user-provided payloads

Why this separation is good:

  • Mapping[str, Any] in the public API accepts any mapping (including TypedDict, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant