Fix bug that allowed containers to be set as a value in ValueNodes #335

pereman2 · 2020-08-20T21:23:55Z

Closes #324 .
I added tests but I think maybe I should remove the one in test_structured_config since is essentially the same as test_errors.
The variable should_set_value is getting pretty big but this fixed it.

omegaconf/basecontainer.py

tests/structured_conf/test_structured_config.py

tests/test_errors.py

news/324.bugfix

omegaconf/basecontainer.py

omegaconf/nodes.py

omegaconf/basecontainer.py

omry · 2020-08-23T01:50:37Z

omegaconf/_utils.py

+    return is_dict_container(obj) or is_list_container(obj)
+
+
+def is_list_container(obj: Any) -> bool:


this would be true, and it's clearly not a list container.

is_list_container(OmegaConf.create({"foo": 10})

omry · 2020-08-23T01:51:56Z

omegaconf/_utils.py

+def is_container(obj: Any) -> bool:
+    return is_dict_container(obj) or is_list_container(obj)


You already have OmegaConf.is_config() and utils.is_primitive_container().
Use those writing so many new functions.

omry · 2020-08-23T01:54:10Z

omegaconf/nodes.py

@@ -177,7 +177,7 @@ def __init__(
        )

    def validate_and_convert(self, value: Any) -> Optional[str]:
-        if is_primitive_container(value):
+        if is_container(value):


Suggested change

if is_container(value):

from omegaconf import OmegaConf

if OmegaConf.is_config(value) or utils.is_primitive_container(value):

omegaconf/basecontainer.py

omry · 2020-08-23T16:13:57Z

omegaconf/basecontainer.py

+            # if target_node is a valuenode and is not an anynode then
+            # it should set value


why is AnyNode different than other nodes in this context?

Because AnyNode could be anything from primitive to container?

You are not supposed to put containers in AnyNode, containers are ALWAYS converted to DictConfig or ListConfig.

Then if you make a config cfg = OmegaConf.create({"foo": 4}) and cfg.foo = [] it should wrap a new ListConfig. But if instead the config is:

@dataclass class C: foo: Any = 4 cfg= OmegaConf.structured(C) cfg.foo = []

it should give an error?

No, it should do the same in both cases.
Why would the second one give an error?

Because AnyNode doesn't allow containers.

As I said, that container (dict or list) should be wrapped in a DictConfig or a ListConfig and REPLACE the AnyNode.
It can do that because the ref_type allows it.

Then an AnyNode will never set its value it will always wrap a new node even if it's a primitive type. ref_type=Any is the default so it should never be a problem wrapping always.

omry · 2020-08-23T16:19:22Z

omegaconf/basecontainer.py

+                # Target node is a container and has an special value or has an
+                # ref_type assigned


so this comment does not really explain the reason for needing a set.
That sentence does not make sense to me.
a sentence that would make sense:

# We use set_value if: # 1. input is a primitive and the value is a leaf node # 2. Input is anything any the target is MISSING or None # 3. ...

Can you maybe explain it in terms of when do we NOT use set_value?
Don't explain the code ,explain the reasons behind it.
I know this is hard, but I think it will lead to much better code here overall.

omry · 2020-08-23T16:22:49Z

omegaconf/basecontainer.py

-                self.__dict__["_content"][key]._set_value(value)
+                self.__dict__["_content"][key] = wrap(key, value)


see, the fact you can do that means that wrap is actually not wrap. (indeed internally it calls maybe wrap.
it feels like there is an opportunity for a significant cleanup of this function.

However, this does not need to happen in this diff.
Let's try to at least clarify what we are doing here and why. Actual changes can happen later.

omry · 2020-08-23T16:25:11Z

omegaconf/nodes.py

+        from omegaconf import OmegaConf
+
+        if OmegaConf.is_config(value) or is_primitive_container(value):
+            raise ValidationError(
+                "Value '$VALUE' is not a valid string (type $VALUE_TYPE)"
+            )


AnyNode should also reject containers (+ tests).

did you address this? re-reviewing the entire diff looking for things like this is time consuming.
if you did not address this please do, if you did please clarify it here.

It rejects them with the current implementation, line149: if not is_primitive_type(value):

omegaconf/basecontainer.py

tests/test_basic_ops_dict.py

omry · 2020-08-25T18:04:00Z

omegaconf/nodes.py

+        from omegaconf import OmegaConf
+
+        if OmegaConf.is_config(value) or is_primitive_container(value):
+            raise ValidationError(
+                "Value '$VALUE' is not a valid string (type $VALUE_TYPE)"
+            )


did you address this? re-reviewing the entire diff looking for things like this is time consuming.
if you did not address this please do, if you did please clarify it here.

omegaconf/basecontainer.py

tests/test_nodes.py

omry · 2020-08-26T16:09:40Z

tests/test_nodes.py

@@ -74,6 +74,10 @@ def test_valid_inputs(type_: type, input_: Any, output_: Any) -> None:
    assert str(node) == str(output_)


+class Person:


There is already a class we can use for this, look for "IllegalType"

remove class Person and use IllegalType instead.

omry · 2020-08-26T16:10:12Z

tests/test_nodes.py

-    with pytest.raises(ValidationError):
-        type_(input_)
+    if type_ == AnyNode:
+        with pytest.raises(UnsupportedValueType):


didn't we say we can make UnsupportedValueType a subclass of ValidationError?

Ah I thought you meant we could make them on 2.1

I misunderstood you, I'll change it.

You don't need to special-case AnyNode now that the Exception is a ValidationError.

pereman2 · 2020-08-27T15:20:52Z

I rebased incorrectly i'll re-commit.

lgtm-com · 2020-08-27T15:26:14Z

This pull request introduces 2 alerts when merging 0cdb64f into 2579bd8 - view on LGTM.com

new alerts:

1 for Unused import
1 for Syntax error

omry · 2020-08-27T16:54:52Z

tests/test_basic_ops_dict.py

@@ -539,17 +539,39 @@ def test_set_with_invalid_key() -> None:
        cfg[1] = "a"  # type: ignore


-def test_set_anynode() -> None:
+@pytest.mark.parametrize("value", [1, 3.14, True, None, Enum1.FOO])  # type: ignore


these tests should probably be in test_nodes.py

Well this one tests if the node is not replaced. That cannot be done in test_nodes.

But yeah, I missed adding these testcases in test_valid_inputs in test_nodes

It's okay. Please move the tests to test_nodes.

omry

I am ready to release Hydra 1.0 and I am going to release OmegaConf 2.0.1 as well.
Can you finish your two pending pull requests during the weekend?
If not I can take over.

omry · 2020-08-29T05:36:41Z

tests/test_basic_ops_dict.py

@@ -539,17 +539,39 @@ def test_set_with_invalid_key() -> None:
        cfg[1] = "a"  # type: ignore


-def test_set_anynode() -> None:
+@pytest.mark.parametrize("value", [1, 3.14, True, None, Enum1.FOO])  # type: ignore


It's okay. Please move the tests to test_nodes.

omry · 2020-08-29T05:39:07Z

tests/test_errors.py

+    pytest.param(
+        Expected(
+            create=lambda: OmegaConf.structured(User),
+            op=lambda cfg: cfg.__setitem__("name", [1, 2]),


Why not cfg.name = [1,2] ?

Lambda doesn't allow that, and every other test uses setitem

oh, right. forgot about that shit.

omry · 2020-08-29T05:40:48Z

tests/test_errors.py

+            create=lambda: OmegaConf.structured(User),
+            op=lambda cfg: cfg.__setitem__("name", [1, 2]),
+            exception_type=ValidationError,
+            msg="Value '[1, 2]' is not a valid string (type list)",


This is a better error:

Cannot convert 'list' to string : '[1, 2]'

omry · 2020-08-29T05:41:17Z

tests/test_nodes.py

@@ -74,6 +74,10 @@ def test_valid_inputs(type_: type, input_: Any, output_: Any) -> None:
    assert str(node) == str(output_)


+class Person:


remove class Person and use IllegalType instead.

omry · 2020-08-29T05:43:06Z

tests/test_nodes.py

-    with pytest.raises(ValidationError):
-        type_(input_)
+    if type_ == AnyNode:
+        with pytest.raises(UnsupportedValueType):


You don't need to special-case AnyNode now that the Exception is a ValidationError.

omry

Awesome!

omry · 2020-08-29T16:37:43Z

tests/test_errors.py

+    pytest.param(
+        Expected(
+            create=lambda: OmegaConf.structured(User),
+            op=lambda cfg: cfg.__setitem__("name", [1, 2]),


oh, right. forgot about that shit.

pereman2 changed the title ~~Bug/valuenodes~~ Fix bug that allowed Containers to be set in unwished ValueNodes. Aug 20, 2020

omry changed the title ~~Fix bug that allowed Containers to be set in unwished ValueNodes.~~ Fix bug that allowed containers to be set as a value in ValueNodes Aug 20, 2020

omry requested changes Aug 22, 2020

View reviewed changes

omegaconf/basecontainer.py Outdated Show resolved Hide resolved

tests/structured_conf/test_structured_config.py Outdated Show resolved Hide resolved

tests/test_errors.py Outdated Show resolved Hide resolved

news/324.bugfix Outdated Show resolved Hide resolved

omry reviewed Aug 22, 2020

View reviewed changes

omegaconf/basecontainer.py Show resolved Hide resolved

omegaconf/nodes.py Outdated Show resolved Hide resolved

pereman2 commented Aug 23, 2020

View reviewed changes

omegaconf/basecontainer.py Outdated Show resolved Hide resolved

omry requested changes Aug 23, 2020

View reviewed changes

omry requested changes Aug 24, 2020

View reviewed changes

omegaconf/basecontainer.py Outdated Show resolved Hide resolved

omegaconf/basecontainer.py Show resolved Hide resolved

omry reviewed Aug 24, 2020

View reviewed changes

omegaconf/basecontainer.py Outdated Show resolved Hide resolved

tests/test_basic_ops_dict.py Outdated Show resolved Hide resolved

omry requested changes Aug 25, 2020

View reviewed changes

omry reviewed Aug 26, 2020

View reviewed changes

pereman2 force-pushed the bug/valuenodes branch from 0cdb64f to bc7f32c Compare August 27, 2020 16:12

omry reviewed Aug 27, 2020

View reviewed changes

pereman2 and others added 15 commits August 27, 2020 22:55

fix and tests

5b3b9bc

bugfix

ba34ef0

bugfix update

229ca6c

test cases for more types of nodes

8998e51

update news, move should_set_value, remove test

f880d20

Update 324.bugfix

2ce164d

refactor _set_value, add testcases

92fcd06

omegaconf.is_config and clean

117e0fa

set value primitive in anynode

e0a186d

test covering _set_item_impl with anynode

ae1c497

change explanation

add7b1f

remove print

01e8487

added test cases for anynode invalid set

7596772

test anynode test_basic_ops_dict

12849d5

add testcases test_valid_inputs

0f5cd66

pereman2 force-pushed the bug/valuenodes branch from 876f26f to 0f5cd66 Compare August 27, 2020 20:55

omry requested changes Aug 29, 2020

View reviewed changes

pereman2 added 2 commits August 29, 2020 17:18

change error msg, illegaltype

f9d59ff

move tests

a50b6a7

omry approved these changes Aug 29, 2020

View reviewed changes

omry merged commit 647c1c7 into omry:master Aug 29, 2020

		return is_dict_container(obj) or is_list_container(obj)


		def is_list_container(obj: Any) -> bool:

		def is_container(obj: Any) -> bool:
		return is_dict_container(obj) or is_list_container(obj)

		# if target_node is a valuenode and is not an anynode then
		# it should set value

		# Target node is a container and has an special value or has an
		# ref_type assigned

		self.__dict__["_content"][key]._set_value(value)
		self.__dict__["_content"][key] = wrap(key, value)

		@@ -74,6 +74,10 @@ def test_valid_inputs(type_: type, input_: Any, output_: Any) -> None:
		assert str(node) == str(output_)


		class Person:

Fix bug that allowed containers to be set as a value in ValueNodes #335

Fix bug that allowed containers to be set as a value in ValueNodes #335

Conversation

pereman2 commented Aug 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pereman2 commented Aug 27, 2020

lgtm-com bot commented Aug 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pereman2 Aug 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pereman2 Aug 27, 2020 •

edited

Loading