Skip to content

fix(qdrant): resolve URL port handling bug for HTTPS URLs #4992

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

Merged
merged 1 commit into from
Jun 23, 2025

Conversation

benashby
Copy link

@benashby benashby commented Jun 21, 2025

Related GitHub Issue

Closes: #4991

Description

Fixed QdrantVectorStore URL parsing logic that incorrectly appended :6333 to HTTPS URLs.

Key implementation changes:

  • Added parseQdrantUrl() method with protocol-aware port handling
  • HTTPS URLs without explicit ports now preserve default behavior (port 443)
  • HTTP URLs without explicit ports correctly get :6333 appended
  • Added parseHostname() helper for hostname-only inputs
  • Maintains backward compatibility for all existing URL formats

Design choice: Used JavaScript's built-in URL constructor for robust parsing with fallback to hostname detection for malformed inputs.

Test Procedure

Unit Tests: 46 comprehensive tests covering all URL formats:

cd src/services/code-index/vector-store && npm test -- __tests__/qdrant-client.spec.ts

Manual Testing:

  1. Initialize QdrantVectorStore with 'https://qdrant.example.com'
  2. Verify internal URL remains 'https://qdrant.example.com' (not 'https://qdrant.example.com:6333')
  3. Test HTTP URLs still get :6333 appended correctly

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (QdrantVectorStore incorrectly appends :6333 port to HTTPS URLs #4991).
  • Scope: Changes focused solely on URL parsing bug fix.
  • Self-Review: Performed thorough code review.
  • Testing: Added comprehensive test coverage (46 test cases).
  • Documentation Impact: No user-facing documentation updates required.
  • Contribution Guidelines: Read and followed guidelines.

Documentation Updates

  • No documentation updates are required.

Additional Notes

All tests pass, ESLint clean. Fix is backward compatible and handles edge cases including IP addresses, custom ports, and malformed URLs.


Important

Fixes URL parsing in QdrantVectorStore to correctly handle HTTPS URLs without appending default port :6333.

  • Behavior:
    • Fixes URL parsing in QdrantVectorStore to prevent appending :6333 to HTTPS URLs without explicit ports.
    • Adds parseQdrantUrl() in qdrant-client.ts for protocol-aware port handling.
    • HTTP URLs without explicit ports get :6333 appended; HTTPS URLs default to port 443.
    • Adds parseHostname() for hostname-only inputs.
  • Testing:
    • 46 unit tests in qdrant-client.spec.ts for various URL formats and edge cases.
    • Tests include handling of IP addresses, custom ports, and malformed URLs.
  • Misc:
    • Uses JavaScript's URL constructor for robust parsing with fallback for malformed inputs.

This description was created by Ellipsis for fee98ec. You can customize this summary. It will automatically update as commits are pushed.

@benashby benashby requested review from mrubens, cte and jr as code owners June 21, 2025 18:57
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Jun 21, 2025
private parseQdrantUrl(url: string | undefined): string {
// Handle undefined/null/empty cases
if (!url || url.trim() === "") {
return "http://localhost:6333"
Copy link

Choose a reason for hiding this comment

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

Hardcoded default port (6333) is used in multiple places. Consider defining this as a constant for better maintainability.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Jun 21, 2025
@benashby benashby force-pushed the fix/qdrant-url-port-handling branch 2 times, most recently from 4fc728c to 813698a Compare June 21, 2025 19:20
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Jun 21, 2025
Copy link
Collaborator

@mrubens mrubens left a comment

Choose a reason for hiding this comment

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

LGTM if @daniel-lxs agrees!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 21, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Jun 21, 2025
@benashby benashby force-pushed the fix/qdrant-url-port-handling branch from 813698a to be3a84c Compare June 21, 2025 19:32
@benashby
Copy link
Author

Sorry I made a few more changes. Should be good now.

@CW-B-W
Copy link

CW-B-W commented Jun 21, 2025

Hello @benashby,

Would you also use parameter prefix for QdrantClient?

I use Nginx as reverse proxy to connect my Qdrant service, the url used was like https://my-domain/qdrant/.
This setting shall not work without the prefix parameter, as I tried.

(Also QdrantClient doesn't properly set the prefix when parsing url.
ref: qdrant/qdrant-js#104)

Would you refer to my version of fix main...CW-B-W:Roo-Code:fix_qdrant_connection .
I have tried it, it worked for my case.

Thanks!

Copy link
Collaborator

@daniel-lxs daniel-lxs left a comment

Choose a reason for hiding this comment

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

Looks good!

Thank you @benashby!

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Jun 21, 2025
@mrubens mrubens merged commit f39da32 into RooCodeInc:main Jun 23, 2025
13 checks passed
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jun 23, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Jun 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lgtm This PR has been approved by a maintainer PR - Needs Review size:L This PR changes 100-499 lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

QdrantVectorStore incorrectly appends :6333 port to HTTPS URLs
5 participants