classification
Title: Inconsistent urlparse/urllib.parse handling of invalid port values?
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, chad.birch, cvrebert, demian.brecht, martin.panter, orsenthil, python-dev, r.david.murray, rbcollins, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2013-12-23 23:51 by chad.birch, last changed 2015-08-09 21:54 by rbcollins. This issue is now closed.

Files
File name Uploaded Description Edit
port-ValueError.patch martin.panter, 2015-02-10 03:10 review
port-ValueError.v2.patch martin.panter, 2015-03-23 04:57 review
port-ValueError.v3.patch martin.panter, 2015-07-30 00:47 For 3.6 review
Messages (15)
msg206881 - (view) Author: Chad Birch (chad.birch) Date: 2013-12-23 23:51
I'm not sure if this is something that needs adjustment, but it seems somewhat inconsistent to me.

After using urlparse() on various urls with invalid port values, trying to access .port on the result will raise a ValueError. This case includes urls such as:

"http://www.example.com:asdf"

"http://www.example.com:1.5"

"http://www.example.com:"

However, as of May 24 2012 (http://hg.python.org/cpython/diff/d769e64aed79/Lib/urllib/parse.py), if the invalid port value is an integer, accessing .port will result in None. So this includes urls such as:

"http://www.example.com:66000"

"http://www.example.com:-1"

Should these two cases be made consistent, so that either .port is always None or always results in a ValueError if the port section of the url is invalid? I'd be happy to write a patch for it if it's wanted, but I thought I'd check first (and see which of the two options would be correct, if so).
msg232797 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2014-12-17 04:03
I would go for raising ValueError for port numbers out of range. The value of None was already defined to mean that no port is included in the URL.

Also, the ValueError exception should be documented. It is surprising that urlsplit() does not raise any exception, but then simply getting the “port” attribute does raise the exception.

BTW your third example is wrong:

>>> urlparse("http://www.example.com:").port is None
True

It’s probably safest to keep this one as it is, but maybe it also needs documenting :)
msg235660 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-02-10 03:10
Mapping out-of-range ports to None was added in Issue 14036, though I don’t understand why that approach was taken instead of raising ValueError. Here is a patch to raise ValueError for out-of-range integer values instead.
msg237002 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-02 01:27
See also Issue 20271, which has a proposed patch with more strict urlsplit() etc behaviour before even returning a SplitResult object.
msg237038 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-02 10:19
LGTM and I think it is worth to be applied to maintained releases.
msg237173 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-03-04 04:30
> I think it is worth to be applied to maintained releases.

I'd commit this only to the default branch. Changing the return value from None to an exception after three 3.4 bugfix releases(3.4.1, 3.4.2 and 3.4.3 -- also since 3.4.3 was released in Feb 2015, 3.4.4 will probably be the last bugfix release of 3.4) would not be the best action. I understand that the current situation is inconsistent, but I'm not sure it's worth it to commit to the 2.7 and 3.4 branches.
msg237399 - (view) Author: Demian Brecht (demian.brecht) * Date: 2015-03-06 23:35
> It is surprising that urlsplit() does not raise any exception

I have a bit of a TL;DR in #20271, trying to capture what the responsibilities of split and parse methods in urllib are and what they should be if consistency is something that we're after.


Around the patch though:

It seems quite odd to me to be raising exceptions on accessors rather than on instantiation or when the parsing occurs (in the body of urlparse). Wouldn't it better to raise the exception there?
msg238979 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-03-23 04:57
Patch v2 just changes a test to use “with self.assertRaises()”.

The behaviour of urlparse() succeeding and then result.port failing is indeed odd and surprising. Hopefully documenting this behaviour will help with the “surprising” aspect. But changing it would surely break compatibility. For example, the following is still potentially useful:

>>> urlsplit("http://localhost:ipp/").netloc
'localhost:ipp'

