Merge 8a40ea5c87 into 1c745e0847

Deleting the last entry of a mapping or sequence via config/set orphaned ruamel's comment tokens, which were then emitted above a flow-style {} / [] at column 0, which is unparseable yaml that failed validation and silently rolled the change back. The fix is to clear the emptied collection's stale comment metadata (and the parent's entry for it) so the dump stays valid. Non-empty collections are left untouched so sibling comments are preserved. This covers both emptied maps and emptied lists.

frigate/test/test_update_yaml.py

@@ -0,0 +1,183 @@
+"""Test in-place yaml config updates."""
+
+import os
+import tempfile
+import unittest
+
+from ruamel.yaml import YAML
+
+from frigate.util.builtin import update_yaml_file_bulk
+
+
+class TestUpdateYaml(unittest.TestCase):
+    def setUp(self) -> None:
+        self.yaml = YAML()
+        fd, self.config_path = tempfile.mkstemp(suffix=".yml")
+        os.close(fd)
+
+    def tearDown(self) -> None:
+        os.unlink(self.config_path)
+
+    def _write(self, text: str) -> None:
+        with open(self.config_path, "w") as f:
+            f.write(text)
+
+    def _read(self) -> str:
+        with open(self.config_path) as f:
+            return f.read()
+
+    def _load(self):
+        with open(self.config_path) as f:
+            return self.yaml.load(f)
+
+    def test_delete_key(self):
+        """Deleting a key removes it and leaves valid yaml."""
+        self._write(
+            "cameras:\n"
+            "  cam1:\n"
+            "    objects:\n"
+            "      filters:\n"
+            "        car:\n"
+            "          mask: 0,0.45,0.245,0.45\n"
+        )
+        update_yaml_file_bulk(
+            self.config_path, {"cameras.cam1.objects.filters.car.mask": ""}
+        )
+        data = self._load()
+        assert "mask" not in data["cameras"]["cam1"]["objects"]["filters"]["car"]
+
+    def test_delete_commented_key_emptying_map(self):
+        """Deleting the only key of a map whose key carries comments must not
+        emit unparseable yaml (orphaned comment tokens above a flow-style {})."""
+        self._write(
+            "cameras:\n"
+            "  cam1:\n"
+            "    objects:\n"
+            "      filters:\n"
+            "        car:\n"
+            "          # cars parked across the street\n"
+            "          # second comment line\n"
+            "          mask: 0,0.45,0.245,0.45\n"
+            "    motion:\n"
+            "      mask: 0,0.449,0.686,0.395\n"
+        )
+        update_yaml_file_bulk(
+            self.config_path, {"cameras.cam1.objects.filters.car.mask": ""}
+        )
+        # must re-parse cleanly
+        data = self._load()
+        assert "mask" not in data["cameras"]["cam1"]["objects"]["filters"]["car"]
+        assert data["cameras"]["cam1"]["motion"]["mask"] == "0,0.449,0.686,0.395"
+        # the orphaned comments must be gone from the file, not just parseable
+        content = self._read()
+        assert "cars parked across the street" not in content
+        assert "second comment line" not in content
+
+    def test_delete_last_named_mask_emptying_map(self):
+        """The path the current UI actually sends: a named object mask deleted
+        down to an empty `mask` map, with a comment inside that map."""
+        self._write(
+            "cameras:\n"
+            "  cam1:\n"
+            "    objects:\n"
+            "      filters:\n"
+            "        car:\n"
+            "          mask:\n"
+            "            # ignore the neighbor's driveway\n"
+            "            driveway:\n"
+            "              coordinates: 0,0.1,0.2,0.3\n"
+        )
+        update_yaml_file_bulk(
+            self.config_path,
+            {"cameras.cam1.objects.filters.car.mask.driveway": ""},
+        )
+        data = self._load()
+        assert data["cameras"]["cam1"]["objects"]["filters"]["car"]["mask"] == {}
+        assert "ignore the neighbor's driveway" not in self._read()
+
+    def test_delete_last_commented_list_item(self):
+        """Deleting the last element of a commented sequence must not emit
+        an orphaned comment above a flow-style [] at column 0."""
+        self._write(
+            "cameras:\n"
+            "  cam1:\n"
+            "    motion:\n"
+            "      mask:\n"
+            "        # driveway motion mask\n"
+            "        - 0,0.4,0.6,0.4\n"
+        )
+        update_yaml_file_bulk(self.config_path, {"cameras.cam1.motion.mask.0": ""})
+        data = self._load()
+        assert data["cameras"]["cam1"]["motion"]["mask"] == []
+        assert "driveway motion mask" not in self._read()
+
+    def test_delete_list_item_preserves_remaining(self):
+        """Deleting one element of a sequence keeps the others and stays valid."""
+        self._write(
+            "cameras:\n"
+            "  cam1:\n"
+            "    motion:\n"
+            "      mask:\n"
+            "        - 0,0.4,0.6,0.4\n"
+            "        - 0,0.1,0.2,0.3\n"
+        )
+        update_yaml_file_bulk(self.config_path, {"cameras.cam1.motion.mask.0": ""})
+        data = self._load()
+        assert data["cameras"]["cam1"]["motion"]["mask"] == ["0,0.1,0.2,0.3"]
+
+    def test_delete_key_preserves_siblings(self):
+        """Deleting one key among several keeps the sibling entries and any
+        comments on keys preceding the deleted one."""
+        self._write(
+            "cameras:\n"
+            "  cam1:\n"
+            "    objects:\n"
+            "      filters:\n"
+            "        car:\n"
+            "          # mask drawn around the parked suv\n"
+            "          mask: 0,0.45,0.245,0.45\n"
+            "          threshold: 0.8\n"
+        )
+        update_yaml_file_bulk(
+            self.config_path, {"cameras.cam1.objects.filters.car.threshold": ""}
+        )
+        data = self._load()
+        car = data["cameras"]["cam1"]["objects"]["filters"]["car"]
+        assert "threshold" not in car
+        assert car["mask"] == "0,0.45,0.245,0.45"
+        assert "# mask drawn around the parked suv" in self._read()
+
+    def test_delete_first_commented_key_keeps_map_valid(self):
+        """Deleting a commented key from a map that still has other keys
+        leaves the remaining entries intact and the file parseable."""
+        self._write(
+            "cameras:\n"
+            "  cam1:\n"
+            "    objects:\n"
+            "      filters:\n"
+            "        car:\n"
+            "          # comment on the deleted key\n"
+            "          mask: 0,0.45,0.245,0.45\n"
+            "          threshold: 0.8\n"
+        )
+        update_yaml_file_bulk(
+            self.config_path, {"cameras.cam1.objects.filters.car.mask": ""}
+        )
+        data = self._load()
+        car = data["cameras"]["cam1"]["objects"]["filters"]["car"]
+        assert "mask" not in car
+        assert car["threshold"] == 0.8
+
+    def test_update_value_preserves_comments(self):
+        """Updating a value keeps surrounding comments intact."""
+        self._write(
+            "cameras:\n  cam1:\n    detect:\n      # tuned for the pi\n      fps: 4\n"
+        )
+        update_yaml_file_bulk(self.config_path, {"cameras.cam1.detect.fps": 5})
+        data = self._load()
+        assert data["cameras"]["cam1"]["detect"]["fps"] == 5
+        assert "# tuned for the pi" in self._read()
+
+
+if __name__ == "__main__":
+    unittest.main(verbosity=2)

frigate/util/builtin.py

@@ -293,26 +293,51 @@ def update_yaml_file_bulk(file_path: str, updates: Dict[str, Any]):
         logger.error(f"Unable to write to Frigate config file {file_path}: {e}")
 
 
+def clear_orphaned_comments(collection, parent, parent_key) -> None:
+    """Drop stale ruamel comment tokens after a deletion empties a collection.
+
+    When the last entry of a mapping or sequence is removed, any comments that
+    lived inside that collection's block are orphaned. ruamel then emits them
+    above a flow-style `{}`/`[]` dedented to column 0, which is unparseable and
+    corrupts the config. Clearing the emptied collection's own comment metadata
+    (and the parent's entry pointing at it) keeps the dump valid. Non-empty
+    collections are left untouched so comments on remaining siblings survive.
+    """
+    if not hasattr(collection, "ca") or len(collection) != 0:
+        return
+
+    collection.ca.items.clear()
+    collection.ca.comment = None
+    if parent is not None and hasattr(parent, "ca"):
+        parent.ca.items.pop(parent_key, None)
+
+
 def update_yaml(data, key_path, new_value):
     temp = data
+    parent = None
+    parent_key = None
     for key in key_path[:-1]:
         if isinstance(key, tuple):
             if key[0] not in temp:
                 temp[key[0]] = [{}] * max(1, key[1] + 1)
             elif len(temp[key[0]]) <= key[1]:
                 temp[key[0]] += [{}] * (key[1] - len(temp[key[0]]) + 1)
+            parent, parent_key = temp[key[0]], key[1]
             temp = temp[key[0]][key[1]]
         else:
             if key not in temp or temp[key] is None:
                 temp[key] = {}
+            parent, parent_key = temp, key
             temp = temp[key]
 
     last_key = key_path[-1]
     if new_value == "":
         if isinstance(last_key, tuple):
             del temp[last_key[0]][last_key[1]]
+            clear_orphaned_comments(temp[last_key[0]], temp, last_key[0])
         else:
             del temp[last_key]
+            clear_orphaned_comments(temp, parent, parent_key)
     else:
         if isinstance(last_key, tuple):
             if last_key[0] not in temp: