From 8386c10ce5c1492097c9b6754b97d2d99fbf7df9 Mon Sep 17 00:00:00 2001 From: BryanFauble <17128019+BryanFauble@users.noreply.github.com> Date: Mon, 6 Jul 2026 23:53:53 +0000 Subject: [PATCH 01/12] [SYNR-1534] Preserve file name casing on Windows uploads/downloads 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. --- synapseclient/core/CLAUDE.md | 3 +- synapseclient/core/cache.py | 67 ++++++++++++-- synapseclient/core/utils.py | 13 ++- .../synapseclient/core/unit_test_Cache.py | 91 +++++++++++++++++++ .../synapseclient/core/unit_test_utils.py | 18 ++++ 5 files changed, 181 insertions(+), 11 deletions(-) diff --git a/synapseclient/core/CLAUDE.md b/synapseclient/core/CLAUDE.md index 6b91fee72..502a98c6e 100644 --- a/synapseclient/core/CLAUDE.md +++ b/synapseclient/core/CLAUDE.md @@ -1,4 +1,4 @@ - + ## Project @@ -44,6 +44,7 @@ Maps Java class names from Synapse REST API for polymorphic deserialization. Whe - `log_dataclass_diff(obj1, obj2)` — logs field-by-field differences between two dataclass instances - `snake_case(name)` — converts camelCase to snake_case - `normalize_whitespace(s)` — collapses whitespace +- `normalize_path(path)` — absolute path with forward slashes. Deliberately does NOT call `os.path.normcase()` (which lowercases on Windows and corrupted derived entity names — SYNR-1534). Case-insensitive cache matching lives in `cache.py::_match_cache_map_key`, which prefers an exact match and falls back to `os.path.normcase` so legacy lowercased cache keys still resolve without forcing re-downloads. - `MB`, `KB`, `GB` — byte size constants - `make_bogus_data_file()`, `make_bogus_binary_file(n)`, `make_bogus_uuid_file()` — test file generators (in production code, used by tests) diff --git a/synapseclient/core/cache.py b/synapseclient/core/cache.py index 080081da9..3248d7362 100644 --- a/synapseclient/core/cache.py +++ b/synapseclient/core/cache.py @@ -77,6 +77,40 @@ def _get_modified_time(path): return None +def _match_cache_map_key( + cache_map: dict, path: typing.Union[str, None] +) -> typing.Union[str, None]: + """ + Find the key in ``cache_map`` that corresponds to ``path``. + + 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. + + Arguments: + cache_map: The parsed ``.cacheMap`` contents (path -> entry). + path: A path already run through ``utils.normalize_path``. + + Returns: + The matching key from ``cache_map``, or ``None`` if there is no match. + """ + if path is None: + return None + if path in cache_map: + return path + normalized_path = os.path.normcase(path) + for key in cache_map: + if os.path.normcase(key) == normalized_path: + return key + return None + + class Cache: """ Represent a cache in which files are accessed by file handle ID. @@ -228,7 +262,10 @@ def contains( path = utils.normalize_path(path) - cached_time = self._get_cache_modified_time(cache_map.get(path, None)) + matched_key = _match_cache_map_key(cache_map, path) + cached_time = self._get_cache_modified_time( + cache_map.get(matched_key, None) + ) if cached_time: return compare_timestamps(_get_modified_time(path), cached_time) @@ -286,7 +323,12 @@ def get( # iterate a copy of cache_map to allow modifying original cache_map for cached_file_path, cache_map_entry in dict(cache_map).items(): - if path == os.path.dirname(cached_file_path): + # Compare case-insensitively via os.path.normcase (a + # no-op on POSIX) so directories cached by older clients + # with lowercased keys on Windows are still matched. + if os.path.normcase(path) == os.path.normcase( + os.path.dirname(cached_file_path) + ): if self._cache_item_unmodified( cache_map_entry, cached_file_path ): @@ -311,7 +353,8 @@ def get( # if we're given a full file path, look up a matching file in the cache else: - cache_map_entry = cache_map.get(path, None) + matched_key = _match_cache_map_key(cache_map, path) + cache_map_entry = cache_map.get(matched_key, None) if cache_map_entry: matching_file_path = ( path @@ -357,6 +400,13 @@ def add( cache_map = self._read_cache_map(cache_dir) path = utils.normalize_path(path) + # Drop any pre-existing entry that refers to the same file under a + # different casing (e.g. a lowercased key written by an older client + # on Windows) so the cache map migrates to the case-preserving key + # instead of accumulating duplicates. + existing_key = _match_cache_map_key(cache_map, path) + if existing_key is not None and existing_key != path: + del cache_map[existing_key] # write .000 milliseconds for backward compatibility cache_map[path] = { "modified_time": epoch_time_to_iso( @@ -409,11 +459,12 @@ def remove( cache_map = {} else: path = utils.normalize_path(path) - if path in cache_map: - if delete is True and os.path.exists(path): - os.remove(path) - del cache_map[path] - removed.append(path) + 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] + removed.append(matched_key) self._write_cache_map(cache_dir, cache_map) diff --git a/synapseclient/core/utils.py b/synapseclient/core/utils.py index 508a715e6..3a30d68b7 100644 --- a/synapseclient/core/utils.py +++ b/synapseclient/core/utils.py @@ -399,10 +399,19 @@ def guess_file_name(string): def normalize_path(path): - """Transforms a path into an absolute path with forward slashes only.""" + """Transforms a path into an absolute path with forward slashes only. + + 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. + """ if path is None: return None - return re.sub(r"\\", "/", os.path.normcase(os.path.abspath(path))) + return re.sub(r"\\", "/", os.path.abspath(path)) def equal_paths(path1, path2): diff --git a/tests/unit/synapseclient/core/unit_test_Cache.py b/tests/unit/synapseclient/core/unit_test_Cache.py index 9466146df..f071f18e5 100644 --- a/tests/unit/synapseclient/core/unit_test_Cache.py +++ b/tests/unit/synapseclient/core/unit_test_Cache.py @@ -230,6 +230,97 @@ def test_cache_store_get(): assert my_cache.get(file_handle_id=101202) is None +def test_match_cache_map_key(): + """ + SYNR-1534: _match_cache_map_key prefers an exact match, then falls back to a + case-insensitive (os.path.normcase) match so that cache entries written by + older clients -- which lowercased their keys on Windows -- are still found. + """ + cache_map = { + "c:/users/me/data/file.csv": {"modified_time": "t"}, + "c:/users/me/data/other.csv": {"modified_time": "t"}, + } + + # exact match + assert ( + cache._match_cache_map_key(cache_map, "c:/users/me/data/file.csv") + == "c:/users/me/data/file.csv" + ) + # no match / edge cases + assert cache._match_cache_map_key(cache_map, "c:/users/me/data/missing.csv") is None + assert cache._match_cache_map_key({}, "c:/x") is None + assert cache._match_cache_map_key(cache_map, None) is None + + # case-insensitive fallback (simulate Windows os.path.normcase) + with patch("os.path.normcase", side_effect=str.lower): + assert ( + cache._match_cache_map_key(cache_map, "C:/Users/Me/Data/File.csv") + == "c:/users/me/data/file.csv" + ) + # an exact match is still preferred over a case-insensitive one + mixed = {"C:/Data/File.csv": 1, "c:/data/file.csv": 2} + assert ( + cache._match_cache_map_key(mixed, "C:/Data/File.csv") == "C:/Data/File.csv" + ) + + +def test_get_matches_legacy_lowercased_key(): + """ + SYNR-1534: a cache map written by an older Windows client stored lowercased + keys. After preserving case in normalize_path, a mixed-case query must still + hit those legacy entries so users are not forced to re-download. + """ + tmp_dir = tempfile.mkdtemp() + my_cache = cache.Cache(cache_root_dir=tmp_dir) + cache_dir = my_cache.get_cache_dir(101201) + + path = utils.touch(os.path.join(cache_dir, "Mixed_Case.ext")) + my_cache.add(file_handle_id=101201, path=path) + + normalized = utils.normalize_path(path) + legacy_key = normalized.lower() + assert legacy_key != normalized # the file name has mixed case + + # Rewrite the cache map the way an older Windows client would have: lowercased key + cache_map = my_cache._read_cache_map(cache_dir) + entry = cache_map.pop(normalized) + cache_map[legacy_key] = entry + my_cache._write_cache_map(cache_dir, cache_map) + + with patch("os.path.normcase", side_effect=str.lower): + found = my_cache.get(file_handle_id=101201, path=path) + assert utils.equal_paths(found, path) + + +def test_add_migrates_legacy_lowercased_key(): + """ + SYNR-1534: adding a case-preserving path when a legacy lowercased key already + exists for the same file should replace the old key rather than create a + duplicate cache map entry. + """ + tmp_dir = tempfile.mkdtemp() + my_cache = cache.Cache(cache_root_dir=tmp_dir) + cache_dir = my_cache.get_cache_dir(101201) + + path = utils.touch(os.path.join(cache_dir, "Mixed_Case.ext")) + normalized = utils.normalize_path(path) + legacy_key = normalized.lower() + assert legacy_key != normalized + + # Pre-seed the cache map with a legacy lowercased entry + my_cache._write_cache_map( + cache_dir, + {legacy_key: {"modified_time": "2020-01-01T00:00:00.000Z", "content_md5": "x"}}, + ) + + with patch("os.path.normcase", side_effect=str.lower): + cache_map = my_cache.add(file_handle_id=101201, path=path) + + assert normalized in cache_map + assert legacy_key not in cache_map + assert len(cache_map) == 1 + + def test_cache_modified_time(): tmp_dir = tempfile.mkdtemp() my_cache = cache.Cache(cache_root_dir=tmp_dir) diff --git a/tests/unit/synapseclient/core/unit_test_utils.py b/tests/unit/synapseclient/core/unit_test_utils.py index 9ed9857ed..63d9959ff 100644 --- a/tests/unit/synapseclient/core/unit_test_utils.py +++ b/tests/unit/synapseclient/core/unit_test_utils.py @@ -249,6 +249,24 @@ def test_normalize_path() -> None: assert utils.normalize_path(None) is None +def test_normalize_path_preserves_case() -> None: + # SYNR-1534: normalize_path must not lowercase the path (previously it used + # os.path.normcase, which lowercases on Windows and corrupted derived names). + assert utils.normalize_path("SomeDir/File_Name.TSV").endswith( + "SomeDir/File_Name.TSV" + ) + assert utils.normalize_path("Mixed\\Case\\Path.CSV").endswith("Mixed/Case/Path.CSV") + + +def test_guess_file_name_preserves_case() -> None: + # SYNR-1534: the entity name derived from a path must keep its original casing + # on every platform, including Windows. + assert utils.guess_file_name("/some/path/File_Name.TSV") == "File_Name.TSV" + assert ( + utils.guess_file_name("C:\\Users\\Me\\Docs\\Mixed_Case.CSV") == "Mixed_Case.CSV" + ) + + def test_limit_and_offset() -> None: def query_params(uri): """Return the query params as a dict""" From 339dcb8dd97fd902447bff2616691a7f4ca674e9 Mon Sep 17 00:00:00 2001 From: danlu1 Date: Tue, 28 Jul 2026 23:50:23 -0700 Subject: [PATCH 02/12] update doctstring of _match_cache_map_key --- synapseclient/core/cache.py | 23 +++++++---------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/synapseclient/core/cache.py b/synapseclient/core/cache.py index 3248d7362..1358c2d66 100644 --- a/synapseclient/core/cache.py +++ b/synapseclient/core/cache.py @@ -82,16 +82,13 @@ def _match_cache_map_key( ) -> typing.Union[str, None]: """ Find the key in ``cache_map`` that corresponds to ``path``. - - 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. + An exact match is tried first. If none exists, fall back to a + case-insensitive (``os.path.normcase``) comparison so cache entries + written by older clients, which lowercased keys via ``os.path.normcase`` + on Windows, are still found — preserving path casing in + ``utils.normalize_path`` doesn't force a re-download of already-cached + files. On POSIX, ``os.path.normcase`` is a no-op, so the fallback never + changes behavior there. Arguments: cache_map: The parsed ``.cacheMap`` contents (path -> entry). @@ -236,7 +233,6 @@ def _cache_item_unmodified( """ cached_time = self._get_cache_modified_time(cache_map_entry) cached_md5 = self._get_cache_content_md5(cache_map_entry) - # compare_timestamps has an implicit check for whether the path exists return compare_timestamps(_get_modified_time(path), cached_time) and ( cached_md5 is None or cached_md5 == utils.md5_for_file(path).hexdigest() @@ -312,7 +308,6 @@ def get( # but has been modified, we need to indicate no match by returning # None. The logic for updating a synapse entity depends on this to # determine the need to upload a new file. - if path is not None: # If we're given a path to a directory, look for a cached file in that directory if os.path.isdir(path): @@ -320,7 +315,6 @@ def get( removed_entry_from_cache = ( False # determines if cache_map needs to be rewritten to disk ) - # iterate a copy of cache_map to allow modifying original cache_map for cached_file_path, cache_map_entry in dict(cache_map).items(): # Compare case-insensitively via os.path.normcase (a @@ -378,7 +372,6 @@ def get( if self._cache_item_unmodified(cache_map_entry, cached_file_path): trace.get_current_span().set_attributes({"synapse.cache.hit": True}) return cached_file_path - trace.get_current_span().set_attributes({"synapse.cache.hit": False}) return None @@ -393,12 +386,10 @@ def add( """ if not path or not os.path.exists(path): raise ValueError('Can\'t find file "%s"' % path) - cache_dir = self.get_cache_dir(file_handle_id) content_md5 = md5 or utils.md5_for_file(path).hexdigest() with Lock(self.cache_map_file_name, dir=cache_dir): cache_map = self._read_cache_map(cache_dir) - path = utils.normalize_path(path) # Drop any pre-existing entry that refers to the same file under a # different casing (e.g. a lowercased key written by an older client From 6ea8a89bd5c55803bb43f6ad0bd646afacdb5883 Mon Sep 17 00:00:00 2001 From: danlu1 Date: Tue, 28 Jul 2026 23:52:49 -0700 Subject: [PATCH 03/12] reformat unit test --- .../synapseclient/core/unit_test_Cache.py | 154 ++++++++++++++++-- 1 file changed, 136 insertions(+), 18 deletions(-) diff --git a/tests/unit/synapseclient/core/unit_test_Cache.py b/tests/unit/synapseclient/core/unit_test_Cache.py index f071f18e5..240239b00 100644 --- a/tests/unit/synapseclient/core/unit_test_Cache.py +++ b/tests/unit/synapseclient/core/unit_test_Cache.py @@ -232,7 +232,7 @@ def test_cache_store_get(): def test_match_cache_map_key(): """ - SYNR-1534: _match_cache_map_key prefers an exact match, then falls back to a + Test that _match_cache_map_key prefers an exact match, then falls back to a case-insensitive (os.path.normcase) match so that cache entries written by older clients -- which lowercased their keys on Windows -- are still found. """ @@ -241,11 +241,6 @@ def test_match_cache_map_key(): "c:/users/me/data/other.csv": {"modified_time": "t"}, } - # exact match - assert ( - cache._match_cache_map_key(cache_map, "c:/users/me/data/file.csv") - == "c:/users/me/data/file.csv" - ) # no match / edge cases assert cache._match_cache_map_key(cache_map, "c:/users/me/data/missing.csv") is None assert cache._match_cache_map_key({}, "c:/x") is None @@ -264,11 +259,69 @@ def test_match_cache_map_key(): ) +def test_match_cache_map_key_casing_change(): + """ + If Synapse changes a file's name casing and the user matches it locally, + the cache falls back to case-insensitive matching on Windows, but not POSIX. + """ + cache_map = { + "/Users/me/data/File.csv": { + "modified_time": "2020-01-01T00:00:00.000Z", + "content_md5": "abc123", + } + } + + # exact case always matches directly, no fallback needed + assert ( + cache._match_cache_map_key(cache_map, "/Users/me/data/File.csv") + == "/Users/me/data/File.csv" + ) + + # on POSIX + assert cache._match_cache_map_key(cache_map, "/Users/me/data/file.csv") is None + + # on Windows + with patch("os.path.normcase", side_effect=str.lower): + assert ( + cache._match_cache_map_key(cache_map, "/Users/me/data/file.csv") + == "/Users/me/data/File.csv" + ) + assert ( + cache._match_cache_map_key(cache_map, "/USERS/ME/DATA/FILE.CSV") + == "/Users/me/data/File.csv" + ) + + +def test_cache_contains_after_casing_change_locally(): + """ + A casing-only local rename should be a miss on POSIX (os.path.normcase is a + no-op) but a hit when simulating Windows case-insensitive matching. + """ + tmp_dir = tempfile.mkdtemp() + my_cache = cache.Cache(cache_root_dir=tmp_dir) + file_handle_id = 101201 + + original_path = utils.touch( + os.path.join(my_cache.get_cache_dir(file_handle_id), "File.csv") + ) + my_cache.add(file_handle_id=file_handle_id, path=original_path) + + # user renames their local file to match Synapse's re-cased file name; + # only the casing of the base name changes + recased_path = os.path.join(os.path.dirname(original_path), "file.csv") + os.rename(original_path, recased_path) + + # on POSIX, os.path.normcase is a no-op so casing-only changes are a miss + assert not my_cache.contains(file_handle_id=file_handle_id, path=recased_path) + + with patch("os.path.normcase", side_effect=str.lower): + assert my_cache.contains(file_handle_id=file_handle_id, path=recased_path) + + def test_get_matches_legacy_lowercased_key(): """ - SYNR-1534: a cache map written by an older Windows client stored lowercased - keys. After preserving case in normalize_path, a mixed-case query must still - hit those legacy entries so users are not forced to re-download. + Test that a cache map written by an older Windows client stored lowercased + keys still hits those legacy entries. """ tmp_dir = tempfile.mkdtemp() my_cache = cache.Cache(cache_root_dir=tmp_dir) @@ -279,23 +332,85 @@ def test_get_matches_legacy_lowercased_key(): normalized = utils.normalize_path(path) legacy_key = normalized.lower() - assert legacy_key != normalized # the file name has mixed case + assert legacy_key != normalized - # Rewrite the cache map the way an older Windows client would have: lowercased key cache_map = my_cache._read_cache_map(cache_dir) entry = cache_map.pop(normalized) cache_map[legacy_key] = entry my_cache._write_cache_map(cache_dir, cache_map) + rewritten_cache_map = my_cache._read_cache_map(cache_dir) + assert legacy_key in rewritten_cache_map + assert normalized not in rewritten_cache_map with patch("os.path.normcase", side_effect=str.lower): - found = my_cache.get(file_handle_id=101201, path=path) - assert utils.equal_paths(found, path) + assert my_cache.get(file_handle_id=101201, path=path) == path -def test_add_migrates_legacy_lowercased_key(): +def test_contains_matches_legacy_lowercased_key(): + """ + Test that contains() can still match a legacy lowercased cache-map key when + simulating Windows case-insensitive path normalization. """ - SYNR-1534: adding a case-preserving path when a legacy lowercased key already - exists for the same file should replace the old key rather than create a + tmp_dir = tempfile.mkdtemp() + my_cache = cache.Cache(cache_root_dir=tmp_dir) + cache_dir = my_cache.get_cache_dir(101201) + + path = utils.touch(os.path.join(cache_dir, "Mixed_Case.ext")) + my_cache.add(file_handle_id=101201, path=path) + + normalized = utils.normalize_path(path) + legacy_key = normalized.lower() + assert legacy_key != normalized + + cache_map = my_cache._read_cache_map(cache_dir) + entry = cache_map.pop(normalized) + cache_map[legacy_key] = entry + my_cache._write_cache_map(cache_dir, cache_map) + + # POSIX normcase is a no-op, so this mixed-case lookup misses. + assert not my_cache.contains(file_handle_id=101201, path=path) + + # Simulate Windows behavior where lowercased legacy keys are still matched. + with patch("os.path.normcase", side_effect=str.lower): + assert my_cache.contains(file_handle_id=101201, path=path) + + +def test_remove_matches_legacy_lowercased_key(): + """ + Test that remove() can resolve and remove a legacy lowercased cache-map key + when simulating Windows case-insensitive path normalization. + """ + tmp_dir = tempfile.mkdtemp() + my_cache = cache.Cache(cache_root_dir=tmp_dir) + cache_dir = my_cache.get_cache_dir(101201) + + path = utils.touch(os.path.join(cache_dir, "Mixed_Case.ext")) + my_cache.add(file_handle_id=101201, path=path) + + normalized = utils.normalize_path(path) + legacy_key = normalized.lower() + assert legacy_key != normalized + + cache_map = my_cache._read_cache_map(cache_dir) + entry = cache_map.pop(normalized) + cache_map[legacy_key] = entry + my_cache._write_cache_map(cache_dir, cache_map) + + # POSIX normcase is a no-op, so this mixed-case lookup misses. + assert my_cache.remove(file_handle_id=101201, path=path) == [] + + # Simulate Windows behavior: remove should match and delete the legacy key. + with patch("os.path.normcase", side_effect=str.lower): + removed = my_cache.remove(file_handle_id=101201, path=path) + + assert removed == [legacy_key] + assert my_cache._read_cache_map(cache_dir) == {} + + +def test_add_replaces_legacy_lowercased_key(): + """ + Test that adding a case-preserving path when a legacy lowercased key already + exists for the same file replaces the old key rather than creating a duplicate cache map entry. """ tmp_dir = tempfile.mkdtemp() @@ -307,15 +422,18 @@ def test_add_migrates_legacy_lowercased_key(): legacy_key = normalized.lower() assert legacy_key != normalized - # Pre-seed the cache map with a legacy lowercased entry my_cache._write_cache_map( cache_dir, {legacy_key: {"modified_time": "2020-01-01T00:00:00.000Z", "content_md5": "x"}}, ) + # check that the legacy key is still in the cache map + cache_map = my_cache._read_cache_map(cache_dir) + assert legacy_key in cache_map + assert normalized not in cache_map + # add the case-preserving path with patch("os.path.normcase", side_effect=str.lower): cache_map = my_cache.add(file_handle_id=101201, path=path) - assert normalized in cache_map assert legacy_key not in cache_map assert len(cache_map) == 1 From a621b8c49a1f364db893e517de42496a7d92cf05 Mon Sep 17 00:00:00 2001 From: danlu1 Date: Tue, 28 Jul 2026 23:55:42 -0700 Subject: [PATCH 04/12] reformat test_upload.py --- .../synapseclient/core/test_upload.py | 404 ++++++++++++++++++ 1 file changed, 404 insertions(+) create mode 100644 tests/integration/synapseclient/core/test_upload.py diff --git a/tests/integration/synapseclient/core/test_upload.py b/tests/integration/synapseclient/core/test_upload.py new file mode 100644 index 000000000..5c3bdd697 --- /dev/null +++ b/tests/integration/synapseclient/core/test_upload.py @@ -0,0 +1,404 @@ +"""Integration tests for uploading files, focused on avoiding redundant re-uploads via the local disk cache and MD5 comparison.""" + +import os +import shutil +import sys +import tempfile +import uuid +from typing import Callable +from unittest.mock import patch + +import pytest +from pytest_mock import MockerFixture + +from synapseclient import Synapse +from synapseclient.core import utils +from synapseclient.models import File, Project + +DESCRIPTION = "This is an example file." +CONTENT_TYPE = "text/plain" +VERSION_COMMENT = "My version comment" + + +class TestFileStoreAvoidsRedundantUpload: + """ + Tests that File.store_async avoids re-uploading file content that has + not changed, even when the local path no longer matches what's recorded + in the local disk cache (renamed, moved, or re-cased). In those cases + `Cache.contains()` (synapseclient/core/cache.py) misses because it only + matches by path, and it's the MD5 comparison against what Synapse + already has on record (`_needs_upload` in synapseclient/models/file.py) + that actually prevents the redundant upload. + """ + + async def test_reupload_after_local_rename_same_content( + self, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + # Given a file stored in Synapse + original_path = utils.make_bogus_uuid_file() + schedule_for_cleanup(original_path) + file = await File( + path=original_path, + description=DESCRIPTION, + content_type=CONTENT_TYPE, + version_comment=VERSION_COMMENT, + version_label=str(uuid.uuid4()), + ).store_async(parent=project_model, synapse_client=syn) + schedule_for_cleanup(file.id) + before_file_handle_id = file.file_handle.id + before_version_number = file.version_number + before_file_handle_file_name = file.file_handle.file_name + + # WHEN the local file is renamed, with no change to its content + renamed_path = os.path.join( + os.path.dirname(original_path), f"renamed_{uuid.uuid4()}.txt" + ) + os.rename(original_path, renamed_path) + schedule_for_cleanup(renamed_path) + file.path = renamed_path + + with patch( + "synapseclient.models.file.upload_file_handle" + ) as mocked_upload_file_handle: + new_file = await file.store_async(synapse_client=syn) + + # THEN the already-uploaded file handle is reused + assert not mocked_upload_file_handle.called + assert new_file.file_handle.id == before_file_handle_id + assert new_file.version_number == before_version_number + # AND the file handle's saved filename still reflects the original + # upload, not the local rename -- Synapse never saw the new name + assert new_file.file_handle.file_name == before_file_handle_file_name + + async def test_reupload_from_different_directory_same_content( + self, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + # Given a file stored in Synapse + original_path = utils.make_bogus_uuid_file() + schedule_for_cleanup(original_path) + file = await File( + path=original_path, + description=DESCRIPTION, + content_type=CONTENT_TYPE, + version_comment=VERSION_COMMENT, + version_label=str(uuid.uuid4()), + ).store_async(parent=project_model, synapse_client=syn) + schedule_for_cleanup(file.id) + before_file_handle_id = file.file_handle.id + before_version_number = file.version_number + before_file_handle_file_name = file.file_handle.file_name + + # WHEN the same content is re-stored from a copy of the file in a different directory + other_dir = tempfile.mkdtemp() + schedule_for_cleanup(other_dir) + copied_path = os.path.join(other_dir, os.path.basename(original_path)) + shutil.copyfile(original_path, copied_path) + file.path = copied_path + + with patch( + "synapseclient.models.file.upload_file_handle" + ) as mocked_upload_file_handle: + new_file = await file.store_async(synapse_client=syn) + + # THEN no redundant upload occurs because the MD5 match + assert not mocked_upload_file_handle.called + assert new_file.file_handle.id == before_file_handle_id + assert new_file.version_number == before_version_number + # AND the file handle's saved filename is unchanged + assert new_file.file_handle.file_name == before_file_handle_file_name + + @pytest.mark.skipif( + sys.platform == "win32", + reason=( + "Windows filesystems are case-insensitive, so os.path.normcase " + "folds this casing-only rename into an exact cache hit instead " + "of exercising the POSIX MD5-fallback path this test targets." + ), + ) + async def test_reupload_after_casing_change_same_content( + self, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + # Given a file stored in Synapse + original_path = utils.make_bogus_uuid_file() + schedule_for_cleanup(original_path) + file = await File( + path=original_path, + description=DESCRIPTION, + content_type=CONTENT_TYPE, + version_comment=VERSION_COMMENT, + version_label=str(uuid.uuid4()), + ).store_async(parent=project_model, synapse_client=syn) + schedule_for_cleanup(file.id) + before_file_handle_id = file.file_handle.id + before_version_number = file.version_number + before_file_handle_file_name = file.file_handle.file_name + + # WHEN the local file name is re-cased + recased_path = os.path.join( + os.path.dirname(original_path), + os.path.basename(original_path).upper(), + ) + os.rename(original_path, recased_path) + schedule_for_cleanup(recased_path) + file.path = recased_path + + with patch( + "synapseclient.models.file.upload_file_handle" + ) as mocked_upload_file_handle: + new_file = await file.store_async(synapse_client=syn) + + # THEN the MD5 comparison against Synapse's record avoids a redundant upload + assert not mocked_upload_file_handle.called + assert new_file.file_handle.id == before_file_handle_id + assert new_file.version_number == before_version_number + # AND the saved filename keeps its original casing -- the re-cased + # local name was never uploaded, so Synapse's record didn't change + assert new_file.file_handle.file_name == before_file_handle_file_name + + async def test_reupload_after_move_and_rename_same_content( + self, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + # GIVEN a file stored in Synapse + original_path = utils.make_bogus_uuid_file() + schedule_for_cleanup(original_path) + file = await File( + path=original_path, + description=DESCRIPTION, + content_type=CONTENT_TYPE, + version_comment=VERSION_COMMENT, + version_label=str(uuid.uuid4()), + ).store_async(parent=project_model, synapse_client=syn) + schedule_for_cleanup(file.id) + before_file_handle_id = file.file_handle.id + before_version_number = file.version_number + before_file_handle_file_name = file.file_handle.file_name + + # WHEN the file is both moved to a new directory AND renamed + other_dir = tempfile.mkdtemp() + schedule_for_cleanup(other_dir) + new_path = os.path.join(other_dir, f"moved_and_renamed_{uuid.uuid4()}.txt") + shutil.move(original_path, new_path) + file.path = new_path + + with patch( + "synapseclient.models.file.upload_file_handle" + ) as mocked_upload_file_handle: + new_file = await file.store_async(synapse_client=syn) + + # THEN no redundant upload occurs because the MD5 match + assert not mocked_upload_file_handle.called + assert new_file.file_handle.id == before_file_handle_id + assert new_file.version_number == before_version_number + # AND the file handle's saved filename is unchanged + assert new_file.file_handle.file_name == before_file_handle_file_name + + @pytest.mark.skipif( + sys.platform == "win32", + reason=( + "Windows filesystems are case-insensitive, so os.path.normcase " + "folds this casing-only rename into an exact cache hit instead " + "of exercising the POSIX MD5-fallback path this test targets." + ), + ) + async def test_reupload_after_casing_change_and_move_same_content( + self, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + # GIVEN a file stored in Synapse + original_path = utils.make_bogus_uuid_file() + schedule_for_cleanup(original_path) + file = await File( + path=original_path, + description=DESCRIPTION, + content_type=CONTENT_TYPE, + version_comment=VERSION_COMMENT, + version_label=str(uuid.uuid4()), + ).store_async(parent=project_model, synapse_client=syn) + schedule_for_cleanup(file.id) + before_file_handle_id = file.file_handle.id + before_version_number = file.version_number + before_file_handle_file_name = file.file_handle.file_name + + # WHEN the file is moved to a new directory AND its name is re-cased + other_dir = tempfile.mkdtemp() + schedule_for_cleanup(other_dir) + new_path = os.path.join(other_dir, os.path.basename(original_path).upper()) + shutil.move(original_path, new_path) + file.path = new_path + + with patch( + "synapseclient.models.file.upload_file_handle" + ) as mocked_upload_file_handle: + new_file = await file.store_async(synapse_client=syn) + + # THEN no redundant upload occurs + assert not mocked_upload_file_handle.called + assert new_file.file_handle.id == before_file_handle_id + assert new_file.version_number == before_version_number + # AND the saved filename keeps its original casing + assert new_file.file_handle.file_name == before_file_handle_file_name + + async def test_reupload_after_rename_with_force_version_false( + self, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + """ + force_version has no bearing on whether _needs_upload decides a + re-upload is needed -- it only affects whether an actual content or + metadata change is written as a new version. Confirms a rename with + unchanged content is just as much a no-op with force_version=False + as it is with the default force_version=True. + """ + # GIVEN a file stored in Synapse + original_path = utils.make_bogus_uuid_file() + schedule_for_cleanup(original_path) + file = await File( + path=original_path, + description=DESCRIPTION, + content_type=CONTENT_TYPE, + version_comment=VERSION_COMMENT, + version_label=str(uuid.uuid4()), + ).store_async(parent=project_model, synapse_client=syn) + schedule_for_cleanup(file.id) + before_file_handle_id = file.file_handle.id + before_version_number = file.version_number + before_file_handle_file_name = file.file_handle.file_name + + # WHEN the local file is renamed, with no change to its content, + # and force_version is explicitly disabled + renamed_path = os.path.join( + os.path.dirname(original_path), f"renamed_{uuid.uuid4()}.txt" + ) + os.rename(original_path, renamed_path) + schedule_for_cleanup(renamed_path) + file.path = renamed_path + file.force_version = False + + with patch( + "synapseclient.models.file.upload_file_handle" + ) as mocked_upload_file_handle: + new_file = await file.store_async(synapse_client=syn) + + # THEN no redundant upload occurs + assert not mocked_upload_file_handle.called + assert new_file.file_handle.id == before_file_handle_id + assert new_file.version_number == before_version_number + # AND the file handle's saved filename is unchanged + assert new_file.file_handle.file_name == before_file_handle_file_name + + async def test_reupload_after_rename_cache_self_heals( + self, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + mocker: MockerFixture, + ) -> None: + """ + Verifies the actual mechanism behind + test_reupload_after_local_rename_same_content + """ + # Given a file stored in Synapse + original_path = utils.make_bogus_uuid_file() + schedule_for_cleanup(original_path) + file = await File( + path=original_path, + description=DESCRIPTION, + content_type=CONTENT_TYPE, + version_comment=VERSION_COMMENT, + version_label=str(uuid.uuid4()), + ).store_async(parent=project_model, synapse_client=syn) + schedule_for_cleanup(file.id) + before_file_handle_file_name = file.file_handle.file_name + + # WHEN the local file is renamed, with no change to its content + renamed_path = os.path.join( + os.path.dirname(original_path), f"renamed_{uuid.uuid4()}.txt" + ) + os.rename(original_path, renamed_path) + schedule_for_cleanup(renamed_path) + file.path = renamed_path + + spy_contains = mocker.spy(syn.cache, "contains") + spy_add = mocker.spy(syn.cache, "add") + new_file = await file.store_async(synapse_client=syn) + + # THEN the renamed path was a genuine miss in the local disk cache... + spy_contains.assert_called_once_with(new_file.file_handle.id, renamed_path) + assert spy_contains.spy_return is False + + # _needs_upload self-healed the cache under the new path + spy_add.assert_called_once() + _, add_kwargs = spy_add.call_args + assert add_kwargs["file_handle_id"] == new_file.file_handle.id + assert add_kwargs["path"] == renamed_path + # AND the MD5-fallback match left the saved filename unchanged + assert new_file.file_handle.file_name == before_file_handle_file_name + + # check the updated cache + spy_contains.reset_mock() + spy_add.reset_mock() + file_after_cache_hit = await file.store_async(synapse_client=syn) + spy_contains.assert_called_once_with( + file_after_cache_hit.file_handle.id, renamed_path + ) + assert spy_contains.spy_return + spy_add.assert_not_called() + # AND the fast cache-hit path also leaves the saved filename unchanged + assert ( + file_after_cache_hit.file_handle.file_name == before_file_handle_file_name + ) + + async def test_reupload_after_rename_with_changed_content_creates_new_version( + self, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + """ + Confirms only a content change (not just rename) triggers re-upload/version. + """ + # GIVEN a file stored in Synapse + original_path = utils.make_bogus_uuid_file() + schedule_for_cleanup(original_path) + file = await File( + path=original_path, + description=DESCRIPTION, + content_type=CONTENT_TYPE, + version_comment=VERSION_COMMENT, + version_label=str(uuid.uuid4()), + ).store_async(parent=project_model, synapse_client=syn) + schedule_for_cleanup(file.id) + before_file_handle_id = file.file_handle.id + before_version_number = file.version_number + + # WHEN the local file is renamed AND its content is changed + renamed_path = os.path.join( + os.path.dirname(original_path), f"renamed_{uuid.uuid4()}.txt" + ) + os.rename(original_path, renamed_path) + schedule_for_cleanup(renamed_path) + with open(renamed_path, "w") as f: + f.write("this content is different from what was uploaded") + file.path = renamed_path + + new_file = await file.store_async(synapse_client=syn) + + # THEN a new file handle and version are created + assert new_file.file_handle.id != before_file_handle_id + assert new_file.version_number > before_version_number From fb100df495eddb5ebeb62c6a7eec134800220654 Mon Sep 17 00:00:00 2001 From: danlu1 Date: Wed, 29 Jul 2026 00:03:08 -0700 Subject: [PATCH 05/12] reformat test_download.py --- .../synapseclient/core/test_download.py | 490 +++++++++++++++++- 1 file changed, 488 insertions(+), 2 deletions(-) diff --git a/tests/integration/synapseclient/core/test_download.py b/tests/integration/synapseclient/core/test_download.py index 9dce6680e..85e43de8f 100644 --- a/tests/integration/synapseclient/core/test_download.py +++ b/tests/integration/synapseclient/core/test_download.py @@ -18,6 +18,7 @@ import synapseclient.core.utils as utils from synapseclient import Synapse from synapseclient.api.file_services import get_file_handle_for_download +from synapseclient.core import cache as cache_module from synapseclient.core.download import ( PresignedUrlInfo, download_from_url, @@ -224,8 +225,6 @@ async def test_download_cached_file_to_new_directory( schedule_for_cleanup(file.path) # THEN the file is not downloaded again, but it copied to the new location - assert os.path.exists(file.path) - assert os.path.exists(original_file_path) assert not utils.equal_paths(original_file_path, file.path) # AND download_by_file_handle was not called @@ -234,6 +233,493 @@ async def test_download_cached_file_to_new_directory( # AND download_file_entity_model was called spy_file_entity.assert_called_once() + async def test_redownload_after_local_rename_same_content( + self, + mocker: MockerFixture, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + """ + Contrast with the upload-side equivalent, Cache.get() has no equivalent + md5 fallback for downloads -- it only ever matches by recorded path, so + renaming a cached file, even within the same directory, invalidates + the stale cache entry and forces a real re-download. + """ + spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") + + # GIVEN a file stored in Synapse + original_file_path = utils.make_bogus_data_file() + file = await File( + path=original_file_path, parent_id=project_model.id + ).store_async(synapse_client=syn) + schedule_for_cleanup(file.id) + + # AND the file is in the cache under its original path + assert syn.cache.contains( + file_handle_id=file.file_handle.id, path=original_file_path + ) + + # WHEN the cached file is renamed, with no change to its content + renamed_path = f"{original_file_path}.renamed" + os.rename(original_file_path, renamed_path) + schedule_for_cleanup(renamed_path) + + redownloaded_file = await File( + id=file.id, path=os.path.dirname(renamed_path) + ).get_async(synapse_client=syn) + + # THEN a real download occurs since the cache entry pointing to the old path (no longer exists) is removed + spy_file_handle.assert_called_once() + assert utils.equal_paths(original_file_path, redownloaded_file.path) + assert utils.md5_for_file_hex( + filename=redownloaded_file.path + ) == utils.md5_for_file_hex(filename=renamed_path) + + async def test_redownload_after_moved_to_different_directory_same_content_no_explicit_destination( + self, + mocker: MockerFixture, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + """ + File is moved to an entirely different directory instead of renamed in place, + and no explicit download path is given. + The cache can't be retrieved since the cached file is no longer accessible and can't determine if it is unmodified. + So cache.get() returns None and a real download occurs. + """ + spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") + + # GIVEN a file stored in Synapse + original_file_path = utils.make_bogus_data_file() + file = await File( + path=original_file_path, parent_id=project_model.id + ).store_async(synapse_client=syn) + schedule_for_cleanup(file.id) + synapse_cache_location = syn.cache.get_cache_dir( + file_handle_id=file.data_file_handle_id + ) + + # AND the file is in the cache under its original path + assert syn.cache.contains( + file_handle_id=file.file_handle.id, path=original_file_path + ) + + # WHEN the cached file is moved to a different directory, with no change to its content + other_dir = tempfile.mkdtemp() + schedule_for_cleanup(other_dir) + moved_path = shutil.move( + original_file_path, + os.path.join(other_dir, os.path.basename(original_file_path)), + ) + + # when no explicit destination is given, the file is downloaded to the synapse cache location + redownloaded_file = await File(id=file.id).get_async(synapse_client=syn) + + # THEN a real download occurs. + spy_file_handle.assert_called_once() + assert utils.equal_paths( + os.path.dirname(redownloaded_file.path), synapse_cache_location + ) + assert utils.md5_for_file_hex( + filename=redownloaded_file.path + ) == utils.md5_for_file_hex(filename=moved_path) + + async def test_redownload_after_moved_to_different_directory_same_content_with_explicit_destination( + self, + mocker: MockerFixture, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + """ + File is moved to an entirely different directory instead of renamed in place, + and no explicit download path is given. + The cache can't be retrieved since the cached file is no longer accessible and can't determine if it is unmodified. + So cache.get() returns None and a real download occurs. + """ + spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") + + # GIVEN a file stored in Synapse + original_file_path = utils.make_bogus_data_file() + file = await File( + path=original_file_path, parent_id=project_model.id + ).store_async(synapse_client=syn) + schedule_for_cleanup(file.id) + + # AND the file is in the cache under its original path + assert syn.cache.contains( + file_handle_id=file.file_handle.id, path=original_file_path + ) + + # WHEN the cached file is moved to a different directory, with no change to its content + other_dir = tempfile.mkdtemp() + schedule_for_cleanup(other_dir) + moved_path = shutil.move( + original_file_path, + os.path.join(other_dir, os.path.basename(original_file_path)), + ) + + redownloaded_file = await File( + id=file.id, path=os.path.dirname(original_file_path) + ).get_async(synapse_client=syn) + + # THEN a real download occurs. + spy_file_handle.assert_called_once() + assert utils.equal_paths( + os.path.dirname(redownloaded_file.path), os.path.dirname(original_file_path) + ) + assert utils.md5_for_file_hex( + filename=redownloaded_file.path + ) == utils.md5_for_file_hex(filename=moved_path) + + async def test_redownload_after_casing_change_both_dir_file_on_case_sensitive_filesystem( + self, + mocker: MockerFixture, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + """ + Test the behavior when both the containing directory and the filename + are re-cased. Forcing os.path.normcase to compare case-sensitively + makes Cache.get()'s directory-match branch miss, but the fallback + loop still finds the entry unmodified, so no download occurs. + """ + spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") + + # GIVEN a file stored in Synapse + original_file_path = utils.make_bogus_data_file() + file = await File( + path=original_file_path, parent_id=project_model.id + ).store_async(synapse_client=syn) + schedule_for_cleanup(file.id) + + assert syn.cache.contains( + file_handle_id=file.file_handle.id, path=original_file_path + ) + + # WHEN both the containing directory and the filename are re-cased with no change to content + recased_file_path = original_file_path.upper() + os.rename(original_file_path, recased_file_path) + schedule_for_cleanup(recased_file_path) + + with ( + patch("os.path.normcase", side_effect=lambda p: p), + patch( + "synapseclient.api.entity_factory.download_file_entity_model", + wraps=spy_for_async_function( + download_functions.download_file_entity_model + ), + ) as spy_file_entity, + ): + redownloaded_file = await File( + id=file.id, path=os.path.dirname(recased_file_path) + ).get_async(synapse_client=syn) + + # THEN no download occurs since cache.get() returns the cached file path and the file is unmodified + spy_file_entity.assert_called_once() + spy_file_handle.assert_not_called() + # AND the cached content is copied into the requested (recased) directory + assert utils.equal_paths( + os.path.dirname(redownloaded_file.path), os.path.dirname(recased_file_path) + ) + assert utils.md5_for_file_hex( + filename=redownloaded_file.path + ) == utils.md5_for_file_hex(filename=recased_file_path) + + async def test_redownload_after_casing_change_only_file_on_case_sensitive_filesystem( + self, + mocker: MockerFixture, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + """ + Test the behavior when only the filename's casing changes and the + containing directory is untouched. Since the requested directory + string is identical to the cached entry's directory, Cache.get()'s + directory-match succeeds, and the resolved download_path lands back + on the exact cached path so no download occurs. No download occurs. + """ + spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") + + # GIVEN a file stored in Synapse + original_file_path = utils.make_bogus_data_file() + file = await File( + path=original_file_path, parent_id=project_model.id + ).store_async(synapse_client=syn) + schedule_for_cleanup(file.id) + + assert syn.cache.contains( + file_handle_id=file.file_handle.id, path=original_file_path + ) + + recased_file_path = os.path.join( + os.path.dirname(original_file_path), + os.path.basename(original_file_path).upper(), + ) + os.rename(original_file_path, recased_file_path) + schedule_for_cleanup(recased_file_path) + + with ( + patch("os.path.normcase", side_effect=lambda p: p), + patch( + "synapseclient.api.entity_factory.download_file_entity_model", + wraps=spy_for_async_function( + download_functions.download_file_entity_model + ), + ) as spy_file_entity, + ): + redownloaded_file = await File( + id=file.id, path=os.path.dirname(recased_file_path) + ).get_async(synapse_client=syn) + + # THEN no download occurs since cache.get() returns the cached file path and the file is unmodified + # and downloaded filename is extracted from the os.path.basename(cached_file_path) + # so the redownloaded file path is the original file path. + spy_file_entity.assert_called_once() + spy_file_handle.assert_not_called() + assert utils.equal_paths(redownloaded_file.path, original_file_path) + assert utils.md5_for_file_hex( + filename=redownloaded_file.path + ) == utils.md5_for_file_hex(filename=recased_file_path) + + async def test_redownload_after_casing_change_both_dir_file_on_case_insensitive_filesystem( + self, + mocker: MockerFixture, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + """ + Contrast with the case-sensitive variant: on a case-insensitive + filesystem, Cache.get()'s directory-match compares paths via the + real (unpatched) os.path.normcase, so a recased directory is still + recognized as the same directory. No download occurs and and the + cached content is copied into the recased directory under a + keep-both disambiguated name. + """ + spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") + + # GIVEN a file stored in Synapse + original_file_path = utils.make_bogus_data_file() + file = await File( + path=original_file_path, parent_id=project_model.id + ).store_async(synapse_client=syn) + schedule_for_cleanup(file.id) + + assert syn.cache.contains( + file_handle_id=file.file_handle.id, path=original_file_path + ) + + # WHEN both the containing directory and the filename are re-cased with no change to content + recased_file_path = original_file_path.upper() + os.rename(original_file_path, recased_file_path) + schedule_for_cleanup(recased_file_path) + + with ( + patch("os.path.normcase", side_effect=str.lower), + patch( + "synapseclient.api.entity_factory.download_file_entity_model", + wraps=spy_for_async_function( + download_functions.download_file_entity_model + ), + ) as spy_file_entity, + ): + redownloaded_file = await File( + id=file.id, path=os.path.dirname(recased_file_path) + ).get_async(synapse_client=syn) + + # THEN no download occurs since cache.get() returns the cached file path and the file is unmodified + spy_file_entity.assert_called_once() + spy_file_handle.assert_not_called() + # AND the cached content is copied into the requested (recased) directory -- + # under a disambiguated name, since the directory string matches the cached entry's directory but file name is different + # so a collision resolution is triggered. + assert utils.equal_paths( + os.path.dirname(redownloaded_file.path), os.path.dirname(recased_file_path) + ) + assert utils.md5_for_file_hex( + filename=redownloaded_file.path + ) == utils.md5_for_file_hex(filename=recased_file_path) + + async def test_redownload_after_casing_change_only_file_on_case_insensitive_filesystem( + self, + mocker: MockerFixture, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + """ + Contrast with the case-sensitive variant: with normcase patched to + str.lower instead of forced to identity, the directory-match still + succeeds so no download occurs. + """ + spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") + + # GIVEN a file stored in Synapse + original_file_path = utils.make_bogus_data_file() + file = await File( + path=original_file_path, parent_id=project_model.id + ).store_async(synapse_client=syn) + schedule_for_cleanup(file.id) + + assert syn.cache.contains( + file_handle_id=file.file_handle.id, path=original_file_path + ) + + recased_file_path = os.path.join( + os.path.dirname(original_file_path), + os.path.basename(original_file_path).upper(), + ) + os.rename(original_file_path, recased_file_path) + schedule_for_cleanup(recased_file_path) + + with ( + patch("os.path.normcase", side_effect=str.lower), + patch( + "synapseclient.api.entity_factory.download_file_entity_model", + wraps=spy_for_async_function( + download_functions.download_file_entity_model + ), + ) as spy_file_entity, + ): + redownloaded_file = await File( + id=file.id, path=os.path.dirname(recased_file_path) + ).get_async(synapse_client=syn) + + # THEN no download occurs since cache.get() returns the cached file path as the file is unmodified + # and downloaded filename is extracted from the file handle so the redownloaded file path is the original file path. + spy_file_entity.assert_called_once() + spy_file_handle.assert_not_called() + assert utils.equal_paths(redownloaded_file.path, original_file_path) + assert utils.md5_for_file_hex( + filename=redownloaded_file.path + ) == utils.md5_for_file_hex(filename=recased_file_path) + + async def test_redownload_after_casing_change_only_dir_on_case_sensitive_filesystem( + self, + mocker: MockerFixture, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + """ + Test the behavior when only the containing directory's casing + changes and the filename itself is untouched. Forcing os.path.normcase + to compare case-sensitively makes the directory-match miss, but the + fallback loop still finds the entry unmodified -- so this behaves the + same as the both-dir-and-file variant: no download occurs, and the + cached content is copied into the recased directory under a + keep-both disambiguated name. + """ + spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") + + # GIVEN a file stored in Synapse + original_file_path = utils.make_bogus_data_file() + file = await File( + path=original_file_path, parent_id=project_model.id + ).store_async(synapse_client=syn) + schedule_for_cleanup(file.id) + + assert syn.cache.contains( + file_handle_id=file.file_handle.id, path=original_file_path + ) + + # WHEN only the containing directory is re-cased, with no change to the filename or content + recased_file_path = os.path.join( + os.path.dirname(original_file_path).upper(), + os.path.basename(original_file_path), + ) + os.rename(original_file_path, recased_file_path) + schedule_for_cleanup(recased_file_path) + + with ( + patch("os.path.normcase", side_effect=lambda p: p), + patch( + "synapseclient.api.entity_factory.download_file_entity_model", + wraps=spy_for_async_function( + download_functions.download_file_entity_model + ), + ) as spy_file_entity, + ): + redownloaded_file = await File( + id=file.id, path=os.path.dirname(recased_file_path) + ).get_async(synapse_client=syn) + + # THEN no download occurs since cache.get() returns the cached file path and the file is unmodified + spy_file_entity.assert_called_once() + spy_file_handle.assert_not_called() + # AND the cached content is copied into the requested (recased) directory -- + # under a disambiguated name, since the directory string matches the cached + # entry's directory but file name is different so a collision resolution is triggered. + assert utils.equal_paths( + os.path.dirname(redownloaded_file.path), os.path.dirname(recased_file_path) + ) + assert utils.md5_for_file_hex( + filename=redownloaded_file.path + ) == utils.md5_for_file_hex(filename=recased_file_path) + + async def test_redownload_after_casing_change_only_dir_on_case_insensitive_filesystem( + self, + mocker: MockerFixture, + syn: Synapse, + project_model: Project, + schedule_for_cleanup: Callable[..., None], + ) -> None: + """ + Contrast with the case-sensitive variant: with str.lower-folded + normcase, the directory-match succeeds directly (rather than via the + fallback loop), but the resulting download_path lands back on the exact + cached path so no download occurs. No download occurs. + """ + spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") + + # GIVEN a file stored in Synapse + original_file_path = utils.make_bogus_data_file() + file = await File( + path=original_file_path, parent_id=project_model.id + ).store_async(synapse_client=syn) + schedule_for_cleanup(file.id) + + assert syn.cache.contains( + file_handle_id=file.file_handle.id, path=original_file_path + ) + + recased_file_path = os.path.join( + os.path.dirname(original_file_path).upper(), + os.path.basename(original_file_path), + ) + os.rename(original_file_path, recased_file_path) + schedule_for_cleanup(recased_file_path) + + with ( + patch("os.path.normcase", side_effect=str.lower), + patch( + "synapseclient.api.entity_factory.download_file_entity_model", + wraps=spy_for_async_function( + download_functions.download_file_entity_model + ), + ) as spy_file_entity, + ): + redownloaded_file = await File( + id=file.id, path=os.path.dirname(recased_file_path) + ).get_async(synapse_client=syn) + + # THEN no download occurs since cache.get() returns the cached file path as the file is unmodified + # and downloaded filename is extracted from the file handle so the redownloaded file path is the original file path. + spy_file_entity.assert_called_once() + spy_file_handle.assert_not_called() + + assert utils.equal_paths( + os.path.dirname(redownloaded_file.path), os.path.dirname(recased_file_path) + ) + assert utils.md5_for_file_hex( + filename=redownloaded_file.path + ) == utils.md5_for_file_hex(filename=recased_file_path) + class TestDownloadFromUrl: """Integration tests that will route through From 99030ee12d2d0005890eab069e5d699e56f8cf03 Mon Sep 17 00:00:00 2001 From: danlu1 Date: Wed, 29 Jul 2026 12:14:32 -0700 Subject: [PATCH 06/12] compare path in a platform-neutral way --- tests/unit/synapseclient/core/unit_test_Cache.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/synapseclient/core/unit_test_Cache.py b/tests/unit/synapseclient/core/unit_test_Cache.py index 240239b00..2fabb80bc 100644 --- a/tests/unit/synapseclient/core/unit_test_Cache.py +++ b/tests/unit/synapseclient/core/unit_test_Cache.py @@ -129,13 +129,13 @@ def test_subsecond_timestamps(): _read_cache_map_mock.return_value = {path: "2015-05-05T21:34:55.001Z"} _get_modified_time_mock.return_value = 1430861695.001111 - assert path == my_cache.get(file_handle_id=1234, path=path) + assert utils.equal_paths(path, my_cache.get(file_handle_id=1234, path=path)) # The R client always writes .000 for milliseconds, for compatibility, # we should match .000 with any number of milliseconds _read_cache_map_mock.return_value = {path: "2015-05-05T21:34:55.000Z"} - assert path == my_cache.get(file_handle_id=1234, path=path) + assert utils.equal_paths(path, my_cache.get(file_handle_id=1234, path=path)) def test_unparseable_cache_map(): From 48b77b212d04e58d511631c4cfe659c8ac0f21d8 Mon Sep 17 00:00:00 2001 From: danlu1 Date: Thu, 30 Jul 2026 10:40:16 -0700 Subject: [PATCH 07/12] reformat --- .../synapseclient/core/unit_test_Cache.py | 218 +++++++++++++++--- 1 file changed, 181 insertions(+), 37 deletions(-) diff --git a/tests/unit/synapseclient/core/unit_test_Cache.py b/tests/unit/synapseclient/core/unit_test_Cache.py index 2fabb80bc..1ddd09acc 100644 --- a/tests/unit/synapseclient/core/unit_test_Cache.py +++ b/tests/unit/synapseclient/core/unit_test_Cache.py @@ -2,6 +2,7 @@ import json import math import os +import platform import random import re import tempfile @@ -259,10 +260,15 @@ def test_match_cache_map_key(): ) -def test_match_cache_map_key_casing_change(): +@pytest.mark.skipif( + platform.system() == "Windows", + reason="Simulates non-Windows (case-sensitive) path normalization.", +) +def test_match_cache_map_key_casing_change_non_windows(): """ If Synapse changes a file's name casing and the user matches it locally, - the cache falls back to case-insensitive matching on Windows, but not POSIX. + the cache does NOT fall back to case-insensitive matching on a non-Windows + (case-sensitive) platform. """ cache_map = { "/Users/me/data/File.csv": { @@ -277,25 +283,50 @@ def test_match_cache_map_key_casing_change(): == "/Users/me/data/File.csv" ) - # on POSIX assert cache._match_cache_map_key(cache_map, "/Users/me/data/file.csv") is None - # on Windows - with patch("os.path.normcase", side_effect=str.lower): - assert ( - cache._match_cache_map_key(cache_map, "/Users/me/data/file.csv") - == "/Users/me/data/File.csv" - ) - assert ( - cache._match_cache_map_key(cache_map, "/USERS/ME/DATA/FILE.CSV") - == "/Users/me/data/File.csv" - ) +@pytest.mark.skipif( + platform.system() != "Windows", + reason="Simulates Windows (case-insensitive) path normalization.", +) +def test_match_cache_map_key_casing_change_windows(): + """ + If Synapse changes a file's name casing and the user matches it locally, + the cache falls back to case-insensitive matching on a Windows + (case-insensitive) platform. + """ + cache_map = { + "/Users/me/data/File.csv": { + "modified_time": "2020-01-01T00:00:00.000Z", + "content_md5": "abc123", + } + } -def test_cache_contains_after_casing_change_locally(): + # exact case always matches directly, no fallback needed + assert ( + cache._match_cache_map_key(cache_map, "/Users/me/data/File.csv") + == "/Users/me/data/File.csv" + ) + + assert ( + cache._match_cache_map_key(cache_map, "/Users/me/data/file.csv") + == "/Users/me/data/File.csv" + ) + assert ( + cache._match_cache_map_key(cache_map, "/USERS/ME/DATA/FILE.CSV") + == "/Users/me/data/File.csv" + ) + + +@pytest.mark.skipif( + platform.system() == "Windows", + reason="Simulates non-Windows (case-sensitive) path normalization.", +) +def test_cache_contains_after_casing_change_locally_non_windows(): """ - A casing-only local rename should be a miss on POSIX (os.path.normcase is a - no-op) but a hit when simulating Windows case-insensitive matching. + A casing-only local rename should be a miss on non-Windows (case-sensitive, + no-op normcase) platforms. """ tmp_dir = tempfile.mkdtemp() my_cache = cache.Cache(cache_root_dir=tmp_dir) @@ -311,17 +342,44 @@ def test_cache_contains_after_casing_change_locally(): recased_path = os.path.join(os.path.dirname(original_path), "file.csv") os.rename(original_path, recased_path) - # on POSIX, os.path.normcase is a no-op so casing-only changes are a miss assert not my_cache.contains(file_handle_id=file_handle_id, path=recased_path) - with patch("os.path.normcase", side_effect=str.lower): - assert my_cache.contains(file_handle_id=file_handle_id, path=recased_path) +@pytest.mark.skipif( + platform.system() != "Windows", + reason="Simulates Windows (case-insensitive) path normalization.", +) +def test_cache_contains_after_casing_change_locally_windows(): + """ + A casing-only local rename should still be a hit on a Windows + (case-insensitive) platform. + """ + tmp_dir = tempfile.mkdtemp() + my_cache = cache.Cache(cache_root_dir=tmp_dir) + file_handle_id = 101201 + + original_path = utils.touch( + os.path.join(my_cache.get_cache_dir(file_handle_id), "File.csv") + ) + my_cache.add(file_handle_id=file_handle_id, path=original_path) + + # user renames their local file to match Synapse's re-cased file name; + # only the casing of the base name changes + recased_path = os.path.join(os.path.dirname(original_path), "file.csv") + os.rename(original_path, recased_path) + + assert my_cache.contains(file_handle_id=file_handle_id, path=recased_path) -def test_get_matches_legacy_lowercased_key(): + +@pytest.mark.skipif( + platform.system() == "Windows", + reason="Simulates non-Windows (case-sensitive) path normalization.", +) +def test_get_match_legacy_lowercased_key_non_windows(): """ Test that a cache map written by an older Windows client stored lowercased - keys still hits those legacy entries. + keys does hit those legacy entries when on a non-Windows (case-sensitive) + platform since fall back to modified time comparison. """ tmp_dir = tempfile.mkdtemp() my_cache = cache.Cache(cache_root_dir=tmp_dir) @@ -339,17 +397,53 @@ def test_get_matches_legacy_lowercased_key(): cache_map[legacy_key] = entry my_cache._write_cache_map(cache_dir, cache_map) rewritten_cache_map = my_cache._read_cache_map(cache_dir) + assert legacy_key in rewritten_cache_map assert normalized not in rewritten_cache_map - with patch("os.path.normcase", side_effect=str.lower): - assert my_cache.get(file_handle_id=101201, path=path) == path + assert my_cache.get(file_handle_id=101201, path=path) == legacy_key + + +@pytest.mark.skipif( + platform.system() != "Windows", + reason="Simulates Windows (case-insensitive) path normalization.", +) +def test_get_matches_legacy_lowercased_key_windows(): + """ + Test that a cache map written by an older Windows client stored lowercased + keys still hits those legacy entries when on a Windows (case-insensitive) + platform. + """ + tmp_dir = tempfile.mkdtemp() + my_cache = cache.Cache(cache_root_dir=tmp_dir) + cache_dir = my_cache.get_cache_dir(101201) + + path = utils.touch(os.path.join(cache_dir, "Mixed_Case.ext")) + my_cache.add(file_handle_id=101201, path=path) + + normalized = utils.normalize_path(path) + legacy_key = normalized.lower() + assert legacy_key != normalized + + cache_map = my_cache._read_cache_map(cache_dir) + entry = cache_map.pop(normalized) + cache_map[legacy_key] = entry + my_cache._write_cache_map(cache_dir, cache_map) + rewritten_cache_map = my_cache._read_cache_map(cache_dir) + assert legacy_key in rewritten_cache_map + assert normalized not in rewritten_cache_map + + assert my_cache.get(file_handle_id=101201, path=path) == legacy_key -def test_contains_matches_legacy_lowercased_key(): +@pytest.mark.skipif( + platform.system() == "Windows", + reason="Simulates non-Windows (case-sensitive) path normalization.", +) +def test_contains_does_not_match_legacy_lowercased_key_non_windows(): """ - Test that contains() can still match a legacy lowercased cache-map key when - simulating Windows case-insensitive path normalization. + Test that contains() does NOT match a legacy lowercased cache-map key + when on a non-Windows (case-sensitive, no-op normcase) platform. """ tmp_dir = tempfile.mkdtemp() my_cache = cache.Cache(cache_root_dir=tmp_dir) @@ -367,18 +461,45 @@ def test_contains_matches_legacy_lowercased_key(): cache_map[legacy_key] = entry my_cache._write_cache_map(cache_dir, cache_map) - # POSIX normcase is a no-op, so this mixed-case lookup misses. assert not my_cache.contains(file_handle_id=101201, path=path) - # Simulate Windows behavior where lowercased legacy keys are still matched. - with patch("os.path.normcase", side_effect=str.lower): - assert my_cache.contains(file_handle_id=101201, path=path) +@pytest.mark.skipif( + platform.system() != "Windows", + reason="Simulates non-Windows (case-sensitive) path normalization.", +) +def test_contains_matches_legacy_lowercased_key_windows(): + """ + Test that contains() can still match a legacy lowercased cache-map key + when on a Windows (case-insensitive) platform. + """ + tmp_dir = tempfile.mkdtemp() + my_cache = cache.Cache(cache_root_dir=tmp_dir) + cache_dir = my_cache.get_cache_dir(101201) + + path = utils.touch(os.path.join(cache_dir, "Mixed_Case.ext")) + my_cache.add(file_handle_id=101201, path=path) -def test_remove_matches_legacy_lowercased_key(): + normalized = utils.normalize_path(path) + legacy_key = normalized.lower() + assert legacy_key != normalized + + cache_map = my_cache._read_cache_map(cache_dir) + entry = cache_map.pop(normalized) + cache_map[legacy_key] = entry + my_cache._write_cache_map(cache_dir, cache_map) + + assert my_cache.contains(file_handle_id=101201, path=path) + + +@pytest.mark.skipif( + platform.system() == "Windows", + reason="Simulates non-Windows (case-sensitive) path normalization.", +) +def test_remove_does_not_match_legacy_lowercased_key_non_windows(): """ - Test that remove() can resolve and remove a legacy lowercased cache-map key - when simulating Windows case-insensitive path normalization. + Test that remove() does NOT remove a legacy lowercased cache-map key + when on a non-Windows (case-sensitive, no-op normcase) platform. """ tmp_dir = tempfile.mkdtemp() my_cache = cache.Cache(cache_root_dir=tmp_dir) @@ -396,12 +517,35 @@ def test_remove_matches_legacy_lowercased_key(): cache_map[legacy_key] = entry my_cache._write_cache_map(cache_dir, cache_map) - # POSIX normcase is a no-op, so this mixed-case lookup misses. assert my_cache.remove(file_handle_id=101201, path=path) == [] - # Simulate Windows behavior: remove should match and delete the legacy key. - with patch("os.path.normcase", side_effect=str.lower): - removed = my_cache.remove(file_handle_id=101201, path=path) + +@pytest.mark.skipif( + platform.system() != "Windows", + reason="Simulates Windows (case-insensitive) path normalization.", +) +def test_remove_matches_legacy_lowercased_key_windows(): + """ + Test that remove() can resolve and remove a legacy lowercased cache-map key + when on a Windows (case-insensitive) platform. + """ + tmp_dir = tempfile.mkdtemp() + my_cache = cache.Cache(cache_root_dir=tmp_dir) + cache_dir = my_cache.get_cache_dir(101201) + + path = utils.touch(os.path.join(cache_dir, "Mixed_Case.ext")) + my_cache.add(file_handle_id=101201, path=path) + + normalized = utils.normalize_path(path) + legacy_key = normalized.lower() + assert legacy_key != normalized + + cache_map = my_cache._read_cache_map(cache_dir) + entry = cache_map.pop(normalized) + cache_map[legacy_key] = entry + my_cache._write_cache_map(cache_dir, cache_map) + + removed = my_cache.remove(file_handle_id=101201, path=path) assert removed == [legacy_key] assert my_cache._read_cache_map(cache_dir) == {} From 5a63c8d78d32ef32bbc1c972c6f011d140689fe7 Mon Sep 17 00:00:00 2001 From: danlu1 Date: Thu, 30 Jul 2026 15:19:19 -0700 Subject: [PATCH 08/12] reformat --- .../synapseclient/core/test_download.py | 487 ------------------ .../synapseclient/core/test_upload.py | 404 --------------- .../synapseclient/core/unit_test_Cache.py | 9 +- 3 files changed, 5 insertions(+), 895 deletions(-) delete mode 100644 tests/integration/synapseclient/core/test_upload.py diff --git a/tests/integration/synapseclient/core/test_download.py b/tests/integration/synapseclient/core/test_download.py index 85e43de8f..f32fdb890 100644 --- a/tests/integration/synapseclient/core/test_download.py +++ b/tests/integration/synapseclient/core/test_download.py @@ -233,493 +233,6 @@ async def test_download_cached_file_to_new_directory( # AND download_file_entity_model was called spy_file_entity.assert_called_once() - async def test_redownload_after_local_rename_same_content( - self, - mocker: MockerFixture, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - """ - Contrast with the upload-side equivalent, Cache.get() has no equivalent - md5 fallback for downloads -- it only ever matches by recorded path, so - renaming a cached file, even within the same directory, invalidates - the stale cache entry and forces a real re-download. - """ - spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") - - # GIVEN a file stored in Synapse - original_file_path = utils.make_bogus_data_file() - file = await File( - path=original_file_path, parent_id=project_model.id - ).store_async(synapse_client=syn) - schedule_for_cleanup(file.id) - - # AND the file is in the cache under its original path - assert syn.cache.contains( - file_handle_id=file.file_handle.id, path=original_file_path - ) - - # WHEN the cached file is renamed, with no change to its content - renamed_path = f"{original_file_path}.renamed" - os.rename(original_file_path, renamed_path) - schedule_for_cleanup(renamed_path) - - redownloaded_file = await File( - id=file.id, path=os.path.dirname(renamed_path) - ).get_async(synapse_client=syn) - - # THEN a real download occurs since the cache entry pointing to the old path (no longer exists) is removed - spy_file_handle.assert_called_once() - assert utils.equal_paths(original_file_path, redownloaded_file.path) - assert utils.md5_for_file_hex( - filename=redownloaded_file.path - ) == utils.md5_for_file_hex(filename=renamed_path) - - async def test_redownload_after_moved_to_different_directory_same_content_no_explicit_destination( - self, - mocker: MockerFixture, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - """ - File is moved to an entirely different directory instead of renamed in place, - and no explicit download path is given. - The cache can't be retrieved since the cached file is no longer accessible and can't determine if it is unmodified. - So cache.get() returns None and a real download occurs. - """ - spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") - - # GIVEN a file stored in Synapse - original_file_path = utils.make_bogus_data_file() - file = await File( - path=original_file_path, parent_id=project_model.id - ).store_async(synapse_client=syn) - schedule_for_cleanup(file.id) - synapse_cache_location = syn.cache.get_cache_dir( - file_handle_id=file.data_file_handle_id - ) - - # AND the file is in the cache under its original path - assert syn.cache.contains( - file_handle_id=file.file_handle.id, path=original_file_path - ) - - # WHEN the cached file is moved to a different directory, with no change to its content - other_dir = tempfile.mkdtemp() - schedule_for_cleanup(other_dir) - moved_path = shutil.move( - original_file_path, - os.path.join(other_dir, os.path.basename(original_file_path)), - ) - - # when no explicit destination is given, the file is downloaded to the synapse cache location - redownloaded_file = await File(id=file.id).get_async(synapse_client=syn) - - # THEN a real download occurs. - spy_file_handle.assert_called_once() - assert utils.equal_paths( - os.path.dirname(redownloaded_file.path), synapse_cache_location - ) - assert utils.md5_for_file_hex( - filename=redownloaded_file.path - ) == utils.md5_for_file_hex(filename=moved_path) - - async def test_redownload_after_moved_to_different_directory_same_content_with_explicit_destination( - self, - mocker: MockerFixture, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - """ - File is moved to an entirely different directory instead of renamed in place, - and no explicit download path is given. - The cache can't be retrieved since the cached file is no longer accessible and can't determine if it is unmodified. - So cache.get() returns None and a real download occurs. - """ - spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") - - # GIVEN a file stored in Synapse - original_file_path = utils.make_bogus_data_file() - file = await File( - path=original_file_path, parent_id=project_model.id - ).store_async(synapse_client=syn) - schedule_for_cleanup(file.id) - - # AND the file is in the cache under its original path - assert syn.cache.contains( - file_handle_id=file.file_handle.id, path=original_file_path - ) - - # WHEN the cached file is moved to a different directory, with no change to its content - other_dir = tempfile.mkdtemp() - schedule_for_cleanup(other_dir) - moved_path = shutil.move( - original_file_path, - os.path.join(other_dir, os.path.basename(original_file_path)), - ) - - redownloaded_file = await File( - id=file.id, path=os.path.dirname(original_file_path) - ).get_async(synapse_client=syn) - - # THEN a real download occurs. - spy_file_handle.assert_called_once() - assert utils.equal_paths( - os.path.dirname(redownloaded_file.path), os.path.dirname(original_file_path) - ) - assert utils.md5_for_file_hex( - filename=redownloaded_file.path - ) == utils.md5_for_file_hex(filename=moved_path) - - async def test_redownload_after_casing_change_both_dir_file_on_case_sensitive_filesystem( - self, - mocker: MockerFixture, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - """ - Test the behavior when both the containing directory and the filename - are re-cased. Forcing os.path.normcase to compare case-sensitively - makes Cache.get()'s directory-match branch miss, but the fallback - loop still finds the entry unmodified, so no download occurs. - """ - spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") - - # GIVEN a file stored in Synapse - original_file_path = utils.make_bogus_data_file() - file = await File( - path=original_file_path, parent_id=project_model.id - ).store_async(synapse_client=syn) - schedule_for_cleanup(file.id) - - assert syn.cache.contains( - file_handle_id=file.file_handle.id, path=original_file_path - ) - - # WHEN both the containing directory and the filename are re-cased with no change to content - recased_file_path = original_file_path.upper() - os.rename(original_file_path, recased_file_path) - schedule_for_cleanup(recased_file_path) - - with ( - patch("os.path.normcase", side_effect=lambda p: p), - patch( - "synapseclient.api.entity_factory.download_file_entity_model", - wraps=spy_for_async_function( - download_functions.download_file_entity_model - ), - ) as spy_file_entity, - ): - redownloaded_file = await File( - id=file.id, path=os.path.dirname(recased_file_path) - ).get_async(synapse_client=syn) - - # THEN no download occurs since cache.get() returns the cached file path and the file is unmodified - spy_file_entity.assert_called_once() - spy_file_handle.assert_not_called() - # AND the cached content is copied into the requested (recased) directory - assert utils.equal_paths( - os.path.dirname(redownloaded_file.path), os.path.dirname(recased_file_path) - ) - assert utils.md5_for_file_hex( - filename=redownloaded_file.path - ) == utils.md5_for_file_hex(filename=recased_file_path) - - async def test_redownload_after_casing_change_only_file_on_case_sensitive_filesystem( - self, - mocker: MockerFixture, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - """ - Test the behavior when only the filename's casing changes and the - containing directory is untouched. Since the requested directory - string is identical to the cached entry's directory, Cache.get()'s - directory-match succeeds, and the resolved download_path lands back - on the exact cached path so no download occurs. No download occurs. - """ - spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") - - # GIVEN a file stored in Synapse - original_file_path = utils.make_bogus_data_file() - file = await File( - path=original_file_path, parent_id=project_model.id - ).store_async(synapse_client=syn) - schedule_for_cleanup(file.id) - - assert syn.cache.contains( - file_handle_id=file.file_handle.id, path=original_file_path - ) - - recased_file_path = os.path.join( - os.path.dirname(original_file_path), - os.path.basename(original_file_path).upper(), - ) - os.rename(original_file_path, recased_file_path) - schedule_for_cleanup(recased_file_path) - - with ( - patch("os.path.normcase", side_effect=lambda p: p), - patch( - "synapseclient.api.entity_factory.download_file_entity_model", - wraps=spy_for_async_function( - download_functions.download_file_entity_model - ), - ) as spy_file_entity, - ): - redownloaded_file = await File( - id=file.id, path=os.path.dirname(recased_file_path) - ).get_async(synapse_client=syn) - - # THEN no download occurs since cache.get() returns the cached file path and the file is unmodified - # and downloaded filename is extracted from the os.path.basename(cached_file_path) - # so the redownloaded file path is the original file path. - spy_file_entity.assert_called_once() - spy_file_handle.assert_not_called() - assert utils.equal_paths(redownloaded_file.path, original_file_path) - assert utils.md5_for_file_hex( - filename=redownloaded_file.path - ) == utils.md5_for_file_hex(filename=recased_file_path) - - async def test_redownload_after_casing_change_both_dir_file_on_case_insensitive_filesystem( - self, - mocker: MockerFixture, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - """ - Contrast with the case-sensitive variant: on a case-insensitive - filesystem, Cache.get()'s directory-match compares paths via the - real (unpatched) os.path.normcase, so a recased directory is still - recognized as the same directory. No download occurs and and the - cached content is copied into the recased directory under a - keep-both disambiguated name. - """ - spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") - - # GIVEN a file stored in Synapse - original_file_path = utils.make_bogus_data_file() - file = await File( - path=original_file_path, parent_id=project_model.id - ).store_async(synapse_client=syn) - schedule_for_cleanup(file.id) - - assert syn.cache.contains( - file_handle_id=file.file_handle.id, path=original_file_path - ) - - # WHEN both the containing directory and the filename are re-cased with no change to content - recased_file_path = original_file_path.upper() - os.rename(original_file_path, recased_file_path) - schedule_for_cleanup(recased_file_path) - - with ( - patch("os.path.normcase", side_effect=str.lower), - patch( - "synapseclient.api.entity_factory.download_file_entity_model", - wraps=spy_for_async_function( - download_functions.download_file_entity_model - ), - ) as spy_file_entity, - ): - redownloaded_file = await File( - id=file.id, path=os.path.dirname(recased_file_path) - ).get_async(synapse_client=syn) - - # THEN no download occurs since cache.get() returns the cached file path and the file is unmodified - spy_file_entity.assert_called_once() - spy_file_handle.assert_not_called() - # AND the cached content is copied into the requested (recased) directory -- - # under a disambiguated name, since the directory string matches the cached entry's directory but file name is different - # so a collision resolution is triggered. - assert utils.equal_paths( - os.path.dirname(redownloaded_file.path), os.path.dirname(recased_file_path) - ) - assert utils.md5_for_file_hex( - filename=redownloaded_file.path - ) == utils.md5_for_file_hex(filename=recased_file_path) - - async def test_redownload_after_casing_change_only_file_on_case_insensitive_filesystem( - self, - mocker: MockerFixture, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - """ - Contrast with the case-sensitive variant: with normcase patched to - str.lower instead of forced to identity, the directory-match still - succeeds so no download occurs. - """ - spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") - - # GIVEN a file stored in Synapse - original_file_path = utils.make_bogus_data_file() - file = await File( - path=original_file_path, parent_id=project_model.id - ).store_async(synapse_client=syn) - schedule_for_cleanup(file.id) - - assert syn.cache.contains( - file_handle_id=file.file_handle.id, path=original_file_path - ) - - recased_file_path = os.path.join( - os.path.dirname(original_file_path), - os.path.basename(original_file_path).upper(), - ) - os.rename(original_file_path, recased_file_path) - schedule_for_cleanup(recased_file_path) - - with ( - patch("os.path.normcase", side_effect=str.lower), - patch( - "synapseclient.api.entity_factory.download_file_entity_model", - wraps=spy_for_async_function( - download_functions.download_file_entity_model - ), - ) as spy_file_entity, - ): - redownloaded_file = await File( - id=file.id, path=os.path.dirname(recased_file_path) - ).get_async(synapse_client=syn) - - # THEN no download occurs since cache.get() returns the cached file path as the file is unmodified - # and downloaded filename is extracted from the file handle so the redownloaded file path is the original file path. - spy_file_entity.assert_called_once() - spy_file_handle.assert_not_called() - assert utils.equal_paths(redownloaded_file.path, original_file_path) - assert utils.md5_for_file_hex( - filename=redownloaded_file.path - ) == utils.md5_for_file_hex(filename=recased_file_path) - - async def test_redownload_after_casing_change_only_dir_on_case_sensitive_filesystem( - self, - mocker: MockerFixture, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - """ - Test the behavior when only the containing directory's casing - changes and the filename itself is untouched. Forcing os.path.normcase - to compare case-sensitively makes the directory-match miss, but the - fallback loop still finds the entry unmodified -- so this behaves the - same as the both-dir-and-file variant: no download occurs, and the - cached content is copied into the recased directory under a - keep-both disambiguated name. - """ - spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") - - # GIVEN a file stored in Synapse - original_file_path = utils.make_bogus_data_file() - file = await File( - path=original_file_path, parent_id=project_model.id - ).store_async(synapse_client=syn) - schedule_for_cleanup(file.id) - - assert syn.cache.contains( - file_handle_id=file.file_handle.id, path=original_file_path - ) - - # WHEN only the containing directory is re-cased, with no change to the filename or content - recased_file_path = os.path.join( - os.path.dirname(original_file_path).upper(), - os.path.basename(original_file_path), - ) - os.rename(original_file_path, recased_file_path) - schedule_for_cleanup(recased_file_path) - - with ( - patch("os.path.normcase", side_effect=lambda p: p), - patch( - "synapseclient.api.entity_factory.download_file_entity_model", - wraps=spy_for_async_function( - download_functions.download_file_entity_model - ), - ) as spy_file_entity, - ): - redownloaded_file = await File( - id=file.id, path=os.path.dirname(recased_file_path) - ).get_async(synapse_client=syn) - - # THEN no download occurs since cache.get() returns the cached file path and the file is unmodified - spy_file_entity.assert_called_once() - spy_file_handle.assert_not_called() - # AND the cached content is copied into the requested (recased) directory -- - # under a disambiguated name, since the directory string matches the cached - # entry's directory but file name is different so a collision resolution is triggered. - assert utils.equal_paths( - os.path.dirname(redownloaded_file.path), os.path.dirname(recased_file_path) - ) - assert utils.md5_for_file_hex( - filename=redownloaded_file.path - ) == utils.md5_for_file_hex(filename=recased_file_path) - - async def test_redownload_after_casing_change_only_dir_on_case_insensitive_filesystem( - self, - mocker: MockerFixture, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - """ - Contrast with the case-sensitive variant: with str.lower-folded - normcase, the directory-match succeeds directly (rather than via the - fallback loop), but the resulting download_path lands back on the exact - cached path so no download occurs. No download occurs. - """ - spy_file_handle = mocker.spy(download_functions, "download_by_file_handle") - - # GIVEN a file stored in Synapse - original_file_path = utils.make_bogus_data_file() - file = await File( - path=original_file_path, parent_id=project_model.id - ).store_async(synapse_client=syn) - schedule_for_cleanup(file.id) - - assert syn.cache.contains( - file_handle_id=file.file_handle.id, path=original_file_path - ) - - recased_file_path = os.path.join( - os.path.dirname(original_file_path).upper(), - os.path.basename(original_file_path), - ) - os.rename(original_file_path, recased_file_path) - schedule_for_cleanup(recased_file_path) - - with ( - patch("os.path.normcase", side_effect=str.lower), - patch( - "synapseclient.api.entity_factory.download_file_entity_model", - wraps=spy_for_async_function( - download_functions.download_file_entity_model - ), - ) as spy_file_entity, - ): - redownloaded_file = await File( - id=file.id, path=os.path.dirname(recased_file_path) - ).get_async(synapse_client=syn) - - # THEN no download occurs since cache.get() returns the cached file path as the file is unmodified - # and downloaded filename is extracted from the file handle so the redownloaded file path is the original file path. - spy_file_entity.assert_called_once() - spy_file_handle.assert_not_called() - - assert utils.equal_paths( - os.path.dirname(redownloaded_file.path), os.path.dirname(recased_file_path) - ) - assert utils.md5_for_file_hex( - filename=redownloaded_file.path - ) == utils.md5_for_file_hex(filename=recased_file_path) - class TestDownloadFromUrl: """Integration tests that will route through diff --git a/tests/integration/synapseclient/core/test_upload.py b/tests/integration/synapseclient/core/test_upload.py deleted file mode 100644 index 5c3bdd697..000000000 --- a/tests/integration/synapseclient/core/test_upload.py +++ /dev/null @@ -1,404 +0,0 @@ -"""Integration tests for uploading files, focused on avoiding redundant re-uploads via the local disk cache and MD5 comparison.""" - -import os -import shutil -import sys -import tempfile -import uuid -from typing import Callable -from unittest.mock import patch - -import pytest -from pytest_mock import MockerFixture - -from synapseclient import Synapse -from synapseclient.core import utils -from synapseclient.models import File, Project - -DESCRIPTION = "This is an example file." -CONTENT_TYPE = "text/plain" -VERSION_COMMENT = "My version comment" - - -class TestFileStoreAvoidsRedundantUpload: - """ - Tests that File.store_async avoids re-uploading file content that has - not changed, even when the local path no longer matches what's recorded - in the local disk cache (renamed, moved, or re-cased). In those cases - `Cache.contains()` (synapseclient/core/cache.py) misses because it only - matches by path, and it's the MD5 comparison against what Synapse - already has on record (`_needs_upload` in synapseclient/models/file.py) - that actually prevents the redundant upload. - """ - - async def test_reupload_after_local_rename_same_content( - self, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - # Given a file stored in Synapse - original_path = utils.make_bogus_uuid_file() - schedule_for_cleanup(original_path) - file = await File( - path=original_path, - description=DESCRIPTION, - content_type=CONTENT_TYPE, - version_comment=VERSION_COMMENT, - version_label=str(uuid.uuid4()), - ).store_async(parent=project_model, synapse_client=syn) - schedule_for_cleanup(file.id) - before_file_handle_id = file.file_handle.id - before_version_number = file.version_number - before_file_handle_file_name = file.file_handle.file_name - - # WHEN the local file is renamed, with no change to its content - renamed_path = os.path.join( - os.path.dirname(original_path), f"renamed_{uuid.uuid4()}.txt" - ) - os.rename(original_path, renamed_path) - schedule_for_cleanup(renamed_path) - file.path = renamed_path - - with patch( - "synapseclient.models.file.upload_file_handle" - ) as mocked_upload_file_handle: - new_file = await file.store_async(synapse_client=syn) - - # THEN the already-uploaded file handle is reused - assert not mocked_upload_file_handle.called - assert new_file.file_handle.id == before_file_handle_id - assert new_file.version_number == before_version_number - # AND the file handle's saved filename still reflects the original - # upload, not the local rename -- Synapse never saw the new name - assert new_file.file_handle.file_name == before_file_handle_file_name - - async def test_reupload_from_different_directory_same_content( - self, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - # Given a file stored in Synapse - original_path = utils.make_bogus_uuid_file() - schedule_for_cleanup(original_path) - file = await File( - path=original_path, - description=DESCRIPTION, - content_type=CONTENT_TYPE, - version_comment=VERSION_COMMENT, - version_label=str(uuid.uuid4()), - ).store_async(parent=project_model, synapse_client=syn) - schedule_for_cleanup(file.id) - before_file_handle_id = file.file_handle.id - before_version_number = file.version_number - before_file_handle_file_name = file.file_handle.file_name - - # WHEN the same content is re-stored from a copy of the file in a different directory - other_dir = tempfile.mkdtemp() - schedule_for_cleanup(other_dir) - copied_path = os.path.join(other_dir, os.path.basename(original_path)) - shutil.copyfile(original_path, copied_path) - file.path = copied_path - - with patch( - "synapseclient.models.file.upload_file_handle" - ) as mocked_upload_file_handle: - new_file = await file.store_async(synapse_client=syn) - - # THEN no redundant upload occurs because the MD5 match - assert not mocked_upload_file_handle.called - assert new_file.file_handle.id == before_file_handle_id - assert new_file.version_number == before_version_number - # AND the file handle's saved filename is unchanged - assert new_file.file_handle.file_name == before_file_handle_file_name - - @pytest.mark.skipif( - sys.platform == "win32", - reason=( - "Windows filesystems are case-insensitive, so os.path.normcase " - "folds this casing-only rename into an exact cache hit instead " - "of exercising the POSIX MD5-fallback path this test targets." - ), - ) - async def test_reupload_after_casing_change_same_content( - self, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - # Given a file stored in Synapse - original_path = utils.make_bogus_uuid_file() - schedule_for_cleanup(original_path) - file = await File( - path=original_path, - description=DESCRIPTION, - content_type=CONTENT_TYPE, - version_comment=VERSION_COMMENT, - version_label=str(uuid.uuid4()), - ).store_async(parent=project_model, synapse_client=syn) - schedule_for_cleanup(file.id) - before_file_handle_id = file.file_handle.id - before_version_number = file.version_number - before_file_handle_file_name = file.file_handle.file_name - - # WHEN the local file name is re-cased - recased_path = os.path.join( - os.path.dirname(original_path), - os.path.basename(original_path).upper(), - ) - os.rename(original_path, recased_path) - schedule_for_cleanup(recased_path) - file.path = recased_path - - with patch( - "synapseclient.models.file.upload_file_handle" - ) as mocked_upload_file_handle: - new_file = await file.store_async(synapse_client=syn) - - # THEN the MD5 comparison against Synapse's record avoids a redundant upload - assert not mocked_upload_file_handle.called - assert new_file.file_handle.id == before_file_handle_id - assert new_file.version_number == before_version_number - # AND the saved filename keeps its original casing -- the re-cased - # local name was never uploaded, so Synapse's record didn't change - assert new_file.file_handle.file_name == before_file_handle_file_name - - async def test_reupload_after_move_and_rename_same_content( - self, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - # GIVEN a file stored in Synapse - original_path = utils.make_bogus_uuid_file() - schedule_for_cleanup(original_path) - file = await File( - path=original_path, - description=DESCRIPTION, - content_type=CONTENT_TYPE, - version_comment=VERSION_COMMENT, - version_label=str(uuid.uuid4()), - ).store_async(parent=project_model, synapse_client=syn) - schedule_for_cleanup(file.id) - before_file_handle_id = file.file_handle.id - before_version_number = file.version_number - before_file_handle_file_name = file.file_handle.file_name - - # WHEN the file is both moved to a new directory AND renamed - other_dir = tempfile.mkdtemp() - schedule_for_cleanup(other_dir) - new_path = os.path.join(other_dir, f"moved_and_renamed_{uuid.uuid4()}.txt") - shutil.move(original_path, new_path) - file.path = new_path - - with patch( - "synapseclient.models.file.upload_file_handle" - ) as mocked_upload_file_handle: - new_file = await file.store_async(synapse_client=syn) - - # THEN no redundant upload occurs because the MD5 match - assert not mocked_upload_file_handle.called - assert new_file.file_handle.id == before_file_handle_id - assert new_file.version_number == before_version_number - # AND the file handle's saved filename is unchanged - assert new_file.file_handle.file_name == before_file_handle_file_name - - @pytest.mark.skipif( - sys.platform == "win32", - reason=( - "Windows filesystems are case-insensitive, so os.path.normcase " - "folds this casing-only rename into an exact cache hit instead " - "of exercising the POSIX MD5-fallback path this test targets." - ), - ) - async def test_reupload_after_casing_change_and_move_same_content( - self, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - # GIVEN a file stored in Synapse - original_path = utils.make_bogus_uuid_file() - schedule_for_cleanup(original_path) - file = await File( - path=original_path, - description=DESCRIPTION, - content_type=CONTENT_TYPE, - version_comment=VERSION_COMMENT, - version_label=str(uuid.uuid4()), - ).store_async(parent=project_model, synapse_client=syn) - schedule_for_cleanup(file.id) - before_file_handle_id = file.file_handle.id - before_version_number = file.version_number - before_file_handle_file_name = file.file_handle.file_name - - # WHEN the file is moved to a new directory AND its name is re-cased - other_dir = tempfile.mkdtemp() - schedule_for_cleanup(other_dir) - new_path = os.path.join(other_dir, os.path.basename(original_path).upper()) - shutil.move(original_path, new_path) - file.path = new_path - - with patch( - "synapseclient.models.file.upload_file_handle" - ) as mocked_upload_file_handle: - new_file = await file.store_async(synapse_client=syn) - - # THEN no redundant upload occurs - assert not mocked_upload_file_handle.called - assert new_file.file_handle.id == before_file_handle_id - assert new_file.version_number == before_version_number - # AND the saved filename keeps its original casing - assert new_file.file_handle.file_name == before_file_handle_file_name - - async def test_reupload_after_rename_with_force_version_false( - self, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - """ - force_version has no bearing on whether _needs_upload decides a - re-upload is needed -- it only affects whether an actual content or - metadata change is written as a new version. Confirms a rename with - unchanged content is just as much a no-op with force_version=False - as it is with the default force_version=True. - """ - # GIVEN a file stored in Synapse - original_path = utils.make_bogus_uuid_file() - schedule_for_cleanup(original_path) - file = await File( - path=original_path, - description=DESCRIPTION, - content_type=CONTENT_TYPE, - version_comment=VERSION_COMMENT, - version_label=str(uuid.uuid4()), - ).store_async(parent=project_model, synapse_client=syn) - schedule_for_cleanup(file.id) - before_file_handle_id = file.file_handle.id - before_version_number = file.version_number - before_file_handle_file_name = file.file_handle.file_name - - # WHEN the local file is renamed, with no change to its content, - # and force_version is explicitly disabled - renamed_path = os.path.join( - os.path.dirname(original_path), f"renamed_{uuid.uuid4()}.txt" - ) - os.rename(original_path, renamed_path) - schedule_for_cleanup(renamed_path) - file.path = renamed_path - file.force_version = False - - with patch( - "synapseclient.models.file.upload_file_handle" - ) as mocked_upload_file_handle: - new_file = await file.store_async(synapse_client=syn) - - # THEN no redundant upload occurs - assert not mocked_upload_file_handle.called - assert new_file.file_handle.id == before_file_handle_id - assert new_file.version_number == before_version_number - # AND the file handle's saved filename is unchanged - assert new_file.file_handle.file_name == before_file_handle_file_name - - async def test_reupload_after_rename_cache_self_heals( - self, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - mocker: MockerFixture, - ) -> None: - """ - Verifies the actual mechanism behind - test_reupload_after_local_rename_same_content - """ - # Given a file stored in Synapse - original_path = utils.make_bogus_uuid_file() - schedule_for_cleanup(original_path) - file = await File( - path=original_path, - description=DESCRIPTION, - content_type=CONTENT_TYPE, - version_comment=VERSION_COMMENT, - version_label=str(uuid.uuid4()), - ).store_async(parent=project_model, synapse_client=syn) - schedule_for_cleanup(file.id) - before_file_handle_file_name = file.file_handle.file_name - - # WHEN the local file is renamed, with no change to its content - renamed_path = os.path.join( - os.path.dirname(original_path), f"renamed_{uuid.uuid4()}.txt" - ) - os.rename(original_path, renamed_path) - schedule_for_cleanup(renamed_path) - file.path = renamed_path - - spy_contains = mocker.spy(syn.cache, "contains") - spy_add = mocker.spy(syn.cache, "add") - new_file = await file.store_async(synapse_client=syn) - - # THEN the renamed path was a genuine miss in the local disk cache... - spy_contains.assert_called_once_with(new_file.file_handle.id, renamed_path) - assert spy_contains.spy_return is False - - # _needs_upload self-healed the cache under the new path - spy_add.assert_called_once() - _, add_kwargs = spy_add.call_args - assert add_kwargs["file_handle_id"] == new_file.file_handle.id - assert add_kwargs["path"] == renamed_path - # AND the MD5-fallback match left the saved filename unchanged - assert new_file.file_handle.file_name == before_file_handle_file_name - - # check the updated cache - spy_contains.reset_mock() - spy_add.reset_mock() - file_after_cache_hit = await file.store_async(synapse_client=syn) - spy_contains.assert_called_once_with( - file_after_cache_hit.file_handle.id, renamed_path - ) - assert spy_contains.spy_return - spy_add.assert_not_called() - # AND the fast cache-hit path also leaves the saved filename unchanged - assert ( - file_after_cache_hit.file_handle.file_name == before_file_handle_file_name - ) - - async def test_reupload_after_rename_with_changed_content_creates_new_version( - self, - syn: Synapse, - project_model: Project, - schedule_for_cleanup: Callable[..., None], - ) -> None: - """ - Confirms only a content change (not just rename) triggers re-upload/version. - """ - # GIVEN a file stored in Synapse - original_path = utils.make_bogus_uuid_file() - schedule_for_cleanup(original_path) - file = await File( - path=original_path, - description=DESCRIPTION, - content_type=CONTENT_TYPE, - version_comment=VERSION_COMMENT, - version_label=str(uuid.uuid4()), - ).store_async(parent=project_model, synapse_client=syn) - schedule_for_cleanup(file.id) - before_file_handle_id = file.file_handle.id - before_version_number = file.version_number - - # WHEN the local file is renamed AND its content is changed - renamed_path = os.path.join( - os.path.dirname(original_path), f"renamed_{uuid.uuid4()}.txt" - ) - os.rename(original_path, renamed_path) - schedule_for_cleanup(renamed_path) - with open(renamed_path, "w") as f: - f.write("this content is different from what was uploaded") - file.path = renamed_path - - new_file = await file.store_async(synapse_client=syn) - - # THEN a new file handle and version are created - assert new_file.file_handle.id != before_file_handle_id - assert new_file.version_number > before_version_number diff --git a/tests/unit/synapseclient/core/unit_test_Cache.py b/tests/unit/synapseclient/core/unit_test_Cache.py index 1ddd09acc..cd1dbe637 100644 --- a/tests/unit/synapseclient/core/unit_test_Cache.py +++ b/tests/unit/synapseclient/core/unit_test_Cache.py @@ -375,11 +375,11 @@ def test_cache_contains_after_casing_change_locally_windows(): platform.system() == "Windows", reason="Simulates non-Windows (case-sensitive) path normalization.", ) -def test_get_match_legacy_lowercased_key_non_windows(): +def test_get_does_not_match_legacy_lowercased_key_non_windows(): """ Test that a cache map written by an older Windows client stored lowercased - keys does hit those legacy entries when on a non-Windows (case-sensitive) - platform since fall back to modified time comparison. + keys does NOT hit those legacy entries when on a genuinely case-sensitive + platform. """ tmp_dir = tempfile.mkdtemp() my_cache = cache.Cache(cache_root_dir=tmp_dir) @@ -401,6 +401,7 @@ def test_get_match_legacy_lowercased_key_non_windows(): assert legacy_key in rewritten_cache_map assert normalized not in rewritten_cache_map + # cache misses but falls back to modified time comparison assert my_cache.get(file_handle_id=101201, path=path) == legacy_key @@ -433,7 +434,7 @@ def test_get_matches_legacy_lowercased_key_windows(): assert legacy_key in rewritten_cache_map assert normalized not in rewritten_cache_map - assert my_cache.get(file_handle_id=101201, path=path) == legacy_key + assert my_cache.get(file_handle_id=101201, path=path) == path @pytest.mark.skipif( From d5202739f7568f1d9b5e3f4f75c7668ccec2a93f Mon Sep 17 00:00:00 2001 From: danlu1 Date: Thu, 30 Jul 2026 16:23:00 -0700 Subject: [PATCH 09/12] distinguish non-windows os macos and linux since APFS volume is case-insensitive --- .../synapseclient/core/unit_test_Cache.py | 62 +++++++++++++++++-- 1 file changed, 56 insertions(+), 6 deletions(-) diff --git a/tests/unit/synapseclient/core/unit_test_Cache.py b/tests/unit/synapseclient/core/unit_test_Cache.py index cd1dbe637..0dec8b77d 100644 --- a/tests/unit/synapseclient/core/unit_test_Cache.py +++ b/tests/unit/synapseclient/core/unit_test_Cache.py @@ -372,14 +372,20 @@ def test_cache_contains_after_casing_change_locally_windows(): @pytest.mark.skipif( - platform.system() == "Windows", - reason="Simulates non-Windows (case-sensitive) path normalization.", + platform.system() != "Darwin", + reason=( + "Simulates macOS, where the default APFS volume is case-insensitive " + "like Windows/NTFS, so get()'s modified-time fallback can still " + "resolve a legacy lowercased key." + ), ) -def test_get_does_not_match_legacy_lowercased_key_non_windows(): +def test_get_matches_legacy_lowercased_key_macos(): """ Test that a cache map written by an older Windows client stored lowercased - keys does NOT hit those legacy entries when on a genuinely case-sensitive - platform. + keys still hits those legacy entries on macOS. get()'s final "most + recently cached" fallback stats the cache-map key itself (the lowercased + legacy path); on the default case-insensitive APFS volume that resolves + to the real file, so the modified-time comparison succeeds. """ tmp_dir = tempfile.mkdtemp() my_cache = cache.Cache(cache_root_dir=tmp_dir) @@ -401,10 +407,54 @@ def test_get_does_not_match_legacy_lowercased_key_non_windows(): assert legacy_key in rewritten_cache_map assert normalized not in rewritten_cache_map - # cache misses but falls back to modified time comparison + # cache misses on the exact/normcase lookup but falls back to the + # case-insensitive filesystem's modified time comparison assert my_cache.get(file_handle_id=101201, path=path) == legacy_key +@pytest.mark.skipif( + platform.system() != "Linux", + reason=( + "Simulates Linux, where ext4 is genuinely case-sensitive, so " + "get()'s modified-time fallback cannot resolve a legacy lowercased " + "key -- no file exists on disk at that lowercased path." + ), +) +def test_get_does_not_match_legacy_lowercased_key_linux(): + """ + Test that a cache map written by an older Windows client stored lowercased + keys does NOT hit those legacy entries on Linux. Unlike macOS/Windows, + ext4 is case-sensitive, so get()'s final "most recently cached" fallback + can't find a file on disk at the lowercased legacy path, and the + modified-time comparison fails -- consistent with contains() and + remove(), which never match a legacy lowercased key on any non-Windows + platform. + """ + tmp_dir = tempfile.mkdtemp() + my_cache = cache.Cache(cache_root_dir=tmp_dir) + cache_dir = my_cache.get_cache_dir(101201) + + path = utils.touch(os.path.join(cache_dir, "Mixed_Case.ext")) + my_cache.add(file_handle_id=101201, path=path) + + normalized = utils.normalize_path(path) + legacy_key = normalized.lower() + assert legacy_key != normalized + + cache_map = my_cache._read_cache_map(cache_dir) + entry = cache_map.pop(normalized) + cache_map[legacy_key] = entry + my_cache._write_cache_map(cache_dir, cache_map) + rewritten_cache_map = my_cache._read_cache_map(cache_dir) + + assert legacy_key in rewritten_cache_map + assert normalized not in rewritten_cache_map + + # no file exists at the lowercased legacy path, so the modified-time + # fallback can't match it either + assert my_cache.get(file_handle_id=101201, path=path) is None + + @pytest.mark.skipif( platform.system() != "Windows", reason="Simulates Windows (case-insensitive) path normalization.", From 576c8ee01d2a4a7aee9ab9fcd598681a8d564134 Mon Sep 17 00:00:00 2001 From: danlu1 Date: Thu, 30 Jul 2026 16:47:33 -0700 Subject: [PATCH 10/12] fix path format --- tests/unit/synapseclient/core/unit_test_Cache.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/synapseclient/core/unit_test_Cache.py b/tests/unit/synapseclient/core/unit_test_Cache.py index 0dec8b77d..7434e44a0 100644 --- a/tests/unit/synapseclient/core/unit_test_Cache.py +++ b/tests/unit/synapseclient/core/unit_test_Cache.py @@ -484,7 +484,7 @@ def test_get_matches_legacy_lowercased_key_windows(): assert legacy_key in rewritten_cache_map assert normalized not in rewritten_cache_map - assert my_cache.get(file_handle_id=101201, path=path) == path + assert my_cache.get(file_handle_id=101201, path=path) == normalized @pytest.mark.skipif( From 190f566449de861c48471a16652d5a1513f04415 Mon Sep 17 00:00:00 2001 From: danlu1 Date: Thu, 30 Jul 2026 16:52:30 -0700 Subject: [PATCH 11/12] revert changes in test_download --- tests/integration/synapseclient/core/test_download.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/integration/synapseclient/core/test_download.py b/tests/integration/synapseclient/core/test_download.py index f32fdb890..9dce6680e 100644 --- a/tests/integration/synapseclient/core/test_download.py +++ b/tests/integration/synapseclient/core/test_download.py @@ -18,7 +18,6 @@ import synapseclient.core.utils as utils from synapseclient import Synapse from synapseclient.api.file_services import get_file_handle_for_download -from synapseclient.core import cache as cache_module from synapseclient.core.download import ( PresignedUrlInfo, download_from_url, @@ -225,6 +224,8 @@ async def test_download_cached_file_to_new_directory( schedule_for_cleanup(file.path) # THEN the file is not downloaded again, but it copied to the new location + assert os.path.exists(file.path) + assert os.path.exists(original_file_path) assert not utils.equal_paths(original_file_path, file.path) # AND download_by_file_handle was not called From 8e9135da4e51ff27e8a89bd982346a70555b6ac3 Mon Sep 17 00:00:00 2001 From: danlu1 Date: Thu, 30 Jul 2026 18:04:30 -0700 Subject: [PATCH 12/12] update docstring --- tests/unit/synapseclient/core/unit_test_utils.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/unit/synapseclient/core/unit_test_utils.py b/tests/unit/synapseclient/core/unit_test_utils.py index fe358b1d1..6e247407f 100644 --- a/tests/unit/synapseclient/core/unit_test_utils.py +++ b/tests/unit/synapseclient/core/unit_test_utils.py @@ -259,8 +259,9 @@ def test_normalize_path() -> None: def test_normalize_path_preserves_case() -> None: - # SYNR-1534: normalize_path must not lowercase the path (previously it used - # os.path.normcase, which lowercases on Windows and corrupted derived names). + # Normalize_path must not lowercase the path -- it previously + # used os.path.normcase, which lowercases on Windows and corrupted derived + # names. assert utils.normalize_path("SomeDir/File_Name.TSV").endswith( "SomeDir/File_Name.TSV" ) @@ -268,7 +269,7 @@ def test_normalize_path_preserves_case() -> None: def test_guess_file_name_preserves_case() -> None: - # SYNR-1534: the entity name derived from a path must keep its original casing + # The entity name derived from a path must keep its original casing # on every platform, including Windows. assert utils.guess_file_name("/some/path/File_Name.TSV") == "File_Name.TSV" assert (