Read the local library list as JSON instead of splitting columns
get_local_libraries unpacked "line.rsplit(maxsplit=3)" into three names, but maxsplit=3 yields up to four fields. seaf-cli prints "name id path" separated by whitespace, so any library whose name contains a space produced a fourth field and raised ValueError, which nothing caught. The failure only appeared after the first run: the list is empty until a library has been synced, so a container would work when it was set up and then crash on every restart. seaf-cli list --json returns name, id and path as structured fields, which display output cannot represent unambiguously for names or paths containing whitespace. Its exit code is now checked and unreadable output is reported as a daemon error instead of a traceback. The image smoke check asserts that the installed seaf-cli offers --json and --token, so a future version bump that drops either fails the build rather than the container.
This commit is contained in:
1 parent
8043ac1409
commit
b75863ba51
4 files changed
+73
-9
No files matched your search
@@ -68,6 +68,8 @@ RUN chmod +x /dsc/start.py && \
|
|||||||
# Smoke checks: seaf-cli has no --version flag, so assert its subcommands, and
|
# Smoke checks: seaf-cli has no --version flag, so assert its subcommands, and
|
||||||
# confirm the app's own imports resolve.
|
# confirm the app's own imports resolve.
|
||||||
RUN seaf-cli --help | grep -q 'download-by-name' && \
|
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"
|
python3 -c "import seafile, pysearpc, dsc.client, dsc.misc"
|
||||||
|
|
||||||
VOLUME /dsc/seafile-data
|
VOLUME /dsc/seafile-data
|
||||||
|
|||||||
+4
-1
@@ -152,7 +152,10 @@ Acceptance criteria:
|
|||||||
server errors separately.
|
server errors separately.
|
||||||
- [ ] Make TLS configuration consistent between Python requests and Seafile.
|
- [ ] Make TLS configuration consistent between Python requests and Seafile.
|
||||||
- [ ] Support a custom CA bundle; keep certificate disabling as a last resort.
|
- [ ] 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.
|
- [ ] Reject ambiguous library names and an empty requested library set.
|
||||||
|
|
||||||
### Runtime hardening
|
### Runtime hardening
|
||||||
|
|||||||
+11
-8
@@ -1,4 +1,5 @@
|
|||||||
import argparse
|
import argparse
|
||||||
|
import json
|
||||||
import logging
|
import logging
|
||||||
import os
|
import os
|
||||||
import subprocess
|
import subprocess
|
||||||
@@ -271,16 +272,18 @@ class SeafileClient:
|
|||||||
prev_status[library] = cur_status[library]
|
prev_status[library] = cur_status[library]
|
||||||
|
|
||||||
def get_local_libraries(self) -> set:
|
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))
|
_lg.info("Listing local libraries: %s", " ".join(cmd))
|
||||||
out = subprocess.check_output(self.__gen_cmd(cmd))
|
out = self.__run(cmd, stdout=subprocess.PIPE).stdout
|
||||||
out = out.decode().splitlines()[1:] # first line is a header
|
|
||||||
|
|
||||||
local_libs = set()
|
try:
|
||||||
for line in out:
|
libraries = json.loads(out.decode())
|
||||||
lib_name, lib_id, lib_path = line.rsplit(maxsplit=3)
|
except (UnicodeDecodeError, ValueError) as err:
|
||||||
local_libs.add(lib_id)
|
raise DaemonError(f"Cannot read the library list from seaf-cli: {err}")
|
||||||
return local_libs
|
|
||||||
|
return {lib["id"] for lib in libraries}
|
||||||
|
|
||||||
def configure(self, args: argparse.Namespace, check_for_daemon: bool = True):
|
def configure(self, args: argparse.Namespace, check_for_daemon: bool = True):
|
||||||
need_restart = False
|
need_restart = False
|
||||||
|
|||||||
@@ -1,12 +1,14 @@
|
|||||||
"""Every subprocess must be an argument list, never a shell string."""
|
"""Every subprocess must be an argument list, never a shell string."""
|
||||||
|
|
||||||
import argparse
|
import argparse
|
||||||
|
import json
|
||||||
import subprocess
|
import subprocess
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
|
|
||||||
from dsc import client as client_module
|
from dsc import client as client_module
|
||||||
from dsc import const
|
from dsc import const
|
||||||
|
from dsc.errors import DaemonError
|
||||||
from dsc.client import SeafileClient
|
from dsc.client import SeafileClient
|
||||||
from dsc.misc import hide_secrets, user_cmd
|
from dsc.misc import hide_secrets, user_cmd
|
||||||
|
|
||||||
@@ -157,3 +159,57 @@ def test_configure_passes_option_values_as_arguments(client, calls, monkeypatch)
|
|||||||
assert prefix + ["-k", "upload_limit", "-v", "100"] in argvs
|
assert prefix + ["-k", "upload_limit", "-v", "100"] in argvs
|
||||||
# An option that seaf-cli does not know about is never forwarded.
|
# An option that seaf-cli does not know about is never forwarded.
|
||||||
assert not any("ignored" in argv for argv in argvs)
|
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()
|
||||||
Reference in new issue
Block a user