feat(cdk): allow empty secrets (bing-ads)#519
feat(cdk): allow empty secrets (bing-ads)#519Aldo Gonzalez (aldogonzalez8) merged 6 commits intomainfrom
Conversation
|
/autofix
|
📝 WalkthroughWalkthroughThe update modifies the behavior of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DeclarativeOauth2Authenticator
participant Logger
User->>DeclarativeOauth2Authenticator: Call get_client_secret()
DeclarativeOauth2Authenticator->>DeclarativeOauth2Authenticator: Evaluate client secret
alt Client secret cannot be evaluated
DeclarativeOauth2Authenticator->>Logger: Log warning
DeclarativeOauth2Authenticator-->>User: Return empty/None client secret
else Client secret evaluated
DeclarativeOauth2Authenticator-->>User: Return client secret
end
Would you like me to suggest adding a test case that specifically asserts the warning log is emitted when the client secret is empty? Wdyt? ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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)
airbyte_cdk/sources/declarative/auth/oauth.py (2)
207-211: Nice improvement to handle empty client secrets.The change from raising an error to logging a warning is well-justified by the comment explaining that some APIs allow empty client secrets. This makes the authenticator more flexible while still alerting users to potential issues.
Would it make sense to also update the type annotation comment on line 211? It still mentions "or an error will be raised" which is no longer true for empty client secrets. wdyt?
- return client_secret # type: ignore # value will be returned as a string, or an error will be raised + return client_secret # type: ignore # value will be returned as a string, which might be empty
26-50: Consider updating class documentation.Since this class now supports empty client secrets, it might be helpful to update the documentation to mention this capability, especially in the client_secret attribute description on line 36.
Would something like this be useful? wdyt?
- client_secret (Union[InterpolatedString, str]): Client secret + client_secret (Union[InterpolatedString, str]): Client secret (can be empty for APIs that support this)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/auth/oauth.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (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: SDM Docker Image Build
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
airbyte_cdk/sources/declarative/auth/oauth.py (3)
5-5: Good choice adding the logging module.The import of the logging module is appropriate for the warning functionality you're adding. Clean integration with the existing imports.
Also applies to: 8-8
23-24: Logger initialization looks good.Creating a logger with the "airbyte" name follows standard Python logging practices and aligns with the package organization. The empty line after it helps with code readability.
164-170: Consider updating validation logic for consistency.This validation block requires both client_id and client_secret when not using profile assertion. Since we now allow empty client_secret values, should we consider relaxing this validation too? Or perhaps clarify that client_secret can be empty but must be provided?
I'm not suggesting a change is needed, just highlighting a potential inconsistency between this validation and the new behavior. wdyt?
Aldo Gonzalez (aldogonzalez8)
requested a review
from Maxime Carbonneau-Leclerc (maxi297)
Maxime Carbonneau-Leclerc (maxi297)
left a comment
Maxime Carbonneau-Leclerc (maxi297)
left a comment
There was a problem hiding this comment.
LGTM!
Can we add this test as well?
I will allow it |
Tests added Maxime Carbonneau-Leclerc (@maxi297) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unit_tests/sources/declarative/auth/test_oauth.py (1)
132-145: Test correctly verifies handling of empty client secrets.This test nicely complements the implementation change that allows empty secrets during OAuth authentication. It ensures that when the client secret is missing, the authenticator includes an empty string in the refresh request body instead of raising an error.
Would it be helpful to also add tests for cases where
"client_secret": ""or"client_secret": None? These might be useful edge cases to consider, especially with Bing Ads implementation. Also, adding a docstring to explain the test purpose (similar to other test methods) might be helpful for future developers, wdyt?def test_client_secret_empty(self): + """ + Test that when client_secret is missing from config, an empty string is used in the refresh request body. + This ensures compatibility with services like Bing Ads that allow empty secrets. + """ config_without_client_secret = deepcopy(config) del config_without_client_secret["client_secret"] oauth = DeclarativeOauth2Authenticator(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unit_tests/sources/declarative/auth/test_oauth.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (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: SDM Docker Image Build
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
unit_tests/sources/declarative/auth/test_oauth.py (1)
8-8: Good addition of the deepcopy import.The import of
deepcopyis used appropriately in the new test to avoid modifying the sharedconfigdictionary, which could affect other tests.
|
Why are all the smoke tests failing? If we're fine with this, let's move forward |
Maxime Carbonneau-Leclerc (@maxi297) :( More context here |
2 participants
Allow empty secrets during oauth
Required for: airbytehq/airbyte#59136
Resolves: https://github.com/airbytehq/airbyte-internal-issues/issues/12679
Summary by CodeRabbit