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"""