chore: reorder and modify title/defaults for partition router fields #626
Conversation
lmossman
force-pushed
the
lmossman/more-reordering-in-declarative-schema
branch
from
0c28494 to
951c9e0
Compare
There was a problem hiding this comment.
Pull Request Overview
A chore to reorder and update titles and defaults for partition router fields to improve Builder UI behavior.
- Reordered
ParentStreamConfigfields and movedlazy_read_pointerbelowincremental_dependency. - Changed
partition_routerdefault from an empty list toNoneand updated its description. - Synced YAML schema: mirrored Python changes, removed obsolete defaults, and added a title for multiple routers.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| airbyte_cdk/sources/declarative/models/declarative_component_schema.py | Reordered fields in ParentStreamConfig and updated partition_router default and description. |
| airbyte_cdk/sources/declarative/declarative_component_schema.yaml | Mirrored field order changes, removed default arrays, added Multiple Partition Routers title. |
Comments suppressed due to low confidence (4)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py:2789
- Changing the default of
partition_routerfrom[]toNonealters the API contract and could break downstream code that expects a list. Consider documenting this breaking change or providing a migration path.
None,
airbyte_cdk/sources/declarative/declarative_component_schema.yaml:3138
- The YAML title for
partition_field(Current Parent Key Value Identifier) does not match the Python schema title (Partition Field). Align these titles to avoid confusion in the UI.
title: Current Parent Key Value Identifier
airbyte_cdk/sources/declarative/declarative_component_schema.yaml:3593
- The YAML schema no longer specifies a default for
partition_router. To clearly reflect the Python default ofNone, consider addingdefault: nullin the schema.
partition_router:
airbyte_cdk/sources/declarative/models/declarative_component_schema.py:2773
- With the updated default behavior of
partition_routerbeingNone, add or update tests to verify that downstream code handles aNonevalue correctly.
partition_router: Optional[
📝 WalkthroughWalkthroughThe changes involve reordering properties and updating descriptions in both the YAML schema and Python model for declarative components. No structural or functional logic was altered; only the order of fields, default values, and property descriptions were modified to improve clarity and consistency. Changes
Sequence Diagram(s)No sequence diagram generated, as the changes are limited to property order and descriptions without affecting control flow or interactions. Would you like to see a suggestion for documenting these changes in your changelog, 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 (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
3595-3599: Updated description reads well – mirror it in docs?The new wording for
partition_routeris clearer. Should the public docs/snippets be refreshed to keep wording consistent, 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(3 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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.
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.
Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#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.
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.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (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>
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (4)
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/_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: #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>
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-intercom
- GitHub Check: Check: source-amplitude
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-shopify
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
🔇 Additional comments (4)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
1-2963: Auto-generated file - changes appear to be correctly propagated from the source schema.Based on the retrieved learnings, this file is auto-generated from
declarative_component_schema.yamland typically should be ignored during code review. The changes visible here (field reordering inParentStreamConfigandSimpleRetrieverclasses, and default value updates) appear to be correctly propagated from the underlying YAML schema changes, which aligns with the PR objective of improving Builder UI appearance and behavior.Since this is generated code, manual review of implementation details isn't necessary - the focus should be on ensuring the source schema changes are appropriate, wdyt?
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (3)
3124-3130: Property order change looks safeRe-ordering
streamto the top of the properties list shouldn’t affect validation, and it will surface the most-relevant field first in Builder UI – nice touch! 👍 wdyt?
3153-3162: Double-check downstream UI expectations after movinglazy_read_pointerMoving this block farther down is inert for pure JSON-Schema validation, but a few internal rendering tools rely on the YAML order to decide field placement. Could you verify the Builder still renders
lazy_read_pointerwhere you expect, just to be safe, wdyt?
3602-3607: Array-type title addition LGTMAdding the “Multiple Partition Routers” title will improve UI grouping without breaking existing manifests – looks good!
## What This PR overrides the parent stream config `stream` in the Builder UI to render a dropdown allowing the user to simply select the parent stream. This is necessary because the [schema for the parent stream field](https://github.com/airbytehq/airbyte-python-cdk/blob/5824a5eabcd4e4bd72cbb06af7d6f18430ca1be5/airbyte_cdk/sources/declarative/declarative_component_schema.yaml#L3164-L3169) is simply a nested stream, so the default SchemaForm behavior is to render an entire nested stream form UI for this field, like in this screenshot:  This is not a good UX, as we don't want to show an entire nested stream form in this case. ## How This PR solves the issue by using SchemaForm overrides to render the custom [ParentStreamSelector component](https://github.com/airbytehq/airbyte-platform-internal/pull/16809/files#diff-ca8382b13f366cc8915a7548db4813bf52b3675e444dfda0963b4e305a40425cR895), which checks if the value contains a `$ref` (which it should as we are normalizing the manifest when loading into into the UI - see airbytehq/airbyte-platform-internal#16708 for details on that), and simply renders a dropdown menu containing the names of all other streams. When the user selects a stream from this menu, this component writes a `$ref` to that field pointing to the selected stream. Here is what this looks like:  As you can see, this is a much more reasonable user experience for configuring a parent stream. Note that the field order is slightly different than you would see when testing this branch, as that screenshot includes the changes from my python CDK PR [here](airbytehq/airbyte-python-cdk#626). --- Using a `$ref` for the parent stream field in the actual form required making a few other changes to make everything play nicely: - [ConnectorBuilderStateService.tsx](https://github.com/airbytehq/airbyte-platform-internal/pull/16809/files#diff-9d461bd6ebcf80e74b63d4061cf322a9979e5542b28857d139f8c82b6aaf4c5b) - we can no longer filter the manifest down to just the stream being tested when running a test read, as it needs the other streams in case any are being pointed to with a `$ref` - [useBuilderErrors.ts](https://github.com/airbytehq/airbyte-platform-internal/pull/16809/files#diff-3e6cff93a47accbabb30b171c9fbbda89012c3393deb1389aeca18ed5b6f9edd) and [useFocusField.ts](https://github.com/airbytehq/airbyte-platform-internal/pull/16809/files#diff-e9f3e416dcb1d95b7c718d1cb8509f668279c447f682ad5c2e72743bd530b4ad) - drive-by fixes to make the error and focusing behavior work better across different stream views and tabs - [dynamicValidator.ts](https://github.com/airbytehq/airbyte-platform-internal/pull/16809/files#diff-cd80c0cf03bbe7e00e8a3b503cb13fa2f890acecaf7ca35bd91ff844a28f6dd6) - don't try to validate fields with `$ref` values, as they will not conform to the schema - [updateStreamsAndRefsAfterDelete()](https://github.com/airbytehq/airbyte-platform-internal/pull/16809/files#diff-ca8382b13f366cc8915a7548db4813bf52b3675e444dfda0963b4e305a40425cR976) - since streams are pointed to with $refs by _index_, we need to update these $refs when a stream is removed, as that is the only time the index of an existing stream may change.
Some more reordering and modifying titles/defaults, this time for partition router fields.
This is to get things to look and behave as desired in the Builder UI
Summary by CodeRabbit