feat(low-code): added json.loads to jwt authenticator #301
Conversation
|
/autofix
|
📝 WalkthroughWalkthroughThe pull request introduces modifications to the JWT authentication mechanism in the Airbyte CDK. The changes primarily focus on enhancing JSON processing within the Changes
Sequence DiagramsequenceDiagram
participant JwtAuthenticator
participant Config
participant JSON
JwtAuthenticator->>Config: Retrieve secret key
Config-->>JSON: Parse JSON string
JSON-->>JwtAuthenticator: Return parsed secret key
JwtAuthenticator->>JwtAuthenticator: Validate and use secret key
✨ 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 (
|
|
Should we use orjson instead? |
![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 (4)
unit_tests/sources/declarative/auth/test_jwt.py (1)
129-142: The test looks good, but could we add more edge cases? wdyt?The test effectively covers the happy path for JSON parsing. Consider adding test cases for:
- Malformed JSON in config
- Missing keys in the JSON object
- Nested JSON structures
Example additional test case:
def test_get_secret_key_from_config_malformed_json(self): authenticator = JwtAuthenticator( config={"secrets": 'invalid json'}, parameters={}, secret_key="{{ json_loads(config['secrets'])['secret_key'] }}", algorithm="test_algo", token_duration=1200, base64_encode_secret_key=False, ) with pytest.raises(json.JSONDecodeError): authenticator._get_secret_key()airbyte_cdk/sources/declarative/auth/jwt.py (3)
111-111: Should we add some error handling for JSON parsing? wdyt?While adding json_loads is great, we might want to catch and handle JSON parsing errors gracefully. Consider wrapping the eval calls in try-except blocks to provide more meaningful error messages.
try: headers = self._additional_jwt_headers.eval(self.config, json_loads=json.loads) except json.JSONDecodeError as e: raise ValueError(f"Failed to parse JWT header JSON: {str(e)}")Also applies to: 118-118, 120-120, 122-122
134-134: Should we apply the same error handling pattern here too?For consistency with the headers method, consider adding similar JSON parsing error handling.
Also applies to: 141-141, 143-143, 145-145
6-6: Consider centralizing JSON parsing error handling, wdyt?Since we're adding JSON parsing across multiple methods, we could create a helper method to handle JSON parsing errors consistently. This would reduce code duplication and ensure uniform error handling.
def _safe_json_eval(self, interpolated_value, config: dict, context: str = "") -> Any: try: return interpolated_value.eval(config, json_loads=json.loads) except json.JSONDecodeError as e: raise ValueError(f"Failed to parse JSON in {context}: {str(e)}")Also applies to: 111-111, 134-134, 156-156, 181-185
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/auth/jwt.py(5 hunks)unit_tests/sources/declarative/auth/test_jwt.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Publish SDM to DockerHub
- 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)
🔇 Additional comments (3)
airbyte_cdk/sources/declarative/auth/jwt.py (3)
6-6: LGTM! Clean import addition.The json import is appropriately placed with other standard library imports.
181-185: LGTM! Clean and consistent implementation.The changes align well with the pattern used in other methods.
156-156: Should we validate the secret key after parsing? wdyt?Consider adding validation to ensure the secret key is non-empty and of the correct type after JSON parsing.
secret_key: str = self._secret_key.eval(self.config, json_loads=json.loads) if not secret_key: raise ValueError("Secret key cannot be empty")
In that case I don't think that |
|
I'm not too attached on orjson on this one because it won't be called very often so the impact on performance does not seem very high |
lazebnyi
deleted the
lazebnyi/add-json-loads-to-jwt-authentication-params
branch
What
Some sources use JWT authentication and store all parameters in the 'config' field in JSON format (e.g., Google Sheets). This approach makes it problematic to natively use this JSON-format config without a custom implementation.
How
Pass
json.loadsto all interpolated fields in the JWT authenticator.Summary by CodeRabbit
New Features
Tests