feat: update DpathValidator to skip validation if returned value is falsy
#590
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates DpathValidator to skip validation when the extracted value(s) at the specified path are falsy, and adds a unit test to cover no-value scenarios.
- Added guards in
DpathValidator.validateto return early ifdpath.valuesordpath.getyield falsy results. - Introduced a new test to verify that the validation strategy isn’t called when there are no values.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| unit_tests/sources/declarative/validators/test_dpath_validator.py | Added test_given_no_values_when_validate_then_validate_is_not_called to ensure no validation on missing paths |
| airbyte_cdk/sources/declarative/validators/dpath_validator.py | Added if not values/value: return checks to skip validation when results are falsy |
Comments suppressed due to low confidence (2)
unit_tests/sources/declarative/validators/test_dpath_validator.py:104
- Add tests for legitimate falsy values (e.g.,
0,False, empty string) to ensure they are handled as intended rather than skipped incorrectly.
assert not strategy.validate_called
airbyte_cdk/sources/declarative/validators/dpath_validator.py:59
- This check will skip validation for all falsy values (e.g., 0, False, ""). If the intent is to only skip when the value is missing, use
if value is None:.
if not value:
|
@brianjlai Hm. I'm on the fence if it's needed there. Couldn't the connector dev include a condition in the provided interpolation value (or does jinja not allow if/then statements?) Either way, this can be a follow-up. |
📝 WalkthroughWalkthroughThe Changes
Suggested labels
Would you like to consider adding more test cases for other edge scenarios, such as when the dpath points to a falsy but present value (e.g., empty string or zero), 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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/validators/dpath_validator.py(1 hunks)unit_tests/sources/declarative/validators/test_dpath_validator.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
airbyte_cdk/sources/declarative/validators/dpath_validator.py (3)
unit_tests/sources/declarative/validators/test_dpath_validator.py (1)
validate(18-22)airbyte_cdk/sources/declarative/validators/validation_strategy.py (1)
validate(15-22)airbyte_cdk/sources/declarative/validators/validator.py (1)
validate(11-18)
unit_tests/sources/declarative/validators/test_dpath_validator.py (2)
unit_tests/sources/declarative/validators/test_predicate_validator.py (2)
MockValidationStrategy(9-20)validate(16-20)airbyte_cdk/sources/declarative/validators/dpath_validator.py (1)
validate(35-63)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (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
🔇 Additional comments (1)
airbyte_cdk/sources/declarative/validators/dpath_validator.py (1)
50-51: Early return logic for wildcard paths looks good!The check for empty values list makes sense here - no point validating when there's nothing to validate. This aligns well with the PR objective.
PyTest Results (Full)3 661 tests 3 651 ✅ 17m 28s ⏱️ Results for commit b074084. |
pnilan
deleted the
pnilan/feat/update-validator-to-skip-validation-if-value-is-falsy
branch
What
DpathValidatordoes not invoke validation strategy if value at specified path is falsyHow
Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.
Summary by CodeRabbit
Bug Fixes
Tests