feat(low-code cdk): add interpolation for limit field in Rate #353
Conversation
|
/autofix
|
📝 WalkthroughWalkthroughThis pull request updates the definition and handling of the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Component Caller
participant MTFactory as ModelToComponentFactory
participant Interp as InterpolatedString Evaluator
Caller->>MTFactory: create_rate(model, config)
MTFactory->>MTFactory: Extract model.limit (int or str)
MTFactory->>Interp: Create InterpolatedString from model.limit
Interp->>MTFactory: Evaluate interpolated string using config
MTFactory->>MTFactory: Convert result to integer
MTFactory->>Caller: Return Rate object with evaluated limit
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (3)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
649-653: Consider enhancing the field description to document string interpolation support?The implementation looks good! Since we're now supporting string interpolation for dynamic configuration, would you like to update the field description to include examples of both integer and interpolated string values? Something like:
limit: Union[int, str] = Field( ..., - description="The maximum number of calls allowed within the interval.", + description="The maximum number of calls allowed within the interval. Supports both fixed integers and interpolated strings for dynamic configuration.", + examples=[100, "{{ config['rate_limit'] }}"], title="Limit", )wdyt?
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
3027-3029: Consider adding type validation for the limit field.The changes look good for supporting interpolation of the
limitfield. However, what do you think about adding validation to ensure the interpolated value can be safely converted to an integer? This could help catch configuration errors early. wdyt?def create_rate(self, model: RateModel, config: Config, **kwargs: Any) -> Rate: interpolated_limit = InterpolatedString.create(str(model.limit), parameters={}) + try: + limit = int(interpolated_limit.eval(config=config)) + except ValueError as e: + raise ValueError(f"Rate limit must be a valid integer. Got: {interpolated_limit.eval(config=config)}") from e - return Rate( - limit=int(interpolated_limit.eval(config=config)), + return Rate( + limit=limit, interval=parse_duration(model.interval), )airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1490-1498: Enhanced Flexibility for the "limit" Field
The updated "limit" property now accepts either an integer or a string and has an associatedinterpolation_contextwithconfig. This aligns perfectly with the PR objective of enabling dynamic interpolation for configuring the call rate limit. Would you consider adding an example for the string interpolation use case to guide users on how to leverage this new capability? 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/models/declarative_component_schema.py(1 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(1 hunks)unit_tests/sources/declarative/requesters/test_http_requester.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- unit_tests/sources/declarative/requesters/test_http_requester.py
⏰ Context from checks skipped due to timeout of 90000ms (8)
- 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: Analyze (python)
* main: fix: update cryptography package to latest version to address CVE (airbytehq#377) fix: (CDK) (HttpRequester) - Make the `HttpRequester.path` optional (airbytehq#370) feat: improved custom components handling (airbytehq#350) feat: add microseconds timestamp format (airbytehq#373) fix: Replace Unidecode with anyascii for permissive license (airbytehq#367) feat: add IncrementingCountCursor (airbytehq#346) feat: (low-code cdk) datetime format with milliseconds (airbytehq#369) fix: (CDK) (AsyncRetriever) - Improve UX on variable naming and interpolation (airbytehq#368) fix: (CDK) (AsyncRetriever) - Add the `request` and `response` to each `async` operations (airbytehq#356) fix: (CDK) (ConnectorBuilder) - Add `auxiliary requests` to slice; support `TestRead` for AsyncRetriever (part 1/2) (airbytehq#355) feat(concurrent perpartition cursor): Add parent state updates (airbytehq#343) fix: update csv parser for builder compatibility (airbytehq#364) feat(low-code cdk): add interpolation for limit field in Rate (airbytehq#353) feat(low-code cdk): add AbstractStreamFacade processing as concurrent streams in declarative source (airbytehq#347) fix: (CDK) (CsvParser) - Fix the `\\` escaping when passing the `delimiter` from Builder's UI (airbytehq#358) feat: expose `str_to_datetime` jinja macro (airbytehq#351) fix: update CDK migration for 6.34.0 (airbytehq#348) feat: Removes `stream_state` interpolation from CDK (airbytehq#320) fix(declarative): Pass `extra_fields` in `global_substream_cursor` (airbytehq#195) feat(concurrent perpartition cursor): Refactor ConcurrentPerPartitionCursor (airbytehq#331) feat(HttpMocker): adding support for PUT requests and bytes responses (airbytehq#342) chore: use certified source for manifest-only test (airbytehq#338) feat: check for request_option mapping conflicts in individual components (airbytehq#328) feat(file-based): sync file acl permissions and identities (airbytehq#260) fix: (CDK) (Connector Builder) - refactor the `MessageGrouper` > `TestRead` (airbytehq#332) fix(low code): Fix missing cursor for ClientSideIncrementalRecordFilterDecorator (airbytehq#334) feat(low-code): Add API Budget (airbytehq#314) chore(decoder): clean decoders and make csvdecoder available (airbytehq#326)
What
Some connectors allow configuring the call rate limit via a config file. However, this is currently only possible through the manifest.
Fixed: https://github.com/airbytehq/airbyte-internal-issues/issues/11756
How
Enable interpolation for the limit field of the Rate component.
Summary by CodeRabbit
New Features
Tests