fix: (CDK) (HttpRequester) - Make the HttpRequester.path optional
#370
Conversation
📝 WalkthroughWalkthroughThis PR updates the HTTP request construction logic. In the declarative component schema and its corresponding model, the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant HR as HttpRequester
participant R as Requester
C->>HR: Call get_url_base(stream_state, stream_slice, next_page_token)
HR->>HR: Build interpolation context (_get_interpolation_context)
HR->>HR: Format 'url_base' with context
HR->>HR: Evaluate optional 'path'
HR->>HR: Call _join_url to merge base and path
HR-->>C: Return constructed URL
C->>R: Call get_url_base with new parameters
R-->>C: Return URL
Possibly related PRs
Suggested labels
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/requesters/http_requester.py (1)
118-134: Good refactoring with _get_interpolation_context method.Extracting the interpolation context creation logic into a separate method reduces duplication and centralizes this functionality. I like how you're handling the extra_fields from stream_slice too.
One minor suggestion: would it make sense to also include stream_state in the returned context since it's passed as a parameter? wdyt?
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (2)
1769-1775: Enhance URL base interpolation contexts.
I see you've added additional interpolation keys (next_page_token,stream_interval,stream_partition,stream_slice,creation_response,polling_response, anddownload_target) to theurl_baseproperty. This should provide extra flexibility for dynamic URL construction. Have you verified that all downstream components correctly support these additional contexts? wdyt?
1781-1798: Make thepathargument optional.
I noticed that thepathproperty now mirrors many aspects of theurl_base(including multiple interpolation contexts) and is no longer listed as a required property. This change aligns with the PR objective to make thepathargument optional. Would you consider adding a brief note in the description explicitly stating that this field is now optional to help guide implementers? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
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/requesters/http_requester.py(6 hunks)airbyte_cdk/sources/declarative/requesters/requester.py(1 hunks)airbyte_cdk/sources/types.py(1 hunks)unit_tests/sources/declarative/requesters/test_http_requester.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- airbyte_cdk/sources/types.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)
🔇 Additional comments (11)
unit_tests/sources/declarative/requesters/test_http_requester.py (1)
828-828: This change aligns with the URL construction updates.The test now verifies that trailing slashes from paths are correctly removed in the final URL, which makes the URL construction more consistent with common practices.
airbyte_cdk/sources/declarative/requesters/requester.py (1)
38-44: Good enhancement of theget_url_basemethod signature.The method now accepts additional context parameters (stream_state, stream_slice, next_page_token), making it consistent with other methods like
get_path. This allows for dynamic URL construction based on runtime context, enabling more flexible API integration patterns. This is the core change that addresses the PR objective.airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
2051-2053: Enhanced examples forurl_basewith interpolation contexts.These new examples effectively demonstrate how to use the new capabilities for dynamic URL construction, including stream partition IDs and next page tokens. Great additions that will help users understand how to use the feature.
2057-2058: Makingpathoptional is the key change in this PR.By changing
pathfrom required to optional with a default value ofNone, you've successfully implemented the core requirement of the PR. This provides more flexibility for URL construction while maintaining backward compatibility.airbyte_cdk/sources/declarative/requesters/http_requester.py (6)
28-28: Nice addition of EmptyString to import statement.I see you've added
EmptyStringto the imports. This will be used to handle the new optional path parameter, making the code more robust when no path is provided.
55-55: Good design choice making path optional.Making the
pathparameter optional aligns well with the PR objective of allowing more flexibility in URL construction, especially when only the base URL might be sufficient.
70-72: Nice handling of optional path value.Using
EmptyStringas a fallback whenpathis None is a clean solution. This ensures that the interpolation logic works consistently whether a path is provided or not.
135-148: Great enhancement to get_url_base.The method now accepts the same parameters as get_path, providing consistency in the interface. Using the new interpolation context method ensures that dynamic values from stream_slice and next_page_token can now be used in the URL base construction.
The use of
os.path.joinwithEmptyStringis clever to ensure we get a properly formatted URL.
149-162: Nicely updated get_path method.Making the parameters optional with default values of None maintains backward compatibility while adding new functionality. The method now leverages the shared interpolation context logic, making the code more maintainable.
359-372: Excellent docstring addition to _join_url.The detailed docstring clearly explains the purpose and behavior of this method, particularly regarding trailing slash handling. This kind of documentation is very helpful for both users and maintainers of the code.
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1778-1780: Update URL base examples for dynamic interpolation.
The new examples now clearly illustrate the dynamic generation of the URL base (using expressions like{{ stream_partition['id'] }}and{{ next_page_token['id'] }}). Do these updated examples fully capture your use cases for various API configurations? wdyt?
bazarnov
changed the title
path arg optionalHttpRequester.path optional
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
airbyte_cdk/sources/declarative/requesters/paginators/paginator.py (1)
52-57: Updated abstract method signature to support additional context.The
pathmethod signature now includes optionalstream_stateandstream_sliceparameters, which enhances the flexibility of URL path generation. This is a good change that supports the PR objective of making the HTTP request construction more flexible.Just curious, should we update the method docstring as well to reflect these new parameters? wdyt?
airbyte_cdk/sources/declarative/retrievers/simple_retriever.py (1)
243-247: Update the docstring to reflect new parameters?The docstring hasn't been updated to include descriptions for the new
stream_stateandstream_sliceparameters. Would you consider enhancing it for better developer experience? wdyt?""" If the paginator points to a path, follow it, else return nothing so the requester is used. :param next_page_token: + :param stream_state: The current state of the stream + :param stream_slice: The slice of the stream to process :return: """airbyte_cdk/sources/declarative/declarative_component_schema.yaml (2)
1807-1814: Enhanced Interpolation Context inurl_base
You've expanded theinterpolation_contextfor theurl_baseproperty to include new variables such asnext_page_token,stream_interval,stream_partition,stream_slice,creation_response,polling_response, anddownload_target. This update broadens the flexibility of URL construction. Could you please confirm that all components using this context always supply appropriate values for these variables? wdyt?
1824-1832: UpdatedpathProperty Interpolation and Optionality
Thepathproperty now includes an expandedinterpolation_contextsimilar tourl_base. Since the PR objective was to make thepathoptional, please ensure that downstream processing correctly handles cases wherepathis null or missing. Would you consider adding or updating tests to validate that behavior? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(3 hunks)airbyte_cdk/sources/declarative/requesters/http_requester.py(6 hunks)airbyte_cdk/sources/declarative/requesters/paginators/default_paginator.py(3 hunks)airbyte_cdk/sources/declarative/requesters/paginators/no_pagination.py(1 hunks)airbyte_cdk/sources/declarative/requesters/paginators/paginator.py(2 hunks)airbyte_cdk/sources/declarative/retrievers/simple_retriever.py(3 hunks)airbyte_cdk/utils/mapping_helpers.py(2 hunks)unit_tests/sources/declarative/requesters/paginators/test_default_paginator.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- airbyte_cdk/sources/declarative/requesters/http_requester.py
- airbyte_cdk/sources/declarative/models/declarative_component_schema.py
⏰ 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 (14)
airbyte_cdk/sources/declarative/requesters/paginators/no_pagination.py (1)
22-27: LGTM! Method signature updated to match abstract method.The signature has been correctly updated to match the new abstract method signature in the
Paginatorclass. This change allows for additional context viastream_stateandstream_sliceto be provided, which aligns with the PR objective of enhancing URL path handling.unit_tests/sources/declarative/requesters/paginators/test_default_paginator.py (3)
478-490: Good test coverage for edge case when token is None.This test ensures the
pathmethod returnsNonewhen no token is provided, which is an important edge case to verify.
492-511: LGTM! Testing RequestOption vs RequestPath behavior.This test validates the behavior when using a different option type (RequestOption instead of RequestPath), ensuring correct handling of different page token option types.
513-549: Comprehensive test for interpolation context with stream_slice.This test thoroughly validates that the
pathmethod correctly uses the additional interpolation context fromstream_sliceand properly handles token values. The test covers a complexStreamSliceobject with partition, cursor_slice, and extra_fields, which is a good representation of real-world scenarios.airbyte_cdk/sources/declarative/requesters/paginators/paginator.py (1)
14-14: LGTM! Added StreamSlice import.The StreamSlice import is correctly added to support the updated method signature.
airbyte_cdk/utils/mapping_helpers.py (2)
13-13: Updated imports to include StreamSlice and StreamState.The new imports support the type hinting for the new
get_interpolation_contextfunction.
148-162: Well-implemented utility function for creating interpolation context.This new function consolidates logic for creating an interpolation context with stream state, stream slice, and next page token. The implementation correctly handles extra fields from the stream slice when available.
I like how this centralizes the context creation logic that can be reused across different components. It's a clean implementation that handles the edge case when
stream_sliceisNoneor doesn't haveextra_fields.airbyte_cdk/sources/declarative/requesters/paginators/default_paginator.py (2)
154-172: Good enhancement to the path method!This change adds support for additional context (stream_state, stream_slice) when constructing the path for pagination. The addition of the interpolation context using these parameters will make URL construction more flexible.
One thought: Have you considered updating the docstring for this method to reflect the new parameters? This would help future developers understand the expanded functionality. wdyt?
273-284: The decorator update looks correct!The PaginatorTestReadDecorator's path method has been properly updated to match the new signature and pass all parameters to the decorated paginator.
airbyte_cdk/sources/declarative/retrievers/simple_retriever.py (4)
237-242: Good enhancement of the paginator path methodThe expansion of the method signature to include
stream_stateandstream_slicealigns perfectly with the PR objectives. This allows for more dynamic URL construction using additional contextual information.
248-252: Clean implementation passing context to paginatorThe implementation correctly passes all parameters to the paginator's path method, which ensures consistent behavior with the enhanced functionality.
311-315: Consistent context propagationThis change properly propagates the stream state and slice contexts to the paginator, ensuring that the URL construction has access to all relevant data.
586-590: Test decorator also properly updatedGood job ensuring that the test decorator class also passes the same parameters, maintaining consistency between the main implementation and the test helper.
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1817-1819: Updatedurl_baseExamples
The examples now show dynamic usage of the new interpolation contexts (e.g. conditional expressions, usingstream_partition['id']andnext_page_token['id']). Are these examples reflective of actual usage scenarios in production? Perhaps a quick run with sample configurations would be helpful. wdyt?
* 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
Resolving:
How
For the
airbyte_cdk/sources/declarative/declarative_component_schema.yaml:For the
airbyte_cdk/sources/declarative/models/declarative_component_schema.py:For the
airbyte_cdk/sources/declarative/requesters/http_requester.py:For the
airbyte_cdk/sources/declarative/requesters/requester.py:For the
airbyte_cdk/sources/types.py:For the
unit_tests/sources/declarative/requesters/test_http_requester.py:User Impact
This update introduces the
Requesterinterface change, which adds the ability forget_url_base()to utilize the interpolation context passed, such asstream_state,stream_slice/stream_interval, andnext_page_token.Sources that reuse the interface may need to update the interface for the
get_url_base()method if it has been overridden. Otherwise, we anticipate no breaking changes from this CDK update.Summary by CodeRabbit
Summary by CodeRabbit
DefaultPaginatorclass and ensure correct path generation with various conditions.