Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

changed validate filename to use auto operating system detection. #2478

Open
wants to merge 4 commits into
base: nightly
Choose a base branch
from

Conversation

dextrous0z
Copy link

@dextrous0z dextrous0z commented Jan 24, 2025

Description

Added a change to the function is_valid_filepath function to check that the filepath is compatible based on the operating system. allowing support for a wider range of possible filenames.

References the issue #2477
docs

  • [] Bug fix - Supporting more filename types based on operating system,

@YozoraXCII
Copy link
Contributor

Without much knowledge on this aspect of Kometa - if people are using asset folders etc, will this create issue where files with : characters are now treated as valid and thus a new asset folder will be created?

@chazlarson
Copy link
Contributor

chazlarson commented Jan 24, 2025

Without much knowledge on this aspect of Kometa - if people are using asset folders etc, will this create issue where files with : characters are now treated as valid and thus a new asset folder will be created?

Yes, it would break (some) existing setups.

It also potentially breaks all third-party scripts that read from or add to an asset directory.

I could see adding this with a toggle to turn it on if someone wants this behavior.

@chazlarson
Copy link
Contributor

What's the use case where this is important enough to risk so much breakage? The only one I can think of is where one wants to use the media directories as asset directories, and one's media folders contain "illegal" characters.

@YozoraXCII
Copy link
Contributor

@dextrous0z can you amend this PR so that it's a toggle (perhaps something within the settings section of config.yml?) with the default behavior being disabled.

@dextrous0z
Copy link
Author

@YozoraXCII
Been giving this some thought and yes @chazlarson is right this could potentially break some existing setups my current use case for this is I've currently started using a fscorrupt/Posterizarr setup and I noticed whilst the asset directories existed some where getting skipped by Kometa as the folder name determined after sanitizing is different to the actual Plex name.
As my plex folders contain a bunch of characters that probably won't be valid on windows.

I'm thinking the default behavior will be how it currently originally is and adding a config within the "config.yml" that allows users to set the option based on the operating system for example the spec lists the below as valid:

    "Linux"

    "Windows"

    "macOS"

    "auto"

    "POSIX": POSIX-compliant systems (Linux, macOS, etc.)

    "universal"/None

However if this is run in Docker I'm guessing that auto will always default to linux as this is built on alpine. So allowing a user to pick is a better option.

@chazlarson
Copy link
Contributor

As someone who maintains one of those third-party scripts that works with the asset directory, I'm not crazy about this complication, but it's not my decision.

@meisnate12
Copy link
Member

im also not crazy about this complication but while its being discussed there also needs to be validation on the input. Use the test_list parameter of the check_for_attribute method which takes a dictionary where the key is the attribute to validate and the value is a description. see run_order, sync_mode, or overlay_artwork_filetype for reference.

@meisnate12
Copy link
Member

also please specifiy the attribute platform when calling sanitize_filename and is_valid_filename right now you're passing the operating_system variable to sanitize_filename as replacement_text not platform

@planetrocky
Copy link

@YozoraXCII Been giving this some thought and yes @chazlarson is right this could potentially break some existing setups my current use case for this is I've currently started using a fscorrupt/Posterizarr setup and I noticed whilst the asset directories existed some where getting skipped by Kometa as the folder name determined after sanitizing is different to the actual Plex name. As my plex folders contain a bunch of characters that probably won't be valid on windows.

I'm thinking the default behavior will be how it currently originally is and adding a config within the "config.yml" that allows users to set the option based on the operating system for example the spec lists the below as valid:

    "Linux"

    "Windows"

    "macOS"

    "auto"

    "POSIX": POSIX-compliant systems (Linux, macOS, etc.)

    "universal"/None

However if this is run in Docker I'm guessing that auto will always default to linux as this is built on alpine. So allowing a user to pick is a better option.

Python really only supports two OSes:

  • nt = Windows
  • posix = Unix
isWindows = os.name == 'nt'

os.name will only return one of 'nt' or 'posix' for Python 3.8 ... 3.13. Python 2.7 will still return 'nt' or 'posix', and some other dead platforms. Python 3.0 you will never get, but has 'ce' and 'riscos'

platform.system() returns many aliases and the platform library is for detailed system information.

https://docs.python.org/3/library/os.html#os.name

os.name
The name of the operating system dependent module imported. The following names have currently been registered: 'posix', 'nt', 'java'.

BTW - the Python docs says java, but in the Python source code only nt and posix are returned!

@planetrocky
Copy link

Without much knowledge on this aspect of Kometa - if people are using asset folders etc, will this create issue where files with : characters are now treated as valid and thus a new asset folder will be created?

Yes, it would break (some) existing setups.

It also potentially breaks all third-party scripts that read from or add to an asset directory.

I could see adding this with a toggle to turn it on if someone wants this behavior.

Definitely not keen here on breaking changes which affect the very core of Kometa, the asset folders etc.

@dextrous0z #2477 doesn't contain much information. What OS and file-system are you using for your setup. Personally I'd like to see #2477 bug report with more details added so that we can review the breaking-change requested here in #2478

@dextrous0z
Copy link
Author

@planetrocky I'm running Kometa on Unraid Docker, the file system is brfs. I can provide log files if it helps.

Between the two different logs files, the only thing you'll see however is what is mentioned in the original issue #2477
Which is a bunch of missing Asset Folder lines.

I don't mind making changes to the pull request. If you believe the approach is wrong just tell me how'd you like to see this resolved ?

For example we can just say Kometa will never allow asset folders not following a Universal layout. Then no action is needed and I can just fix my library folders and rebuild plex.

Or we,
We can go the route of using python methods to determine the operating system running the program, which would IMO be the cleaner route.

My original concern with not doing this was due to Windows Docker, which WILL most likely cause issues, as the operating system will be deemed as Linux, and in turn use Linux file validation.

@meisnate12 First time contributing to the code missed that one, added in the latest commit.

Also will do the documentation if you want just really don't understand how to see an accurate preview of the .md files correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants