Conversation
❌ Deploy Preview for poetic-froyo-8baba7 failed.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe PR introduces context-local header propagation to the Qdrant client. A new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qdrant_client/connection.py`:
- Around line 176-177: The metadata list is appended with get_context_headers()
causing duplicate keys; update both the sync interceptor block (where metadata
is built from new_metadata/client_call_details.metadata and appended with
get_context_headers()) and the async interceptor block to first remove any
existing entries whose key matches a context header key, then extend the
metadata with context headers; specifically, in the places that reference
metadata, new_metadata, client_call_details.metadata and get_context_headers(),
filter out entries with keys present in get_context_headers().keys() (preserving
order for remaining entries) before adding the context headers so context
headers deterministically overwrite conflicting keys.
In `@qdrant_client/context_headers.py`:
- Around line 5-9: _context_headers currently uses a mutable default dict and
get_context_headers returns the internal dict directly, risking cross-context
mutation; change the ContextVar declaration to use a default of None (or no
default) and update all setters/getters (e.g., get_context_headers,
set_context_headers, and any clear_context_headers) to treat the stored value as
optional, always store copies when calling _context_headers.set(...) and have
get_context_headers return a shallow copy (or an empty dict copy) rather than
the internal object so external mutation cannot taint the ContextVar across
contexts; apply the same copy-on-set/copy-on-get approach to the other related
functions referenced in the diff.
- Around line 34-43: The two middleware functions rest_headers_middleware and
async_rest_headers_middleware lack type annotations and fail mypy; update their
signatures to include explicit types: annotate the request parameter as
starlette.requests.Request (or the project's Request alias), annotate call_next
as Callable[[Request], Response] for the sync middleware and Callable[[Request],
Awaitable[Response]] for the async middleware, and set return types to Response
and Awaitable[Response] respectively; import the needed types (Request,
Response, Callable, Awaitable) and adjust any existing imports accordingly so
both function definitions are fully typed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 221adc5e-726a-4bd7-8f7d-4f526ba5f5c4
📒 Files selected for processing (7)
qdrant_client/__init__.pyqdrant_client/async_qdrant_remote.pyqdrant_client/connection.pyqdrant_client/context_headers.pyqdrant_client/qdrant_remote.pytests/test_tracing.pytools/async_client_generator/remote_generator.py
| for key, value in get_context_headers().items(): | ||
| metadata.append((key, value)) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Python grpcio client metadata, if the same metadata key is added multiple times, are all values transmitted and is there a defined precedence order for server-side consumption?
💡 Result:
Yes—all values are transmitted.
- In grpcio (Python), call metadata is represented as an ordered sequence of
(key, value)pairs, and the same key may appear multiple times (i.e., it’s a multimap, not a dict). (grpc.github.io) - On the wire, gRPC metadata is carried in HTTP/2 headers, and while general header order isn’t guaranteed, the gRPC over HTTP/2 protocol states that ordering is preserved for values with duplicate header names. (grpc.github.io)
Precedence: gRPC does not define a “winner” (no first-wins/last-wins rule) for server-side consumption. Servers receive repeated entries; any precedence is application/framework-specific (e.g., if you convert metadata to a map/dict yourself, you may implicitly drop duplicates and end up with “last one wins,” but that’s not a gRPC guarantee). (grpc.github.io)
Citations:
- 1: https://grpc.github.io/grpc/python/_modules/grpc/aio/_metadata.html?utm_source=openai
- 2: https://grpc.github.io/grpc/core/md_doc__p_r_o_t_o_c_o_l-_h_t_t_p2.html?utm_source=openai
- 3: https://grpc.github.io/grpc/python/_modules/grpc/aio/_metadata.html?utm_source=openai
🏁 Script executed:
cat -n qdrant_client/connection.py | sed -n '170,185p'Repository: qdrant/qdrant-client
Length of output: 858
🏁 Script executed:
cat -n qdrant_client/connection.py | sed -n '230,250p'Repository: qdrant/qdrant-client
Length of output: 1043
🏁 Script executed:
cd qdrant_client && grep -n "get_context_headers" connection.pyRepository: qdrant/qdrant-client
Length of output: 249
🏁 Script executed:
rg "get_context_headers" --type py -B 3 -A 3Repository: qdrant/qdrant-client
Length of output: 8140
🏁 Script executed:
rg "metadata" tests/test_tracing.py -B 2 -A 2 | head -50Repository: qdrant/qdrant-client
Length of output: 1895
🏁 Script executed:
grep -n "metadata" qdrant_client/connection.py | head -20Repository: qdrant/qdrant-client
Length of output: 1157
🏁 Script executed:
# Check if there's a test case for metadata conflicts (context headers + new_metadata with same key)
grep -A 20 "test.*metadata" tests/test_tracing.py | grep -E "new_metadata|conflict|duplicate" || echo "No conflict tests found"Repository: qdrant/qdrant-client
Length of output: 87
🏁 Script executed:
# Check what new_metadata contains when passed to interceptors
grep -B 5 -A 10 "new_metadata" qdrant_client/connection.py | head -40Repository: qdrant/qdrant-client
Length of output: 1292
Ensure context headers deterministically overwrite conflicting metadata keys in gRPC calls.
At lines 176–177 and 238–239, context headers are appended directly to the metadata list. If new_metadata or client_call_details.metadata already contains a key present in context headers (e.g., x-tracing-id), duplicate entries are sent to the server. Since gRPC transmits all duplicate values with undefined precedence, this creates ambiguous behavior.
The fix should remove existing metadata entries with conflicting keys before extending with context headers:
Proposed solution
- for key, value in get_context_headers().items():
- metadata.append((key, value))
+ context_headers = get_context_headers()
+ if context_headers:
+ context_keys = {key.lower() for key in context_headers}
+ metadata = [
+ (key, value) for key, value in metadata if key.lower() not in context_keys
+ ]
+ metadata.extend(context_headers.items())Apply to both sync (lines 176–177) and async (lines 238–239) interceptors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for key, value in get_context_headers().items(): | |
| metadata.append((key, value)) | |
| context_headers = get_context_headers() | |
| if context_headers: | |
| context_keys = {key.lower() for key in context_headers} | |
| metadata = [ | |
| (key, value) for key, value in metadata if key.lower() not in context_keys | |
| ] | |
| metadata.extend(context_headers.items()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qdrant_client/connection.py` around lines 176 - 177, The metadata list is
appended with get_context_headers() causing duplicate keys; update both the sync
interceptor block (where metadata is built from
new_metadata/client_call_details.metadata and appended with
get_context_headers()) and the async interceptor block to first remove any
existing entries whose key matches a context header key, then extend the
metadata with context headers; specifically, in the places that reference
metadata, new_metadata, client_call_details.metadata and get_context_headers(),
filter out entries with keys present in get_context_headers().keys() (preserving
order for remaining entries) before adding the context headers so context
headers deterministically overwrite conflicting keys.
| _context_headers: ContextVar[dict[str, str]] = ContextVar("_context_headers", default={}) | ||
|
|
||
|
|
||
| def get_context_headers() -> dict[str, str]: | ||
| return _context_headers.get() |
There was a problem hiding this comment.
Avoid mutable ContextVar default and returning mutable internal state.
Line 5 uses a mutable default dict, and Line 9 returns the internal object directly. External mutation can leak/taint header state across contexts.
🔧 Proposed fix
from contextlib import asynccontextmanager, contextmanager
from contextvars import ContextVar
-from typing import AsyncIterator, Iterator
+from typing import AsyncIterator, Iterator
-_context_headers: ContextVar[dict[str, str]] = ContextVar("_context_headers", default={})
+_context_headers: ContextVar[dict[str, str] | None] = ContextVar(
+ "_context_headers", default=None
+)
def get_context_headers() -> dict[str, str]:
- return _context_headers.get()
+ return dict(_context_headers.get() or {})
@@
def headers(extra_headers: dict[str, str]) -> Iterator[None]:
- current = _context_headers.get()
+ current = _context_headers.get() or {}
merged = {**current, **extra_headers}
@@
async def async_headers(extra_headers: dict[str, str]) -> AsyncIterator[None]:
- current = _context_headers.get()
+ current = _context_headers.get() or {}
merged = {**current, **extra_headers}Also applies to: 14-16, 25-27
🧰 Tools
🪛 Ruff (0.15.5)
[warning] 5-5: Do not use mutable data structures for ContextVar defaults
(B039)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qdrant_client/context_headers.py` around lines 5 - 9, _context_headers
currently uses a mutable default dict and get_context_headers returns the
internal dict directly, risking cross-context mutation; change the ContextVar
declaration to use a default of None (or no default) and update all
setters/getters (e.g., get_context_headers, set_context_headers, and any
clear_context_headers) to treat the stored value as optional, always store
copies when calling _context_headers.set(...) and have get_context_headers
return a shallow copy (or an empty dict copy) rather than the internal object so
external mutation cannot taint the ContextVar across contexts; apply the same
copy-on-set/copy-on-get approach to the other related functions referenced in
the diff.
2 participants
Depends on: qdrant/qdrant#8402
AI generated:
Manually tested with:
Details