🐛 fix declarative oauth initialization #32967
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
octavia-squidington-iii
added
area/connectors
| @@ -80,7 +80,7 @@ def build_refresh_request_body(self) -> Mapping[str, Any]: | |||
| "refresh_token": self.get_refresh_token(), | |||
| } | |||
|
|
|||
| if self.get_scopes: | |||
| if self.get_scopes(): | |||
There was a problem hiding this comment.
@brianjlai flagging this because this is a change in behavior. The existing condition always evaluates to true since the method always exists. I'm pretty sure this is a bug but I don't know how to verify the new behavior works for all possible connectors (including connector-builder connectors)...
Would it make more sense to remove the condition to keep the existing behavior but make the code clearer?
There was a problem hiding this comment.
So basically this would mean retaining the existing behavior means that we can set a token with a payload where scopes = []?
I'd be willing to take the risk and say we only set the scopes payload field if theres at least one to pass along. So how you have it seems fine and was the original intent of the code
|
@girarda can we merge this? |
| self.client_id = InterpolatedString.create(self.client_id, parameters=parameters) | ||
| self.client_secret = InterpolatedString.create(self.client_secret, parameters=parameters) | ||
| def __post_init__(self, parameters: Mapping[str, Any]) -> None: | ||
| super().__init__() |
There was a problem hiding this comment.
this is the fix. Other changes are necessary to please mypy
| @@ -80,7 +80,7 @@ def build_refresh_request_body(self) -> Mapping[str, Any]: | |||
| "refresh_token": self.get_refresh_token(), | |||
| } | |||
|
|
|||
| if self.get_scopes: | |||
| if self.get_scopes(): | |||
There was a problem hiding this comment.
So basically this would mean retaining the existing behavior means that we can set a token with a payload where scopes = []?
I'd be willing to take the risk and say we only set the scopes payload field if theres at least one to pass along. So how you have it seems fine and was the original intent of the code
| self.client_secret = InterpolatedString.create(self.client_secret, parameters=parameters) | ||
| def __post_init__(self, parameters: Mapping[str, Any]) -> None: | ||
| super().__init__() | ||
| self._token_refresh_endpoint = InterpolatedString.create(self.token_refresh_endpoint, parameters=parameters) |
There was a problem hiding this comment.
was mypy complaining about the non-private fields for refresh, client id and client secret or some name collision? I noticed you made this change, but not to the access_token_name, expires_in_name, and grant_type which follow the same naming convention, so curious why this was needed for only some?
There was a problem hiding this comment.
yes. taking the refresh token as an example, if using the public variable, mypy complains on line 82 because the value is expected to be an interpolated string, not an Union[str, InterpolatedString]
Co-authored-by: girarda <girarda@users.noreply.github.com>
Co-authored-by: girarda <girarda@users.noreply.github.com>
What
Oauth2Authenticator, butDeclarativeOauth2Authenticatorwasn't updated to set the fieldsThis is a blocker for updating source-quickbooks to the latest version of the cdk.
How
super().__init__()to initialize the parent paremetersRecommended reading order
airbyte-cdk/python/airbyte_cdk/sources/declarative/auth/oauth.pyairbyte-cdk/python/unit_tests/sources/declarative/auth/test_oauth.py