From f390aba22a83d128d2dff03dc9ab5f522268ac87 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Saugat=20Pachhai=20=28=E0=A4=B8=E0=A5=8C=E0=A4=97=E0=A4=BE?=
 =?UTF-8?q?=E0=A4=A4=29?= <suagatchhetri@outlook.com>
Date: Mon, 22 Mar 2021 22:08:32 +0545
Subject: [PATCH] plots: quote and resolve paths properly

This commit fixes 3 things:
1. The path printed on the terminal is properly is quoted.  This was
problematic when the output is piped to `xdg-open` if the path contained
spaces or quotes or special characters.

2. The path is resolved. This is not an issue at all, but makes the
output nicer.

3. The path with which the browser is opened with uses relative path.
This is done to solve issue in WSL, that the absolute path is not
accessible outside the WSL for host programs as the linux filesystem
is available on `/wsl$` path.
---
 dvc/command/plots.py             | 13 ++++++-----
 tests/func/plots/test_show.py    |  2 +-
 tests/unit/command/test_plots.py | 39 +++++++++++++++++++++++++++-----
 3 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/dvc/command/plots.py b/dvc/command/plots.py
index 8deb5aff2f..0e28162a11 100644
--- a/dvc/command/plots.py
+++ b/dvc/command/plots.py
@@ -1,6 +1,5 @@
 import argparse
 import logging
-import os
 
 from dvc.command import completion
 from dvc.command.base import CmdBase, append_doc_link, fix_subparsers
@@ -23,6 +22,8 @@ def _props(self):
         return {k: v for k, v in props.items() if v is not None}
 
     def run(self):
+        from pathlib import Path
+
         if self.args.show_vega:
             if not self.args.targets:
                 logger.error("please specify a target for `--show-vega`")
@@ -41,20 +42,20 @@ def run(self):
                 logger.info(plots[target])
                 return 0
 
-            path = self.args.out or "plots.html"
-            path = os.path.join(os.getcwd(), path)
-
+            rel: str = self.args.out or "plots.html"
+            path = (Path.cwd() / rel).resolve()
             write(path, plots)
-            url = f"file://{path}"
         except DvcException:
             logger.exception("")
             return 1
 
+        assert path.is_absolute()  # as_uri throws ValueError if not absolute
+        url = path.as_uri()
         logger.info(url)
         if self.args.open:
             import webbrowser
 
-            opened = webbrowser.open(url)
+            opened = webbrowser.open(rel)
             if not opened:
                 logger.error("Failed to open. Please try opening it manually.")
                 return 1
diff --git a/tests/func/plots/test_show.py b/tests/func/plots/test_show.py
index f7c378a25a..59efab8187 100644
--- a/tests/func/plots/test_show.py
+++ b/tests/func/plots/test_show.py
@@ -708,7 +708,7 @@ def test_show_from_subdir(tmp_dir, dvc, caplog):
     with subdir.chdir(), caplog.at_level(logging.INFO, "dvc"):
         assert main(["plots", "show", "metric.json"]) == 0
 
-    assert f"file://{str(subdir)}" in caplog.text
+    assert subdir.as_uri() in caplog.text
     assert (subdir / "plots.html").exists()
 
 
diff --git a/tests/unit/command/test_plots.py b/tests/unit/command/test_plots.py
index a2f5b1cc60..510d699b1d 100644
--- a/tests/unit/command/test_plots.py
+++ b/tests/unit/command/test_plots.py
@@ -1,4 +1,8 @@
 import logging
+import os
+import posixpath
+
+import pytest
 
 from dvc.cli import parse_args
 from dvc.command.plots import CmdPlotsDiff, CmdPlotsShow
@@ -116,12 +120,11 @@ def test_plots_diff_open(tmp_dir, dvc, mocker, caplog):
     )
 
     assert cmd.run() == 0
+    mocked_open.assert_called_once_with("plots.html")
 
-    expected_url = f"file://{tmp_dir / 'plots.html'}"
+    expected_url = posixpath.join(tmp_dir.as_uri(), "plots.html")
     assert expected_url in caplog.text
 
-    mocked_open.assert_called_once_with(expected_url)
-
 
 def test_plots_diff_open_failed(tmp_dir, dvc, mocker, caplog):
     mocked_open = mocker.patch("webbrowser.open", return_value=False)
@@ -132,12 +135,36 @@ def test_plots_diff_open_failed(tmp_dir, dvc, mocker, caplog):
     )
 
     assert cmd.run() == 1
-
-    expected_url = f"file://{tmp_dir / 'plots.html'}"
-    mocked_open.assert_called_once_with(expected_url)
+    mocked_open.assert_called_once_with("plots.html")
 
     error_message = "Failed to open. Please try opening it manually."
+    expected_url = posixpath.join(tmp_dir.as_uri(), "plots.html")
+
     assert caplog.record_tuples == [
         ("dvc.command.plots", logging.INFO, expected_url),
         ("dvc.command.plots", logging.ERROR, error_message),
     ]
+
+
+@pytest.mark.parametrize(
+    "output, expected_url_path",
+    [
+        ("plots file with spaces.html", "plots%20file%20with%20spaces.html"),
+        (os.path.join("dir", "..", "plots.html"), "plots.html"),
+    ],
+    ids=["quote", "resolve"],
+)
+def test_plots_path_is_quoted_and_resolved_properly(
+    tmp_dir, dvc, mocker, caplog, output, expected_url_path
+):
+    cli_args = parse_args(
+        ["plots", "diff", "--targets", "datafile", "--out", output]
+    )
+    cmd = cli_args.func(cli_args)
+    mocker.patch(
+        "dvc.repo.plots.diff.diff", return_value={"datafile": "filledtemplate"}
+    )
+
+    assert cmd.run() == 0
+    expected_url = posixpath.join(tmp_dir.as_uri(), expected_url_path)
+    assert expected_url in caplog.text