weaviate / typescript-client

Official Weaviate TypeScript Client
https://www.npmjs.com/package/weaviate-client
BSD 3-Clause "New" or "Revised" License
65 stars 23 forks source link

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

Open tritos-design opened 4 months ago

tritos-design commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 😁