fix: dpath_validator should not raise when key does not exist #591
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that DpathValidator no longer raises on missing keys, and adjusts tests accordingly.
- Replace
ValueErroronKeyErrorwith a warning + early return inDpathValidator. - Update unit tests to verify successful validation on top-level paths and remove obsolete error-raising tests.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| unit_tests/sources/declarative/validators/test_dpath_validator.py | Restructure test class, add top-level path test, remove KeyError tests |
| airbyte_cdk/sources/declarative/validators/dpath_validator.py | Catch KeyError, log a warning, and skip missing keys instead of raising |
Comments suppressed due to low confidence (3)
unit_tests/sources/declarative/validators/test_dpath_validator.py:34
- This test method appears unindented (outside of any
TestCasesubclass) after the refactor. It should be nested underclass TestDpathValidatoror adjusted to run as a standalone pytest function.
def test_given_valid_path_and_input_validate_is_successful(self):
unit_tests/sources/declarative/validators/test_dpath_validator.py:38
TestCaseis not imported in this file. Please addfrom unittest import TestCaseat the top to avoid a NameError.
class TestDpathValidator(TestCase):
📝 WalkthroughWalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DpathValidator
participant Logger
User->>DpathValidator: validate(input_data)
DpathValidator->>DpathValidator: Check for path/wildcard in input_data
alt Path not found or wildcard missing
DpathValidator->>Logger: Log warning
DpathValidator-->>User: Return (no exception)
else Path found
DpathValidator->>DpathValidator: Proceed with validation
DpathValidator-->>User: Complete validation
end
Possibly related PRs
Suggested labels
Suggested reviewers
Would you like to consider further tests for nested paths or more granular logging, 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🔭 Outside diff range comments (1)
unit_tests/sources/declarative/validators/test_dpath_validator.py (1)
25-49:⚠️ Potential issueFix the duplicate class definition!
There are two
TestDpathValidatorclasses defined (lines 25 and 38), which will cause the first class to be overwritten. This is a critical issue that needs immediate attention.Could you merge these into a single class or rename one of them? The second class (line 38) appears to contain a new test method that should be part of the main test class. wdyt about consolidating them?
-class TestDpathValidator(TestCase): - def test_given_valid_path_and_input_validate_is_successful(self): - strategy = MockValidationStrategy() - validator = DpathValidator(field_path=["user", "profile", "email"], strategy=strategy) - - test_data = {"user": {"profile": {"email": "test@example.com", "name": "Test User"}}} - - validator.validate(test_data) - - assert strategy.validate_called - assert strategy.validated_value - - -class TestDpathValidator(TestCase): - def test_given_valid_top_level_path_and_input_validate_is_successful(self): +class TestDpathValidator(TestCase): + def test_given_valid_path_and_input_validate_is_successful(self): + strategy = MockValidationStrategy() + validator = DpathValidator(field_path=["user", "profile", "email"], strategy=strategy) + + test_data = {"user": {"profile": {"email": "test@example.com", "name": "Test User"}}} + + validator.validate(test_data) + + assert strategy.validate_called + assert strategy.validated_value + + def test_given_valid_top_level_path_and_input_validate_is_successful(self):🧰 Tools
🪛 Pylint (3.3.7)
[error] 38-38: class already defined line 25
(E0102)
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/validators/dpath_validator.py (1)
55-58: Great improvement in wildcard path error handling!The change from raising
ValueErrorto logging a warning and returning early is exactly what the PR objectives called for. This makes the validator more robust when dealing with missing keys in wildcard scenarios.One small question - should we consider logging at debug level instead of warning level for missing keys, since this might be expected behavior in some cases? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/validators/dpath_validator.py(2 hunks)unit_tests/sources/declarative/validators/test_dpath_validator.py(2 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
unit_tests/sources/declarative/validators/test_dpath_validator.py
[error] 38-38: class already defined line 25
(E0102)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-amplitude' (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 (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
airbyte_cdk/sources/declarative/validators/dpath_validator.py (3)
5-5: LGTM on the import additions!The logging import and typing import changes look good and support the new warning-based approach. The imports are clean and necessary for the functionality.
Also applies to: 7-7
15-16: Logger setup looks good!Using the "airbyte" logger name maintains consistency with the project's logging conventions. The logger placement is appropriate for the module scope.
63-66: Consistent error handling for regular paths!The KeyError handling here mirrors the wildcard path logic perfectly, maintaining consistency in how missing keys are handled throughout the validator. The early return approach is clean and prevents unnecessary processing.
unit_tests/sources/declarative/validators/test_dpath_validator.py (2)
39-48: Nice addition of top-level path test coverage!This new test method provides good coverage for validating top-level keys, which complements the existing nested path tests. The test logic is sound and follows the same pattern as other tests.
57-57: Good simplification of the exception test!Removing the specific exception message check makes sense since the validator now logs warnings instead of raising detailed error messages. The test still properly verifies that strategy failures bubble up as
ValueError.
PyTest Results (Fast)3 656 tests - 2 3 646 ✅ - 2 5m 45s ⏱️ -1s Results for commit b1d20f3. ± Comparison against base commit 99a1a96. This pull request removes 3 and adds 1 tests. Note that renamed tests count towards both. |
PyTest Results (Full)3 659 tests - 2 3 649 ✅ - 2 17m 17s ⏱️ +7s Results for commit b1d20f3. ± Comparison against base commit 99a1a96. This pull request removes 3 and adds 1 tests. Note that renamed tests count towards both. |
What
DpathValidatorwhere KeyErrors were not properly skipped.Summary by CodeRabbit
Bug Fixes
Tests
Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.