ValueNode.validate_and_convert() now properly validates None

omegaconf/base.py

@@ -554,7 +554,7 @@ def quoted_string_callback(quoted_str: str) -> str:
                     value=quoted_str,
                     key=key,
                     parent=parent,
-                    is_optional=False,
+                    is_optional=True,
                 ),
                 throw_on_resolution_failure=True,
             )

omegaconf/dictconfig.py

@@ -292,9 +292,7 @@ def _s_validate_and_normalize_key(self, key_type: Any, key: Any) -> DictKeyType:
             return key  # type: ignore
         elif issubclass(key_type, Enum):
             try:
-                ret = EnumNode.validate_and_convert_to_enum(
-                    key_type, key, allow_none=False
-                )
+                ret = EnumNode.validate_and_convert_to_enum(key_type, key)
                 assert ret is not None
                 return ret
             except ValidationError:

omegaconf/nodes.py

@@ -1,6 +1,7 @@
 import copy
 import math
 import sys
+from abc import abstractmethod
 from enum import Enum
 from typing import Any, Dict, Optional, Type, Union
 
@@ -40,17 +41,24 @@ def _set_value(self, value: Any, flags: Optional[Dict[str, bool]] = None) -> Non
         ):
             self._val = value
         else:
-            if not self._metadata.optional and value is None:
-                raise ValidationError("Non optional field cannot be assigned None")
             self._val = self.validate_and_convert(value)
 
     def validate_and_convert(self, value: Any) -> Any:
         """
         Validates input and converts to canonical form
         :param value: input value
-        :return:  converted value ("100" may be converted to 100 for example)
+        :return: converted value ("100" may be converted to 100 for example)
         """
-        return value
+        if value is None:
+            if self._is_optional():
+                return None
+            raise ValidationError("Non optional field cannot be assigned None")
+        # Subclasses can assume that `value` is not None in `_validate_and_convert_impl()`.
+        return self._validate_and_convert_impl(value)
+
+    @abstractmethod
+    def _validate_and_convert_impl(self, value: Any) -> Any:
+        ...
 
     def __str__(self) -> str:
         return str(self._val)
@@ -123,7 +131,7 @@ def __init__(
             ),
         )
 
-    def validate_and_convert(self, value: Any) -> Any:
+    def _validate_and_convert_impl(self, value: Any) -> Any:
         from ._utils import is_primitive_type
 
         # allow_objects is internal and not an official API. use at your own risk.
@@ -159,12 +167,12 @@ def __init__(
             ),
         )
 
-    def validate_and_convert(self, value: Any) -> Optional[str]:
+    def _validate_and_convert_impl(self, value: Any) -> Optional[str]:
         from omegaconf import OmegaConf
 
         if OmegaConf.is_config(value) or is_primitive_container(value):
             raise ValidationError("Cannot convert '$VALUE_TYPE' to string : '$VALUE'")
-        return str(value) if value is not None else None
+        return str(value)
 
     def __deepcopy__(self, memo: Dict[int, Any]) -> "StringNode":
         res = StringNode()
@@ -188,11 +196,9 @@ def __init__(
             ),
         )
 
-    def validate_and_convert(self, value: Any) -> Optional[int]:
+    def _validate_and_convert_impl(self, value: Any) -> Optional[int]:
         try:
-            if value is None:
-                val = None
-            elif type(value) in (str, int):
+            if type(value) in (str, int):
                 val = int(value)
             else:
                 raise ValueError()
@@ -222,9 +228,7 @@ def __init__(
             ),
         )
 
-    def validate_and_convert(self, value: Any) -> Optional[float]:
-        if value is None:
-            return None
+    def _validate_and_convert_impl(self, value: Any) -> Optional[float]:
         try:
             if type(value) in (float, str, int):
                 return float(value)
@@ -273,16 +277,14 @@ def __init__(
             ),
         )
 
-    def validate_and_convert(self, value: Any) -> Optional[bool]:
+    def _validate_and_convert_impl(self, value: Any) -> Optional[bool]:
         if isinstance(value, bool):
             return value
         if isinstance(value, int):
             return value != 0
-        elif value is None:
-            return None
         elif isinstance(value, str):
             try:
-                return self.validate_and_convert(int(value))
+                return self._validate_and_convert_impl(int(value))
             except ValueError as e:
                 if value.lower() in ("yes", "y", "on", "true"):
                     return True
@@ -335,16 +337,13 @@ def __init__(
             ),
         )
 
-    def validate_and_convert(self, value: Any) -> Optional[Enum]:
+    def _validate_and_convert_impl(self, value: Any) -> Optional[Enum]:
         return self.validate_and_convert_to_enum(enum_type=self.enum_type, value=value)
 
     @staticmethod
     def validate_and_convert_to_enum(
-        enum_type: Type[Enum], value: Any, allow_none: bool = True
+        enum_type: Type[Enum], value: Any
     ) -> Optional[Enum]:
-        if allow_none and value is None:
-            return None
-
         if not isinstance(value, (str, int)) and not isinstance(value, enum_type):
             raise ValidationError(
                 f"Value $VALUE ($VALUE_TYPE) is not a valid input for {enum_type}"

tests/test_interpolation.py

@@ -830,6 +830,15 @@ def drop_last(s: str) -> str:
             ),
             id="type_mismatch_node_interpolation",
         ),
+        pytest.param(
+            StructuredWithMissing(opt_num=None, num=II("opt_num")),
+            "num",
+            pytest.raises(
+                ValidationError,
+                match=re.escape("Non optional field cannot be assigned None"),
+            ),
+            id="non_optional_node_interpolation",
+        ),
     ],
 )
 def test_interpolation_type_validated_error(
@@ -861,3 +870,9 @@ def test_resolver_output_listconfig(restore_resolvers: Any) -> None:
     cfg = OmegaConf.create({"x": "${list:}"})
     assert isinstance(cfg.x, ListConfig)
     assert cfg.x == [0, 1]
+
+
+def test_none_value_in_quoted_string(restore_resolvers: Any) -> None:
+    OmegaConf.register_new_resolver("test", lambda x: x)
+    cfg = OmegaConf.create({"x": "${test:'${missing}'}", "missing": None})
+    assert cfg.x == "None"