Merge pull request #25 from snegov/feature/typed-env-parsing

Read the environment with typed parsers and apply the documented defaults

IMPROVEMENTS.md

@@ -84,12 +84,19 @@ Acceptance criteria:
 
 ### Correct environment parsing and defaults
 
-- [ ] Replace direct `os.getenv()` assignments in `start.py` with typed parsers.
+- [x] Replace direct `os.getenv()` assignments in `start.py` with typed parsers.
-- [ ] Preserve the documented defaults for upload limit, download limit, and
+  See `dsc/config.py` and `defaults_from_env()`.
-  delete-confirm threshold when variables are absent.
+- [x] Preserve the documented defaults for upload limit, download limit, and
-- [ ] Validate UID, GID, limits, threshold, and booleans before daemon startup.
+  delete-confirm threshold when variables are absent. They were not: an unset
-- [ ] Reject negative and malformed numeric values with useful messages.
+  variable overwrote the argparse default with `None`, so a default container
-- [ ] Do not pass `None` as a Seafile configuration value.
+  sent the literal string `None` for all three.
+- [x] Validate UID, GID, limits, threshold, and booleans before daemon startup.
+  UID and GID are now integers; as strings they never equalled the integers
+  they were compared against, so `usermod` ran on every start.
+- [x] Reject negative and malformed numeric values with useful messages.
+- [x] Do not pass `None` as a Seafile configuration value. Booleans are also
+  rendered as `true`/`false` rather than Python's `True`/`False`, since the
+  daemon reads that setting with its boolean accessor.
 
 Acceptance criteria:
 

README.md

@@ -46,10 +46,15 @@ volumes:
  - `DELETE_CONFIRM_THRESHOLD` - represents the number of files that require
  confirmation before being deleted simultaneously. Default value is _500_.
  - `DISABLE_VERIFY_CERTIFICATE` - set to _true_ to disable server's certificate
- verification. Default value is _false_.
+ verification. Default value is _false_. Booleans accept _true_/_false_,
+ _1_/_0_, _yes_/_no_ and _on_/_off_, in any case.
  - `UPLOAD_LIMIT` / `DOWNLOAD_LIMIT` -  upload/download speed limit in B/s
  (bytes per second). Default values are _0_ (unlimited).
 
+Numeric variables must be whole, non-negative numbers. A malformed or negative
+ value stops the container at startup with a message naming the variable,
+ rather than being passed on to the Seafile client.
+
 ### Using Docker secrets:
 Mount the secret and point `PASSWORD_FILE` at it, so the password never enters
  the environment:

dsc/client.py

@@ -14,7 +14,7 @@ import seafile
 
 from dsc import const
 from dsc.errors import ConfigError, DaemonError, DaemonTimeout
-from dsc.misc import create_dir, hide_secrets, user_cmd
+from dsc.misc import config_value, create_dir, hide_secrets, user_cmd
 
 _lg = logging.getLogger(__name__)
 
@@ -304,6 +304,9 @@ class SeafileClient:
             if key not in const.AVAILABLE_SEAFCLI_OPTIONS:
                 continue
 
+            if value is None:
+                raise ConfigError(f"No value for seafile option {key}")
+
             # check current value
             cmd = ["seaf-cli", "config", "-k", key]
             _lg.info("Checking seafile client option: %s", " ".join(cmd))
@@ -314,11 +317,11 @@ class SeafileClient:
                 cur_value = cur_value.split(sep="=")[1].strip()
             except IndexError:
                 cur_value = None
-            if cur_value == str(value):
+            if cur_value == config_value(value):
                 continue
 
             # set new value
-            cmd = ["seaf-cli", "config", "-k", key, "-v", str(value)]
+            cmd = ["seaf-cli", "config", "-k", key, "-v", config_value(value)]
             _lg.info("Setting seafile client option: %s", " ".join(cmd))
             self.__run(cmd)
             need_restart = True

dsc/config.py

@@ -0,0 +1,66 @@
+"""Typed reading of the container's environment.
+
+Everything in the environment is text, including values the client later
+compares as numbers or passes to seaf-cli. Reading them directly leaves two
+failures that are silent until much later: an unset variable becomes None and
+reaches seaf-cli as the string "None", and a numeric setting stays a string
+that never equals the integer it is compared against.
+"""
+
+import os
+from typing import Optional
+
+from dsc.errors import ConfigError
+
+_TRUE_VALUES = {"true", "1", "yes", "on"}
+_FALSE_VALUES = {"false", "0", "no", "off"}
+
+
+def env_str(name: str, default: Optional[str] = None) -> Optional[str]:
+    """Return a text setting. An empty variable counts as unset."""
+    value = os.getenv(name)
+    if value is None or value == "":
+        return default
+    return value
+
+
+def env_int(name: str, default: int, minimum: int = 0) -> int:
+    """
+    Return a numeric setting as an int, never as text and never as None.
+
+    Values below ``minimum`` are rejected: a negative speed limit or deletion
+    threshold is not something to pass on to seaf-cli and hope for the best.
+    """
+    raw = env_str(name)
+    if raw is None:
+        return default
+
+    stripped = raw.strip()
+    try:
+        value = int(stripped, 10)
+    except ValueError:
+        raise ConfigError(
+            f"{name} must be a whole number, got {stripped!r}"
+        )
+
+    if value < minimum:
+        raise ConfigError(
+            f"{name} must be {minimum} or greater, got {stripped!r}"
+        )
+    return value
+
+
+def env_bool(name: str, default: bool = False) -> bool:
+    """Return a boolean setting, accepting the usual spellings."""
+    raw = env_str(name)
+    if raw is None:
+        return default
+
+    value = raw.strip().lower()
+    if value in _TRUE_VALUES:
+        return True
+    if value in _FALSE_VALUES:
+        return False
+
+    accepted = ", ".join(sorted(_TRUE_VALUES | _FALSE_VALUES))
+    raise ConfigError(f"{name} must be one of {accepted}, got {raw.strip()!r}")

dsc/misc.py

@@ -65,3 +65,15 @@ def hide_secrets(cmd: list, *secrets: str) -> list:
             if secret in arg:
                 cmd[i] = arg.replace(secret, "********")
     return cmd
+
+
+def config_value(value) -> str:
+    """
+    Render a setting the way seaf-cli stores it.
+
+    Booleans are written as "true" and "false": the daemon reads this setting
+    with its boolean accessor, and Python's str() would produce "True".
+    """
+    if isinstance(value, bool):
+        return "true" if value else "false"
+    return str(value)

start.py

@@ -7,6 +7,7 @@ import signal
 import sys
 
 from dsc import SeafileClient, const
+from dsc.config import env_bool, env_int, env_str
 from dsc.errors import ConfigError, DscError, GracefulShutdown
 from dsc.misc import setup_uid, create_dir
 from dsc.paths import plan_lib_dirs
@@ -26,6 +27,30 @@ def install_signal_handlers():
         signal.signal(signum, handle_signal)
 
 
+def defaults_from_env() -> dict:
+    """
+    Read every setting from the environment with its documented default.
+
+    Reading these directly leaves an unset variable as None, which reaches
+    seaf-cli as the string "None" and silently replaces the documented value.
+    """
+    return dict(
+        server=env_str("SERVER_HOST"),
+        username=env_str("USERNAME"),
+        password=env_str("PASSWORD"),
+        password_file=env_str("PASSWORD_FILE"),
+        token=env_str("TOKEN"),
+        token_file=env_str("TOKEN_FILE"),
+        libraries=env_str("LIBRARY_ID"),
+        uid=env_int("SEAFILE_UID", 1000),
+        gid=env_int("SEAFILE_GID", 1000),
+        upload_limit=env_int("UPLOAD_LIMIT", 0),
+        download_limit=env_int("DOWNLOAD_LIMIT", 0),
+        delete_confirm_threshold=env_int("DELETE_CONFIRM_THRESHOLD", 500),
+        disable_verify_certificate=env_bool("DISABLE_VERIFY_CERTIFICATE", False),
+    )
+
+
 def resolve_libraries(client, requested: str, server: str) -> set:
     """Map the requested names or IDs to library IDs that are not synced yet."""
     libs_to_sync = set()
@@ -94,21 +119,11 @@ def main():
     parser.add_argument("--disable-verify-certificate", action="store_true")
     parser.add_argument("--delete-confirm-threshold", type=int, default=500)
 
-    parser.set_defaults(
+    try:
-        server=os.getenv("SERVER_HOST"),
+        parser.set_defaults(**defaults_from_env())
-        username=os.getenv("USERNAME"),
+    except ConfigError as err:
-        password=os.getenv("PASSWORD"),
+        _lg.error("%s", err)
-        password_file=os.getenv("PASSWORD_FILE"),
+        return 2
-        token=os.getenv("TOKEN"),
-        token_file=os.getenv("TOKEN_FILE"),
-        libraries=os.getenv("LIBRARY_ID"),
-        uid=os.getenv("SEAFILE_UID", default=1000),
-        gid=os.getenv("SEAFILE_GID", default=1000),
-        upload_limit=os.getenv("UPLOAD_LIMIT"),
-        download_limit=os.getenv("DOWNLOAD_LIMIT"),
-        disable_verify_certificate=os.getenv("DISABLE_VERIFY_CERTIFICATE") in ("true", "1", "True"),
-        delete_confirm_threshold=os.getenv("DELETE_CONFIRM_THRESHOLD"),
-    )
     args = parser.parse_args()
     if not args.server:
         parser.error("Seafile server is not specified")

tests/test_commands.py

@@ -8,7 +8,7 @@ import pytest
 
 from dsc import client as client_module
 from dsc import const
-from dsc.errors import DaemonError
+from dsc.errors import ConfigError, DaemonError
 from dsc.client import SeafileClient
 from dsc.misc import hide_secrets, user_cmd
 
@@ -235,3 +235,33 @@ def test_local_libraries_error_keeps_the_original_cause(client, monkeypatch):
     with pytest.raises(DaemonError) as err:
         client.get_local_libraries()
     assert err.value.__cause__ is not None
+
+
+def test_configure_never_sends_none_as_a_value(client, calls):
+    """A None here reaches seaf-cli as the literal string "None"."""
+    with pytest.raises(ConfigError):
+        client.configure(argparse.Namespace(upload_limit=None), check_for_daemon=False)
+
+    for argv, _ in calls:
+        assert "None" not in argv
+
+
+@pytest.mark.parametrize("value, expected", [
+    (True, "true"),
+    (False, "false"),
+    (0, "0"),
+    (500, "500"),
+])
+def test_configure_renders_values_the_way_seaf_cli_stores_them(
+        client, calls, monkeypatch, value, expected):
+    # Changing an option restarts the daemon; keep this on the value only.
+    monkeypatch.setattr(client, "stop_daemon", lambda: None)
+    monkeypatch.setattr(client, "start_daemon", lambda: None)
+
+    client.configure(argparse.Namespace(disable_verify_certificate=value)
+                     if isinstance(value, bool)
+                     else argparse.Namespace(upload_limit=value),
+                     check_for_daemon=False)
+
+    setting = [argv for argv, _ in calls if "-v" in argv][-1]
+    assert setting[setting.index("-v") + 1] == expected

tests/test_config.py

