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

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

Merged

Conversation

sushain97
Copy link
Contributor

@sushain97 sushain97 commented May 11, 2024

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 sushain97 changed the title Support special characters in library paths fix(server): support special characters in library paths May 11, 2024
@@ -186,7 +186,8 @@ export class StorageRepository implements IStorageRepository {
}

private asGlob(pathsToCrawl: string[]): string {
const base = pathsToCrawl.length === 1 ? pathsToCrawl[0] : `{${pathsToCrawl.join(',')}}`;
const escapedPaths = pathsToCrawl.map((path) => escapePath(path));
const base = escapedPaths.length === 1 ? escapedPaths[0] : `{${escapedPaths.join(',')}}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code feels a bit redundant since Array.join should work fine on a singleton array. I just left it as is but can tweak.

@sushain97
Copy link
Contributor Author

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

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to just add a note to the docs explaining how to escape special characters, rather than removing the "glob-matching" feature altogether for every user.

@sushain97
Copy link
Contributor Author

sushain97 commented May 13, 2024

@jrasm91 I don't think that feature really works since invalid paths are rejected:

const stat = await this.storageRepository.stat(importPath);
.

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 sushain97 requested a review from jrasm91 May 13, 2024 18:27
Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@sushain97
Copy link
Contributor Author

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.

@jrasm91 jrasm91 enabled auto-merge (squash) May 13, 2024 21:44
@jrasm91 jrasm91 merged commit 4e6aeed into immich-app:main May 13, 2024
23 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants