Add startup validation for required LFTP config fields (#310)

src/python/controller/controller.py

@@ -233,6 +233,9 @@ def __init__(self, context: Context, persist: ControllerPersist):
         # Decide the password here
         self.__password = context.config.lftp.remote_password if not context.config.lftp.use_ssh_key else None
 
+        # Validate required config fields before building anything
+        self._validate_config()
+
         # The command queue
         self.__command_queue = Queue()
 
@@ -273,6 +276,57 @@ def __init__(self, context: Context, persist: ControllerPersist):
 
         self.__started = False
 
+    def _validate_config(self) -> None:
+        """Validate that all required config fields are set (non-None) at startup.
+
+        Collects all missing fields and raises a single ControllerError listing them.
+        """
+        missing: list[str] = []
+        config = self.__context.config
+
+        # Lftp required fields
+        lftp_fields = [
+            "remote_address",
+            "remote_username",
+            "remote_port",
+            "remote_path_to_scan_script",
+            "use_ssh_key",
+            "use_temp_file",
+            "num_max_parallel_downloads",
+            "num_max_parallel_files_per_download",
+            "num_max_connections_per_root_file",
+            "num_max_connections_per_dir_file",
+            "num_max_total_connections",
+        ]
+        for field in lftp_fields:
+            if getattr(config.lftp, field) is None:
+                missing.append("Lftp.{}".format(field))
+
+        # Controller required fields
+        controller_fields = [
+            "interval_ms_remote_scan",
+            "interval_ms_local_scan",
+            "interval_ms_downloading_scan",
+        ]
+        for field in controller_fields:
+            if getattr(config.controller, field) is None:
+                missing.append("Controller.{}".format(field))
+
+        # General required fields
+        if config.general.verbose is None:
+            missing.append("General.verbose")
+
+        # AutoQueue required fields
+        if config.autoqueue.auto_delete_remote is None:
+            missing.append("AutoQueue.auto_delete_remote")
+
+        # Args required fields
+        if self.__context.args.local_path_to_scanfs is None:
+            missing.append("Args.local_path_to_scanfs")
+
+        if missing:
+            raise ControllerError("Required config fields are not set: {}".format(", ".join(missing)))
+
     def _build_pair_contexts(self) -> list[_PairContext]:
         """
         Build a _PairContext for each configured path pair.
@@ -289,12 +343,25 @@ def _build_pair_contexts(self) -> list[_PairContext]:
                 self.logger.warning("All path pairs are disabled. Enable a pair in Settings to start syncing.")
                 return []
             # Backward compatibility: no path pairs configured, use config.lftp
+            remote_path = self.__context.config.lftp.remote_path
+            local_path = self.__context.config.lftp.local_path
+            if remote_path is None or local_path is None:
+                missing = [
+                    name
+                    for name, val in [("Lftp.remote_path", remote_path), ("Lftp.local_path", local_path)]
+                    if val is None
+                ]
+                fields = ", ".join(missing)
+                raise ControllerError(
+                    "No path pairs configured and {} not set. "
+                    "Configure at least one path pair in Settings, or set {}.".format(fields, fields)
+                )
             return [
                 self._create_pair_context(
                     pair_id=None,
                     name="Default",
-                    remote_path=self.__context.config.lftp.remote_path,  # type: ignore[arg-type]
-                    local_path=self.__context.config.lftp.local_path,  # type: ignore[arg-type]
+                    remote_path=remote_path,
+                    local_path=local_path,
                 )
             ]
 

src/python/tests/unittests/test_controller/test_validate_config.py

@@ -0,0 +1,212 @@
+"""Unit tests for Controller._validate_config() startup validation."""
+
+import logging
+import unittest
+from unittest.mock import MagicMock, patch
+
+from common import Args, Config, Context, Status
+from common.path_pairs_config import PathPairsConfig
+from controller import Controller, ControllerPersist
+from controller.controller import ControllerError
+
+# All required Lftp fields and their valid test values
+_LFTP_REQUIRED = {
+    "remote_address": "seedbox.example.com",
+    "remote_username": "user",
+    "remote_port": 22,
+    "remote_path_to_scan_script": "/path/to/scanfs",
+    "use_ssh_key": False,
+    "use_temp_file": True,
+    "num_max_parallel_downloads": 2,
+    "num_max_parallel_files_per_download": 3,
+    "num_max_connections_per_root_file": 4,
+    "num_max_connections_per_dir_file": 4,
+    "num_max_total_connections": 10,
+}
+
+# All required Controller fields and their valid test values
+_CONTROLLER_REQUIRED = {
+    "interval_ms_remote_scan": 30000,
+    "interval_ms_local_scan": 30000,
+    "interval_ms_downloading_scan": 2000,
+}
+
+
+def _make_config(skip_lftp=None, skip_controller=None, skip_general_verbose=False, skip_autoqueue=False):
+    """Return a Config with required fields populated, optionally skipping some.
+
+    Fields left at None are the ones we want to test as missing.
+    Since the InnerConfig property system only allows setting None on the first
+    call (before any value is set), we simply don't set values we want to remain None.
+    """
+    skip_lftp = skip_lftp or set()
+    skip_controller = skip_controller or set()
+
+    config = Config()
+
+    # Lftp — always set password (not required but needed for use_ssh_key logic)
+    config.lftp.remote_password = "pass"
+    # Also set remote_path/local_path for backward-compat path (not validated by _validate_config)
+    config.lftp.remote_path = "/remote"
+    config.lftp.local_path = "/local"
+    for field, value in _LFTP_REQUIRED.items():
+        if field not in skip_lftp:
+            setattr(config.lftp, field, value)
+
+    # Controller
+    for field, value in _CONTROLLER_REQUIRED.items():
+        if field not in skip_controller:
+            setattr(config.controller, field, value)
+
+    # General
+    if not skip_general_verbose:
+        config.general.verbose = False
+
+    # AutoQueue
+    if not skip_autoqueue:
+        config.autoqueue.auto_delete_remote = False
+
+    return config
+
+
+def _make_args(skip_scanfs=False):
+    """Return Args with required fields populated."""
+    args = Args()
+    if not skip_scanfs:
+        args.local_path_to_scanfs = "/path/to/scanfs"
+    return args
+
+
+def _make_context(config, args):
+    """Build a Context suitable for Controller construction."""
+    logger = logging.getLogger("test_validate_config")
+    web_logger = logging.getLogger("test_validate_config.web")
+    status = Status()
+    path_pairs_config = PathPairsConfig()
+    return Context(
+        logger=logger,
+        web_access_logger=web_logger,
+        config=config,
+        args=args,
+        status=status,
+        path_pairs_config=path_pairs_config,
+    )
+
+
+class TestValidateConfig(unittest.TestCase):
+    """Tests for Controller._validate_config()."""
+
+    @patch.object(Controller, "_build_pair_contexts", return_value=[])
+    @patch.object(Controller, "_sync_persist_to_all_builders")
+    def test_valid_config_does_not_raise(self, _mock_sync, _mock_build):
+        """A fully-populated config should not raise."""
+        config = _make_config()
+        args = _make_args()
+        context = _make_context(config, args)
+        persist = MagicMock(spec=ControllerPersist)
+        # Should not raise
+        Controller(context, persist)
+
+    @patch.object(Controller, "_build_pair_contexts", return_value=[])
+    @patch.object(Controller, "_sync_persist_to_all_builders")
+    def test_missing_single_lftp_field_raises(self, _mock_sync, _mock_build):
+        """A single missing Lftp field should raise with that field name."""
+        config = _make_config(skip_lftp={"remote_address"})
+        args = _make_args()
+        context = _make_context(config, args)
+        persist = MagicMock(spec=ControllerPersist)
+        with self.assertRaises(ControllerError) as cm:
+            Controller(context, persist)
+        self.assertIn("Lftp.remote_address", str(cm.exception))
+
+    @patch.object(Controller, "_build_pair_contexts", return_value=[])
+    @patch.object(Controller, "_sync_persist_to_all_builders")
+    def test_missing_multiple_fields_lists_all(self, _mock_sync, _mock_build):
+        """Multiple missing fields should all appear in the error message."""
+        config = _make_config(
+            skip_lftp={"remote_address", "remote_port"},
+            skip_controller={"interval_ms_remote_scan"},
+            skip_general_verbose=True,
+        )
+        args = _make_args()
+        context = _make_context(config, args)
+        persist = MagicMock(spec=ControllerPersist)
+        with self.assertRaises(ControllerError) as cm:
+            Controller(context, persist)
+        msg = str(cm.exception)
+        self.assertIn("Lftp.remote_address", msg)
+        self.assertIn("Lftp.remote_port", msg)
+        self.assertIn("Controller.interval_ms_remote_scan", msg)
+        self.assertIn("General.verbose", msg)
+
+    @patch.object(Controller, "_build_pair_contexts", return_value=[])
+    @patch.object(Controller, "_sync_persist_to_all_builders")
+    def test_missing_args_local_path_to_scanfs_raises(self, _mock_sync, _mock_build):
+        """Missing Args.local_path_to_scanfs should raise."""
+        config = _make_config()
+        args = _make_args(skip_scanfs=True)
+        context = _make_context(config, args)
+        persist = MagicMock(spec=ControllerPersist)
+        with self.assertRaises(ControllerError) as cm:
+            Controller(context, persist)
+        self.assertIn("Args.local_path_to_scanfs", str(cm.exception))
+
+    @patch.object(Controller, "_build_pair_contexts", return_value=[])
+    @patch.object(Controller, "_sync_persist_to_all_builders")
+    def test_missing_autoqueue_field_raises(self, _mock_sync, _mock_build):
+        """Missing AutoQueue.auto_delete_remote should raise."""
+        config = _make_config(skip_autoqueue=True)
+        args = _make_args()
+        context = _make_context(config, args)
+        persist = MagicMock(spec=ControllerPersist)
+        with self.assertRaises(ControllerError) as cm:
+            Controller(context, persist)
+        self.assertIn("AutoQueue.auto_delete_remote", str(cm.exception))
+
+    @patch.object(Controller, "_build_pair_contexts", return_value=[])
+    @patch.object(Controller, "_sync_persist_to_all_builders")
+    def test_missing_controller_field_raises(self, _mock_sync, _mock_build):
+        """Missing Controller.interval_ms_local_scan should raise."""
+        config = _make_config(skip_controller={"interval_ms_local_scan"})
+        args = _make_args()
+        context = _make_context(config, args)
+        persist = MagicMock(spec=ControllerPersist)
+        with self.assertRaises(ControllerError) as cm:
+            Controller(context, persist)
+        self.assertIn("Controller.interval_ms_local_scan", str(cm.exception))
+
+
+class TestBackwardCompatValidation(unittest.TestCase):
+    """Tests for backward-compat path pair validation in _build_pair_contexts."""
+
+    @patch.object(Controller, "_validate_config")
+    @patch.object(Controller, "_sync_persist_to_all_builders")
+    def test_backward_compat_missing_remote_path_raises(self, _mock_sync, _mock_validate):
+        """When no path pairs exist and remote_path is None, should raise."""
+        # remote_path defaults to None in Config.Lftp.__init__
+        config2 = Config()
+        config2.lftp.remote_password = "pass"
+        # Set all the required fields that _validate_config would check (but it's mocked)
+        # remote_path is left as None (default)
+        config2.lftp.local_path = "/local"
+        args = _make_args()
+        context = _make_context(config2, args)
+        persist = MagicMock(spec=ControllerPersist)
+        with self.assertRaises(ControllerError) as cm:
+            Controller(context, persist)
+        self.assertIn("remote_path", str(cm.exception))
+
+    @patch.object(Controller, "_validate_config")
+    @patch.object(Controller, "_sync_persist_to_all_builders")
+    def test_backward_compat_missing_local_path_raises(self, _mock_sync, _mock_validate):
+        """When no path pairs exist and local_path is None, should raise."""
+        config = Config()
+        config.lftp.remote_password = "pass"
+        config.lftp.remote_path = "/remote"
+        # local_path left as None (default)
+        args = _make_args()
+        context = _make_context(config, args)
+        persist = MagicMock(spec=ControllerPersist)
+        with self.assertRaises(ControllerError) as cm:
+            Controller(context, persist)
+        self.assertIn("local_path", str(cm.exception))