Update WeaviateDocumentStore authentication to use new Secret class #425
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
silvanocerza
requested review from
davidsbatista
and removed request for
a team
github-actions
bot
added
integration:weaviate
type:documentation
shadeMe
suggested changes
Feb 15, 2024
| """ | ||
|
|
||
| access_token: Secret = field(default_factory=lambda: Secret.from_env_var(["WEAVIATE_ACCESS_TOKEN"])) | ||
| expires_in: Secret = field(default_factory=lambda: Secret.from_env_var(["WEAVIATE_EXPIRES_IN"], strict=False)) |
There was a problem hiding this comment.
This doesn't look like a secret. Let's just make it a regular int that defaults to 60.
| """ | ||
|
|
||
|
|
||
| @dataclass |
There was a problem hiding this comment.
Let's freeze all dataclasses.
| if "type" not in data: | ||
| msg = "Missing 'type' in serialization data" | ||
| raise DeserializationError(msg) | ||
| return _AUTH_CLASSES[data["type"]]._from_dict(data) |
There was a problem hiding this comment.
Let's just move the map into the function.
integrations/weaviate/src/haystack_integrations/document_stores/weaviate/document_store.py
Show resolved
Hide resolved
| """ | ||
| Converts the object to a dictionary representation for serialization. | ||
| """ | ||
| # We assume all fields are Secret instances |
There was a problem hiding this comment.
We should check instead of assuming, especially if the expires_in field is going to be an int (see below.)
Comment on lines
29
to
32
| return default_to_dict( | ||
| self, | ||
| **_fields, | ||
| ) |
There was a problem hiding this comment.
I'd be more comfortable if we used an explicit enum instead of the full type name. That way, it won't break if we move/rename the underlying classes.
shadeMe
approved these changes
Feb 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #369.
Weaviate supports different manner of authentication via some utility dataclasses.
Cause of this it wasn't feasible to use an init parameterer for each possible secret, so we mirrored a bit their utility classes structures to use the
Secretclass.You'll notice that I used a
default_factoryin all dataclasses fields cause we can't usedefaultasSecretis not a frozen dataclass in the latest beta. When that changes we should be able to usedefault.