Skip to content

Move from urlparse to parse_url for prepending schemes#5917

Merged
sigmavirus24 merged 1 commit into
psf:mainfrom
nateprewitt:proxy_scheme_unknown_fix
Dec 29, 2021
Merged

Move from urlparse to parse_url for prepending schemes#5917
sigmavirus24 merged 1 commit into
psf:mainfrom
nateprewitt:proxy_scheme_unknown_fix

Conversation

@nateprewitt
Copy link
Copy Markdown
Member

This addresses the base problem raised in #5855 with the urlparse changes in Python 3.9 and potential issues in future 3.7/3.8 releases. We've avoided changing other uses of urlparse for now due to issues with parse_url's strict validation requirements when performing parsing.

@nateprewitt nateprewitt marked this pull request as ready for review November 28, 2021 03:14
@geofft
Copy link
Copy Markdown

geofft commented Nov 30, 2021

Thanks for this! I've pulled in this patch into our internal monorepo and some preliminary testing shows that it's working (I can export https_proxy=some-proxy.example.com:3128, like a lot of our scripts do, and things work again under Python 3.9). I'll hopefully get it deployed today and can report on whether our users can successfully switch to 3.9.

@geofft
Copy link
Copy Markdown

geofft commented Dec 3, 2021

We've rolled this out and it works in prod.

@nateprewitt
Copy link
Copy Markdown
Member Author

Thanks for checking, @geofft! I think we just need another maintainer to give this a glance and we can look at moving the review forward.

Comment thread requests/utils.py
# urlparse and parse_url determine that there isn't a netloc present in some
# urls. We've chosen to assume parsing is being over-cautious, and switch
# the netloc and path. This is maintained for backwards compatibility.
netloc = parsed.netloc
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhhh, what? This doesn't make any sense to me, is this something that's only needed for urlparse?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is only a quirk for urlparse. I'd thought I repro'd with parse_url initially, but I'm unable to now.

I'll reword the whole comment for clarity. I'm not confident the test suite covers all of the initial issue though. I think I'm still in favor of leaving the swap in place as a fallback for anything we're missing.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sethmlarson I've reworded things to be a bit clearer.

@nateprewitt nateprewitt force-pushed the proxy_scheme_unknown_fix branch from bfab037 to ef59aa0 Compare December 29, 2021 04:12
@nateprewitt nateprewitt mentioned this pull request Dec 29, 2021
@sigmavirus24 sigmavirus24 merged commit 28d537d into psf:main Dec 29, 2021
@nateprewitt nateprewitt deleted the proxy_scheme_unknown_fix branch January 3, 2022 16:43
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants