feat: add conditions to validators #589
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds an optional condition parameter to both PredicateValidator and DpathValidator, allowing validation strategies to be skipped based on runtime config. Core changes include:
- Extending validator constructors and
validatemethods with config-driven conditional logic. - Updating the component factory and schemas to support the new
conditionfield. - Revising unit tests to pass
configandcondition, and adding negative-case tests for skipped validation.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| unit_tests/.../test_predicate_validator.py | Updated instantiation to include config/condition; added skip test |
| unit_tests/.../test_dpath_validator.py | Same updates as above for DpathValidator |
| airbyte_cdk/.../predicate_validator.py | Added config, condition fields and conditional skip in validate |
| airbyte_cdk/.../dpath_validator.py | Added config, condition fields and conditional skip in validate |
| airbyte_cdk/.../model_to_component_factory.py | Passes config and condition into newly extended validators |
| airbyte_cdk/.../declarative_component_schema.py | Added condition property to Pydantic models |
| airbyte_cdk/.../declarative_component_schema.yaml | Added condition definitions in YAML schema |
Comments suppressed due to low confidence (4)
airbyte_cdk/sources/declarative/validators/predicate_validator.py:27
- Update the
validatedocstring to mention the newconfigandconditionparameters and clarify that validation is skipped when the condition evaluates to false.
def validate(self) -> None:
airbyte_cdk/sources/declarative/validators/dpath_validator.py:39
- Enhance the docstring to explain that if
conditionevaluates to false based onconfig, the validation strategy is skipped.
def validate(self, input_data: dict[str, Any]) -> None:
unit_tests/sources/declarative/validators/test_predicate_validator.py:59
- [nitpick] Add a complementary test where the condition evaluates to true (e.g.,
"{{ config.get('test') }}") to verify that the strategy is invoked when the condition passes.
def test_given_condition_is_false_when_validate_then_validate_is_not_called(self):
unit_tests/sources/declarative/validators/test_dpath_validator.py:126
- [nitpick] Similarly, include a test where the condition evaluates to true to ensure the DpathValidator applies the strategy when expected.
def test_given_condition_is_false_when_validate_then_validate_is_not_called(self):
📝 WalkthroughWalkthroughThis change introduces a new optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ValidatorFactory
participant Validator
participant Config
User->>ValidatorFactory: Create Validator (with model, config)
ValidatorFactory->>Validator: Instantiate (pass config, condition)
User->>Validator: validate(input_data)
Validator->>Validator: Evaluate condition against config
alt Condition is true
Validator->>Validator: Perform validation strategy
else Condition is false
Validator-->>User: Skip validation
end
Suggested reviewers
Would you like to consider adding more examples of complex conditions in the schema documentation to help users understand possible use cases, 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
🧹 Nitpick comments (9)
unit_tests/sources/declarative/validators/test_predicate_validator.py (1)
59-70: Excellent test coverage for the conditional validation feature!This test properly verifies that validation is skipped when the condition evaluates to False. The condition logic is correct:
{{ not config.get('test') }}withconfig={"test": "test"}evaluates tonot "test"which isFalse, sostrategy.validate_calledshould indeed beFalse.One small suggestion - would it be worth adding a comment explaining the condition evaluation for clarity, wdyt? Something like:
# Condition evaluates to False since config.get('test') returns "test" (truthy) condition="{{ not config.get('test') }}",airbyte_cdk/sources/declarative/validators/predicate_validator.py (3)
21-22: Consider making the condition field optional, wdyt?Since conditional validation is a new optional feature, should the
conditionfield be typed asOptional[str] = Noneto better reflect that it's not always required? This would make the API more explicit about when conditions are used.- condition: str + condition: Optional[str] = NoneDon't forget to import
Optionalfromtypingif you go with this approach.
24-25: Add null safety to the post_init method, wdyt?The current implementation always creates an
InterpolatedBooleaneven whenconditionmight be None or empty. Consider adding a guard to handle this more gracefully?def __post_init__(self) -> None: - self._interpolated_condition = InterpolatedBoolean(condition=self.condition, parameters={}) + if self.condition: + self._interpolated_condition = InterpolatedBoolean(condition=self.condition, parameters={}) + else: + self._interpolated_condition = NoneYou'd also need to update the condition check in the validate method accordingly.
33-35: Consider adding error handling and updating documentation, wdyt?The condition evaluation logic looks correct, but a couple of suggestions:
Should we handle potential exceptions from
_interpolated_condition.eval()? Condition evaluation errors could be confusing to debug.The class docstring doesn't mention the new conditional behavior - might be worth updating it to reflect this enhancement?
For error handling:
- if self.condition and not self._interpolated_condition.eval(self.config): - return + if self.condition: + try: + if not self._interpolated_condition.eval(self.config): + return + except Exception as e: + raise ValueError(f"Failed to evaluate validator condition '{self.condition}': {e}")For documentation:
class PredicateValidator: """ - Validator that applies a validation strategy to a value. + Validator that applies a validation strategy to a value. + + If a condition is specified, the validation is only performed when the condition + evaluates to true against the provided configuration. """airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
870-879: create_dpath_validator: forwardparametersfrom the model?I see you’ve added
configandconditionsupport—which is great—but unlike most factory methods, you’re not forwardingmodel.parametersinto theDpathValidator. Do we need to passparameters=model.parameters or {}here for interpolation inside the validator? wdyt?
881-892: create_predicate_validator: consider includingparametersas wellSimilar to
DpathValidator, you’ve wired inconfigandconditiononPredicateValidatorbut didn’t forwardmodel.parameters. Would it make sense to addparameters=model.parameters or {}to the constructor to keep it consistent with other components? wdyt?airbyte_cdk/sources/declarative/validators/dpath_validator.py (3)
26-27: Consider making the new fields more explicit about their optionality?The new
configandconditionfields look good for supporting conditional validation. However, I'm wondering if these should be typed as optional or have default values, since conditions might not always be needed? What do you think about making this more explicit - wdyt?- config: Config - condition: str + config: Config + condition: str = ""Or alternatively with Optional typing if conditions are truly optional?
48-50: Consider adding error handling for condition evaluation?The conditional logic looks good and implements the feature as intended! However, I'm wondering if we should add some safety around the condition evaluation? What happens if
self._interpolated_condition.eval(self.config)throws an exception or ifself.configis None?Maybe something like this to be more defensive - wdyt?
- if self.condition and not self._interpolated_condition.eval(self.config): - return + if self.condition: + try: + if not self._interpolated_condition.eval(self.config): + return + except Exception as e: + raise ValueError(f"Error evaluating validator condition '{self.condition}': {e}")
41-47: Update docstring to mention conditional behavior?The docstring for the
validatemethod doesn't mention the new conditional validation behavior. Would it be helpful to update it to reflect that validation can be skipped based on conditions - wdyt?def validate(self, input_data: dict[str, Any]) -> None: """ - Extracts the value at the specified path and applies the validation strategy. + Extracts the value at the specified path and applies the validation strategy. + Validation is skipped if the condition evaluates to false. :param input_data: Dictionary containing the data to validate :raises ValueError: If the path doesn't exist or validation fails """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(2 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(3 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(1 hunks)airbyte_cdk/sources/declarative/validators/dpath_validator.py(3 hunks)airbyte_cdk/sources/declarative/validators/predicate_validator.py(2 hunks)unit_tests/sources/declarative/validators/test_dpath_validator.py(7 hunks)unit_tests/sources/declarative/validators/test_predicate_validator.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
unit_tests/sources/declarative/validators/test_predicate_validator.py (2)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
PredicateValidator(2024-2050)unit_tests/sources/declarative/validators/test_dpath_validator.py (3)
validate(18-22)test_given_condition_is_false_when_validate_then_validate_is_not_called(126-137)MockValidationStrategy(11-22)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (15)
unit_tests/sources/declarative/validators/test_predicate_validator.py (2)
1-2: Copyright header update looks good!Nice to see the copyright year updated to 2025.
29-29: Good approach to maintain backward compatibility.Adding the new
config={}andcondition=""parameters with sensible defaults ensures existing behavior is preserved. This is a clean way to introduce the conditional validation feature, wdyt?unit_tests/sources/declarative/validators/test_dpath_validator.py (3)
1-2: Copyright header update is consistent.Good to see the year updated consistently across test files.
28-33: Consistent parameter addition maintains backward compatibility.The approach of adding
config={}andcondition=""with empty defaults is applied consistently across all existing tests. This ensures that the new conditional validation feature doesn't break existing functionality.
126-137: Well-implemented conditional validation test!This test effectively verifies the conditional validation behavior and mirrors the approach used in the PredicateValidator tests, which shows good consistency across the codebase. The test logic is sound - the condition
{{ not config.get('test') }}correctly evaluates toFalsewith the provided config, ensuring validation is skipped.The test data structure with nested objects (
{"user": {"profile": {"email": "test@example.com"}}}) also properly exercises the DpathValidator's path traversal functionality, wdyt?airbyte_cdk/sources/declarative/declarative_component_schema.yaml (2)
4279-4288: Looks great!
The newconditionfield forDpathValidatoris correctly declared with a default empty string, proper interpolation context, and clear examples. wdyt?
4323-4332: LGTM!
Theconditionproperty forPredicateValidatormirrors theDpathValidatoraddition and maintains consistency in defaults, context, and examples. wdyt?airbyte_cdk/sources/declarative/validators/predicate_validator.py (1)
8-10: LGTM on the import additions!The new imports for
InterpolatedBooleanandConfigare necessary for the conditional validation functionality and are appropriately placed.airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
2013-2021: Add optionalconditionto DpathValidator looks good
The newconditionfield aligns perfectly with the schema changes and enables skipping validation based on a template expression. wdyt?
2042-2050: Add optionalconditionto PredicateValidator looks good
This mirrors theDpathValidatorupdate and provides the same conditional flexibility for predicate-based validation. wdyt?airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (3)
2489-2493: create_types_map: newconditiondefault looks goodYou’ve introduced
conditionwith a fallback to"True"so that mappings are applied by default. This aligns withTypesMapsemantics—looks solid.
2997-2999: create_record_filter: conditional filtering enabledThe addition of
condition=model.condition or ""along with passingconfigandparameterscorrectly enables skipping filters when not needed. Implementation matches the pattern used elsewhere.
3096-3098: create_remove_fields: conditional removal looks correct
condition=model.condition or ""has been added toRemoveFields, enabling optional field removal. This change is consistent withConfigRemoveFieldsand other transformations.airbyte_cdk/sources/declarative/validators/dpath_validator.py (2)
10-10: LGTM on the new imports!The imports for
InterpolatedBooleanandConfigare correctly added and align with the new functionality.Also applies to: 14-14
30-30: Question about the interpolation pattern consistency?I notice that the
InterpolatedBooleanis created with empty parameters{}, but later in thevalidatemethod it's evaluated withself.config. This seems different from the field path pattern where both creation and evaluation use empty contexts.Is this intentional because conditions need access to config during evaluation? Just want to make sure this follows the expected pattern for this codebase - wdyt?
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
unit_tests/sources/declarative/spec/test_spec.py (1)
212-276: 🛠️ Refactor suggestionConsider adding tests for the new conditional validation feature
While these changes correctly adapt the existing tests to the new API, it looks like we're missing test coverage for the actual conditional validation functionality. Would you like me to help generate some test cases that verify validators are skipped/executed based on different condition values? This would ensure the new feature works as expected, wdyt?
🧹 Nitpick comments (2)
unit_tests/sources/declarative/spec/test_spec.py (2)
237-239: Quick question about the empty condition behaviorThe addition of
config=input_configandcondition=""parameters aligns with the new conditional validation feature. However, I'm curious about the empty condition string - does this mean the validation will always run, or does it get skipped? Would it be clearer to use a more explicit value likecondition="true"or document the expected behavior, wdyt?
269-271: Same question about condition behaviorSame observation as the previous test - the empty
condition=""works, but would benefit from clarification on whether this means "always validate" or "never validate", wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unit_tests/sources/declarative/spec/test_spec.py(3 hunks)
⏰ 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.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
unit_tests/sources/declarative/spec/test_spec.py (2)
213-213: Good initialization timing!Moving the
input_configinitialization before thecomponent_speccreation makes sense since it's now needed for theDpathValidatorconstructor.
246-246: Consistent initialization patternGood consistency with the first test function - initializing
input_configearly for use in the validator constructor.
|
Closing in favor of simply skipping validation if returned values are falsy: #590 |
What
Summary by CodeRabbit