fix(remote): skip default port when URL path implies a reverse proxy#1215
Open
xodn348 wants to merge 1 commit into
Open
fix(remote): skip default port when URL path implies a reverse proxy#1215xodn348 wants to merge 1 commit into
xodn348 wants to merge 1 commit into
Conversation
When a user supplies a URL such as https://proxy.example.com/qdrant (a reverse-proxy endpoint with an implicit port), the client was appending the default Qdrant port 6333, turning it into https://proxy.example.com:6333/qdrant — an address that typically does not resolve. Root cause: the fallback `self._port = self._port if self._port else port` treated a URL-embedded path the same as a bare host, so the 6333 default was applied even when the caller had not provided an explicit port number. Fix: when `parsed_url.port` is absent but `parsed_url.path` is present the URL already contains routing information (the path) that implies the proxy owns the port. In that case leave `self._port` as `None` so the scheme's standard port (80/443) is used instead of 6333. URLs with an explicit port in the path position (https://proxy.example.com:8443/qdrant) are unaffected — the explicit port still wins. Fixes qdrant#1146
✅ Deploy Preview for poetic-froyo-8baba7 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThis PR fixes URL path handling in the QdrantRemote client for reverse-proxy deployments. The Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@qdrant_client/qdrant_remote.py`:
- Around line 107-113: The port-assignment logic in qdrant_remote.py uses
parsed_url.path truthiness which treats a root path "/" as a reverse-proxy path
and skips applying the default Qdrant port; update the assignment of self._port
(the line using parsed_url.path) so that it only treats non-root subpaths as
proxy paths (i.e., consider parsed_url.path != '/' before choosing None),
otherwise apply the default port variable when no explicit port was provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8521ffa2-f72e-4164-b5d9-e0d0ebe94703
📒 Files selected for processing (2)
qdrant_client/qdrant_remote.pytests/test_qdrant_client.py
Comment on lines
+107
to
+113
| # When the URL contains a path (e.g. https://proxy.example.com/qdrant) the user | ||
| # is connecting through a reverse proxy that already handles the port. Forcing the | ||
| # default Qdrant port (6333) onto such a URL produces an incorrect address like | ||
| # https://proxy.example.com:6333/qdrant. Only apply the default port when the URL | ||
| # has no path component, which is the plain "host-only" form where the default port | ||
| # is meaningful. | ||
| self._port = self._port if self._port else (None if parsed_url.path else port) |
There was a problem hiding this comment.
Handle root path ("/") as host-only when applying default port.
On Line 113, parsed_url.path truthiness makes http(s)://host/ skip the default port, which can incorrectly route to 80/443 instead of Qdrant’s default port. Treat only real subpaths as reverse-proxy paths.
Suggested fix
- self._port = self._port if self._port else (None if parsed_url.path else port)
+ has_subpath = parsed_url.path not in (None, "", "/")
+ self._port = self._port if self._port else (None if has_subpath else port)📝 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.
Suggested change
| # When the URL contains a path (e.g. https://proxy.example.com/qdrant) the user | |
| # is connecting through a reverse proxy that already handles the port. Forcing the | |
| # default Qdrant port (6333) onto such a URL produces an incorrect address like | |
| # https://proxy.example.com:6333/qdrant. Only apply the default port when the URL | |
| # has no path component, which is the plain "host-only" form where the default port | |
| # is meaningful. | |
| self._port = self._port if self._port else (None if parsed_url.path else port) | |
| # When the URL contains a path (e.g. https://proxy.example.com/qdrant) the user | |
| # is connecting through a reverse proxy that already handles the port. Forcing the | |
| # default Qdrant port (6333) onto such a URL produces an incorrect address like | |
| # https://proxy.example.com:6333/qdrant. Only apply the default port when the URL | |
| # has no path component, which is the plain "host-only" form where the default port | |
| # is meaningful. | |
| has_subpath = parsed_url.path not in (None, "", "/") | |
| self._port = self._port if self._port else (None if has_subpath else port) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@qdrant_client/qdrant_remote.py` around lines 107 - 113, The port-assignment
logic in qdrant_remote.py uses parsed_url.path truthiness which treats a root
path "/" as a reverse-proxy path and skips applying the default Qdrant port;
update the assignment of self._port (the line using parsed_url.path) so that it
only treats non-root subpaths as proxy paths (i.e., consider parsed_url.path !=
'/' before choosing None), otherwise apply the default port variable when no
explicit port was provided.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All Submissions:
devbranch. Did you create your branch fromdev? (Note: this repo has onlymaster; branched from that)New Feature Submissions:
N/A — this is a bug fix, not a new feature.
Changes to Core Features:
Summary
When a user connects to Qdrant through a reverse proxy at a sub-path URL —
for example
https://proxy.example.com/qdrant— the client was incorrectlyappending the default Qdrant port 6333, producing
https://proxy.example.com:6333/qdrant. That address does not resolve onstandard proxies because the proxy listens on port 443 and routes by path, not
by Qdrant's internal port. The result is a
ConnectionErroron every request.Root cause — one line in
QdrantRemote.__init__:This fallback applied the default port 6333 regardless of whether the URL
already contained a path. A URL with a sub-path implies a reverse-proxy setup
where the port is determined by the scheme (80/443), not by Qdrant's default.
Fix — only apply the default port when
parsed_url.pathis absent (bare-host form). When a path is present but no explicit port is given,
self._portis set to
Noneso the scheme's standard port is used instead.URLs with an explicit port in the URL (e.g.
:8443) are unaffected.Issue
Fixes #1146
Local verification
Risk
Low. The change is one line in the
url=code path ofQdrantRemote.__init__,affecting only the case where a URL contains a path but no explicit port (which
was always broken and untested). All existing tests that supply an explicit port
(e.g.
http://localhost:6333) continue to use that port unchanged. Two newregression assertions were added to
test_client_initto cover both theno-port-with-path and the explicit-port-with-path cases.