fix: (DeclarativeOAuthFlow) - add the ability to override the refresh_token_request_body key names, updated oauth_connector_input_specification tooltips
#220
Conversation
📝 WalkthroughWalkthroughThis pull request enhances the OAuth2 authentication capabilities across multiple Airbyte CDK components by introducing more flexible configuration options for token-related parameters. The changes allow developers to customize the names of client ID, client secret, refresh token, and grant type parameters during OAuth authentication, providing greater flexibility in adapting to different API implementations. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Would you like me to elaborate on any specific aspect of these changes? 🤔 Wdyt about the new flexibility in OAuth configuration? 🚀 Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
airbyte_cdk/sources/declarative/auth/oauth.py (1)
76-84: Consider grouping related attribute initializations together.The initialization of client ID and client secret related attributes are separated. Would you consider grouping them together for better readability, wdyt?
- self._client_id_name = InterpolatedString.create(self.client_id_name, parameters=parameters) - self._client_id = InterpolatedString.create(self.client_id, parameters=parameters) - self._client_secret_name = InterpolatedString.create( - self.client_secret_name, parameters=parameters - ) - self._client_secret = InterpolatedString.create(self.client_secret, parameters=parameters) + # Initialize client ID related attributes + self._client_id_name = InterpolatedString.create(self.client_id_name, parameters=parameters) + self._client_id = InterpolatedString.create(self.client_id, parameters=parameters) + + # Initialize client secret related attributes + self._client_secret_name = InterpolatedString.create(self.client_secret_name, parameters=parameters) + self._client_secret = InterpolatedString.create(self.client_secret, parameters=parameters)airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (1)
209-212: Consider adding more detailed docstrings for the abstract methods.The docstrings could be more descriptive about the purpose and expected return values. Would you consider adding examples, wdyt?
@abstractmethod def get_client_id_name(self) -> str: - """The client id name to authenticate""" + """Returns the key name for the client ID in the OAuth2 request body. + + Example: + return "client_id" # Standard OAuth2 + return "x-client-id" # Custom implementation + """Also applies to: 217-220, 225-228, 261-264
unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py (1)
168-202: Consider adding edge cases to the test.The test covers the happy path well. Would you consider adding tests for:
- Empty string key names
- Key names with special characters
- Duplicate key names in refresh_request_body
wdyt?airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
Line range hint
1050-1084: The new OAuth property names look good, but should we add more examples?The addition of configurable property names for OAuth authentication is a great enhancement for flexibility. The default values and descriptions are clear. However, we could make it even more helpful by adding more real-world examples, wdyt?
Consider adding more examples like:
examples: - custom_app_id - client_identifier - x_client_idairbyte_cdk/sources/declarative/declarative_component_schema.yaml (6)
1050-1056: Consider adding validation pattern for property name.The
client_id_nameproperty allows customization of the OAuth request body key. To prevent invalid property names, would you consider adding a regex pattern validation to ensure the name follows valid identifier rules? wdyt?client_id_name: pattern: "^[a-zA-Z_][a-zA-Z0-9_]*$"
1064-1070: LGTM! Consider adding cross-field validation.The property is well-documented. However, to prevent naming conflicts, would you consider adding validation to ensure
client_secret_namedoesn't duplicate other property names? wdyt?
1078-1084: LGTM! Consider adding examples of common variations.The property is well-defined. To help users, would you consider adding more examples of commonly used refresh token property names from popular APIs? wdyt?
examples: - refresh_token - oauth_refresh_token - custom_app_refresh_value - rt # Some APIs use abbreviated forms
1118-1124: LGTM! Consider adding enum for common grant types.The property is well-documented. To prevent typos and provide better IDE support, would you consider adding an enum of commonly used grant type property names? wdyt?
grant_type_name: enum: - "grant_type" - "grantType" - "grant-type" - "oauth_grant_type"
2235-2243: Enhance documentation for nested variable resolution.The interpolation syntax documentation is clear, but the nested resolution example could be more detailed. Would you consider adding:
- More complex examples of nested resolution
- Common pitfalls to avoid
- Performance implications of deep nesting
wdyt?
2252-2257: LGTM! Consider adding error handling examples.The examples effectively demonstrate the new interpolation syntax. To make it more robust, would you consider adding examples of:
- How to handle missing or null values
- Default value fallbacks
- Error messages for invalid syntax
wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
airbyte_cdk/sources/declarative/auth/oauth.py(5 hunks)airbyte_cdk/sources/declarative/declarative_component_schema.yaml(8 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(6 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(2 hunks)airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py(3 hunks)airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py(5 hunks)unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (9)
airbyte_cdk/sources/declarative/auth/oauth.py (2)
59-64: LGTM! The new attributes enhance flexibility.These new attributes allow customization of key names in the OAuth2 request body, which is great for supporting APIs with non-standard parameter names. The default values align with OAuth2 standards.
139-141: LGTM! The getter methods are well-implemented.The getter methods properly evaluate the interpolated strings and include appropriate type hints. They align well with the abstract base class requirements.
Also applies to: 148-150, 157-159, 172-174
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (1)
84-87: LGTM! The request body now uses dynamic key names.The build_refresh_request_body method now properly uses the getter methods for key names, making it more flexible for different OAuth2 implementations.
airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py (3)
33-35: LGTM! The new parameters are well-defined.The parameters have appropriate default values that align with standard OAuth2 implementations.
Also applies to: 42-42
74-76: LGTM! The getter implementations are straightforward.The getter methods simply return the stored values, which is appropriate for this implementation.
Also applies to: 80-82, 86-88, 104-106
50-54: Consider adding validation for empty strings.Would you consider adding validation to ensure these values aren't empty strings, wdyt? This could prevent issues with malformed requests.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
Line range hint
2235-2243: LGTM! Clear documentation of interpolation capabilities.The updated interpolation syntax documentation is well-structured and provides clear examples for each capability. The base64 encoding/decoding, URL encoding/decoding, and code challenge examples are particularly helpful.
Line range hint
2252-2257: The updated examples with new interpolation syntax look good.The examples have been correctly updated to use the new interpolation syntax with double curly braces. The examples are comprehensive and demonstrate both simple and complex use cases.
Also applies to: 2283-2284
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
1888-1890: The OAuth authenticator creation looks good, but should we add validation?The implementation of configurable property names in the OAuth authenticator creation is well-structured. The code correctly evaluates the interpolated strings with appropriate default values.
However, should we consider adding validation to ensure the property names don't contain invalid characters that could cause issues in HTTP requests? For example, checking that they don't contain spaces or special characters, wdyt?
Let's verify if there are any existing validation patterns we could reuse:
Also applies to: 1894-1896, 1903-1905, 1923-1931
bazarnov
deleted the
baz/cdk/add-oauth-request-body-keys-overrides-fix-tooltips
branch
What
During the QA sessions for
DeclarativeOAuthFlowin Builder, the following issues were discovered:refresh_token_request_bodyto provide the custom names as keys to add more flexibility and cover more custom OAuth flows, which have non-defaultrequest_bodyto be provided.oauth_connector_input_specification.extract_outputfieldOptional, since it's going to be deprecated soon.oauth_connector_input_specification(DeclarativeOAuthFlow) tooltips to reflect the updatedJinja2variables declaration style. Resolving: https://github.com/airbytehq/airbyte-internal-issues/issues/11337Summary by CodeRabbit
New Features
Improvements