diff --git a/src/httpx2/httpx2/_urls.py b/src/httpx2/httpx2/_urls.py index eb3c66e5..5312ee47 100644 --- a/src/httpx2/httpx2/_urls.py +++ b/src/httpx2/httpx2/_urls.py @@ -105,20 +105,32 @@ def __init__(self, url: URL | str = "", **kwargs: typing.Any) -> None: kwargs[key] = value.decode("ascii") if "params" in kwargs: - # Replace any "params" keyword with the raw "query" instead. + # Merge any "params" keyword with the URL's existing "query" + # string, so that calling `URL("https://x?a=1", params={"b": 2})` + # keeps the original query string and appends new params. # - # Ensure that empty params use `kwargs["query"] = None` rather - # than `kwargs["query"] = ""`, so that generated URLs do not - # include an empty trailing "?". + # `params` takes precedence on key collisions (matches + # `requests` and `URL.copy_merge_params` semantics). params = kwargs.pop("params") - kwargs["query"] = None if not params else str(QueryParams(params)) + if params: + # Defer the merge until after the URL has been parsed, so + # we can read the existing query string off the parsed URL. + kwargs["__merge_params__"] = params + merge_params = kwargs.pop("__merge_params__", None) if isinstance(url, str): self._uri_reference = urlparse(url, **kwargs) elif isinstance(url, URL): self._uri_reference = url._uri_reference.copy_with(**kwargs) else: raise TypeError(f"Invalid type for url. Expected str or httpx2.URL, got {type(url)}: {url!r}") + if merge_params: + # Now that the URL is parsed, read its current query and merge the + # new params on top. Re-parse so the new query becomes canonical. + existing_query = self._uri_reference.query + existing = QueryParams(existing_query) if existing_query else QueryParams() + merged = existing.merge(merge_params) + self._uri_reference = self._uri_reference.copy_with(query=str(merged)) @property def scheme(self) -> str: @@ -343,7 +355,11 @@ def copy_add_param(self, key: str, value: typing.Any = None) -> URL: return self.copy_with(params=self.params.add(key, value)) def copy_remove_param(self, key: str) -> URL: - return self.copy_with(params=self.params.remove(key)) + # With the new merge semantics of the URL/Request constructor, passing + # `params=self.params.remove(key)` would re-merge the (now-empty) params + # with the existing query string and silently leave the param in place. + # Reset the query string to None so the param is actually removed. + return self.copy_with(query=None, params=self.params.remove(key)) def copy_merge_params(self, params: QueryParamTypes) -> URL: return self.copy_with(params=self.params.merge(params)) diff --git a/tests/httpx2/models/test_requests.py b/tests/httpx2/models/test_requests.py index 9428f0d7..e560e6e7 100644 --- a/tests/httpx2/models/test_requests.py +++ b/tests/httpx2/models/test_requests.py @@ -233,7 +233,9 @@ def test_request_params() -> None: assert str(request.url) == "http://example.com" request = httpx2.Request("GET", "http://example.com?c=3", params={"a": "1", "b": "2"}) - assert str(request.url) == "http://example.com?a=1&b=2" + assert str(request.url) == "http://example.com?c=3&a=1&b=2" + # `params={}` no longer wipes the URL's existing query string — merge + # semantics now apply in both directions. request = httpx2.Request("GET", "http://example.com?a=1", params={}) - assert str(request.url) == "http://example.com" + assert str(request.url) == "http://example.com?a=1" diff --git a/tests/httpx2/models/test_url.py b/tests/httpx2/models/test_url.py index f1f2288c..b39502e1 100644 --- a/tests/httpx2/models/test_url.py +++ b/tests/httpx2/models/test_url.py @@ -157,8 +157,8 @@ def test_url_params() -> None: assert url.params == httpx2.QueryParams({"a": "123"}) url = httpx2.URL("https://example.org:123/path/to/somewhere?b=456", params={"a": "123"}) - assert str(url) == "https://example.org:123/path/to/somewhere?a=123" - assert url.params == httpx2.QueryParams({"a": "123"}) + assert str(url) == "https://example.org:123/path/to/somewhere?b=456&a=123" + assert url.params == httpx2.QueryParams({"a": "123", "b": "456"}) # Tests for username and password diff --git a/tests/httpx2/test_url_params_merge.py b/tests/httpx2/test_url_params_merge.py new file mode 100644 index 00000000..53e48079 --- /dev/null +++ b/tests/httpx2/test_url_params_merge.py @@ -0,0 +1,62 @@ +""" +Tests for the URL constructor: `URL(url, params=...)` should merge the new +params with the URL's existing query string instead of replacing it. +Regression for issue #905. +""" + +from __future__ import annotations + +import httpx2 + + +def test_url_constructor_params_merges_with_existing_query() -> None: + """`URL("https://example.com/?a=1", params={"b": 2})` should keep `a=1`.""" + url = httpx2.URL("https://example.com/?a=1", params={"b": 2}) + assert url.params["a"] == "1" + assert url.params["b"] == "2" + + +def test_url_constructor_params_keeps_order_existing_first() -> None: + url = httpx2.URL("https://example.com/?a=1&b=2", params={"c": 3}) + assert str(url) == "https://example.com/?a=1&b=2&c=3" + + +def test_url_constructor_params_overrides_existing_on_key_collision() -> None: + """When the same key is given, the explicit `params` value wins.""" + url = httpx2.URL("https://example.com/?a=1", params={"a": "2"}) + assert url.params["a"] == "2" + + +def test_url_constructor_no_params_keeps_existing_query() -> None: + """No `params` keyword, existing query is preserved (sanity).""" + url = httpx2.URL("https://example.com/?a=1&b=2") + assert url.params["a"] == "1" + assert url.params["b"] == "2" + + +def test_url_constructor_empty_params_keeps_existing_query() -> None: + """Empty `params` should not wipe out the URL's query string.""" + url = httpx2.URL("https://example.com/?a=1", params={}) + assert url.params["a"] == "1" + + +def test_url_constructor_no_existing_query_just_uses_params() -> None: + url = httpx2.URL("https://example.com/", params={"a": "1", "b": "2"}) + assert url.params["a"] == "1" + assert url.params["b"] == "2" + + +def test_get_request_merges_query_with_params() -> None: + """End-to-end: `httpx2.get(url, params={...})` should concatenate.""" + # We don't make a network call — we only check the request that would be sent. + transport = httpx2.MockTransport(lambda req: httpx2.Response(200, json={"ok": True})) + with httpx2.Client(transport=transport) as client: + req = client.build_request( + "GET", + "https://httpbin.org/get?page=post&s=list", + params={"pid": 0, "tags": "k-on!"}, + ) + assert "page=post" in str(req.url) + assert "s=list" in str(req.url) + assert "pid=0" in str(req.url) + assert "tags=k-on" in str(req.url) # `!` is percent-encoded