diff --git a/Dockerfile b/Dockerfile index 5eec5f0..3646f90 100644 --- a/Dockerfile +++ b/Dockerfile @@ -68,6 +68,8 @@ RUN chmod +x /dsc/start.py && \ # Smoke checks: seaf-cli has no --version flag, so assert its subcommands, and # confirm the app's own imports resolve. RUN seaf-cli --help | grep -q 'download-by-name' && \ + seaf-cli sync --help | grep -q '\--token' && \ + seaf-cli list --help | grep -q '\--json' && \ python3 -c "import seafile, pysearpc, dsc.client, dsc.misc" VOLUME /dsc/seafile-data diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index f300cbe..2330bb4 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -152,7 +152,10 @@ Acceptance criteria: server errors separately. - [ ] Make TLS configuration consistent between Python requests and Seafile. - [ ] Support a custom CA bundle; keep certificate disabling as a last resort. -- [ ] Prefer structured JSON output from `seaf-cli` over parsing display text. +- [x] Prefer structured JSON output from `seaf-cli` over parsing display text. + `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. ### Runtime hardening diff --git a/dsc/client.py b/dsc/client.py index 757414b..62c56f0 100644 --- a/dsc/client.py +++ b/dsc/client.py @@ -1,4 +1,5 @@ import argparse +import json import logging import os import subprocess @@ -271,16 +272,27 @@ class SeafileClient: prev_status[library] = cur_status[library] def get_local_libraries(self) -> set: - cmd = ["seaf-cli", "list"] + # The display output is whitespace separated, so a library name or a + # path containing a space cannot be parsed back apart. Ask for JSON. + cmd = ["seaf-cli", "list", "--json"] _lg.info("Listing local libraries: %s", " ".join(cmd)) - out = subprocess.check_output(self.__gen_cmd(cmd)) - out = out.decode().splitlines()[1:] # first line is a header + out = self.__run(cmd, stdout=subprocess.PIPE).stdout - local_libs = set() - for line in out: - lib_name, lib_id, lib_path = line.rsplit(maxsplit=3) - local_libs.add(lib_id) - return local_libs + try: + libraries = json.loads(out.decode()) + except (UnicodeDecodeError, ValueError) as err: + raise DaemonError( + f"Cannot read the library list from seaf-cli: {err}" + ) from err + + # Valid JSON is not necessarily the expected shape, and an unexpected + # one must fail the same way rather than as a bare TypeError. + try: + return {lib["id"] for lib in libraries} + except (KeyError, TypeError) as err: + raise DaemonError( + f"Unexpected library list from seaf-cli: {err}" + ) from err def configure(self, args: argparse.Namespace, check_for_daemon: bool = True): need_restart = False diff --git a/tests/test_commands.py b/tests/test_commands.py index 497c913..082f16f 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -1,12 +1,14 @@ """Every subprocess must be an argument list, never a shell string.""" import argparse +import json import subprocess import pytest from dsc import client as client_module from dsc import const +from dsc.errors import DaemonError from dsc.client import SeafileClient from dsc.misc import hide_secrets, user_cmd @@ -157,3 +159,79 @@ def test_configure_passes_option_values_as_arguments(client, calls, monkeypatch) assert prefix + ["-k", "upload_limit", "-v", "100"] in argvs # An option that seaf-cli does not know about is never forwarded. assert not any("ignored" in argv for argv in argvs) + + +def run_returning(monkeypatch, stdout=b"", returncode=0): + """Run seaf-cli commands that produce the given output.""" + calls = [] + + def fake_run(argv, **kwargs): + calls.append(argv) + return subprocess.CompletedProcess(argv, returncode, stdout=stdout) + + monkeypatch.setattr(subprocess, "run", fake_run) + return calls + + +def test_local_libraries_are_read_as_json(client, monkeypatch): + out = json.dumps([ + {"name": "Docs", "id": "id-1", "path": "/dsc/seafile/Docs"}, + ]).encode() + calls = run_returning(monkeypatch, out) + + assert client.get_local_libraries() == {"id-1"} + assert "--json" in calls[-1] + + +@pytest.mark.parametrize("name, path", [ + # Display output is whitespace separated, so these used to be unparseable. + ("My Docs", "/dsc/seafile/My_Docs"), + ("Docs", "/mnt/my libraries/Docs"), + ("My Docs", "/mnt/my libraries/My Docs"), + ("Ünïcödé ☂", "/dsc/seafile/Ünïcödé_☂"), + ("tab\tname", "/dsc/seafile/tab_name"), +]) +def test_local_libraries_survive_whitespace_in_names_and_paths(client, monkeypatch, name, path): + out = json.dumps([{"name": name, "id": "id-1", "path": path}]).encode() + run_returning(monkeypatch, out) + + assert client.get_local_libraries() == {"id-1"} + + +def test_no_local_libraries_gives_an_empty_set(client, monkeypatch): + run_returning(monkeypatch, b"[]") + assert client.get_local_libraries() == set() + + +def test_local_libraries_reports_unreadable_output(client, monkeypatch): + run_returning(monkeypatch, b"not json at all") + with pytest.raises(DaemonError): + client.get_local_libraries() + + +def test_local_libraries_reports_a_failed_command(client, monkeypatch): + run_returning(monkeypatch, b"", returncode=1) + with pytest.raises(DaemonError): + client.get_local_libraries() + + +@pytest.mark.parametrize("payload", [ + b'{"name": "Docs"}', # an object where a list is expected + b'[{"name": "Docs"}]', # an entry with no id + b'["Docs"]', # entries that are not objects + b'[null]', + b'null', + b'42', +]) +def test_local_libraries_reports_unexpected_json_shapes(client, monkeypatch, payload): + """Valid JSON of the wrong shape must not escape as a raw TypeError.""" + run_returning(monkeypatch, payload) + with pytest.raises(DaemonError): + client.get_local_libraries() + + +def test_local_libraries_error_keeps_the_original_cause(client, monkeypatch): + run_returning(monkeypatch, b"not json at all") + with pytest.raises(DaemonError) as err: + client.get_local_libraries() + assert err.value.__cause__ is not None