search

immich-app / immich

High performance self-hosted photo and video management solution.
https://immich.app
GNU Affero General Public License v3.0
34.19k stars 1.62k forks source link

fix(server): support special characters in library paths #9385

Closed sushain97 closed 2 weeks ago

sushain97 commented 2 weeks ago

Directories with special characters, like (, aren't properly crawled. This is fixed by escaping the provided paths before they're passed to fast-glob. My newly added test now passes. But, another test that uses * had to be deleted since we can't treat input paths as patterns and verbatim at the same time. I audited the use cases and it seems like only actual paths would be passed (please double check!). In particular, library paths are validated as real paths before being crawled.

If this behavior is intentional, the docs/UI should probably clarify that users need to escape special characters themselves. The path validation logic would also need to be tweaked to 'unescape' before checking the path. That seems a bit confusing.

cc @jrasm91 since you recently touched this and have last blame on the test that I deleted :)

sushain97 commented 2 weeks ago

@danieldietzler thanks for the review! would it be possible to get this merged before the next release?

sushain97 commented 2 weeks ago

@jrasm91 I don't think that feature really works since invalid paths are rejected: https://github.com/immich-app/immich/blob/540e568e9d29abbf4306c4ff761614bb5cc93291/server/src/services/library.service.ts#L315.

image

I'd argue the current behavior is a regression rather than a feature :) External libraries with ( used to work and now all their assets are marked offline.

sushain97 commented 2 weeks ago

Oh, I see. Maybe it does make more sense then for the import paths to just point to an existing folder (no glob syntax) and only the exclude paths use special characters (glob syntax).

That seems like reasonable behavior and this PR will preserve it since the exclusion patterns are still passed to fast-glob verbatim.

I could see a future feature that lets you use globs on an per path opt-in basis. Not sure it'd be a common ask, though.