RFC3986 compatible URL.join honoring empty segments

CHANGES/1039.bugfix.rst

@@ -0,0 +1,41 @@
+:meth:`URL.join <yarl.URL.join>` has been changed to match
+:rfc:`3986` and align with
+:meth:`/ operation <yarl.URL.__truediv__>` and :meth:`URL.joinpath() <yarl.URL.joinpath>`
+when joining URLs with empty segments.
+Previously :py:func:`urllib.parse.urljoin` was used,
+which has known issues with empty segments
+(`python/cpython#84774 <https://github.com/python/cpython/issues/84774>`_).
+
+Due to the semantics of :meth:`URL.join <yarl.URL.join>`, joining an
+URL with scheme requires making it relative, prefixing with ./.
+
+.. code-block:: python
+
+   URL("https://web.archive.org/web/").join(URL("./https://github.com/aio-libs/yarl"))
+
+
+Empty segments are honored in the base as well as the joined part.
+
+.. code-block:: python
+
+   URL("https://web.archive.org/web/https://").join(URL("github.com/aio-libs/yarl"))
+
+The required changes for URL.join affect the rendering of empty URL
+parameters.
+Previously empty URL parameters been presented as an assignment
+without value.
+
+e.g. :
+
+.. code-block:: text
+
+    http://b/p?a=
+
+Now empty parameters are presented without assignment.
+
+.. code-block:: text
+
+    http://b/p?a
+
+
+-- by :user:`commonism`

tests/test_url.py

@@ -1657,6 +1657,78 @@
     assert base.join(url) == expected
 
 
+EMPTY_SEGMENTS = [
+    (
+        "https://web.archive.org/web/",
+        "./https://github.com/aio-libs/yarl",
+        "https://web.archive.org/web/https://github.com/aio-libs/yarl",
+    ),
+    (
+        "https://web.archive.org/web/https://github.com/",
+        "aio-libs/yarl",
+        "https://web.archive.org/web/https://github.com/aio-libs/yarl",
+    ),
+]
+
+
+@pytest.mark.parametrize("base,url,expected", EMPTY_SEGMENTS)
+def test_join_empty_segments(base, url, expected):
+    base = URL(base)
+    url = URL(url)
+    expected = URL(expected)
+    joined = base.join(url)
+    assert joined == expected
+
+
+SIMPLE_BASE = "http://a/b/c/d"
+URLLIB_URLJOIN = [
+    ("", "http://a/b/c/g?y/./x", "http://a/b/c/g?y/./x"),
+    ("", "http://a/./g", "http://a/./g"),
+    ("svn://pathtorepo/dir1", "dir2", "svn://pathtorepo/dir2"),
+    ("svn+ssh://pathtorepo/dir1", "dir2", "svn+ssh://pathtorepo/dir2"),
+    ("ws://a/b", "g", "ws://a/g"),
+    ("wss://a/b", "g", "wss://a/g"),
+    # test for issue22118 duplicate slashes
+    (SIMPLE_BASE + "/", "foo", SIMPLE_BASE + "/foo"),
+    # Non-RFC-defined tests, covering variations of base and trailing
+    # slashes
+    ("http://a/b/c/d/e/", "../../f/g/", "http://a/b/c/f/g/"),
+    ("http://a/b/c/d/e", "../../f/g/", "http://a/b/f/g/"),
+    ("http://a/b/c/d/e/", "/../../f/g/", "http://a/f/g/"),
+    ("http://a/b/c/d/e", "/../../f/g/", "http://a/f/g/"),
+    ("http://a/b/c/d/e/", "../../f/g", "http://a/b/c/f/g"),
+    ("http://a/b/", "../../f/g/", "http://a/f/g/"),
+]
+
+
+@pytest.mark.parametrize("base,url,expected", URLLIB_URLJOIN)
+def test_join_cpython_urljoin(base, url, expected):
+    # tests from cpython urljoin
+    base = URL(base)
+    url = URL(url)
+    expected = URL(expected)
+    joined = base.join(url)
+    assert joined == expected
+
+
+URLLIB_URLJOIN_FAIL = [
+    # FIXME? - host is None
+    ("http:///", "..", "http:///"),
+    # issue 23703: don't duplicate filename
+    # FIXME? path without schema is not normalized
+    ("a", "b", "b"),
+]
+
+
+@pytest.mark.xfail(strict=True)
+@pytest.mark.parametrize("base,url,expected", URLLIB_URLJOIN_FAIL)
+def test_join_cpython_urljoin_fail(base, url, expected):
+    # non portable tests from cpython urljoin
+    base = URL(base)
+    url = URL(url)
+    base.join(url)
+
+
 def test_split_result_non_decoded():
     with pytest.raises(ValueError):
         URL(SplitResult("http", "example.com", "path", "qs", "frag"))

yarl/_url.py

@@ -6,7 +6,9 @@
 from contextlib import suppress
 from ipaddress import ip_address
 from typing import Union
-from urllib.parse import SplitResult, parse_qsl, quote, urljoin, urlsplit, urlunsplit
+from urllib.parse import SplitResult, parse_qsl, quote, urlsplit, urlunsplit
+from urllib.parse import uses_netloc as uses_authority
+from urllib.parse import uses_relative
 
 import idna
 from multidict import MultiDict, MultiDictProxy
@@ -259,12 +261,14 @@
                 netloc = authority
             else:
                 tmp = SplitResult("", authority, "", "", "")
+                port = None if tmp.port == cls._default_port(scheme) else tmp.port
                 netloc = cls._make_netloc(
-                    tmp.username, tmp.password, tmp.hostname, tmp.port, encode=True
+                    tmp.username, tmp.password, tmp.hostname, port, encode=True
                 )
         elif not user and not password and not host and not port:
             netloc = ""
         else:
+            port = None if port == cls._default_port(scheme) else port
             netloc = cls._make_netloc(
                 user, password, host, port, encode=not encoded, encode_host=not encoded
             )
@@ -455,12 +459,15 @@
     def _get_default_port(self) -> Union[int, None]:
         if not self.scheme:
             return None
+        return self._default_port(self.scheme)
 
+    @staticmethod
+    def _default_port(scheme: str) -> Union[int, None]:
         with suppress(KeyError):
-            return DEFAULT_PORTS[self.scheme]
+            return DEFAULT_PORTS[scheme]
 
         with suppress(OSError):
-            return socket.getservbyname(self.scheme)
+            return socket.getservbyname(scheme)
 
         return None
 
@@ -762,9 +769,7 @@
                     f"Appending path {path!r} starting from slash is forbidden"
                 )
             path = path if encoded else self._PATH_QUOTER(path)
-            segments = [
-                segment for segment in reversed(path.split("/")) if segment != "."
-            ]
+            segments = list(reversed(path.split("/")))
             if not segments:
                 continue
             # remove trailing empty segment for all but the last path
@@ -1153,7 +1158,48 @@
         # See docs for urllib.parse.urljoin
         if not isinstance(url, URL):
             raise TypeError("url should be URL")
-        return URL(urljoin(str(self), str(url)), encoded=True)
+        other: URL = url
+        scheme = other.scheme or self.scheme
+        parts = {
+            k: getattr(self, k)
+            for k in ["authority", "path", "query_string", "fragment"]
+        }
+        parts["scheme"] = scheme
+
+        if scheme != self.scheme or scheme not in uses_relative:
+            return URL(str(other))
+
+        if scheme in uses_authority:
+            if other.authority:
+                parts.update(
+                    {
+                        k: getattr(other, k)
+                        for k in ["authority", "path", "query_string", "fragment"]
+                    }
+                )
+                return URL.build(**parts)
+
+        if other.path or other.fragment:
+            parts["fragment"] = other.fragment
+        if other.path or other.query:
+            parts["query_string"] = other.query_string
+
+        if not other.path:
+            return URL.build(**parts)
+
+        if other.path[0] == "/":
+            parts["path"] = other.path
+            return URL.build(**parts)
+
+        if self.path[-1] == "/":
+            # using an intermediate to avoid URL.joinpath dropping query & fragment
+            parts["path"] = URL(self.path).joinpath(other.path).path
+        else:
+            # …
+            # and relativizing ".."
+            parts["path"] = URL(self.path).joinpath("..", other.path).path
+
+        return URL.build(**parts)
 
     def joinpath(self, *other, encoded=False):
         """Return a new URL with the elements in other appended to the path."""