fix: (DeclarativeOAuthFlow) - fix the bug when refresh_token is not provided from the test authentication
#186
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Hey there! 👋 I noticed you've made some interesting refinements to the OAuth authentication flow. A couple of quick thoughts:
Would love to hear your thoughts on these suggestions! Cheers! 🚀 Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (1)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (1)
60-62: The new property looks clean! Maybe add a docstring? wdyt?The property implementation is solid, but adding a docstring would help other developers understand its purpose better. Something like:
@property def _is_access_token_flow(self) -> bool: + """ + Determines if the current flow is using a direct access token without refresh capability. + Returns True when token_refresh_endpoint is None and access_token is present. + """ return self.get_token_refresh_endpoint() is None and self.access_token is not None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py(1 hunks)
🔇 Additional comments (1)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (1)
57-57: LGTM! The simplified token retrieval logic looks good.
The ternary operation makes the code more readable and correctly addresses the issue when refresh_token is not provided. Would you like me to verify if this change affects any other parts of the codebase? 🤔
✅ Verification successful
The simplified token retrieval logic looks safe and well-tested! 🎯
The test coverage shows that both access token and refresh token flows are properly tested, and the change aligns perfectly with the existing behavior. The tests in both test_requests_native_auth.py and test_oauth.py validate that:
- Direct access token usage works correctly
- Token refresh flow functions as expected
- Error handling for missing refresh tokens is in place
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct calls to get_auth_header to understand the impact
rg -A 3 "get_auth_header\(" --type python
Length of output: 74
Script:
#!/bin/bash
# Search for any direct calls to get_auth_header
rg "get_auth_header\("
# Search for classes that inherit from AbstractOauth2Authenticator
ast-grep --pattern 'class $_ (AbstractOauth2Authenticator)'
# Search for imports of AbstractOauth2Authenticator
rg "AbstractOauth2Authenticator"
Length of output: 3653
Script:
#!/bin/bash
# Let's check the test files to understand the usage patterns and verify the change is safe
rg -A 10 "get_auth_header\(\)" unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py
# Also check the declarative OAuth tests
rg -A 10 "get_auth_header\(\)" unit_tests/sources/declarative/auth/test_oauth.py
Length of output: 3277
bazarnov
deleted the
baz/cdk/oauth2-fix-bug-when-no-refresh-token-is-provided
branch
What
Resolving the small regression after this change:
The case, when
token_refresh_endpointandclient_id, client_secretare provided to obtain therefresh_tokenoraccess_tokenfrom theOAuthauthentication, with norefresh_tokenvalue provided in prior.Related slack thread: https://airbytehq-team.slack.com/archives/C027KKE4BCZ/p1734877961838859
How
self.get_refresh_token()value check from the condition to determine the presence of theaccess_tokenand related flow.Summary by CodeRabbit