Merge lp:~allenap/storm/json-variable-unicode-bug-846867 into lp:storm

[Gavin Panella]

JSON properties now must back onto a unicode text column rather than a byte column.

Bug 846867 and bug 845904 have the background to this change. In essence, this ensures that JSONVariable always produces unicode strings at the right points, so that the JSON property can/should be used with a text column in the database rather than a byte column.

It also fixes a tiny bug in zstorm.py where tests are not skipped when the transaction module is not available but zope.component is.

[Jamu Kakar (jkakar)]

Nice one, +1!

review: Approve

[Stuart Bishop (stub)]

As discussed, we should drop the _loads() change if it isn't absolutely necessary. If we are getting byte strings passed into _loads(), we need to know where they are coming from to determine the correct encoding rather than assume UTF-8. Assuming UTF-8 could cause data loss in some situations. I think that with the _dumps() change we will never see byte strings in _loads(), so replacing the str -> unicode decode with an assert seems appropriate.

review: Needs Fixing

[lp:~allenap/storm/json-variable-unicode-bug-846867]

405. By Gavin Panella on 2011-09-13

Complain loudly in JSONVariable._loads() when given a byte string, instead of trying to guess the encoding.

406. By Gavin Panella on 2011-09-13

Switch from assert to TypeError to be consistent with other parts of Storm.

[Gavin Panella (allenap)]

I've changed _loads() to raise a TypeError (suggested by Jamu as more
consistent with the rest of Storm than an assert).

[Stuart Bishop (stub)]

review: Approve

=== modified file 'storm/variables.py'
--- storm/variables.py	2011-09-07 14:04:04 +0000
+++ storm/variables.py	2011-09-13 10:28:34 +0000
@@ -633,10 +633,22 @@
         super(JSONVariable, self).__init__(*args, **kwargs)
 
     def _loads(self, value):
+        if not isinstance(value, unicode):
+            raise TypeError(
+                "Cannot safely assume encoding of byte string %r." % value)
         return json.loads(value)
 
     def _dumps(self, value):
-        return json.dumps(value)
+        # http://www.ietf.org/rfc/rfc4627.txt states that JSON is text-based
+        # and so we treat it as such here. In other words, this method returns
+        # unicode and never str.
+        dump = json.dumps(value, ensure_ascii=False)
+        if not isinstance(dump, unicode):
+            # json.dumps() does not always return unicode. See
+            # http://code.google.com/p/simplejson/issues/detail?id=40 for one
+            # of many discussions of str/unicode handling in simplejson.
+            dump = dump.decode("utf-8")
+        return dump
 
 
 class ListVariable(MutableValueVariable):

=== modified file 'tests/variables.py'
--- tests/variables.py	2011-09-07 14:04:04 +0000
+++ tests/variables.py	2011-09-13 10:28:34 +0000
@@ -833,7 +833,7 @@
 
     def test_get_set(self):
         d = {"a": 1}
-        d_dump = self.encoding.dumps(d, -1)
+        d_dump = self.encode(d)
 
         variable = self.variable_type()
 
@@ -893,18 +893,31 @@
 
 class PickleVariableTest(EncodedValueVariableTestMixin, TestHelper):
 
-    encoding = pickle
+    encode = staticmethod(lambda data: pickle.dumps(data, -1))
     variable_type = PickleVariable
 
 
 class JSONVariableTest(EncodedValueVariableTestMixin, TestHelper):
 
-    encoding = json
+    encode = staticmethod(lambda data: json.dumps(data).decode("utf-8"))
     variable_type = JSONVariable
 
     def is_supported(self):
         return json is not None
 
+    def test_unicode_from_db_required(self):
+        # JSONVariable._loads() complains loudly if it does not receive a
+        # unicode string because it has no way of knowing its encoding.
+        variable = self.variable_type()
+        self.assertRaises(TypeError, variable.set, '"abc"', from_db=True)
+
+    def test_unicode_to_db(self):
+        # JSONVariable._dumps() works around unicode/str handling issues in
+        # simplejson/json.
+        variable = self.variable_type()
+        variable.set({u"a": 1})
+        self.assertIsInstance(variable.get(to_db=True), unicode)
+
 
 class ListVariableTest(TestHelper):
 

=== modified file 'tests/zope/zstorm.py'
--- tests/zope/zstorm.py	2010-07-01 17:47:01 +0000
+++ tests/zope/zstorm.py	2011-09-13 10:28:34 +0000
@@ -266,7 +266,7 @@
 class ZStormUtilityTest(TestHelper):
 
     def is_supported(self):
-        return has_zope_component
+        return has_transaction and has_zope_component
 
     def test_utility(self):
         provideUtility(ZStorm())