feat: make use_parent_params configurable #633
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds configurability for the use_parent_parameters flag in dynamic streams, allowing connectors to override the default parent-parameter precedence.
- Introduces a new
use_parent_parametersfield in the Python and YAML schemas (defaulting toTrue). - Updates the manifest loader to read the flag from each dynamic definition instead of hardcoding
True. - Adds unit tests covering
use_parent_parametersbehavior.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| unit_tests/sources/declarative/parsers/test_manifest_component_transformer.py | Adds tests for use_parent_parameters configuration |
| airbyte_cdk/sources/declarative/models/declarative_component_schema.py | Defines new Pydantic field use_parent_parameters |
| airbyte_cdk/sources/declarative/manifest_declarative_source.py | Reads use_parent_parameters from dynamic definitions |
| airbyte_cdk/sources/declarative/declarative_component_schema.yaml | Adds use_parent_parameters to the JSON schema default |
Comments suppressed due to low confidence (2)
unit_tests/sources/declarative/parsers/test_manifest_component_transformer.py:773
- These
printstatements do not verify behavior; replace them with explicit assertions to validate expected output.
print("use_parent_parameters=True:", result_true)
airbyte_cdk/sources/declarative/manifest_declarative_source.py:558
- The variable
dynamic_definitionis not defined in this scope; it should bedynamic_streamto match the loop variable.
use_parent_parameters = dynamic_definition.get("use_parent_parameters", True)
unit_tests/sources/declarative/parsers/test_manifest_component_transformer.py
Outdated
Show resolved
Hide resolved
📝 WalkthroughWalkthroughA new boolean property, Changes
Sequence Diagram(s)sequenceDiagram
participant ManifestDeclarativeSource
participant ManifestComponentTransformer
ManifestDeclarativeSource->>ManifestDeclarativeSource: Read dynamic_definition
ManifestDeclarativeSource->>ManifestDeclarativeSource: Extract use_parent_parameters (default True)
ManifestDeclarativeSource->>ManifestComponentTransformer: propagate_types_and_parameters(..., use_parent_parameters)
ManifestComponentTransformer-->>ManifestDeclarativeSource: Return transformed component with correct parameter precedence
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
✨ 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 (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
4238-4242: Nice addition — a small doc tweak?
use_parent_parametersis well-defined and the default keeps us backward-compatible 👍.
Would it help future authors if we also add anexamples:array (e.g.,- false) so generated docs show the toggle in action, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/manifest_declarative_source.py(1 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(2 hunks)unit_tests/sources/declarative/parsers/test_manifest_component_transformer.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
airbyte_cdk/sources/declarative/manifest_declarative_source.py (3)
Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in `airbyte_cdk/cli/source_declarative_manifest/`, including `_run.py`, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/spec.json:9-15
Timestamp: 2024-11-15T00:59:08.154Z
Learning: When code in `airbyte_cdk/cli/source_declarative_manifest/` is being imported from another repository, avoid suggesting modifications to it during the import process.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (4)
undefined
<retrieved_learning>
Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, the declarative_component_schema.py file is auto-generated from declarative_component_schema.yaml and should be ignored in the recommended reviewing order.
</retrieved_learning>
<retrieved_learning>
Learnt from: ChristoGrab
PR: #58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the YamlDeclarativeSource class in airbyte_cdk/sources/declarative/yaml_declarative_source.py, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
</retrieved_learning>
<retrieved_learning>
Learnt from: aaronsteers
PR: #58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in airbyte_cdk/cli/source_declarative_manifest/, including _run.py, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.
</retrieved_learning>
<retrieved_learning>
Learnt from: aaronsteers
PR: #174
File: airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py:1093-1102
Timestamp: 2025-01-14T00:20:32.310Z
Learning: In the airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py file, the strict module name checks in _get_class_from_fully_qualified_class_name (requiring module_name to be "components" and module_name_full to be "source_declarative_manifest.components") are intentionally designed to provide early, clear feedback when class declarations won't be found later in execution. These restrictions may be loosened in the future if the requirements for class definition locations change.
</retrieved_learning>
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (3)
undefined
<retrieved_learning>
Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, the declarative_component_schema.py file is auto-generated from declarative_component_schema.yaml and should be ignored in the recommended reviewing order.
</retrieved_learning>
<retrieved_learning>
Learnt from: ChristoGrab
PR: #58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the YamlDeclarativeSource class in airbyte_cdk/sources/declarative/yaml_declarative_source.py, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.
</retrieved_learning>
<retrieved_learning>
Learnt from: aaronsteers
PR: #58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in airbyte_cdk/cli/source_declarative_manifest/, including _run.py, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.
</retrieved_learning>
🧬 Code Graph Analysis (1)
airbyte_cdk/sources/declarative/manifest_declarative_source.py (1)
airbyte_cdk/sources/declarative/parsers/manifest_component_transformer.py (2)
ManifestComponentTransformer(91-226)propagate_types_and_parameters(92-193)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: source-intercom
- GitHub Check: Check: source-amplitude
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-shopify
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
🔇 Additional comments (6)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
2963-2967: LGTM! Correctly implements configurableuse_parent_parametersfeature.The addition of the
use_parent_parametersfield toDynamicDeclarativeStreamproperly addresses the PR objectives. The implementation maintains backward compatibility with the defaultTruevalue while allowing connectors to override parent parameters when needed. The field typing, description, and title are all appropriate.Just noting that this file is auto-generated from
declarative_component_schema.yaml, so the real source of truth would be the YAML schema file, wdyt?airbyte_cdk/sources/declarative/manifest_declarative_source.py (2)
556-558: LGTM! Clean parameter extraction with good backward compatibility.The implementation correctly extracts the
use_parent_parametersflag from the dynamic definition with a sensible default value. The comment clearly explains the backward compatibility rationale.
562-562: Perfect! The hardcoded value is now properly configurable.This change enables the intended functionality while preserving existing behavior through the default value.
unit_tests/sources/declarative/parsers/test_manifest_component_transformer.py (3)
574-649: Excellent test coverage for the parameter precedence logic!This test thoroughly validates both
TrueandFalsescenarios foruse_parent_parameters, ensuring the flag correctly controls which parameters take precedence. The test structure is clear and the assertions verify the expected behavior well.
652-699: Great backward compatibility validation!This test ensures that
use_parent_parameters=Nonemaintains the existing behavior, which is crucial for not breaking existing implementations. The test clearly validates the default precedence logic.
702-749: Comprehensive validation of dynamic stream parameter behavior!This test excellently validates the
use_parent_parametersfunctionality in the dynamic stream context, covering both scenarios where only parent parameters exist and where both parent and component parameters are present. This ensures the feature works as intended in its primary use case, wdyt?
What
In version 6.43.1 we introduced the
use_parent_parametersparam to allow parent parameters to take precedence over component parameters when propagating them in a component. However, because we setuse_parent_parametersto always be true for dynamic streams, connectors that need to override parent parameters dynamically can no longer do so. This PR makes the parameter configurable in the manifest. We still default to True as I do not want to risk breaking connectors that are already using the parent_param logic.How
use_parent_parametersto the DeclarativeDynamicStream schema, making it configurable in the manifest.Summary by CodeRabbit
New Features
Tests
Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.