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

Closed
yanyongyu opened this issue Dec 1, 2024 · 14 comments
Closed

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

yanyongyu opened this issue Dec 1, 2024 · 14 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.

@eliasdorneles
Copy link

Beware, this breaks compatibility with Requests, which does merge the parameters:

Image

@zanieb
Copy link
Contributor

zanieb commented Jan 26, 2025

It seems like it may be worth restoring the previous behavior for parity with requests? Is there a way we can discourage the "bad pattern" while still merging?

@jhominal
Copy link
Member

Personally, I believe that the "merge" behaviour is more intuitive for someone who specifies both a url with query parameters and a params dictionary.

I often encounter the case of a GET web service, that can be implemented by many different actors, who will give me a URL string that I will stick in a configuration.
And then, in order to call that web service, I will need to add some parameters that are specific to the call (a user ID, search parameters, etc.)
With the replace behaviour being the default, this means that, in order to handle the case where the original URL contains a query string, I need to do one of these things:

  1. parse the URL and put everything in params so that I do not lose anything (taking in account that httpx query params handling is not necessarily round-tripping for every query string, which can sometimes be important);
  2. parse the URL and use the copy_with_merge_params manually;
  3. detect whether the URL contains a ? and add a string suffix to the URL based on the parameters I want to add;

No matter the solution we recommend for merging, the params argument seems like a trap: params works only if I can be sure that my URL never has a query string - otherwise I need to do the merging dance systematically.

It would be a more honest API, in that case, if the params argument was removed and we said "you need to add the parameters to the URL in order for them to work". However, we should not do that because that would be a significant usability regression.

@urschrei
Copy link

The new behaviour also makes it more difficult to work with link URLs (e.g. prev, next, last) in http responses – I'm surprised it hasn't been brought up before. Now, extra logic is required to convert the query parameters in each link to a dict and then merge that with any other query parameters that might be required. We aren't short of tools to do that in the stdlib, but adding new logic like this requires extensive testing to ensure correctness because there are several corner cases.

@danieldotnl
Copy link

I run into this issue as well. Query params were removed from the url, just because I provided an empty dictionary: params = {}

@tomchristie tomchristie closed this as not planned Won't fix, can't repro, duplicate, stale Feb 7, 2025
@mcgfeller
Copy link

mcgfeller commented Feb 9, 2025

As a grateful user (and not in a position to contribute) to httpx, I can understand any decision made by maintainers.
Nevertheless, I'd like to ask @tomchristie to please explain the decision to close this, given that:

  • it changes the behavior from previous releases and breaks code (as shown in the links to issues in other projects), without a warning in the release notes.
  • it deviates from requests
  • some people consider it a practical (albeit maybe special case) feature (along the lines of Special cases aren't special enough to break the rules. Although practicality beats purity. in the Zen of Python).
  • makes it more difficult to provide link headers commonly used in HATEOAS (see @urschrei 's comment above)
  • workarounds are somewhat clumsy.

Thanks again for the great httpx and all the effort that go into it.
Martin

@tomchristie
Copy link
Member

Simply put...

The behavior of params=... matches browser behavior when submitting an HTML <form ...> element.

The existing docs don't yet describe this clearly.

@jhominal
Copy link
Member

jhominal commented Feb 9, 2025

I have taken a bit of time to look at HTTP client libraries in Java, .NET, Javascript, Ruby, Python and Go (you can find the raw results in an attachment), and:

  • Among 31 HTTP client APIs that were looked at, there are 3 main ways that these client libraries allow for setting the HTTP request URL and query parameters:

    1. Only have a parameter for a URL, generally as a string object (12 libraries, including all client APIs that are part of a standard library);
    2. Allow setting the URL and request query parameters using a Builder design pattern (7 libraries, only in statically typed languages);
    3. Have parameters for both URL and params objects (generally specified using the equivalent of a dict) (11 libraries, mostly in dynamically typed languages);
  • httpx has the same API pattern as the third category. However, all of the 11 libraries that follow that API pattern implement a merge of the query parameters. In other words, I want to insist that the choice at issue here is really unconventional, not only among Python HTTP libraries, but among all dynamically typed languages.

http-libraries.md

@urschrei
Copy link

urschrei commented Feb 9, 2025

submitting an HTML <form ...> element.

Apart from anything else, an HTTP request library is not a web browser, and isn't subject to the same constraints. I don't recall an argument that their behaviour should be consistent in this respect ever having been made, and I can't see how it would be justified.

@encode encode deleted a comment from ofer-pd Feb 12, 2025
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.

8 participants