Merge url params with provided params since httpx doesn't do it anymore #478
Conversation
WalkthroughThe changes introduce a new function named Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpWrapper
participant MergeFunc as merge_url_with_params
participant HTTP as HTTP_Request
Client->>HttpWrapper: Call async_request(resource, params)
HttpWrapper->>MergeFunc: merge_url_with_params(resource, params)
MergeFunc-->>HttpWrapper: merged_resource
HttpWrapper->>HTTP: Execute HTTP request(merged_resource)
HTTP-->>HttpWrapper: Return response
HttpWrapper-->>Client: Return response
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (4)
✨ Finishing Touches
🪧 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)
tests/test_http.py (1)
31-35: Consider adding more edge cases to the test suite.The test case effectively verifies handling of None parameters. Consider adding tests for:
- URLs with special characters in parameters
- URLs with duplicate parameters
- URLs with array parameters
- URLs with empty parameter values
custom_components/multiscrape/http.py (1)
218-239: Consider handling additional edge cases in merge_url_with_params.The implementation is clean and handles basic cases well. However, consider handling these edge cases:
- URLs without query parameters (currently works but could be optimized)
- Special characters in parameters (might need URL encoding)
- Array parameters (currently only keeps the first value)
Here's a suggested improvement:
def merge_url_with_params(url, new_params): """Merge URL with provided parameters.""" # Parse the URL if new_params is None or new_params == {}: return url parsed = urlparse(url) # Get existing query parameters as dict existing_params = parse_qs(parsed.query) - # Convert existing params from lists to single values - existing_params = {k: v[0] for k, v in existing_params.items()} + # Handle array parameters by keeping all values + existing_params = { + k: v[0] if len(v) == 1 else v + for k, v in existing_params.items() + } # Merge with new parameters (new_params take precedence) merged_params = {**existing_params, **new_params} # Reconstruct the URL - new_query = urlencode(merged_params) + new_query = urlencode(merged_params, doseq=True) new_url = f"{parsed.scheme}://{parsed.netloc}{parsed.path}?{new_query}" return new_url
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/multiscrape/http.py(5 hunks)tests/test_http.py(1 hunks)
🔇 Additional comments (6)
tests/test_http.py (4)
7-11: LGTM! Test case verifies basic URL parameter merging.The test case effectively verifies the basic functionality of merging a URL with string parameters.
13-17: LGTM! Test case verifies mixed-type parameter handling.The test case effectively verifies that non-string parameters are correctly converted to strings during URL merging.
19-23: LGTM! Test case verifies merging with existing parameters.The test case effectively verifies that existing URL parameters are preserved when merging with new parameters.
25-29: LGTM! Test case verifies handling of empty parameters.The test case effectively verifies that the URL remains unchanged when merging with empty parameters.
custom_components/multiscrape/http.py (2)
5-5: LGTM! Using standard library functions for URL manipulation.The added imports from urllib.parse are appropriate for URL manipulation tasks.
92-93: LGTM! Clean implementation of URL parameter merging.The changes effectively address the issue with httpx no longer merging parameters. The comment provides good context by referencing the relevant httpx issue.
Also applies to: 100-100, 115-115
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_http.py (1)
67-73: LGTM! Special characters are properly tested.Test verifies correct URL encoding of:
- Spaces (converted to +)
- Special characters (properly percent-encoded)
Consider adding tests for additional URL components:
def test_merge_url_with_params_url_components(): """Test merge_url_with_params with various URL components.""" # Test URL with port url = "https://example.com:8080" params = {"param1": "value1"} result = merge_url_with_params(url, params) assert result == "https://example.com:8080?param1=value1" # Test URL with fragment url = "https://example.com#section1" params = {"param1": "value1"} result = merge_url_with_params(url, params) assert result == "https://example.com?param1=value1#section1"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/multiscrape/http.py(5 hunks)tests/test_http.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
custom_components/multiscrape/http.py (2)
92-93: LGTM! Good documentation of the change.The comment clearly explains why this change is needed, with a reference to the relevant httpx issue.
218-242: LGTM! Well-structured URL parameter merging implementation.The function correctly:
- Handles edge cases (None/empty params)
- Preserves array parameters
- Uses standard library functions for URL manipulation
- Maintains proper URL structure
tests/test_http.py (4)
5-30: LGTM! Comprehensive test coverage for basic scenarios.The test cases effectively cover:
- Basic parameter merging
- Mixed parameter types (string and numeric)
- Edge cases (empty params, None)
33-43: LGTM! Good test coverage for existing parameters.Tests verify that parameters are correctly appended to URLs that already have query parameters.
46-51: LGTM! Parameter override behavior is properly tested.Test verifies that new parameters correctly override existing ones with the same name.
54-64: LGTM! Array parameter handling is well tested.Tests verify that:
- Array values are correctly encoded
- Multiple values for the same parameter are properly handled
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
custom_components/multiscrape/http.py (1)
143-172: Update error messages to include merged_resource.The error logging still uses the original resource instead of merged_resource.
_LOGGER.debug( "%s # Timeout error while executing %s request to url: %s.\n Error message:\n %s", self._config_name, method, - resource, + merged_resource, repr(ex), ) await self._handle_request_exception(context, response) raise except httpx.RequestError as ex: _LOGGER.debug( "%s # Request error while executing %s request to url: %s.\n Error message:\n %s", self._config_name, method, - resource, + merged_resource, repr(ex), ) await self._handle_request_exception(context, response) raise except Exception as ex: _LOGGER.debug( "%s # Error executing %s request to url: %s.\n Error message:\n %s", self._config_name, method, - resource, + merged_resource, repr(ex), ) await self._handle_request_exception(context, response) raise
🧹 Nitpick comments (1)
tests/test_http.py (1)
76-88: Consider adding more URL component test cases.While the current tests cover URLs with ports and fragments, consider adding tests for:
- Empty URLs
- Invalid URLs
- URLs with username/password (e.g.,
https://user:pass@example.com)- URLs with path components (e.g.,
https://example.com/path/to/resource)def test_merge_url_with_params_additional_components(): """Test merge_url_with_params with additional URL components.""" # Test empty URL url = "" params = {"param1": "value1"} result = merge_url_with_params(url, params) assert result == "?param1=value1" # Test URL with auth url = "https://user:pass@example.com" params = {"param1": "value1"} result = merge_url_with_params(url, params) assert result == "https://user:pass@example.com?param1=value1" # Test URL with path url = "https://example.com/path/to/resource" params = {"param1": "value1"} result = merge_url_with_params(url, params) assert result == "https://example.com/path/to/resource?param1=value1"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
custom_components/multiscrape/http.py(5 hunks)tests/test_http.py(1 hunks)
🔇 Additional comments (7)
tests/test_http.py (5)
5-30: LGTM! Comprehensive test coverage for basic URL merging scenarios.The test cases cover string parameters, mixed-type parameters, existing URL parameters, empty parameters, and None parameters.
33-43: LGTM! Good test coverage for merging with existing URL parameters.The test cases verify that new parameters are correctly appended to existing ones.
46-51: LGTM! Parameter override behavior is correctly tested.The test verifies that new parameter values take precedence over existing ones.
54-64: LGTM! Array parameter handling is well tested.The test cases verify both single and multiple array values, ensuring correct URL encoding.
67-73: LGTM! Special character handling is properly tested.The test verifies URL encoding of spaces and special characters.
custom_components/multiscrape/http.py (2)
5-5: LGTM! Required imports are added.The necessary urllib.parse functions are imported for URL manipulation.
92-93: LGTM! Clear documentation of the change.The comment explains why the change is needed and links to the relevant httpx issue.
| def merge_url_with_params(url, params): | ||
| """Merge URL with parameters.""" | ||
| if not params: | ||
| return url | ||
|
|
||
| url_parts = list(urlparse(url)) | ||
| query = parse_qs(url_parts[4]) | ||
| query.update(params) | ||
| url_parts[4] = urlencode(query, doseq=True) | ||
| return urlunparse(url_parts) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and input validation.
The merge_url_with_params function should handle invalid URLs and validate input parameters.
def merge_url_with_params(url, params):
"""Merge URL with parameters."""
+ if not url:
+ url = ""
+
if not params:
return url
+ # Validate params
+ if not isinstance(params, dict):
+ raise ValueError("params must be a dictionary")
+
+ # Convert all parameter values to strings
+ params = {k: str(v) if not isinstance(v, (list, tuple)) else [str(i) for i in v]
+ for k, v in params.items()}
+
url_parts = list(urlparse(url))
query = parse_qs(url_parts[4])
query.update(params)
url_parts[4] = urlencode(query, doseq=True)
- return urlunparse(url_parts)
+ try:
+ return urlunparse(url_parts)
+ except Exception as e:
+ raise ValueError(f"Failed to construct URL: {e}")Raise exception when merging fails
Fix #475
Summary by CodeRabbit