Merge pull request #79 from NextThought/tighter-anon-dict

InterfaceObjectIO checks if a IDict wants Object for its value before returning a factory

CHANGES.rst

@@ -6,7 +6,8 @@
 1.0.0a6 (unreleased)
 ====================
 
-- Nothing changed yet.
+- ``InterfaceObjectIO`` only returns an anonymous factory for ``IDict``
+  fields when it wants objects for the value.
 
 
 1.0.0a5 (2018-07-30)

src/nti/externalization/_datastructures.pxd

@@ -17,8 +17,8 @@ from nti.externalization.internalization._factories cimport find_factory_for
 
 from nti.externalization.__interface_cache cimport cache_for
 
-
-
+cdef IDict
+cdef IObject
 cdef IInternalObjectIOFinder
 cdef IAnonymousObjectFactory
 cdef SEF StandardExternalFields
@@ -29,6 +29,7 @@ cdef isSyntheticKey
 cdef find_most_derived_interface
 cdef NotGiven
 cdef IDict_providedBy
+cdef IObject_providedBy
 cdef _anonymous_dict_factory
 
 cdef class ExternalizableDictionaryMixin(object):

src/nti/externalization/datastructures.py

@@ -23,6 +23,7 @@
 from zope import schema
 from zope.schema.interfaces import SchemaNotProvided
 from zope.schema.interfaces import IDict
+from zope.schema.interfaces import IObject
 
 from nti.schema.interfaces import find_most_derived_interface
 
@@ -52,6 +53,7 @@
 StandardExternalFields = get_standard_external_fields()
 StandardInternalFields = get_standard_internal_fields()
 IDict_providedBy = IDict.providedBy
+IObject_providedBy = IObject.providedBy
 
 __all__ = [
     'ExternalizableDictionaryMixin',
@@ -381,10 +383,12 @@ class ExternalizableInstanceDict(object):
     Meant to be used as a super class; also can be used as an external
     object superclass.
 
+    Consider carefully before using this class. Generally, an interface
+    and `InterfaceObjectIO` are better.
+
     .. versionchanged:: 1.0a5
        No longer extends `AbstractDynamicObjectIO`, just delegates to it.
-    .. deprecated:: 1.0a5
-       Prefer interfaces.
+       Most of the `_ext_`` prefixed methods can no longer be overridden.
     """
     # This class is sometimes subclassed while also subclassing persistent.Persistent,
     # which doesn't work if it's an extension class with an incompatible layout,
@@ -593,6 +597,15 @@ def find_factory_for_named_value(self, key, value, registry):
         A second limitation is that the external data key must match
         the internal schema field name. Again, the only way to
         remove this limitation is to subclass this object.
+
+        If no registered factory is found, and the schema field is
+        a `zope.schema.Dict` with a value type of `zope.schema.Object`,
+        then we return a factory which will update the object in place.
+
+        .. versionchanged:: 1.0a6
+           Only return an anonymous factory for ``IDict`` fields when
+           it wants objects for the value.
+
         """
         factory = AbstractDynamicObjectIO.find_factory_for_named_value(self, key, value, registry)
         if factory is None:
@@ -616,7 +629,12 @@ def find_factory_for_named_value(self, key, value, registry):
                 if isinstance(factory, str):
                     factory = registry.getUtility(IAnonymousObjectFactory, factory)
 
-                if factory is None and IDict_providedBy(field) and isinstance(value, dict):
+                if (
+                        factory is None
+                        and IDict_providedBy(field)
+                        and isinstance(value, dict)
+                        and IObject_providedBy(field.value_type)
+                ):
                     # If is no factory found, check to see if the
                     # schema field is a Dict with a complex value type, and if
                     # so, automatically update it in place. The alternative

src/nti/externalization/tests/test_datastructures.py

@@ -21,6 +21,7 @@
 from hamcrest import has_property
 from hamcrest import is_
 from hamcrest import is_not as does_not
+is_not = does_not
 from hamcrest import none
 
 # disable: accessing protected members, too many methods
@@ -450,6 +451,64 @@ class O(object):
         with self.assertRaises(component.ComponentLookupError):
             inst.find_factory_for_named_value('field', {}, component)
 
+    def test_no_factory_for_dict_with_no_types(self):
+        from zope.schema import Dict
+        from zope import component
+
+        class I(interface.Interface):
+            field = Dict(title=u'A blank field')
+
+        @interface.implementer(I)
+        class O(object):
+            pass
+
+        inst = self._makeOne(O(), iface_upper_bound=I)
+        factory = inst.find_factory_for_named_value('field', {}, component)
+        assert_that(factory, is_(none()))
+
+    def test_no_factory_for_dict_with_non_object_value(self):
+        from zope.schema import Dict
+        from zope.schema import Object
+        from zope.schema import TextLine
+        from zope import component
+
+        class I(interface.Interface):
+            field = Dict(
+                title=u'A blank field',
+                value_type=TextLine(title=u'text')
+            )
+
+        @interface.implementer(I)
+        class O(object):
+            pass
+
+        inst = self._makeOne(O(), iface_upper_bound=I)
+        factory = inst.find_factory_for_named_value('field', {}, component)
+        assert_that(factory, is_(none()))
+
+    def test_factory_for_dict_with_object_value(self):
+        from zope.schema import Dict
+        from zope.schema import Object
+        from zope import component
+
+        class I2(interface.Interface):
+            pass
+
+        class I(interface.Interface):
+            field = Dict(
+                title=u'A blank field',
+                value_type=Object(I2)
+            )
+
+        @interface.implementer(I)
+        class O(object):
+            pass
+
+        inst = self._makeOne(O(), iface_upper_bound=I)
+        factory = inst.find_factory_for_named_value('field', {}, component)
+        assert_that(factory, is_not(none()))
+
+
 
 class TestModuleScopedInterfaceObjectIO(TestInterfaceObjectIO):
 
@@ -541,15 +600,18 @@ def __init__(self, replacement):
                 super(MappingIO, self).__init__()
                 self.context = replacement
 
+            # These are never called since we create an instance
+            # of a different class during update.
+
             def _ext_setattr(self, ext_self, k, v):
-                ext_self[k] = v
+                raise NotImplementedError
+
+            def _ext_accept_update_key(self, k, unused_ext_self, unused_ext_keys):
+                raise NotImplementedError
 
             def _ext_getattr(self, ext_self, k):
                 return ext_self.get(k)
 
-            def _ext_accept_update_key(self, k, unused_ext_self, unused_ext_keys):
-                return not k.startswith('_')
-
             def _ext_replacement(self):
                 return self.context