-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix(qdrant): add URL prefix handling for QdrantClient initialization #5033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
daniel-lxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this fix! The implementation looks good. I've left a few suggestions for edge cases and error handling that might be worth considering.
| host: urlObj.hostname, | ||
| https: useHttps, | ||
| port: port, | ||
| prefix: urlObj.pathname === "/" ? undefined : urlObj.pathname, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that URLs with trailing slashes aren't normalized. Would http://localhost:6333/api/ and http://localhost:6333/api result in different prefix values (/api/ vs /api)? If so, should we consider normalizing the pathname to ensure consistent behavior?
For example:
prefix: urlObj.pathname === "/" ? undefined : urlObj.pathname.replace(/\/$/, '')There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appreciate for you suggestion, I apprently ignored that.
I have revised it and committed.
revised in accordance with the suggestions from @daniel-lxs * RooCodeInc#5033 (comment) * Normalize Qdrant URL pathname * RooCodeInc#5033 (comment) * More robust test cases
The previous commit may not work if there are multiple trailing slashes
daniel-lxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for addressing the suggestions, I tested it and it works!
|
@CW-B-W If you have discord would you be able to message me (hrudolph)? |
Related GitHub Issue
Closes: #5032
Potentially (along with #4992) Resolves: #4092
Description
This PR fixes a bug in the
QdrantVectorStoreconstructor where theQdrantClientwas not correctly configured to handle URLs with path prefixes. Previously, theprefixparameter was not being extracted from the URL's pathname, leading to connection issues for certain Qdrant deployments.The fix involves:
QdrantClientinstantiation insrc/services/code-index/vector-store/qdrant-client.tsto extract thepathnamefrom the parsed URL and pass it as theprefixoption to theQdrantClientconstructor, but only if the pathname is not the root (/).Test Procedure
Manual Verification (Realistic Scenarios):
I have tested with the following type of URLs:
https://example.com/qdranthttps://example.comhttps://example.com:6333http://example.comhttp://example.com:6333Unit Tests (Mocked Environment):
New unit test cases have been added to
src/services/code-index/vector-store/__tests__/qdrant-client.spec.tsunder a new "URL Prefix Handling" describe block. These tests verify:prefixis correctly passed toQdrantClientwhen the URL has a non-root pathname (e.g.,http://localhost:6333/some/path).prefixisundefinedwhen the URL has a root pathname (e.g.,http://localhost:6333/).To run the unit tests:
src/directory:cd src/npm testAll unit tests, including the newly added ones, should pass.
Pre-Submission Checklist
Screenshots / Videos
N/A (Backend logic change)
Documentation Updates
Additional Notes
None.
Get in Touch
Important
Fixes URL prefix handling in
QdrantVectorStoreby correctly settingprefixinQdrantClientinitialization.QdrantVectorStoreconstructor inqdrant-client.tsby extractingpathnamefrom URL and passing it asprefixtoQdrantClientif not root.qdrant-client.spec.tsto verifyprefixis set correctly for non-root path URLs and isundefinedfor root path URLs.This description was created by
for 9b09f08. You can customize this summary. It will automatically update as commits are pushed.