Skip to content

Commit

Permalink
Fixed Merging into a MISSING Structured config
Browse files Browse the repository at this point in the history
  • Loading branch information
omry committed Jun 20, 2020
1 parent 92a1d5c commit 92ef9f5
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 38 deletions.
1 change: 1 addition & 0 deletions news/269.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Merging into a MISSING Structured config node expands the node first to ensure the result is legal
1 change: 1 addition & 0 deletions news/271.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix merging into a config with a read only node if merge is not mutating that node
12 changes: 3 additions & 9 deletions omegaconf/basecontainer.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@
_convert_to_omegaconf_container,
_get_value,
_is_interpolation,
_resolve_optional,
get_ref_type,
get_value_kind,
get_yaml_loader,
is_dict_annotation,
is_primitive_container,
is_primitive_dict,
is_structured_config,
get_ref_type,
is_dict_annotation,
_resolve_optional,
)
from .base import Container, ContainerMetadata, Node
from .errors import MissingMandatoryValue, ReadonlyConfigError, ValidationError
Expand Down Expand Up @@ -232,10 +232,7 @@ def pretty(self, resolve: bool = False, sort_keys: bool = False) -> str:
@staticmethod
def _map_merge(dest: "BaseContainer", src: "BaseContainer") -> None:
"""merge src into dest and return a new copy, does not modified input"""
from omegaconf import OmegaConf

from .dictconfig import DictConfig
from .listconfig import ListConfig
from .nodes import ValueNode

assert isinstance(dest, DictConfig)
Expand All @@ -253,9 +250,6 @@ def _map_merge(dest: "BaseContainer", src: "BaseContainer") -> None:
else:
dest._set_value(type_)

elif isinstance(dest, ListConfig) and dest._is_missing():
dest._set_value([])

for key, src_value in src.items_ex(resolve=False):
dest_node = dest._get_node(key, validate_access=False)
if dest_node is not None:
Expand Down
10 changes: 10 additions & 0 deletions tests/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,3 +104,13 @@ class MissingDict:
class DictEnum:
color_key: Dict[Color, str] = field(default_factory=lambda: {})
color_val: Dict[str, Color] = field(default_factory=lambda: {})


@dataclass
class A:
a: int = 10


@dataclass
class B:
x: A = MISSING
45 changes: 16 additions & 29 deletions tests/test_merge.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from typing import Any, Tuple
from dataclasses import dataclass

import pytest

from omegaconf import (
Expand All @@ -14,6 +14,7 @@
from omegaconf._utils import is_structured_config

from . import (
B,
ConcretePlugin,
ConfWithMissingDict,
Group,
Expand All @@ -25,16 +26,6 @@
)


@dataclass
class A:
a: int = 10


@dataclass
class B:
x: A = MISSING


@pytest.mark.parametrize( # type: ignore
"inputs, expected",
[
Expand All @@ -51,14 +42,26 @@ class B:
(({"a": 1}, {"a": nodes.IntegerNode(10)}), {"a": nodes.IntegerNode(10)}),
(({"a": nodes.IntegerNode(10)}, {"a": 1}), {"a": 1}),
(({"a": nodes.IntegerNode(10)}, {"a": 1}), {"a": nodes.IntegerNode(1)}),
pytest.param(({"a": "???"}, {"a": {}}), {"a": {}}, id="merge_into_missing"),
pytest.param(
({"a": "???"}, {"a": {"b": 10}}), {"a": {"b": 10}}, id="merge_into_missing"
({"a": "???"}, {"a": {}}), {"a": {}}, id="dict_merge_into_missing"
),
pytest.param(
({"a": "???"}, {"a": {"b": 10}}),
{"a": {"b": 10}},
id="dict_merge_into_missing",
),
# lists
(([1, 2, 3], [4, 5, 6]), [4, 5, 6]),
(([[1, 2, 3]], [[4, 5, 6]]), [[4, 5, 6]]),
(([1, 2, {"a": 10}], [4, 5, {"b": 20}]), [4, 5, {"b": 20}]),
pytest.param(
({"a": "???"}, {"a": []}), {"a": []}, id="list_merge_into_missing"
),
pytest.param(
({"a": "???"}, {"a": [1, 2, 3]}),
{"a": [1, 2, 3]},
id="list_merge_into_missing",
),
# Interpolations
# value interpolation
pytest.param(
Expand Down Expand Up @@ -217,22 +220,6 @@ def test_merge(inputs: Any, expected: Any) -> None:
OmegaConf.merge(*configs)


"""
from omegaconf import OmegaConf, MISSING
from dataclasses import dataclass
@dataclass
class A:
a : int = 10
@dataclass
class B:
x : A = MISSING
OmegaConf.merge(B, {"x": {}})
"""


def test_merge_error_retains_type() -> None:
cfg = OmegaConf.structured(ConcretePlugin)
with pytest.raises(ValidationError):
Expand Down

0 comments on commit 92ef9f5

Please sign in to comment.