Refactoring merge Method into a New Class & Adding Unit Tests (#137)

5 files changed, 219 insertions, 14 deletions

diff --git a/kvmd/apps/__init__.py b/kvmd/apps/__init__.py
index 04f88ffa..31847bc3 100644
--- a/kvmd/apps/__init__.py
+++ b/kvmd/apps/__init__.py
@@ -53,6 +53,7 @@ from ..yamlconf import Option
 from ..yamlconf import build_raw_from_options
 from ..yamlconf.dumper import make_config_dump
 from ..yamlconf.loader import load_yaml_file
+from ..yamlconf.merger import yaml_merge
 
 from ..validators.basic import valid_stripped_string
 from ..validators.basic import valid_stripped_string_not_empty
@@ -177,8 +178,8 @@ def _init_config(config_path: str, override_options: list[str], **load_flags: bo
 
     scheme = _get_config_scheme()
     try:
-        tools.merge(raw_config, (raw_config.pop("override", {}) or {}))
-        tools.merge(raw_config, build_raw_from_options(override_options))
+        yaml_merge(raw_config, (raw_config.pop("override", {}) or {}))
+        yaml_merge(raw_config, build_raw_from_options(override_options), "raw command line options")
         _patch_raw(raw_config)
         config = make_config(raw_config, scheme)
 
diff --git a/kvmd/tools.py b/kvmd/tools.py
index 773fcb5a..1c03f8ce 100644
--- a/kvmd/tools.py
+++ b/kvmd/tools.py
@@ -45,15 +45,6 @@ def efmt(err: Exception) -> str:
 
 
 # =====
-def merge(dest: dict, src: dict) -> None:
-    for key in src:
-        if key in dest:
-            if isinstance(dest[key], dict) and isinstance(src[key], dict):
-                merge(dest[key], src[key])
-                continue
-        dest[key] = src[key]
-
-
 def rget(dct: dict, *keys: Hashable) -> dict:
     result = functools.reduce((lambda nxt, key: nxt.get(key, {})), keys, dct)
     if not isinstance(result, dict):
diff --git a/kvmd/yamlconf/loader.py b/kvmd/yamlconf/loader.py
index 55bc7aa6..ffd69e3e 100644
--- a/kvmd/yamlconf/loader.py
+++ b/kvmd/yamlconf/loader.py
@@ -22,6 +22,8 @@
 
 import os
 
+from .. import tools
+
 from typing import IO
 from typing import Any
 
@@ -30,7 +32,7 @@ import yaml.nodes
 import yaml.resolver
 import yaml.constructor
 
-from .. import tools
+from .merger import yaml_merge
 
 
 # =====
@@ -70,9 +72,9 @@ class _YamlLoader(yaml.SafeLoader):
                 for child in sorted(os.listdir(inc_path)):
                     child_path = os.path.join(inc_path, child)
                     if os.path.isfile(child_path) or os.path.islink(child_path):
-                        tools.merge(tree, (load_yaml_file(child_path) or {}))
+                        yaml_merge(tree, (load_yaml_file(child_path) or {}), child_path)
             else:  # Try file
-                tools.merge(tree, (load_yaml_file(inc_path) or {}))
+                yaml_merge(tree, (load_yaml_file(inc_path) or {}), inc_path)
         return tree
 
 
diff --git a/kvmd/yamlconf/merger.py b/kvmd/yamlconf/merger.py
new file mode 100644
index 00000000..51a168c8
--- /dev/null
+++ b/kvmd/yamlconf/merger.py
@@ -0,0 +1,48 @@
+# ========================================================================== #
+#                                                                            #
+#    KVMD - The main PiKVM daemon.                                           #
+#                                                                            #
+#    Copyright (C) 2018-2022  Maxim Devaev <[email protected]>               #
+#                                                                            #
+#    This program is free software: you can redistribute it and/or modify    #
+#    it under the terms of the GNU General Public License as published by    #
+#    the Free Software Foundation, either version 3 of the License, or       #
+#    (at your option) any later version.                                     #
+#                                                                            #
+#    This program is distributed in the hope that it will be useful,         #
+#    but WITHOUT ANY WARRANTY; without even the implied warranty of          #
+#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the           #
+#    GNU General Public License for more details.                            #
+#                                                                            #
+#    You should have received a copy of the GNU General Public License       #
+#    along with this program.  If not, see <https://www.gnu.org/licenses/>.  #
+#                                                                            #
+# ========================================================================== #
+
+from typing import Optional
+
+
+# =====
+def _merge(dest: dict, src: dict) -> None:
+    for key in src:
+        if key in dest:
+            if isinstance(dest[key], dict) and isinstance(src[key], dict):
+                _merge(dest[key], src[key])
+                continue
+        dest[key] = src[key]
+
+
+def yaml_merge(dest: dict, src: dict, source_name: Optional[str]=None) -> None:
+    """ Merges the source dictionary into the destination dictionary. """
+
+    # Checking if destination is None
+    if dest is None:
+        # We can't merge into a None
+        raise ValueError(f"Could not merge {source_name} into None. The destination cannot be None")
+
+    # Checking if source is None or empty
+    if src is None:
+        # If src is None or empty, there's nothing to merge
+        return
+
+    _merge(dest, src)
diff --git a/testenv/tests/yamlconf/test_merger.py b/testenv/tests/yamlconf/test_merger.py
new file mode 100644
index 00000000..661a187e
--- /dev/null
+++ b/testenv/tests/yamlconf/test_merger.py
@@ -0,0 +1,163 @@
+# ========================================================================== #
+#                                                                            #
+#    KVMD - The main PiKVM daemon.                                           #
+#                                                                            #
+#    Copyright (C) 2018-2023  Maxim Devaev <[email protected]>               #
+#                                                                            #
+#    This program is free software: you can redistribute it and/or modify    #
+#    it under the terms of the GNU General Public License as published by    #
+#    the Free Software Foundation, either version 3 of the License, or       #
+#    (at your option) any later version.                                     #
+#                                                                            #
+#    This program is distributed in the hope that it will be useful,         #
+#    but WITHOUT ANY WARRANTY; without even the implied warranty of          #
+#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the           #
+#    GNU General Public License for more details.                            #
+#                                                                            #
+#    You should have received a copy of the GNU General Public License       #
+#    along with this program.  If not, see <https://www.gnu.org/licenses/>.  #
+#                                                                            #
+# ========================================================================== #
+
+
+import pytest
+from kvmd.yamlconf import merger
+
+
+# =====
+def test_simple_override() -> None:
+    base = {"key1": "value1", "key2": "value2"}
+    incoming = {"key1": "new_value1"}
+    merger.yaml_merge(base, incoming)
+    assert base == {"key1": "new_value1", "key2": "value2"}
+
+
+def test_nested_override() -> None:
+    base = {"key1": {"nested_key1": "value1"}, "key2": "value2"}
+    incoming = {"key1": {"nested_key1": "new_value1"}}
+    merger.yaml_merge(base, incoming)
+    assert base == {"key1": {"nested_key1": "new_value1"}, "key2": "value2"}
+
+
+def test_dest_none() -> None:
+    base = None
+    incoming = {"key1": "value1"}
+    with pytest.raises(ValueError, match="destination cannot be None"):
+        merger.yaml_merge(base, incoming)  # type: ignore[arg-type]
+
+
+def test_src_none_or_empty() -> None:
+    base = {"key1": "value1"}
+    incoming = None
+    merger.yaml_merge(base, incoming)  # type: ignore[arg-type]
+    assert base == {"key1": "value1"}
+
+    base = {"key1": "value1"}
+    incoming2: dict = {}
+    merger.yaml_merge(base, incoming2)
+    assert base == {"key1": "value1"}
+
+
+def test_merged_new_keys() -> None:
+    base = {"key1": "value1"}
+    incoming = {"key2": "value2"}
+    merger.yaml_merge(base, incoming)
+    assert base == {"key1": "value1", "key2": "value2"}
+
+
+def test_dest_not_dict() -> None:
+    base = "I'm not a dict"
+    incoming = {"key1": "value1"}
+    with pytest.raises(TypeError, match="object does not support item assignment"):
+        merger.yaml_merge(base, incoming)  # type: ignore[arg-type]
+
+
+def test_src_not_dict() -> None:
+    base = {"key1": "value1"}
+    incoming = "I'm not a dict"
+    with pytest.raises(TypeError, match="string indices must be integers, not 'str'"):
+        merger.yaml_merge(base, incoming)  # type: ignore[arg-type]
+
+
+def test_nested_lists_overwrite() -> None:
+    base = {"key1": [1, 2, 3]}
+    incoming = {"key1": ["a", "b", "c"]}
+    merger.yaml_merge(base, incoming)
+    assert base == {"key1": ["a", "b", "c"]}
+
+
+def test_same_information_rewrite() -> None:
+    base = {"key1": "value1", "key2": "value2"}
+    incoming = {"key1": "value1", "key2": "value2"}
+    merger.yaml_merge(base, incoming)
+    assert base == {"key1": "value1", "key2": "value2"}
+
+
+def test_deeply_nested_dictionaries() -> None:
+    base = {"key1": {"nested_key1": {"deep_nested_key1": "value1"}}, "key2": "value2"}
+    incoming = {"key1": {"nested_key1": {"deep_nested_key1": "new_value1"}}}
+    merger.yaml_merge(base, incoming)
+    assert base == {"key1": {"nested_key1": {"deep_nested_key1": "new_value1"}}, "key2": "value2"}
+
+
+def test_non_dict_values_in_source() -> None:
+    base = {"key1": "value1", "key2": "value2"}
+    incoming = {"key1": 123, "key2": ["value3", "value4"]}
+    merger.yaml_merge(base, incoming)
+    assert base == {"key1": 123, "key2": ["value3", "value4"]}
+
+
+def test_empty_base() -> None:
+    base = {}
+    incoming = {"key1": "value1"}
+    merger.yaml_merge(base, incoming)
+    assert base == {"key1": "value1"}
+
+
+def test_none_values_in_source() -> None:
+    base = {"key1": "value1", "key2": "value2"}
+    incoming = {"key1": None, "key2": "new_value2"}
+    merger.yaml_merge(base, incoming)
+    assert base == {"key1": None, "key2": "new_value2"}
+
+
+def test_key_not_present_in_incoming() -> None:
+    base = {"key1": "value1", "key2": "value2"}
+    incoming = {"key3": "value3"}
+    merger.yaml_merge(base, incoming)
+    assert base == {"key1": "value1", "key2": "value2", "key3": "value3"}
+
+
+def test_mixed_nested_non_nested_keys() -> None:
+    base = {"key1": "value1", "key2": {"nested_key1": "value2"}}
+    incoming = {"key1": "new_value1", "key2": {"nested_key1": "new_value2"}}
+    merger.yaml_merge(base, incoming)
+    assert base == {"key1": "new_value1", "key2": {"nested_key1": "new_value2"}}
+
+
+def test_additional_nested_keys_in_incoming() -> None:
+    base = {"key1": "value1", "key2": {"nested_key1": "value2"}}
+    incoming = {"key1": "new_value1", "key2": {"nested_key1": "new_value2", "nested_key2": "value3"}}
+    merger.yaml_merge(base, incoming)
+    assert base == {"key1": "new_value1", "key2": {"nested_key1": "new_value2", "nested_key2": "value3"}}
+
+
+def test_override_nested_dict_with_non_dict() -> None:
+    base = {"key1": "value1", "key2": {"nested_key1": "value2"}}
+    incoming = {"key1": "new_value1", "key2": "new_value2"}
+    merger.yaml_merge(base, incoming)
+    assert base == {"key1": "new_value1", "key2": "new_value2"}
+
+
+def test_multiple_value_types() -> None:
+    base = {"key1": 1, "key2": True, "key3": [1, 2, 3], "key4": {"nested_key1": "value1"}}
+    incoming = {"key1": 2, "key2": False, "key3": [4, 5, 6], "key4": {"nested_key1": "value2"}}
+    merger.yaml_merge(base, incoming)
+    assert base == {"key1": 2, "key2": False, "key3": [4, 5, 6], "key4": {"nested_key1": "value2"}}
+
+
+def test_non_string_keys() -> None:
+    base: dict = {1: "value1", 2: "value2"}
+    incoming: dict = {1: "new_value1", 3: "value3"}
+    merger.yaml_merge(base, incoming)
+    assert base == {1: "new_value1", 2: "value2", 3: "value3"}