Allow merge into config with readonly node if they are not changed

omegaconf/_utils.py

@@ -650,3 +650,15 @@ def type_str(t: Any) -> str:
         return f"Optional[{ret}]"
     else:
         return ret
+
+
+def _convert_to_omegaconf_container(target: Any):
+    from omegaconf import OmegaConf
+
+    if is_primitive_container(target):
+        assert isinstance(target, (list, dict))
+        target = OmegaConf.create(target)
+    elif is_structured_config(target):
+        target = OmegaConf.structured(target)
+    assert OmegaConf.is_config(target)
+    return target

omegaconf/basecontainer.py

@@ -16,6 +16,7 @@
     is_primitive_container,
     is_primitive_dict,
     is_structured_config,
+    _convert_to_omegaconf_container,
 )
 from .base import Container, ContainerMetadata, Node
 from .errors import MissingMandatoryValue, ReadonlyConfigError, ValidationError
@@ -272,7 +273,7 @@ def _map_merge(dest: "BaseContainer", src: "BaseContainer") -> None:
                         assert isinstance(dest_node, ValueNode)
                         try:
                             dest_node._set_value(src_value)
-                        except ValidationError as e:
+                        except (ValidationError, ReadonlyConfigError) as e:
                             dest._format_and_raise(key=key, value=src_value, cause=e)
             else:
                 dest[key] = src._get_node(key)
@@ -295,17 +296,12 @@ def _merge_with(
     ) -> None:
         from .dictconfig import DictConfig
         from .listconfig import ListConfig
-        from .omegaconf import OmegaConf
 
         """merge a list of other Config objects into this one, overriding as needed"""
         for other in others:
             if other is None:
                 raise ValueError("Cannot merge with a None config")
-            if is_primitive_container(other):
-                assert isinstance(other, (list, dict))
-                other = OmegaConf.create(other)
-            elif is_structured_config(other):
-                other = OmegaConf.structured(other)
+            other = _convert_to_omegaconf_container(other)
             if isinstance(self, DictConfig) and isinstance(other, DictConfig):
                 BaseContainer._map_merge(self, other)
             elif isinstance(self, ListConfig) and isinstance(other, ListConfig):
@@ -339,6 +335,9 @@ def _set_item_impl(self, key: Any, value: Any) -> None:
         else:
             self._validate_set(key, value)
 
+        if self._get_flag("readonly"):
+            raise ReadonlyConfigError("Cannot change read-only config container")
+
         input_config = isinstance(value, Container)
         target_node_ref = self._get_node(key)
         special_value = value is None or value == "???"

omegaconf/dictconfig.py

@@ -163,15 +163,6 @@ def _validate_set_merge_impl(self, key: Any, value: Any, is_assign: bool) -> Non
         else:
             target = self._get_node(key)
 
-        if (target is not None and target._get_flag("readonly")) or self._get_flag(
-            "readonly"
-        ):
-            if is_assign:
-                msg = f"Cannot assign to read-only node : {value}"
-            else:
-                msg = f"Cannot merge into read-only node : {value}"
-            raise ReadonlyConfigError(msg)
-
         if target is None:
             return
 

omegaconf/nodes.py

@@ -10,6 +10,7 @@
     MissingMandatoryValue,
     UnsupportedValueType,
     ValidationError,
+    ReadonlyConfigError,
 )
 
 
@@ -26,6 +27,9 @@ def _value(self) -> Any:
     def _set_value(self, value: Any) -> None:
         from ._utils import ValueKind, get_value_kind
 
+        if self._get_flag("readonly"):
+            raise ReadonlyConfigError("Cannot set value of read-only config node")
+
         if isinstance(value, str) and get_value_kind(value) in (
             ValueKind.INTERPOLATION,
             ValueKind.STR_INTERPOLATION,

omegaconf/omegaconf.py

@@ -42,6 +42,7 @@
     is_structured_config,
     isint,
     type_str,
+    _convert_to_omegaconf_container,
 )
 from .base import Container, Node
 from .basecontainer import BaseContainer
@@ -300,12 +301,7 @@ def merge(
         """Merge a list of previously created configs into a single one"""
         assert len(others) > 0
         target = copy.deepcopy(others[0])
-        if is_primitive_container(target):
-            assert isinstance(target, (list, dict))
-            target = OmegaConf.create(target)
-        elif is_structured_config(target):
-            target = OmegaConf.structured(target)
-        assert isinstance(target, (DictConfig, ListConfig))
+        target = _convert_to_omegaconf_container(target)
         with flag_override(target, "readonly", False):
             target.merge_with(*others[1:])
         return target

tests/structured_conf/data/attr_classes.py

@@ -242,6 +242,12 @@ class FrozenClass:
     list: List[int] = [1, 2, 3]
 
 
+@attr.s(auto_attribs=True)
+class ContainsFrozen:
+    x: int = 10
+    frozen: FrozenClass = FrozenClass()
+
+
 @attr.s(auto_attribs=True)
 class WithTypedList:
     list: List[int] = [1, 2, 3]

tests/structured_conf/data/dataclasses.py

@@ -238,6 +238,12 @@ class FrozenClass:
     list: List[int] = field(default_factory=lambda: [1, 2, 3])
 
 
+@dataclass
+class ContainsFrozen:
+    x: int = 10
+    frozen: FrozenClass = FrozenClass()
+
+
 @dataclass
 class WithTypedList:
     list: List[int] = field(default_factory=lambda: [1, 2, 3])

tests/structured_conf/test_structured_config.py

@@ -822,3 +822,12 @@ def test_nested_with_any_var_type(self, class_type: str) -> None:
                 "interpolation": "${value_at_root}",
             }
         }
+
+    def test_noop_merge_into_frozen(self, class_type: str) -> None:
+        module: Any = import_module(class_type)
+        cfg = OmegaConf.structured(module.ContainsFrozen)
+        ret = OmegaConf.merge(cfg, {"x": 20, "frozen": {}})
+        assert ret == {
+            "x": 20,
+            "frozen": {"user": {"name": "Bart", "age": 10}, "x": 10, "list": [1, 2, 3]},
+        }

tests/test_errors.py

@@ -225,7 +225,7 @@ def finalize(self, cfg: Any) -> None:
             create=lambda: create_readonly({"foo": "bar"}),
             op=lambda cfg: setattr(cfg, "foo", 20),
             exception_type=ReadonlyConfigError,
-            msg="Cannot assign to read-only node : 20",
+            msg="Cannot change read-only config container",
             key="foo",
             child_node=lambda cfg: cfg.foo,
         ),
@@ -325,13 +325,13 @@ def finalize(self, cfg: Any) -> None:
         ),
         id="DictConfig[str,str]:getitem_color_key",
     ),
-    # merge
     pytest.param(
         Expected(
-            create=lambda: create_readonly({"foo": "bar"}),
-            op=lambda cfg: cfg.merge_with(OmegaConf.create()),
+            create=lambda: create_readonly({"foo1": "bar"}),
+            op=lambda cfg: cfg.merge_with({"foo2": "bar"}),
             exception_type=ReadonlyConfigError,
-            msg="Cannot merge into read-only node",
+            key="foo2",
+            msg="Cannot change read-only config container",
         ),
         id="dict,readonly:merge_with",
     ),

tests/test_readonly.py

