fix: (OAuthAuthenticator) - get the access_token, refresh_token, expires_in recursively from response
#285
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a new exception class Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Hey there! 👋 I noticed you've made some really cool improvements to the OAuth token retrieval process. The new recursive search method looks quite elegant! Wdyt about adding some additional comments explaining the max_depth parameter? It might help future developers understand the recursion limit. Cheers! 🚀 ✨ 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 (3)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (2)
145-148: Consider improving the error message for better debugging?The error message could include the actual path tried during the token extraction for better debugging. wdyt?
- raise Exception( - "Token refresh API response was missing access token {self.get_access_token_name()}" - ) + raise Exception( + f"Token refresh API response was missing access token '{self.get_access_token_name()}'. Response: {json.dumps(response_json)}" + )
301-352: Consider some optimizations for better performance?The implementation is solid, but here are some suggestions that might improve performance, wdyt?
- Consider using a set for faster key lookups in deeply nested structures
- Add early return if the response is None or empty
- Consider caching found paths for subsequent lookups
def _find_and_get_value_from_response( self, response_data: Mapping[str, Any], key_name: str, max_depth: int = 5, current_depth: int = 0, ) -> Any: + if not response_data: + return None + if current_depth > max_depth: message = f"The maximum level of recursion is reached. Couldn't find the speficied `{key_name}` in the response." raise AirbyteTracedException( internal_message=message, message=message, failure_type=FailureType.config_error ) if isinstance(response_data, dict): if key_name in response_data: return response_data[key_name] for _, value in response_data.items(): result = self._find_and_get_value_from_response( value, key_name, max_depth, current_depth + 1 ) if result is not None: return result elif isinstance(response_data, list): for item in response_data: result = self._find_and_get_value_from_response( item, key_name, max_depth, current_depth + 1 ) if result is not None: return result return Noneunit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py (1)
319-351: Consider adding more edge case tests?The max depth test is great! Would you consider adding tests for these scenarios as well?
- Empty response
- Response with circular references
- Response with array of arrays
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py(4 hunks)airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py(1 hunks)unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py(4 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 (5)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (2)
170-171: LGTM! Good error handling.Preserving the original
AirbyteTracedExceptionmaintains the error context and failure type.
183-186: LGTM! Consistent implementation.Using the same extraction method for both fields ensures consistent behavior.
airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py (1)
343-345: LGTM! Clean implementation.The changes maintain consistency with the parent class while supporting nested token structures.
unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py (2)
280-289: LGTM! Good test coverage.The test case properly validates the extraction of nested tokens and their types.
291-318: LGTM! Thorough edge case testing.Great job testing deeply nested structures with various intermediate fields.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (2)
27-32: Fix typo in class nameThere's a typo in the class name: "Recurtion" should be "Recursion". What do you think about renaming it to
ResponseKeysMaxRecursionReached? 🤔-class ResponseKeysMaxRecurtionReached(AirbyteTracedException): +class ResponseKeysMaxRecursionReached(AirbyteTracedException):
308-359: Great implementation! A couple of minor suggestions:
- There's a typo in the error message: "speficied" should be "specified".
- Would you consider adding a return type hint to make the method signature more complete? The docstring mentions it returns
Any.def _find_and_get_value_from_response( self, response_data: Mapping[str, Any], key_name: str, max_depth: int = 5, current_depth: int = 0, - ) -> Any: + ) -> Any: # Consider being more specific about return type if possible- message = f"The maximum level of recursion is reached. Couldn't find the speficied `{key_name}` in the response." + message = f"The maximum level of recursion is reached. Couldn't find the specified `{key_name}` in the response."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py(5 hunks)unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py(5 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 (Fast)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (2)
Line range hint
152-168: LGTM! Good error handling 👍The integration with
_find_and_get_value_from_responseis clean, and I appreciate that you're explicitly handling the recursion exception.
190-193: Clean implementation! ✨The method now elegantly handles nested tokens while maintaining its interface.
unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py (4)
Line range hint
263-272: Great test coverage! 🎯Good job testing different
expires_informats and their type preservation.
283-292: Good test for basic nesting! 👍Clear test case demonstrating single-level nested token retrieval.
294-320: Excellent edge case coverage! 🌟Great job testing deep nesting with a mix of different fields. This will help catch any regression issues.
322-354: Don't forget to update the testAfter fixing the typo in the exception class name from
ResponseKeysMaxRecurtionReachedtoResponseKeysMaxRecursionReached, this test will need to be updated too. Otherwise, great job testing the max depth scenario! 🎯- with pytest.raises(ResponseKeysMaxRecurtionReached) as exc_info: + with pytest.raises(ResponseKeysMaxRecursionReached) as exc_info:
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (3)
28-33: Fix typo in class name: "Recurtion" → "Recursion"The class name contains a typo. What do you think about renaming it to
ResponseKeysMaxRecursionReachedto match the standard spelling? 🤔-class ResponseKeysMaxRecurtionReached(AirbyteTracedException): +class ResponseKeysMaxRecursionReached(AirbyteTracedException):
320-371: Consider adding type hints for better code maintainabilityThe method could benefit from more specific type hints. What do you think about using
Union[dict, list]forresponse_datato better reflect its supported types? Also, there's a typo in "speficied". 🤔def _find_and_get_value_from_response( self, - response_data: Mapping[str, Any], + response_data: Union[Mapping[str, Any], List[Any]], key_name: str, max_depth: int = 5, current_depth: int = 0, ) -> Any:- message = f"The maximum level of recursion is reached. Couldn't find the speficied `{key_name}` in the response." + message = f"The maximum level of recursion is reached. Couldn't find the specified `{key_name}` in the response."
229-256: Fix missing f-string prefix in error messageThere's a missing 'f' prefix in the error message string. Would you like to fix it? 🔍
- "Token refresh API response was missing access token {self.get_access_token_name()}" + f"Token refresh API response was missing access token {self.get_access_token_name()}"unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py (2)
284-354: Consider using more descriptive test case labelsThe nested token test cases are comprehensive, but what do you think about making them more descriptive using pytest's parametrize? This would make the test report more readable. 🤔
@pytest.mark.parametrize( + "test_name, response_data, expected_result", + [ + ( + "single_level_nesting", + {"data": {"access_token": "access_token_nested", "expires_in": "2001"}}, + ("access_token_nested", "2001"), + ), + ( + "deep_nesting_within_limits", + {"data": {"data2": {"data3": {"data4": {"data5": {"access_token": "token", "expires_in": "2002"}}}}}}, + ("token", "2002"), + ), + # ... more cases + ], + ) + def test_nested_token_extraction(self, mocker, test_name, response_data, expected_result):
323-354: Add explicit documentation about max depth limitThe test case for maximum recursion is good, but what do you think about adding a comment explaining why 6 levels of nesting triggers the exception? This would make it clearer that we're testing against the default max_depth of 5. 🤔
- # this is the edge case, but worth testing. + # Testing recursion limit: 6 levels of nesting exceeds default max_depth of 5airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py (3)
Line range hint
178-199: Consider adding parameter validation in the constructor?The constructor has great documentation now, but what do you think about adding validation for critical parameters like
token_refresh_endpointandconnector_configto fail fast if they're invalid or missing? wdyt?def __init__( self, connector_config: Mapping[str, Any], token_refresh_endpoint: str, # ... other parameters ... ) -> None: + if not connector_config: + raise ValueError("connector_config is required") + if not token_refresh_endpoint: + raise ValueError("token_refresh_endpoint is required") + if not token_refresh_endpoint.startswith(("http://", "https://")): + raise ValueError("token_refresh_endpoint must be a valid HTTP(S) URL") self._connector_config = connector_config
372-402: Consider caching frequently accessed config values?For values that are accessed often (like client_id, client_secret), what do you think about caching them after the first retrieval to avoid repeated dpath lookups? This could improve performance, especially with deeply nested configs. wdyt?
class SingleUseRefreshTokenOauth2Authenticator(Oauth2Authenticator): + _config_cache: Dict[str, Any] = {} def _get_config_value_by_path( self, config_path: Union[str, Sequence[str]], default: Optional[str] = None ) -> str | Any: + cache_key = str(config_path) + if cache_key in self._config_cache: + return self._config_cache[cache_key] - return dpath.get( + value = dpath.get( self._connector_config, config_path, default=default if default is not None else "", ) + self._config_cache[cache_key] = value + return value
405-430: Address the FIXME comment about deprecated function?There's a FIXME comment about
emit_configuration_as_airbyte_control_messagebeing deprecated. Should we create a follow-up issue to remove this technical debt and fully migrate toairbyte_cdk.sources.message? wdyt?Would you like me to create an issue to track this deprecation removal?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py(6 hunks)airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py(8 hunks)unit_tests/connector_builder/test_connector_builder_handler.py(1 hunks)unit_tests/sources/streams/http/requests_native_auth/test_requests_native_auth.py(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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.12, Ubuntu)
- 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 (4)
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (1)
133-145: LGTM! Clean refactoring of token refresh logicThe separation of token extraction logic into dedicated methods improves code organization and maintainability.
unit_tests/connector_builder/test_connector_builder_handler.py (1)
603-603: LGTM! Good test coverage for OAuth token refreshThe test properly validates the integration between the connector builder and the OAuth token refresh mechanism.
airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py (2)
358-370: LGTM! Clean and well-documented implementation.The token refresh logic is now more robust with proper error handling and clear separation of concerns.
199-204: Consider improving type safety in config value retrieval?The return type hint
str | Anyin_get_config_value_by_pathcould lead to runtime errors if non-string values are retrieved. What do you think about making it more specific or adding type checking? wdyt?
…-tokens-recursively
…-tokens-recursively
What
Resolving:
Currently the
OAuthAuthenticatore.gDeclarativeOauth2AuthenticatorandSingleUseRefreshTokenOauth2Authenticatordon't support the case when theaccess_tokenor similar field isnestedunder another entity likedataor similar.We need a way to handle this dynamically for the Customer and never think about what is the
PATHfor the targetkeysto set.How
_find_and_get_value_from_responseto pull out the target key recursively, up to 5 levels of nesting (by default)unit_teststo reflect the changeUser Impact
No impact is expected. This is not a breaking change.
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements