Skip to content

[SYNPY-1882] preserve file name casing when uploading/downloading data - #1431

Merged
danlu1 merged 15 commits into
developfrom
SYNPY-1882-file-name-casing
Jul 31, 2026
Merged

[SYNPY-1882] preserve file name casing when uploading/downloading data #1431
danlu1 merged 15 commits into
developfrom
SYNPY-1882-file-name-casing

Conversation

@danlu1

@danlu1 danlu1 commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Problem

utils.normalize_path() called os.path.normcase() on every path, which lowercases the entire string on Windows but not on other operating systems. This corrupted derived entity/file names (SYNR-1534) — e.g., an uploaded file like File_Name.TSV would get its name mangled to lowercase when the path round-tripped through normalize_path, and downloaded files could be renamed on disk to the lowercased form.

Solution

  1. Replace the os.path.normcase() with os.path.abspath from normalize_path so path casing is preserved on all platforms. Since Cache previously relied on that implicit lowercasing for case-insensitive matching, added a new _match_cache_map_key() helper in cache.py that tries an exact match first and only falls back to a normcase-based case-insensitive comparison.
  2. Wired this helper into Cache.contains(), Cache.get(), Cache.add() (replacing a legacy lowercased key instead of duplicating it), and Cache.remove(). On POSIX, os.path.normcase is a no-op, so the fallback is inert there and behavior is unchanged.

Note: Cache.get() (download path) still only matches by path — a rename/move with unchanged content still forces a real re-download, since there's no MD5 fallback on the download side (unlike upload, which self-heals via _needs_upload's MD5 comparison against Synapse's record).

Testing

  1. Unit tests in unit_test_utils.py confirm normalize_path() and guess_file_name() preserve casing.
  2. Unit tests in unit_test_Cache.py cover _match_cache_map_key()directly and Cache.contains()/get()/add()/remove() against legacy lowercased keys and casing-only renames, simulating Windows via a mocked os.path.normcase.
  3. New integration tests in test_upload.py confirm File.store_async avoids redundant re-uploads after local renames, moves, and (on POSIX) casing changes, since MD5 comparison against Synapse's record catches these even when the cache misses by path.
  4. Expanded integration tests in test_download.py exercise the download-side cache-hit/miss behavior for renamed, moved, and re-cased files under both simulated case-sensitive and case-insensitive filesystems.

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
Copilot AI review requested due to automatic review settings July 22, 2026 18:20
@danlu1
danlu1 requested a review from a team as a code owner July 22, 2026 18:20
@danlu1
danlu1 marked this pull request as draft July 22, 2026 18:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Synapse Python Client’s path normalization to preserve original path casing (notably on Windows) so derived entity names and downloaded filenames are not unintentionally lowercased, while updating cache lookup behavior to remain compatible with legacy (lowercased) cache keys.

Changes:

  • Updated utils.normalize_path() to stop using os.path.normcase() so path casing is preserved.
  • Added cache-key matching logic in core/cache.py that prefers exact matches and falls back to os.path.normcase matching to support legacy Windows cache maps.
  • Added unit tests to validate case preservation and legacy cache-key compatibility.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
synapseclient/core/utils.py Preserves path casing by removing os.path.normcase() from normalize_path().
synapseclient/core/cache.py Adds _match_cache_map_key() and updates cache operations to tolerate legacy lowercased keys.
tests/unit/synapseclient/core/unit_test_utils.py Adds coverage ensuring normalize_path() and guess_file_name() preserve case.
tests/unit/synapseclient/core/unit_test_Cache.py Adds tests for legacy lowercased cache keys and key migration behavior.
synapseclient/core/CLAUDE.md Updates internal documentation to reflect the new normalize_path()/cache behavior contract.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread synapseclient/core/cache.py
@danlu1
danlu1 marked this pull request as ready for review July 29, 2026 17:51

@thomasyu888 thomasyu888 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔥 Thanks for taking a look at this, and what an interesting bug fix. I'll defer to @BryanFauble for final review of this!

@thomasyu888
thomasyu888 requested a review from BryanFauble July 30, 2026 01:06
Comment thread tests/integration/synapseclient/core/test_download.py Outdated

@BryanFauble BryanFauble left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would remove all integration tests on this change and replace it with 100% branch code coverage via unit tests instead.

Approving ahead of that because the core logic looks good to me.

@danlu1
danlu1 merged commit 8e69548 into develop Jul 31, 2026
32 checks passed
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.

4 participants