From 85c99634b2994cf07e0e8f9d10b13b8cf4b6fad3 Mon Sep 17 00:00:00 2001 From: nitrobass24 Date: Mon, 23 Mar 2026 20:32:31 -0500 Subject: [PATCH 1/3] Add startup validation for required LFTP config fields (#310) Add _validate_config() to Controller that checks all required config fields are non-None at startup and raises ControllerError listing all missing fields. Also add backward-compat guard in _build_pair_contexts for when no path pairs are configured and remote_path/local_path are missing. Co-Authored-By: Claude Opus 4.6 --- src/python/controller/controller.py | 65 +++++- .../test_controller/test_validate_config.py | 212 ++++++++++++++++++ 2 files changed, 275 insertions(+), 2 deletions(-) create mode 100644 src/python/tests/unittests/test_controller/test_validate_config.py diff --git a/src/python/controller/controller.py b/src/python/controller/controller.py index 2f818d12..5354cefc 100644 --- a/src/python/controller/controller.py +++ b/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,19 @@ 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: + raise ControllerError( + "No path pairs configured and Lftp.remote_path / Lftp.local_path are not set. " + "Configure at least one path pair in Settings, or set remote_path and local_path." + ) 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, ) ] diff --git a/src/python/tests/unittests/test_controller/test_validate_config.py b/src/python/tests/unittests/test_controller/test_validate_config.py new file mode 100644 index 00000000..7a921e38 --- /dev/null +++ b/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)) From 3c83762b9df49769be96ab694aa560750cd1d79f Mon Sep 17 00:00:00 2001 From: nitrobass24 Date: Tue, 24 Mar 2026 11:07:24 -0500 Subject: [PATCH 2/3] Improve backward-compat error to list specific missing field(s) Instead of a generic message when remote_path/local_path are unset, now names exactly which field(s) are missing. Co-Authored-By: Claude Opus 4.6 --- src/python/controller/controller.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/python/controller/controller.py b/src/python/controller/controller.py index 5354cefc..ec2e8065 100644 --- a/src/python/controller/controller.py +++ b/src/python/controller/controller.py @@ -346,9 +346,16 @@ def _build_pair_contexts(self) -> list[_PairContext]: 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 + ] raise ControllerError( - "No path pairs configured and Lftp.remote_path / Lftp.local_path are not set. " - "Configure at least one path pair in Settings, or set remote_path and local_path." + "No path pairs configured and {} not set. " + "Configure at least one path pair in Settings, or set remote_path and local_path.".format( + ", ".join(missing) + ) ) return [ self._create_pair_context( From bec227486367ecd2bbdd4924dc61089af423d12f Mon Sep 17 00:00:00 2001 From: nitrobass24 Date: Tue, 24 Mar 2026 11:39:29 -0500 Subject: [PATCH 3/3] Use dynamic field names in backward-compat error message Co-Authored-By: Claude Opus 4.6 --- src/python/controller/controller.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/python/controller/controller.py b/src/python/controller/controller.py index ec2e8065..583157f6 100644 --- a/src/python/controller/controller.py +++ b/src/python/controller/controller.py @@ -351,11 +351,10 @@ def _build_pair_contexts(self) -> list[_PairContext]: 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 remote_path and local_path.".format( - ", ".join(missing) - ) + "Configure at least one path pair in Settings, or set {}.".format(fields, fields) ) return [ self._create_pair_context(