feat(low-code): pass refresh headers to oauth #219
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new optional attribute Changes
Possibly related PRs
Suggested labels
Suggested reviewers
What do you think about these changes? The new 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 (2)
unit_tests/sources/declarative/auth/test_oauth.py (1)
437-442: Consider enhancing error message format?The error message includes all request details, but wdyt about formatting it more clearly? Maybe something like:
- f"Error while refreshing access token with request: {method}, {url}, {data}, {headers}" + f"Error while refreshing access token with request:\n Method: {method}\n URL: {url}\n Data: {data}\n Headers: {headers}"unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py (1)
168-199: LGTM! The test coverage for refresh request headers looks good.The test cases cover both scenarios where headers are provided and when they're not. The assertions verify the expected behavior.
Would you consider adding a test case for invalid headers (e.g., malformed Content-Type) to ensure proper error handling? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
airbyte_cdk/sources/declarative/auth/oauth.py(4 hunks)airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(1 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(6 hunks)unit_tests/sources/declarative/auth/test_oauth.py(3 hunks)unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py(3 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 (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (18)
airbyte_cdk/sources/declarative/auth/oauth.py (4)
42-42: LGTM! Clear and well-documented attribute in docstring.The new
refresh_request_headersattribute is well-documented with a clear description of its purpose.
62-62: LGTM! Consistent attribute initialization.The attribute initialization follows the same pattern as other similar attributes in the class.
92-94: LGTM! Consistent initialization in post_init.The initialization of
_refresh_request_headersfollows the same pattern as_refresh_request_body.
160-162: LGTM! Well-implemented getter method.The
get_refresh_request_headersmethod follows the same pattern as other getter methods in the class.airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (3)
101-108: LGTM! Clear and concise implementation.The
build_refresh_request_headersmethod follows a similar pattern tobuild_refresh_request_body, with appropriate handling of None case.
139-139: LGTM! Proper integration of headers in request.The headers are correctly integrated into the request call.
254-257: LGTM! Well-defined abstract method.The abstract method declaration follows the same pattern as other abstract methods in the class.
airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py (3)
39-39: LGTM! Consistent parameter and attribute addition.The new parameter and its initialization follow the existing patterns in the class.
Also applies to: 54-54
89-91: LGTM! Proper implementation of abstract method.The
get_refresh_request_headersimplementation correctly returns the headers.
201-201: LGTM! Consistent parameter passing.The
refresh_request_headersparameter is correctly passed to the superclass.unit_tests/sources/declarative/auth/test_oauth.py (2)
72-104: LGTM! Comprehensive test coverage for headers.The test cases cover both scenarios with and without headers, ensuring proper functionality.
227-256: LGTM! Well-structured integration test.The test verifies that headers are correctly passed through to the request.
unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py (2)
245-273: LGTM! Good integration test for headers in refresh token requests.The test verifies that the headers are correctly passed through to the request. The mocked request verification is thorough.
586-591: Good update to include headers in error messages.The addition of headers to the error message will help with debugging when requests fail.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
1903-1905: LGTM! Good integration of refresh request headers.The implementation uses InterpolatedMapping for header evaluation, maintaining consistency with how other similar parameters are handled.
1922-1922: LGTM! Consistent parameter passing.The refresh_request_headers parameter is correctly passed through to the OAuth authenticator.
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1114-1121: LGTM! Well-documented schema addition.The refresh_request_headers property is well-defined with clear examples and proper documentation.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
550-560: Great addition! Clean implementation of the refresh headers support.The new
refresh_request_headersfield is well-documented, properly typed, and maintains backward compatibility. The examples provided are particularly helpful for understanding the usage. This change elegantly solves the Airtable OAuth use case mentioned in the PR description.
|
I just want to highlight that @bazarnov and @darynaishchenko are playing on the same part of the code. Just want to confirm that there is no conflict here |
|
There is no conflict in terms of the code base; we updated different parts of the same components. Once the merge conflicts are fixed, we should be good. |
|
@bazarnov give this a review please! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py (1)
621-626: Consider enhancing the error message format, wdyt?The error message could be more structured for better readability. Maybe use JSON formatting?
- f"Error while refreshing access token with request: {method}, {url}, {data}, {headers}" + f"Error while refreshing access token with request:\n Method: {method}\n URL: {url}\n Data: {data}\n Headers: {headers}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
airbyte_cdk/sources/declarative/auth/oauth.py(4 hunks)airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(1 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(6 hunks)unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py
- airbyte_cdk/sources/declarative/models/declarative_component_schema.py
- airbyte_cdk/sources/declarative/declarative_component_schema.yaml
⏰ 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 (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (11)
airbyte_cdk/sources/declarative/auth/oauth.py (4)
42-42: LGTM! Well-documented new parameter.The docstring for
refresh_request_headersfollows the established pattern and clearly explains its purpose.
65-65: LGTM! Clean attribute declaration.The attribute declaration follows the class's pattern and uses appropriate typing.
106-108: LGTM! Consistent initialization.The initialization in
__post_init__follows the established pattern and properly handles the None case.
186-188: LGTM! Clean getter implementation.The
get_refresh_request_headersmethod is simple, focused, and consistent with other getter methods in the class.airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (3)
101-108: LGTM! Clean implementation of header building.The
build_refresh_request_headersmethod is well-documented and handles the None case appropriately.
139-139: LGTM! Proper header integration.Headers are correctly integrated into the refresh token request.
266-269: LGTM! Well-defined abstract method.The abstract method
get_refresh_request_headersis properly documented and follows the class's pattern.unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py (2)
168-199: LGTM! Comprehensive header configuration tests.Good test coverage for both cases: with and without headers.
280-308: LGTM! Thorough header integration test.The test properly verifies that headers are passed correctly to the refresh token request.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
1922-1924: LGTM! Clean header integration for single-use authenticator.Headers are properly evaluated using InterpolatedMapping, consistent with other parameters.
1944-1944: LGTM! Consistent header passing.The refresh headers are passed correctly to the regular authenticator.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
unit_tests/sources/declarative/auth/test_oauth.py (3)
72-107: Great test coverage! Would you consider a few enhancements? 🤔The test covers both scenarios well. To make it even more robust, what do you think about:
- Extracting the common header values into constants at the module level?
- Adding a test case for invalid base64 encoding in the Authorization header?
wdyt?
# At module level CONTENT_TYPE_HEADER = "application/x-www-form-urlencoded" AUTHORIZATION_PREFIX = "Basic " # In the test expected = { "Authorization": f"{AUTHORIZATION_PREFIX}c29tZV9jbGllbnRfaWQ6c29tZV9jbGllbnRfc2VjcmV0", "Content-Type": CONTENT_TYPE_HEADER, }
230-259: Solid test! Here's a thought for making it even more comprehensive 💭The test effectively verifies the happy path. How about adding test cases for:
- Malformed headers (e.g., missing Content-Type)?
- Verifying that headers aren't modified during the request?
This would help catch potential edge cases. wdyt?
def test_refresh_access_token_when_headers_malformed(self, mocker): # Test with missing required headers malformed_headers = { "Authorization": "Bearer some_access_token", # Missing Content-Type } # ... rest of the test
440-445: How about adding some type hints and documentation? 📝The error message improvement is great! To make it even more maintainable, what do you think about adding:
- Type hints for the parameters and return value
- A docstring explaining the function's purpose and parameters
wdyt?
def mock_request( method: str, url: str, data: dict, headers: dict | None ) -> requests.Response: """Mock the requests.request function for testing. Args: method: HTTP method url: Request URL data: Request data headers: Request headers Returns: Response object for successful token refresh Raises: Exception: For any URL other than the refresh endpoint """ if url == "refresh_end": return resp raise Exception( f"Error while refreshing access token with request: {method}, {url}, {data}, {headers}" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unit_tests/sources/declarative/auth/test_oauth.py(3 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)
There was a problem hiding this comment.
the result of get_auth_header is unaffected - https://github.com/airbytehq/airbyte-python-cdk/pull/219/files#r1918454347, then LGTM
darynaishchenko
deleted the
daryna/low-code/pass-refresh-headers-to-oauth
branch
Problem
Some oauth flows (like in Airtable) require passing specific headers to a refresh token request.
Solution
To make it possible from low code new param
refresh_request_headerswas added. Default isNone.Updated
DeclarativeSingleUseRefreshTokenOauth2AuthenticatorandDeclarativeOauth2Authenticatorwith a new paramrefresh_request_headersand_get_refresh_access_token_response()with a request with headers.Summary by CodeRabbit
New Features
refresh_request_headersparameter for OAuth authenticators to specify custom headers.Improvements
Tests