feat: hmac hashing jinja filter#476
Conversation
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
airbyte_cdk/sources/declarative/interpolation/filters.py:151
- The docstring example uses 'hmac_hash', which is inconsistent with the actual filter name 'hmac'. Please update the example to use 'hmac' to avoid confusion.
signature: "{{ 'message_to_sign' | hmac_hash('my_secret_key') }}"
Natik Gadzhi (natikgadzhi)
requested review from
Ben Church (bnchrch) and
Maxime Carbonneau-Leclerc (maxi297)
📝 WalkthroughWalkthroughThis pull request introduces a new custom Jinja2 filter, Changes
Sequence Diagram(s)sequenceDiagram
participant T as Template Engine
participant F as hmac Filter
participant H as HMAC Library
T->>F: Call hmac(value, key, hash_type)
alt Supported hash type (SHA-256)
F->>H: Compute HMAC using SHA-256
H-->>F: Return hex digest
F-->>T: Return computed HMAC digest
else Unsupported hash type
F-->>T: Raise ValueError
end
Does this updated summary and the diagram meet your expectations, or is there anything you’d like to modify or expand upon? wdyt? Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/interpolation/filters.py (1)
159-166: Consider defining ALLOWED_HASH_TYPES as a module-level constantThe
ALLOWED_HASH_TYPESdictionary is recreated every time the function is called. For better performance and maintainability, perhaps defining it as a module-level constant would be better? This way it's only created once when the module is loaded, wdyt?+# Supported hash types for HMAC +_HMAC_ALLOWED_HASH_TYPES: Dict[str, Any] = { + "sha256": hashlib.sha256, +} + def hmac(value: Any, key: str, hash_type: str = "sha256") -> str: # ... - # Define allowed hash functions - ALLOWED_HASH_TYPES: Dict[str, Any] = { - "sha256": hashlib.sha256, - } - if hash_type not in ALLOWED_HASH_TYPES: + if hash_type not in _HMAC_ALLOWED_HASH_TYPES: raise ValueError( - f"Hash type '{hash_type}' is not allowed. Allowed types: {', '.join(ALLOWED_HASH_TYPES.keys())}" + f"Hash type '{hash_type}' is not allowed. Allowed types: {', '.join(_HMAC_ALLOWED_HASH_TYPES.keys())}" )unit_tests/sources/declarative/interpolation/test_filters.py (1)
159-166: Could be more specific with the expected exception typeThe test is currently checking for any exception, but the
hmacfunction specifically raises aValueErrorfor invalid hash types. Would it be clearer to expect the specific exception, wdyt?- with pytest.raises(Exception): + with pytest.raises(ValueError): interpolation.eval(s, config={})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/interpolation/filters.py(2 hunks)unit_tests/sources/declarative/interpolation/test_filters.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
unit_tests/sources/declarative/interpolation/test_filters.py (2)
airbyte_cdk/sources/declarative/interpolation/filters.py (1)
hmac(139-174)airbyte_cdk/sources/declarative/interpolation/jinja.py (1)
eval(85-123)
⏰ 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: SDM Docker Image Build
- 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/declarative/interpolation/filters.py (2)
168-174: The implementation for hmac looks good!The implementation correctly:
- Handles string conversion for both key and value
- Uses the specified hash algorithm
- Returns the hexadecimal digest
177-185: Filter successfully added to _filters_listThe new HMAC filter has been correctly added to the list of available filters.
unit_tests/sources/declarative/interpolation/test_filters.py (3)
111-125: Test for default HMAC usage is well-structuredThis test correctly verifies the default behavior of the HMAC filter (using SHA-256). Good job computing the expected value directly with the hmac library for comparison!
127-141: Test for explicit SHA-256 specification is thoroughThis test nicely complements the default test by explicitly specifying 'sha256' as the hash type.
143-157: Test for numeric value handling is comprehensiveGreat job testing numeric values! This ensures the filter handles non-string inputs correctly by converting them to strings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unit_tests/sources/declarative/interpolation/test_filters.py (1)
111-166: Comprehensive test coverage for the new HMAC filterThe test suite thoroughly covers all important aspects of the new filter: default behavior, explicit parameters, edge cases with different input types, and error handling. Have you considered adding a test for an empty message or key? Just a thought - the current coverage is already quite solid, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/interpolation/filters.py(2 hunks)unit_tests/sources/declarative/interpolation/test_filters.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- airbyte_cdk/sources/declarative/interpolation/filters.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
unit_tests/sources/declarative/interpolation/test_filters.py (2)
airbyte_cdk/sources/declarative/interpolation/filters.py (1)
hmac(139-174)airbyte_cdk/sources/declarative/interpolation/jinja.py (1)
eval(85-123)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
unit_tests/sources/declarative/interpolation/test_filters.py (5)
6-6: Good addition of the hmac import aliased as hmac_libThis is a clean way to avoid naming conflicts with the filter function. The import is essential for verifying the filter's output against a reference implementation.
111-124: LGTM! Test for default HMAC behavior with SHA-256The test follows the established pattern in the file and properly validates the HMAC filter with default parameters against the expected output from the hmac_lib implementation. This ensures the filter works correctly with its default settings.
127-140: LGTM! Test for explicit SHA-256 specificationGood test to verify that explicitly specifying 'sha256' works correctly. This reinforces that the filter properly handles parameter specification and maintains consistent behavior regardless of how it's invoked.
143-156: LGTM! Testing numeric input handlingExcellent edge case testing! Verifying that numeric inputs are properly converted to strings before hashing is important for robustness. The test confirms that the filter correctly handles different input types.
159-166: LGTM! Proper error handling testGood validation of error handling behavior. The test ensures that the filter rejects unsupported hash types with an appropriate exception, following the implementation's requirement to only accept SHA-256.
Ben Church (bnchrch)
left a comment
Ben Church (bnchrch)
left a comment
There was a problem hiding this comment.
No argument here. Its additive, I trust you that it works, and this is your last day so no need to go back and forth.
| auth_headers: | ||
| $ref: "#/definitions/base_auth" | ||
| $parameters: | ||
| signature: "{{ 'message_to_sign' | hmac('my_secret_key') }}" |
There was a problem hiding this comment.
Ah gotcha, so to use this in many cases its up to the caller to create the list of query params by hand to sign.
Think thats a fine work around.
But it seems like the next win is
- a jinja function to get all query params
- a jinja function to transform it into a delimited string (after you optionally sort / filter / map the list)
There was a problem hiding this comment.
Yes, that would indeed make things easier.
2 participants
What
This PR adds hmac hashing filter! We need this for TikTok Shops API because reasons /shrug
Summary by CodeRabbit
New Features
Tests