Hi Łukasz ,

I wanted to push a revert in urlparse module in Python 3.8.2

I sent a note about this to python-committers, are you okay with this?

Thanks, Senthil

On Mon, Feb 10, 2020 at 6:27 AM Senthil Kumaran senthil@uthcode.com wrote:

Hello Python-Committers,

In https://bugs.python.org/issue27657, I introduced a regression in a minor release. The original patch to parsing logic of URL, cleaned up a lot of corner cases (https://github.com/python/cpython/pull/661) and I felt good about the change. However, the mistake was with the backport.

Demo:

$ ./python Python 3.8.0 (default, Feb 10 2020, 06:15:43) [GCC 9.2.1 20191008] on linux Type "help", "copyright", "credits" or "license" for more information.

...

...

...

from urllib.parse import urlparse urlparse('localhost:8080') ParseResult(scheme='', netloc='', path='localhost:8080', params='', query='', fragment='')

---

$ ./python Python 3.8.1+ (heads/3.8:b086ea5edc, Feb 10 2020, 06:15:44) [GCC 9.2.1 20191008] on linux Type "help", "copyright", "credits" or "license" for more information.

...

...

...

from urllib.parse import urlparse urlparse('localhost:8080') ParseResult(scheme='localhost', netloc='', path='8080', params='', query='', fragment='')

---