@@ -10,37 +10,64 @@
 @pytest.mark.parametrize(  # type: ignore
     "src, func, expectation",
     [
-        ({}, lambda c: c.__setitem__("a", 1), raises(ReadonlyConfigError, match="a")),
-        (
-            dict(a=dict(b=dict(c=1))),
+        pytest.param(
+            {},
+            lambda c: c.__setitem__("a", 1),
+            raises(ReadonlyConfigError, match="a"),
+            id="dict_setitem",
+        ),
+        pytest.param(
+            {"a": {"b": {"c": 1}}},
             lambda c: c.__getattr__("a").__getattr__("b").__setitem__("c", 1),
             raises(ReadonlyConfigError, match="a.b.c"),
+            id="dict_nested_setitem",
         ),
-        (
+        pytest.param(
             {},
             lambda c: OmegaConf.update(c, "a.b", 10),
             raises(ReadonlyConfigError, match="a"),
+            id="dict_update",
         ),
-        (
-            dict(a=10),
+        pytest.param(
+            {"a": 10},
             lambda c: c.__setattr__("a", 1),
             raises(ReadonlyConfigError, match="a"),
+            id="dict_setattr",
         ),
-        (dict(a=10), lambda c: c.pop("a"), raises(ReadonlyConfigError, match="a")),
-        (
-            dict(a=10),
+        pytest.param(
+            {"a": 10},
+            lambda c: c.pop("a"),
+            raises(ReadonlyConfigError, match="a"),
+            id="dict_pop",
+        ),
+        pytest.param(
+            {"a": 10},
             lambda c: c.__delitem__("a"),
             raises(ReadonlyConfigError, match="a"),
+            id="dict_delitem",
         ),
         # list
-        ([], lambda c: c.__setitem__(0, 1), raises(ReadonlyConfigError, match="0")),
-        (
+        pytest.param(
+            [],
+            lambda c: c.__setitem__(0, 1),
+            raises(ReadonlyConfigError, match="0"),
+            id="list_setitem",
+        ),
+        pytest.param(
             [],
             lambda c: OmegaConf.update(c, "0.b", 10),
             raises(ReadonlyConfigError, match="[0]"),
+            id="list_update",
+        ),
+        pytest.param(
+            [10], lambda c: c.pop(), raises(ReadonlyConfigError), id="list_pop"
+        ),
+        pytest.param(
+            [0],
+            lambda c: c.__delitem__(0),
+            raises(ReadonlyConfigError, match="[0]"),
+            id="list_delitem",
         ),
-        ([10], lambda c: c.pop(), raises(ReadonlyConfigError)),
-        ([0], lambda c: c.__delitem__(0), raises(ReadonlyConfigError, match="[0]")),
     ],
 )
 def test_readonly(
@@ -154,3 +181,45 @@ def test_readonly_from_cli() -> None:
     cfg2 = OmegaConf.merge(c, cli)
     assert OmegaConf.is_readonly(c)
     assert OmegaConf.is_readonly(cfg2)
+
+
+@pytest.mark.parametrize(
+    "cfg1, cfg2",
+    [
+        pytest.param({"foo": {"bar": 10}}, {"foo": {"bar": 20}}, id="override_value"),
+        pytest.param({"foo": {"bar": 10}}, {"foo": {"yup": 20}}, id="adding_key"),
+        pytest.param({"a": 1}, {"b": 2}, id="adding_key"),
+        pytest.param({"a": 1}, OmegaConf.create({"b": 2}), id="adding_key"),
+    ],
+)
+def test_merge_with_readonly(cfg1, cfg2) -> None:
+    c = OmegaConf.create(cfg1)
+    OmegaConf.set_readonly(c, True)
+    with raises(ReadonlyConfigError):
+        c.merge_with(cfg2)
+
+
+@pytest.mark.parametrize(
+    "readonly_key, cfg1, cfg2, expected",
+    [
+        pytest.param(
+            "",
+            {"foo": {"bar": 10}},
+            {"foo": {}},
+            {"foo": {"bar": 10}},
+            id="merge_empty_dict",
+        ),
+        pytest.param(
+            "foo",
+            {"foo": {"bar": 10}},
+            {"xyz": 10},
+            {"foo": {"bar": 10}, "xyz": 10},
+            id="merge_different_node",
+        ),
+    ],
+)
+def test_merge_with_readonly_nop(readonly_key, cfg1, cfg2, expected) -> None:
+    c = OmegaConf.create(cfg1)
+    OmegaConf.set_readonly(OmegaConf.select(c, readonly_key), True)
+    c.merge_with(cfg2)
+    assert c == OmegaConf.create(expected)