Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing query params in url when params option is set #3433

Open
yanyongyu opened this issue Dec 1, 2024 · 5 comments
Open

Missing query params in url when params option is set #3433

yanyongyu opened this issue Dec 1, 2024 · 5 comments

Comments

@yanyongyu
Copy link

yanyongyu commented Dec 1, 2024

In version 0.28.0, query params are never merged.

import httpx

client = httpx.Client()
request = client.build_request(
    "GET", "https://httpbin.org/get?param=default", params={"foo": "bar"}
)
print(request.url)  # This will be `https://httpbin.org/get?foo=bar`

The query string in the request url will be ignored. It seems this is caused by the pr #3364. Params are replaced instead of merged when parsing.

query = kwargs.get("query", url_dict["query"])

Related discussion #3428

@tomchristie
Copy link
Member

Params are replaced instead of merged when parsing.

This is exactly the behavior I would expect.
As I see it, the simplification in #3364 introduced a bugfix that we haven't documented in the release.

@yanyongyu
Copy link
Author

I know providing both params and query in url is a bad practice. But this can be counterintuitive in some situations like:

import httpx

client = httpx.Client(params={"foo": "bar"})
request = client.build_request(
    "GET", "https://httpbin.org/get?param=default"
)
print(request.url)  # This will be `https://httpbin.org/get?foo=bar`

The params are provided in the client options. This will also replace the query in the url.

May be a warning can be raised when the replaced query is not empty?

query = kwargs.get("query", url_dict["query"])

@tomchristie
Copy link
Member

tomchristie commented Dec 3, 2024

Related issue... Client(params=...) is a not-good idea.

But this can be counterintuitive in some situations like

That is surprising behaviour, yes. I'd consider that a bug.

To the extent that we do currently support Client(params=...) we should probably? be merging those with any params=... passed by the user. Tho it's a bad pattern in the first place that we ideally could have avoided.

@yanyongyu
Copy link
Author

Currently, as a workaround, we merge all the params by ourselves before passing to httpx. The code is clearer logically 😂 .

atomiechen added a commit to atomiechen/HandyLLM that referenced this issue Dec 17, 2024
…ement (httpx #3433)

httpx issue: When passing params={}, always strictly update rather than merge with an existing querystring.

PR: encode/httpx#3364
Issue: encode/httpx#3433
ndelon added a commit to auditize/auditize that referenced this issue Dec 18, 2024
…with_filters broke by httpx upgrade

test_get_log_entities_with_filters breakage: encode/httpx#3433
@mcgfeller
Copy link

Broke quite some of my code, so at least a warning and a hint on how best to merge parameters should be issued.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants