Skip to content

fix: (CDK) (design) correct field titles/orders in declarative_component_schema.yaml #480

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Apr 25, 2025

Conversation

lmossman
Copy link
Contributor

@lmossman lmossman commented Apr 13, 2025

Now that we are using the declarative_component_schema to automatically generate the Builder UI in https://github.com/airbytehq/airbyte-platform-internal/pull/15845, there are several places where fields lack titles or are ordered non-ideally.

This PR corrects these places to achieve an auto-generated UI that produces a better UX.

This PR is only reordering some fields/anyOf options and changing titles, so it should not be breaking at all and therefore safe to merge.

There will probably be more PRs like this coming in the future as well.

Summary by CodeRabbit

Summary by CodeRabbit

  • Documentation
    • Improved schema clarity by adding and refining titles and descriptions for various fields, including pagination, backoff strategies, secret keys, and stream names.
    • Enhanced property grouping and ordering for better readability and consistency.
    • Added more descriptive metadata and examples to assist with schema usage and understanding.
  • New Features
    • Added support for handling query properties in request parameters to improve request configuration.
    • Introduced default values for certain optional fields to enhance stability and prevent errors during processing.

@lmossman lmossman changed the title Correct fields in declarative_component_schema Correct field titles/orders in declarative_component_schema Apr 23, 2025
@lmossman lmossman requested a review from bazarnov April 23, 2025 04:33
@lmossman lmossman marked this pull request as ready for review April 23, 2025 04:33
@Copilot Copilot AI review requested due to automatic review settings April 23, 2025 04:33
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR reorders fields and updates titles in the declarative_component_schema to improve the auto-generated UI for the Builder. Key changes include updating field titles, reordering anyOf options, and removing duplicate or inconsistent field definitions.

Comments suppressed due to low confidence (3)

airbyte_cdk/sources/declarative/declarative_component_schema.yaml:2004

  • The $ref formatting for QueryProperties has been corrected; please ensure this change aligns with expected reference syntax in the rest of the schema.
-                - $ref: "#/definitions/QueryProperties"

airbyte_cdk/sources/declarative/declarative_component_schema.yaml:3170

  • Removal of the DpathExtractor reference in the extractor's anyOf list removes a fallback option; please confirm that this change is intentional.
-          - "$ref": "#/definitions/DpathExtractor"

airbyte_cdk/sources/declarative/declarative_component_schema.yaml:2000

  • [nitpick] Consider standardizing the naming convention of title values across similar schema fields to maintain a uniform and clear UI experience.
title: Key/Value Pairs

Copy link
Contributor

coderabbitai bot commented Apr 23, 2025

📝 Walkthrough

Walkthrough

The schema file for declarative components was updated to enhance clarity and structure. Titles and descriptions were added or refined for various fields to provide better context. The order of properties and enum values was adjusted in several components, and deprecated components were explicitly marked. New properties such as error_handler were introduced, and existing properties were reorganized for consistency. No changes were made to validation rules or core logic—these updates are limited to schema metadata, property ordering, and documentation improvements. Additionally, the model-to-component factory was updated to set default values for certain optional fields in RecordSelector before component creation and to improve handling and filtering of QueryPropertiesModel instances in request parameters within HttpRequester and SimpleRetriever.

Changes

File(s) Change Summary
airbyte_cdk/sources/declarative/declarative_component_schema.yaml Added and refined title and description fields for better documentation; reordered properties, enums, and anyOf lists for consistency; introduced new properties such as error_handler to HttpRequester; marked LegacySessionTokenAuthenticator as deprecated; updated InlineSchemaLoader to allow additional properties; grouped or reordered component properties for clarity and maintainability.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py Added and reordered union types in multiple declarative component models; introduced new optional fields like name in DeclarativeStream and decoder in SimpleRetriever; refined field types and defaults; added titles and descriptions to many fields; removed deprecated or redundant fields; reordered enum members; updated HttpRequester fields for better typing and metadata; called update_forward_refs() on HttpRequester.
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py Updated create_record_selector method to set default values for transform_before_filtering (to False) and schema_normalization (to SchemaNormalizationModel.None_) when these fields are None in the model, ensuring consistent downstream processing. Also improved handling and filtering of QueryPropertiesModel instances in request parameters within create_http_requester and create_simple_retriever by filtering them out and ensuring only one instance is used. No changes to method signatures or control flow.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant DeclarativeComponentSchema
    participant ModelToComponentFactory

    User ->> DeclarativeComponentSchema: Define or update component configuration
    DeclarativeComponentSchema -->> User: Present enhanced titles, descriptions, and property order
    User ->> DeclarativeComponentSchema: Use new error_handler or decoder properties (if applicable)
    DeclarativeComponentSchema -->> User: Validate and document schema with updated metadata
    User ->> ModelToComponentFactory: Create RecordSelector with optional fields possibly None
    ModelToComponentFactory ->> ModelToComponentFactory: Set defaults for transform_before_filtering and schema_normalization if None
    ModelToComponentFactory -->> User: Return fully initialized RecordSelector component
