Conversation
Signed-off-by: Anush008 <[email protected]>
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an optional Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 unit tests (beta)
📝 Coding Plan
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
🧹 Nitpick comments (1)
qdrant_client/async_qdrant_client.py (1)
107-112:headerswill be stored ininit_options(consistent with existing sensitive params).Custom headers (which may contain sensitive values like authentication tokens) are captured in
_init_optionsand exposed via the publicinit_optionsproperty. This is consistent with howapi_keyandauth_token_providerare already handled.No change is required, but users should be aware that
init_optionsmay contain sensitive data and should not be logged or serialized without redaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qdrant_client/async_qdrant_client.py` around lines 107 - 112, The constructor currently stores the incoming headers parameter in self._init_options which is exposed via the public init_options property, so sensitive header values (e.g., auth tokens) can leak if init_options is logged or serialized; either avoid exposing raw headers by removing or redacting the "headers" key from self._init_options before assignment or implement redaction in the init_options property getter (reference symbols: the constructor that populates _init_options, the _init_options dict itself, and the public init_options property) and ensure any logging or serialization paths use the redacted form.
🤖 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/async_qdrant_remote.py`:
- Around line 120-123: The headers merge currently applies user-provided headers
before the client sets "api-key" and "User-Agent", which silently overwrites
user values; update the merging logic in the async_qdrant_remote constructor
(the block that writes to self._rest_headers and self._grpc_headers) to
explicitly handle reserved keys: define a reserved set containing "api-key" and
"User-Agent" and either (A) prevent overrides by checking headers for any
reserved keys and raising a clear ValueError if present, or (B) allow overrides
by applying user headers after the client sets its defaults (i.e., set defaults
first then merge headers), and add matching behavior to the synchronous
implementation; also update the parameter docstring to state the chosen behavior
for reserved headers.
---
Nitpick comments:
In `@qdrant_client/async_qdrant_client.py`:
- Around line 107-112: The constructor currently stores the incoming headers
parameter in self._init_options which is exposed via the public init_options
property, so sensitive header values (e.g., auth tokens) can leak if
init_options is logged or serialized; either avoid exposing raw headers by
removing or redacting the "headers" key from self._init_options before
assignment or implement redaction in the init_options property getter (reference
symbols: the constructor that populates _init_options, the _init_options dict
itself, and the public init_options property) and ensure any logging or
serialization paths use the redacted form.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2406679c-962f-42c5-a86a-fa49c0f32aa0
📒 Files selected for processing (4)
qdrant_client/async_qdrant_client.pyqdrant_client/async_qdrant_remote.pyqdrant_client/qdrant_client.pyqdrant_client/qdrant_remote.py
| if headers: | ||
| for key, value in headers.items(): | ||
| self._rest_headers[key] = value | ||
| self._grpc_headers.append((key, value)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine both async and sync implementations of QdrantRemote
fd -t py ".*qdrant_remote" | head -20Repository: qdrant/qdrant-client
Length of output: 233
🏁 Script executed:
# Search for header handling patterns in both files
rg -n "headers" --type=py -B 3 -A 3 qdrant_client/async_qdrant_remote.py qdrant_client/qdrant_remote.pyRepository: qdrant/qdrant-client
Length of output: 10040
🏁 Script executed:
# Get more context around lines 120-123 in the async file
cat -n qdrant_client/async_qdrant_remote.py | sed -n '110,140p'Repository: qdrant/qdrant-client
Length of output: 1924
🏁 Script executed:
# Also check the sync version structure
cat -n qdrant_client/qdrant_remote.py | sed -n '110,140p'Repository: qdrant/qdrant-client
Length of output: 1719
Document or reconsider the immutability of api-key and User-Agent headers.
Custom headers provided via the headers parameter are merged before api-key (line 131) and User-Agent (line 136) are set. If a user supplies these keys, they will be silently overwritten. This same pattern exists in the synchronous version, suggesting it is intentional to prevent users from breaking authentication or client identification.
If preventing user override is the intended behavior (to maintain critical functionality), add explicit documentation to the parameter description. If users should be able to customize these values, either apply custom headers last or raise an error if users attempt to override reserved keys.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qdrant_client/async_qdrant_remote.py` around lines 120 - 123, The headers
merge currently applies user-provided headers before the client sets "api-key"
and "User-Agent", which silently overwrites user values; update the merging
logic in the async_qdrant_remote constructor (the block that writes to
self._rest_headers and self._grpc_headers) to explicitly handle reserved keys:
define a reserved set containing "api-key" and "User-Agent" and either (A)
prevent overrides by checking headers for any reserved keys and raising a clear
ValueError if present, or (B) allow overrides by applying user headers after the
client sets its defaults (i.e., set defaults first then merge headers), and add
matching behavior to the synchronous implementation; also update the parameter
docstring to state the chosen behavior for reserved headers.
There was a problem hiding this comment.
@Anush008 I actually agree with the coderabbit points, I think we might address those
I can do it if you'd like me to
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
* feat: Add support for custom headers Signed-off-by: Anush008 <[email protected]> * add warnings when user provides user-agent or api-key in headers or options * fix: fix is none condition --------- Signed-off-by: Anush008 <[email protected]> Co-authored-by: George Panchuk <[email protected]>
2 participants
All Submissions:
devbranch. Did you create your branch fromdev?New Feature Submissions:
pre-commitwithpip3 install pre-commitand set up hooks withpre-commit install?