Improve handling of "media_dirs" file extension config

This moves almost all of the processing for the "media_dirs" file setting
into the config system, with the aim of making it more robust while also
resolving some bugs. The only thing left for the file library backend to
do manually now is verify that the input directories actually exist as
folders.

The main bug this solves is an issue of whitespace handling around
paths, as detailed in #1965.

To make this possible, the Path config type has been altered to return
pathlib objects rather than strings.

mopidy/config/types.py

@@ -1,10 +1,20 @@
 import logging
+import os
+import pathlib
 import re
 import socket
+from typing import Type
 
 from mopidy.config import validators
 from mopidy.internal import log, path
 
+_is_windows = os.name == "nt"
+_ExpandedPathBase: Type[pathlib.Path]
+if _is_windows:
+    _ExpandedPathBase = pathlib.WindowsPath
+else:
+    _ExpandedPathBase = pathlib.PosixPath
+
 
 def decode(value):
     if isinstance(value, bytes):
@@ -411,9 +421,14 @@ def __init__(self, choices=None, optional=False):
         )
 
 
-# Keep this for backwards compatibility
-class _ExpandedPath(_TransformedValue):
-    pass
+# Necessary to ignore type error here, because mypy won't accept a dynamic
+# conditional base class.
+class _ExpandedPath(_ExpandedPathBase):  # type: ignore
+    def __new__(cls, original: str, transformed: pathlib.Path):
+        return super().__new__(cls, transformed)
+
+    def __init__(self, original: str, transformed: pathlib.Path):
+        self.original = original
 
 
 class Path(ConfigValue):

mopidy/file/__init__.py

@@ -19,7 +19,20 @@ def get_default_config(self):
 
     def get_config_schema(self):
         schema = super().get_config_schema()
-        schema["media_dirs"] = config.List(optional=True)
+        schema["media_dirs"] = config.List(
+            optional=True,
+            subtype=config.Pair(
+                optional=False,
+                optional_pair=True,
+                subtypes=(
+                    config.Path(),
+                    config.String(
+                        # TODO Mpd client should accept / in dir name
+                        transformer=lambda value: value.replace(os.sep, "+")
+                    ),
+                ),
+            ),
+        )
         schema["excluded_file_extensions"] = config.List(optional=True)
         schema["show_dotfiles"] = config.Boolean(optional=True)
         schema["follow_symlinks"] = config.Boolean(optional=True)

mopidy/file/library.py

@@ -1,5 +1,4 @@
 import logging
-import os
 
 from mopidy import backend, exceptions, models
 from mopidy.audio import scan, tags
@@ -109,33 +108,26 @@ def lookup(self, uri):
         return [track]
 
     def _get_media_dirs(self, config):
-        for entry in config["file"]["media_dirs"]:
-            media_dir = {}
-            media_dir_split = entry.split("|", 1)
-            local_path = path.expand_path(media_dir_split[0])
-
+        for local_path, name in config["file"]["media_dirs"]:
             if local_path is None:
                 logger.debug(
                     "Failed expanding path (%s) from file/media_dirs config "
                     "value.",
-                    media_dir_split[0],
+                    local_path.original,
                 )
                 continue
             elif not local_path.is_dir():
                 logger.warning(
                     "%s is not a directory. Please create the directory or "
                     "update the file/media_dirs config value.",
-                    local_path,
+                    local_path.original,
                 )
                 continue
 
-            media_dir["path"] = local_path
-            if len(media_dir_split) == 2:
-                media_dir["name"] = media_dir_split[1]
-            else:
-                # TODO Mpd client should accept / in dir name
-                media_dir["name"] = media_dir_split[0].replace(os.sep, "+")
-
+            media_dir = {
+                "path": local_path,
+                "name": name,
+            }
             yield media_dir
 
     def _get_media_dirs_refs(self):

tests/config/test_types.py

@@ -1,6 +1,8 @@
 import logging
+import pathlib
 import re
 import socket
+import sys
 from unittest import mock
 
 import pytest
@@ -9,6 +11,19 @@
 from mopidy.internal import log
 
 
+@pytest.fixture(autouse=True)
+def patch_magic_mock(monkeypatch):
+    if sys.version_info < (3, 8):
+        # Polyfill MagicMock.__fspath__ for Python 3.7
+        # https://bugs.python.org/issue35022
+        def fspath(self):
+            typename = type(self).__name__
+            mockname = self._extract_mock_name()
+            return f"{typename}/{mockname}/{id(self)}"
+
+        monkeypatch.setattr(mock.MagicMock, "__fspath__", fspath, raising=False)
+
+
 @pytest.mark.parametrize(
     "value, expected",
     [
@@ -653,7 +668,7 @@ def test_deserialize_with_custom_subtypes(self):
 
         cv = types.Pair(subtypes=(types.Path(), types.String()))
         result = cv.deserialize("/dev/null | empty")
-        assert result == ("/dev/null", "empty")
+        assert result == (pathlib.Path("/dev/null"), "empty")
 
         with mock.patch("socket.getaddrinfo") as getaddrinfo_mock:
             cv = types.Pair(subtypes=(types.Hostname(), types.Port()))
@@ -695,7 +710,7 @@ def test_deserialize_with_custom_subtypes_respects_optional_separator(self):
             optional_pair=True, subtypes=(types.Path(), types.String())
         )
         result = cv.deserialize("/dev/null")
-        assert result == ("/dev/null", "/dev/null")
+        assert result == (pathlib.Path("/dev/null"), "/dev/null")
 
         cv = types.Pair(
             optional_pair=True, subtypes=(types.Port(), types.Port())
@@ -1341,12 +1356,14 @@ def test_deserialize_respects_optional(self, getaddrinfo_mock):
         assert cv.deserialize(" ") is None
         assert getaddrinfo_mock.call_count == 0
 
-    @mock.patch("mopidy.internal.path.expand_path")
-    def test_deserialize_with_unix_socket(self, expand_path_mock):
+    def test_deserialize_with_unix_socket(self):
         cv = types.Hostname()
 
-        assert cv.deserialize("unix:/tmp/mopidy.socket") is not None
-        expand_path_mock.assert_called_once_with("/tmp/mopidy.socket")
+        with mock.patch(
+            "mopidy.internal.path.expand_path",
+        ) as expand_path_mock:
+            assert cv.deserialize("unix:/tmp/mopidy.socket") is not None
+            expand_path_mock.assert_called_once_with("/tmp/mopidy.socket")
 
 
 class TestPort:
@@ -1377,11 +1394,13 @@ def test_invalid_ports(self):
 
 class TestExpandedPath:
     def test_is_str(self):
-        assert isinstance(types._ExpandedPath(b"/tmp", b"foo"), str)
+        assert isinstance(
+            types._ExpandedPath("/tmp", pathlib.Path("/tmp")), pathlib.Path
+        )
 
     def test_stores_both_expanded_and_original_path(self):
         original = "~"
-        expanded = "expanded_path"
+        expanded = pathlib.Path("expanded_path")
 
         result = types._ExpandedPath(original, expanded)
 
@@ -1393,34 +1412,36 @@ class TestPath:
     def test_deserialize_conversion_success(self):
         cv = types.Path()
 
-        result = cv.deserialize(b"/foo")
+        result = cv.deserialize("/foo")
 
-        assert result == "/foo"
+        assert result == pathlib.Path("/foo")
         assert isinstance(result, types._ExpandedPath)
-        assert isinstance(result, str)
+        assert isinstance(result, pathlib.Path)
 
     def test_deserialize_enforces_required(self):
         cv = types.Path()
 
         with pytest.raises(ValueError):
-            cv.deserialize(b"")
+            cv.deserialize("")
 
     def test_deserialize_respects_optional(self):
         cv = types.Path(optional=True)
 
-        assert cv.deserialize(b"") is None
-        assert cv.deserialize(b" ") is None
+        assert cv.deserialize("") is None
+        assert cv.deserialize(" ") is None
 
     def test_serialize_uses_original(self):
         cv = types.Path()
-        path = types._ExpandedPath("original_path", "expanded_path")
+        path = types._ExpandedPath(
+            "original_path", pathlib.Path("expanded_path")
+        )
 
         assert cv.serialize(path) == "original_path"
 
     def test_serialize_plain_string(self):
         cv = types.Path()
 
-        assert cv.serialize(b"path") == "path"
+        assert cv.serialize("path") == "path"
 
     def test_serialize_supports_unicode_string(self):
         cv = types.Path()

tests/file/test_browse.py

@@ -11,11 +11,12 @@
 
 @pytest.fixture
 def config():
+    media_dir = (path_to_data_dir(""), "Test Media")
     return {
         "proxy": {},
         "file": {
             "show_dotfiles": False,
-            "media_dirs": [str(path_to_data_dir(""))],
+            "media_dirs": (media_dir,),
             "excluded_file_extensions": [],
             "follow_symlinks": False,
             "metadata_timeout": 1000,

tests/file/test_lookup.py

@@ -14,7 +14,7 @@ def config():
         "proxy": {},
         "file": {
             "show_dotfiles": False,
-            "media_dirs": [],
+            "media_dirs": tuple(),
             "excluded_file_extensions": [],
             "follow_symlinks": False,
             "metadata_timeout": 1000,