Skip to content

Remove default port to improve security and flexibility for self-hosted instances #83

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ray0907
Copy link

@Ray0907 Ray0907 commented Sep 2, 2024

Key changes:

  • Removed default port assignment in host configuration
  • Modified connection logic to only use a port if explicitly provided
  • Updated relevant functions to handle connections without a specified port
  • Added warning logs when attempting to connect without a port, if appropriate
  • Included unit tests to verify behavior with and without port specification

@generall generall requested a review from Copilot April 23, 2025 16:50
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the default port assignment when using the host parameter to improve security and accommodate self-hosted instances. It updates the constructor logic to only use a port if explicitly provided, reforms the URL parsing behavior, and adjusts the REST endpoint construction accordingly.

Comments suppressed due to low confidence (1)

packages/js-client-rest/src/qdrant-client.ts:81

  • Defaulting to port 6333 in the fallback branch contradicts the PR's aim of removing default port assignment. Consider setting _port to null instead of 6333 when no explicit port is provided.
this._port = port || 6333; // Use default port 6333 if not provided

'Try to use the `url` parameter instead.',
);

if (host) {
Copy link
Preview

Copilot AI Apr 23, 2025

Choose a reason for hiding this comment

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

The validation for the host parameter no longer checks for a port in the host string. To avoid ambiguity, consider stripping or validating any port information embedded in the host if it should be handled exclusively via the port parameter.

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant