From 28a02ba62051b1e98548f8807d16cfae62680b93 Mon Sep 17 00:00:00 2001
From: Arun Babu Neelicattu <arun.neelicattu@gmail.com>
Date: Sun, 12 Apr 2020 15:10:40 +0200
Subject: [PATCH] Improve support for PEP-440 direct references

With this change we allow for PEP-508 string for file dependencies
to use PEP-440 direct reference using the file URI scheme (RFC-8089).

Note that this implementation will not allow non RFC-8089 file path
references. In order to allow for sdist to be portable in sane use
cases, we ensure that relative path dependencies do not use the
file URI scheme, but preserve path if relative or directories.

In addition to file resource, directory dependencies now support
the same scheme.

References:
- https://www.python.org/dev/peps/pep-0508/#backwards-compatibility
- https://www.python.org/dev/peps/pep-0440/#direct-references
- https://tools.ietf.org/html/rfc8089
- https://discuss.python.org/t/what-is-the-correct-interpretation-of-path-based-pep-508-uri-reference/2815/11
---
 poetry/core/masonry/builders/complete.py     |  6 +-
 poetry/core/packages/__init__.py             | 62 +++++++++++++++++--
 poetry/core/packages/directory_dependency.py | 11 ++++
 poetry/core/packages/file_dependency.py      | 13 ++++
 poetry/core/packages/utils/utils.py          | 56 +++++++++++------
 poetry/core/utils/_compat.py                 |  8 +--
 poetry/core/version/requirements.py          | 12 ++--
 tests/masonry/builders/test_builder.py       | 14 ++---
 tests/packages/test_directory_dependency.py  | 45 ++++++++++++++
 tests/packages/test_file_dependency.py       | 44 ++++++++++++++
 tests/packages/utils/test_utils_urls.py      | 63 ++++++++++++++++++++
 11 files changed, 292 insertions(+), 42 deletions(-)
 create mode 100644 tests/packages/utils/test_utils_urls.py

diff --git a/poetry/core/masonry/builders/complete.py b/poetry/core/masonry/builders/complete.py
index 5c91a34dc..c99d851ed 100644
--- a/poetry/core/masonry/builders/complete.py
+++ b/poetry/core/masonry/builders/complete.py
@@ -38,14 +38,12 @@ def build(self):
 
                 with self.unpacked_tarball(sdist_file) as tmpdir:
                     WheelBuilder.make_in(
-                        Factory().create_poetry(tmpdir),
-                        dist_dir,
-                        original=self._poetry,
+                        Factory().create_poetry(tmpdir), dist_dir, original=self._poetry
                     )
         else:
             with self.unpacked_tarball(sdist_file) as tmpdir:
                 WheelBuilder.make_in(
-                    Factory().create_poetry(tmpdir), dist_dir, original=self._poetry,
+                    Factory().create_poetry(tmpdir), dist_dir, original=self._poetry
                 )
 
     @classmethod
diff --git a/poetry/core/packages/__init__.py b/poetry/core/packages/__init__.py
index 3e98a5398..e89b8a12f 100644
--- a/poetry/core/packages/__init__.py
+++ b/poetry/core/packages/__init__.py
@@ -1,7 +1,11 @@
 import os
 import re
 
+from typing import Optional
+from typing import Union
+
 from poetry.core.semver import Version
+from poetry.core.utils._compat import Path
 from poetry.core.utils.patterns import wheel_file_re
 from poetry.core.version.requirements import Requirement
 
@@ -19,10 +23,38 @@
 from .utils.utils import is_url
 from .utils.utils import path_to_url
 from .utils.utils import strip_extras
+from .utils.utils import url_to_path
 from .vcs_dependency import VCSDependency
 
 
-def dependency_from_pep_508(name):
+def _make_file_or_dir_dep(
+    name, path, base=None
+):  # type: (str, Path, Optional[Path]) -> Optional[Union[FileDependency, DirectoryDependency]]
+    """
+    Helper function to create a file or directoru dependency with the given arguments. If
+    path is not a file or directory that exists, `None` is returned.
+    """
+    _path = path
+    if not path.is_absolute() and base:
+        # a base path was specified, so we should respect that
+        _path = Path(base) / path
+
+    if _path.is_file():
+        return FileDependency(name, path, base=base)
+    elif _path.is_dir():
+        return DirectoryDependency(name, path, base=base)
+
+    return None
+
+
+def dependency_from_pep_508(
+    name, relative_to=None
+):  # type: (str, Optional[Path]) -> Dependency
+    """
+    Resolve a PEP-508 requirement string to a `Dependency` instance. If a `relative_to`
+    path is specified, this is used as the base directory if the identified dependency is
+    of file or directory type.
+    """
     from poetry.core.vcs.git import ParsedUrl
 
     # Removing comments
@@ -63,20 +95,27 @@ def dependency_from_pep_508(name):
 
     # it's a local file, dir, or url
     if link:
+        is_file_uri = link.scheme == "file"
+        is_relative_uri = is_file_uri and re.search(r"\.\./", link.url)
+
         # Handle relative file URLs
-        if link.scheme == "file" and re.search(r"\.\./", link.url):
-            link = Link(path_to_url(os.path.normpath(os.path.abspath(link.path))))
+        if is_file_uri and is_relative_uri:
+            path = Path(link.path)
+            if relative_to:
+                path = relative_to / path
+            link = Link(path_to_url(path))
+
         # wheel file
         if link.is_wheel:
             m = wheel_file_re.match(link.filename)
             if not m:
                 raise ValueError("Invalid wheel name: {}".format(link.filename))
-
             name = m.group("name")
             version = m.group("ver")
             dep = Dependency(name, version)
         else:
             name = req.name or link.egg_fragment
+            dep = None
 
             if link.scheme.startswith("git+"):
                 url = ParsedUrl.parse(link.url)
@@ -84,8 +123,21 @@ def dependency_from_pep_508(name):
             elif link.scheme == "git":
                 dep = VCSDependency(name, "git", link.url_without_fragment)
             elif link.scheme in ["http", "https"]:
-                dep = URLDependency(name, link.url_without_fragment)
+                dep = URLDependency(name, link.url)
+            elif is_file_uri:
+                # handle RFC 8089 references
+                path = url_to_path(req.url)
+                dep = _make_file_or_dir_dep(name=name, path=path, base=relative_to)
             else:
+                try:
+                    # this is a local path not using the file URI scheme
+                    dep = _make_file_or_dir_dep(
+                        name=name, path=Path(req.url), base=relative_to
+                    )
+                except ValueError:
+                    pass
+
+            if dep is None:
                 dep = Dependency(name, "*")
     else:
         if req.pretty_constraint:
diff --git a/poetry/core/packages/directory_dependency.py b/poetry/core/packages/directory_dependency.py
index d27375344..a9bf53cfd 100644
--- a/poetry/core/packages/directory_dependency.py
+++ b/poetry/core/packages/directory_dependency.py
@@ -79,3 +79,14 @@ def supports_poetry(self):
 
     def is_directory(self):
         return True
+
+    @property
+    def base_pep_508_name(self):  # type: () -> str
+        requirement = self.pretty_name
+
+        if self.extras:
+            requirement += "[{}]".format(",".join(self.extras))
+
+        requirement += " @ {}".format(str(self.path))
+
+        return requirement
diff --git a/poetry/core/packages/file_dependency.py b/poetry/core/packages/file_dependency.py
index aee85e6d5..8e7272c2e 100644
--- a/poetry/core/packages/file_dependency.py
+++ b/poetry/core/packages/file_dependency.py
@@ -4,6 +4,7 @@
 from poetry.core._vendor.pkginfo.distribution import HEADER_ATTRS
 from poetry.core._vendor.pkginfo.distribution import HEADER_ATTRS_2_0
 
+from poetry.core.packages.utils.utils import path_to_url
 from poetry.core.utils._compat import Path
 
 from .dependency import Dependency
@@ -59,3 +60,15 @@ def hash(self):
                 h.update(content)
 
         return h.hexdigest()
+
+    @property
+    def base_pep_508_name(self):  # type: () -> str
+        requirement = self.pretty_name
+
+        if self.extras:
+            requirement += "[{}]".format(",".join(self.extras))
+
+        path = path_to_url(self.path) if self.path.is_absolute() else self.path
+        requirement += " @ {}".format(path)
+
+        return requirement
diff --git a/poetry/core/packages/utils/utils.py b/poetry/core/packages/utils/utils.py
index 77ac0746e..c0fed5388 100644
--- a/poetry/core/packages/utils/utils.py
+++ b/poetry/core/packages/utils/utils.py
@@ -1,6 +1,13 @@
 import os
 import posixpath
 import re
+import sys
+
+from typing import Union
+
+from poetry.core._vendor.six.moves.urllib.parse import unquote  # noqa
+from poetry.core._vendor.six.moves.urllib.parse import urlsplit  # noqa
+from poetry.core._vendor.six.moves.urllib.request import url2pathname  # noqa
 
 from poetry.core.packages.constraints.constraint import Constraint
 from poetry.core.packages.constraints.multi_constraint import MultiConstraint
@@ -11,24 +18,13 @@
 from poetry.core.semver import VersionRange
 from poetry.core.semver import VersionUnion
 from poetry.core.semver import parse_constraint
+from poetry.core.utils._compat import Path
 from poetry.core.version.markers import BaseMarker
 from poetry.core.version.markers import MarkerUnion
 from poetry.core.version.markers import MultiMarker
 from poetry.core.version.markers import SingleMarker
 
 
-try:
-    import urllib.parse as urlparse
-except ImportError:
-    import urlparse
-
-
-try:
-    import urllib.request as urllib2
-except ImportError:
-    import urllib2
-
-
 BZ2_EXTENSIONS = (".tar.bz2", ".tbz")
 XZ_EXTENSIONS = (".tar.xz", ".txz", ".tlz", ".tar.lz", ".tar.lzma")
 ZIP_EXTENSIONS = (".zip", ".whl")
@@ -52,14 +48,38 @@
     pass
 
 
-def path_to_url(path):
+def path_to_url(path):  # type: (Union[str, Path]) -> str
     """
-    Convert a path to a file: URL.  The path will be made absolute and have
-    quoted path parts.
+    Convert a path to a file: URL.  The path will be made absolute unless otherwise
+    specified and have quoted path parts.
     """
-    path = os.path.normpath(os.path.abspath(path))
-    url = urlparse.urljoin("file:", urllib2.pathname2url(path))
-    return url
+    return Path(path).absolute().as_uri()
+
+
+def url_to_path(url):  # type: (str) -> Path
+    """
+    Convert an RFC8089 file URI to path.
+
+    The logic used here is borrowed from pip
+    https://github.com/pypa/pip/blob/4d1932fcdd1974c820ea60b3286984ebb0c3beaa/src/pip/_internal/utils/urls.py#L31
+    """
+    if not url.startswith("file:"):
+        raise ValueError("{} is not a valid file URI".format(url))
+
+    _, netloc, path, _, _ = urlsplit(url)
+
+    if not netloc or netloc == "localhost":
+        # According to RFC 8089, same as empty authority.
+        netloc = ""
+    elif netloc not in {".", ".."} and sys.platform == "win32":
+        # If we have a UNC path, prepend UNC share notation.
+        netloc = "\\\\" + netloc
+    else:
+        raise ValueError(
+            "non-local file URIs are not supported on this platform: {}".format(url)
+        )
+
+    return Path(url2pathname(netloc + unquote(path)))
 
 
 def is_url(name):
diff --git a/poetry/core/utils/_compat.py b/poetry/core/utils/_compat.py
index 9935e102b..4c96e1ce9 100644
--- a/poetry/core/utils/_compat.py
+++ b/poetry/core/utils/_compat.py
@@ -1,10 +1,10 @@
 import sys
 
+import poetry.core._vendor.six.moves.urllib.parse as urllib_parse
+
+
+urlparse = urllib_parse
 
-try:
-    import urllib.parse as urlparse
-except ImportError:
-    import urlparse
 
 try:  # Python 2
     long = long
diff --git a/poetry/core/version/requirements.py b/poetry/core/version/requirements.py
index d2748cb53..8b70b7860 100644
--- a/poetry/core/version/requirements.py
+++ b/poetry/core/version/requirements.py
@@ -216,10 +216,14 @@ def __init__(self, requirement_string):
         self.name = req.name
         if req.url:
             parsed_url = urlparse.urlparse(req.url)
-            if not (parsed_url.scheme and parsed_url.netloc) or (
-                not parsed_url.scheme and not parsed_url.netloc
-            ):
-                raise InvalidRequirement("Invalid URL given")
+            if parsed_url.scheme == "file":
+                if urlparse.urlunparse(parsed_url) != req.url:
+                    raise InvalidRequirement("Invalid URL given")
+            elif (
+                not (parsed_url.scheme and parsed_url.netloc)
+                or (not parsed_url.scheme and not parsed_url.netloc)
+            ) and not parsed_url.path:
+                raise InvalidRequirement("Invalid URL: {0}".format(req.url))
             self.url = req.url
         else:
             self.url = None
diff --git a/tests/masonry/builders/test_builder.py b/tests/masonry/builders/test_builder.py
index 39de435f1..2bf9f6f26 100644
--- a/tests/masonry/builders/test_builder.py
+++ b/tests/masonry/builders/test_builder.py
@@ -11,7 +11,7 @@ def test_builder_find_excluded_files(mocker):
     p.return_value = []
 
     builder = Builder(
-        Factory().create_poetry(Path(__file__).parent / "fixtures" / "complete"),
+        Factory().create_poetry(Path(__file__).parent / "fixtures" / "complete")
     )
 
     assert builder.find_excluded_files() == {"my_package/sub_pkg1/extra_file.xml"}
@@ -24,7 +24,7 @@ def test_builder_find_case_sensitive_excluded_files(mocker):
     builder = Builder(
         Factory().create_poetry(
             Path(__file__).parent / "fixtures" / "case_sensitive_exclusions"
-        ),
+        )
     )
 
     assert builder.find_excluded_files() == {
@@ -45,7 +45,7 @@ def test_builder_find_invalid_case_sensitive_excluded_files(mocker):
     builder = Builder(
         Factory().create_poetry(
             Path(__file__).parent / "fixtures" / "invalid_case_sensitive_exclusions"
-        ),
+        )
     )
 
     assert {"my_package/Bar/foo/bar/Foo.py"} == builder.find_excluded_files()
@@ -53,7 +53,7 @@ def test_builder_find_invalid_case_sensitive_excluded_files(mocker):
 
 def test_get_metadata_content():
     builder = Builder(
-        Factory().create_poetry(Path(__file__).parent / "fixtures" / "complete"),
+        Factory().create_poetry(Path(__file__).parent / "fixtures" / "complete")
     )
 
     metadata = builder.get_metadata_content()
@@ -103,7 +103,7 @@ def test_get_metadata_content():
 
 def test_metadata_homepage_default():
     builder = Builder(
-        Factory().create_poetry(Path(__file__).parent / "fixtures" / "simple_version"),
+        Factory().create_poetry(Path(__file__).parent / "fixtures" / "simple_version")
     )
 
     metadata = Parser().parsestr(builder.get_metadata_content())
@@ -115,7 +115,7 @@ def test_metadata_with_vcs_dependencies():
     builder = Builder(
         Factory().create_poetry(
             Path(__file__).parent / "fixtures" / "with_vcs_dependency"
-        ),
+        )
     )
 
     metadata = Parser().parsestr(builder.get_metadata_content())
@@ -129,7 +129,7 @@ def test_metadata_with_url_dependencies():
     builder = Builder(
         Factory().create_poetry(
             Path(__file__).parent / "fixtures" / "with_url_dependency"
-        ),
+        )
     )
 
     metadata = Parser().parsestr(builder.get_metadata_content())
diff --git a/tests/packages/test_directory_dependency.py b/tests/packages/test_directory_dependency.py
index f000aa725..522935da1 100644
--- a/tests/packages/test_directory_dependency.py
+++ b/tests/packages/test_directory_dependency.py
@@ -1,5 +1,6 @@
 import pytest
 
+from poetry.core.packages import dependency_from_pep_508
 from poetry.core.packages.directory_dependency import DirectoryDependency
 from poetry.core.utils._compat import Path
 
@@ -10,3 +11,47 @@
 def test_directory_dependency_must_exist():
     with pytest.raises(ValueError):
         DirectoryDependency("demo", DIST_PATH / "invalid")
+
+
+def _test_directory_dependency_pep_508(name, path, pep_508_input, pep_508_output=None):
+    dep = dependency_from_pep_508(pep_508_input, relative_to=Path(__file__).parent)
+
+    assert dep.is_directory()
+    assert dep.name == name
+    assert dep.path == path
+    assert dep.to_pep_508() == pep_508_output or pep_508_input
+
+
+def test_directory_dependency_pep_508_local_absolute():
+    path = (
+        Path(__file__).parent.parent
+        / "fixtures"
+        / "project_with_multi_constraints_dependency"
+    )
+    requirement = "{} @ file://{}".format("demo", path.as_posix())
+    _test_directory_dependency_pep_508("demo", path, requirement)
+
+    requirement = "{} @ {}".format("demo", path)
+    _test_directory_dependency_pep_508("demo", path, requirement)
+
+
+def test_directory_dependency_pep_508_localhost():
+    path = (
+        Path(__file__).parent.parent
+        / "fixtures"
+        / "project_with_multi_constraints_dependency"
+    )
+    requirement = "{} @ file://localhost{}".format("demo", path.as_posix())
+    requirement_expected = "{} @ file://{}".format("demo", path.as_posix())
+    _test_directory_dependency_pep_508("demo", path, requirement, requirement_expected)
+
+
+def test_directory_dependency_pep_508_local_relative():
+    path = Path("..") / "fixtures" / "project_with_multi_constraints_dependency"
+
+    with pytest.raises(ValueError):
+        requirement = "{} @ file://{}".format("demo", path.as_posix())
+        _test_directory_dependency_pep_508("demo", path, requirement)
+
+    requirement = "{} @ {}".format("demo", path)
+    _test_directory_dependency_pep_508("demo", path, requirement)
diff --git a/tests/packages/test_file_dependency.py b/tests/packages/test_file_dependency.py
index e80b35f9e..0dc5781be 100644
--- a/tests/packages/test_file_dependency.py
+++ b/tests/packages/test_file_dependency.py
@@ -1,6 +1,7 @@
 import pytest
 
 from poetry.core.packages import FileDependency
+from poetry.core.packages import dependency_from_pep_508
 from poetry.core.utils._compat import Path
 
 
@@ -15,3 +16,46 @@ def test_file_dependency_wrong_path():
 def test_file_dependency_dir():
     with pytest.raises(ValueError):
         FileDependency("demo", DIST_PATH)
+
+
+def _test_file_dependency_pep_508(
+    mocker, name, path, pep_508_input, pep_508_output=None
+):
+    mocker.patch.object(Path, "exists").return_value = True
+    mocker.patch.object(Path, "is_file").return_value = True
+
+    dep = dependency_from_pep_508(pep_508_input, relative_to=Path(__file__).parent)
+
+    assert dep.is_file()
+    assert dep.name == name
+    assert dep.path == path
+    assert dep.to_pep_508() == pep_508_output or pep_508_input
+
+
+def test_file_dependency_pep_508_local_file_absolute(mocker):
+    path = DIST_PATH / "demo-0.2.0.tar.gz"
+    requirement = "{} @ file://{}".format("demo", path.as_posix())
+    _test_file_dependency_pep_508(mocker, "demo", path, requirement)
+
+    requirement = "{} @ {}".format("demo", path)
+    _test_file_dependency_pep_508(mocker, "demo", path, requirement)
+
+
+def test_file_dependency_pep_508_local_file_localhost(mocker):
+    path = DIST_PATH / "demo-0.2.0.tar.gz"
+    requirement = "{} @ file://localhost{}".format("demo", path.as_posix())
+    requirement_expected = "{} @ file://{}".format("demo", path.as_posix())
+    _test_file_dependency_pep_508(
+        mocker, "demo", path, requirement, requirement_expected
+    )
+
+
+def test_file_dependency_pep_508_local_file_relative_path(mocker):
+    path = Path("..") / "fixtures" / "distributions" / "demo-0.2.0.tar.gz"
+
+    with pytest.raises(ValueError):
+        requirement = "{} @ file://{}".format("demo", path.as_posix())
+        _test_file_dependency_pep_508(mocker, "demo", path, requirement)
+
+    requirement = "{} @ {}".format("demo", path)
+    _test_file_dependency_pep_508(mocker, "demo", path, requirement)
diff --git a/tests/packages/utils/test_utils_urls.py b/tests/packages/utils/test_utils_urls.py
new file mode 100644
index 000000000..b96f63f34
--- /dev/null
+++ b/tests/packages/utils/test_utils_urls.py
@@ -0,0 +1,63 @@
+# These test scenarios are ported over from pypa/pip
+# https://raw.githubusercontent.com/pypa/pip/b447f438df08303f4f07f2598f190e73876443ba/tests/unit/test_urls.py
+
+import sys
+
+from poetry.core._vendor.six.moves.urllib.request import pathname2url  # noqa
+
+import pytest
+
+from poetry.core.packages import path_to_url
+from poetry.core.packages import url_to_path
+from poetry.core.utils._compat import Path
+
+
+@pytest.mark.skipif("sys.platform == 'win32'")
+def test_path_to_url_unix():
+    assert path_to_url("/tmp/file") == "file:///tmp/file"
+    path = Path(".") / "file"
+    assert path_to_url("file") == "file://" + path.absolute().as_posix()
+
+
+@pytest.mark.skipif("sys.platform != 'win32'")
+def test_path_to_url_win():
+    assert path_to_url("c:/tmp/file") == "file:///c:/tmp/file"
+    assert path_to_url("c:\\tmp\\file") == "file:///c:/tmp/file"
+    assert path_to_url(r"\\unc\as\path") == "file://unc/as/path"
+    path = Path(".") / "file"
+    assert path_to_url("file") == "file:///" + path.absolute().as_posix()
+
+
+@pytest.mark.parametrize(
+    "url,win_expected,non_win_expected",
+    [
+        ("file:tmp", "tmp", "tmp"),
+        ("file:c:/path/to/file", r"C:\path\to\file", "c:/path/to/file"),
+        ("file:/path/to/file", r"\path\to\file", "/path/to/file"),
+        ("file://localhost/tmp/file", r"\tmp\file", "/tmp/file"),
+        ("file://localhost/c:/tmp/file", r"C:\tmp\file", "/c:/tmp/file"),
+        ("file://somehost/tmp/file", r"\\somehost\tmp\file", None),
+        ("file:///tmp/file", r"\tmp\file", "/tmp/file"),
+        ("file:///c:/tmp/file", r"C:\tmp\file", "/c:/tmp/file"),
+    ],
+)
+def test_url_to_path(url, win_expected, non_win_expected):
+    if sys.platform == "win32":
+        expected_path = win_expected
+    else:
+        expected_path = non_win_expected
+
+    if expected_path is None:
+        with pytest.raises(ValueError):
+            url_to_path(url)
+    else:
+        assert url_to_path(url) == Path(expected_path)
+
+
+@pytest.mark.skipif("sys.platform != 'win32'")
+def test_url_to_path_path_to_url_symmetry_win():
+    path = r"C:\tmp\file"
+    assert url_to_path(path_to_url(path)) == Path(path)
+
+    unc_path = r"\\unc\share\path"
+    assert url_to_path(path_to_url(unc_path)) == Path(unc_path)