Merge pull request #62 from NextThought/issue59

Variant requires Dict to have both key and value.

CHANGES.rst

@@ -13,6 +13,19 @@
   deprecation warnings it emits more precise. See `issue 58
   <https://github.com/NextThought/nti.schema/issues/58>`_.
 
+- Fix constructing a ``Variant`` to do some extra validation of
+  ``IMapping`` fields to require that both the ``key_type`` and
+  ``value_type`` are given. Previously, if one or both were left
+  ``None``, an ``AttributeError`` would be raised when the field was
+  asked to validate data or its ``fromObject()`` method was called.
+  Now, a ``RequiredMissing`` error will be raised when the ``Variant``
+  is created. Some other nested uses of ``Dict`` fields, such as a
+  ``Sequence``, could also have raised the ``AttributeError``; this
+  may change in the future to raise at construction time as well.
+
+  See `issue 59 <https://github.com/NextThought/nti.schema/issues>`_.
+
+
 1.15.1 (2020-07-02)
 ===================
 

src/nti/schema/field.py

@@ -166,7 +166,7 @@ def X(cls):
         return cls
     return X
 
-def _fixup_Object_field(field):
+def _fixup_Object_field(field, early_error=False):
     # If possible, make the field provide IFromObject
     if not IFromObject.providedBy(field):
         if isinstance(field, _ObjectBase):
@@ -198,10 +198,21 @@ def _collection_fromObject(value):
             # Has a value_type and key_type
             try:
                 # pylint:disable=protected-access
-                _SequenceFromObjectMixin._validate_contained_field(field.value_type)
-                _SequenceFromObjectMixin._validate_contained_field(field.key_type)
+                _SequenceFromObjectMixin._validate_contained_field(field.value_type,
+                                                                   required=True,
+                                                                   early_error=early_error)
+                _SequenceFromObjectMixin._validate_contained_field(field.key_type,
+                                                                   required=True,
+                                                                   early_error=early_error)
+            except sch_interfaces.RequiredMissing:
+                # Previously, we could pass with None for the key or value. That *will*
+                # blow up in the fromObject case, though. So now we don't add IFromObject
+                # that we *know* will blow up. Further, we raise this error so that the invalid
+                # Dict gets caught, when requested
+                if early_error:
+                    raise
             except sch_interfaces.SchemaNotProvided:
-                # Nothing we can do
+                # Nothing to add.
                 pass
             else:
                 def _map_fromObject(value):
@@ -388,6 +399,12 @@ class Variant(FieldValidationMixin, schema.Field):
         :class:`zope.schema.Dict`) will automatically be given a ``fromObject`` method
         when they are used as a field of this object *if* their *value_type* is an
         :class:`zope.schema.interfaces.IObject` (recursively).
+
+    .. versionchanged:: 1.16.0
+       If one of the fields of the variant is (recursively) a ``IMapping``
+       such as a ``Dict``, both the key and value type must be specified. Previously,
+       if they were left ``None``, a validation-time :exc:`AttributeError`
+       would be raised. Now, constructing the variant will raise a ``RequiredMissing``.
     """
 
     fields = ()
@@ -401,14 +418,13 @@ def __init__(self, fields, variant_raise_when_schema_provided=False, **kwargs):
             a validation error, that error will be propagated instead
             of the error raised by the last field, and no additional fields
             will be asked to do validation.
-
         """
         if not fields or not all((sch_interfaces.IField.providedBy(x) for x in fields)):
             raise sch_interfaces.SchemaNotProvided(sch_interfaces.IField)
 
         # assign our children first so anything we copy to them as a result of the super
         # constructor (__name__) gets set
-        self.fields = [_fixup_Object_field(field) for field in fields]
+        self.fields = [_fixup_Object_field(field, early_error=True) for field in fields]
         for f in self.fields:
             f.__parent__ = self
 
@@ -766,12 +782,15 @@ def __init__(self, *args, **kwargs):
         self._validate_contained_field(self.value_type)
 
     @classmethod
-    def _validate_contained_field(cls, field):
-        field = _fixup_Object_field(field)
-        # We allow None for a value type
+    def _validate_contained_field(cls, field, required=False, early_error=False):
+        # We allow None for a value type, unless required=True
+        if field is None and required:
+            raise sch_interfaces.RequiredMissing()
+        field = _fixup_Object_field(field, early_error=early_error)
         if field is not None and not any(
-                (iface.providedBy(field)
-                 for iface in (IFromObject, IFromUnicode, IFromBytes))
+                iface.providedBy(field)
+                for iface
+                in (IFromObject, IFromUnicode, IFromBytes)
         ):
             raise sch_interfaces.SchemaNotProvided(IFromObject, field)
 

src/nti/schema/tests/test_field.py

@@ -155,6 +155,20 @@ def test_variant(self):
                     has_property('errors',
                                  has_length(len(syntax_or_lookup.fields))))
 
+    def test_variant_with_dict_requires_both(self):
+        from zope.schema.interfaces import RequiredMissing
+        with self.assertRaises(RequiredMissing):
+            Variant((Dict(),))
+
+        with self.assertRaises(RequiredMissing):
+            Variant((Dict(key_type=TextLine()),))
+
+        with self.assertRaises(RequiredMissing):
+            Variant((Dict(value_type=TextLine()),))
+
+        with self.assertRaises(RequiredMissing):
+            Variant((Dict(value_type=Dict(), key_type=TextLine()),))
+
     def test_getDoc(self):
         syntax_or_lookup = Variant((Object(cmn_interfaces.ISyntaxError),
                                     Object(cmn_interfaces.ILookupError),
@@ -818,3 +832,39 @@ def fromBytes(self, value):
         assert_that(converter(b''), is_(b'from bytes'))
         assert_that(converter(u''), is_(u'from unicode'))
         assert_that(converter(1), is_(b'from object'))
+
+class TestFunctions(unittest.TestCase):
+
+    def test_fixup_Object_field_mapping_requires_key_and_value(self):
+        from zope.interface import implementer
+        from zope.schema.interfaces import IMapping
+        from nti.schema.field import _fixup_Object_field
+
+        @implementer(IMapping)
+        class Mapping(object):
+            value_type = None
+            key_type = None
+
+        # neither
+        field = Mapping()
+        _fixup_Object_field(field)
+        assert_that(field, does_not(verifiably_provides(IFromObject)))
+
+        # An invalid key type
+        field = Mapping()
+        field.key_type = TextLine()
+        _fixup_Object_field(field)
+        assert_that(field, does_not(verifiably_provides(IFromObject)))
+
+        # An invalid value type
+        field = Mapping()
+        field.value_type = TextLine()
+        _fixup_Object_field(field)
+        assert_that(field, does_not(verifiably_provides(IFromObject)))
+
+        # Both valid
+        field = Mapping()
+        field.value_type = TextLine()
+        field.key_type = TextLine()
+        _fixup_Object_field(field)
+        assert_that(field, verifiably_provides(IFromObject))