Loading

Would you like me to generate a more detailed diagram focusing on the new error_handler property flow in HttpRequester? Wdyt?

Possibly related PRs

Suggested labels

chore, declarative-component-schema

Suggested reviewers

  • lmossman
  • maxi297

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3309380 and a147ba1.

📒 Files selected for processing (1)
  • airbyte_cdk/sources/declarative/declarative_component_schema.yaml (27 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • airbyte_cdk/sources/declarative/declarative_component_schema.yaml
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Pytest (Fast)
  • GitHub Check: SDM Docker Image Build
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (3)

1678-1685: Reorder paginator strategies and name options
Switching PageIncrement/OffsetIncrement order and adding titles for page size/token injection options will aid discoverability. Have we confirmed this order aligns with the most common use cases? wdyt?

Also applies to: 1686-1690


3169-3172: Enhance RecordSelector extractor and filtering options
Reordering built-in vs custom extractor/filter anyOf and adding the transform_before_filtering title will improve UI grouping. Could we also add a short example for this new field? wdyt?

Also applies to: 3175-3178, 3185-3188


3298-3298: Mark LegacySessionTokenAuthenticator as deprecated
Flagging this authenticator as deprecated will prompt users to migrate to SessionTokenAuthenticator. Should we add a link to the migration guide in the docs? wdyt?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 836d587 and 99a1b9b.

📒 Files selected for processing (1)
  • airbyte_cdk/sources/declarative/declarative_component_schema.yaml (27 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
  • GitHub Check: Pytest (Fast)
🔇 Additional comments (11)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (11)

441-446: Add descriptive titles for backoff strategies
Titles for both numeric and interpolated backoff time values improve clarity in the Builder UI—great addition! wdyt?


1062-1074: Enhance JWT authenticator field descriptions
Adding titles for secret_key, base64_encode_secret_key, and algorithm aligns with other components and will improve UX. Have we ensured consistency with other authenticator docs? wdyt?


1397-1404: Refine DeclarativeStream properties and ordering
The addition of the name field, the reordered retriever anyOf list, and inclusion of CustomIncrementalSync and DynamicSchemaLoader make the stream schema more intuitive. Can we verify in the Builder UI that these appear in the intended order? wdyt?

Also applies to: 1408-1411, 1417-1429


2238-2238: Allow additional schema properties
Enabling additionalProperties on InlineSchemaLoader.schema is necessary for flexibility—looks good. wdyt?


1926-1939: Enhance HttpRequester authentication and error handling
Great to see the authenticator and new error_handler anyOf lists reordered and expanded, plus clearer titles on headers, parameters, and body types. Can we test a few connector definitions in the Builder UI to confirm these changes render correctly? wdyt?

Also applies to: 1948-1948, 1964-1964, 1982-1987, 2000-2007, 2017-2023


2893-2896: Add titles for pagination increments
Introducing titles to both OffsetIncrement.page_size and PageIncrement.page_size options clarifies intent and matches the Backoff naming. wdyt?

Also applies to: 2927-2930


3023-3031: Clarify composite key options
The titles for single and composite primary key variants guide users in selecting the right schema type—nice touch. wdyt?


3140-3144: Add title to record filter condition
Labeling the condition field ensures users understand that it controls which records are filtered out. Good catch. wdyt?


3198-3200: Add examples for schema normalization
Including examples for the SchemaNormalization enum enhances discoverability of the Default vs None options—nice. wdyt?


3377-3383: Add titles for StateDelegatingStream fields
Titles for full_refresh_stream and incremental_stream improve clarity on their roles depending on state presence. wdyt?


3388-3394: Streamline SimpleRetriever component metadata
Great addition of the retriever title, plus inclusion of decoder and custom requester/partition router options. Could we double‑check the placement of the ignore_stream_slicer_parameters_on_paginated_requests description remains clear in the UI? wdyt?

Also applies to: 3403-3404, 3405-3415, 3436-3444

@lmossman lmossman changed the title Correct field titles/orders in declarative_component_schema design: correct field titles/orders in declarative_component_schema Apr 23, 2025
@bazarnov bazarnov changed the title design: correct field titles/orders in declarative_component_schema fix: (CDK) (design) correct field titles/orders in declarative_component_schema.yaml Apr 23, 2025
@github-actions github-actions bot added bug Something isn't working security labels Apr 23, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)

2230-2297: Potential over‑restriction of request option value types

request_body_data, request_headers, and request_parameters have been narrowed to accept Dict[str, str] (or limited unions). The runtime code in HttpRequester later casts str(v) but happily handles non‑string values today (e.g., ints, bools). Would relaxing the annotation back to Dict[str, Any] preserve flexibility for users who pass numbers or nested structures?

-    request_body_data: Optional[Union[Dict[str, str], str]] = Field(
+    request_body_data: Optional[Union[Dict[str, Any], str]] = Field(

Same for request_headers / request_parameters. Thoughts?


2426-2446: New decoder on SimpleRetriever – check docs sync

The optional decoder property is now exposed in the schema (💯). Could we update the Connector Builder docs/examples to show how to use it so builders discover the feature, wdyt?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 99a1b9b and e7f9848.

📒 Files selected for processing (2)
  • airbyte_cdk/sources/declarative/declarative_component_schema.yaml (27 hunks)
  • airbyte_cdk/sources/declarative/models/declarative_component_schema.py (16 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (8)
airbyte_cdk/sources/declarative/requesters/request_option.py (1)
  • RequestOption (25-117)
airbyte_cdk/sources/declarative/extractors/record_filter.py (1)
  • RecordFilter (17-49)
airbyte_cdk/sources/declarative/extractors/record_selector.py (3)
  • name (57-65)
  • name (68-70)
  • RecordSelector (25-170)
airbyte_cdk/sources/declarative/retrievers/simple_retriever.py (5)
  • name (107-115)
  • name (118-120)
  • SimpleRetriever (55-624)
  • primary_key (291-293)
  • primary_key (296-298)
airbyte_cdk/sources/declarative/declarative_stream.py (5)
  • name (93-97)
  • name (100-102)
  • primary_key (70-71)
  • primary_key (74-76)
  • DeclarativeStream (32-241)
airbyte_cdk/sources/declarative/retrievers/async_retriever.py (1)
  • AsyncRetriever (19-124)
airbyte_cdk/sources/declarative/auth/jwt.py (1)
  • JwtAuthenticator (41-197)
airbyte_cdk/sources/declarative/requesters/http_requester.py (1)
  • HttpRequester (38-433)
⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • GitHub Check: Pytest (All, Python 3.11, Ubuntu)
  • GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (36)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (29)

443-445: Descriptive titles improve ConstantBackoffStrategy clarity
Great addition of explicit titles for the numeric and interpolated backoff intervals—this will surface more informative labels in the Builder UI, wdyt?


1062-1076: Add missing metadata for JwtAuthenticator fields
Including title attributes for secret_key, base64_encode_secret_key, and algorithm aligns this authenticator with the rest of the schema and ensures the UI shows friendly labels, wdyt?


1397-1403: Introduce name property on DeclarativeStream
Adding a top‑level name field makes stream identification more explicit in the UI—excellent UX enhancement, wdyt?


1408-1409: Prioritize SimpleRetriever in retriever union
Reordering to surface the built‑in SimpleRetriever first will guide users toward the common case—nice touch, wdyt?


1417-1418: Expose CustomIncrementalSync in incremental_sync union
Including the custom incremental sync option empowers connectors with bespoke logic—looks good, wdyt?


1428-1429: Add DynamicSchemaLoader to schema_loader union
Reordering to include runtime schema loading aligns the schema with the Python model—great for advanced use cases, wdyt?


1823-1825: Add metadata for SessionTokenAuthenticator
Providing title and description for this authenticator enhances discoverability in the Builder UI—nice, wdyt?


1962-1975: Reorder authenticator options under HttpRequester
Aligning the union order to list core methods (NoAuth, ApiKey, Basic, etc.) before custom/deprecated strategies improves UX consistency, wdyt?


1983-1983: Allow string shorthand for request_body_data
Supporting type: string alongside object payloads gives users concise syntax for simple cases, wdyt?


2000-2001: Allow string shorthand for request_body_json
Enabling type: string for JSON bodies improves flexibility in defining payload templates, wdyt?


2018-2023: Enhance request_headers union with titles and string case
Adding a title for key/value pairs and allowing a raw string form makes header definitions clearer and more succinct, wdyt?


2036-2042: Extend request_parameters union with QueryProperties and string form
Including QueryProperties and shorthand string support diversifies parameter definitions for common patterns, wdyt?


2053-2059: Expand error_handler union with Composite and Custom handlers
Surfacing CompositeErrorHandler and CustomErrorHandler enriches the retry logic options available to users, wdyt?


2274-2275: Enable additional properties in InlineSchemaLoader.schema
Allowing free‐form schema objects supports more complex inline definitions—this matches the Python model change, wdyt?


2956-2958: Add titles to OffsetIncrement page_size options
Descriptive titles for numeric vs. interpolated page size values will make the UI labels self‑explanatory, wdyt?


2990-2992: Add titles to PageIncrement page_size options
Similarly enhancing the titles in the page increment strategy clarifies the choice between static and dynamic sizes, wdyt?


3086-3093: Add titles to PrimaryKey union variants
Labelling single key, composite key, and nested composite key variants improves user guidance when selecting a primary key format, wdyt?


3360-3360: Mark LegacySessionTokenAuthenticator as deprecated
Deprecation flag is critical to steer users toward the modern SessionTokenAuthenticator component, wdyt?


3419-3419: Add title for StateDelegatingStream
Including a title makes this experimental component more discoverable in the UI, wdyt?


3450-3450: Add title for SimpleRetriever
Labeling the synchronous retriever improves consistency with other components, wdyt?


3464-3466: Include CustomRequester in SimpleRetriever.requester union
Supporting custom requesters alongside built‑in HTTP ones offers extensibility for edge cases, wdyt?


3467-3478: Add decoder component to SimpleRetriever
Exposing decoder options directly in the retriever consolidates response processing settings, wdyt?


3479-3481: Refactor record_selector reference to core definition
Switching to a direct $ref for RecordSelector centralizes record extraction logic, wdyt?


3498-3505: Include CustomPartitionRouter in SimpleRetriever.partition_router
Allowing custom partition routers alongside built‑in ones maximizes flexibility for complex pagination scenarios, wdyt?


3598-3608: Allow CustomRecordExtractor in AsyncRetriever extractors
Adding the custom extractor option to status, target, and download phases harmonizes with the rest of the schema’s extensibility model, wdyt?


3613-3651: Support CustomRequester across AsyncRetriever request phases
Enabling custom requesters for creation, polling, download, abort, and delete steps ensures full extensibility of asynchronous workflows, wdyt?


3656-3667: Include CustomPartitionRouter in AsyncRetriever
Extending the partition router union in the async workflow mirrors the synchronous case and provides parity, wdyt?


3669-3692: Allow CustomDecoder in AsyncRetriever decoders
Supporting custom decoders for both response and download phases completes the set of decoder options, wdyt?


2239-2241: Reorder retriever options in DynamicSchemaLoader
Bringing built‑in retrievers (Async, Custom, Simple) into the correct order improves schema consistency, wdyt?

airbyte_cdk/sources/declarative/models/declarative_component_schema.py (7)

418-430: Titles added – looks good

The new title metadata on secret_key, base64_encode_secret_key, and algorithm makes the generated UI clearer without affecting validation logic. Nice incremental UX win – no action required from my side.


1804-1815: Minor refactor only – no behavioural change detected

The union reorder plus extra title fields for page_size_option / page_token_option appear to be strictly cosmetic. Validation order in Pydantic is the same, so LGTM.


1856-1871: Default transform_before_filtering switched to None

Because the runtime code treats truthiness (if self.transform_before_filtering:) a None behaves like the old False. 👍 Still, double‑check any direct boolean comparisons (== False) downstream just in case, wdyt?


2102-2110: Nice addition of optional name field

Adding name directly to DeclarativeStream aligns the schema with the runtime dataclass that already maintains _name. This removes the “why isn’t name in the schema?” confusion – thanks!


2492-2580: AsyncRetriever expansions LGTM

All newly re‑ordered unions and the download/abort/delete requester fields are optional and backward‑compatible. Implementation code already supports these properties, so happy path here.


2656-2656: Forward refs call retained

Keeping HttpRequester.update_forward_refs() prevents lazy‑import issues with circular types – thanks for not dropping it.


884-896:

❓ Verification inconclusive

Confirm backward‑compat on newly required type field?

KeyTransformation now explicitly includes type: Literal["KeyTransformation"]. If existing manifests omitted this field (because the surrounding object already implied the type), they would now fail validation. Do we have evidence that all production YAMLs already provide the type attribute, or do we need to add a fallback/default in the ingest pipeline, wdyt?


Ensure backward‑compatibility for the new required type field?
We’ve added type: Literal["KeyTransformation"] (without a default) in

airbyte_cdk/sources/declarative/models/declarative_component_schema.py
lines 884–896

making type mandatory. This may break existing YAML manifests that omit the type attribute.

  • Should we provide a default here (e.g. type: Literal["KeyTransformation"] = Field("KeyTransformation", const=True)) to preserve backward‑compatibility?
  • Or handle missing type values in the ingest pipeline as a fallback? wdyt?

Copy link
Contributor

@bazarnov bazarnov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left comments about my findings.

@bazarnov bazarnov requested a review from brianjlai April 23, 2025 09:51
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (4)

2197-2205: Could we make _remove_query_properties a pure “filter” without losing non‑str value types?

_remove_query_properties silently drops every non‑str value in request_parameters.
If a future schema allows other immutable types (e.g. int, bool, or an interpolated expression object), those would also be discarded – not just QueryPropertiesModel.
Would switching to an explicit isinstance(..., QueryPropertiesModel) filter instead of an implicit “keep only str” rule make the intent clearer and safer, wdyt?
For example:

- if not isinstance(request_parameter, QueryPropertiesModel)
+ if not isinstance(request_parameter, QueryPropertiesModel)

(the comprehension currently keeps only strings but does not express that intent.)


2830-2833: Mutating the parsed Pydantic model could be fragile – prefer a local default?

transform_before_filtering is set by mutating the model instance.
Because Pydantic models may become immutable (or validate on assignment) in v2, would it be safer to assign to a local variable instead?

transform_before_filtering = (
    model.transform_before_filtering
    if model.transform_before_filtering is not None
    else False
)

You could then pass that variable downstream without touching the model itself, wdyt?


2848-2851: Same concern for schema_normalization defaulting

For consistency with the previous comment, perhaps default schema_normalization
without mutating model:

schema_normalization_enum = (
    model.schema_normalization
    if model.schema_normalization is not None
    else SchemaNormalizationModel.None_
)

and then use schema_normalization_enum below.
This avoids side‑effects on the parsed object.


2953-2965: Early‑exit on multiple QueryProperties could improve readability

Inside the loop we continue scanning even after finding a second
QueryPropertiesModel, only to raise afterwards.
Would breaking as soon as len(query_properties_definitions) > 1
simplify control‑flow and save a little work, wdyt?
Nothing critical, just a small readability tweak.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 68b4118 and 3e2dc96.

📒 Files selected for processing (1)
  • airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
airbyte_cdk/sources/declarative/requesters/request_options/interpolated_request_options_provider.py (1)
  • InterpolatedRequestOptionsProvider (25-142)
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py

[error] 3613-3613: "FileUploader" has no attribute "parameters" (mypy attr-defined error)

⏰ Context from checks skipped due to timeout of 90000ms (9)
  • GitHub Check: Check: 'source-pokeapi' (skip=false)
  • GitHub Check: Check: 'source-amplitude' (skip=false)
  • GitHub Check: Check: 'source-shopify' (skip=false)
  • GitHub Check: Check: 'source-hardcoded-records' (skip=false)
  • 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 (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)

3092-3098: Return type annotation might be too strict

_remove_query_properties is annotated to return Mapping[str, str],
but upstream (InterpolatedRequestOptionsProvider) accepts
Mapping[str, str] | str.
If callers later pass objects such as InterpolatedString
(which are not subclasses of str) they would violate the annotation even
though the runtime would work.

Would loosening the return type to Mapping[str, Any]
(or keeping the str but adding a # type: ignore[arg-type])
align the annotation with real‑world usage, wdyt?

octavia-squidington-iii pushed a commit to airbytehq/airbyte-platform that referenced this pull request Apr 24, 2025
…nd feature flag) (#15845)

## What
https://github.com/user-attachments/assets/8ed2f725-1567-441c-bd96-88a812607f19

This is the first step in integrating SchemaForm into Builder UI.

This PR adds a feature flag `connectorBuilder.schemaForm`, which when set to `true` causes the UI mode of Builder to render a raw SchemaForm for the stream view.

## How
I approached this by adding a third case to the ConnectorBuilderEditPage's [rendering logic](https://github.com/airbytehq/airbyte-platform-internal/pull/15845/files#diff-f605f9d9694d2052bb126db465f2c37d16fa81917fce08c485db4e44581f8f28R142) (alongside yaml mode and current UI), which is executed when the feature flag is true and `mode` is `ui`.

Most of the new logic lies in the new [SchemaFormBuilder](https://github.com/airbytehq/airbyte-platform-internal/pull/15845/files#diff-538753de1b3ea281377366b407b242856f39ecc6ec93e035bbca4166ae85fde7) component, which handles rendering both a new sidebar and the new stream config view.

While this meant I had to repeat a bit of logic from the other UI mode, I felt this was better for feature flagging.

**Note**: this state is not perfect, namely:
- You can't always switch to YAML mode and then back to UI mode, since it is still validating against what is possible in the old UI mode
- There is no way to configure `User Inputs` - I just didn't have time to get this working
- No AI assist buttons, jinja expression suggestions, linking, etc., as I didn't have time
- Doesn't break apart stream fields into separate cards, didn't have time

But since this is behind a feature flag and the old implementation _should_ be unaffected by these changes, I think this is fine to merge in its current state.

## Testing
If you want to test this out yourself, I recommend checking out [this branch](airbytehq/airbyte-python-cdk#480) in the `airbyte-python-cdk` repo, and after you run `pnpm start` on this branch, just copy over the `declarative_component_schema.yaml` file into this repo's webapp `build` folder, e.g (running from the `airbyte-python-cdk` repo):
```
cp airbyte_cdk/sources/declarative/declarative_component_schema.yaml ~/code/airbyte-platform-internal/oss/airbyte-webapp/build
```

This will use the corrected declarative_component_schema that changes some field ordering and titles to get to a slightly better UX.
@bazarnov bazarnov self-assigned this Apr 24, 2025
@bazarnov bazarnov merged commit 1b1d1a8 into main Apr 25, 2025
28 checks passed
@bazarnov bazarnov deleted the lmossman/corrected-declarative-component-schema branch April 25, 2025 12:21
saifsafsf pushed a commit to saifsafsf/airbyte-platform that referenced this pull request May 4, 2025
…nd feature flag) (#15845)

## What
https://github.com/user-attachments/assets/8ed2f725-1567-441c-bd96-88a812607f19

This is the first step in integrating SchemaForm into Builder UI.

This PR adds a feature flag `connectorBuilder.schemaForm`, which when set to `true` causes the UI mode of Builder to render a raw SchemaForm for the stream view.

## How
I approached this by adding a third case to the ConnectorBuilderEditPage's [rendering logic](https://github.com/airbytehq/airbyte-platform-internal/pull/15845/files#diff-f605f9d9694d2052bb126db465f2c37d16fa81917fce08c485db4e44581f8f28R142) (alongside yaml mode and current UI), which is executed when the feature flag is true and `mode` is `ui`.

Most of the new logic lies in the new [SchemaFormBuilder](https://github.com/airbytehq/airbyte-platform-internal/pull/15845/files#diff-538753de1b3ea281377366b407b242856f39ecc6ec93e035bbca4166ae85fde7) component, which handles rendering both a new sidebar and the new stream config view.

While this meant I had to repeat a bit of logic from the other UI mode, I felt this was better for feature flagging.

**Note**: this state is not perfect, namely:
- You can't always switch to YAML mode and then back to UI mode, since it is still validating against what is possible in the old UI mode
- There is no way to configure `User Inputs` - I just didn't have time to get this working
- No AI assist buttons, jinja expression suggestions, linking, etc., as I didn't have time
- Doesn't break apart stream fields into separate cards, didn't have time

But since this is behind a feature flag and the old implementation _should_ be unaffected by these changes, I think this is fine to merge in its current state.

## Testing
If you want to test this out yourself, I recommend checking out [this branch](airbytehq/airbyte-python-cdk#480) in the `airbyte-python-cdk` repo, and after you run `pnpm start` on this branch, just copy over the `declarative_component_schema.yaml` file into this repo's webapp `build` folder, e.g (running from the `airbyte-python-cdk` repo):
```
cp airbyte_cdk/sources/declarative/declarative_component_schema.yaml ~/code/airbyte-platform-internal/oss/airbyte-webapp/build
```

This will use the corrected declarative_component_schema that changes some field ordering and titles to get to a slightly better UX.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants