Extra spaces in File backend's media_dirs setting are not ignored

[kingosticks]

Originally reported at https://discourse.mopidy.com/t/problems-accessing-files-via-nfs/4637/7

The following config results in the warning:

WARNING [MainThread] mopidy.file.library /music/Network/flac is not a directory.

[file]
media_dirs = /music/Network/flac | NAS

This happens because we don't strip whitespace from each item so we end up trying to use the non-existent path "/music/Network/flac ". This problem is not obvious from the warning message.

We should strip whitespace and we should probably also use .as_uri() when displaying the path, this would have made it more obvious:

WARNING [MainThread] mopidy.file.library file:///music/Network/flac%20 is not a directory.

[djmattyg007]

I don't think we should be making any assumptions about pathnames specified by the user. If they legitimately had a path that ended in a space character, they'd never be able to sensibly use it.

Representing the path better in error messages is definitely a good idea though.

[kingosticks]

I didn't try it but our Path config type will strip() the value so I figured this should do the same. Which would mean you probably can't currently set a path that ends in a space in a Mopidy config file. I don't disagree with your point but I can imagine accidental whitespace at the end of lines in config files is far more common than paths that end with spaces. So this might actually be intentional, unsure.

[djmattyg007]

That makes sense.

As part of thinking about #1966, I realised my configuration handling improvements idea could be utilised in the File extension too. Extending from the example I provided on that issue:

class Pair(ConfigValue):
    def __init__(self, optional=False, optional_pair=False, separator="|", subtypes=None):
        self._required = not optional
        self._optional_pair = optional_pair
        self._separator = separator
        if subtype:
            self._subtypes = subtype
        else:
            self._subtypes = (String(), String())

    def deserialize(self, value):
        raw_value = decode(value).strip()
        validators.validate_required(raw_value, self._required)

        if self._separator in raw_value:
            value = value.split(self._separator, 1)
        elif self._optional_pair:
            value = (raw_value, raw_value)
        else:
            raise ValueError("must have separator")

        return (self._subtypes[0].deserialize(value[0]), self._subtypes[1].deserialize(value[1]))

    def serialize(self, value, display=False):
        return "{0}{1}{2}".format(
            self._subtypes[0].serialize(value, display),
            self._separator,
            self._subtypes[1].serialize(value, display),
        )

You could then compose this like so for the File extension:

from mopidy import config
import os

schema["media_dirs"] = config.List(
    optional=True,
    subtype=config.Pair(
        optional=False,
        optional_pair=True,
        subtypes=(
            config.Path(),
            config.String(transformer=lambda x: x.replace(os.sep, "+")),
        ),
    ),
)

This removes the need to handle any of this inside the File extension's actor code, and ensures the configuration is fully validated before Mopidy even starts.

[djmattyg007]

As an aside, the code sample above unconditionally replaces os.sep with "+". This differs from what Mopidy currently does - path separators are only substituted with + if we're re-using the real file path as the path's label. I wasn't able to think of a reason why path labels couldn't have slashes in them, so I think it makes sense to perform the substitution unconditionally.

Reference: https://github.com/mopidy/mopidy/blob/develop/mopidy/file/library.py#L133

[abidahmadq]

Is this issue still open ? I can work on it if someone provides me with a direction. I am a new contributor.

[kingosticks]

Yes. I think the original post has the direction but please ask if you have a specific question.

[djmattyg007]

@kingosticks What do you think about my slightly larger proposal of making the configuration parsing system more powerful?

[kingosticks]

Sorry, yes, since File is bundled with Mopidy that direction is also fine. It's just more complicated in both the fix and the test. If you are happy to help/review what @Abid1998 comes up with then that sounds good.

[abidahmadq]

@kingosticks @djmattyg007
From what i understand i have to make changes in these files ?
https://github.com/mopidy/mopidy/blob/HEAD/mopidy/file/__init__.py
https://github.com/mopidy/mopidy/blob/HEAD/mopidy/file/library.py
So should i strip of spaces from right side before | character ? or should i display an helpful error message if there is a space?

[djmattyg007]

@Abid1998 You're going to want to start by making changes to this file:

https://github.com/mopidy/mopidy/blob/develop/mopidy/config/types.py

Specifically:

• Update the String and List types, as documented in Make it easier to re-use CSRF protection for custom web handlers #1966
• Implement the Pair type as mentioned above

You should also add tests for this new functionality here:

https://github.com/mopidy/mopidy/blob/develop/tests/config/test_types.py

Once that's done, you can update the File extension. The config definition in __init__.py should be updated along the lines of my example above. You should then be able to drastically simplify the media directory parsing code in library.py.

Please let us know if you need more help or advice, or if anything I've written doesn't make sense.

[abidahmadq]

@djmattyg007 Thanks alot for the clarifications I will create a draft PR and tag you there.

[djmattyg007]

@Abid1998 How are you going with these updates? Do you require any assistance?