@@ -0,0 +1,120 @@
+"""The environment is untyped text; everything derived from it is checked."""
+
+import pytest
+
+import start
+from dsc.config import env_bool, env_int, env_str
+from dsc.errors import ConfigError
+
+
+@pytest.fixture
+def env(monkeypatch):
+    def set_env(**values):
+        for name, value in values.items():
+            if value is None:
+                monkeypatch.delenv(name, raising=False)
+            else:
+                monkeypatch.setenv(name, value)
+    return set_env
+
+
+@pytest.mark.parametrize("raw, expected", [
+    (None, 7),        # unset keeps the documented default
+    ("", 7),          # an empty variable is the same as unset
+    ("0", 0),         # zero is a real value, not "missing"
+    ("1", 1),
+    ("4096", 4096),
+    (" 42 ", 42),
+    # int() accepts any Unicode decimal digit and normalises it; the value
+    # leaves as an ASCII integer, so this is harmless rather than rejected.
+    ("\u0663", 3),
+])
+def test_env_int_accepts(env, raw, expected):
+    env(LIMIT=raw)
+    assert env_int("LIMIT", 7) == expected
+
+
+@pytest.mark.parametrize("raw", ["-1", "-0.5", "abc", "1.5", "1e3", "0x10", "1 2", "٣ x"])
+def test_env_int_rejects(env, raw):
+    env(LIMIT=raw)
+    with pytest.raises(ConfigError) as err:
+        env_int("LIMIT", 7)
+    assert "LIMIT" in str(err.value)
+    assert raw.strip() in str(err.value)
+
+
+def test_env_int_allows_a_negative_value_when_permitted(env):
+    env(LIMIT="-1")
+    assert env_int("LIMIT", 7, minimum=-1) == -1
+
+
+@pytest.mark.parametrize("raw, expected", [
+    (None, False),
+    ("", False),
+    ("true", True), ("True", True), ("TRUE", True),
+    ("1", True), ("yes", True), ("on", True),
+    ("false", False), ("False", False), ("0", False),
+    ("no", False), ("off", False), (" true ", True),
+])
+def test_env_bool_accepts(env, raw, expected):
+    env(FLAG=raw)
+    assert env_bool("FLAG", False) is expected
+
+
+@pytest.mark.parametrize("raw", ["maybe", "2", "-1", "y3s", "truthy"])
+def test_env_bool_rejects(env, raw):
+    env(FLAG=raw)
+    with pytest.raises(ConfigError) as err:
+        env_bool("FLAG", False)
+    assert "FLAG" in str(err.value)
+
+
+def test_env_bool_keeps_a_true_default_when_unset(env):
+    env(FLAG=None)
+    assert env_bool("FLAG", True) is True
+
+
+@pytest.mark.parametrize("raw, expected", [
+    (None, None), ("", None), ("value", "value"), ("  spaced  ", "  spaced  "),
+])
+def test_env_str(env, raw, expected):
+    env(NAME=raw)
+    assert env_str("NAME") == expected
+
+
+def test_a_default_container_gets_exactly_the_documented_defaults(monkeypatch):
+    """The values the README promises when nothing optional is configured."""
+    for name in ("UPLOAD_LIMIT", "DOWNLOAD_LIMIT", "DELETE_CONFIRM_THRESHOLD",
+                 "SEAFILE_UID", "SEAFILE_GID", "DISABLE_VERIFY_CERTIFICATE"):
+        monkeypatch.delenv(name, raising=False)
+
+    defaults = start.defaults_from_env()
+
+    assert defaults["upload_limit"] == 0
+    assert defaults["download_limit"] == 0
+    assert defaults["delete_confirm_threshold"] == 500
+    assert defaults["uid"] == 1000
+    assert defaults["gid"] == 1000
+    assert defaults["disable_verify_certificate"] is False
+
+
+def test_no_default_is_none_where_a_value_is_documented(monkeypatch):
+    """A None here reaches seaf-cli as the literal string "None"."""
+    for name in ("UPLOAD_LIMIT", "DOWNLOAD_LIMIT", "DELETE_CONFIRM_THRESHOLD",
+                 "SEAFILE_UID", "SEAFILE_GID"):
+        monkeypatch.delenv(name, raising=False)
+
+    defaults = start.defaults_from_env()
+    for key in ("upload_limit", "download_limit", "delete_confirm_threshold",
+                "uid", "gid"):
+        assert defaults[key] is not None, f"{key} must never default to None"
+
+
+def test_numeric_settings_are_integers_not_strings(monkeypatch):
+    """A string UID never equals the integer it is compared against."""
+    monkeypatch.setenv("SEAFILE_UID", "1000")
+    monkeypatch.setenv("UPLOAD_LIMIT", "50")
+
+    defaults = start.defaults_from_env()
+    assert defaults["uid"] == 1000 and isinstance(defaults["uid"], int)
+    assert defaults["upload_limit"] == 50 and isinstance(defaults["upload_limit"], int)