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..1358c2d66 100644 --- a/synapseclient/core/cache.py +++ b/synapseclient/core/cache.py @@ -77,6 +77,37 @@ 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 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). + 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. @@ -202,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() @@ -228,7 +258,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) @@ -275,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): @@ -283,10 +315,14 @@ 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(): - 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 +347,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 @@ -335,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 @@ -350,13 +386,18 @@ 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 + # 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 +450,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 efe4671d8..e79442d37 100644 --- a/synapseclient/core/utils.py +++ b/synapseclient/core/utils.py @@ -406,10 +406,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..7434e44a0 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 @@ -129,13 +130,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(): @@ -230,6 +231,409 @@ def test_cache_store_get(): assert my_cache.get(file_handle_id=101202) is None +def test_match_cache_map_key(): + """ + 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. + """ + cache_map = { + "c:/users/me/data/file.csv": {"modified_time": "t"}, + "c:/users/me/data/other.csv": {"modified_time": "t"}, + } + + # 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" + ) + + +@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 does NOT fall back to case-insensitive matching on a non-Windows + (case-sensitive) platform. + """ + 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" + ) + + assert cache._match_cache_map_key(cache_map, "/Users/me/data/file.csv") is None + + +@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", + } + } + + # 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 non-Windows (case-sensitive, + no-op normcase) platforms. + """ + 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 not 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) + + +@pytest.mark.skipif( + 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_matches_legacy_lowercased_key_macos(): + """ + Test that a cache map written by an older Windows client stored lowercased + 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) + 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 + + # 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.", +) +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) == normalized + + +@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() 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) + 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) + + assert not 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) + + 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() 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) + 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) + + assert 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) == {} + + +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() + 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 + + 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 + + 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 bcd08b399..6e247407f 100644 --- a/tests/unit/synapseclient/core/unit_test_utils.py +++ b/tests/unit/synapseclient/core/unit_test_utils.py @@ -258,6 +258,25 @@ def test_normalize_path() -> None: assert utils.normalize_path(None) is None +def test_normalize_path_preserves_case() -> None: + # 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" + ) + assert utils.normalize_path("Mixed\\Case\\Path.CSV").endswith("Mixed/Case/Path.CSV") + + +def test_guess_file_name_preserves_case() -> None: + # 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"""