urljoin should raise a TypeError if URL is not a string

[jaraco]

BPO	19094
Nosy	@jaraco, @orsenthil, @berkerpeksag, @serhiy-storchaka, @vajrasky, @iritkatriel, @jacobtylerwalls
PRs	gh-63293: Deprecate providing false values besides strings or bytes to urllib.parse functions #26687
Files	urljoin_throws_type_error.patch urljoin_throws_type_error_v2.patch urljoin_throws_type_error_v3.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2013-09-25.20:55:41.793>
labels = ['type-bug', 'library', '3.9', '3.10', '3.11']
title = 'urljoin should raise a TypeError if URL is not a string'
updated_at = <Date 2021-06-12.01:47:56.398>
user = 'https://github.com/jaraco'

bugs.python.org fields:

activity = <Date 2021-06-12.01:47:56.398>
actor = 'jacobtylerwalls'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2013-09-25.20:55:41.793>
creator = 'jaraco'
dependencies = []
files = ['31876', '31889', '31933']
hgrepos = []
issue_num = 19094
keywords = ['patch']
message_count = 12.0
messages = ['198418', '198435', '198455', '198462', '198493', '198510', '198750', '198784', '198808', '230742', '386875', '395598']
nosy_count = 7.0
nosy_names = ['jaraco', 'orsenthil', 'berker.peksag', 'serhiy.storchaka', 'vajrasky', 'iritkatriel', 'jacobtylerwalls']
pr_nums = ['26687']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue19094'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

[jaraco]

>>> import urllib.parse
>>> urllib.parse.urljoin('foo', [])
'foo'

That should raise a TypeError, not succeed quietly as if an empty string were passed.

[vajrasky]

This is the preliminary patch to raise TypeError in that situation.

[jaraco]

Thanks Vajraski for the patch (especially the tests).

A colleague reminded me of an aphorism by Raymond Hettinger from the recent PyCon paraphrased: duck typing is superior to isinstance.

Maybe instead consider something like this:

diff -r 7f13d5ecf71f Lib/urllib/parse.py
--- a/Lib/urllib/parse.py	Sun Sep 22 09:33:45 2013 -0400
+++ b/Lib/urllib/parse.py	Thu Sep 26 18:52:18 2013 -0400
@@ -405,10 +405,9 @@
 def urljoin(base, url, allow_fragments=True):
     """Join a base URL and a possibly relative URL to form an absolute
     interpretation of the latter."""
-    if not base:
-        return url
-    if not url:
-        return base
+    if not base or not url:
+        # one of the inputs is empty, so simply concatenate
+        return base + url
     base, url, _coerce_result = _coerce_args(base, url)
     bscheme, bnetloc, bpath, bparams, bquery, bfragment = \
             urlparse(base, '', allow_fragments)

This implementation is not only shorter and raises TypeErrors if the types aren't compatible, but those errors are triggered by the underlying implementation. Furthermore, if either the url or base were to be a duck-type that met the necessary interface, it need not be a string subclass.

I don't feel strongly either way, but I'm slightly more inclined to accept the simpler implementation. I'm interested in the everyone's thoughts on either approach.

[vajrasky]

What about this one?

>>> urljoin(['a'], [])
['a']
>>> urljoin(['a'], ['b'])
..... omitted ......
AttributeError: 'list' object has no attribute 'decode'

Is this desirable? Both patches missed this case. Should we only accept str and bytes?

[jaraco]

I don't mind the AttributeError - that's a reasonable exception when passing invalid types in, and that's in fact the current behavior. The example of (['a'], []) does bother me though. Those inputs are also seemingly invalid, though somewhat more compatible from a 'urljoin' perspective than two different types.

You've given me reason to re-consider your patch.

[vajrasky]

I modified the patch to handle the last case using your way as well.

Anyway, I found out that urlsplit and urlparse got the same issue as well.

>>> urlparse('python.org', b'http://')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/sky/Code/python/programming_language/cpython/Lib/urllib/parse.py", line 292, in urlparse
    url, scheme, _coerce_result = _coerce_args(url, scheme)
  File "/home/sky/Code/python/programming_language/cpython/Lib/urllib/parse.py", line 109, in _coerce_args
    raise TypeError("Cannot mix str and non-str arguments")
TypeError: Cannot mix str and non-str arguments
>>> urlparse('python.org', b'')
ParseResult(scheme=b'', netloc='', path='python.org', params='', query='', fragment='')
>>> urlparse('python.org', 0)
ParseResult(scheme=0, netloc='', path='python.org', params='', query='', fragment='')

Same thing happens in urlsplit. Fortunately, urlunsplit and urlunparse don't have this issue.

[orsenthil]

I have provided my comments in the review tool. Please check them out.

[vajrasky]

Okay, attached the patch based on your comment, Senthil Kumaran. Thanks.

The reason I use isinstance to check the type is because I want to support the inheritance as much as possible.

>>> class new_str(str):
...   pass
>>> urljoin(new_str('http://python.org') + new_str('hehe'))
will not work with the newest patch.

Also, in Lib/urllib/parse.py, most of the time, we use isinstance(x, str) not type(x) == str.

But I don't know. Maybe it does not matter.

[jaraco]

Vajrasky, you're right. Comparing against type(obj) is an anti-pattern. isinstance is better. Duck typing is even better (in many cases).

[serhiy-storchaka]

About acceptable behavior with wrong arguments types see discussions in bpo-22766 and http://comments.gmane.org/gmane.comp.python.devel/150073 .

[iritkatriel]

Still broken in 3.10:

Running Release|x64 interpreter...
Python 3.10.0a5+ (heads/master:bf2e7e55d7, Feb 11 2021, 23:09:25) [MSC v.1928 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import urllib.parse
>>> urllib.parse.urljoin('foo', [])
'foo'
>>>