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
| **( | ||
| stream_slice.extra_fields | ||
| if stream_slice is not None and hasattr(stream_slice, "extra_fields") | ||
| else {} | ||
| ), | ||
| } | ||
| path = str(self._path.eval(self.config, **kwargs)) | ||
|
|
||
| def get_url_base( |
There was a problem hiding this comment.
I'm not sure how this can work. It seems like we query get_url_base here in order to create the paginator. This is needed to remove the base_url from the next_page_token here. This means that we either need to:
- not rely on params we don't have at the start of the main.py
- find another way to do that in the paginator
I would prefer to do the first option as I don't think we have a case to interpolate on anything than the config, right?
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