[ENH] Add an init method that eagerly reports errors with the tenant or DB #2537
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
AlabasterAxe
force-pushed
the
matt/client-verification
branch
from
100d8d3 to
4b41678
Compare
| @@ -74,10 +74,6 @@ export class AdminClient { | |||
| ...this.api.options.headers, | |||
| ...this.authProvider.authenticate(), | |||
| }; | |||
| this.api.options.headers = { | |||
There was a problem hiding this comment.
These lines are exactly the same as the four above so should have no effect, I think.
clients/js/test/auth.basic.test.ts
Outdated
| expect(version).toMatch(/^[0-9]+\.[0-9]+\.[0-9]+$/); | ||
| }); | ||
|
|
||
| test("it should get the heartbeat without auth needed", async () => { |
There was a problem hiding this comment.
Removed because the validation logic fails the auth check.
There was a problem hiding this comment.
maybe we should keep these routes unprotected if there's test cases making these assertions?
| cloudPort: PORT, | ||
| cloudHost: "http://localhost", | ||
| }); | ||
| export const chromaBasic = () => |
There was a problem hiding this comment.
I've changed these to be factory methods because it seems like there is a race condition between when these are constructed and when the tenant/database becomes available. If we initialize the client in the test, the tenant and database exist by that point and the validation check succeeds.
An alternative approach here would be to always validate before every call but that seems like a lot of unnecessary overhead only to catch the IMO rare case that either the database and tenant are being created simultaneously with the client or the database / tenant is being deleted, in which case, the requests will start failing anyway.
| @@ -1,5 +1,3 @@ | |||
| version: '3.9' | |||
There was a problem hiding this comment.
according to the logs when running the tests, "version is obsolete"
| export const chromaTokenBearer = () => | ||
| new ChromaClient({ | ||
| path: URL, | ||
| auth: { | ||
| provider: "token", | ||
| credentials: "test-token", | ||
| tokenHeaderType: "AUTHORIZATION", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
It looks like the token header type was dropped off of this config in the previous PR but let me know if this configuration is no longer necessary:
AlabasterAxe
force-pushed
the
matt/client-verification
branch
from
4a54ca8 to
60d0ade
Compare
|
Okay, I've stopped calling the validation function for the version and heartbeat methods and switched to calling the validation function lazily so that we don't wind up with an unhandle exception in the case that the user is calling methods that don't require the tenant and database to exist. |
Description of changes
This PR puts some initialization work in an init function, and then awaits that work in every async method so that those issues are reported to callers regardless of which call they make.
Test plan
How are these changes tested?
pytestfor python,yarn testfor js,cargo testfor rustDocumentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?