chore: fix more fields in declarative_component_schema #525
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request refines the declarative component schema for improved Builder UI consistency by updating field titles, descriptions, and the ordering of anyOf lists.
- Reorders and adjusts authentication and requester definitions (e.g. shifting NoAuth and updating references to Authenticator).
- Updates API endpoint, request, and response field metadata to better align with UI expectations.
- Removes duplicate definitions and adjusts decoder and retriever references.
Comments suppressed due to low confidence (2)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml:3545
- The removal of JsonDecoder from the decoders list may result in loss of JSON response decoding support. Please confirm that this removal is intentional.
- - "$ref": "#/definitions/JsonDecoder"
airbyte_cdk/sources/declarative/declarative_component_schema.yaml:3202
- The removal of SimpleRetriever from the retriever anyOf list might unintentionally limit retriever functionality. Verify if excluding SimpleRetriever is intended.
+ - "$ref": "#/definitions/CustomRetriever"
lmossman
changed the title
|
@bazarnov do you mind looking this over to check if it can be merged without any other changes? It should just be some more reordering and updating titles/descriptions |
📝 WalkthroughWalkthroughThis change updates the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DeclarativeSchema
participant HttpRequester
participant Authenticator
User->>DeclarativeSchema: Define stream with HttpRequester config
DeclarativeSchema->>HttpRequester: Pass config including request_headers/parameters
HttpRequester->>Authenticator: Apply selected authenticator (NoAuth last, deprecated excluded)
HttpRequester-->>DeclarativeSchema: Return configured request behavior
Possibly related PRs
Suggested labels
Suggested reviewers
Would you like me to group similar schema reorderings together more explicitly in the changelog for clarity, or keep each adjustment separate as currently done? Wdyt? 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
🔇 Additional comments (8)
✨ 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
♻️ Duplicate comments (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
2005-2026:⚠️ Potential issueAdd
request_parametersfor dynamic query parameters
Introducingrequest_parametersis a powerful way to customize outgoing queries. However, in the examples, the keysort_by[asc]: updated_atincludes brackets and should be quoted to avoid YAML parsing errors. wdyt?- - sort_by[asc]: updated_at + - "sort_by[asc]": updated_at
🧹 Nitpick comments (3)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (3)
1918-1925: Clarify deprecation guidance forurl_base
The updated description flagsurl_baseas deprecated and shifts emphasis tourland the Authenticator component for sensitive data. Consider if we want to standardize phrasing across similar deprecated fields (e.g., include a link to migration docs)? wdyt?
1937-1943: Introduceurlfor HTTPRequester
Great addition of theurlfield with clear instructions to keep credentials out of it. This replacesurl_base/pathnicely. wdyt on addinglinkable: truehere for consistency with other fields?
2027-2044: Introducerequest_headersfor non-auth headers
The newrequest_headersproperty complementsrequest_parametersby letting users define extra HTTP headers. This addition makes header customization explicit—should we add an example showing both object and string interpolation? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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)
🔇 Additional comments (8)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (8)
300-305: Reorder NoAuth to end of SelectiveAuthenticator options
TheNoAuthoption has been moved afterSessionTokenAuthenticatorand beforeLegacySessionTokenAuthenticatorto align the authenticator list with the Builder UI's expected order—placing “no authentication” at the end. wdyt?
1468-1482: Add support for custom components in FileUploader
You’ve includedCustomRequesterunderrequesterandCustomRecordExtractorunder bothdownload_target_extractorandfile_extractor, enabling users to inject custom logic. This looks good—should we consider adding examples to demonstrate usage of a custom implementation? wdyt?
1957-1964: Update deprecation notice forpath
Thepathproperty is now deprecated in favor ofurl, matchingurl_basechanges. It maintains consistent messaging and ensures users migrate. wdyt?
1986-2001: Alignauthenticatoroptions order in HTTPRequester
TheNoAuthoption has been repositioned afterCustomAuthenticatorto mirror the SelectiveAuthenticator change and UI ordering. Looks aligned—shall we confirm this order is consistent across the codebase? wdyt?
3099-3105: Fixlazy_read_pointeritem type
Changing thelazy_read_pointeritems from an array to a string simplifies configuration and matches usage patterns. This looks correct. wdyt?
3157-3170: EnhancePrimaryKeyvariant descriptions
Adding descriptive text for single key, composite key, and nested composite key variants greatly improves clarity. The new titles and descriptions align with UX guidelines. wdyt?
3542-3550: Refine SimpleRetrieverdecodertitle and ordering
Renaming the property to “HTTP Response Format” and reordering the decoders (XmlDecoderbeforeCsvDecoder,GzipDecoderafterJsonlDecoder) enhances discoverability in the UI. Should we verify that the default selection in the Builder still behaves as expected? wdyt?
4001-4005: Review retriever ordering in HttpComponentsResolver
The current order is[AsyncRetriever, CustomRetriever, SimpleRetriever], but the PR summary mentioned movingCustomRetrieverafterSimpleRetriever. Could you confirm the intended sequence for the UI? wdyt?Likely an incorrect or invalid review comment.
|
@bazarnov do you think I need to be worried about those |
|
Merge it! Don't get bothered with it. |
Fixes more titles, descriptions, and ordering of anyOfs in declarative_component_schema to get it closer to what we want for the Builder UI.
Summary by CodeRabbit
New Features
Improvements
Other Changes