Conversation
os.path.normcase() in normalize_path() lowercases the entire path on Windows (a no-op on POSIX), which cascaded into guess_file_name() and corrupted the derived entity name (e.g. File_Name.tsv -> file_name.tsv) while the FileHandle fileName kept its case. It also lowercased downloaded files on disk. Remove normcase() from normalize_path() so casing is preserved on every platform, and move case-insensitive cache matching into a dedicated _match_cache_map_key() helper in cache.py. The helper prefers an exact (case-sensitive) match -- correct for case-sensitive NTFS directories -- and falls back to an os.path.normcase() comparison so cache entries written by older clients with lowercased keys still resolve. This avoids forcing Windows users to re-download already-cached files. Cache.add() now migrates a legacy lowercased key to the case-preserving key instead of accumulating duplicates. Adds regression tests for case-preserving normalize_path/guess_file_name and for the case-tolerant cache matching, add-migration, and legacy lowercased key lookup.
…g-v1bujx-wbf2ka [SYNR-1534] Preserve file name casing on Windows uploads/downloads
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates path normalization and cache key handling to preserve original file name casing during upload/download workflows (especially on Windows), while maintaining compatibility with legacy cache entries.
Changes:
- Updated
normalize_path()to stop lowercasing paths (removesos.path.normcase) while still normalizing to absolute paths with forward slashes. - Added cache-lookup logic to prefer exact matches and fall back to
os.path.normcasematching for legacy (lowercased) Windows cache keys, plus migration behavior onadd(). - Added unit tests covering case preservation and legacy cache-key compatibility, and updated internal documentation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
synapseclient/core/utils.py |
Preserves path casing in normalize_path(); impacts downstream path comparisons. |
synapseclient/core/cache.py |
Adds case-aware cache key matching, legacy fallback behavior, and key migration logic. |
tests/unit/synapseclient/core/unit_test_utils.py |
Adds tests ensuring path/name casing is preserved. |
tests/unit/synapseclient/core/unit_test_Cache.py |
Adds tests for legacy lowercased cache keys and migration behavior. |
synapseclient/core/CLAUDE.md |
Documents the updated path/caching behavior and rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+462
to
+466
| matched_key = _match_cache_map_key(cache_map, path) | ||
| if matched_key is not None: | ||
| if delete is True and os.path.exists(matched_key): | ||
| os.remove(matched_key) | ||
| del cache_map[matched_key] |
Comment on lines
+407
to
+409
| existing_key = _match_cache_map_key(cache_map, path) | ||
| if existing_key is not None and existing_key != path: | ||
| del cache_map[existing_key] |
Comment on lines
+404
to
+410
| Note: this intentionally does NOT use os.path.normcase(). On Windows | ||
| normcase() lowercases the entire path, which corrupted derived entity names | ||
| (SYNR-1534) and would rename downloaded files on disk. os.path.abspath() | ||
| already normalizes separators to the OS default; the re.sub then converts | ||
| them to forward slashes. Case-insensitive matching for the local file cache | ||
| is handled separately in core/cache.py so that this function can preserve | ||
| casing without breaking cache lookups on case-insensitive Windows volumes. |
Comment on lines
+86
to
+94
| An exact (case-sensitive) match is always preferred, which keeps the cache | ||
| correct on case-sensitive filesystems -- including case-sensitive | ||
| directories on Windows/NTFS. When there is no exact match we fall back to a | ||
| case-insensitive (``os.path.normcase``) comparison. This lets cache entries | ||
| written by older clients -- which lowercased their keys via | ||
| ``os.path.normcase`` on Windows -- continue to be found, so preserving path | ||
| casing in ``utils.normalize_path`` does not force a re-download of already | ||
| cached files. On POSIX ``os.path.normcase`` is a no-op, so the fallback is | ||
| equivalent to the exact match and behavior is unchanged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem:
The current normalize_path function lowercases the entire file path on Windows during data transfer, which alters the original path casing.
#Solution
Update normalize_path to preserve the original path casing instead of converting it to lowercase.
Continue using lowercase paths for the download cache to avoid duplicate file downloads on Windows, where file paths are case-insensitive.
#Testing
Unit and integration test suites are added.