fix: add array type to field_pointers items #658
Conversation
lmossman
requested review from
Copilot and
ChristoGrab
and removed request for
Copilot
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@lmossman/add-array-type#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch lmossman/add-array-typeHelpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
There was a problem hiding this comment.
Pull Request Overview
This PR ensures the field_pointers.items schema is correctly declared as an array by adding type: array in the YAML, and regenerates the Python models to reflect that change.
- Added
type: arrayto the outeritemsinfield_pointersin the YAML schema. - Regenerated the Python datamodel, which updated ordering and details of
CustomConfigTransformation.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| airbyte_cdk/sources/declarative/declarative_component_schema.yaml | Added type: array to outer items in field_pointers definition. |
| airbyte_cdk/sources/declarative/models/declarative_component_schema.py | Regenerated model code; moved and updated CustomConfigTransformation. |
Comments suppressed due to low confidence (2)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py:1646
- [nitpick] The
title="Class Name"argument was removed from thisField, which may affect autogenerated docs or UI labels. Consider re-adding thetitleto keep the schema documentation consistent.
class_name: str = Field(
airbyte_cdk/sources/declarative/models/declarative_component_schema.py:1644
- The inner
Configclass withextra = Extra.allowwas removed in this regeneration, which changes Pydantic's validation behavior for extra fields. Re-addclass Config: extra = Extra.allowto maintain existing model behavior.
class CustomConfigTransformation(BaseModel):
📝 WalkthroughWalkthroughThis update clarifies the schema for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConfigMigration
participant ConfigNormalizationRules
participant CustomConfigTransformation
User->>ConfigMigration: Provide transformation config (may include CustomConfigTransformation)
ConfigMigration->>CustomConfigTransformation: Invoke transformation logic
User->>ConfigNormalizationRules: Provide normalization rules (may include CustomConfigTransformation)
ConfigNormalizationRules->>CustomConfigTransformation: Invoke transformation logic
Would you like me to draft a more detailed diagram or breakdown of the new 📜 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
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/models/declarative_component_schema.py (1)
1-3: Note: This is auto-generated code - focus review on source YAML file.Based on the file header and previous learnings, this file is auto-generated from
declarative_component_schema.yaml. While the generated changes look correct, the real review should focus on the source YAML file to ensure the schema definitions are properly structured there. The changes here are just the Python representation of those schema updates, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (3)
undefined
<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: 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: aaronsteers
PR: #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.
</retrieved_learning>
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (5)
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: 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>
<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: ChristoGrab
PR: #221
File: airbyte_cdk/sources/utils/transform.py:0-0
Timestamp: 2025-01-16T00:50:39.069Z
Learning: In the TypeTransformer class, the data being transformed comes from API responses or source systems, so only standard JSON-serializable types are expected. The python_to_json mapping covers all expected types, and it's designed to fail fast (KeyError) on unexpected custom types rather than providing fallbacks.
</retrieved_learning>
🧬 Code Graph Analysis (1)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
airbyte_cdk/sources/declarative/transformations/config_transformations/remap_field.py (1)
ConfigRemapField(16-65)airbyte_cdk/sources/declarative/transformations/config_transformations/add_fields.py (1)
ConfigAddFields(22-106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Check: source-shopify
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-intercom
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-pokeapi
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
3397-3404: Innertype: arrayaddition looks good – have you regenerated the auto-generated.pyschema as well, wdyt?The new
type: arrayunderfield_pointers.itemscorrectly tells the validator that each pointer is itself an array.
Becausedeclarative_component_schema.pyis generated from this YAML, please make sure you run the generation script so the Python artefact stays in sync and downstream builds don’t pick up mismatched schemas.airbyte_cdk/sources/declarative/models/declarative_component_schema.py (3)
1644-1656: LGTM! The CustomConfigTransformation class follows established patterns.The class definition is well-structured and consistent with other custom transformation classes in the file. The field descriptions are clear and the parameter structure aligns with the existing codebase patterns.
1165-1170: Good addition of CustomConfigTransformation to ConfigMigration transformations.The union type expansion properly includes the new CustomConfigTransformation alongside the existing transformation types. The formatting is consistent and the change enables custom config transformations to be used in config migrations, which seems like a logical extension.
1190-1195: Consistent addition of CustomConfigTransformation to ConfigNormalizationRules.The union type expansion mirrors the ConfigMigration changes and maintains consistency across the schema. This enables custom config transformations to be used in normalization rules as well.
However, I notice the PR objectives mention adding
type: arrayto field_pointers items, but I don't see those specific changes in the visible code. Could you clarify if those changes are in the YAML source file that generates this code, or if they're related to the RemoveFields transformation mentioned in the AI summary? Just want to make sure we're addressing the core issue described in the PR objectives, wdyt?
The
itemsfield offield_pointersin declarative_component_schema has a nesteditemsfield, since that is meant to contain nested array values.However, there is no
type: arrayon the outeritemsschema, which causes issues in the Builder UI as it relies on the types of fields to know what to render.This PR fixes the issue by explicitly declaring
type: arrayon the outeritemsfield.Summary by CodeRabbit
New Features
Bug Fixes