forked from viewit/KX-Bridge-Release
viewit
60156fd589
Found during a targeted code review, not from a user report. MQTT_PORT, POLL_INTERVAL, and the other numeric module-level shortcuts in config_loader.py ran int(get(...)) unguarded at import time. A hand-edited config.ini with a typo (e.g. "mqtt_port = 98833x") raised an uncaught ValueError before the bridge even started, with a raw traceback instead of a usable diagnostic - list_printers() already guarded this exact class of input the same way, but the module-level constants didn't. Added _safe_int() (same try/except-with-fallback pattern) and applied it everywhere int() was called unguarded on a config value. Also wrapped migrate_env_to_config()'s filesystem writes (which also run at import time during first-run .env migration) in try/except, so a permission or disk-full error logs a clear message before re-raising instead of surfacing as a bare traceback pointing into configparser. New tests in tests/test_config_loader_robustness.py cover both, including an end-to-end subprocess test that imports config_loader against a malformed config.ini (a plain re-import wouldn't re-exercise the import-time code path due to Python's module caching).
67 lines
2.6 KiB
Python
67 lines
2.6 KiB
Python
"""config_loader.py robustness fixes found during code review:
|
|
|
|
1. _safe_int() must fall back to a default instead of raising - the
|
|
module-level numeric shortcuts (MQTT_PORT, POLL_INTERVAL, etc.) run this
|
|
at import time, so an uncaught ValueError there previously crashed the
|
|
entire bridge on startup if config.ini had a typo'd numeric value
|
|
(e.g. "mqtt_port = 98833x"), with a raw traceback instead of a clear
|
|
diagnostic. list_printers() already guarded this same class of input the
|
|
same way; this applies it to the module-level shortcuts too.
|
|
2. migrate_env_to_config() must log a clear error (not a bare traceback)
|
|
if writing the migrated config.ini fails (e.g. permission error).
|
|
"""
|
|
import subprocess
|
|
import sys
|
|
import textwrap
|
|
|
|
import config_loader
|
|
|
|
|
|
def test_safe_int_returns_value_for_valid_numeric_string():
|
|
assert config_loader._safe_int("42", 0) == 42
|
|
|
|
|
|
def test_safe_int_falls_back_to_default_on_garbage():
|
|
assert config_loader._safe_int("98833x", 9883) == 9883
|
|
|
|
|
|
def test_safe_int_falls_back_to_default_on_empty_string():
|
|
assert config_loader._safe_int("", 3) == 3
|
|
|
|
|
|
def test_safe_int_falls_back_to_default_on_none():
|
|
assert config_loader._safe_int(None, 5) == 5
|
|
|
|
|
|
def test_config_loader_import_survives_malformed_config_ini(tmp_path):
|
|
"""End-to-end regression guard: importing config_loader with a
|
|
config.ini containing a non-numeric mqtt_port must not raise - it must
|
|
fall back to the default instead. Run in a subprocess since
|
|
config_loader executes its migration/loading logic at import time and
|
|
Python caches modules, so a plain re-import in this test process
|
|
wouldn't actually re-exercise the import-time code path."""
|
|
config_dir = tmp_path / "config"
|
|
config_dir.mkdir()
|
|
(config_dir / "config.ini").write_text(
|
|
"[connection]\n"
|
|
"printer_ip = 192.168.1.50\n"
|
|
"mqtt_port = 98833x\n" # malformed - not a number
|
|
)
|
|
|
|
script = textwrap.dedent(f"""
|
|
import sys
|
|
sys.path.insert(0, {str(tmp_path.parent.parent)!r})
|
|
import config_loader
|
|
config_loader._BASE = __import__("pathlib").Path({str(tmp_path)!r})
|
|
config_loader._find_config_file = lambda: {str(config_dir / "config.ini")!r} \
|
|
and __import__("pathlib").Path({str(config_dir / "config.ini")!r})
|
|
import importlib
|
|
importlib.reload(config_loader)
|
|
assert config_loader.MQTT_PORT == 9883, config_loader.MQTT_PORT
|
|
print("OK")
|
|
""")
|
|
|
|
result = subprocess.run([sys.executable, "-c", script], capture_output=True, text=True, timeout=10)
|
|
assert result.returncode == 0, f"stdout={result.stdout!r} stderr={result.stderr!r}"
|
|
assert "OK" in result.stdout
|