Skip to content
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

Handle existing scheme in host #88

Merged
merged 3 commits into from
Sep 25, 2023

Conversation

thomashacker
Copy link
Contributor

This PR aims to fix #87 and adds a simple check before creating the baseUrl

const version = '/v1';
const baseUri = config.host.startsWith(`${config.scheme}://`)
    ? `${config.host}${version}`
    : `${config.scheme}://${config.host}${version}`;

This allows users to use a host URL with a scheme if it matches the specified scheme.

The PR also adds two unit tests to verify that it constructs the correct URLs.

@weaviate-git-bot
Copy link

To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge.

beep boop - the Weaviate bot 👋🤖

PS:
Are you already a member of the Weaviate Slack channel?

Copy link
Contributor

@tsmith023 tsmith023 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @thomashacker 😁

Shall we make the config.host parameter optional now that users are free to supply it within the host? Then the scheme would be pulled out of config.host by splitting the string on ://. If they don't provide config.scheme and it cannot be found in config.host, then an error is thrown.

I'm wondering if it would ever be confusing for users coming from Python to specify the URL in config.host including the protocol only to then have to specify it in config.scheme also. What do you think?

@thomashacker
Copy link
Contributor Author

I agree,

As far as I can see, the scheme parameter is only being used to build the baseURL for both the HTTPClient and GQLClient. As you suggested, If the scheme is already present in the host, we can make specifying the scheme optional.

I would also move the whole scheme/host processing upwards, I'll look into it!

@thomashacker
Copy link
Contributor Author

  • Made the scheme parameter optional and removed throw error when it doesn't exist
  • Moved schema and host checks to Connection, adding following unit tests:
  1. Host contains scheme and it's the same as specified (Use host only)
  2. Host contains scheme but it's not the same as specified (throw error)
  3. Host contains scheme and scheme was not specified (Use host only)
  4. Host does not contain scheme and no scheme was specified (throw error)
  5. Host does not contain scheme but a scheme was specified (default)
  • Both the HttpClient and GQLClient are now using params.host parameter + adding their prefixes

Copy link
Contributor

@tsmith023 tsmith023 left a comment

Choose a reason for hiding this comment

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

LGTM otherwise :)

src/connection/unit.test.ts Outdated Show resolved Hide resolved
@tsmith023 tsmith023 merged commit 5c60264 into weaviate:main Sep 25, 2023
3 checks passed
@thomashacker thomashacker deleted the feature/handle-scheme-in-host branch September 25, 2023 19:19
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.

Connection fails when user-provided host URL contains the scheme
3 participants