On Unix, some programs look up port names in /etc/services, even though this is not valid for URLs according to the RFCs, and urlsplit().port does not support it. In this case “ipp” would be whatever port the Internet Printing Protocol server is configured to run on.
msg247621 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-29 20:40
So, I think this is worth applying. The discussion around :ipp etc is irrelevant here: this patch changes large or negative ints to be a valueerror, as non-ints are.

The only question is where. I think this is in the category of 'will only break buggy applications' - applications that already handle ValueError to deal with bad inputs, will not be broken. Applications that depend on ports outside the valid range for ports will be broken, but thats the definition of the bug.

So I propose to apply to 2.7/3.4/3.5/3.6, but I'm going to seek a second opinion.
msg247625 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-07-29 20:52
Because it raises an error where none was raised before, I'd only apply this to 3.6.  This is especially true since this issue is not a *bug* report, but a "shouldn't this be more consistent" report.  That is, there's no great weight (the OP wasn't even sure it should be changed) in favor of the backport, so even a small possibility of breaking working code argues against the backport.

(To be clear: by working code I'm envisioning code that is getting that None value and using it as an error signal or treating it the same as a missing port.  Such code is technically "broken" but could be working if the default port happens to work, or the only out of range values it encounters are integers, or it is happy with the exception bubbling up in the non-integer cases.)
msg247628 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-29 20:55
ok, 3.6 only.
msg247638 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-07-29 22:01
If we take the 3.6-only path, does that warrant adding “Version changed” notices, and/or a What’s New entry?
msg247642 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-07-29 23:18
Hmm.  Good question.  I think it probably does, because it means getting an exception where one did not previously happen.
msg247643 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2015-07-30 00:47
Added versioning notices in port-ValueError.v3.patch.
msg248340 - (view) Author: Roundup Robot (python-dev) Date: 2015-08-09 21:53
New changeset 7c78279afc30 by Robert Collins in branch 'default':
Issue #20059: urllib.parse raises ValueError on all invalid ports.
https://hg.python.org/cpython/rev/7c78279afc30
History
Date User Action Args
2015-08-09 21:54:02rbcollinssetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2015-08-09 21:53:48python-devsetnosy: + python-dev
messages: + msg248340
2015-07-30 00:47:10martin.pantersetfiles: + port-ValueError.v3.patch

messages: + msg247643
2015-07-29 23:18:20r.david.murraysetmessages: + msg247642
2015-07-29 22:01:33martin.pantersetmessages: + msg247638
2015-07-29 20:55:09rbcollinssetmessages: + msg247628
versions: + Python 3.6, - Python 2.7, Python 3.4, Python 3.5
2015-07-29 20:52:06r.david.murraysetnosy: + r.david.murray
messages: + msg247625
2015-07-29 20:40:02rbcollinssetnosy: + rbcollins
messages: + msg247621
2015-03-23 04:57:00martin.pantersetfiles: + port-ValueError.v2.patch

messages: + msg238979
2015-03-06 23:35:28demian.brechtsetnosy: + demian.brecht
messages: + msg237399
2015-03-04 04:30:41berker.peksagsetmessages: + msg237173
2015-03-02 10:19:12serhiy.storchakasetnosy: + orsenthil

messages: + msg237038
stage: commit review
2015-03-02 01:27:07martin.pantersetmessages: + msg237002
2015-02-14 23:01:09berker.peksagsetnosy: + berker.peksag
2015-02-10 03:10:16martin.pantersetfiles: + port-ValueError.patch
keywords: + patch
messages: + msg235660
2014-12-17 04:03:48martin.pantersetmessages: + msg232797
2014-11-19 09:04:28serhiy.storchakasetnosy: + serhiy.storchaka

versions: + Python 2.7, Python 3.4, Python 3.5
2013-12-28 03:27:09martin.pantersetnosy: + martin.panter
2013-12-27 18:15:33cvrebertsetnosy: + cvrebert
2013-12-23 23:51:50chad.birchcreate