When I read an associated bug report against a user of the software (like this: https://github.com/mozilla/bleach/issues/503) - I feel that this was a mistake. The change of test-suite in minor versions should have alerted me, but I seem to have missed it.

I am planning to revert this change in 3.8.2 and 3.7.7 Should I highlight this in any documentation? Thoughts and opinions?

Thank you, Senthil

Thanks for sharing your thoughts.

I was originally thinking that if the problem is contained only to 3.8.1 and 3.7.6 and, reverted 3.8.2. If revert is not an option, then, I can add either of these in the documentation as the recommendation to deal with this problem.

1. Special case the "sys.version_info < (3, 8, 1)" for urlparse behavior.
2. Recommend making sure that urlparse is used on URL with the scheme. If the URL had a scheme ("http" or anything supported), the parsing behavior remains consistent.

It is only with schema-less URL this backport broke the parsing and upset maintainers of certain utilities (correctly).

On Tue, Feb 11, 2020 at 5:26 AM Łukasz Langa lukasz@langa.pl wrote:

I'll let others voice their opinions but my intuition for 3.8.x is to leave your patch be. True, it should not have been backported but it was, and it was already released as part of 3.8.1 and now 3.8.2rc1.

The user will have to special-case the change in behavior anyway. I feel like it is easier for them to do so by only special-casing "sys.version_info < (3, 8, 1)". The alternative if you revert your patch will be to special case both "sys.version_info < (3, 9)" and "sys.version_info == (3, 8, 1)" (let alone 3.8.2rc1). That is worse I think.

So, the only thing to do is to clearly document that there was a change, not in 3.9 but in 3.8.1 and forward.

Agreed that it's a bit of a mess for 3.7 since that was way later in the release cadence with only 3.7.6 getting the change. I will let Ned decide here.

• Ł

On 10 Feb 2020, at 15:27, Senthil Kumaran senthil@uthcode.com wrote:

Hello Python-Committers,

In https://bugs.python.org/issue27657, I introduced a regression in a minor release. The original patch to parsing logic of URL, cleaned up a lot of corner cases (https://github.com/python/cpython/pull/661) and I felt good about the change. However, the mistake was with the backport.

Demo:

$ ./python Python 3.8.0 (default, Feb 10 2020, 06:15:43) [GCC 9.2.1 20191008] on linux Type "help", "copyright", "credits" or "license" for more information.

...

...

...

from urllib.parse import urlparse urlparse('localhost:8080') ParseResult(scheme='', netloc='', path='localhost:8080', params='', query='', fragment='')

---

$ ./python Python 3.8.1+ (heads/3.8:b086ea5edc, Feb 10 2020, 06:15:44) [GCC 9.2.1 20191008] on linux Type "help", "copyright", "credits" or "license" for more information.

...

...

...

from urllib.parse import urlparse urlparse('localhost:8080') ParseResult(scheme='localhost', netloc='', path='8080', params='', query='', fragment='')

---

When I read an associated bug report against a user of the software (like this: https://github.com/mozilla/bleach/issues/503) - I feel that this was a mistake. The change of test-suite in minor versions should have alerted me, but I seem to have missed it.

I am planning to revert this change in 3.8.2 and 3.7.7 Should I highlight this in any documentation? Thoughts and opinions?

Thank you, Senthil

---

python-committers mailing list -- python-committers@python.org To unsubscribe send an email to python-committers-leave@python.org https://mail.python.org/mailman3/lists/python-committers.python.org/ Message archived at https://mail.python.org/archives/list/python-committers@python.org/message/2... Code of Conduct: https://www.python.org/psf/codeofconduct/

On Thu, Feb 13, 2020 at 1:57 AM Antoine Pitrou antoine@python.org wrote:

I would say it depends what the "revert" means:

• revert to pre-bugfix behavior?

This. Because the bug-fix was a bringing in backward-incompatible change that certain users are not happy with. https://github.com/mozilla/bleach/issues/503

• revert then apply a safer fix to the original bug?

No. The change introduced (bug-fix) was helpful in terms of consistent parsing and further improvement from maintainers pov. A safer fix might essentially mean revert to the old behavior.

The original bug(s) were about parsing expectations of an ill-defined 'scheme-less' URLs. Developers who relied on older behavior are surprised.

Ned, what are you doing with this for 3.7.7? Reverting?

• Ł

On 13 Feb 2020, at 22:18, Barry Warsaw barry@python.org wrote:

On Feb 11, 2020, at 05:26, Łukasz Langa lukasz@langa.pl wrote:

...

I'll let others voice their opinions but my intuition for 3.8.x is to leave your patch be. True, it should not have been backported but it was, and it was already released as part of 3.8.1 and now 3.8.2rc1.

I don’t think you should worry about 3.8.2rc1. That’s not a release version so shouldn’t impact the decision IMHO.

Whether 3.8.1 is important enough not to revert this change is an RM decision.

-Barry

On Feb 15, 2020, at 09:00, Senthil Kumaran senthil@uthcode.com wrote:

As we have to a decision here, my vote is to revert the patch in 3.8.2 and 3.7.7 I have gone back-and-forth with this thinking, and it seems revert might address some definite complaints we have got. The problem is contained to single version, and users can upgrade to the next one.

On Fri, Feb 14, 2020 at 8:14 AM Łukasz Langa lukasz@langa.pl wrote:

...

Ned, what are you doing with this for 3.7.7? Reverting?

Ugh!

As others have noted, urlparse is a big can of worms. I am certainly not a subject expert but, from some investigation and thinking about it, it seems to me that we kinda brought this on ourselves by allowing the scheme part (e.g. "https:" or "ftp:" or "any-old-scheme:" etc) of the urlstring parameter to be optional:

urllib.parse.urlparse(urlstring, scheme='', allow_fragments=True)

therby introducing the ambiguity of whether a string like "localhost:80" denotes a relative url with a user-defined scheme of "localhost" and a path of "80" (as it now does with the changes for bpo-27657 introduced in 3.8.1 and 3.7.6):

...

...

urlparse("localhost:80") ParseResult(scheme='localhost', netloc='', path='80', params='', query='', fragment='')

or denotes a relative url with no scheme and a path of "localhost:80" (as happened in previous releases):

...

...

urlparse("localhost:80") ParseResult(scheme='', netloc='', path='localhost:80', params='', query='', fragment='')

With an explicit scheme, in either case you get what you would expect - an absolute url:

...

...

urlparse("http://localhost:80") ParseResult(scheme='http', netloc='localhost:80', path='', params='', query='', fragment='')

AFAICT the intent of the original RFCs was to require an explicit scheme in a urlstring, thus avoiding any ambiguity. But the now universal practice of web browsers supplying a default http: or https: scheme for (partial) urls typed into a location bar has understandably changed user expectations to often be that schemes are optional when the scheme is clear in context.

So it seems to me that there is no one obviously correct behavior here. Judging from the comments and the reports of broken packages, many users are clearly used to using urlparse with schemeless urlstrings even if they aren't truly conformant URLs and even with the at first glance unintuitive way they were parsed by urlparse; for example, there is this snippet in the third-party requests package:

# urlparse is a finicky beast, and sometimes decides that there isn't a
# netloc present. Assume that it's being over-cautious, and switch netloc
# and path if urlparse decided there was no netloc.
if not netloc:
    netloc, path = path, netloc

OTOH, there are also undoubtedly users who want a urlparser that more strictly parses schemeless URLs, which is now the behavior as of 3.8.1 and 3.7.6, again, even if the new behavior is also unintuitive.

I don't see how we can satisfy both use cases without changing the API somehow. And there may be other use cases.

The good news is that, AFAICT from a quick survey, the change didn't affect urllib.urlopen or thrid-party urllib3 or requests. But from the "me-toos" on the bpo issue and the PR, it's clear that we broke stuff downstream and it seems that most of those users are waiting for a resolution from us and likely would prefer to stick to the previous behavior.

So my take is that we should revert the 3.7 changes (bpo-27657 / PR 16837 / 82b5f6b16e051f8a2ac6e87ba86b082fa1c4a77f ). Senthil, please go ahead and do so for the 3.7 branch. Thanks!

While it's not my call, I think we should also revert for 3.8.2.

For 3.9.0, I recommend we reconsider this change (temporarily reverting it) and consider whether an API change to accommodate the various use cases would be better; perhaps something like adding a new parameter to urlparse to indicate whether urlstrings should be parsed like webbrowser "urls" (and defining exactly what that means) and also review the many remaining open urlparse bpo issues to look for commonalities. (Perhaps that could be a post-3.9 GsoC project?)

Thoughts?

In any case, ugh!

-- Ned Deily nad@python.org -- []

FWIW, I agree with Senthil here. A slight behaviour change in 3.9 is fine, especially in an area where the "right" semantics are not immediately obvious. What we want to avoid is breaking behaviour changes in bugfix releases.

Regards

Antoine.

Le 16/02/2020 à 13:13, Senthil Kumaran a écrit :

On Sun, Feb 16, 2020 at 2:20 AM Ned Deily mailto:nad@python.org> wrote:

For 3.9.0, I recommend we reconsider this change (temporarily
reverting it) and consider whether an API change to accommodate the
various use cases would be better

For 3.9. - I am ready to defend the patch even at the cost of the breaking of the parsing of undefined behavior.  We should keep it. The patch simplifies a lot of corner cases and fixes the reported bugs. We don't guarantee backward compatibility between major versions, so I assume users will be careful when relying upon this undefined behavior and will take corrective action on their side before upgrading to 3.9.

We want patch releases to be backward compatible. That was the user-complaint.

Thanks, Senthil

---

python-committers mailing list -- python-committers@python.org To unsubscribe send an email to python-committers-leave@python.org https://mail.python.org/mailman3/lists/python-committers.python.org/ Message archived at https://mail.python.org/archives/list/python-committers@python.org/message/S... Code of Conduct: https://www.python.org/psf/codeofconduct/

I have created the PRs for the revert in 3.8.2 and 3.7.7

https://github.com/python/cpython/pull/18525 (3.8) - Acceptance definitely depends on the RM ( Łukasz' s call) and I will support it. https://github.com/python/cpython/pull/18526 (3.7) - Let this be reverted only if 3.8 gets reverted.

@Ned -

Note that we strongly imply that we sanely handle them by offering the "scheme=" parameter to urlparse.

It is a good idea. I will dig deeper with the help of test cases for 3.8. ( At the moment, given the tests cases the only test case which obviously fails the expectation in 'localhost:8000') But it seems that in the bug report, it was discussed and analyzed before the call was taken to simplify the logic of parsing support. So, for 3.9 use case, we can revisit this decision separately and i will account for the points that you have brought up.

If do not end up reverting in 3.8.2 - at the moment, my approach will be to document the behavior and encourage users to make sure that URL's have a scheme for the valid parsing behavior between 3.7, 3.8, and 3.9

On Sun, Feb 16, 2020 at 4:21 AM Antoine Pitrou antoine@python.org wrote:

FWIW, I agree with Senthil here. A slight behaviour change in 3.9 is fine, especially in an area where the "right" semantics are not immediately obvious. What we want to avoid is breaking behaviour changes in bugfix releases.

Regards

Antoine.

Le 16/02/2020 à 13:13, Senthil Kumaran a écrit :

...

On Sun, Feb 16, 2020 at 2:20 AM Ned Deily mailto:nad@python.org> wrote:

For 3.9.0, I recommend we reconsider this change (temporarily
reverting it) and consider whether an API change to accommodate the
various use cases would be better

For 3.9. - I am ready to defend the patch even at the cost of the breaking of the parsing of undefined behavior. We should keep it. The patch simplifies a lot of corner cases and fixes the reported bugs. We don't guarantee backward compatibility between major versions, so I assume users will be careful when relying upon this undefined behavior and will take corrective action on their side before upgrading to 3.9.

We want patch releases to be backward compatible. That was the user-complaint.

Thanks, Senthil

---

python-committers mailing list -- python-committers@python.org To unsubscribe send an email to python-committers-leave@python.org https://mail.python.org/mailman3/lists/python-committers.python.org/ Message archived at

https://mail.python.org/archives/list/python-committers@python.org/message/S...

...

Code of Conduct: https://www.python.org/psf/codeofconduct/

---

python-committers mailing list -- python-committers@python.org To unsubscribe send an email to python-committers-leave@python.org https://mail.python.org/mailman3/lists/python-committers.python.org/ Message archived at https://mail.python.org/archives/list/python-committers@python.org/message/P... Code of Conduct: https://www.python.org/psf/codeofconduct/

Thank you all. This is reverted in 3.8.2 and 3.7.7 - with NEWS explaining the behavior clearly. For the points that Ned brought up for 3.9, I will continue in the bug report https://bugs.python.org/issue27657 This discussion can be considered complete and closed.

Thanks for the support.

On Sun, Feb 16, 2020 at 12:13 PM Łukasz Langa lukasz@langa.pl wrote:

OK, let's revert this for 3.8.2. I will make 3.8.2rc2 with this to highlight the revert and get some testing in.

-- Best regards, Łukasz Langa

On 16 Feb 2020, at 19:31, Senthil Kumaran senthil@uthcode.com wrote:

 I have created the PRs for the revert in 3.8.2 and 3.7.7

https://github.com/python/cpython/pull/18525 (3.8) - Acceptance definitely depends on the RM ( Łukasz' s call) and I will support it. https://github.com/python/cpython/pull/18526 (3.7) - Let this be reverted only if 3.8 gets reverted.

@Ned -

...

Note that we strongly imply that we sanely handle them by offering the "scheme=" parameter to urlparse.

It is a good idea. I will dig deeper with the help of test cases for 3.8. ( At the moment, given the tests cases the only test case which obviously fails the expectation in 'localhost:8000') But it seems that in the bug report, it was discussed and analyzed before the call was taken to simplify the logic of parsing support. So, for 3.9 use case, we can revisit this decision separately and i will account for the points that you have brought up.

If do not end up reverting in 3.8.2 - at the moment, my approach will be to document the behavior and encourage users to make sure that URL's have a scheme for the valid parsing behavior between 3.7, 3.8, and 3.9

On Sun, Feb 16, 2020 at 4:21 AM Antoine Pitrou antoine@python.org wrote:

...

FWIW, I agree with Senthil here. A slight behaviour change in 3.9 is fine, especially in an area where the "right" semantics are not immediately obvious. What we want to avoid is breaking behaviour changes in bugfix releases.

Regards

Antoine.

Le 16/02/2020 à 13:13, Senthil Kumaran a écrit :

...

On Sun, Feb 16, 2020 at 2:20 AM Ned Deily mailto:nad@python.org> wrote:

For 3.9.0, I recommend we reconsider this change (temporarily
reverting it) and consider whether an API change to accommodate the
various use cases would be better

For 3.9. - I am ready to defend the patch even at the cost of the breaking of the parsing of undefined behavior. We should keep it. The patch simplifies a lot of corner cases and fixes the reported bugs. We don't guarantee backward compatibility between major versions, so I assume users will be careful when relying upon this undefined behavior and will take corrective action on their side before upgrading to 3.9.

We want patch releases to be backward compatible. That was the user-complaint.

Thanks, Senthil

---

python-committers mailing list -- python-committers@python.org To unsubscribe send an email to python-committers-leave@python.org https://mail.python.org/mailman3/lists/python-committers.python.org/ Message archived at

https://mail.python.org/archives/list/python-committers@python.org/message/S...

...

Code of Conduct: https://www.python.org/psf/codeofconduct/

---

python-committers mailing list -- python-committers@python.org To unsubscribe send an email to python-committers-leave@python.org https://mail.python.org/mailman3/lists/python-committers.python.org/ Message archived at https://mail.python.org/archives/list/python-committers@python.org/message/P... Code of Conduct: https://www.python.org/psf/codeofconduct/

---

python-committers mailing list -- python-committers@python.org To unsubscribe send an email to python-committers-leave@python.org https://mail.python.org/mailman3/lists/python-committers.python.org/ Message archived at https://mail.python.org/archives/list/python-committers@python.org/message/C... Code of Conduct: https://www.python.org/psf/codeofconduct/