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

Port setting is ignored in weaviate.client (rest settings) #131

Open
tritos-design opened this issue May 2, 2024 · 2 comments
Open

Port setting is ignored in weaviate.client (rest settings) #131

tritos-design opened this issue May 2, 2024 · 2 comments

Comments

@tritos-design
Copy link

tritos-design commented May 2, 2024

In versions 3.0.0-beta.23 and 3.0.0-rc.0 the setting for port in the rest settings seems to be ignored.

If I set the host to http://localhost and port to 8080 I get an error message as soon as I try to interact with the API, e.g. this.client.collection.exists('MyClass')

ERROR error getting User: TypeError: fetch failed
ERROR TypeError: fetch failed

because the client seems to try and establish a connection at http://localhost:80.

When setting host to http://localhost:8080 everything works fine regardless of which port I specify in the port option.

Here is my code:

this.client = await weaviate.client({
	rest: {
		host: process.env.WEAVIATE_HOSTNAME_REST!,
		port: 8080,
		secure: isOnServer() ? true : false,
	},
	grpc: {
		host: process.env.WEAVIATE_HOSTNAME_GRPC!,
		port: 50051,
		secure: isOnServer() ? true : false,
	},
	auth: {
		apiKey: process.env.WEAVIATE_API_KEY!,
	},
});

For grpc the host and port settings are used as expected.

See also: https://forum.weaviate.io/t/fetch-error-between-ts-client-and-weaviate-when-deployed-with-docker-compose-on-windows/2146/4?u=accesspointai

@tritos-design
Copy link
Author

tritos-design commented May 2, 2024

When I leave out the leading http::// in the host setting, everything works fine.
The issue is located in node/esm/index.js, line 91 (3.0.0-rc.0). I propose to change this

host: params.rest.host.startsWith('http')
          ? `${params.rest.host}${params.rest.path || ''}`
          : `${scheme}://${params.rest.host}:${params.rest.port}${params.rest.path || ''}`,

to that

host: params.rest.host.startsWith('http')
          ? `${params.rest.host}:${params.rest.port}${params.rest.path || ''}`
          : `${scheme}://${params.rest.host}:${params.rest.port}${params.rest.path || ''}`,

@tsmith023
Copy link
Contributor

Hi @tritos-design, thanks for flagging this one! It's actually not intended that users provide the host parameter as http://localhost since this is a URL. This is why the client infers that the port must be 80 in such a case.

I will refactor for the next release indicating that the http:// prefix should not be supplied with a warning. To specify whether a connection should be encrypted, the secure boolean should be used instead 😁

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

No branches or pull requests

2 participants