fix: improve URL path handling in local storage for special characters #4378
Conversation
Enhanced the local storage implementation to properly handle URL-decoded paths and fix issues with file paths containing special characters. Added decodedPath field to localStorage struct to separate URL parsing concerns from file system operations. Key changes: - Added decodedPath field to localStorage struct for proper URL decoding - Modified newLocalStorage to use decoded path instead of modifying original URL - Fixed Windows path handling to work with decoded paths - Improved URL escaping in storage.For() to handle special characters - Added comprehensive test suite covering URL decoding, symlink resolution, Windows paths, and edge cases - Refactored test extractor to use mockTestExtractor for better isolation This ensures that file paths with spaces, hash symbols, and other special characters are handled correctly throughout the storage system. Signed-off-by: Deluan <deluan@navidrome.org>
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes URL path handling in the local storage system to properly handle file paths containing special characters like hash symbols (#), spaces, and other URL-reserved characters. The fix addresses "Server communication error: Internal Server Error" when accessing file information for files with special characters in their paths.
Key changes implemented:
- Added proper URL encoding/decoding in the storage path handling system
- Separated URL path concerns by introducing a
decodedPathfield for file system operations - Enhanced Windows path handling to work correctly with decoded paths
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| core/storage/storage.go | Enhanced URL path escaping logic in the For() function to properly encode path components |
| core/storage/local/local.go | Added decodedPath field and updated path handling to separate URL parsing from file system operations |
| core/storage/local/local_test.go | Added comprehensive test suite covering URL decoding, symlink resolution, Windows paths, and edge cases |
| core/storage/local/local_suite_test.go | Updated test suite initialization with proper logging configuration |
| core/storage/storage_test.go | Added table-driven tests for paths with various special characters |
Comments suppressed due to low confidence (3)
core/storage/storage.go:47
- The URL construction is missing a slash after the scheme. It should be
LocalSchemaID + "://" + strings.Join(escapedParts, "/")to match the expectedfile:///pathformat for local file URLs.
uri = LocalSchemaID + ":///" + strings.Join(escapedParts, "/")
core/storage/storage_test.go:74
- The test comment suggests the path should be exactly the same as input, but this assertion may not hold true for all special characters after URL encoding/decoding cycles. The test should verify the actual behavior rather than assume the path remains unchanged.
// The path should be exactly the same as the input - after URL parsing it gets decoded back
core/storage/local/local_test.go:17
- [nitpick] Setting log level to Fatal in tests may suppress important warning or error messages that could be useful for debugging test failures. Consider using a less restrictive log level or making it configurable.
)
There was a problem hiding this comment.
Summary of Changes
Hello @deluan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses critical issues related to handling file paths containing special characters, particularly hash symbols, within the local storage system. My changes refine how URL-encoded paths are processed and constructed, ensuring that file system operations correctly identify and access files. This resolves 'Server communication error' when retrieving file information and prevents potential album grouping discrepancies.
Highlights
- Enhanced URL Path Handling: I've introduced a
decodedPathfield in thelocalStoragestruct to explicitly store the URL-decoded file path. This ensures that all subsequent file system operations, such as symlink resolution (filepath.EvalSymlinks) and directory file system creation (os.DirFS), consistently use the correctly decoded path, resolving issues with special characters. - Improved URL Construction for Local Paths: The
storage.Forfunction has been updated to properly URL-escape individual path components when converting a plain file path into afile://URI. This prevents incorrect interpretation of special characters (like#or spaces) when the URI is later parsed, ensuring the original path is accurately represented. - Comprehensive Test Coverage: I've added a new, extensive test suite (
local_test.go) for thelocalstorage package. These tests cover various scenarios, including correct URL decoding, symlink resolution, handling of Windows paths, and robust metadata extraction, significantly improving the reliability and confidence in the path handling logic. - Windows Path Compatibility Fixes: The logic for handling Windows drive letters and paths within
newLocalStoragehas been refined. This ensures that local storage paths are correctly joined and resolved on Windows systems, addressing potential issues with file access and information retrieval.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request addresses issues with special characters in file paths by introducing URL encoding and decoding. The decodedPath field in localStorage clarifies the separation between URL paths and filesystem paths. The test suite is thorough.
However, there's a potential regression in storage.go that could affect schemaless path handling on Windows. Additionally, there's a comment on a test case in local_test.go that might be a duplicate.
Addressing the Windows path issue will enhance the codebase.
Signed-off-by: Deluan <deluan@navidrome.org>
Signed-off-by: Deluan <deluan@navidrome.org>
Signed-off-by: Deluan <deluan@navidrome.org>
Description
This pull request fixes issues with file paths containing special characters (especially hash symbols
#) by improving URL path handling in the local storage system. The issue was causing "Server communication error: Internal Server Error" when clicking "Get Info" on files with special characters in their paths.Related Issues
Fixes #4377
Type of Change
Checklist
Please review and check all that apply:
How to Test
#, spaces)Screenshots / Demos (if applicable)
N/A - This is a backend fix for path handling.
Additional Notes
Key Changes:
localStorageto handle special characters in file pathsdecodedPathfield to separate URL parsing from file system operationstests.Initcall per project testing standardsThe fix ensures that file paths with spaces, hash symbols, and other special characters are handled correctly throughout the storage system, resolving both the "Get Info" error and potential album grouping issues mentioned in the issue.
Technical Details:
The root cause was that URL-encoded paths (like
%23for#) were not being properly decoded before file system operations, causing the system to look for files with encoded characters in their actual file names. This fix properly decodes URLs while maintaining the original URL structure for other operations.