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

fix #2247 #2249

Merged
merged 2 commits into from Oct 5, 2014

Conversation

@t-8ch
Copy link
Contributor

commented Sep 25, 2014

We have to pass urllib3 the url without the authentication information,
else it will be parsed by httplib as a netloc and included in the request line
and Host header

fix #2247
We have to pass urllib3 the url without the authentication information,
else it will be parsed by httplib as a netloc and included in the request line
and Host header
@Lukasa

This comment has been minimized.

Copy link
Member

commented Sep 25, 2014

This looks right, but it's hard for me to know because URLs are a crazy mess. I'll let @sigmavirus24 review this, he knows URLs better than I do.

@sigmavirus24

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2014

So if we used a better (read: less forgiving) URL parser library we wouldn't need to split on @ (like we do here). In principle this works fine. In reality, we should get something like rfc3986 into acceptable shape for @shazow or just make the Url object in urllib3 more RFC compliant (whichever works better) and use that for parsing the URL and reconstructing it. In short, you have 5 major components of the URL:

{scheme}://{authority}{/path}{?query}{#fragment}

And authority which we're dealing with right now has 3 sub-components so a URL would look like:

{scheme}://{userinfo@}{hostname}{:port}{/path}{?query}{#fragment}

rfc3986 would split this up and allow you to replace userinfo with None and then reconstruct the URL. For a quick fix, this is great. I'd rather not have so much URL/URI parsing logic in requests though, this is an HTTP library not an HTTP + URL + ... library.

@@ -17,7 +17,7 @@
from .packages.urllib3.util import Timeout as TimeoutSauce
from .compat import urlparse, basestring, urldefrag

This comment has been minimized.

Copy link
@sigmavirus24

sigmavirus24 Sep 26, 2014

Contributor

Please remove the import of urldefrag here.

This comment has been minimized.

Copy link
@t-8ch

t-8ch Sep 26, 2014

Author Contributor
@t-8ch

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2014

+∞. I think it would be best to have URL objects that are immutable and an API like URL.replace(userinfo=None, fragment=None)

@sigmavirus24

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2014

@t-8ch that's roughly rfc3986's API, except that I don't think I call it replace but maybe I do.... I'll double check later.

@shazow

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2014

@sigmavirus24 Please open the specific issue with urllib3's url parser, no need to be backhanded. :)

@sigmavirus24

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2014

Sorry @shazow, it wasn't meant to be back handed. I'll pull together the list of things the object is missing and make an issue with it tonight.

@shazow

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2014

Thanks. :)

@kennethreitz

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2014

What's the status of this?

@sigmavirus24

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2014

kennethreitz added a commit that referenced this pull request Oct 5, 2014

@kennethreitz kennethreitz merged commit 5850b1f into psf:master Oct 5, 2014

1 check passed

default Merged build finished.
Details

@t-8ch t-8ch deleted the t-8ch:fix_2247 branch Oct 5, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.