Align New with NewClient

[taigrr]

The New() function doesn't take the Auth options into account, which is confusing, since the two functions (New() and NewClient()) are otherwise identical.

This PR unifies the logic, but preserves the New() function's panic() behavior and return signature for backwards compatibility.

[orca-security-eu]

Orca Security Scan Summary

Status	Check	Issues by priority
Passed	Infrastructure as Code	0   11   9   30	View in Orca
Passed	SAST	0   0   0   0	View in Orca
Passed	Secrets	0   0   0   0	View in Orca
Passed	Vulnerabilities	0   0   0   0	View in Orca

🛡️ The following IaC misconfigurations have been detected

	NAME	FILE
	Container Traffic Not Bound To Host Interface	...recated-api-test.yml	View in code
	Container Traffic Not Bound To Host Interface	...recated-api-test.yml	View in code
	Container Traffic Not Bound To Host Interface	...er-compose-azure.yml	View in code
	Container Traffic Not Bound To Host Interface	...-compose-cluster.yml	View in code
	Container Traffic Not Bound To Host Interface	...-compose-cluster.yml	View in code
	Container Traffic Not Bound To Host Interface	...-compose-okta-cc.yml	View in code
	Container Traffic Not Bound To Host Interface	...mpose-okta-users.yml	View in code
	Container Traffic Not Bound To Host Interface	...cker-compose-wcs.yml	View in code
	Container Traffic Not Bound To Host Interface	...t/docker-compose.yml	View in code
	Healthcheck Not Set	...recated-api-test.yml	View in code
	Healthcheck Not Set	...recated-api-test.yml	View in code
	Restart Policy On Failure Not Set To 5	...recated-api-test.yml	View in code
	Healthcheck Not Set	...er-compose-azure.yml	View in code
	Restart Policy On Failure Not Set To 5	...er-compose-azure.yml	View in code
	Healthcheck Not Set	...-compose-cluster.yml	View in code
	Healthcheck Not Set	...-compose-cluster.yml	View in code
	Restart Policy On Failure Not Set To 5	...-compose-cluster.yml	View in code
	Healthcheck Not Set	...-compose-okta-cc.yml	View in code
	Restart Policy On Failure Not Set To 5	...-compose-okta-cc.yml	View in code
	Healthcheck Not Set	...mpose-okta-users.yml	View in code
	Healthcheck Not Set	...cker-compose-wcs.yml	View in code
	Healthcheck Not Set	...cker-compose-wcs.yml	View in code
	Restart Policy On Failure Not Set To 5	...cker-compose-wcs.yml	View in code
	Healthcheck Not Set	...t/docker-compose.yml	View in code
	Healthcheck Not Set	...t/docker-compose.yml	View in code
...	...	...	...

[taigrr]

I've contributed to verba in the past but if there's anything i need to sign (CLA, etc) please let me know

[weaviate-git-bot]

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?

[taigrr]

i agree

[bevzzz]

Yes, I agree, that is definitely a confusing distinction between the two, and unnecessary by the looks of it. Good catch 👍

I am wondering, however, if this wouldn't be a breaking change, in a way. I.e., someone's New() might now panic because of an authorization issue -- something it didn't do before. WDYT @dirkkul?

NewClient also waits for the server to startup (New doesn't), but that shouldn't be an issue.

[dirkkul]

I am wondering, however, if this wouldn't be a breaking change, in a way. I.e., someone's New() might now panic because of an authorization issue -- something it didn't do before. WDYT @dirkkul?

Could you check what was happening before in this case? I can't remember.

[dirkkul]

I think we should add a deprecation notice to New(), I thought we did that a while back, but apparently not

[bevzzz]

Could you check what was happening before in this case?

As @taigrr mentioned, New() just ignored authorization altogether, so I imagine any unauthorized clients will just be getting errors later when trying to query data.

I think we should add a deprecation notice to New()

Agreed, document and deprecate would be the best way to resolve this. @taigrr would you like to update the PR, or should I just do that in a new one?

[taigrr]

Unfortunately many libraries I've seen are using the New() call and won't work anymore because they throw unauthorized error panics. I've mentioned one here, langchaingo.

I presume they used to work, and the authorization flow here on the weaviate side was changed, and the breaking change is not my PR but rather some change that happened a ways back.

For the sake of those other libraries I'd ask you to consider the change I presented once more, since in their current state they're completely broken, and a go get -u could fix them back to working status.

However, a deprecation notice also makes sense for new users going forward, since calling panic on connect is not great, returning an error is clearly better.

As an aside, the behavior is the same for unauthorized clients. (I apologize for formatting, responding from GitHub mobile)
This code crashes:

        client = weaviate.New(weaviate.Config{
                Host:       weaviateURL,
                Scheme:     "https",
                AuthConfig: auth.ApiKey{Value: os.Getenv(vars.WeaviateAPIKey)},
                Headers: map[string]string{
                        "X-Azure-Api-Key": os.Getenv(vars.AZOAIVectorToken),
                },
        })
  

I provided an AuthConfig, and it was ignored, and the call to New() threw a panic later as a result.

If you still disagree, that's fine, I understand, just close my PR and make the new one. I'd just have to maintain my fork for now.

[bevzzz]

Hey @taigrr, thanks for providing more details here. I'm going to take another look at it tomorrow to see what the best solution would be.

Do I understand you correctly that libs like langchaingo are creating a client using the New() method, which panics immediately because it skips authorization and then cannot create a connection to Weaviate server?

[taigrr]

The User of langchaingo uses the weaviate vector provider, and passes it to an LLM tool array. On first execution calling the tool, the plugin attempts to search, weaviate responds with a 401 unauthorized error when the toolcalls run. Following their example code, this triggers a runtime panic, the usage of the weavaite client is opaque post-initialization.

Additionally, if you try to provision schema in the init() call, which most will do, init() can't return an error, and most people panic here.

New() itself of course doesn't panic, it just creates the client offline without actually connecting

[bevzzz]

hi again @taigrr!
After some deliberation, it seems that the best way to move forward would be to preserve the old code in New() as-is, but have it call NewClient() in the very beginning and return the created Client if there was no error.

This way a Client with a valid Auth config will be correctly authenticated whether it was created with New or NewClient.
In case the Auth config is invalid and NewClient returns an error we will retain the current behavior of getting a 401 when sending a query/request.

func New(c Config) *Client {
    if client, err := NewClient(c); err == nil {
        return client
    }
    // ... current code of New()
}

[taigrr]

That sounds great. I've updated the PR and rebased to match

[bevzzz]

Thank you for your contribution @taigrr 🙌