Merge pull request #89 from NextThought/feature/EID-allow-non-str

Make subclasses of ExternalizableInstanceDict deal with non-str keys

CHANGES.rst

@@ -9,6 +9,11 @@
 - The ``@WithRepr`` decorator takes into account the updated default
   repr of Persistent objects with persistent 4.4 and doesn't hide it.
 
+- Subclasses of ``ExternalizableInstanceDict`` that have non-str
+  (unicode on Python 2, bytes on Python 3) keys in their ``__dict__``
+  do not throw ``TypeError`` when externalizing. Instead, the non-str
+  values are converted to strs (using ASCII encoding) and the
+  ``_p_changed`` attribute, if any, is set.
 
 1.0.0a10 (2018-08-21)
 =====================

src/nti/externalization/_datastructures.pxd

@@ -42,7 +42,7 @@ cdef class ExternalizableDictionaryMixin(object):
 
 cdef class AbstractDynamicObjectIO(ExternalizableDictionaryMixin):
     cpdef _ext_replacement(self)
-    cpdef _ext_all_possible_keys(self)
+    cpdef frozenset _ext_all_possible_keys(self)
     cpdef _ext_setattr(self, ext_self, k, value)
     cpdef _ext_getattr(self, ext_self, k, default=*)
     cpdef _ext_replacement_getattr(self, k, default=*)
@@ -63,6 +63,10 @@ cdef class _ExternalizableInstanceDict(AbstractDynamicObjectIO):
     cdef context
 
     cpdef _ext_accept_update_key(self, k, ext_self, ext_keys)
+    @cython.locals(
+        ext_dict=dict,
+    )
+    cpdef frozenset _ext_all_possible_keys(self)
 
 cdef class InterfaceObjectIO(AbstractDynamicObjectIO):
     cdef readonly _ext_self

src/nti/externalization/datastructures.py

@@ -356,7 +356,29 @@ def _ext_replacement(self):
         return self.context
 
     def _ext_all_possible_keys(self):
-        return frozenset(self._ext_replacement().__dict__.keys())
+        # Be sure that this returns native strings, even if the dict
+        # has unicode (Python 2) or bytes (Python 3) values.
+        # Because we are likely to turn around and pass the strings
+        # we return here to _ext_getattr(), the best solution is to actually
+        # fix the dict if we find any broken attributes; Python 3 would fail
+        # if we encode a bytes value in the dict and then ask for it by string.
+        ext_self = self._ext_replacement()
+        ext_dict = ext_self.__dict__
+        # Do our best to avoid copying in the common case that no
+        # fixup is needed
+        for key in ext_dict:
+            if not isinstance(key, str):
+                # fixup
+                if hasattr(ext_self, '_p_changed'):
+                    ext_self._p_changed = 1
+                for k in list(ext_dict):
+                    if not isinstance(k, str):
+                        new_k = k.encode('ascii') if not isinstance(k, bytes) else k.decode('ascii')
+                        val = ext_dict.pop(k)
+                        ext_dict[new_k] = val
+
+                break
+        return frozenset(ext_dict)
 
     def _ext_getattr(self, ext_self, k, default=NotGiven):
         if default is NotGiven:
@@ -583,7 +605,6 @@ def _ext_accept_external_id(self, ext_self, parsed):
         of ``__external_accept_id__`` on the ``id`` field, then
         this will return that value; otherwise, returns false.
         """
-        __traceback_info__ = ext_self, parsed,
         cache = cache_for(self, ext_self)
         if cache.ext_accept_external_id is None:
             try:

src/nti/externalization/tests/test_datastructures.py

@@ -673,3 +673,39 @@ class DictSubclass(dict):
         io.updateFromExternalObject(d)
         assert_that(d2, is_({}))
         assert_that(d2, has_property('a', 'b'))
+
+
+    def test_unicode_strs_in_dict(self):
+        # Seen in the wild with legacy data.
+        # The unicode path is bad on Python 2,
+        # the bytes path is bad on Python 3.
+        from persistent import Persistent
+        class MappingIO(self._getTargetClass(), Persistent):
+            pass
+
+        m = MappingIO()
+        m.__dict__[u'bad_u_key'] = 'bad_value'
+        m.__dict__[b'bad_b_key'] = 'bad_value'
+
+        class Jar(object):
+            def register(self, obj):
+                "Needed for IJar interface"
+
+        # p_changed can only be set if we have a jar
+        m._p_jar = Jar()
+
+        assert_that(m, has_property('_p_changed', False))
+        ext = m.toExternalObject()
+
+        assert_that(ext, is_({
+            'Class': 'MappingIO',
+            'bad_u_key': 'bad_value',
+            'bad_b_key': 'bad_value',
+        }))
+
+        # Updated in place
+        for k in m.__dict__:
+            assert_that(k, is_(str))
+
+        # And marked as changed
+        assert_that(m, has_property('_p_changed', True))