diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 1b28d3f..906f273 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -185,7 +185,13 @@ Acceptance criteria: `seaf-cli list --json` is used instead of splitting the display columns, which could not represent a library name or a path containing whitespace. `list-remote` still uses the HTTP API rather than the CLI. -- [ ] Reject ambiguous library names and an empty requested library set. +- [x] Reject ambiguous library names and an empty requested library set. + `get_library_id` used to silently return the first library matching a + requested name, even if two libraries shared it; it now raises + `ConfigError` when a name matches more than one library ID. + `resolve_libraries` rejects an empty entry from `LIBRARY_ID` (an empty + string, or one produced by `"a::b"`/a leading or trailing `:`) the same + way. See `tests/test_libraries.py`. ### Runtime hardening diff --git a/dsc/client.py b/dsc/client.py index 206b4da..88c150b 100644 --- a/dsc/client.py +++ b/dsc/client.py @@ -166,10 +166,22 @@ class SeafileClient: _lg.info("Seafile daemon is stopped") def get_library_id(self, library) -> Optional[str]: - for lib_id, lib_name in self.remote_libraries.items(): - if library in (lib_id, lib_name): - return lib_id - return None + """ + Resolve a requested name or ID to the one library ID it names. + + Library IDs are unique, but two libraries can share a name. Silently + matching more than one and picking the first would sync whichever + happened to come first in the server's response, not necessarily the + one requested. + """ + matches = [lib_id for lib_id, lib_name in self.remote_libraries.items() + if library in (lib_id, lib_name)] + if len(matches) > 1: + raise ConfigError( + f"{library!r} matches {len(matches)} libraries on the server; " + "use the library ID instead" + ) + return matches[0] if matches else None def sync_lib(self, lib_id: str, lib_dir: str) -> bool: """ diff --git a/start.py b/start.py index 45dc498..94da328 100755 --- a/start.py +++ b/start.py @@ -53,8 +53,14 @@ def defaults_from_env() -> dict: def resolve_libraries(client, requested: str, server: str) -> set: """Map the requested names or IDs to library IDs that are not synced yet.""" + requested_names = requested.split(sep=":") + if any(name == "" for name in requested_names): + raise ConfigError( + f"library set must not contain an empty entry, got {requested!r}" + ) + libs_to_sync = set() - for arg_lib in requested.split(sep=":"): + for arg_lib in requested_names: lib_id = client.get_library_id(arg_lib) if lib_id: libs_to_sync.add(lib_id) diff --git a/tests/test_libraries.py b/tests/test_libraries.py new file mode 100644 index 0000000..cca7059 --- /dev/null +++ b/tests/test_libraries.py @@ -0,0 +1,56 @@ +"""A library set must resolve to exactly one library ID per entry, or fail.""" + +import pytest + +from dsc.client import SeafileClient +from dsc.errors import ConfigError +from start import resolve_libraries + + +@pytest.fixture +def client(monkeypatch): + c = SeafileClient("seafile.example", "user", "pw", "/dsc") + monkeypatch.setattr(type(c), "remote_libraries", { + "id-1": "Docs", + "id-2": "Docs", + "id-3": "Photos", + }) + monkeypatch.setattr(c, "get_local_libraries", lambda: set()) + return c + + +def test_get_library_id_resolves_a_unique_name(client): + assert client.get_library_id("Photos") == "id-3" + + +def test_get_library_id_resolves_by_id(client): + assert client.get_library_id("id-1") == "id-1" + + +def test_get_library_id_returns_none_for_no_match(client): + assert client.get_library_id("Missing") is None + + +def test_get_library_id_rejects_an_ambiguous_name(client): + with pytest.raises(ConfigError, match="Docs.*2 libraries"): + client.get_library_id("Docs") + + +def test_resolve_libraries_rejects_an_empty_requested_set(client): + with pytest.raises(ConfigError, match="empty entry"): + resolve_libraries(client, "", "seafile.example") + + +@pytest.mark.parametrize("requested", ["Photos:", ":Photos", "Photos::id-1"]) +def test_resolve_libraries_rejects_an_empty_entry_between_separators(client, requested): + with pytest.raises(ConfigError, match="empty entry"): + resolve_libraries(client, requested, "seafile.example") + + +def test_resolve_libraries_rejects_an_ambiguous_name_end_to_end(client): + with pytest.raises(ConfigError, match="Docs"): + resolve_libraries(client, "Docs", "seafile.example") + + +def test_resolve_libraries_resolves_unambiguous_names(client): + assert resolve_libraries(client, "Photos:id-1", "seafile.example") == {"id-3", "id-1"}