fix: (CDK) (HttpRequester) - fix trailing slash for url_base when no path has been provided
#412
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a regression in URL joining logic for the HttpRequester by adjusting how the trailing slash for the url_base is handled when no explicit path is provided. The key changes are:
- Removing the use of os.path.join in get_url_base to preserve the evaluated string.
- Updating _join_url to return the url_base as-is when the path is empty, with documentation examples revised accordingly.
- Adjusting unit tests to reflect the new behavior for trailing slashes.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| airbyte_cdk/sources/declarative/requesters/http_requester.py | Removes os.path.join usage in get_url_base and updates _join_url behavior for trailing slash handling |
| unit_tests/sources/declarative/requesters/test_http_requester.py | Updates expected URL values in assertions to match the revised logic |
Comments suppressed due to low confidence (2)
airbyte_cdk/sources/declarative/requesters/http_requester.py:134
- Removing os.path.join changes the cross-platform URL formatting behavior; please confirm that self._url_base.eval consistently returns the URL without an undesired trailing slash across different environments.
return os.path.join(self._url_base.eval(self.config, **interpolation_context), EmptyString)
airbyte_cdk/sources/declarative/requesters/http_requester.py:378
- The updated _join_url method now returns url_base as-is for an empty path, which may result in preserving an unexpected trailing slash; ensure that this updated behavior is fully aligned with all documented examples and use cases.
if path == EmptyString or path is None:
📝 WalkthroughWalkthroughThe changes update the URL handling logic in the HTTP requester. In the source file, the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant HttpRequester
Caller->>HttpRequester: get_url_base(...)
Note right of HttpRequester: Convert evaluated URL base to string
HttpRequester-->>Caller: Return URL base
Caller->>HttpRequester: _join_url(url_base, path)
alt path is empty
Note right of HttpRequester: Retain trailing slash in base URL
HttpRequester-->>Caller: Return base URL
else base missing trailing slash
HttpRequester->>HttpRequester: Append slash to base URL
HttpRequester-->>Caller: Return joined URL
end
Suggested reviewers
Would you like to review the sequence diagram and labels to ensure they match your expectations, wdyt? Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
🪧 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 (2)
airbyte_cdk/sources/declarative/requesters/http_requester.py (2)
368-369: Docstring doesn't match the implementation.The docstring incorrectly states that trailing slashes are removed when the path is empty, but the implementation actually preserves them. Would you consider updating the docstring to accurately reflect the new behavior?
- - If the path is an empty string or None, the method returns the base URL with any trailing slash removed. + - If the path is an empty string or None, the method returns the base URL as is, preserving any trailing slash.
377-385: Fixed trailing slash handling for URL_base when no path is provided.The implementation now:
- Preserves the trailing slash when path is empty (fixing the regression)
- Only applies
os.path.join()when adding a trailing slash to url_base that doesn't have oneThis implementation aligns with the PR objectives to fix the trailing slash issue.
Consider using a more direct approach for adding a trailing slash in line 384, which would be clearer and avoid potential cross-platform inconsistencies with
os.path.join():- if not url_base.endswith("/"): - url_base = os.path.join(url_base, EmptyString) + if not url_base.endswith("/"): + url_base = url_base + "/"What do you think?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/requesters/http_requester.py(2 hunks)unit_tests/sources/declarative/requesters/test_http_requester.py(2 hunks)
⏰ 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: SDM Docker Image Build
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (3)
unit_tests/sources/declarative/requesters/test_http_requester.py (2)
124-124: Test assertion updated to match implementation change.The assertion now expects a URL without a trailing slash, aligning with the updated behavior in the
get_url_base()method.
148-148: Test expectations updated for consistency.These test case updates ensure the expected behavior matches the implementation changes for URL base handling without trailing slashes.
Also applies to: 150-150
airbyte_cdk/sources/declarative/requesters/http_requester.py (1)
135-135: Simplified URL base handling.The method now directly returns the string representation of the evaluated URL base without using
os.path.join(), addressing the issue mentioned in the PR objectives.
…trailing-slash-when-no-path
What
Resolving regression after:
HttpRequester.pathoptional #370Resolving OC:
How
os.path.join()in the_join_url()method, use it when thepathis provided and there is no/at the end of theurl_baseto join the URL parts correctlyUser Impact
No impact is expected. This is not a breaking change.
Summary by CodeRabbit