gh-76960: Fix urljoin() and urldefrag() for URIs with empty components #123273

serhiy-storchaka · 2024-08-23T20:24:50Z

urljoin() with relative reference "?" sets empty query and removes fragment.
Preserve empty components (authority, params, query, fragment) in urljoin().
Preserve empty components (authority, params, query) in urldefrag().

Also refactor the code and get rid of double _coerce_args() and _coerce_result() calls in urljoin(), urldefrag(), urlparse() and urlunparse().

Issue: urljoining an empty query string doesn't clear query string #76960

…ponents * urljoin() with relative reference "?" sets empty query and removes fragment. * Preserve empty components (authority, params, query, fragment) in urljoin(). * Preserve empty components (authority, params, query) in urldefrag(). Also refactor the code and get rid of double _coerce_args() and _coerce_result() calls in urljoin(), urldefrag(), urlparse() and urlunparse().

serhiy-storchaka · 2024-08-23T20:25:40Z

Lib/test/test_urlparse.py

-        self.checkJoin(SIMPLE_BASE, 'http:g','http://a/b/c/g')
-        self.checkJoin(SIMPLE_BASE, 'http:','http://a/b/c/d')


Duplicated below.

serhiy-storchaka · 2024-08-23T20:26:11Z

Lib/test/test_urlparse.py

-        self.checkJoin(SIMPLE_BASE, 'http:','http://a/b/c/d')
-        self.checkJoin(SIMPLE_BASE, 'http:?y','http://a/b/c/d?y')


Duplicated in new tests below.

serhiy-storchaka · 2024-08-23T20:34:10Z

Lib/urllib/parse.py

+    v = SplitResult(scheme or '', netloc or '', url, query or '', fragment or '')
+    return _coerce_result(v)
+
+def _urlsplit(url, scheme=None, allow_fragments=True):


This refactoring is also a preparation for #67041. These private functions distinguish undefined components (None) from empty components (empty string), but public functions remove this difference. With new options added for #67041 this will be optional.

serhiy-storchaka · 2024-08-23T20:40:24Z

Lib/urllib/parse.py

+            if fragment is None:
+                fragment = bfragment


This contradicts RFC 3986 according to which the target fragment is always set to the relative reference fragment. But this means that an empty string (which has no defined fragment) should remove fragment. I think this is an error in RFC 3986.

But this means that an empty string (which has no defined fragment) should remove fragment. I think this is an error in RFC 3986.

Just to be explicit, this means we are keeping the existing behavior when a relative fragment is given an empty string (or None). Is that right?

Practically, this only affects 4 cases: an empty string, '//'. and URIs like 'http:' and http:// where the scheme is the same as in the base URI.

urljoin('http:/a/b/c/d;p?q#f', '') currently returns the base URI 'http:/a/b/c/d;p?q#f'. The same in Ruby and Go. According to the RFC 3986 pseudocode it should drop fragment. According to previous RFCs it should return the base URI. I think it is important to keep the current behavior in this case, so there should be some error in RFC 3986.

urljoin('http:/a/b/c/d;p?q#f', '//') currently returns the base URI without fragment 'http:/a/b/c/d;p?q', which is consistent with RFC 3986 for non-strict parser. Ruby and Go do not distinguish between '//' and '', so they preserve fragment. The current PR preserves it too.

urljoin('http:/a/b/c/d;p?q#f', 'http:') currently returns the base URI without fragment 'http:/a/b/c/d;p?q', which is consistent with RFC 3986 for non-strict parser. But Ruby and Go return 'http: which is consistent with RFC 3986 for strict parser. With this PR it will return the base URI unchanged, this is a change from the current behavior and is not consistent with RFC 3986 unless we suppose that RFC 3986 was wrong in this place.

But maybe RFC 3986 is wrong in other place. Previous RFCs contained special statement for empty relative URI. If it is missed in RFC 3986 by mistake, then we can only add special case for empty relative URI and preserve the current behavior for urljoin('http:/a/b/c/d;p?q#f', 'http:') which is consistent with the pseudocode in RFC 3986.

We can only add a special case for empty relative URI (it is already here for performance reasons), with possible addition of '//'. Then the behavior for 'http:' and 'http://' will be consistent with 3986 and match the current behavior (but Ruby and Go go other way).

serhiy-storchaka · 2024-08-23T20:43:59Z

Lib/urllib/parse.py


+    if scheme is None:
+        scheme = bscheme
    if scheme != bscheme or scheme not in uses_relative:
        return _coerce_result(url)
    if scheme in uses_netloc:
        if netloc:


According to RFC 3986, this should be netlog is not None (undefined). But this gives results too different from Ruby and Go (they may be wrong) and from the current Python code. I leave this for a separate issue.

Being consistent with Ruby, Go, and Java (and usually with browser/curl) somes seems to be more beneficial, and going with that is expected by the programmer.

serhiy-storchaka · 2024-08-23T21:11:36Z

@orsenthil, please take a look at this PR.

orsenthil · 2024-08-30T17:01:22Z

Misc/NEWS.d/next/Library/2024-08-23-22-01-30.gh-issue-76960.vsANPu.rst

+Fix :func:`urllib.parse.urljoin` and :func:`urllib.parse.urldefrag` for URIs
+containing empty components. For example, :func:`!urljoin()` with relative
+reference "?" now sets empty query and removes fragment. Empty components are
+preserved in the result.


What is meant by " Empty components are preserved in the result." in this news entry?

Got it what is meant in the ticket summary.

Preserve empty components (authority, params, query, fragment) in urljoin(). Preserve empty components (authority, params, query) in urldefrag().

I guess that I should be more explicit (even if a little repetitive) here.

orsenthil · 2024-08-30T17:28:32Z

Lib/test/test_urlparse.py

+        self.checkJoin(RFC1808_BASE, '?', 'http://a/b/c/d;p?', relroundtrip=False)
+        self.checkJoin(RFC1808_BASE, '?#z', 'http://a/b/c/d;p?#z', relroundtrip=False)
+        self.checkJoin(RFC1808_BASE, '?y', 'http://a/b/c/d;p?y')
+        self.checkJoin(RFC1808_BASE, ';', 'http://a/b/c/;')


Making a note, using parameter ; removes the final portion of the path in the base url /d in this case.

And this as per the rfc3986 requirement as given in the 5.4. Reference Resolution Examples and these tests handle them properly for the empty ? cases correctly.

From RFC3986's point of view, ; is not a delimiter, it is a normal character. So /b/c/d;p is the old path, d;p is its last part, and it will be replaced with relative path ;.

Other example:

>>> urljoin('http:/a/b/c/d;?#f', '#z') 'http:///a/b/c/d#z'

Currently it removes ; and ? which correspond to empty params and query. But for RFC3986 they should be preserved. It is not only a matter of representation of empty component, params is not a thing in RFC3986. Removing trailing ; from path /b/c/d; is a bug.

orsenthil

LGTM.

Thank you @serhiy-storchaka for bringing this long pending and desirable change into urllib.parse module in a relatively safe way.

serhiy-storchaka · 2024-08-31T09:19:33Z

Thank you for your review @orsenthil.

During writing responses to your comments I reconsider my approach to empty URI problem, tried different ways, but have not found good reasons to prefer one way or another for such corner cases like //, http: and http://. The current approach at least looks more consistent for all 4 cases, so I leave it. This is not the last PR for these functions, we will need to add more options for consistency with Ruby and Go, and maybe change some defaults in future.

WinChua · 2024-09-20T02:59:43Z

This commin change the behavior of urljoin. Consider this code urljoin('file:', '/home'), before this commit it produced 'file:///home', while it produces 'file:/home' now, which may break some code

serhiy-storchaka requested a review from orsenthil August 23, 2024 20:24

bedevere-app bot mentioned this pull request Aug 23, 2024

urljoining an empty query string doesn't clear query string #76960

Closed

bedevere-app bot added the awaiting core review label Aug 23, 2024

serhiy-storchaka commented Aug 23, 2024

View reviewed changes

orsenthil reviewed Aug 30, 2024

View reviewed changes

orsenthil approved these changes Aug 30, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Aug 30, 2024

Update NEWS.

a65a5f0

serhiy-storchaka merged commit fc897fc into python:main Aug 31, 2024
34 checks passed

bedevere-app bot removed the awaiting merge label Aug 31, 2024

serhiy-storchaka deleted the urllib-urljoin2 branch August 31, 2024 09:42

WinChua mentioned this pull request Sep 20, 2024

fix DocumentURI error for gopls@v0.16.1 ycm-core/ycmd#1762

Closed

nineteendo mentioned this pull request Oct 24, 2024

urljoin() undocumented behavior change in Python 3.14. #125926

Closed

hroncok mentioned this pull request Oct 25, 2024

3.14.0a1 urljoin regression wrt // #125974

Open

serhiy-storchaka mentioned this pull request Nov 11, 2024

gh-76960: Fix urljoining with an empty query string. #5645

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-76960: Fix urljoin() and urldefrag() for URIs with empty components #123273

gh-76960: Fix urljoin() and urldefrag() for URIs with empty components #123273

serhiy-storchaka commented Aug 23, 2024 •

edited by bedevere-app bot

Loading

serhiy-storchaka Aug 23, 2024

serhiy-storchaka Aug 23, 2024

serhiy-storchaka Aug 23, 2024

serhiy-storchaka Aug 23, 2024

orsenthil Aug 30, 2024

serhiy-storchaka Aug 31, 2024

serhiy-storchaka Aug 23, 2024

orsenthil Aug 30, 2024

serhiy-storchaka commented Aug 23, 2024

orsenthil Aug 30, 2024

orsenthil Aug 30, 2024

serhiy-storchaka Aug 31, 2024

orsenthil Aug 30, 2024

orsenthil Aug 30, 2024

serhiy-storchaka Aug 31, 2024

orsenthil left a comment

serhiy-storchaka commented Aug 31, 2024

WinChua commented Sep 20, 2024

		self.checkJoin(SIMPLE_BASE, 'http:g','http://a/b/c/g')
		self.checkJoin(SIMPLE_BASE, 'http:','http://a/b/c/d')

		self.checkJoin(SIMPLE_BASE, 'http:','http://a/b/c/d')
		self.checkJoin(SIMPLE_BASE, 'http:?y','http://a/b/c/d?y')

gh-76960: Fix urljoin() and urldefrag() for URIs with empty components #123273

gh-76960: Fix urljoin() and urldefrag() for URIs with empty components #123273

Conversation

serhiy-storchaka commented Aug 23, 2024 • edited by bedevere-app bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka commented Aug 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orsenthil left a comment

Choose a reason for hiding this comment

serhiy-storchaka commented Aug 31, 2024

WinChua commented Sep 20, 2024

serhiy-storchaka commented Aug 23, 2024 •

edited by bedevere-app bot

Loading