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

Reverting #2354 #2712

Closed
karpetrosyan opened this issue May 21, 2023 · 3 comments
Closed

Reverting #2354 #2712

karpetrosyan opened this issue May 21, 2023 · 3 comments

Comments

@karpetrosyan
Copy link
Member

karpetrosyan commented May 21, 2023

According to RFC 3986 Section-3.4 Query

These query parameters are 100% correct.

/path-end?test
/path-end?test+test
/path-end?test/test
/path-end?test/test&test/test=test

RFC 3986 Query:

The characters slash ("/") and question mark ("?") may represent data
within the query component
. Beware that some older, erroneous
implementations
may not handle such data correctly when it is used as
the base URI for relative references (Section 5.1), apparently
because they fail to distinguish query data from path data when
looking for hierarchical separators. However, as query components
are often used to carry identifying information in the form of
"key=value" pairs and one frequently used value is a reference to
another URI, it is sometimes better for usability to avoid percent-
encoding those characters.

Furthermore, Rfc7230 Hypertext Transfer Protocol (HTTP/1.1): Message Syntax and Routing has no query constraints and supports RFC 3986 syntax, which allows such strange queries.

RFC 7230

http-URI = "http://" authority path-abempty [ "?" query ] [ "#"
fragment ]
https-URI = "https://" authority path-abempty [ "?" query ] [ "#"
fragment ]
query = <query, see [RFC3986], Section 3.4>

So when our uri is 'http://example.com?test' with params={'key':'value'} we should either override "?test" or raise an exception because merging these two formats does not make sense.

@karpetrosyan
Copy link
Member Author

karpetrosyan commented May 21, 2023

There are several helpful links related to this issue.

The initial issue that represents this problem: #2331
The pull request that 'fixed' the #2331: #2354

@karpetrosyan
Copy link
Member Author

I had no idea why we were merging 'params' with queries passed into strings in this manner.

>>> httpx.get('http://example.com?test', params={'test1': None}).request.url
URL('http://example.com?test=&test1=')

but expected

URL('http://example.com?test&test1=')

or

URL('http://example.com?testtest1=')

or even better

URL('http://example.com?test1=')

Params means we want an acceptable format "key=val&", but when we combine this with the string supplied into the uri, I don't believe there is a server that will recognize such a format.

Overriding queries passed into the string if they are not in "key=value" format is much more expected behavior, in my opinion.

For example

>>> httpx.get('http://example.com?test', params={'test1': None}).request.uri  # overriding because query is not 'key=value' format
URL('http://example.com?test1=')
>>> httpx.get('http://example.com?test=value', params={'test1': None}).request.uri
URL('http://example.com?test=value&test1=') # keep queries since they are in 'key=value' format.

@tomchristie Please reopen this issue if you find this behavior weird, or convert it to discussion if it appears to you to be fine.

@tomchristie
Copy link
Member

tomchristie commented May 22, 2023

There are two different aspects you're highlighting here...

There's "why do we merge query params, rather than replace them..." which we can narrow down a bit...

>>> httpx.Request('GET', 'http://example.com?a=1', params={'b': '2'})
<Request('GET', 'http://example.com?a=1&b=2')>

I'd agree that I don't think we have the most intuitive behaviour here. I'll raise a ticket for this. (EDIT: done #2713)

There's also "how do we handle ?a= vs ?a style empty query parameters". This is more complex but might also deserve a ticket.

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

No branches or pull requests

2 participants