Trim excess leading path separators #6644
Conversation
sigmavirus24
force-pushed
the
bug/6643
branch
from
2cd2815
to
57e8c3b
Compare
src/requests/adapters.py
Outdated
| @@ -389,7 +390,7 @@ def request_url(self, request, proxies): | |||
| proxy_scheme = urlparse(proxy).scheme.lower() | |||
| using_socks_proxy = proxy_scheme.startswith("socks") | |||
|
|
|||
| url = request.path_url | |||
| url = re.sub("^/+", "/", request.path_url) | |||
There was a problem hiding this comment.
As mentioned on urllib3/urllib3#3352 this could also be
url = f"/{request.path_url.lstrip('/')}"I could benchmark these but I don't particularly care what the implementation is. I just threw this together to show that it can be fixed
There was a problem hiding this comment.
It looks like the f-string (Python 3.9-3.12 tested) is ~4x faster but we're talking on the scale of nanoseconds so it's basically moot. I'd vote the f-string for readability, but don't have a strong opinion.
There was a problem hiding this comment.
Yeah, I'm also happy to shove this into a branch too like
if path.startswith('//'):To make it clearer that we only care about the separator being repeated. What I want is clarity in the reader as to why we're doing this. My old school brain things the regexp is clearer and the f-string looks sus but that's just my opinion and I'm not holding it closely
There was a problem hiding this comment.
Yeah, branching seems fine to me too.
There was a problem hiding this comment.
Did both (f string and branch)
A URL with excess leading / (path-separator)s would cause urllib3 to attempt to reparse the request-uri as a full URI with a host and port. This bypasses that logic in ConnectionPool.urlopen by replacing these leading /s with just a single /. Closes psf#6643
sigmavirus24
force-pushed
the
bug/6643
branch
from
57e8c3b
to
60389df
Compare
A URL with excess leading / (path-separator)s would cause urllib3 to attempt to reparse the request-uri as a full URI with a host and port. This bypasses that logic in ConnectionPool.urlopen by replacing these leading /s with just a single /.
Closes #6643