fix: can instantiate connectors without callbacks/secrets-keeper (TCTC-10081) #1894
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
Fanaen
approved these changes
Feb 5, 2025
| def _call_retrieve_token(self) -> str: | ||
| """Retrieve the access token for Google Sheets | ||
|
|
||
| Raise an GoogleSheetsInvalidConfiguration if something missing to retrieve the token |
There was a problem hiding this comment.
lukapeschke
requested changes
Feb 5, 2025
| name="test_connector", | ||
| auth_id="test_auth_id", | ||
| ) | ||
| assert gsheet_connector |
There was a problem hiding this comment.
Could you make an assertion on the fields here ? For example on the token callback ?
| assert gsheet_connector | ||
|
|
||
|
|
||
| def test_raise_when_trying_to_retrieve_token_if_callable_missing(): |
There was a problem hiding this comment.
Suggested change
| def test_raise_when_trying_to_retrieve_token_if_callable_missing(): | |
| def test_raises_when_trying_to_retrieve_token_if_callable_missing(): |
| @@ -234,3 +246,20 @@ def test_get_refresh_token(mocker, oauth2_connector): | |||
| mocked_load.return_value = {"refresh_token": "bla"} | |||
| token = oauth2_connector.get_refresh_token() | |||
| assert token == "bla" | |||
|
|
|||
|
|
|||
| def test_raise_error_if_secret_keeper_not_set(oauth2_connector_without_secret_keeper: OAuth2Connector): | |||
There was a problem hiding this comment.
Suggested change
| def test_raise_error_if_secret_keeper_not_set(oauth2_connector_without_secret_keeper: OAuth2Connector): | |
| def test_raises_exception_if_secret_keeper_not_set(oauth2_connector_without_secret_keeper: OAuth2Connector): |
Comment on lines
107
to
109
| def __init__(self, retrieve_token: Callable[[str, str], str] | None = None, *args, **kwargs): | ||
| super().__init__(**kwargs) | ||
| self._retrieve_token = retrieve_token # Could be async |
There was a problem hiding this comment.
I don't think __init__ should be overloaded on models.. could you please remove it ?
Comment on lines
112
to
116
| """Retrieve the access token for Google Sheets | ||
|
|
||
| Raise an GoogleSheetsInvalidConfiguration if retrieve_token callback is not set | ||
| Raise an GoogleSheetsRetrieveTokenError if an error is encountered while retrieving the token | ||
| """ |
There was a problem hiding this comment.
Suggested change
| """Retrieve the access token for Google Sheets | |
| Raise an GoogleSheetsInvalidConfiguration if retrieve_token callback is not set | |
| Raise an GoogleSheetsRetrieveTokenError if an error is encountered while retrieving the token | |
| """ | |
| """Retrieves the access token for Google Sheets | |
| Raises a GoogleSheetsInvalidConfiguration if retrieve_token callback is not set | |
| Raises a GoogleSheetsRetrieveTokenError if an error is encountered while retrieving the token | |
| """ |
Comment on lines
73
to
75
| if self.secrets_keeper is None: | ||
| raise ValueError("Secret Keeper not initialized.") | ||
|
|
There was a problem hiding this comment.
Could you please make a method for that ? + Add a custom exception ?
lukapeschke
reviewed
Feb 5, 2025
Comment on lines
73
to
75
| def _check_secret_keeper_exists(self) -> None: | ||
| if self.secrets_keeper is None: | ||
| raise SecretKeeperMissingError("Secret keeper is not set on oauth2 connector.") |
There was a problem hiding this comment.
Suggested change
| def _check_secret_keeper_exists(self) -> None: | |
| if self.secrets_keeper is None: | |
| raise SecretKeeperMissingError("Secret keeper is not set on oauth2 connector.") | |
| def _secrets_keeper(self) -> SecretsKeeper: | |
| if self.secrets_keeper is None: | |
| raise SecretKeeperMissingError("Secret keeper is not set on oauth2 connector.") | |
| return self.secrets_keeper |
It'd allow you to do self._secrets_keeper().something everywhere in the code
lukapeschke
approved these changes
Feb 6, 2025
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.
No description provided.