feat: adds new ValidateInLineCondition component
#580
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new configuration validation strategy that evaluates Jinja expressions in-line.
- Introduces the ValidateInLineCondition validator with associated unit tests
- Updates the component schema, factory, and module exports to support the new validation strategy
- Adds the dagger-io dependency required for the assemble task
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| unit_tests/sources/declarative/validators/test_validate_in_line_condition.py | Added comprehensive unit tests for the new validator |
| pyproject.toml | Added dagger-io dependency |
| airbyte_cdk/sources/declarative/validators/validate_in_line_condition.py | New implementation of ValidateInLineCondition |
| airbyte_cdk/sources/declarative/validators/init.py | Exported ValidateInLineCondition for external use |
| airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py | Added factory method to support the new component |
| airbyte_cdk/sources/declarative/models/declarative_component_schema.py | Updated model schema union types to include ValidateInLineCondition |
| airbyte_cdk/sources/declarative/declarative_component_schema.yaml | Added definition for ValidateInLineCondition |
airbyte_cdk/sources/declarative/declarative_component_schema.yaml
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughA new validation strategy, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Validator (PredicateValidator/DpathValidator)
participant ValidateInLineCondition
participant Config
User->>Validator: Provide value and config
Validator->>ValidateInLineCondition: Call validate(value)
ValidateInLineCondition->>Config: Access config for interpolation
ValidateInLineCondition->>ValidateInLineCondition: Evaluate Jinja condition
alt Condition is True
ValidateInLineCondition-->>Validator: Validation passes
else Condition is False or Error
ValidateInLineCondition-->>Validator: Raise ValueError
end
Validator-->>User: Return result or error
Possibly related PRs
Suggested reviewers
By the way, would you like to discuss if the new validation strategy should be documented in user-facing docs as well, 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: 1
🧹 Nitpick comments (7)
unit_tests/sources/declarative/validators/test_validate_in_line_condition.py (2)
26-28: Consider removing the duplicate assertion?Both assertions check for "evaluated to False" in the error message. Would you like to keep just one of them or check for different parts of the error message, wdyt?
assert "Condition evaluated to False" in str(exc_info.value) - assert "evaluated to False" in str(exc_info.value)
88-95: Consider if this error message is most helpful for missing keys?While this test works correctly, accessing a missing config key (
config['non_existent_key']) might benefit from a more specific error message than "Condition evaluated to False". Would a message like "Missing configuration key" be more helpful for debugging, wdyt?airbyte_cdk/sources/declarative/validators/validate_in_line_condition.py (1)
31-38: Consider more specific exception handling?The logic flow is excellent, but the broad
except Exceptionon line 37 might catch more than intended. Would it be better to catch specific exceptions likeKeyErrorfor missing config keys orTypeErrorfor type issues, allowing unexpected errors to bubble up with their original context, wdyt?try: result = interpolated_condition.eval(self.config) except TemplateError as e: raise ValueError(f"Invalid jinja expression: {value}.") from e - except Exception as e: - raise ValueError(f"Unexpected error evaluating condition: {value}.") from e + except (KeyError, TypeError, AttributeError) as e: + raise ValueError(f"Configuration or evaluation error: {value}. {str(e)}") from eairbyte_cdk/sources/declarative/declarative_component_schema.yaml (4)
4277-4278: Order validation strategies alphabetically for consistency?You’ve added
ValidateInLineConditionalongsideValidateAdheresToSchemaunder DpathValidator’svalidation_strategy.anyOf. To keep the list tidy, would you consider alphabetizing these entries (e.g.Adheres…thenInLine…)? wdyt?
4307-4308: Centralize inline condition example?Great call showing
"{{ config['start_date'] < now_utc() }}"in the PredicateValidator examples. Would it help to also add this sample under theValidateInLineConditiondefinition (or link back to it) so users can find the example in one place? wdyt?
4313-4314: Consistent ordering in PredicateValidator.anyOf?You mirrored the inline strategy addition in PredicateValidator’s
validation_strategy.anyOf. Similar to DpathValidator, would you mind alphabetizing the two strategy refs here for uniformity? wdyt?
4315-4324: Add examples to the new strategy definition?The
ValidateInLineConditionschema block looks solid. To improve discoverability, how about adding anexamples:section under its properties to show a minimal usage snippet? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(2 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(4 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(3 hunks)airbyte_cdk/sources/declarative/validators/__init__.py(2 hunks)airbyte_cdk/sources/declarative/validators/validate_in_line_condition.py(1 hunks)pyproject.toml(2 hunks)unit_tests/sources/declarative/validators/test_validate_in_line_condition.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
airbyte_cdk/sources/declarative/validators/validate_in_line_condition.py (3)
airbyte_cdk/sources/declarative/interpolation/interpolated_boolean.py (1)
InterpolatedBoolean(29-66)airbyte_cdk/sources/declarative/validators/validation_strategy.py (1)
ValidationStrategy(9-22)airbyte_cdk/sources/declarative/models/declarative_component_schema.py (16)
Config(136-137)Config(150-151)Config(164-165)Config(178-179)Config(196-197)Config(210-211)Config(224-225)Config(238-239)Config(252-253)Config(266-267)Config(280-281)Config(294-295)Config(310-311)Config(324-325)Config(338-339)Config(372-373)
unit_tests/sources/declarative/validators/test_validate_in_line_condition.py (2)
airbyte_cdk/sources/declarative/validators/validate_in_line_condition.py (2)
ValidateInLineCondition(16-43)validate(23-43)airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
ValidateInLineCondition(1537-1538)
airbyte_cdk/sources/declarative/validators/__init__.py (2)
airbyte_cdk/sources/declarative/validators/validate_in_line_condition.py (1)
ValidateInLineCondition(16-43)airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
ValidateInLineCondition(1537-1538)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
airbyte_cdk/sources/declarative/validators/validate_in_line_condition.py (1)
ValidateInLineCondition(16-43)
🪛 Pylint (3.3.7)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py
[refactor] 1537-1537: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
🔇 Additional comments (18)
pyproject.toml (2)
95-95: Addeddagger-ioto dev dependencies – looks good.
This new development dependency is required for theassembletask and is correctly scoped under[tool.poetry.group.dev.dependencies].
236-236: Includeddagger-ioin DEP002 ignore list – correct.
Matching the dev dependency addition,dagger-iois now ignored for unused dependency checks with an appropriate comment.airbyte_cdk/sources/declarative/validators/__init__.py (2)
10-12: Imported newValidateInLineConditionstrategy – correct.
The new validator is now available in the package namespace and aligns with the implementation invalidate_in_line_condition.py.
22-22: ExportedValidateInLineConditionin__all__– good to go.
This ensures the new validation strategy is part of the public API.airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
427-429: LGTM on the import!The import of
ValidateInLineConditionModellooks good and follows the established pattern for other model imports.
573-573: LGTM on the validator import!The import of
ValidateInLineConditionis correctly placed with other validator imports.unit_tests/sources/declarative/validators/test_validate_in_line_condition.py (5)
13-17: LGTM! Clean and straightforward positive test case.This test effectively validates the happy path scenario with a simple but representative Jinja expression.
29-39: Excellent comprehensive type checking!Love how you've covered all the important non-string types in a concise loop. This provides great coverage for input validation.
41-68: Great use of parametrized testing for complex scenarios!This test beautifully demonstrates the validator's capability with nested configs and complex logical expressions. The parametrization makes it easy to add more test cases in the future.
70-77: Good edge case coverage!Testing empty string is important since it's a valid string but would evaluate to falsy. The expected error message is appropriate.
79-86: Perfect syntax error handling test!The invalid Jinja expression is clearly malformed and the expected error message appropriately identifies the issue.
airbyte_cdk/sources/declarative/validators/validate_in_line_condition.py (3)
15-21: Clean class structure and appropriate inheritance!The dataclass approach with ValidationStrategy inheritance is perfect for this use case. Nice clear docstring too.
40-43: Excellent error handling with clear, actionable messages!The error messages are descriptive and will help users understand both what went wrong and what's expected. Perfect implementation.
23-29: Well-documented method with comprehensive docstring!The parameter documentation and exception specifications are exactly what developers need to understand the method's behavior.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (4)
1537-1539: Simple and clean model definition!The
ValidateInLineConditionmodel follows the established pattern nicely. The static analysis hint about "too few public methods" is a false positive - Pydantic models inherit validation methods from the base class, so no additional methods are needed here, wdyt?🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 1537-1537: Too few public methods (0/2)
(R0903)
2012-2016: Great integration with existing validation patterns!The Union type update to include
ValidateInLineConditionalongsideValidateAdheresToSchemais exactly what's needed. This allows users to choose between schema validation and inline condition evaluation for field path validation, which provides nice flexibility, wdyt?
2028-2028: Perfect example for the new functionality!The Jinja expression example
"{{ config['start_date'] < now_utc() }}"clearly demonstrates how inline conditions can be used for date comparisons. This aligns perfectly with the PR objectives and gives users a concrete example to follow, wdyt?
2033-2037: Consistent implementation across validators!The
PredicateValidatorupdate mirrors theDpathValidatorchanges perfectly, maintaining consistency in how validation strategies are handled across different validator types. This ensures a uniform API experience, wdyt?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
745-745: Mapping registration looks perfect!Great to see this was already addressed from the previous review feedback. The registration correctly maps
ValidateInLineConditionModelto the factory method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
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(4 hunks)pyproject.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🪛 Pylint (3.3.7)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py
[refactor] 1562-1562: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: source-intercom
- GitHub Check: Check: source-shopify
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-amplitude
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: SDM Docker Image Build
🔇 Additional comments (8)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
4354-4359: New validation strategy correctly referenced 👍
ValidateInLineConditionis now an accepted option forvalidation_strategyon bothDpathValidatorandPredicateValidator, so the schema will successfully validate manifests that use the new component.
Nothing blocking here.Also applies to: 4389-4395
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (4)
1562-1564: LGTM! Simple and clean implementation for the new validation strategy.The
ValidateInLineConditionclass follows the expected pattern for Pydantic schema models in this codebase. The minimal implementation with just a type literal is appropriate since the actual validation logic would be implemented elsewhere in the runtime components, wdyt?Note: The pylint warning about "too few public methods" is a false positive here - Pydantic models used for schema definitions typically have minimal method implementations.
2037-2041: Good integration of the new validation strategy.The addition of
ValidateInLineConditionto theDpathValidator.validation_strategyUnion type properly extends the validator to support inline condition evaluation. The field description and title remain accurate for the expanded functionality.
2053-2053: Nice example showing inline condition usage!The addition of
"{{ config['start_date'] < now_utc() }}"as an example effectively demonstrates how the new inline condition validation strategy can be used with Jinja expressions. This helps users understand the practical application of the feature, wdyt?
2058-2062: Consistent integration across validation components.The update to
PredicateValidator.validation_strategymirrors the change made toDpathValidator, ensuring both validators can leverage the new inline condition validation strategy. The implementation maintains consistency across the validation framework.airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (3)
430-432: LGTM on the model import!The import follows the established pattern and is correctly placed with other declarative component schema imports.
578-578: Import looks good!Correctly placed with other validator imports and follows the established pattern.
908-912: Clean factory method implementation - wdyt about the minimal approach?The implementation looks good and follows the established pattern. I notice it only passes
configto the constructor, which seems intentional given thatValidateInLineConditionevaluates Jinja expressions directly and likely only needs config access for interpolation. The actual expressions would come from the parent validator components (likeDpathValidatororPredicateValidator), right?This minimal approach makes sense for this use case!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unit_tests/sources/declarative/validators/test_predicate_validator.py (1)
31-31: LGTM! Test updates are consistent with the new interface.The test calls have been consistently updated to match the new
validatemethod signature. The core test logic remains intact, which is good.One question though - should we consider adding test cases that actually use the
input_dataparameter in some meaningful way, or is the empty dict sufficient for now? wdyt?Also applies to: 43-43, 54-54
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/validators/predicate_validator.py(1 hunks)unit_tests/sources/declarative/validators/test_predicate_validator.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: source-amplitude
- GitHub Check: Check: source-intercom
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-shopify
- 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 (3)
airbyte_cdk/sources/declarative/validators/predicate_validator.py (3)
9-9: Good addition - aligns with the validation framework.Adding the import for the
Validatorbase class supports the consistent interface approach.
13-13: Excellent - inheritance brings consistency to the validation framework.Making
PredicateValidatorinherit fromValidatorestablishes a consistent interface across all validators, which is a solid architectural decision.
21-27: Method signature updated correctly, but curious about the unused parameter.The method signature change aligns well with the
Validatorinterface. However, I notice thatinput_datais accepted but never used in the validation logic.Is this intentional for interface consistency, or might there be scenarios where
PredicateValidatorshould leverageinput_datafor validation context? For instance, with the newValidateInLineConditionstrategy, couldinput_dataprovide context for Jinja expression evaluation? wdyt?Could you clarify the intended usage of the
input_dataparameter? Should it remain unused, or are there plans to incorporate it into the validation logic?
What
ValidateInLineConditionExample Usage
Summary by CodeRabbit